Pg-promise: Helpers do not escape $ signs in data

[ejoebstl]

The following code will raise an error, as the query formatter will attempt to interpret $350 as a parameter:

const data = { text: 'He owes me $450' }
const query = dbp.helpers.insert(data, null, 'NOTES') + ' RETURNING ID'
const result = await db.one(query, [])

I would expect the helper classes to properly escape reserved characters. What is the proper way to use the helpers/work around this issue?

Generated query:

insert into "Notes" ("text") values('He owes me $450') RETURNING ID

Stack trace for reference:

RangeError: Variable $450 out of range. Parameters array length: 0
    at query.replace.name (node_modules/pg-promise/lib/formatting.js:174:19)
    at String.replace (<anonymous>)
    at Object.array (node_modules/pg-promise/lib/formatting.js:158:22)
    at Object.$formatQuery [as formatQuery] (node_modules/pg-promise/lib/formatting.js:274:29)
    at Database.$query (node_modules/pg-promise/lib/query.js:113:40)
    at Database.<anonymous> (node_modules/pg-promise/lib/query.js:241:23)
    at config.$npm.connect.pool.then.db (node_modules/pg-promise/lib/database.js:307:42)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:160:7)

[vitaly-t]

When you pass [] into your query method, you tell it that your query uses syntax $1, $2,... to format the query. The method enumerates all formatting variables, and finds one that points outside the range of parameters. So, the error reported is accurate.

I would expect the helper classes to properly escape reserved characters.

Except, you are not using any :) You are generating a query without any [ColumnSet], based just on the object itself.

What is the proper way to use the helpers/work around this issue?

The following solutions depend on other requirements about the source object and/or the project...

The simplest one, is to remove [] when you call the query method, to avoid the parameter formatting, i.e. just call:

db.one(query)

The more generic one is to define a reusable [ColumnSet], and use it when generating the query.

Either way, you should avoid formatting queries more than once, or it will always create problems. In your case this is exactly what you are doing. Your query already contains values, which means it has been formatted already, and then you are trying to format it again by passing in [], that will inevitably create problems :wink:

This mistake is commonly known as Double-Formatting Issue :wink:, when the first formatting introduces values into the query which may contain text that looks like formatting variables, and then the second formatting naturally trips over that. Formatting a query must always be done in one step, unless you make use of partial formatting, but that's a separate topic.

[ejoebstl]

Thank you for the excellent answer!

[ethanresnick]

I think maybe this should be re-opened? After all, I'm pretty sure it's wrong for pg-promise to identify $450 as a possible parameter anyway, given that it occurs inside a string literal?

[ejoebstl]

I think detecting string literals might be out of scope. It would mean that this library actually had to parse queries.

[vitaly-t]

@ethanresnick This happens only when you mistakenly double-format queries, so the error would be on your side, and not in the library.

Double-formatting - formatting a query that already has been formatted and contains values.

[ethanresnick]

@vitaly-t I'm pretty sure this doesn't have to do with double formatting per se. If you just format this query once: INSERT into some_table ("name", "amount") VALUES ($1, '$10'), with a values array of ["My Name"], I believe you'll get an error. Maybe @ejoebstl is right that actually parsing the query is out of scope, but I wonder if this is an attack vector. If the user controls the query text being fed to format, then maybe it's fine, but I wonder if there's some exploit approach I'm not thinking of.

[ethanresnick]

Of course, fwiw, properly parsing the query would also remove the burden on the user to (remember to properly) specify :name etc as the escaping type. Of course, this proper parsing could slow things down considerably.

[vitaly-t]

This query template is invalid -

INSERT into some_table ("name", "amount") VALUES ($1, '$10')

There can be 3 types of valid queries:

• With formatting variables (not formatted yet)
• With values in it (after formatting)
• Mixed query that has variables, plus non-conflicting values.

Your query doesn't fit into any of those. It contains both variables and variable-conflicting values. You are not supposed to have such a query to begin with.

You should do something like this instead:

format('INSERT into some_table ("name", "amount") VALUES ($1, $2)',  ['My Name', '$10']) 

[tpiecora]

@vitaly-t I'd be curious on your thoughts...
What would you suggest is the safest way to approach this scenario:

When trying to generate an update query using the update helper, the documentation says it "typically needs a WHERE condition appended". But since the helper does the value insertion for you, you cannot format this query a second time. Now my WHERE clause is vulnerable to injection because I have to handle the value interpolation myself like:
let whereClause = 'WHERE id = ' + myResourceId;
and append that to the end of my update helper output.

Is there a good way to format this query in one shot?

[vitaly-t]

@tpiecora format the appended part using pgp.as.format function. See this also.

[tpiecora]

Perfect, that is what I was looking for. Thanks!
Might be good to reference that in the documentation for the update helper? I didn't read through every page of the documentation, but I definitely looked around and did not find that.