October: Installer passwords fail with ] and ) chars

[benjamw]

I tried to install October on my server and got an error that was quite puzzling. It said there was an error in the eval code.

Parse error: syntax error, unexpected 'Pt' (T_STRING), expecting ']'" 
on line 119 of .../install_files/php/InstallerRewrite.php(66) : eval()'d code

I began to dig into the issue and discovered that the InstallRewriter class was using regular expressions to insert data into the config files so the comments and formatting remain the same. The trouble comes from one particular regular expression that gets created when replacing the mysql password and particular _valid_ passwords.

The pattern in question is the third of the password patterns (idx-18):

(['|"]connections['|"]\s*=>\s*(?:[aA][rR]{2}[aA][yY]\(|[\[])[\s\S]*['|"]mysql['|"]\s*=>\s*(?:[aA][rR]{2}[aA][yY]\(|[\[])[\s\S]*?)(['|"]password['|"]\s*=>\s*)([tT][rR][uU][eE]|[fF][aA][lL][sS][eE]|[nN][uU][lL]{2}|[\d]+)

And the replacement value (the password has been changed, but important characters remain the same):

${1}${2}'h6{!G4sIq91)Pt'

Because this third regex matches most of the file, all the way down to the redis section of the config, it matches with the redis portion that reads 'password' => null, and replaces that password with the one above, leaving 'password' => 'h6{!G4sIq91)Pt',

Not only is this incorrect, but when the fourth redis pattern (idx-28) tries to match, to replace the redis config data with 'mysql', instead of matching the entire redis default array, it finishes the match inside the password field which contains a closing paren. This causes the replacement to end up looking like:

    'redis' => [

        'cluster' => false,

        'default' => 'mysql'Pt',  // <--- this line is invalid PHP code
            'port'     => 6379,
            'database' => 0,
        ],

    ],

This in turn causes the eval to fail, and subsequently, the config file to be left unchanged, and everything after this point in the installation fails.

Expected behavior

The third password regex to prevent matching the entire file, and the fourth redis pattern to match the entire redis section regardless of password value contents.

Actual behavior

Installation failure

Reproduce steps

Try to install and enter a mysql password that contains a closing paren ) with some text after it.

October build

Downloaded today, not sure. EDIT: 365

[benjamw]

Short term solution, I will change my password to not contain any closing parens ) or square braces ]. Long term solution...

[daftspunk]

Thanks for reporting this. Perhaps we need to escape the password for regex?

[benjamw]

It's not the password regex that's the problem. The password gets inserted just fine (although it should probably still be escaped for the regex to prevent other issues). The problem comes after the password has been replaced into the file in an incorrect position (because that particular password regex matches the whole file and replaces a portion of the redis section with the mysql password), and a subsequent regex ends prematurely because the password contains characters that subsequent redis regexes are looking for.

I'm sure trying to match every conceivable variation of PHP array syntax is cumbersome and ultimately where the problem lies, and I can't think of a valid solution to that at the moment because I'm not sure of the original intent of the regex.

[daftspunk]

Path of least resistance would be to block the troublesome characters, or use a "bait and switch" style solution (replace bad chars with rare safe chars, swap back when process completes). Alternatively use a more basic solution similar to the Key Generate command, although it is unlikely this will work due to nesting.

Replication instructions

1. Download web installer and proceed with steps
2. Select MySQL as the database driver
3. Enter the password h6{!G4sIq91)Pt for the database
4. At some stage an error will occur (needs confirmation)

Expected exception

Parse error: syntax error, unexpected 'Pt' (T_STRING), expecting ']'"
on line 119 of .../install_files/php/InstallerRewrite.php(66) : eval()'d code

[LukeTowers]

The support demand for October is increasing, while the available time the founders have to give remains the same. These issues will be handled in time, however there are ways you can help:

• Submit a pull request that introduces this feature and wait for it to be reviewed
• Make a financial contribution to the project so we can allocate more time towards making improvements like these that benefit everyone

[Samuell1]

Temporary fix for this can done with pattern html5 attribute. pattern="[A-z0-9À-ž\s]*".

What you think @LukeTowers

[LukeTowers]

@Samuell1 not a fan tbh, people should be able to use special characters in their credentials.

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.

[bennothommo]

Probably still needs to be looked at. I agree with @LukeTowers regarding the special characters.

[benjamw]

This is just my opinion so take this as you may...
I think that, while the installation method is clever and interesting, cleaver and interesting are not good bed fellows with programming.

I appreciate the desire to keep the config files as clean as possible, retaining the comments and leaving everything as it is, but it seems to not be working out as well as can be hoped. Maybe a secondary config file needs to be generated without all the regex nonsense and just output it directly, while leaving the original config file there as a reference or default.

Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two problems.

• Jamie Zawinski

[Samuell1]

Installer should move to save config values to ENV and there will be no issues with regex etc.

[bennothommo]

The installer is precisely for people who don't want to deal with environment variables ;)

[Samuell1]

Yes, but thats what installer should do to setup env file for you and not to edit config files :D

[LukeTowers]

@Samuell1 I would agree with you, and would take it a step further by having the installer run october:env, however the installer is meant for non-technical people, and .env is a hidden file which represents a potentially really tricky problem for non-technical people to solve when copying an "installed" instance of October with hidden files disabled (as they are by default on most popular operating systems) which would fail to copy over the hidden .env file.

While solving the config writer problem is definitely more tricky, I think it's worth it and it's just what we have to do.

[Samuell1]

Then maybe move core configs to ENV and installer still can replace that envs etc. Even some of configs have env variables in ocms and with laravel 6.0 i like to see config using env variables. (I did update of configs few months ago to have all ENVs that are needed https://github.com/Samuell1/october/commit/7598cfae9296ea014e4d0c6640c2d8b2cd793eff)

Like you install october trough composer you should get env variables predefined in configs. And maybe have option for installer too to save values to env for more tech people.

[bennothommo]

@Samuell1 As above though, people who don't want to deal with environment variables will likely not want to deal with Composer either :stuck_out_tongue:.

[Samuell1]

@bennothommo Yeah i know but iam saying that env variables should be included with installation trough composer because its for more tech people :D

[bennothommo]

@Samuell1 Yeah that's fair. That's a separate issue to this though :)

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.

[daftspunk]

The problem comes after the password has been replaced into the file in an incorrect position (because that particular password regex matches the whole file and replaces a portion of the redis section with the mysql password), and a subsequent regex ends prematurely because the password contains characters that subsequent redis regexes are looking for.

I'm unsure why this was reopened, the OP issue should have been fixed already

• https://github.com/octobercms/install/pull/87
• https://github.com/octobercms/library/commit/d51e539614068ba9f55910f55eea2f68aa02a4bf

If there is a new issue, please create a new issue

Tests pass

/*
 * Handle special chars
 */
$contents = $writer->toContent($contents, ['default' => 'h6{!G4sIq91)Pt']);
$result = eval('?>'.$contents);

$this->assertArrayHasKey('default', $result);
$this->assertEquals('h6{!G4sIq91)Pt', $result['default']);

$contents = $writer->toContent($contents, ['connections.mysql.password' => 'h[6{!G4sIq91)Pt']);
$result = eval('?>'.$contents);

$this->assertArrayHasKey('connections', $result);
$this->assertArrayHasKey('mysql', $result['connections']);
$this->assertArrayHasKey('host', $result['connections']['mysql']);
$this->assertEquals('h[6{!G4sIq91)Pt', $result['connections']['mysql']['password']);