Bug: double prefixing with db_prefix array

[dreaming-augustin]

Hello,

This commit:
078a90b
introduced a critical bug affecting sites where $db_prefix (set in settings.php) is an array.

For example:

    $db_prefix = array(
      'default'   => 'default_',
      'users'     => 'www_',
      'sessions'  => 'www_',
      'mytable'  => 'custom_',
    );     

In function db_prefix_tables($sql) (includes/database.inc):

    if (array_key_exists('default', $db_prefix)) {
      $tmp = $db_prefix;
      unset($tmp['default']);

      foreach ($tmp as $key => $val) {
        $sql = strtr($sql, array('{' . $key . '}' => '{' . $val . $key . '}'));
      }
      $sql = strtr($sql, array('{' => '{' . $db_prefix['default']));
    }

The above code will turn {mytable} to default_custom_mytable instead of the expected custom_mytable.

The critical line is this one:

$sql = strtr($sql, array('{' . $key . '}' => '{' . $val . $key . '}'));

which introduces back curly brackets, which triggers the next strstr() command to add the default prefix on top!

The fix is to revert that line to what it was before:

$sql = strtr($sql, array('{' . $key . '}' => $val . $key ));

I don't know what motivated changing that particular line, but looking at the whole commit, it does not seem necessary to change this line to achieve the intended overall purpose, which was to add backquotes for MySQL 8 compatibility purposes.

[ylp-ineris]

Hello,

The same problem occured on a website I maintain during an update. Sharing the user table, the impact is important as nobody can authenticate after the update.

As this kind of settings (mutliple prefix) is not so often met, I understand it's just a regression introduced while making the CMS compatible with MySQL 8+ which ask to escape its keywords.

Here is a patch I developed and that works for the website I maintain:
double_table_name_prefixing_fix.zip

[millenniumtree]

Now it's removing pipes from some string arguments in a query of mine. Not the best way to solve this. ;D

I've replaced the lower part of the function with the following, which I believe fixes the original issue, while not breaking pipes in the rest of the query.

  // Add prefixes and transform curly braces into brace/pipes in order to mark already done operations
  // while preserving table name encapsulation for the next step.
  foreach ($db_prefix_local as $table_name => $prefix) {
    $sql = strtr($sql, array('{' . $table_name . '}' => '{|' . $prefix . $table_name . '|}'));
  }
  $sql = strtr($sql, array('{' => '{|' . $default_replacement, '}' => '|}'));

  // For MySQL 8, escape table names corresponding to reserved keywords.
  if (db_version_compare('mysql', '8.0.0')) {
    foreach ($db_mysql8_reserved_keywords as $keyword) {
      $sql = str_replace('{|' . $keyword . '|}', '`' . $keyword . '`', $sql);
    }
  }

  // Remove curly brace/pipes.
  return strtr($sql, array('{|' => '', '|}' => ''));

[ylp-ineris]

Sorry for the regression.

For the fix, something totally different from/not including curly braces must be used as a replacement. If no, the second strtr will also modify previously modified curly braces.

Maybe triple pipes instead of single pipes? A quick search let me think it's not used in SQL (while double pipes may).
Other possibilities:

1. find another good character to replace the single pipes
2. (use regexp to match well the strings to replace - too heavy for that treatment, isn't it?)
3. introduce a first strtr to double curly braces and then, reduce them to single curly braces in the other calls to strtr

[ylp-ineris]

Fix tried with the third solution proposed:

  // Double curly braces in order to be able to mark already done operations
  // while preserving table name encapsulation for the next step,
  // adding prefixes and then, escaping MySQL 8 reserved keywords.
  $sql = strtr($sql, array('{' => '{{' , '}' => '}}'));

  // Add prefixes.
  foreach ($db_prefix_local as $table_name => $prefix) {
    $sql = strtr($sql, array('{{' . $table_name . '}}' => '{' . $prefix . $table_name . '}'));
  }
  $sql = strtr($sql, array('{{' => '{' . $default_replacement, '}}' => '}'));

  // For MySQL 8, escape table names corersponding to reserved keywords.
  if (db_version_compare('mysql', '8.0.0')) {
    foreach ($db_mysql8_reserved_keywords as $keyword) {
      $sql = str_replace('{' . $keyword . '}', '`' . $keyword . '`', $sql);
    }
  }

  // Remove curly braces.
  return str_replace(array('{', '}'), '', $sql);

And the corresponding patch:
db_prefix_tables_fix.zip

Haven't tried it but seems good...

[millenniumtree]

Problem with deleting at the end is that it will remove those characters from parameters in the query. It doesn't do that in the first parts of the function because it's looking for specific table and keywords within braces. The {| |} puts a pipe between the brace and the tables/keywords, and it's less likely for those characters to appear next to each other in real-world data (though still not impossible). The pipes-only replacement was removing pipes from some table keys. I probably should have used tokenized parameters, however. Anything you change it to could theoretically appear in a key or string one day. It's either a matter of making it less likely to appear, or only cleaning up from around known tables/keywords that were processed, such as a regex solution. I can totally see myself using triple-pipes within a key one day. And of course the more complex it gets, the slower EVERY query will become.

…

On Fri, Aug 27, 2021, 03:31 Yannick ***@***.***> wrote: Fix tried with the third solution proposed: ` // Double curly braces in order to be able to mark already done operations // while preserving table name encapsulation for the next step, // adding prefixes and then, escaping MySQL 8 reserved keywords. $sql = strtr($sql, array('{' => '{{' , '}' => '}}')); // Add prefixes. foreach ($db_prefix_local as $table_name => $prefix) { $sql = strtr($sql, array('{{' . $table_name . '}}' => '{' . $prefix . $table_name . '}')); } $sql = strtr($sql, array('{{' => '{' . $default_replacement, '}}' => '}')); // For MySQL 8, escape table names corersponding to reserved keywords. if (db_version_compare('mysql', '8.0.0')) { foreach ($db_mysql8_reserved_keywords as $keyword) { $sql = str_replace('{' . $keyword . '}', '' . $keyword . '', $sql); } } // Remove curly braces. return str_replace(array('{', '}'), '', $sql);` And the corresponding patch: db_prefix_tables_fix.zip <https://github.com/d6lts/drupal/files/7067158/db_prefix_tables_fix.zip> Haven't tried it but seems good... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#52 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA5DAXZ63Y64QEU3MRD7WTT66HTTANCNFSM4OFEY3PA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.