ReviewOS

stacks/bun-query-builder

SQL injection: orderBy/groupBy/select interpolate column names unvalidated, while where/join validate them

#1181
Closed glennmichael123 opened this 8 days ago ·
8 days ago

The select builder interpolates caller-supplied column names straight into SQL on eight methods. A ?sort= parameter reaching orderBy() executes arbitrary SQL.

The same builder already validates identifiers on the WHERE and join family, so this is a gap in an existing defence rather than a missing one.

Proof, live Postgres

table exists before : true
SQL built           : SELECT * FROM sec_users ORDER BY id; DROP TABLE sec_users; -- ASC
table exists after  : false

That is one call — db.selectFrom('sec_users').orderBy(sortParam).get() — with sortParam = "id; DROP TABLE sec_users; --". The stacked statement runs; the table is gone. Not a syntax error, not a blind oracle: arbitrary statement execution.

Affected

Probed with "id; DROP TABLE users; --" as the column name:

methodresult
orderByinjected
orderByDescinjected
latestinjected
oldestinjected
groupByinjected
selectinjected
addSelectinjected
distinctOninjected
where (column and object key)throws
whereInthrows
whereNullthrows
jointhrows

orderBy is the one that matters most: a sort column is the single most common piece of user input that is legitimately a column name, so it is the parameter most likely to be passed through from a request.

Why this is a gap, not an absence

client.ts already has validateIdentifier (line 474) and calls it in ~20 places — where(column), where(object key), whereIn, whereNull, join, the wherePivot auto-join, and every relationship subquery. It is simply not called from the SELECT / ORDER BY / GROUP BY paths.

The ORM layer already closed exactly this hole. orm.ts has assertValidOrderByColumn, applied at orm.ts:2354, whose comment reads:

The column name is interpolated RAW into SQL at the build sites, so a malformed/malicious identifier here is a SQL-injection / column-enumeration vector. stacksjs/stacks#1858.

So Model.query().orderBy(input) is safe and db.selectFrom(...).orderBy(input) is not. Verified the ORM rule rejects the payload:

accept "id"
accept "users.id"
REJECT "id; DROP TABLE users; --"
REJECT "(SELECT 1)"

Suggested fix

Call the existing validateIdentifier from the eight sites above, exactly as the WHERE family does. For orderBy specifically, port assertValidOrderByColumn's rule so the table-qualified table.column form keeps working — that form is common in ORDER BY and would otherwise regress.

Two things to decide:

  1. select and addSelect legitimately take expressions (COUNT(*) AS c, a + b). A strict identifier rule would break them. The raw tagged template already exists for that case and escapes interpolated values, so the strict rule plus raw for expressions is coherent — but it is a breaking change for anyone passing an expression string today.
  2. distinctOn is Postgres-only and takes column names only, so it can take the strict rule unchanged.

I would treat 1 as the only real design question; the ORDER BY / GROUP BY / distinctOn sites take the existing rule as-is.

Not affected

escapeStringLiteral (client.ts:687) handles values correctly, including doubling backslashes first on MySQL-family dialects so \' cannot break out. This issue is only about identifiers.

Happy to send the patch.

8 days ago

One more site, same root cause and same fix: selectFrom(table) is not validated either.

INJECTED selectFrom (table)   SELECT * FROM users; DROP TABLE users; --

Less commonly user-controlled than a sort column, but multi-tenant code does sometimes derive a table name from request context, and it interpolates the same way. Worth including in the same pass — that makes nine sites.

For completeness, these were probed with the same payload and are not affected:

THREW  limit(n)                expected non-negative integer
THREW  offset(n)               expected non-negative integer
THREW  whereJsonContains       Invalid identifier in whereJsonContains(column)
ok     inRandomOrder           ORDER BY RANDOM()   (no interpolation)

So the numeric clauses and the JSON path helpers already validate. The gap is specifically identifiers on the SELECT / FROM / ORDER BY / GROUP BY family.

Sign in to comment on this issue.