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
docs/raw-queries.mdmodified+42-12
@@ -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:
@@ -235,24 +258,31 @@ stats.forEach((s) => {
235258
236259```
237260
238## Prepared Statements
261## Repeated Queries
239262
240Use prepared statements for repeated queries:
263`unsafe` is not a prepared-statement handle. It returns a query that runs when you
264await it and resolves to an array of rows, so there is no `.get()` to call per
265execution and nothing to finalize:
241266
242267```typescript
268// Run it once per set of values, awaiting each. Postgres placeholders shown —
269// see "Placeholder style is the connection's, not the builder's" above.
270const user1 = await db.unsafe('SELECT * FROM users WHERE id = $1', [1])
271const user2 = await db.unsafe('SELECT * FROM users WHERE id = $1', [2])
243272
244// Prepare a statement
245const stmt = db.unsafe('SELECT * FROM users WHERE id = ?')
246
247// Execute multiple times efficiently
248const user1 = await stmt.get([1])
249const user2 = await stmt.get([2])
250const user3 = await stmt.get([3])
273// Each resolves to an array of rows — index in, or use the builder's first().
274console.log(user1[0]?.name)
275```
251276
252// Finalize when done
253stmt.finalize()
277Keep the SQL text identical across those calls and vary only the bindings. That is
278what lets the driver reuse its plan, and it is also what keeps the values
279parameterized rather than concatenated in.
254280
255```
281::: tip
282The object `unsafe` returns before you await it is the driver's own, and its shape
283differs between `bun:sqlite` and `Bun.sql`. Await it and work with the rows; don't
284reach for methods on the unawaited value.
285:::
256286
257287## Transaction with Raw Queries
258288
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+152-0
@@ -0,0 +1,152 @@
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. A throw here is the false positive that
99 // would matter, so this asserts nothing is thrown at all.
100 expect(refusal(() => qb().unsafe('SELECT $2 AS a, $1 AS b', ['ONE', 'TWO']))).toBeNull()
101 })
102
103 it('accepts a jsonb ? operator when $n is also present', () => {
104 // `data ? 'key'` is a real Postgres operator, not a placeholder. Keying the
105 // check on `?` instead of on the absence of $n would reject this.
106 expect(refusal(() => qb().unsafe('SELECT * FROM t WHERE data ? \'key\' AND id = $1', [7]))).toBeNull()
107 })
108
109 it('never inspects a query with no bindings', () => {
110 // Raw DDL, and the `?` in a literal that is data rather than a placeholder.
111 expect(refusal(() => qb().unsafe('CREATE TABLE t (id int)'))).toBeNull()
112 expect(refusal(() => qb().unsafe('SELECT * FROM users WHERE name = \'what?\''))).toBeNull()
113 expect(refusal(() => qb().unsafe('SELECT * FROM users WHERE email = ?', []))).toBeNull()
114 })
115 })
116
117 describe('other dialects are unaffected', () => {
118 it('SQLite and MySQL still take ?', () => {
119 for (const d of ['sqlite', 'mysql'] as const) {
120 config.dialect = d as any
121 expect(refusal(() => qb().unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', ['a@b.c']))).toBeNull()
122 }
123 })
124 })
125})
126
127/**
128 * The message `fn` threw, or null if it returned.
129 *
130 * `unsafe` returns a LAZY query: it reaches a database when something awaits it,
131 * and not before. So these cases call it and discard the result, which exercises
132 * the guard while executing nothing.
133 *
134 * That is load-bearing, not tidiness. An earlier version of this helper did
135 * `void Promise.resolve(out).catch(…)` to ignore driver errors — but
136 * `Promise.resolve` calls `.then` on the thenable, so every "leaves it alone"
137 * case below really ran, `CREATE TABLE t (id int)` included, against whatever
138 * database the environment pointed at. In CI that is the shared `test_db`, and
139 * the unawaited queries outlived the file and timed out the next one's
140 * `beforeAll` (`closeConnection` → `resetDatabase` → `generateMigration`, on a
141 * 5s hook budget). Returning the message instead of swallowing it also means an
142 * unexpected throw fails loudly rather than passing as "not refused".
143 */
144function refusal(fn: () => unknown): string | null {
145 try {
146 fn()
147 return null
148 }
149 catch (error) {
150 return (error as Error).message
151 }
152}