also looking at this
fix(security): validate column references on the SELECT/ORDER BY family
#1183
2 files
+182
-10
| @@ -353,6 +353,50 @@ export function quoteColumnForDialect(name: string, dialect = config.dialect): s | ||
| 353 | 353 | return alias ? `${column} AS \`${alias}\`` : column |
| 354 | 354 | } |
| 355 | 355 | |
| 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 | */ | |
| 366 | const SAFE_COLUMN_REF = /^(?:[A-Z_][\w$]*)(?:\.(?:[A-Z_][\w$]*))?(?:\s+as\s+[A-Z_][\w$]*)?$/i | |
| 367 | const 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 | */ | |
| 384 | function 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 | ||
| 356 | 400 | /** |
| 357 | 401 | * A date, in the literal shape the dialect will accept. |
| 358 | 402 | * |
| @@ -395,19 +439,31 @@ export function temporalLiteral(value: unknown, dialect = config.dialect): unkno | ||
| 395 | 439 | } |
| 396 | 440 | |
| 397 | 441 | function 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 | |
| 398 | 445 | if (typeof col === 'string') |
| 399 | return quoteColumnForDialect(col) | |
| 446 | return assertSafeColumnRef(col, 'select(column)', { allowStar: true }) | |
| 400 | 447 | if (isRawExpression(col)) |
| 401 | 448 | 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(', ') | |
| 402 | 456 | if (col && typeof col === 'object') { |
| 403 | 457 | const anyCol = col as { raw?: unknown, sql?: unknown } |
| 404 | 458 | if (typeof anyCol.raw === 'function') |
| 405 | 459 | return String((anyCol.raw as () => unknown)()) |
| 406 | 460 | if (typeof anyCol.sql === 'string') |
| 407 | 461 | 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. | |
| 408 | 464 | const str = String(col) |
| 409 | 465 | if (str !== '[object Object]') |
| 410 | return str | |
| 466 | return assertSafeColumnRef(str, 'select(column)', { allowStar: true }) | |
| 411 | 467 | } |
| 412 | 468 | throw new TypeError( |
| 413 | 469 | `[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 | ||
| 4454 | 4510 | return this as any |
| 4455 | 4511 | }, |
| 4456 | 4512 | distinctOn(...columns: any[]) { |
| 4457 | const colList = columns.map(String).join(', ') | |
| 4513 | const colList = columns.map(one => assertSafeColumnRef(one, 'distinctOn(column)')).join(', ') | |
| 4458 | 4514 | text = text.replace(/^SELECT\s+/i, `SELECT DISTINCT ON (${colList}) `) |
| 4459 | 4515 | built = null |
| 4460 | 4516 | return this as any |
| @@ -5930,7 +5986,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial | ||
| 5930 | 5986 | const dir = direction === 'asc' ? 'ASC' : 'DESC' |
| 5931 | 5987 | // Quoted for the same reason a select list is: `ORDER BY key ASC` is a |
| 5932 | 5988 | // syntax error on MySQL, `key` being reserved there. |
| 5933 | const ordered = quoteColumnForDialect(String(column)) | |
| 5989 | const ordered = assertSafeColumnRef(column, 'orderBy(column)') | |
| 5934 | 5990 | text = SQL_PATTERNS.ORDER_BY.test(text) |
| 5935 | 5991 | ? `${text}, ${ordered} ${dir}` |
| 5936 | 5992 | : `${text} ORDER BY ${ordered} ${dir}` |
| @@ -5938,9 +5994,10 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial | ||
| 5938 | 5994 | return this |
| 5939 | 5995 | }, |
| 5940 | 5996 | orderByDesc(column: string) { |
| 5997 | const ordered = assertSafeColumnRef(column, 'orderByDesc(column)') | |
| 5941 | 5998 | 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` | |
| 5944 | 6001 | built = null |
| 5945 | 6002 | return this as any |
| 5946 | 6003 | }, |
| @@ -5959,7 +6016,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial | ||
| 5959 | 6016 | return this as any |
| 5960 | 6017 | }, |
| 5961 | 6018 | latest(column?: any) { |
| 5962 | const col = column ?? config.timestamps.defaultOrderColumn | |
| 6019 | const col = assertSafeColumnRef(column ?? config.timestamps.defaultOrderColumn, 'latest(column)') | |
| 5963 | 6020 | text = SQL_PATTERNS.ORDER_BY.test(text) |
| 5964 | 6021 | ? `${text}, ${col} DESC` |
| 5965 | 6022 | : `${text} ORDER BY ${col} DESC` |
| @@ -5967,7 +6024,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial | ||
| 5967 | 6024 | return this as any |
| 5968 | 6025 | }, |
| 5969 | 6026 | oldest(column?: any) { |
| 5970 | const col = column ?? config.timestamps.defaultOrderColumn | |
| 6027 | const col = assertSafeColumnRef(column ?? config.timestamps.defaultOrderColumn, 'oldest(column)') | |
| 5971 | 6028 | text = SQL_PATTERNS.ORDER_BY.test(text) |
| 5972 | 6029 | ? `${text}, ${col} ASC` |
| 5973 | 6030 | : `${text} ORDER BY ${col} ASC` |
| @@ -6126,8 +6183,8 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial | ||
| 6126 | 6183 | // Compose with any existing GROUP BY so chained calls add columns |
| 6127 | 6184 | // instead of emitting a second clause. |
| 6128 | 6185 | 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(', ')}` | |
| 6131 | 6188 | built = null |
| 6132 | 6189 | } |
| 6133 | 6190 | return this as any |
| @@ -6969,6 +7026,9 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial | ||
| 6969 | 7026 | return makeSelect<any>(table, columns as string[]) as unknown as SelectQueryBuilder<DB, TTable, any, TTable> |
| 6970 | 7027 | }, |
| 6971 | 7028 | 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)') | |
| 6972 | 7032 | return makeSelect<TTable>(table) |
| 6973 | 7033 | }, |
| 6974 | 7034 | selectFromSub(sub, alias) { |