ReviewOS

stacks/bun-query-builder

Clause placement scans SQL text, so a raw fragment can swallow the WHERE or the JOIN and the query silently returns the wrong rows

#1121
Closed glennmichael123 opened this 22 days ago · 0 comments
22 days ago

Found on main @ 9c496fa, all four cases verified by execution against sqlite.

The select builder decides where in the statement a clause goes by scanning the SQL text for keywords. The scanners are paren-depth aware, which handles subqueries — but a raw fragment is arbitrary caller text, and a keyword inside a string literal or a comment is not a clause.

When the scan lands inside one, the clause is spliced into the literal. The statement stays valid, so nothing throws.

1. The predicate is written into a string literal, and the query loses its WHERE

firstTailIndex / TAIL_CLAUSE at client.ts:3314, used for the splice at :3353.

db.selectFrom('users').selectRaw(raw("'a limit 3 b' as t")).where('id', '=', 1)
SELECT *, 'a WHERE id = $1 limit 3 b' as t FROM users

The limit inside the literal is taken for a LIMIT clause, so the predicate is spliced before it — inside the quotes. The statement has no WHERE. Executed against a 3-row table it returns all 3; the control without the literal returns 1.

Nothing errors. $1 inside a literal is not a placeholder, and the surplus bound parameter is ignored rather than rejected.

This is the one I would fix first. A filter silently not applying on a read is the shape that leaks rows across a tenant or a permission boundary.

2. The JOIN is written into a string literal, and the join silently does not happen

insertJoin at client.ts:3504 — its own regex, same blind spot.

db.selectFrom('users').selectRaw(raw("'a where b' as tag")).innerJoin('posts', 'posts.user_id', '=', 'users.id')
SELECT *, 'a INNER JOIN posts ON posts.user_id = users.id where b' as tag FROM users

Returns 3 unjoined rows; the control returns 1. No parameters are involved here at all, so there is nothing that could even in principle catch it.

That regex also matches single-space 'GROUP BY'/'ORDER BY' literals, so a fragment containing GROUP\n BY is missed — an independent weakness in the same function.

3. A SQL comment is promoted to a clause

reorderSelectClauses / computeReorderedClauses at client.ts:2811, which runs on every built select.

This scanner is string-literal aware — it tracks ', " and backticks including doubled quotes, so cases 1 and 2 are genuinely blocked here. It is not comment aware:

db.selectFrom('users').orderByRaw(raw('id desc /* where clause */')).limit(5)
SELECT * FROM users where clause */ ORDER BY id desc /* LIMIT 5

The where inside the comment is treated as the start of a WHERE clause, the statement is cut there, and the fragments are re-emitted in clause order — which moves the comment's opening /* after its closing */. Postgres dollar-quoting ($$…$$) is unhandled for the same reason.

Root cause

Three scanners, three different levels of SQL-awareness, none of them complete:

siteparen depthstring literalscomments
firstTailIndex :3314yesnono
insertJoin :3504yesnono
computeReorderedClauses :2811yesyesno

Every one of them is asking "where does the caller's SQL end and my clause begin?" — a question the builder already knows the answer to, because it composed the statement.

Suggested direction

Same move as #1113: keep the structure instead of re-deriving it from the text. The builder knows the select list, the FROM, the joins, the predicate and the tail as separate pieces; rendering them in order needs no scan at all. If a full restructure is too big, the narrow fix is one shared tokenizer that skips literals, comments and dollar-quoted strings, used by all three — the table above is three partial implementations of it.

Found by an automated sweep of every text-based clause inference in src/, prompted by #1113. Each case above I reproduced myself before filing; the emitted SQL and row counts are from those runs.

Sign in to comment on this issue.