Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to force use of Extended Queries #3214

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

alxndrsn
Copy link
Contributor

In some circumstances, e.g. gajus/slonik#595, gajus/slonik#601, it can add safety to force use of the Extended Query protocol.

@alxndrsn
Copy link
Contributor Author

@brianc do you have any suggestions where tests could be added for this?

@charmander
Copy link
Collaborator

Can you explain what the safety benefit is? I didn’t see anything stick out in gajus/slonik#595 (especially) or gajus/slonik#601.

@gajus
Copy link
Contributor

gajus commented May 14, 2024

Can you explain what the safety benefit is? I didn’t see anything stick out in gajus/slonik#595 (especially) or gajus/slonik#601.

I believe the benefit is that you wouldn't be able to accidentally execute multiple statements.

@charmander
Copy link
Collaborator

I believe the benefit is that you wouldn't be able to accidentally execute multiple statements.

Right, how is that a benefit? Feels more like a false sense of security.

Looking at gajus/slonik#595 again, I assume there was some user-provided-{"type": "SLONIK_TOKEN_FRAGMENT"}-style vulnerability, but the right place to fix that is there.

@gajus
Copy link
Contributor

gajus commented May 14, 2024

I believe the benefit is that you wouldn't be able to accidentally execute multiple statements.

Right, how is that a benefit? Feels more like a false sense of security.

It provides a guarantee that only a single statement can be executed.

At the moment, no one is stopping someone from doing:

await client.query('SELECT 1; SELECT 2')

... and if that's intentional, that's great. However, if a library is explicitly designed to handle only single statements (such as Slonik), then this behavior is undesirable and can produce unexpected results and/or create security vulnerabilities.

@charmander
Copy link
Collaborator

I don’t think this is necessarily a bad option to add, but it’d be nice to have concrete explanations of the motivations. What security vulnerabilities? Are unexpected results maybe better addressed by checking if the result is an array?

@gajus
Copy link
Contributor

gajus commented May 14, 2024

What security vulnerabilities?

await client.query('SELECT id FROM user_account WHERE id = ' + id);

Suppose someone had a query written this way (obviously, not something they should be doing it), then the ability to execute multiple statements at once is going to allow for a lot more harm than if it wasn't.

Are unexpected results maybe better addressed by checking if the result is an array?

Slonik already does that, but that happens after the query is executed, so it does not entirely prevent the potential harm.

@alxndrsn
Copy link
Contributor Author

Are unexpected results maybe better addressed by checking if the result is an array?

This could work, but only if every query was wrapped in its own transaction. An extra transaction seems like it would be more overhead than forcing Extended Query protocol, but I haven't measured.

@brianc
Copy link
Owner

brianc commented May 15, 2024

Hey @charmander sorry fo the delay here - @alxndrsn contacted me via email about this - we're gonna try to use the security vulnerability section here I just turned on to discuss the details:

image

As far as this PR - looks good to me, but I'd suggest writing a test for it. Something as simple as use forcePreparation: true in a query config & attempt to execute 2 statements as the query text - that should cause postgres to return an error which you can check in the test! 😄 Can also be added to the docs in a separate PR I will do after this one! 👏

@sehrope
Copy link
Contributor

sehrope commented May 15, 2024

+1 to adding something like this and the unintended multi-command execution prevention is a perfect example of why.

The PGJDBC driver has something similar but rather than a boolean, it has a string property to pick which query mode to use. See preferQueryMode here: https://jdbc.postgresql.org/documentation/use/#connection-parameters

Having it be a string rather gives someone in the future the option of forcing a "simple" mode or something else that comes up in the backend server (v.s., being stuck with a bunch of orthogonal boolean flags).

Note that I'm not suggesting adding any handling for a "force simple" mode. Just replacing the boolean flag with queryMode where the only acceptable values are undefined/null or "extended" (but we have the option in the future to expand it).

There's a lot of funky stuff the PGJDBC driver does to try to normalize the query modes. Things like automatically splitting SQL statements separated by a ; and sending them as separate commands. Or inlining parameter values when running in simple mode. All of those are terrible ideas and they should never be added as they lead to things like this: GHSA-24rp-q3w6-vc56

So having separate modes to force the driver to operate in a particular way are fine, but there should never be anything more added as result of enabling them. Just change how things operate on the wire and if someone's app breaks, it's up to them to deal with the ramifications of the mode change.

