Suggestion: PG::Connection#complile

[intale]

Introduce PG::Connection#complile that take sql string and position parameters and compiles it into a plain sql. I couldn't find another way how to do it using built-in tools. The motivation behind this implementation:

• debugging sql queries. When you have queries with dozens(or even hundreds) of positional parameters - it is pretty cancer to replace each positional param by hands for later inspection
• such feature as PostreSQL view does not support positional parameters and requires pre-computed sql string. So, imaging you have some sql builder in your app that results in sql with positional params. And now you need to work with PostgreSQL view which does not support them. You will end up implementing very similar to the changes in this PR or somehow compute an sql string based on your app's logic. Both variants would be annoying.

The downside of this implementation is that it can't reliably encode binary data into a string representation except some common cases like with booleans, but do we need them at all?

[larskanis]

That sounds like a great idea! It's because I manually inserted too many parameters into too many queries from rails debug log in order to execute it in psql. That way a more useful debug output in rails would be possible and that would be my intention.

[larskanis]

I added a commit to use generic PG::TypeMap methods only and regarding binary data and binary format.

The method name "compile" is not very specific. Is it used in any other database driver? How sounds embed_params instead?

[intale]

@larskanis thank you. Could you please explain how your changes apply to PG::TypeMapAllStrings as well? Because it has its own interface, all static, not exposed to public and PG::TypeMapAllStrings the default class for type mapping. Nvm, I'm blind. pg_tmas_type is inherited from pg_typemap_type and you added changes exactly into pg_typemap_type. I mixed it with PG::TypeMapByClass. Lgtm

[intale]

The method name "compile" is not very specific. Is it used in any other database driver? How sounds embed_params instead?

I don't have any strong preferences regarding this. embed_params works as well

[larskanis]

🎉 With the latest change the whole activerecord test suite runs through with only 2 expected errors, when I change the following AR lines to got through embed_params and sending the resulting SQL text without binds:
https://github.com/rails/rails/blob/09bf0878ec2a604f4f1430c8757db23d16925f14/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb#L162-L184

[intale]

@larskanis thank you for your effort and covering cases I was not aware of.

[cbandy]

IME, these kinds of not-quite-parsers are vulnerable to SQL injection.

Perhaps a stronger or more specific statement about how this should/not be used with user input.

[intale]

I agree. If there is any mistake in regexp - the result may end up containing sql injection. For example

embed_params("select '$1' as one, $1", [%{', 2 as two, }])

may result in

[{"?column?" => "', 2 as two, ", "two" => "2", "one" => ""}]

instead this

[{"one" => "$1", "?column?" => "', 2 as two, "}]

if we forget to cover regular string literal(e.g. 'my str') in PLACEHOLDER_RE. Thus, as long as we cover all possible variations of string literals and escape the value - we are safe. Fortunately I covered all possible scenarios in tests. As an additional measure - I can put here a guard that fails on new postgresql versions and a dev should check if any new string literals appear in a new postgresql version and adjust the implementation accordingly. Something like this(in #embed_params_and_check):

# Query returns a string like "18.4 (Ubuntu 18.4-0ubuntu0.26.04.1)"
current_pg_version = conn.exec('SHOW server_version').first['server_version'].split(' ').first
current_pg_version = Gem::Version.new(current_pg_version)
expect(current_pg_version).to be < Gem::Version.new('19'), "PostgreSQL 19 is not tested. Please ensure there are no new forms of a string literal and increase the version to make this test pass."

What do you think, guys?

[larskanis]

Yes, maybe we should make a stronger statement about security. Putting SQL and user data together into one string is always more risky than keeping them separate.

On the other hand to exploit a incomplete or broken regex it's necessary to have a strange SQL placeholder string. But this string must not contain any unchecked user inputs anyway, so cannot be manipulated. And the params items go through the escape or escape_bytea methods provided by libpq, so that they should be safe.

So I think it's not necessary to put a guard for new PostgreSQL versions. But maybe someone speaking native English can improve my wording or make it more specific.

[intale]

Unfortunately #escape does not fully protect us from sql injections. This is because we have to support every possible string literals here, but #escape only ensures there is correct amount of ' in the string. Thus, if some regexp contains a mistake - an injection becomes possible for other string literals. In the example bellow you can see an injection using $$ in case this variant is not handled properly(you can simply delete that variant to reproduce the issue):

sql = "select $$ $1 as t, $$ as one, $1"
params = ["$$ as tt, $$"]
@conn.exec(@conn.embed_params(sql, params)).to_a
# => [{"tt" => " '", "one" => "' as t, ", "?column?" => "$$ as tt, $$"}]
@conn.exec_params(sql, params).to_a
# [{"one" => " $1 as t, ", "?column?" => "$$ as tt, $$"}]

Thus, such issue might appear because we basically replace all appearances of positional variable in the string thanks to gsub and incorrect/absent regexp. This is what I meant in my previous message when I was suggesting to add another guard against new potential string literals in future PostgreSQL versions.

[larskanis]

@intale I understand your concern. But what I mean is that the SQL string must be manipulated, in order to exploit a flaw in the regex, which should not be possible for an attacker. An attacker can change the params, but he cannot exploit a flaw in the regex.

[intale]

Thanks for explanation. That is true. Let me rephrase my concern. My intention of adding a guard is to prevent next case:

• current implementation covers all cases for current PostreSQL
• PosteeSQL v25 introduces new string literal. Let's say %% my str %%
• the implementation suddenly becomes vulnerable to sql injections if new string literal is used because it misses a regexp to handle it

But if we would have a guard that demands manual check of new PostreSQL version support before succeeding the build - we would be more safe than without it.