Substituion issue in 2.6.3

[Isakdl]

I'm a maintainer of https://serverpod.dev, and we use this library internally to make SQL queries.

First of I understand that the 3.0 interface has changed substantially, we want to migrate to version 3 but it requires some breaking changes in our API's and we are working on making this transition already. But we would still like to fix this issue in our current live version.

However, we encountered a problem where users supplying a string containing "@sometext:" gets a format exception because the "substitution" string does not contain a type. This happens even though we do not supply any substitutions to the query.

Would it be possible to get a patch for the 2.6 version with a fix for this?

All that would be needed is to not run the substitution logic if no substitutions are supplied. I.e this change in the file lib/src/substituter.dart for method substitute on line 20.

// substituter.dart

static String substitute(String fmtString, Map<String, dynamic>? values,
      {SQLReplaceIdentifierFunction? replace}) {
    if (values == null || values.isEmpty) return fmtString; // <- add this line

    final converter = PostgresTextEncoder();
    values ??= const {};
    replace ??= (spec, index) => converter.convert(values![spec.name]);
  
    ...

[isoos]

Quick question before going into the fix/workaround: is this a possible issue with 3.0 too? Is this string coming from a dynamic builder?

Even though the 2.6 version is no longer developed, this would be a breaking behavior change in terms of not failing on queries where previously we have failed, and some users may rely on it.

[Isakdl]

Not sure if it is an issue in 3.0, as I understand you can create your own substitution format in 3.0 whereas in 2.x you have to use the default one. The string in this case comes from user input and I do not intend to do any substitution on it but the string is interpreted as if we need to run the substitution.

Example:

User supplies the string @Name: Hello world! I then want to store this string in the db as is (escaping it first). But @Name: will cause this lib to throw because it is the template for substitution and expects a type after the : e.g. @Name:int4. I did not give any values to be substituted as I did not want to do any substitutions.

This could be a problem even when you want to use substitution but I think 3.0 should be enough to resolve that problem. The change I'm proposing here is just to ignore substitutions altogether if the programmer did not supply any variables to be substituted. Therefore this wouldn't be a breaking change either as it could only crash on user input that clashes with the substitution syntax.

[isoos]

User supplies the string @name: Hello world! I then want to store this string in the db as is (escaping it first). But @name: will cause this lib to throw because it is the template for substitution and expects a type after the : e.g. @name:int4. I did not give any values to be substituted as I did not want to do any substitutions.

Not sure if I understand this part correctly: do you concatenate and escape the string yourself, instead of relying the client to do it? Why not insert it as any other string, as a parameter, that gets escaped by the client (and be sent over the wire with slightly more efficiency using the binary protocol)?

[Isakdl]

Sorry for the confusion, forget the escaping comment, let me rephrase.

What I'm saying is that @name: Hello world! is the string that should be inserted @name: is part of the raw string that should be saved.

[isoos]

Pseudocode from memory - are you doing this and is it failing?

final input = '@name: Hello world!';
final rs = await conn.execute('INSERT INTO x VALUES (@a)', substitiutionValues: {'a': input});

[Isakdl]

No what I'm doing is this:

final rs = await conn.execute("INSERT INTO x VALUES ('@name: Hello world!')", substitiutionValues: null);

[isoos]

@Isakdl: please don't do that, that's straight up allowing SQL injection attacks, and the reason we want people to use substitutions is to prevent that.

[Isakdl]

Yes hence we are espacing the string manually beforehand of course.

[isoos]

As a summery: the change would allow silent bugs to appear for 2.x users, and it would do so by consciously sidestepping best practices. I don't think we should do it. 3.x allows this to an extent, but now I'm a bit worried about that too.

[Isakdl]

I'm not sure I follow the logic on how this would allow a silent bug or that it could be a breaking change.

To illustrate what I mean look at the following examples:

The following will give no errors and execute without a problem:

final rs = await conn.execute("INSERT INTO x VALUES ('@name Hello world!')", substitutionValues: null);

final rs = await conn.execute("INSERT INTO x VALUES ('@name:int4 Hello world!')", substitutionValues: null);

The library today does not throw where a templated string is used but no substitutions are supplied, this is still allowed in 2.x as a "sidestep" from the substitution interface. Hence there is no real guard against having a template in the query and not using the substitution. If this were a feature I think the above scenarios would have to be covered too. But introducing that would be a breaking change.

I would also like to note that with the proposed change the following code, when a substitution is provided, would still generate the exception as before:

final input = 'John Doe';
final rs = await conn.execute('INSERT INTO x VALUES ('@name: Hello world!')', substitutionValues: {'name': input});

Additionally, a raw query is limited in such a way that an otherwise valid SQL query will throw an error. You are not allowed to have any sequence of characters that matches @<arbitrary-string>: anywhere in the query or it will throw. Even if said string is hardcoded by the programmer.

As an example of this, while unconventional it is allowed to name a table in SQL to "a_table@mytable: stuff".

CREATE TABLE "a_table@mytable: stuff" ("name" TEXT);

SELECT * FROM "a_table@mytable: stuff";

When using any of the above queries, the library will throw the same exception as previously mentioned. Considering the above I still believe this is a bug and that the proposed change is a none breaking fix for it.

[isoos]

The following will give no errors and execute without a problem:
final rs = await conn.execute("INSERT INTO x VALUES ('@name Hello world!')", substitutionValues: null);

Oh, I was not aware of this (most of the v2 code was written before I took over the maintenance). But in that case you could also run these queries with substitutionValues: params.isEmpty ? null : params , without any new release - couldn't you?

Nevertheless, I am now convinced that this may be an acceptable update to the 2.6 branch, and if you are unhappy with the above workaround, I'll release a new version.

[Isakdl]

Oh, I was not aware of this (most of the v2 code was written before I took over the maintenance). But in that case you could also run these queries with substitutionValues: params.isEmpty ? null : params , without any new release - couldn't you?

That is exactly what I wanted to enable with the fix I proposed. Unfortunately, this does not work right now if the query string contains @name:, note that it is only when the string includes the @ and : symbols we currently have a problem.

The reason it doesn't work is this line

static String substitute(String fmtString, Map<String, dynamic>? values,
      {SQLReplaceIdentifierFunction? replace}) {
...
values ??= const {}; // <- changes the substitutionValues from null to an empty map
...
}

Would be very happy with a fix on the 2.6 branch :)

[isoos]

Published as 2.6.4. Hope it works out... feel free to open further issues if you encounter problems during migration to 3.0.0.