Installer should properly encode database usernames and passwords to prevent special characters from breaking the install

[jlfranklin]

One of a few bugs off of #2231.

This issue addresses the immediate issue of parse_url() not handling special characters by encoding them.

[jlfranklin]

Needs tests, but the code itself is ready for review.

[olafgrabienski]

Thanks for the PR! I set up a test site with the code changes and was able to install Backdrop although I used three special characters (forward slash, semicolon, and hash sign) in the database password which would formerly have led to a failed installation.

So, looks good to me, but we still need someone who can make a code review.

[quicksketch]

Thanks @jlfranklin and @olafgrabienski! This looks great. Merged backdrop/backdrop#2034 in to 1.x and 1.8.x.

[Graham-72]

I have experienced a problem with upgrading a site from 1.8.0 to 1.8.1, 1.8.2 and 1.9.0 because my database password contained a % character. This caused the system to crash with the error message
PDOException: SQLSTATE[HY000] [1045] Access denied for user 'just42bd'@'localhost' (using password: YES) in lock_may_be_available() (line 167 of /var/www/vhosts/just42.org.uk/backdrop.just42.org.uk/core/includes/lock.inc).
The updates worked after changing the database password.

[jlfranklin]

Just a % character or was it a % followed by two [0-9a-f]?

Obviously, we can't add an update hook to look for them, the (now) bad password would take the site offline before we could run. We could trap the exception early on in Bootstrap, and add an error message that point to a change record / FAQ entry about the URL encoding.

[Graham-72]

For the record, the password I was using was Xtz4e%92.

[jlfranklin]

I suppose the other option is to try the password both encoded and unencoded.

[quicksketch]

Reopening this to investigate options.

[quicksketch]

Hm, so it turns out that specifically the % followed by two numbers is the only situation where this occurs. A percent symbol followed by a number. So a password like Pass%ord won't have any problem, but a password like Pass%1ad would get modified.

@Graham-72's password of course also met these conditions, resulting in a modified password.

For solutions, here are some thoughts:

• Try both passwords like @jlfranklin suggested.
• Perhaps in addition to that, make a mention in status report that the database credentials should be updated to be encoded?
• Try to automatically fix the settings.php file (update.php somehow?)

[quicksketch]

Here's an initial attempt at trying both passwords. It seems to solve the problem when using a password such as the one suggested by @Graham-72.

backdrop/backdrop#2086

I'm not satisfied with this yet though. It really needs some kind of warning in the status report that indicates your settings.php values should be updated.

[laryn]

Not sure if this is the right issue but it seems maybe so -- reported in the forum recently:

The error I'd had before involved non alphabetic/numeric character in mysql database password: ok in Drupal, not Backdrop it seems. Maybe the documentation could indicate this?

[olafgrabienski]

Not sure if this is the right issue but it seems maybe so -- reported in the forum recently ...

I think it is the right issue. @quicksketch and @jlfranklin, friendly reminder: What is missing to fix the issue?

[klonos]

What is missing to fix the issue?

What @quicksketch said in #2904 (comment) seems to be a technical direction. We just need a PR that implements it I guess.

[klonos]

...there seems to be a PR available in backdrop/backdrop#2086, but @quicksketch said he's not very happy with it (although it seems to get the job done). Perhaps we should reconsider this and merge this working solution, along with a @todo comment, and a follow-up issue to revisit.

[jenlampton]

Perhaps we should reconsider this and merge this working solution, along with a @todo comment, and a follow-up issue to revisit.

When a core committer rejects something, it means that the code is not suitable for core inclusion as it is, and should not be merged. Adding @todo's to fix later only ends up with a codebase full of todos. :)

Perhaps we can find a compromise of something that will make the committer happy, but is also quick & easy to achieve?

[indigoxela]

FWIW, I also have a problem with the PRs current approach - by wrapping the fallback in try/catch, it always lets the connection run into an exception initially - on every database connect for affected credentials. Hopefully there's a smarter way to catch the problem.

[yorkshire-pudding]

I can't quite understand what this is stalled on. For me, having special characters in db passwords is an essential part of site security.

[olafgrabienski]

@yorkshire-pudding I'd also love to see this fixed. There seem to be issues regarding the approach, see #2904 (comment) by @quicksketch and #2904 (comment) by @indigoxela.

[klonos]

If I read everything correctly from past comments here and in #2231, then this seems to be happening when there's a special set/sequence of characters being used as username/password or db name (right?). That "problematic" sequence is % followed by any 2 characters in the 0-9 or a-f range. So the regex to catch that would be %[0-9a-f]{2}. Can we then just check for that input before trying the connection, and if found, then throw a warning?

Also, parse_url() doesn't seem to cut it in these cases, so perhaps use something custom instead of that? 🤔

This seems challenging enough, yet possibly solvable relatively easily. I'd like to take a crack at it 🙂

[klonos]

FYI, PHP handled this better up to 7.0.12 and 5.6.27.

