ReviewOS

also looking at this

stacks/bun-query-builder

fix(security): validate column references in window functions

#1186
Merged glennmichael123 wants to merge fix/window-fn-injection into main
2 files +127 -13
8 days ago

Closes #1185. The loose end from the #1181 audit — and it was real.

buildOverClause and addWindowFunction interpolated all four of their inputs raw. #1183 validated the SELECT / FROM / ORDER BY / GROUP BY family and did not reach here.

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. The payload closes the PARTITION BY ( and opens a subquery, so no stacked statement is needed — this works wherever the driver refuses multiple statements, unlike the #1183 case.

Four vectors

inputlands inbeforeafter
partitionByOVER (PARTITION BY …)injectedthrows
orderByOVER (ORDER BY …)injectedthrows
columnSUM(…)injectedthrows
alias… AS …injectedthrows

All reachable through all nine public methods: sumOver, avgOver, countOver, minOver, maxOver, firstValue, lastValue, lag, lead.

Deliberately left raw

  • over()'s expression is the documented escape hatch, same contract as raw. Its partitionBy is not, and was injectable through it too — that one is now validated.
  • COUNT(*) keeps its star. It is the canonical window count, while SUM(*) and AVG(*) are not valid SQL, so the allowance is exactly that narrow and nothing else gets it.

How I missed this the first time

In the #1181 audit I probed rowNumber({ partitionBy }), which is not the signature. It returned a no-op, I read that as "not reachable", and filed it as an unconfirmed lead rather than checking the real API. The actual shape is sumOver(column, opts).

"My probe did not reach it" was never evidence that nothing does, and I should not have let it stand as a reason to defer.

One real breakage, caught by the existing suite

The first pass rejected countOver('*') and the #1050 window-function tests went red. That is a legitimate call, so the fix was wrong rather than the test — COUNT(*) now has its own allowance and its own regression test.

Verification

  • Live exfiltration now throws before reaching the driver
  • Full probe: all four vectors refused across all nine methods
  • Legitimate usage unchanged — SUM(amount) OVER (PARTITION BY team_id ORDER BY id DESC), array partitionBy, numeric lag offset, and over()'s raw expression all build as before
  • 6 new tests, mutation-checked — disabling the validator fails 4 of them
  • Full suite 2485 pass, 0 fail; tsc and pickier clean

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.