sql(sqlite): pass positional bindings to bun:sqlite as one array, not spread - #35950
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesSQLite binding execution
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change preserves positional SQLite bindings as a single parameter list, preventing object or array values from rebinding other parameters or becoming silent NULLs. The affected behavior is covered by passing tests, and no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:05 PM PT - Aug 14th, 2026
@cirospaciari, your commit e232a4f is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the .apply → .call switch traces correctly through bun:sqlite's variadic wrappers and Database.run, and the empty-values path still works. Deferring to a human because this is a security-sensitive behavior change to SQL parameter binding.
What was reviewed
- Traced
stmt.all/values/raw(values)anddb.run(sql, values)throughsrc/js/bun/sqlite.ts— array arg0 hits the...argsbranch and reaches native as a single positional array. - Checked the
values.length === 0case — now goes through#raw.all([])instead of#allNoArgs(), same net effect. - Tests cover both
canReturnRowsbranches, all three result modes, and confirmsql([...])/sql({...})helpers are unaffected.
Extended reasoning...
Overview
Four-line fix in src/js/internal/sql/sqlite.ts: SQLiteQueryHandle.run previously spread the positional parameter array into bun:sqlite's variadic Statement.all/values/raw and Database.run via .$apply. Because those wrappers sniff args[0] to decide whether the first argument is a named-binding map or the full positional list, a user-controlled object or array in template slot 0 could rewrite every subsequent (trusted) parameter. The fix passes the array as a single argument via .$call, forcing the array-binding path so each element is bound by index and non-bindable elements hit native rebindValue's TypeError. Eight new tests in test/js/sql/sqlite-sql.test.ts cover the attack shapes and confirm the sql(...) helper forms still work.
Security risks
This is the security-relevant part of the PR. The change removes a parameter-injection vector; I verified against src/js/bun/sqlite.ts that passing the array as one argument always lands in the isArray(arg0) branch → #raw.all(values) / SQL.run(..., query, values), so an attacker-controlled element can no longer be reinterpreted as the binding structure. No new injection surface is introduced. The only user-visible fallout is that previously-accepted-but-wrong inputs (object/array/Date as a raw interpolated value) now throw instead of silently misbinding — which is the intended hardening.
Level of scrutiny
High. SQL parameter binding is exactly the kind of path where a subtle mistake becomes a data-exposure bug, and this PR changes user-facing behavior (throws where it previously returned rows). The mechanical change is tiny and I'm confident it's correct, but per policy a human should sign off on security-sensitive binding semantics and the intentional behavior change.
Other factors
- Verified the empty-array case:
stmt.all.$call(stmt, [])→#all([])→#raw.all([])(was#allNoArgs()before) — both bind zero params. - Tests assert both the throw and the absence of side effects (no row inserted), and include positive regression coverage for
sql([...])WHERE IN andsql({...})INSERT helpers. - No CODEOWNERS on these paths. No prior review comments to address.
- The PR description's follow-ups (JS-side
bindParamguard, hardeningbun:sqlite's own variadic API) are reasonable to leave out of scope here.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/internal/sql/sqlite.ts:252-258— Nit:stmt.finalize()is only reached on the success path, so whenstmt.all/values/rawthrows (which this PR now guarantees for object/array/Dateinterpolated values — the exact attacker input being hardened against) the preparedsqlite3_stmtlives until the JS wrapper is GC'd. SinceStatementimplementsSymbol.dispose,using stmt = db.prepare(sql);(or atry/finally) would pair the acquisition with its release. Pre-existing gap, but it sits on the exact lines edited and the PR widens the set of inputs reaching it.Extended reasoning...
What the gap is
In the
canReturnRowsbranch ofSQLiteQueryHandle.run:const stmt = db.prepare(sql); let result; if (mode === SQLQueryResultMode.values) { result = stmt.values.$call(stmt, values); // <-- fallible } else if (mode === SQLQueryResultMode.raw) { result = stmt.raw.$call(stmt, values); // <-- fallible } else { result = stmt.all.$call(stmt, values); // <-- fallible } ... stmt.finalize(); // <-- success-path only
stmt.finalize()is only reached if the.$callline does not throw. If it does, control jumps to the outercatchat the bottom ofrun(), which wraps and re-throws without access tostmt(block-scoped inside theif). The nativesqlite3_stmtis then only reclaimed when JSC garbage-collects theJSSQLStatementwrapper (~JSSQLStatementcallssqlite3_finalize).The specific code path that triggers it
This PR's explicit purpose is to make
stmt.all/values/raw.$call(stmt, values)throw aTypeErrorwhenever any interpolated value is a plain object, array, orDate. Before the PR, an object/array in the first slot was silently reinterpreted as the binding structure and did not throw; after the PR, it always reaches nativerebindValue, which throws"Binding expected string, TypedArray, boolean, number, bigint or null". So the very attacker input this PR is hardening against (${JSON.parse('{"0":2,"1":"bob"}')}in a SELECT) now takes the throw path and skips finalization.Why existing code doesn't prevent it
The outer
catchblock only wrapsSQLiteErrorand re-throws; it has no reference tostmt(which is scoped inside theif (parsedInfo.canReturnRows)block) and does not attempt cleanup. There is notry/finallyor RAII guard around the fallible.$call.Impact
Low. The
Statementwrapper goes out of scope immediately, so~JSSQLStatementfinalizes the native handle on the next GC — this is not a hard leak, and there is no correctness or security consequence. In a request loop where an attacker repeatedly sends{"0":..}payloads, unfinalized statements accumulate on the connection between GC cycles, but GC pressure bounds the growth. This is why it's a nit rather than a merge-blocker.How to fix
Statementimplements[Symbol.dispose](src/js/bun/sqlite.ts:336, callsfinalize()if not already finalized), so a one-token change suffices:using stmt = db.prepare(sql);
and drop the explicit
stmt.finalize(). Alternatively, wrap the fallible calls intry { ... } finally { stmt.finalize(); }.Step-by-step proof
-
sql`SELECT * FROM docs WHERE id = ${"{"}{0:2,1:"bob"}${"}"} AND tenant = ${"$"}{serverTenant}`→parsedInfo.canReturnRows === true` (SELECT). -
const stmt = db.prepare("SELECT * FROM docs WHERE id = ? AND tenant = ?")— nativesqlite3_stmtacquired. -
stmt.all.$call(stmt, [{0:2,1:"bob"}, "alice"])→bun:sqlitetakes the array path, iterates elements, nativerebindValuesees element 0 is a plain object → throwsTypeError. - Throw propagates past line 265 (
stmt.finalize()never runs) to the outercatch. -
err.name === "TypeError", not"SQLiteError", so thecatchre-throws unchanged.stmtis now unreachable but not finalized. - Native
sqlite3_stmtremains allocated until the next GC finalizes theJSSQLStatementwrapper.
Relation to this PR
This is pre-existing — the
.$applycall was already fallible for SQLite runtime errors and for non-first-slot type errors before this PR. But the PR edits these exact lines and, per REVIEW.md, "New early returns or fallible calls → re-audit everything acquired above them" and "Pair every acquisition with its release at the acquisition site — arm a Drop/RAII guard before any fallible call". The PR converts a class of previously-silent inputs into throws right here, so the throw path is now the expected outcome for the attacker scenario the PR is hardening against, which makes the missing release worth pairing in the same change. -
|
Good catch on the finalize-on-throw gap. Addressed in f8d1056: |
There was a problem hiding this comment.
The finalize-on-throw feedback from the earlier pass is addressed in f8d1056 — stmt.finalize() now runs from finally on both success and throw paths. The .$apply → .$call change checks out against bun:sqlite's dispatch (src/js/bun/sqlite.ts:242-291, 531-545): a single-array arg always routes to #raw.all(values) / SQL.run(..., values), so each element is bound positionally and non-bindable elements hit native rebindValue's TypeError. Deferring to a human since this is a security fix that also changes user-visible behavior (objects/arrays/Dates as direct template values now throw where they previously bound something).
What was reviewed:
- Verified
stmt.all/values/raw(arrayArg)anddb.run(sql, arrayArg)take the array-is-positional-list branch inbun:sqlite; emptyvaluesstill works. - Confirmed no other
.$apply(_, values)spread sites remain insrc/js/internal/sql/. finalize()infinallyruns afterresultis captured;resultis a detached JS array so use-after-finalize is fine.
Extended reasoning...
Overview
Four call sites in SQLiteQueryHandle.run change from .$apply(receiver, values) (spreads the positional-values array variadically) to .$call(receiver, values) (passes the array as a single argument). bun:sqlite's variadic wrappers inspect args[0]: when it's an array they forward it to native as the positional list; when it's a non-array object they treat it as a named/indexed binding map. Spreading meant a user-controlled object/array in template slot 0 became args[0] and hijacked the whole binding, silently discarding later trusted parameters. Passing the array whole means args[0] is always the adapter-constructed array, and each element reaches native rebindValue which rejects non-scalar types with a TypeError. A try/finally around the bind/execute call now guarantees stmt.finalize() on the throw path (my earlier review's only finding). Eight new tests cover SELECT/INSERT × object/array/Date, .values()/.raw() modes, and confirm the sql([...]) / sql({...}) helpers are unaffected.
Security risks
The PR closes a parameter-override hole; I couldn't find a way the new shape widens anything. I traced the four new call shapes through src/js/bun/sqlite.ts: #all/#values/#rawValues with args = [arrayOfValues] hit the isArray(arg0) branch and call #raw.all(arrayOfValues); Database.run(sql, arrayOfValues) likewise spreads to SQL.run(..., query, arrayOfValues). Native then iterates the array and type-checks each element. Empty values (queries with no interpolations) now passes [] instead of hitting the *NoArgs() fast path, but the full test file passes so that's handled. No other .$apply-spread-into-bind sites exist under src/js/internal/sql/.
Level of scrutiny
High — this is SQL parameter binding, and the fix intentionally turns previously-accepted inputs (${new Date()}, ${[...]}, ${{...}} as a plain value in slot 0) into thrown TypeErrors. That's the correct hardening and matches slots >0 and the postgres/mysql adapters, but it's a user-visible behavior change on a security boundary that a maintainer should sign off on rather than a bot.
Other factors
All prior review threads are resolved (comment-cop's long-comment flag, my finalize-on-throw note). PR evidence shows the 6 security tests fail on canary and the full sqlite-sql.test.ts passes on both debug+ASAN and release with the fix. The unrelated isDebug ? 1000 : 10000 iteration scaling in the memory test is a reasonable CI-speed adjustment.
SQLiteQueryHandle.run was using $apply to forward positional parameters, which let attacker-controlled objects/arrays override trusted bindings. Fix: Check if values is array and bind positionally instead of using $apply. Fixes oven-sh#35950
|
CI on the current head (e232a4f, build #96874): every finished test lane is green for this change; The one hard failure, Ready for review. |
f8d1056 to
b975d13
Compare
robobun
changed the title
SQLiteQueryHandle.run spread the positional binding array into
Statement.all/values/raw and Database.run. bun:sqlite reads the whole
binding set from its first argument, so an object or array interpolated
into the first slot (or given as the first element of an unsafe() array,
or as the first column of a sql() insert helper) was taken as the
binding set for every placeholder: an indexed-key object such as
{"0": 2, "1": "bob"} overrode the remaining trusted values, and any
other object silently bound NULL.
Pass the array itself, the same way named-parameter objects are already
passed. Non-bindable values in any slot now fail with bun:sqlite's
"Binding expected string, TypedArray, boolean, number, bigint or null".
b975d13 to
aec9cf5
Compare
|
Rebased onto main (aec9cf5). #37109 landed the Tests were extended with two more faces of the same bug: |
There was a problem hiding this comment.
LGTM — minimal, correct fix that collapses the $isArray ? $apply : $call branches into the single $call(stmt, values) form bun:sqlite already documents for positional binding.
What was reviewed:
- Verified against
src/js/bun/sqlite.ts#all/#values/#rawValues/run: passing the array as the sole arg routes tothis.#raw.all(...args)→ native gets the array; named-param objects and empty arrays take the same path. - Checked native
rebindStatementinJSSQLStatement.cpp: array elements now go throughrebindValueindividually, so an object/array in slot 0 throws the standard TypeError instead of being read as the binding set. - The
stmt.finalize()-on-throw concern from my earlier review is addressed by thetry/finallythat landed via #37109 and is present here. - New tests cover all three result modes, the
db.runbranch,unsafe(), and thesql()insert helper, plus a positive check that scalars/TypedArrays/bigint/null still bind in order; theexpect(async () => ...).toThrowshape matches existing tests in the same file.
Extended reasoning...
Overview
The PR changes 5 lines in src/js/internal/sql/sqlite.ts (SQLiteQueryHandle.run), replacing $isArray(values) ? stmt.X.$apply(stmt, values) : stmt.X.$call(stmt, values) with stmt.X.$call(stmt, values) for .all/.values/.raw, and db.run.$apply(db, [sql].concat(values)) with db.run.$call(db, sql, values). It adds a 9-test describe block to test/js/sql/sqlite-sql.test.ts.
The bug: spreading the positional array meant bun:sqlite's first-arg dispatch (src/js/bun/sqlite.ts:249-256, :516-521) inspected values[0] to decide the binding form. If values[0] was an object or array (e.g. a parsed request body), it was taken as the entire binding set, silently overriding later trusted parameters like a tenant filter — a parameter-injection vulnerability. The fix passes the whole array as the single first argument, which is bun:sqlite's documented positional form; each element then goes through native rebindValue, which throws a TypeError for non-bindable values.
Security risks
This closes a security hole rather than opening one. The change is a strict simplification: the array branch now uses the same call shape the named-parameter object branch already used (added in #37109), so both go through identical dispatch. I traced the new call through the JS wrapper: an array first arg is detected by isArray(arg0) and forwarded verbatim to native rebindStatement, which iterates it and binds positionally; an empty array clears bindings and binds nothing; an object first arg (named params) is unchanged. There is no new surface where untrusted input decides control flow.
Level of scrutiny
Medium — the code path handles user-supplied query values, but the diff is a 5-line collapse of two branches into one, both of which already existed. The remaining branch is the one #37109 introduced for named parameters and has been in main since. The mechanism was verified end-to-end against the JS wrapper and native rebindStatement/rebindValue.
Other factors
- My prior inline concern (skipped
stmt.finalize()on throw) is resolved: the bind/execute is insidetry { ... } finally { stmt.finalize(); }, which landed via #37109. - All comment-cop threads are resolved; the remaining one-line comment states the invariant, and the tests enforce it.
- Test coverage is thorough across the variant matrix (SELECT/INSERT, all/values/raw modes, tagged template,
unsafe(),sql()helper, indexed-key object vs array vs plain object vs Date), with a positive test confirming normal bindings still work. Theexpect(async () => ...).toThrowhelper matches the existing pattern at lines 349/710/985/1045 of the same file, and the PR description confirms 8/9 fail on the unfixed build. - CI was reported green on all test shards for the pre-rebase revision; the rebase only shrank the diff (the finalize/try-finally piece landed separately).
|
@robobun do named parameters still works fine one .unsafe? |
|
Yes. The named-parameter path is unchanged by this PR: in the code being replaced, the object branch of each ternary was already Verified on the current head (e232a4f, debug build): the eight
The full |
There was a problem hiding this comment.
LGTM — collapses the array/object branches onto bun:sqlite's single-argument positional form, closing the slot-0 rebind hole.
What was reviewed:
- Traced
stmt.all/values/raw.$call(stmt, values)anddb.run.$call(db, sql, values)throughsrc/js/bun/sqlite.ts(#all/#values/#rawValues/run) into nativerebindStatement: an array first arg binds each element viarebindValue, so a non-bindable element throws and can no longer pose as the whole binding set. - Checked the empty-array,
paramsCount === 0, and named-parameter-object (#37109) paths still bind as before. - The earlier finalize-on-throw note is addressed — the calls sit inside the
try { ... } finally { stmt.finalize() }that landed via #37109. - Tests cover SELECT/INSERT,
.values()/.raw(),unsafe(), thesql({...})helper, and a positive control that ordinary bindings still work; 8 of 9 fail on main per the PR evidence.
Extended reasoning...
Overview
Five-line source change in src/js/internal/sql/sqlite.ts (SQLiteQueryHandle.run): the four $isArray(values) ? fn.$apply(…, values) : fn.$call(…, values) sites collapse to fn.$call(…, values). bun:sqlite's JS wrapper already treats an array first argument as a positional binding list and a plain object as a named-parameter map, so both shapes of values now take the same call form. Plus 83 lines of tests in test/js/sql/sqlite-sql.test.ts.
Security risks
The change is a security fix: previously, spreading values meant values[0] reached bun:sqlite as the first argument, and if it was an object/array it was interpreted as the entire binding set — an attacker-controlled {"0":2,"1":"bob"} could override a trusted tenant filter that appeared later in the template. After the change every element goes through native rebindValue, which throws TypeError on non-bindable objects. Bindable scalars (string/number/bigint/boolean/null/TypedArray) behave identically. This strictly tightens; I don't see a way for it to weaken any existing guard.
Level of scrutiny
Medium — it's an internal-builtin hot path that binds user values into SQLite. I traced the wrapper (src/js/bun/sqlite.ts:247-296,510-) and the native rebindStatement/rebindValue in JSSQLStatement.cpp to confirm: array → per-element rebindValue; empty array → sqlite3_clear_bindings + zero iterations; paramsCount === 0 → #allNoArgs ignores its argument; named-param object → same branch it already took. The $isArray branch being removed only arrived in #37109, so this is a follow-up simplification of very recent code, not a long-standing invariant.
Other factors
- No CODEOWNER covers these files.
- All prior review threads are resolved: my earlier finalize-on-throw note is satisfied by the surrounding
try/finally(landed via #37109), and the comment-cop nags were addressed by trimming the inline comment to one line. - Tests follow the file's existing
expect(async () => …).toThrow(...)convention and are awaited at every call site; the PR's fail-on-main evidence (8 of 9 new tests fail without the src change) confirms the assertions are load-bearing. - The 9th test is a positive control that scalar values,
sql([...])IN-helpers, TypedArray/bigint/null,unsafe()arrays, and thesql({...})insert helper still bind in order — guarding against over-tightening.
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
3 participants
Problem
Bun.SQL, an object or array used as a query value is taken as the binding set for the whole statement instead of being bound as one value.{ x: 1 }, aDate) in that position silently bindsNULL:sql`insert into t values (${ { x: 1 } })`stores a NULL row with no error.sql.unsafe("select ? as a", [{ x: 1 }])and for asql({...})insert helper whose first column holds an object.SQLiteQueryHandle.runinsrc/js/internal/sql/sqlite.tsspread the positional array intostmt.all/values/raw(...values)anddb.run(sql, ...values).bun:sqlitereads the whole binding set from its first argument (array = positional list, other object = named/indexed map; seerebindStatementinsrc/jsc/bindings/sqlite/JSSQLStatement.cpp), so whatever sat in slot 0 decided how the statement was bound and the remaining arguments were ignored.Fix
stmt.all.$call(stmt, values),db.run.$call(db, sql, values)), which isbun:sqlite's documented positional form. Named-parameter objects (sql: support named parameters in SQLite unsafe() queries #37109) already took this path, so the$isArraybranches collapse into one call.rebindValue, so a non-bindable value in any slot fails withbun:sqlite's ownTypeError("Binding expected string, TypedArray, boolean, number, bigint or null"), the same error such a value already produced in every slot other than the first. An empty array binds nothing, as before.test/js/sql/sqlite-sql.test.ts("an object or array interpolated as a value cannot stand in for the parameter list"): 8 of the 9 new tests fail on the unfixed build and all pass with the fix; the 9th checks that positional values,unsafe()arrays and thesql()helpers still bind in order.test/js/sqlpass on a debug build.Background
Bun.SQLtagged templates are turned into?placeholders plus a positional array of values (normalizeQueryinsrc/js/internal/sql/shared.ts); the SQLite adapter hands that array tobun:sqlite.bun:sqlite'sStatement.all/values/rawandDatabase.runaccept either scalars spread as arguments, one array of positional values, or one object of named parameters. The JS wrapper (src/js/bun/sqlite.ts) only looks at the first argument to pick the form, and the native side binds from that argument alone.strict: trueconnections were not affected in the same way: missing parameters throw there, so these queries failed withMissing parameter "1"rather than silently rebinding.Earlier revision of this PR
The first revision also moved
stmt.finalize()into afinallyblock and scaled the 10k-insert finalization test down on debug builds. Both of those landed separately in #37109, which also added named-parameter support forunsafe()and conflicted with this branch; the branch was rebased on top of it and now only contains the positional-array change plus the tests. The tests were extended with theunsafe()array andsql()insert-helper cases.[review] gate passed · iteration 7 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 7
evidence per changed file