https://3v4l.org/Urngf

Originally posted by @jlfranklin in #2231 (comment)

I've done quite some testing on my local, and this is so true 😞 ...I've created a custom backdrop_settings_parse_database_url() function, which accepts the entire database string from settings.php. In the initial implementation of this function, I was trying to use parse_url() if it came back with results (in order to retain the current functionality) - if parse_url() was unsuccessful, the function then did a custom parsing of the string. Here's what that function looked like:

function backdrop_settings_parse_database_url($database_url) {
  // Perform a very basic sanity check based on the expected format of the URL.
  $is_valid_url = is_string($database_url) && preg_match('#^mysql:\/\/.+:.+@.+\/.+$#', $database_url);

  // Attempt to parse the various URL parts using parse_url().
  $database_parts = parse_url($database_url);

  // If parse_url() returned an array, then use that.
  if (is_array($database_parts)) {
    // Remove leading slash from database name.
    $database_parts['path'] = substr($database_parts['path'], 1);
  }
  // When parse_url() encounters certain "problematic" characters (such as /, #,
  // and ;) in the username or password, it may return FALSE in some PHP
  // versions. That doesn't mean that the URL provided is incorrect - we use
  // custom parsing logic in that case.
  elseif (is_bool($database_parts) && $is_valid_url) {
    [custom logic using preg_match() goes here]
    ...
    $database_parts = ...;
  }

  return $database_parts;
}

Unfortunately, parse_url() is so NOT the right tool for this task here. It is so unreliable, that when using p@s5/w#rd as password for example, you get the following parts:

parse_url('mysql://test_user:p@s5/w#rd@localhost:3306/test_database');

Result:

array(
  'scheme' => 'mysql'
  'host' => 's5'
  'user' => 'test_user'
  'pass' => 'p'
  'path' => '/w'
  'fragment' => 'rd@localhost:3306/test_database'
)

See it in action: https://3v4l.org/j3Ygt

This issue addresses the immediate issue of parse_url() not handling special characters by encoding them.

Right, so that was a good effort @jlfranklin, but even with the change that went in with backdrop/backdrop@1f7d2df where we now use rawurldecode() on the various potentially problematic elements of the $database_parts array, such as username and password, this will have absolutely no effect, as the parts provided by parse_url() are totally wrong to begin with.

No matter how much we want to avoid this and use native PHP functions as much as possible, I believe that the only solution here is to create our own function, which should live in core/includes/bootstrap.inc and be specific to only parsing the simple db URL from settings.php. That function is what we should then be calling in backdrop_settings_initialize() (instead of parse_url()) and we should be using the custom regex to parse the db URL in all cases - we should never rely on parse_url().

I'm pretty close to a regex that gets the job done, and I have tested various potentially problematic passwords, but as it often is the case with regex's, there might be something that I might have missed. So I will provide that to everyone subscribed in this thread, and you please do your best to break it then 😉 ...provide feedback, and I will then tweak the regex to account for any of the reported edge cases.

[klonos]

A couple of questions:

1. What's the point of specifying the scheme/driver, if it's always gonna be mysql for Backdrop? Asking because I need to know whether the regex should be trying to parse that part (like preg_match('#^(?<driver>.+):\/\/...), or if it should be hard-coded instead (like preg_match('#^mysql:\/\/...)?
2. I have seen cases where the root user for MySQL has a blank password OOTB, so a password-less connection URL makes sense (mysql://root@localhost/database or mysql://root@localhost:1234/database if non-default port), but the code in backdrop_settings_initialize() seems to imply that a connection is possible without even a username. Is that really possible in MySQL/MariaDB? ...in other words, is this an acceptable and possible scenario?:

   $database = 'mysql://localhost/database_name';

[yorkshire-pudding]

@klonos - I can't see the point of having to include the scheme/driver either.
We tackled this issue in bee by splitting up the parts as, like you, we found that parse_url is really no good for the kind of database passwords that people should be using for security.
I look forward to seeing your regex magic work on this; regex is still a bit of a mystery to me.

[yorkshire-pudding]

This may not be required if the simplified array in #2231 is introduced.

[yorkshire-pudding]

I think this is now fixed resolved by backdrop/backdrop#4567 which has been merged and will be in Backdrop 1.30.0
@quicksketch @olafgrabienski @jlfranklin @Graham-72 @klonos @jenlampton @laryn @indigoxela
Do you agree?

[indigoxela]

I think this is now fixed by backdrop/backdrop#4567 which has been merged and will be in Backdrop 1.30.0

It's a different approach, actually. But IMO a more robust one. Instead of trying to fix the problems with urlencoding and decoding - which turned out to be quite tricky, provide a default db settings approach, which completely avoids encoding/decoding.

[olafgrabienski]

@yorkshire-pudding I also see this issue here resolved by the mentioned PR which fixes issue #2231. Thanks for all your work there, btw!

[yorkshire-pudding]

Resolved in a different way by backdrop/backdrop#4567