ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): unsafe() refuses ?-style bindings on Postgres

#1172
Merged glennmichael123 wants to merge fix/unsafe-placeholder-style into main
3 files +247 -13

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

docs/raw-queries.mdmodified+23-0
@@ -96,6 +96,29 @@ const result = await db.unsafe(`
9696Always prefer parameterized queries when possible to prevent SQL injection. Only use `unsafe` when absolutely necessary and never with user-provided input.
9797:::
9898
99### Placeholder style is the connection's, not the builder's
100
101When you pass bindings to `unsafe`, you are writing for the driver directly, so the
102placeholders have to be the ones that connection understands. Postgres numbers its
103parameters; SQLite and MySQL take `?`:
104
105```typescript
106// Postgres
107await db.unsafe('SELECT * FROM users WHERE email = $1 LIMIT 1', [email])
108
109// SQLite and MySQL
110await db.unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', [email])
111```
112
113Postgres has no `?` parameter — it reads the token as an operator and rejects the
114word that follows, so the error names whatever comes next (`LIMIT`, above) rather
115than the placeholder. Because the same call works on SQLite and MySQL, this shows
116up as a one-dialect failure. `unsafe` therefore refuses bindings on Postgres when
117the query has no `$n`, and names the real problem at the call site.
118
119A query with no bindings is never inspected, so raw DDL and a literal `?` inside a
120string are unaffected.
121
99122## Execute Raw SQL
100123
101124Execute non-query SQL statements:
packages/bun-query-builder/src/client.tsmodified+53-1
@@ -14,7 +14,7 @@ import { bunSql, getOrCreateBunSql, resetConnection } from './db'
1414import { resolvePivot } from './pivot'
1515import { singularizerFor } from './inflect'
1616import type { WhereTerm } from './sql-fragments'
17import { FALSE_PREDICATE, mapPlaceholders, renderInPredicate, renderWhereTerms, scanTopLevelKeywords } from './sql-fragments'
17import { countPlaceholders, FALSE_PREDICATE, mapPlaceholders, renderInPredicate, renderWhereTerms, scanTopLevelKeywords } from './sql-fragments'
1818
1919export { resetConnection }
2020
@@ -583,6 +583,55 @@ function renderRawFragment(fragment: unknown, context: string, extra: readonly u
583583 )
584584}
585585
586/**
587 * Refuse a raw query whose bindings the connection could never reach.
588 *
589 * Postgres has no `?` parameter. It parses the token as an operator, looks for a
590 * right operand, and rejects the next word — so
591 * `unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', [email])` reports a
592 * syntax error at `LIMIT`, 37 characters past the actual fault, while the bound
593 * value in `params` was never going to arrive. The same call passes on SQLite and
594 * MySQL, which both take `?`, so a cross-dialect suite goes red on one dialect
595 * only and the first hour goes into `LIMIT`. stacksjs/bun-query-builder#1171
596 *
597 * Keyed on the ABSENCE of `$n` rather than the presence of `?`, which is what
598 * makes it free of false positives:
599 *
600 * - `?` is a real Postgres operator — `data ? 'key'` on jsonb, and several of
601 * the geometric types — so a query carrying both `$1` and a `?` operator is
602 * legitimate, and seeing `$n` is enough to leave it alone.
603 * - A `?` inside a string literal (`WHERE name = 'what?'`) is not a placeholder
604 * either, and such a query would not be passing bindings in the first place.
605 *
606 * Bindings with no `$n` anywhere, meanwhile, cannot be a working query on
607 * Postgres whatever the text does: either a placeholder is written in the wrong
608 * style, or nothing references the values. Both are worth naming here rather
609 * than at a token the server picked.
610 *
611 * A call with no bindings is never inspected, so raw DDL keeps working untouched.
612 */
613function assertBindablePlaceholders(query: string, params: readonly unknown[] | undefined): void {
614 if (!params || params.length === 0) return
615 if (config.dialect !== 'postgres') return
616 // Not scanned for quoting: a `$1` buried in a string literal only costs this
617 // guard a catch, where a false positive would cost a working query.
618 if (/\$\d/.test(query)) return
619
620 // countPlaceholders skips `?` in quoted runs and comments, so this reports the
621 // ones that were actually meant as placeholders.
622 const marks = countPlaceholders(query)
623 const bindings = `${params.length} binding${params.length === 1 ? '' : 's'}`
624 const cause = marks > 0
625 ? `the query binds them with ${marks} \`?\` placeholder${marks === 1 ? '' : 's'}, which Postgres reads as an operator rather than a parameter`
626 : 'the query contains no $n placeholder for them to bind to'
627 throw new TypeError(
628 `[query-builder] unsafe(): ${bindings} supplied, but ${cause}. Postgres numbers `
629 + `its parameters, so write $1, $2 … in the order the bindings should bind: `
630 + `unsafe('SELECT * FROM users WHERE email = $1 LIMIT 1', [email]). `
631 + `\`?\` is correct on SQLite and MySQL, which is why this call may work there.`,
632 )
633}
634
586635// Module-scoped Set so we warn at most once per call site per process
587636// lifetime (previously this lived inside makeSelect, so it actually
588637// warned once per BUILDER — i.e. on every query).
@@ -8265,6 +8314,9 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
82658314 return false
82668315 },
82678316 unsafe(query: string, params?: any[]) {
8317 // Throws at the call site, before the connection sees a query whose
8318 // bindings it cannot bind — see assertBindablePlaceholders.
8319 assertBindablePlaceholders(query, params)
82688320 // Use this builder's connection. Transaction callbacks receive a
82698321 // builder whose `_sql` is the reserved transaction connection; routing
82708322 // raw SQL through the process-wide pool lets it escape the transaction
packages/bun-query-builder/test/client.unsafe-placeholders-refused.test.tsadded+145-0
@@ -0,0 +1,145 @@
1/**
2 * `unsafe()` refuses `?`-style bindings on Postgres — stacksjs/bun-query-builder#1171.
3 *
4 * `unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', [email])` typechecks,
5 * passes on SQLite and MySQL, and on Postgres reached the server as a bare `?`.
6 * Postgres parses that as an operator, hunts for a right operand and rejects the
7 * next word, so the error named `LIMIT` while the fault was the `?` 37 characters
8 * earlier — and the bound value was never going to arrive. Downstream that took
9 * out every login, password recovery, magic link and two-factor exchange in
10 * Stacks at once: 126 failures in one CI run, all from this one shape.
11 *
12 * The check is keyed on bindings arriving with no `$n` in the query, not on the
13 * presence of `?`, so a jsonb `?` operator alongside `$1` is still allowed
14 * through and raw DDL is never inspected.
15 */
16
17import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
18import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
19import { config } from '../src/config'
20
21function qb(): any {
22 const models = {
23 users: {
24 columns: {
25 id: { type: 'integer', isPrimaryKey: true },
26 email: { type: 'text' },
27 },
28 },
29 } as any
30 return createQueryBuilder<any>({
31 schema: buildDatabaseSchema(models),
32 meta: buildSchemaMeta(models),
33 autoMigration: { enabled: false } as any,
34 })
35}
36
37const REFUSED = /\[query-builder\] unsafe\(\)/
38
39describe('unsafe() placeholder style (#1171)', () => {
40 let dialect: string
41 beforeEach(() => { dialect = config.dialect })
42 afterEach(() => { config.dialect = dialect as any })
43
44 describe('Postgres refuses bindings it cannot bind', () => {
45 beforeEach(() => { config.dialect = 'postgres' as any })
46
47 it('throws on the exact call from the issue', () => {
48 expect(() => qb().unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', ['a@b.c'])).toThrow(REFUSED)
49 })
50
51 it('names the real problem rather than the token Postgres would pick', () => {
52 let message = ''
53 try {
54 qb().unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', ['a@b.c'])
55 }
56 catch (error) {
57 message = (error as Error).message
58 }
59 // The placeholder style the connection expects, and the one that was used.
60 expect(message).toContain('$1')
61 expect(message).toContain('1 binding supplied')
62 // It blames the placeholder, where the server's own error blamed `LIMIT`.
63 expect(message).toContain('`?` placeholder')
64 expect(message).not.toContain('syntax error')
65 // And it says why the same call passes elsewhere, which is the part that
66 // makes a one-dialect-only CI failure legible.
67 expect(message).toMatch(/SQLite and MySQL/)
68 })
69
70 it('throws a TypeError, synchronously, so the stack names the caller', () => {
71 // Not a rejected promise: the point is to fail where the query was written.
72 expect(() => qb().unsafe('SELECT ?', [1])).toThrow(TypeError)
73 })
74
75 it('counts the placeholders and pluralises', () => {
76 let message = ''
77 try {
78 qb().unsafe('SELECT * FROM users WHERE email = ? OR id = ?', ['a@b.c', 1])
79 }
80 catch (error) {
81 message = (error as Error).message
82 }
83 expect(message).toContain('2 bindings supplied')
84 expect(message).toContain('2 `?` placeholders')
85 })
86
87 it('refuses bindings that nothing in the query references', () => {
88 // No `?` either — the values still cannot arrive, and Postgres would have
89 // said "bind message supplies 1 parameters, but prepared statement requires 0".
90 expect(() => qb().unsafe('SELECT 1', [1])).toThrow(/no \$n placeholder/)
91 })
92 })
93
94 describe('Postgres leaves correct and unrelated queries alone', () => {
95 beforeEach(() => { config.dialect = 'postgres' as any })
96
97 it('accepts $n, in any order', () => {
98 // The shape db.d.ts documents. Throwing here would be the false positive
99 // that matters, so assert the guard specifically, not the connection.
100 expect(() => assertOnly(() => qb().unsafe('SELECT $2 AS a, $1 AS b', ['ONE', 'TWO']))).not.toThrow(REFUSED)
101 })
102
103 it('accepts a jsonb ? operator when $n is also present', () => {
104 // `data ? 'key'` is a real Postgres operator, not a placeholder.
105 expect(() => assertOnly(() => qb().unsafe('SELECT * FROM t WHERE data ? \'key\' AND id = $1', [7]))).not.toThrow(REFUSED)
106 })
107
108 it('never inspects a query with no bindings', () => {
109 // Raw DDL, and the `?` in a literal that is data rather than a placeholder.
110 expect(() => assertOnly(() => qb().unsafe('CREATE TABLE t (id int)'))).not.toThrow(REFUSED)
111 expect(() => assertOnly(() => qb().unsafe('SELECT * FROM users WHERE name = \'what?\''))).not.toThrow(REFUSED)
112 expect(() => assertOnly(() => qb().unsafe('SELECT * FROM users WHERE email = ?', []))).not.toThrow(REFUSED)
113 })
114 })
115
116 describe('other dialects are unaffected', () => {
117 it('SQLite and MySQL still take ?', () => {
118 for (const d of ['sqlite', 'mysql'] as const) {
119 config.dialect = d as any
120 expect(() => assertOnly(() => qb().unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', ['a@b.c'])))
121 .not.toThrow(REFUSED)
122 }
123 })
124 })
125})
126
127/**
128 * Run `fn` and swallow anything that is not this guard's refusal.
129 *
130 * These cases assert that the GUARD does not fire. Past it the call reaches a
131 * connection that no test here configures, so a driver error is expected and
132 * says nothing about the thing under test; only a `[query-builder] unsafe()`
133 * message would.
134 */
135function assertOnly(fn: () => unknown): void {
136 try {
137 const out = fn()
138 // A promise that rejects in the driver is not this guard's business either.
139 void Promise.resolve(out).catch(() => {})
140 }
141 catch (error) {
142 if (REFUSED.test((error as Error).message))
143 throw error
144 }
145}