ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(security): validate column references on the SELECT/ORDER BY family

#1183
Merged glennmichael123 wants to merge fix/identifier-injection into main
2 files +182 -10
packages/bun-query-builder/src/client.tsmodified+70-10
@@ -353,6 +353,50 @@ export function quoteColumnForDialect(name: string, dialect = config.dialect): s
353353 return alias ? `${column} AS \`${alias}\`` : column
354354}
355355
356/**
357 * A column reference safe to interpolate: `col`, `tbl.col`, either with
358 * `AS alias`, and `*` / `tbl.*` where a star makes sense.
359 *
360 * Deliberately the same shape `quoteColumnForDialect` already parses, because
361 * anything it cannot parse it returns verbatim - it is a MySQL quoter, not a
362 * validator, and on Postgres and SQLite it returns its argument untouched. So
363 * every caller that passed a column name through it was interpolating that
364 * name raw. stacksjs/bun-query-builder#1181
365 */
366const SAFE_COLUMN_REF = /^(?:[A-Z_][\w$]*)(?:\.(?:[A-Z_][\w$]*))?(?:\s+as\s+[A-Z_][\w$]*)?$/i
367const SAFE_COLUMN_REF_STAR = /^(?:\*|(?:[A-Z_][\w$]*)(?:\.(?:[A-Z_][\w$]*|\*))?(?:\s+as\s+[A-Z_][\w$]*)?)$/i
368
369/**
370 * Validate a caller-supplied column reference, then quote it.
371 *
372 * A sort column is the most common piece of request input that is legitimately
373 * a column name, so `?sort=` reaching `orderBy()` is the realistic path: before
374 * this, `orderBy('id; DROP TABLE users; --')` built that statement and the
375 * driver ran it. `where`, `whereIn`, `whereNull` and `join` already refused the
376 * same input through `validateIdentifier`; the ORDER BY, GROUP BY and SELECT
377 * family did not.
378 *
379 * Expressions belong in the `*Raw` methods or the `raw` tagged template, which
380 * is why this is strict rather than a best-effort sanitiser: there is an
381 * escape hatch, and a validator that tries to allow `COUNT(*) + ?` safely is a
382 * parser pretending to be a regex.
383 */
384function assertSafeColumnRef(name: unknown, context: string, options: { allowStar?: boolean } = {}): string {
385 const raw = String(name).trim()
386 const pattern = options.allowStar ? SAFE_COLUMN_REF_STAR : SAFE_COLUMN_REF
387
388 if (!pattern.test(raw)) {
389 throw new TypeError(
390 `[query-builder] ${context}: '${raw}' is not a plain column reference, so it will not be `
391 + `interpolated into SQL. Expected ${options.allowStar ? '`*`, ' : ''}\`column\`, \`table.column\`, `
392 + `optionally with \`AS alias\`. For an expression use the matching *Raw method or the \`raw\` `
393 + `tagged template, which escape what they interpolate.`,
394 )
395 }
396
397 return quoteColumnForDialect(raw)
398}
399
356400/**
357401 * A date, in the literal shape the dialect will accept.
358402 *
@@ -395,19 +439,31 @@ export function temporalLiteral(value: unknown, dialect = config.dialect): unkno
395439}
396440
397441function renderSelectColumn(col: unknown): string {
442 // Only the plain-string form is validated. A SqlFragment or `raw` expression
443 // falls through untouched below, which is the escape hatch for `COUNT(*)`
444 // and friends. #1181
398445 if (typeof col === 'string')
399 return quoteColumnForDialect(col)
446 return assertSafeColumnRef(col, 'select(column)', { allowStar: true })
400447 if (isRawExpression(col))
401448 return col.raw
449 // `addSelect` is variadic, so `addSelect(['a', 'b'])` - the shape `select`
450 // takes - arrives as one array argument. It used to reach the `String(col)`
451 // fallback below and be spliced in whole, unvalidated: that was the one
452 // SELECT-family hole the first pass of #1181 missed. Recursing instead keeps
453 // the ergonomic and runs each element through the same check.
454 if (Array.isArray(col))
455 return col.map(renderSelectColumn).join(', ')
402456 if (col && typeof col === 'object') {
403457 const anyCol = col as { raw?: unknown, sql?: unknown }
404458 if (typeof anyCol.raw === 'function')
405459 return String((anyCol.raw as () => unknown)())
406460 if (typeof anyCol.sql === 'string')
407461 return anyCol.sql
462 // Anything else that stringifies to something is a bare name, not a
463 // fragment that opted into raw interpolation, so it is validated too.
408464 const str = String(col)
409465 if (str !== '[object Object]')
410 return str
466 return assertSafeColumnRef(str, 'select(column)', { allowStar: true })
411467 }
412468 throw new TypeError(
413469 `[query-builder] select(): unsupported column ${String(col)} — pass a column name, a string[], or a SQL fragment (e.g. sql\`count(*) as c\`)`,
@@ -4454,7 +4510,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
44544510 return this as any
44554511 },
44564512 distinctOn(...columns: any[]) {
4457 const colList = columns.map(String).join(', ')
4513 const colList = columns.map(one => assertSafeColumnRef(one, 'distinctOn(column)')).join(', ')
44584514 text = text.replace(/^SELECT\s+/i, `SELECT DISTINCT ON (${colList}) `)
44594515 built = null
44604516 return this as any
@@ -5930,7 +5986,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
59305986 const dir = direction === 'asc' ? 'ASC' : 'DESC'
59315987 // Quoted for the same reason a select list is: `ORDER BY key ASC` is a
59325988 // syntax error on MySQL, `key` being reserved there.
5933 const ordered = quoteColumnForDialect(String(column))
5989 const ordered = assertSafeColumnRef(column, 'orderBy(column)')
59345990 text = SQL_PATTERNS.ORDER_BY.test(text)
59355991 ? `${text}, ${ordered} ${dir}`
59365992 : `${text} ORDER BY ${ordered} ${dir}`
@@ -5938,9 +5994,10 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
59385994 return this
59395995 },
59405996 orderByDesc(column: string) {
5997 const ordered = assertSafeColumnRef(column, 'orderByDesc(column)')
59415998 text = SQL_PATTERNS.ORDER_BY.test(text)
5942 ? `${text}, ${column} DESC`
5943 : `${text} ORDER BY ${column} DESC`
5999 ? `${text}, ${ordered} DESC`
6000 : `${text} ORDER BY ${ordered} DESC`
59446001 built = null
59456002 return this as any
59466003 },
@@ -5959,7 +6016,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
59596016 return this as any
59606017 },
59616018 latest(column?: any) {
5962 const col = column ?? config.timestamps.defaultOrderColumn
6019 const col = assertSafeColumnRef(column ?? config.timestamps.defaultOrderColumn, 'latest(column)')
59636020 text = SQL_PATTERNS.ORDER_BY.test(text)
59646021 ? `${text}, ${col} DESC`
59656022 : `${text} ORDER BY ${col} DESC`
@@ -5967,7 +6024,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
59676024 return this as any
59686025 },
59696026 oldest(column?: any) {
5970 const col = column ?? config.timestamps.defaultOrderColumn
6027 const col = assertSafeColumnRef(column ?? config.timestamps.defaultOrderColumn, 'oldest(column)')
59716028 text = SQL_PATTERNS.ORDER_BY.test(text)
59726029 ? `${text}, ${col} ASC`
59736030 : `${text} ORDER BY ${col} ASC`
@@ -6126,8 +6183,8 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
61266183 // Compose with any existing GROUP BY so chained calls add columns
61276184 // instead of emitting a second clause.
61286185 text = SQL_PATTERNS.GROUP_BY.test(text)
6129 ? `${text}, ${cols.map(one => quoteColumnForDialect(String(one))).join(', ')}`
6130 : `${text} GROUP BY ${cols.map(one => quoteColumnForDialect(String(one))).join(', ')}`
6186 ? `${text}, ${cols.map(one => assertSafeColumnRef(one, 'groupBy(column)')).join(', ')}`
6187 : `${text} GROUP BY ${cols.map(one => assertSafeColumnRef(one, 'groupBy(column)')).join(', ')}`
61316188 built = null
61326189 }
61336190 return this as any
@@ -6969,6 +7026,9 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
69697026 return makeSelect<any>(table, columns as string[]) as unknown as SelectQueryBuilder<DB, TTable, any, TTable>
69707027 },
69717028 selectFrom<TTable extends keyof DB & string>(table: TTable) {
7029 // The type says `keyof DB`, but a JS caller or an `as any` can pass
7030 // anything, and the name is interpolated into `FROM`. #1181
7031 assertSafeColumnRef(table, 'selectFrom(table)')
69727032 return makeSelect<TTable>(table)
69737033 },
69747034 selectFromSub(sub, alias) {
packages/bun-query-builder/test/client.identifier-injection.test.tsadded+112-0
@@ -0,0 +1,112 @@
1/**
2 * Identifier injection on the SELECT / FROM / ORDER BY / GROUP BY family —
3 * stacksjs/bun-query-builder#1181.
4 *
5 * These methods interpolated caller-supplied column names straight into SQL.
6 * `quoteColumnForDialect` looked like protection and was not: it is a MySQL
7 * quoter that returns anything it cannot parse verbatim, and returns its
8 * argument untouched on every other dialect.
9 *
10 * Measured against live Postgres before the fix, with `?sort=` as the payload:
11 *
12 * table exists before : true
13 * SQL built : SELECT * FROM sec_users ORDER BY id; DROP TABLE sec_users; -- ASC
14 * table exists after : false
15 *
16 * The stacked statement ran. The WHERE and join family already refused the
17 * same input through `validateIdentifier`, and `orm.ts` had closed the ORDER BY
18 * half after stacksjs/stacks#1858, so this was a gap in an existing defence.
19 */
20
21import { describe, expect, it } from 'bun:test'
22import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src'
23
24function qb(): any {
25 const models = {
26 users: {
27 columns: {
28 id: { type: 'integer', isPrimaryKey: true },
29 name: { type: 'text' },
30 team_id: { type: 'integer' },
31 },
32 },
33 } as any
34 return createQueryBuilder<any>({
35 schema: buildDatabaseSchema(models),
36 meta: buildSchemaMeta(models),
37 autoMigration: { enabled: false } as any,
38 })
39}
40
41const PAYLOAD = 'id; DROP TABLE users; --'
42const REFUSED = /is not a plain column reference/
43
44describe('identifier injection is refused (#1181)', () => {
45 it('orderBy, the realistic entry point', () => {
46 // A sort column is the most common request input that is legitimately a
47 // column name, so this is the parameter most likely to be passed through.
48 expect(() => qb().selectFrom('users').orderBy(PAYLOAD)).toThrow(REFUSED)
49 })
50
51 it('every other ORDER BY spelling', () => {
52 expect(() => qb().selectFrom('users').orderByDesc(PAYLOAD)).toThrow(REFUSED)
53 expect(() => qb().selectFrom('users').latest(PAYLOAD)).toThrow(REFUSED)
54 expect(() => qb().selectFrom('users').oldest(PAYLOAD)).toThrow(REFUSED)
55 })
56
57 it('groupBy and distinctOn', () => {
58 expect(() => qb().selectFrom('users').groupBy(PAYLOAD)).toThrow(REFUSED)
59 expect(() => qb().selectFrom('users').distinctOn(PAYLOAD)).toThrow(REFUSED)
60 })
61
62 it('select and addSelect', () => {
63 expect(() => qb().selectFrom('users').select([PAYLOAD])).toThrow(REFUSED)
64 expect(() => qb().selectFrom('users').addSelect(PAYLOAD)).toThrow(REFUSED)
65 })
66
67 it('addSelect given an array, which bypassed the first fix', () => {
68 // `addSelect` is variadic, so `addSelect(['a'])` - the shape `select`
69 // takes - arrived as one array argument and reached the String() fallback
70 // whole. renderSelectColumn now recurses instead.
71 expect(() => qb().selectFrom('users').addSelect([PAYLOAD])).toThrow(REFUSED)
72 })
73
74 it('selectFrom, where the type says keyof DB but JS can pass anything', () => {
75 expect(() => qb().selectFrom('users; DROP TABLE users; --')).toThrow(REFUSED)
76 })
77
78 it('a subquery is refused as well as a stacked statement', () => {
79 // Stacked statements are the loud case; an ORDER BY subquery is the quiet
80 // one, and is a working boolean oracle wherever stacking is blocked.
81 expect(() => qb().selectFrom('users').orderBy('(SELECT secret FROM users LIMIT 1)')).toThrow(REFUSED)
82 expect(() => qb().selectFrom('users').select(['name FROM users UNION SELECT 1 --'])).toThrow(REFUSED)
83 })
84})
85
86describe('legitimate column references still work (#1181)', () => {
87 it('plain, qualified and aliased names', () => {
88 expect(String(qb().selectFrom('users').orderBy('name').toSQL())).toContain('ORDER BY')
89 expect(String(qb().selectFrom('users').orderBy('users.name').toSQL())).toContain('ORDER BY')
90 expect(String(qb().selectFrom('users').select(['name AS n']).toSQL())).toContain('name')
91 expect(String(qb().selectFrom('users').groupBy('team_id').toSQL())).toContain('GROUP BY')
92 })
93
94 it('star, in the one place it belongs', () => {
95 // Allowed for select, refused for ORDER BY, where it is not a column.
96 expect(String(qb().selectFrom('users').select(['*']).toSQL())).toContain('*')
97 expect(String(qb().selectFrom('users').select(['users.*']).toSQL())).toContain('users')
98 expect(() => qb().selectFrom('users').orderBy('*')).toThrow(REFUSED)
99 })
100
101 it('addSelect still takes the array form', () => {
102 expect(String(qb().selectFrom('users').addSelect(['name', 'team_id']).toSQL())).toContain('name')
103 })
104
105 it('expressions go through raw, which is the escape hatch', () => {
106 // The strict rule is only tenable because this exists: `raw` and the *Raw
107 // methods escape what they interpolate, and bypass the identifier check by
108 // design rather than by accident.
109 const sql = String(qb().selectFrom('users').select([raw('COUNT(*) AS c')]).toSQL())
110 expect(sql).toContain('COUNT(*)')
111 })
112})