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[]) {