[D7] Database test table TEST_UPPERCASE causes PostgreSQL tests to fail

The second commit broke other things; best to revert that. The first got rid of the TEST_UPPERCASE errors; it's now failing with other errors. That could be progress.

I've not yet had a chance to review this thoroughly, but it looks like we're trying to fix the case sensitivity problem by introducing quoting of identifiers (or at least table names) throughout the pgsql driver?

I'm not sure that seems proportionate; there may be good reasons to introduce the quoting (e.g. it was added for MySQL 8 support, but made configurable) but just fixing this one test doesn't seem enough of a benefit to risk the regressions this might cause.

We could skip the problem test for PostgreSQL. Not a perfect solution, but less risky than making much more dramatic changes.

Or perhaps we could rewrite the test so that it doesn't fail for PostgreSQL?

Stating the obvious, it's hard working on PostgreSQL tests as overall the test suite doesn't run.

Was a similar test added to D8? If so, how does the PostreSQL driver handle that? (I've not checked if the D8/9 driver has quoting of identifiers, for example).

The problem is that installTables() creates the tables repeatedly. If a table exists, it delete it first. But db_table_exists(), which uses tableExists(), is case-sensitive. So, it is asked to create TEST_UPPERCASE. It actually creates test_uppercase, and when it runs next time, it says that TEST_UPPERCASE does not exist, so it tried to create test_uppercase again.

There doesn't seem to be a test like this in Drupal 9.3, so this doesn't get caught.

If db_create_table() folds to lower-case, perhaps db_table_exists() should as well. That would solve the immediate problem.

The TEST_UPPERCASE table was introduced as part of testing for SQL injection. Why this is needed is not mentioned in the code comments, commit message, or issue. It is mentioned on the parent issue of which this is a backport: #2492967: SQL layer: $match_operator is vulnerable to injection attack.

Since Postgres won't get uppercase table names anyway, perhaps this test should be skipped on Postgres.

I agree it is heavy-handed to add case-sensitive table name support just to fix this.

Couple of examples where we skip certain parts of database tests for the sqlite driver:

https://git.drupalcode.org/project/drupal/-/blob/7.87/modules/simpletest...

https://git.drupalcode.org/project/drupal/-/blob/7.87/modules/simpletest...

I think we could do something similar for pgsql here. Here's a first pass at doing that.

I'll check into exactly what the uppercase test is all about though; I don't really remember all the details.

If db_create_table() folds to lower-case, perhaps db_table_exists() should as well. That would solve the immediate problem.

...is another option worth exploring.

In the second hunk the operator is backwards, should be ===.
if ($connection['driver'] !== 'pgsql') {

It could also test if the table exists instead of testing for the driver.

Otherwise, I think this will do what we need.

Oh - well spotted, thanks. Friday afternoon copypasta :)

Testing for the existence of the table's a good suggestion too, although perhaps it makes the "skip" less grep-able?

I may be missing something, but I'm not convinced that the UPPERCASE table name is actually significant other than exercising the filtering / validation of the operator a bit more thoroughly.

#2492967-35: SQL layer: $match_operator is vulnerable to injection attack is when it first appeared.

I'm doing a few experiments to see if the UPPERCASE-ness actually makes any difference.

Hmm looks like this test is still in 9.3.x AFAICS:

https://git.drupalcode.org/project/drupal/-/blob/9.3.5/core/tests/Drupal...

I'm not sure yet why it's not causing problems for the PostreSQL driver there; could be a difference in case sensitivity elsewhere like db_create_table() / db_table_exists() (or their equivalents)?

On the basis that the validation just does this:

            // If the operator contains an invalid character, throw an
            // exception to protect against SQL injection attempts.
            if (stripos($condition['operator'], 'UNION') !== FALSE || strpbrk($condition['operator'], '[-\'"();') !== FALSE) {
              throw new InvalidQueryConditionOperatorException('Invalid characters in query operator: ' . $condition['operator']);
            }

https://git.drupalcode.org/project/drupal/-/blob/7.87/includes/database/...

...I still don't see how the uppercase table name is significant.

Testing for the existence of the table's a good suggestion too, although perhaps it makes the "skip" less grep-able?

Yes, if someone is grepping for psql, they won't find this. I was thinking about it sort of like capability testing instead of version testing in JS. I could go either way on this.

As far as I saw, UPPERCASE_TEST is only used for that one SQL injection test.

I'm not sure yet why it's not causing problems for the PostreSQL driver there; could be a difference in case sensitivity elsewhere like db_create_table() / db_table_exists() (or their equivalents)?

The problem isn't the test; it's the creation of the tables with the deletion only being done if the table is found to exist. I could not find that code. The tables are created as part of a module install. That probably only happens once or if it happens more than once, any clean-up is taken care of by other code.

Thanks for the explanations.

If db_create_table() folds to lower-case, perhaps db_table_exists() should as well.

Here's a patch taking that approach, which makes the DatabaseQueryTestCase pass for me locally (with PostgreSQL 9.5).

If this doesn't cause any problems, I think it's better than skipping the test(s).

there's few coding standard fixes required

1. +++ b/includes/database/pgsql/schema.inc
   @@ -614,4 +614,16 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
   +   * Build a condition to match a table name against a standard information_schema.
   

   longer then 80 chars

2. +++ b/includes/database/pgsql/schema.inc
   @@ -614,4 +614,16 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
   +  protected function buildTableNameCondition($table_name, $operator = '=', $add_prefix = TRUE) {
   +    return parent::buildTableNameCondition(strtolower($table_name), $operator, $add_prefix);
   +  }
    }
   

   needs empty line

Thanks for the review @andypost

#18.1 The same one-line comment is in the parent schema.inc and the mysql driver. I'd say that's one for a follow-up.

#18.2 Happy to add the extra line on commit. Thanks!

RTBC, looks good to me and makes sense

Thanks everyone!

With any luck, with this committed we should have tests running again for PostgreSQL (albeit with a few failures).

#710858: Meta issue: fix the remaining PostgreSQL bugs is the parent meta issue to work on those remaining problems.