ReviewOS

stacks/bun-query-builder

UPDATE/DELETE silently drop the predicate and write every row when where() gets a SqlFragment, undefined, or null

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

Found on main @ 7299d05, verified by execution against live Postgres 17.

updateTable().where() and deleteFrom().where() silently discard the predicate for several argument shapes and then run unrestricted. No error, and the returned affected-row count reports the full-table write as success.

The SqlFragment case is the serious one: it is a typed, exported, documented call.

Repro

// table has 4 rows
await db.deleteFrom('t').where(undefined).execute()   // -> deletes all 4, returns 4
await db.deleteFrom('t').where(null).execute()        // -> deletes all 4, returns 4

await db.updateTable('t').set({ name: 'X' }).where(raw('id = 1')).execute()
// -> rewrites all 4 rows. Expected 1.

Measured (PG 17, 4 rows, reseeded between each):

callrows leftreturned
deleteFrom(t).where(undefined)0 / 44
deleteFrom(t).where(null)0 / 44
deleteFrom(t).where({})4 / 4throws syntax error at end of input
updateTable(t).set(v).where(raw('id = 1'))4 / 4 rewritten

Note the contrast with the last row of that table: where({}) — the case reported in #1095 — is the safe one. It throws and leaves the table intact. The shapes above do not.

Why raw() is not an exotic call

raw is exported from the package index, and the update builder's own declared signature invites it:

// client.ts:1979-1983
where: <K extends keyof DB[TTable]['columns'] & string>(
  expr: WhereExpression<DB[TTable]['columns']> | K | SqlFragment,   // <- SqlFragment
  op?: WhereOperator,
  value?: WhereValue<DB[TTable]['columns'][K]>,
) => UpdateQueryBuilder<DB, TTable>

So where(raw('id = 1')) typechecks, reads correctly, passes review, and destroys the table.

Cause

Both builders fall through every branch and return without appending anything.

Update (client.ts:6890-6976): the 3-arg branch is guarded by op !== undefined, so a lone SqlFragment skips it. The object branch at :6949 is guarded by !('raw' in expr), which excludes a raw fragment by construction. Nothing else matches, so control reaches the bare return this at :6976 — statement unchanged, no WHERE.

Delete (client.ts:7169): falls through to built = applyWhere(({} as any), ensureDelBuilt(), expr), an empty condition object, which appends nothing.

undefined/null reach the same fall-through in both.

Note

This is the same class of defect as #1083, where the select builder's silent return this fall-through was removed for exactly this reason. The write builders never got that change — the same "writes were left behind when the select builder was fixed" pattern as #1013/#1015.

Suggested fix

Throw on an argument shape the builder cannot turn into a predicate, rather than returning this unchanged. A write builder is the wrong place to interpret "I could not understand your filter" as "match every row". The SqlFragment shape should additionally be supported on the write path, since the type already promises it.

Worth pairing with a test that asserts the surviving row count, not just the emitted SQL — the emitted SQL here is valid, which is why nothing caught this.

Sign in to comment on this issue.