Parametrized queries

[tdammers]

It looks like postgresql-simple does not actually support parametrized queries.

When using ? syntax to inject values into queries, the values are converted to (query) strings (via the ToField class), and the query string is parsed on the client, and ?'s replaced with the encoded values.

From a security perspective, this is kind of bad - all it takes to create a potential opening for an SQLi attack is one incorrect ToField instance, or a particularly quirky query that the query parser isn't prepared to handle, or a bug in the escaping code. And this is a completely unnecessary risk, because postgres natively supports parametrized queries (see for example PQexecParams), keeping the query unchanged, and sending parameters separately; the parametrization happens on the server, and does not involve rewriting SQL source code (the value get injected into the query AST on the server). There is no risk of SQL injection here, because by the time postgres looks at the dynamic values, the SQL query source code has already been discarded.

[SmartHypercube]

+1. The current behavior looks pretty dangerous to me, and other libraries don't do this. Not only one incorrect ToField instance can break the security model, a syntax update by PostgreSQL may too, e.g. the text quoting rules changed (although unlikely), even if you didn't add any custom ToField instances.

Putting it in other words, if this library sends parameters by PQexecParams, it will be a lot easier to prove everything is safe against SQL injections, due to parameters completely isolated from the SQL query. I of course believe that the authors of this library are not evil. But a small, honest mistake made while reading PostgreSQL documentations may result in a big security problem without people noticing. Given that a far better choice exists, it's weird to go this way and try to prove it's safe.

[dustin]

I've run into multiple issues related to this recently, some of which I can't work around because (I believe) there are bugs in escaping of values and query parsing. See #106 and #107

I'd expect it'd make code simpler and more consistent with other libraries while having a clean path to feed parameters in.

[phadej]

Which other libraries?

#107 wouldn't be fixed by this, as even for PQexecParams you have to tell whether the param is text or binary.

How executeMany would work?

I have no intention to work on this. I'm curious to see a patch, but no guarantee I'll merge that though. I don't think code will be made simpler, just different.

[dustin]

Which other libraries?

Any library that uses PQexecParams as there's one canonical way to process parameters. postgresql-simple has its own mechanism instead of using the one postgres provides.

How executeMany would work?

Sounds a lot like PQexecPrepared, which I assume would be a lot more efficient as less data is transferred and you don't have to build lots of redundant ByteString values to get stuff to postgres.

[phadej]

executeMany executes many row inserts as single command. I don't think PQexecPrepared can do it.

I think you misunderstood what executeMany does, as your other issue was about that as well.

[dustin]

That sounds like an implementation detail that I don't see documented. Implementing this function as a traversal over a proper prepared statement would still meet what's specified here.

That's what sqlite-simple does. I was porting code from sqlite-simple which uses executeMany in several instances and only the one in #106 failed to execute because, you know, this bug.

[phadej]

Implementing this function as a traversal over a proper prepared statement would still meet what's specified here.

It won't. Multiple queries are not transactional. That change would be also breaking and of the "type-checker doesn't help you to migrate".

[dustin]

You should update the documentation for executeMany to state all the things you think it should do that it's not currently documented to do and clearly surprises people.

It doesn't state it's atomic (though presumably it would be in an explicit transaction) and it doesn't state it's a single statement. It doesn't state that your query parser won't work without the question marks being inside a VALUES keyword.

The only reasonable implementation of this as documented would be a prepared statement with the parameters safely delivered, which is why it's relevant to this bug. Alternatively, allowing users to programmatically construct a query with a multi-VALUES placeholder might make sense as long it was explicit.

But the "you must know the undocumented ways we manipulate the strings you send us in order to make new strings of user input to send to the database" model you currently have is very surprising and the bugs that fall out of it are quite discouraging.

[hasufell]

@dustin I switched to hasql, because I don't trust this library anymore.