ReviewOS
Reload

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
packages/bun-query-builder/src/client.tsmodified+41-13
@@ -759,13 +759,31 @@ function formatSubqueryValue(val: unknown): string {
759759
760760// Shared OVER (...) builder for the window functions (rowNumber/rank/... and
761761// lag/lead/sumOver/...). Stateless — see the note above on module-level helpers.
762/**
763 * The `OVER (...)` clause, with every column reference validated.
764 *
765 * Both halves used to be interpolated raw, so a `partitionBy` taken from
766 * request input closed the parenthesis and opened a subquery:
767 *
768 * sumOver('amount', { partitionBy: 'team_id) AS x, (SELECT secret FROM t LIMIT 1' })
769 * -> {"amount_sum": "hunter2"}
770 *
771 * No stacked statement needed, so it worked wherever the driver refuses
772 * multiple statements. #1183 fixed the SELECT/ORDER BY family and did not
773 * reach here. stacksjs/bun-query-builder#1185
774 *
775 * The direction is mapped through a ternary and cannot carry a payload.
776 */
762777function buildOverClause(partitionBy?: string | string[], orderBy?: [string, 'asc' | 'desc'][]): string {
763778 const cols = Array.isArray(partitionBy) ? partitionBy : (partitionBy ? [partitionBy] : [])
764779 const parts: string[] = []
765780 if (cols.length)
766 parts.push(`PARTITION BY ${cols.join(', ')}`)
767 if (orderBy && orderBy.length)
768 parts.push(`ORDER BY ${orderBy.map(([c, d]) => `${c} ${d === 'desc' ? 'DESC' : 'ASC'}`).join(', ')}`)
781 parts.push(`PARTITION BY ${cols.map(one => assertSafeColumnRef(one, 'over(partitionBy)')).join(', ')}`)
782 if (orderBy && orderBy.length) {
783 parts.push(`ORDER BY ${orderBy
784 .map(([c, d]) => `${assertSafeColumnRef(c, 'over(orderBy)')} ${d === 'desc' ? 'DESC' : 'ASC'}`)
785 .join(', ')}`)
786 }
769787 return parts.length ? `OVER (${parts.join(' ')})` : 'OVER ()'
770788}
771789
@@ -4292,7 +4310,11 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
42924310 }
42934311
42944312 const addWindowFunction = (fnExpr: string, alias: string, partitionBy?: string | string[], orderBy?: [string, 'asc' | 'desc'][]): void => {
4295 addToSelectClause(`${fnExpr} ${buildOverClause(partitionBy, orderBy)} AS ${alias}`)
4313 // `fnExpr` is built by the caller and may legitimately be an expression
4314 // (`SUM(amount)`, and `over()`'s raw escape hatch). The alias cannot:
4315 // it was interpolated after `AS` and took a payload the same way. #1185
4316 const safeAlias = assertSafeColumnRef(alias, 'over(alias)')
4317 addToSelectClause(`${fnExpr} ${buildOverClause(partitionBy, orderBy)} AS ${safeAlias}`)
42964318 }
42974319
42984320 // Helper function to build hasOne/hasMany subquery with validation
@@ -4588,45 +4610,51 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
45884610 return this as any
45894611 },
45904612 lag(column: string, opts: WindowOpts & { offset?: number, defaultValue?: string | number } = {}) {
4591 const args = [column, String(opts.offset ?? 1)]
4613 // Only the first arg is a column; the offset is stringified from a
4614 // number and the default value is a caller-supplied literal. #1185
4615 const args = [assertSafeColumnRef(column, 'over(column)'), String(opts.offset ?? 1)]
45924616 if (opts.defaultValue !== undefined)
45934617 args.push(String(opts.defaultValue))
45944618 addWindowFunction(`LAG(${args.join(', ')})`, opts.alias ?? `${column}_lag`, opts.partitionBy, opts.orderBy)
45954619 return this as any
45964620 },
45974621 lead(column: string, opts: WindowOpts & { offset?: number, defaultValue?: string | number } = {}) {
4598 const args = [column, String(opts.offset ?? 1)]
4622 // Only the first arg is a column; the offset is stringified from a
4623 // number and the default value is a caller-supplied literal. #1185
4624 const args = [assertSafeColumnRef(column, 'over(column)'), String(opts.offset ?? 1)]
45994625 if (opts.defaultValue !== undefined)
46004626 args.push(String(opts.defaultValue))
46014627 addWindowFunction(`LEAD(${args.join(', ')})`, opts.alias ?? `${column}_lead`, opts.partitionBy, opts.orderBy)
46024628 return this as any
46034629 },
46044630 sumOver(column: string, opts: WindowOpts = {}) {
4605 addWindowFunction(`SUM(${column})`, opts.alias ?? `${column}_sum`, opts.partitionBy, opts.orderBy)
4631 addWindowFunction(`SUM(${assertSafeColumnRef(column, 'over(column)')})`, opts.alias ?? `${column}_sum`, opts.partitionBy, opts.orderBy)
46064632 return this as any
46074633 },
46084634 avgOver(column: string, opts: WindowOpts = {}) {
4609 addWindowFunction(`AVG(${column})`, opts.alias ?? `${column}_avg`, opts.partitionBy, opts.orderBy)
4635 addWindowFunction(`AVG(${assertSafeColumnRef(column, 'over(column)')})`, opts.alias ?? `${column}_avg`, opts.partitionBy, opts.orderBy)
46104636 return this as any
46114637 },
4638 // `COUNT(*)` is the canonical window count, so the star is allowed here
4639 // and nowhere else: `SUM(*)` and `AVG(*)` are not valid SQL. #1185
46124640 countOver(column: string = '*', opts: WindowOpts = {}) {
4613 addWindowFunction(`COUNT(${column})`, opts.alias ?? 'count_over', opts.partitionBy, opts.orderBy)
4641 addWindowFunction(`COUNT(${assertSafeColumnRef(column, 'over(column)', { allowStar: true })})`, opts.alias ?? 'count_over', opts.partitionBy, opts.orderBy)
46144642 return this as any
46154643 },
46164644 minOver(column: string, opts: WindowOpts = {}) {
4617 addWindowFunction(`MIN(${column})`, opts.alias ?? `${column}_min`, opts.partitionBy, opts.orderBy)
4645 addWindowFunction(`MIN(${assertSafeColumnRef(column, 'over(column)')})`, opts.alias ?? `${column}_min`, opts.partitionBy, opts.orderBy)
46184646 return this as any
46194647 },
46204648 maxOver(column: string, opts: WindowOpts = {}) {
4621 addWindowFunction(`MAX(${column})`, opts.alias ?? `${column}_max`, opts.partitionBy, opts.orderBy)
4649 addWindowFunction(`MAX(${assertSafeColumnRef(column, 'over(column)')})`, opts.alias ?? `${column}_max`, opts.partitionBy, opts.orderBy)
46224650 return this as any
46234651 },
46244652 firstValue(column: string, opts: WindowOpts = {}) {
4625 addWindowFunction(`FIRST_VALUE(${column})`, opts.alias ?? `${column}_first`, opts.partitionBy, opts.orderBy)
4653 addWindowFunction(`FIRST_VALUE(${assertSafeColumnRef(column, 'over(column)')})`, opts.alias ?? `${column}_first`, opts.partitionBy, opts.orderBy)
46264654 return this as any
46274655 },
46284656 lastValue(column: string, opts: WindowOpts = {}) {
4629 addWindowFunction(`LAST_VALUE(${column})`, opts.alias ?? `${column}_last`, opts.partitionBy, opts.orderBy)
4657 addWindowFunction(`LAST_VALUE(${assertSafeColumnRef(column, 'over(column)')})`, opts.alias ?? `${column}_last`, opts.partitionBy, opts.orderBy)
46304658 return this as any
46314659 },
46324660 selectAll(tables?: string | string[]) {
packages/bun-query-builder/test/client.window-fn-injection.test.tsadded+86-0
@@ -0,0 +1,86 @@
1/**
2 * Window-function identifier injection — stacksjs/bun-query-builder#1185.
3 *
4 * #1183 validated the SELECT / FROM / ORDER BY / GROUP BY family and did not
5 * reach `buildOverClause` or `addWindowFunction`, which interpolated all four
6 * of their inputs raw. Verified against live Postgres before the fix:
7 *
8 * sumOver('amount', { partitionBy: 'team_id) AS x, (SELECT secret FROM t LIMIT 1' })
9 * -> {"amount_sum": "hunter2"}
10 *
11 * The payload closes the PARTITION BY parenthesis and opens a subquery, so it
12 * exfiltrates without a stacked statement and works wherever the driver
13 * refuses multiple statements.
14 */
15
16import { describe, expect, it } from 'bun:test'
17import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
18
19function qb(): any {
20 const models = {
21 users: {
22 columns: {
23 id: { type: 'integer', isPrimaryKey: true },
24 team_id: { type: 'integer' },
25 amount: { type: 'integer' },
26 },
27 },
28 } as any
29 return createQueryBuilder<any>({
30 schema: buildDatabaseSchema(models),
31 meta: buildSchemaMeta(models),
32 autoMigration: { enabled: false } as any,
33 })
34}
35
36const EXFIL = 'team_id) AS x, (SELECT secret FROM users LIMIT 1'
37const STACKED = 'id; DROP TABLE users; --'
38const REFUSED = /is not a plain column reference/
39
40describe('window functions refuse injected identifiers (#1185)', () => {
41 it('partitionBy, the vector proven against a live database', () => {
42 expect(() => qb().selectFrom('users').sumOver('amount', { partitionBy: EXFIL })).toThrow(REFUSED)
43 expect(() => qb().selectFrom('users').lag('amount', { partitionBy: EXFIL })).toThrow(REFUSED)
44 })
45
46 it('orderBy, column and alias are the other three', () => {
47 expect(() => qb().selectFrom('users').sumOver('amount', { orderBy: [[STACKED, 'asc']] })).toThrow(REFUSED)
48 expect(() => qb().selectFrom('users').sumOver(STACKED, {})).toThrow(REFUSED)
49 expect(() => qb().selectFrom('users').sumOver('amount', { alias: STACKED })).toThrow(REFUSED)
50 })
51
52 it('every convenience method, not just sumOver', () => {
53 const each = (fn: string) => () => (qb().selectFrom('users') as any)[fn]('amount', { partitionBy: EXFIL })
54 for (const fn of ['avgOver', 'countOver', 'minOver', 'maxOver', 'firstValue', 'lastValue', 'lead'])
55 expect(each(fn)).toThrow(REFUSED)
56 })
57
58 it('COUNT(*) is still allowed, and only for COUNT', () => {
59 // The canonical window count. SUM(*) and AVG(*) are not valid SQL, so the
60 // star is permitted here and refused everywhere else - caught by the
61 // existing #1050 suite when the first pass of this fix rejected it.
62 expect(String(qb().selectFrom('users').countOver('*').toSQL())).toContain('COUNT(*)')
63 expect(() => qb().selectFrom('users').sumOver('*', {})).toThrow(REFUSED)
64 })
65
66 it('over() still validates partitionBy, though its expression stays raw', () => {
67 // The expression is the documented escape hatch, same contract as `raw`.
68 // partitionBy is not, and was injectable through it too.
69 expect(() => qb().selectFrom('users').over('ROW_NUMBER()', 'rn', { partitionBy: EXFIL })).toThrow(REFUSED)
70 })
71})
72
73describe('legitimate window usage is unaffected (#1185)', () => {
74 it('builds the clause it always did', () => {
75 const sum = String(qb().selectFrom('users').sumOver('amount', { partitionBy: 'team_id', orderBy: [['id', 'desc']] }).toSQL())
76 expect(sum).toContain('SUM(amount) OVER (PARTITION BY team_id ORDER BY id DESC)')
77
78 // An array partitionBy, and an offset that is a number rather than a column.
79 const lag = String(qb().selectFrom('users').lag('amount', { offset: 2, partitionBy: ['team_id', 'id'] }).toSQL())
80 expect(lag).toContain('LAG(amount, 2) OVER (PARTITION BY team_id, id)')
81
82 // over()'s expression is not a column reference and must stay raw.
83 const over = String(qb().selectFrom('users').over('ROW_NUMBER()', 'rn', { partitionBy: 'team_id' }).toSQL())
84 expect(over).toContain('ROW_NUMBER() OVER (PARTITION BY team_id) AS rn')
85 })
86})