Also, there are some situations where you can't run in extended mode. IIRC, once you enable wal sender mode for the connection you can only send simple mode commands. It wouldn't be a problem for normal applications as that's an unusual situation, but it is a valid use case for use purely simple mode.

@brianc
Copy link
Owner

brianc commented May 15, 2024

@sehrope all that feedback is awesome - thank you! Yea I kinda like preferQueryMode as well w/ a string - makes it slightly less surprising/confusing since it lines up w/ JDBC...and yeah adding more modes or forcing simple (or pipelining etc) in the future would be easier than just adding a ton of booleans (what even happens if we had forcePrepared and forceSimple both set to true for example?)

Things like automatically splitting SQL statements separated by a ; and sending them as separate commands. Or inlining parameter values when running in simple mode. All of those are terrible ideas and they should never be added

haha yeah I am pretty firm on the "do not modify the users query text input at all under any circumstances, just pass it in and let it fly" - even sometimes I regret the type coercion on the input parameters...but some decisions from 14 years ago are too costly to break and too innocuous to worry about.

So having separate modes to force the driver to operate in a particular way are fine, but there should never be anything more added as result of enabling them. Just change how things operate on the wire and if someone's app breaks, it's up to them to deal with the ramifications of the mode change.

yeah we discussed several different approaches to this & this one is the one we came up with that is 100% backwards compatible and likely the least disruptive. For now we'll keep the query mode as dynamic or default or whatever (empty string) so everything works the same, and opting into preparing all your queries really shouldn't cause you any issues unless you doing complex multi-statement in 1 query calls which I would generally not recommend.

@charmander
Copy link
Collaborator

Are unexpected results maybe better addressed by checking if the result is an array?

This could work, but only if every query was wrapped in its own transaction. An extra transaction seems like it would be more overhead than forcing Extended Query protocol, but I haven't measured.

Right, that suggestion was only in reference to the “correctness” part. Seems like it’s pretty much all about security.

As far as security goes, I’m used to treating any SQL injection as equivalently fatal, but it looks like this might actually be a useful mitigation in the described situation. (Not that my not being able to find any way to escape the SELECT context with a quick look means that there necessarily isn’t one – but it’d be nice if there weren’t! That said, being able to get the results of any arbitrary read-only query like that example allows is still pretty bad.)

Anyway, yeah, this seems good to add as an option with a nice, descriptive enumerated value.

@brianc
Copy link
Owner

brianc commented May 15, 2024

Anyway, yeah, this seems good to add as an option with a nice, descriptive enumerated value.

Nice! Thanks @charmander - tomorrow I'll do either an addon to this PR or replacement one to add the string based option. The original author is out and AFK for a couple days, so I'll just drive it home w/ documentation & tests as well.

@alxndrsn
Copy link
Contributor Author

The original author is out and AFK for a couple days

Found a little time (¬:

I've updated the PR with a test, and to change the boolean to a string, but the test is currently failing with the native bindings.

@brianc
Copy link
Owner

brianc commented May 16, 2024

but the test is currently failing with the native bindings.

I can pull down your branch & take a look at it here a bit l8r today. Weird its not throwing an error.

@boromisp
Copy link
Contributor

but the test is currently failing with the native bindings.

I can pull down your branch & take a look at it here a bit l8r today. Weird its not throwing an error.

Could this be the cause?
https://github.com/brianc/node-pg-native/blob/27ebb534b3e0532160c4fb3b822431f5ed783a80/index.js#L60

@brianc
Copy link
Owner

brianc commented May 16, 2024

Could this be the cause?

yah that's it - should be an easy fix, I'll just need to do a PR and minor version bump over there. I should probably pull that lib into this monorepo as it rarely (never?) changes by itself.

@gajus
Copy link
Contributor

gajus commented May 29, 2024

Is there a chance of this getting released without waiting until pg-native is moved to this repo?

@brianc
Copy link
Owner

brianc commented May 30, 2024

gonna move pg-native over here today if i can swing it...working on it now. had to upgrade some stuff in node-libpq for it to build on my machine (node v22)

@brianc brianc merged commit ff47a97 into brianc:master Jun 4, 2024
5 checks passed
@brianc
Copy link
Owner

brianc commented Jun 4, 2024

Heyo! Got it merged - will do a release here in a few min.

@gajus
Copy link
Contributor

gajus commented Jun 4, 2024

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants