Issues with $ placeholds in QueryFiles, pg-promise
First, a concrete issue: the bug identified in #642 (closed) is now fixed when the sql is passed directly to
db.query, but it still exists when the SQL lives in a script file. The issue is that, when massive invokes the Executable generated from the file, somewhere around here the argument with the placeholder values again seems to get defaulted to an empty array, causing the same bug.
Code demonstrating the behavior
// scriptFile.sql CREATE TABLE test ( id SERIAL PRIMARY KEY, column TEXT ); INSERT INTO "test" ("column") VALUES (E'$65,478');
db.scriptFile(); // RangeError: Variable $65 out of range. Parameters array length: 0
Should run the insert with no error.
While I understand that this issue can be fixed by not passing
pg-promise's formatter an empty array, so that it doesn't search the sql text for parameters, the fact that pg-promise is identifying
$65 inside a string literal as a potential parameter seems really, really problematic regardless.
First, I'm pretty sure it's wrong (i.e., I don't think parameters are allowed to occur within strings). More fundamentally, though, it's happening because
pg-promise is parsing the query text itself and substituting in the parameter values, before sending the final concatenated string to the db server.
pg-promise, this query parsing logic seems to be based around a lot of regular expressions — rather than a proper parser derived from a formal grammar — and it's bound have lots of bugs that are nearly guaranteed to create SQL injection vulnerabilities. Identifying "parameters" inside string literals is only one such bug. I actually found another bug today too, which I'll link here once I've opened an issue for it.
By contrast, the
node-postgres library sends the user's query unaltered to the Postgres server, where postgres parses it to identify the parameters, with all its battle-tested parsing logic. (This happens through the extended query protocol. Massive should seriously consider switching to that approach for security.