ReviewOS

stacks/bun-query-builder

security: SQL injection in identifier + operator interpolation (Stacks audit)

#1009
Closed glennmichael123 opened this 2 months ago ·
glennmichael123 2 months ago

Cross-filed from a Stacks-side security audit (stacksjs/stacks#1858) — the bun-query-builder runtime has several SQL-injection vectors when identifiers or operators come from user input. The Stacks framework has shipped TypeScript surface tightening + identifier/operator validators as defense-in-depth (stacksjs/stacks@b2b587d), but the underlying construction paths need to be fixed here.

Reachability: any consumer that passes a column name or operator from req.body / req.query to the methods below produces user-controlled SQL.

Findings

IDFile:line (dist)Issue
Q-1dist/src/index.js:7507, 7528, 7556whereHas / has / whereDoesntHave quote string values with naked '…' and interpolate operator raw.
Q-2dist/src/index.js:19494, 19470, 19550, 19567ModelQueryBuilder.increment / decrement / pluck / aggregate / count interpolate column + _definition.table raw.
Q-3dist/src/index.js:7625, 8377, 8707, 8738, 8743whereRaw / selectRaw / groupByRaw / havingRaw / orderByRaw accept any string.
Q-4dist/src/index.js:8383, 8389whereColumn / orWhereColumn interpolate left, right, and operator raw.
Q-5dist/src/index.js:8243-8256whereJsonPath interpolates path and op raw on every dialect.
Q-6dist/src/index.js:8154, 8186, 8454, 8514where(string, op, value) parameterises the value but interpolates column + operator raw. Object form where(req.body) exposes column injection.
Q-7dist/src/index.js:9496SQLite identifier quoting is a no-op ((id) => id). SQLite is the typical default driver.
Q-8dist/src/index.js:7306-7308applyCondition is / is not cases interpolate val directly.
Q-9dist/src/index.js:8210-8220whereNull / whereNotNull interpolate column raw.
Q-10dist/src/index.js:8573-8606orderBy / orderByDesc / latest / oldest / reorder / inRandomOrder interpolate column raw.

Recommended fixes

  • Identifier paths: route through validateIdentifier (already defined internally) or accept only model-attribute keys; reject anything else.
  • Operator paths: constrain at runtime to the documented WhereOperator union.
  • *Raw methods: change the public TS signatures to accept only a RawBuilder<T> tagged-template fragment, never a bare string. Stacks shipped this on its ORM-side facade in stacksjs/stacks@b2b587d — whereRaw: (raw: RawBuilder<unknown>) => M.
  • SQLite quoting: emit "-quoted identifiers with internal-quote doubling (id.replace(/"/g, '""')) — same shape as the Postgres branch.

INVESTIGATE

  • The bundled runtime uses sql([subquerySQL]) (e.g. dist/src/index.js:7994, 8029, 8062, 8095) to re-flag previously-built raw strings as a Bun.SQL fragment. Confirm Bun.SQL's behaviour on array contents — if it's tightened to escape values in the array form, every whereHas/has/doesntHave path breaks.

Filed upstream so the dist code gets the real fix.

glennmichael123 9 days ago

Most identifier / operator interpolation paths fixed. Closed across 5 commits on main (9983c7f → 426f12d):

FindingCommitNote
Q-7 SQLite identifier quoting no-op9983c7fAll three quoteId arms (insertInto, updateTable, deleteFrom) now emit "col"/`col` with internal quote-doubling per dialect.
Q-2 ModelQueryBuilder.increment / decrement / pluck / aggregate column raw9983c7fNew assertValidIdentifier(name, context) helper in orm.ts gates every column arg.
Q-9 whereNull / whereNotNull / or… column raw9983c7fSame validator applied.
Q-10 orderBy / orderByDesc / orderByAsc column raw9983c7fSame validator; also strict-validates direction against the 'asc' | 'desc' union.
Q-1 whereHas / whereDoesntHave callback naked single quotes + raw op2e81638New assertSafeWhereOperator(op, context) + formatSubqueryValue(val) helpers. Strings now go through SQL single-quote doubling; numbers/booleans/null pass through; everything else rejected.
Q-4 whereColumn left/right/op raw2e81638Both sides through validateIdentifier.
Q-5 whereJsonPath path/op raw4b0f857Op via assertSafeWhereOperator; path through a stricter regex that permits dialect-specific path syntax but rejects ;, ", (, ).
Q-6 where([col, op, val]) column + op raw, plus object-form where({col: val}) key rawceaaebaTuple form validates column + dispatches operator through a closed switch (default branch throws); object form validates keys.
Q-8 is/is not interpolated val directlyceaaebaNow only accepts null/undefined and emits the canonical IS NULL/IS NOT NULL form.

Still open

  • Q-3 — whereRaw/selectRaw/groupByRaw/havingRaw/orderByRaw accept any string at the type level. This is a public API surface change; the Stacks-side ORM façade already tightened whereRaw to require RawBuilder<unknown> (stacksjs/stacks@b2b587d1d). Bun-query-builder itself would benefit from the same tightening at the SelectQueryBuilder interface.

All 1150 tests pass after every commit.

glennmichael123 9 days ago

Q-3 closed in d122bf6 — all SQL-injection findings from the audit now have at least a defense-in-depth measure shipped.

The *Raw methods (whereRaw, selectRaw, groupByRaw, havingRaw, orderByRaw) move from fragment: any → fragment: SqlFragment where SqlFragment = object. Bare-string callers fail to compile; callers using Bun's sql\...`tagged-template (which returns an object) pass through unchanged. Runtime guard emits a once-per-call-site console.warn foras any` casts that bypass the type system, with the warning marked as "will become a hard error in a future release."

Full Q-* closing-out

FindingCommitStatus
Q-1 whereHas value escape + op allow-list2e81638✅
Q-2 increment/decrement/pluck/aggregate column validation9983c7f✅
Q-3 *Raw type signaturesd122bf6✅
Q-4 whereColumn validation2e81638✅
Q-5 whereJsonPath path + op validation4b0f857✅
Q-6 where(tuple) + where(object) validationceaaeba✅
Q-7 SQLite identifier quoting9983c7f✅
Q-8 is/is not value validationceaaeba✅
Q-9 whereNull/whereNotNull validation9983c7f✅
Q-10 orderBy + direction validation9983c7f✅

Tests: 1150/1150 passing after every commit.

Closing.

Sign in to comment on this issue.