ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): refuse a write whose predicate the builder cannot read

#1109
Merged glennmichael123 wants to merge fix/1101-silent-full-table-write into main
2 files +251 -5
packages/bun-query-builder/src/client.tsmodified+69-5
@@ -7053,9 +7053,32 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
70537053 return this
70547054 }
70557055
7056 // A lone SQL fragment: `where(raw('id = 1'))`, `where(sql\`...\`)`.
7057 // The declared signature accepts SqlFragment, so this is an
7058 // advertised call — and it matched no branch, fell through to the
7059 // bare `return this` below, and left the UPDATE with no WHERE at all.
7060 // Every row was rewritten, silently, and the full count was reported
7061 // as success. Handled before the object branch because a bound
7062 // expression is an object and would otherwise be read as the column
7063 // map `{ sql: ... }`. See #1101.
7064 if (isRawExpression(expr)) {
7065 sqlText = `${sqlText} ${getWhereKeyword()} ${expr.raw}`
7066 built = _sql.unsafe(sqlText, params)
7067 return this
7068 }
7069 if (isBoundSqlExpression(expr)) {
7070 const rendered = renderBoundSqlExpression(expr, params.length + 1)
7071 sqlText = `${sqlText} ${getWhereKeyword()} ${rendered.text}`
7072 params.push(...rendered.parameters)
7073 built = _sql.unsafe(sqlText, params)
7074 return this
7075 }
7076
70567077 // Handle object format: where({ column: value })
7057 if (expr && typeof expr === 'object' && !('raw' in expr)) {
7078 if (expr && typeof expr === 'object') {
70587079 const keys = Object.keys(expr)
7080 if (keys.length === 0)
7081 throw new TypeError('[query-builder] updateTable.where({}): an empty object is not a filter. Pass a condition, or drop the where() call if updating every row is intended.')
70597082 const len = keys.length
70607083 const baseIdx = params.length
70617084 const conditions: string[] = Array.from({ length: len })
@@ -7065,8 +7088,19 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
70657088 }
70667089 sqlText = `${sqlText} ${getWhereKeyword()} ${conditions.join(' AND ')}`
70677090 built = _sql.unsafe(sqlText, params)
7091 return this
70687092 }
7069 return this
7093
7094 // Nothing above could turn this into a predicate. Refuse it.
7095 //
7096 // Returning `this` unchanged meant `where(undefined)` and
7097 // `where(null)` produced an UPDATE with no WHERE, so a filter the
7098 // caller believed they had applied rewrote the whole table without
7099 // an error. A write builder is the wrong place to read "I could not
7100 // understand your filter" as "match every row". The select builder
7101 // dropped this same fall-through in #1083; the write builders kept
7102 // it. See #1101.
7103 throw new TypeError(`[query-builder] updateTable.where(): expected a condition, got ${expr === null ? 'null' : typeof expr}. Refusing to run an UPDATE with no WHERE — drop the where() call if updating every row is intended.`)
70707104 },
70717105 whereNull(column: string) {
70727106 const keyword = SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'
@@ -7246,9 +7280,31 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
72467280 built = null
72477281 return this
72487282 }
7283 // A lone SQL fragment, handled before the object branch for the same
7284 // reason as in updateTable.where: a bound expression is an object and
7285 // would otherwise be read as the column map `{ sql: ... }`. Rendering
7286 // it here also fixes it — routed through applyWhere() below, a
7287 // fragment was interpolated as a bound value, so Postgres rejected
7288 // `where(raw('id = 1'))` with "invalid input syntax for type
7289 // boolean". The signature accepts SqlFragment, so it has to work.
7290 if (isRawExpression(expr)) {
7291 sqlText += ` ${getWhereKeyword()} ${expr.raw}`
7292 built = null
7293 return this
7294 }
7295 if (isBoundSqlExpression(expr)) {
7296 const rendered = renderBoundSqlExpression(expr, delParams.length + 1)
7297 sqlText += ` ${getWhereKeyword()} ${rendered.text}`
7298 delParams.push(...rendered.parameters)
7299 built = null
7300 return this
7301 }
7302
72497303 // Object format: where({ id: 1 })
7250 if (expr && typeof expr === 'object' && !('raw' in expr)) {
7304 if (expr && typeof expr === 'object') {
72517305 const keys = Object.keys(expr)
7306 if (keys.length === 0)
7307 throw new TypeError('[query-builder] deleteFrom.where({}): an empty object is not a filter. Pass a condition, or drop the where() call if deleting every row is intended.')
72527308 const conditions: string[] = []
72537309 for (const key of keys) {
72547310 const paramIndex = delParams.length + 1
@@ -7259,8 +7315,16 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
72597315 built = null
72607316 return this
72617317 }
7262 built = applyWhere(({} as any), ensureDelBuilt(), expr)
7263 return this
7318
7319 // Nothing above could turn this into a predicate.
7320 //
7321 // This previously fell through to applyWhere({}, ..., expr), whose
7322 // first line is `if (!expr) return q` — so `where(undefined)` and
7323 // `where(null)` appended nothing and the DELETE removed every row,
7324 // returning the count as though the filter had been honoured. See
7325 // #1101, and #1083 where the select builder lost the same
7326 // fall-through.
7327 throw new TypeError(`[query-builder] deleteFrom.where(): expected a condition, got ${expr === null ? 'null' : typeof expr}. Refusing to run a DELETE with no WHERE — drop the where() call if deleting every row is intended.`)
72647328 },
72657329 whereNull(column: string) {
72667330 sqlText += ` ${getWhereKeyword()} ${quoteId(String(column))} IS NULL`
packages/bun-query-builder/test/client.write-predicate-required.test.tsadded+182-0
@@ -0,0 +1,182 @@
1/**
2 * A write must never run without the predicate the caller supplied.
3 * stacksjs/bun-query-builder#1101 (and the empty-object half of #1095).
4 *
5 * `updateTable(...).where(x)` and `deleteFrom(...).where(x)` accepted argument
6 * shapes they could not turn into a predicate and then returned `this`
7 * unchanged. The statement kept no WHERE at all, so it ran against every row
8 * and reported the full affected count as success:
9 *
10 * deleteFrom(t).where(undefined) -> deleted all 4 rows
11 * updateTable(t).set(v).where(raw('id = 1')) -> rewrote all 4 rows
12 *
13 * The `raw()` case is the serious one, because it is not a mistake the types
14 * catch: the declared signature is `WhereExpression | K | SqlFragment` and
15 * `raw` is a public export, so that call reads correctly and passes review.
16 *
17 * Two different remedies, deliberately:
18 * - a fragment is SUPPORTED, because the signature promises it,
19 * - anything the builder genuinely cannot read is REFUSED, because a write
20 * builder is the wrong place to treat "I could not understand your filter"
21 * as "match every row".
22 *
23 * `where({})` throws with a message naming the problem instead of the SQL
24 * syntax error it used to produce — that is #1095, whose reported symptom was
25 * always the safe one.
26 *
27 * Every assertion below checks SURVIVING ROWS, not just that a call threw. The
28 * bug was damage, so the table is the thing worth asserting on.
29 */
30
31import { mkdtempSync, rmSync } from 'node:fs'
32import { tmpdir } from 'node:os'
33import { join } from 'node:path'
34import { Database } from 'bun:sqlite'
35import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
36import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src'
37import { config, setConfig } from '../src/config'
38import { resetConnection } from '../src/db'
39
40const MODELS = {
41 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
42} as any
43
44let dir: string
45let dbPath: string
46let snapshot: { dialect: string, database: Record<string, unknown> }
47
48function qb(): any {
49 return createQueryBuilder({
50 schema: buildDatabaseSchema(MODELS),
51 meta: buildSchemaMeta(MODELS),
52 autoMigration: { enabled: false } as any,
53 })
54}
55
56function rows(): Array<{ id: number, name: string }> {
57 const probe = new Database(dbPath)
58 const out = probe.query('SELECT id, name FROM t ORDER BY id').all() as any[]
59 probe.close()
60 return out
61}
62
63beforeEach(() => {
64 snapshot = { dialect: config.dialect, database: { ...config.database } }
65 dir = mkdtempSync(join(tmpdir(), 'qb-1101-'))
66 dbPath = join(dir, 'writes.sqlite')
67 const seed = new Database(dbPath, { create: true })
68 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)')
69 seed.run(`INSERT INTO t (id, name) VALUES (1,'a'),(2,'b'),(3,'c'),(4,'d')`)
70 seed.close()
71 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
72 resetConnection()
73})
74
75afterEach(() => {
76 config.dialect = snapshot.dialect as any
77 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
78 Object.assign(config.database, snapshot.database)
79 resetConnection()
80 rmSync(dir, { recursive: true, force: true })
81})
82
83describe('DELETE refuses to run without a predicate (#1101)', () => {
84 for (const [label, value] of [['undefined', undefined], ['null', null]] as const) {
85 it(`where(${label}) throws and leaves every row in place`, () => {
86 // Synchronously, at the point of the mistake — no statement is ever built.
87 expect(() => qb().deleteFrom('t').where(value as any))
88 .toThrow(/expected a condition/)
89
90 // The assertion that matters: all four rows are still here.
91 expect(rows()).toHaveLength(4)
92 })
93 }
94
95 it('where({}) names the problem instead of emitting a broken statement', () => {
96 expect(() => qb().deleteFrom('t').where({}))
97 .toThrow(/an empty object is not a filter/)
98 expect(rows()).toHaveLength(4)
99 })
100})
101
102describe('UPDATE refuses to run without a predicate (#1101)', () => {
103 for (const [label, value] of [['undefined', undefined], ['null', null]] as const) {
104 it(`where(${label}) throws and rewrites nothing`, () => {
105 expect(() => qb().updateTable('t').set({ name: 'X' }).where(value as any))
106 .toThrow(/expected a condition/)
107
108 expect(rows().filter(r => r.name === 'X')).toHaveLength(0)
109 })
110 }
111
112 it('where({}) names the problem instead of emitting a broken statement', () => {
113 expect(() => qb().updateTable('t').set({ name: 'X' }).where({}))
114 .toThrow(/an empty object is not a filter/)
115 expect(rows().filter(r => r.name === 'X')).toHaveLength(0)
116 })
117})
118
119describe('a SQL fragment is honoured, not dropped (#1101)', () => {
120 // The signature accepts SqlFragment, so these have to work — refusing them
121 // would swap a silent catastrophe for a broken advertised API.
122
123 it('DELETE where(raw(...)) removes only the matching row', async () => {
124 await qb().deleteFrom('t').where(raw('id = 1')).execute()
125
126 // Previously this threw on Postgres (the fragment was bound as a value)
127 // and, on the update side, wiped the table.
128 expect(rows().map(r => r.id)).toEqual([2, 3, 4])
129 })
130
131 it('UPDATE where(raw(...)) rewrites only the matching row', async () => {
132 await qb().updateTable('t').set({ name: 'X' }).where(raw('id = 1')).execute()
133
134 const after = rows()
135 expect(after.filter(r => r.name === 'X').map(r => r.id)).toEqual([1])
136 expect(after).toHaveLength(4)
137 })
138
139 it('DELETE honours a parameterised fragment', async () => {
140 await qb().deleteFrom('t').where({ sql: 'id = ?', parameters: [2] } as any).execute()
141 expect(rows().map(r => r.id)).toEqual([1, 3, 4])
142 })
143
144 it('UPDATE honours a parameterised fragment', async () => {
145 await qb().updateTable('t').set({ name: 'X' }).where({ sql: 'id = ?', parameters: [2] } as any).execute()
146 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([2])
147 })
148
149 it('does not mistake a bound fragment for a column map', async () => {
150 // `{ sql, parameters }` is an object, so an ordering slip would render it
151 // as `"sql" = ? AND "parameters" = ?` and match nothing — silently.
152 await qb().deleteFrom('t').where({ sql: 'id = ?', parameters: [3] } as any).execute()
153 expect(rows().map(r => r.id)).toEqual([1, 2, 4])
154 })
155})
156
157describe('ordinary writes are unaffected (#1101)', () => {
158 it('DELETE with an object filter still removes exactly its rows', async () => {
159 await qb().deleteFrom('t').where({ id: 1 }).execute()
160 expect(rows().map(r => r.id)).toEqual([2, 3, 4])
161 })
162
163 it('UPDATE with an object filter still rewrites exactly its rows', async () => {
164 await qb().updateTable('t').set({ name: 'X' }).where({ id: 2 }).execute()
165 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([2])
166 })
167
168 it('the 3-arg form still works on both builders', async () => {
169 await qb().updateTable('t').set({ name: 'X' }).where('id', '=', 3).execute()
170 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([3])
171
172 await qb().deleteFrom('t').where('id', '=', 4).execute()
173 expect(rows().map(r => r.id)).toEqual([1, 2, 3])
174 })
175
176 it('an intentional unfiltered write is still possible without where()', async () => {
177 // Refusing an unreadable filter must not remove the ability to write the
178 // whole table on purpose — the point is that it has to be explicit.
179 await qb().deleteFrom('t').execute()
180 expect(rows()).toHaveLength(0)
181 })
182})