ReviewOS

stacks/bun-query-builder

SQL injection: window functions interpolate partitionBy, orderBy, column and alias unvalidated

#1185
Closed glennmichael123 opened this 8 days ago ·
8 days ago

buildOverClause and addWindowFunction interpolate every one of their inputs raw. #1183 fixed the SELECT / ORDER BY / GROUP BY family and did not touch these.

Proof, live Postgres

const payload = 'team_id) AS x, (SELECT secret FROM win_users LIMIT 1'
await db.selectFrom('win_users').sumOver('amount', { partitionBy: payload }).get()
EXFILTRATED: {"id":1,"team_id":1,"amount":10,"secret":"hunter2","x":"30","amount_sum":"hunter2"}

amount_sum is the secret. No stacked statement needed — the payload closes the PARTITION BY ( and opens a subquery, so this works wherever the driver refuses multiple statements.

Four vectors, not one

inputpayload lands inresult
partitionByOVER (PARTITION BY …)injected
orderByOVER (ORDER BY …)injected
columnSUM(…)injected
alias… AS …injected
SELECT *, SUM(amount) OVER (ORDER BY id; DROP TABLE users; -- ASC) AS amount_sum FROM users
SELECT *, SUM(id; DROP TABLE users; --) OVER () AS id; DROP TABLE users; --_sum FROM users
SELECT *, SUM(amount) OVER () AS id; DROP TABLE users; -- FROM users

Reachable through the whole documented surface: sumOver, avgOver, countOver, minOver, maxOver, firstValue, lastValue, lag, lead — every one forwards opts.partitionBy / opts.orderBy / column / opts.alias to addWindowFunction (client.ts:4587-4629) and on to buildOverClause (client.ts:762).

Not a bug: over(expression, …)

over() takes a raw expression by design — it is the documented escape hatch, the same contract as raw. Its expression argument should stay raw. Its partitionBy should not, and currently is.

Why I missed this in the #1181 audit

I probed rowNumber({ partitionBy }), which is not the signature. The call returned a no-op, I read that as "not reachable", and recorded it as an unconfirmed lead rather than testing the real shape. The actual API is sumOver(column, opts) / lag(column, opts), and it is reachable through all nine methods.

Worth stating plainly because the conclusion was wrong in the safe direction only by luck — "my probe did not reach it" is not evidence that nothing does.

Suggested fix

assertSafeColumnRef already exists from #1183. Apply it to:

  • buildOverClause's partitionBy entries and orderBy column names
  • addWindowFunction's column and alias

The direction (asc/desc) is already mapped through a ternary and is safe. over()'s expression stays untouched.

I will send the patch.

Sign in to comment on this issue.