Repository navigation
sql(postgres): hex-encode Uint8Array/DataView elements in sql.array; stop TypedArray.map coercion - #36241
sql(postgres): hex-encode Uint8Array/DataView elements in sql.array; stop TypedArray.map coercion#36241robobun wants to merge 12 commits into
Conversation
|
Status: diff is green; ready for review. Fail-before ( CI: #84215 and #84406 both green on every docker-backed lane that exercises |
WalkthroughPostgreSQL array serialization now handles ArrayBuffer views as binary values where supported, preserves typed-array element representations, rejects unsupported binary element types, and adds Bind-payload and BYTEA round-trip tests. ChangesPostgreSQL array serialization
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/postgres.ts`:
- Around line 241-252: Update the top-level array handling around $isArray and
arrayValueSerializer to explicitly detect DataView inputs before the
values.length check. Either serialize the DataView as one binary element using
its exact byte offset and length, or reject it with a catchable error; do not
allow it to fall through to "{}". Add a regression case covering a DataView with
a non-zero offset and verify its bytes are preserved or the documented rejection
occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d629a35-7252-4ba5-b290-bdfd5cddb0e6
📒 Files selected for processing (3)
src/js/internal/sql/postgres.tstest/js/sql/postgres-array-typedarray-serialize.test.tstest/js/sql/sql.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/sql/postgres.ts (1)
17-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse realm-independent byte-view detection.
instanceofis realm-sensitive here, so cross-realmUint8Array/Uint8ClampedArray/DataViewvalues are misclassified: typed arrays serialize as nested numbers, andDataViewcan even hit the empty-array fast path. Switch to a brand/prototype-independent check and add cross-realm coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/internal/sql/postgres.ts` around lines 17 - 18, Update isByteView to detect Uint8Array, Uint8ClampedArray, and DataView values without instanceof, using realm-independent brand or prototype-independent checks so cross-realm values are classified correctly. Preserve the existing supported-type scope and add coverage for cross-realm typed arrays and DataView serialization, including the empty-array path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/js/internal/sql/postgres.ts`:
- Around line 17-18: Update isByteView to detect Uint8Array, Uint8ClampedArray,
and DataView values without instanceof, using realm-independent brand or
prototype-independent checks so cross-realm values are classified correctly.
Preserve the existing supported-type scope and add coverage for cross-realm
typed arrays and DataView serialization, including the empty-array path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9405e41e-c21e-4989-8d2f-a820876fb347
📒 Files selected for processing (2)
src/js/internal/sql/postgres.tstest/js/sql/postgres-array-typedarray-serialize.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/postgres.ts`:
- Line 145: Add BYTEA serialization test coverage for arrayValueSerializer using
a populated SharedArrayBuffer, alongside the existing ArrayBuffer test, and
assert that Buffer.from(value) preserves the exact byte contents.
In `@test/js/sql/postgres-array-typedarray-serialize.test.ts`:
- Around line 1-6: Condense the header comment in the postgres array typed-array
serialization test to a brief statement of the durable coercion invariant:
ArrayBufferView elements are hex-encoded in BYTEA or JSON arrays and rejected
elsewhere. Remove the implementation-history details, examples, and workaround
explanation while preserving the test-purpose context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: edbbfe25-b5bf-4877-9594-b086544d7df7
📒 Files selected for processing (2)
src/js/internal/sql/postgres.tstest/js/sql/postgres-array-typedarray-serialize.test.ts
There was a problem hiding this comment.
LGTM — all prior review threads are resolved and the fix now correctly double-escapes the \x prefix, covers bare ArrayBuffer/SharedArrayBuffer, and names JSONB in the rejection message.
What was reviewed:
- BYTEA element escaping:
arrayEscape("\\x" + hex)produces"\\\\x…"on the wire soarray_inhands\x…tobyteain; the docker round-trip in sql.test.ts exercises this against a real server. serializeArraytop-level TypedArray path usesArray.from(values, serialize)so per-element strings are not coerced back through the typed-array species; the removedisTypedArrayhelper has no remaining callers.- The nested-numeric-TypedArray →
ERR_INVALID_ARG_VALUEchange was discussed and intentionally kept strict; it converts previously accidental/inconsistent behavior into a loud pre-I/O error with a clear remedy.
Extended reasoning...
Overview
The PR fixes silent data corruption in sql.array(values, type) for PostgreSQL: non-Buffer ArrayBufferView elements were treated as nested array dimensions and recursed via TypedArray.prototype.map, which coerced each per-element string serialization back into the element type ('"1"' → NaN → 0), so a Uint8Array([1,2,44]) in a bytea[] went on the wire as {{0,0,0}}. Changes are confined to src/js/internal/sql/postgres.ts (arrayValueSerializer and serializeArray), a new scripted-backend test file, and one docker-gated round-trip test in sql.test.ts.
Security risks
None. The change is client-side array-literal formatting for a text-format bind parameter; no new parsing of untrusted input, no auth/crypto/permission code paths. The arrayEscape helper is reused for the \x prefix rather than hand-doubling, so escaping stays centralized.
Level of scrutiny
Medium — this is user-facing runtime behavior in the SQL driver, but the change is small (~46 lines net in one internal JS module), replaces silent corruption with either correct output or a typed error, and is backed by wire-literal assertions plus a real-server round-trip. It went through two prior review rounds here; every finding (under-escaped \x, bare-ArrayBuffer fall-through, JSONB missing from the error text) was addressed with a follow-up commit and test.
Other factors
- The one intentional behavioral tightening (nested numeric TypedArray now throws instead of accidentally serializing via lossless
.mapcoercion) was raised, and the author declined with a solid rationale: the old output was an undocumented byproduct of the exact bug being removed, disagreed with theBufferpath for identical bytes, and now fails loud pre-I/O with a named remedy. That is the safe direction and does not need maintainer sign-off. - The removed
isTypedArrayhelper has no remaining references insrc/js/internal/sql/. - Tests follow the established
test/js/sql/wire-framesscripted-backend pattern used by ~30 sibling files, coverUint8Array/Uint8ClampedArray/DataView/ArrayBuffer/SharedArrayBuffer,byteOffset/byteLengthsub-views, the JSON path, the rejection path across TEXT/INT/REAL, and the top-level TypedArray → TEXT non-coercion case. - All CodeRabbit and github-actions threads are resolved; the bug hunting system found nothing on the current revision.
…stop TypedArray.map coercion
sql.array([new Uint8Array([1,2,44])], "bytea") treated the Uint8Array as a
nested dimension and serialized each element with TypedArray.prototype.map,
which coerces the per-element string ("\"1\"") back into the typed array's
element type (NaN -> 0 for Uint8Array). The literal {{0,0,0}} went on the
wire with no error. The same map coercion zeroed any non-numeric top-level
typed array.
Uint8Array, Uint8ClampedArray and DataView (and Buffer, a Uint8Array
subclass) now take the Buffer hex path as a single binary element. Other
typed arrays remain nested dimensions but build their per-element strings
into a plain Array so nothing is coerced.
… of nesting
Every ArrayBufferView element now hex-encodes in BYTEA and JSON arrays and
throws ERR_INVALID_ARG_VALUE for any other element type, so the silent
{{0,0,0}} / {{NaN,NaN}} corruption becomes a clear error and Uint8Array is
byte-identical to Buffer in bytea[]. Drops the typed-array nested-dimension
arm from arrayValueSerializer; top-level typed arrays still iterate via
Array.from in serializeArray.
The array-literal parser consumes one level of backslash escaping before the
element reaches bytea_in, so {"\x01022c"} was being stored as the ASCII
bytes 'x01022c'. Emit {"\\x01022c"} instead. The pre-existing Buffer path
had the same under-escaping; the new docker round-trip test surfaced it.
…ArrayBuffer
ArrayBuffer.isView(new ArrayBuffer(n)) is false, so a bare ArrayBuffer
element was still falling through to JSON.stringify and serializing as
{"{}"}. Give it (and SharedArrayBuffer) the same hex-or-reject treatment as
every other binary container.
d876c56 to
080b5f8
Compare
There was a problem hiding this comment.
LGTM — all four items from earlier passes are addressed (the \x prefix now goes through arrayEscape so the wire literal has the doubled backslash, bare ArrayBuffer/SharedArrayBuffer are covered, the error message names JSONB, and the nested-numeric-TypedArray throw is a reasoned choice).
Checked that Buffer.from(value.buffer, value.byteOffset, value.byteLength) is only taken for views so offset/length are honoured, and that Array.from(values, serialize) receives the element as the bound serializer's value arg with the index harmlessly ignored. The scripted-backend expectations now assert {"\\\\x…"} (two backslashes on the wire), matching what array_in needs to hand \x… to byteain, and the docker round-trip in sql.test.ts covers a real server.
Extended reasoning...
Overview
~30 lines of runtime change confined to arrayValueSerializer / serializeArray in src/js/internal/sql/postgres.ts, plus a new scripted-backend test file and one docker-gated round-trip case in sql.test.ts. The fix replaces the isTypedArray nested-dimension arm (which used TypedArray.prototype.map and coerced per-element strings back into the typed array) with an ArrayBuffer.isView || instanceof ArrayBuffer || instanceof SharedArrayBuffer branch that hex-encodes for BYTEA, JSON-wraps the hex for JSON/JSONB, and throws ERR_INVALID_ARG_VALUE for every other element type. serializeArray now uses Array.from(values, serialize) for a top-level typed array so nothing round-trips through a numeric coercion.
Security risks
None. Output is a text-format Bind parameter, not interpolated SQL; the only string construction is arrayEscape over hex digits and a literal \x prefix (no user-controlled characters reach the escape path in the new branch). No auth, crypto, or filesystem code touched.
Level of scrutiny
Moderate — it's a data-correctness fix in a database driver where the failure mode was silent corruption, so the wire literal has to be exactly right. Two earlier passes here surfaced a real under-escaping bug (single \x backslash), a same-class gap (bare ArrayBuffer), and a message wording nit; all are fixed in the current diff and every thread is resolved. The one deliberate behavior change — throwing for binary elements in non-BYTEA/JSON arrays, including the previously-accidental nested-Int32Array-in-INT case — was discussed and the author's reasoning (undocumented, inconsistent with the Buffer path on main, fails loud pre-I/O, trivial Array.from workaround) is sound.
Other factors
Test coverage is thorough: exact wire-literal assertions for Uint8Array/Uint8ClampedArray/DataView/ArrayBuffer/SharedArrayBuffer in BYTEA, byteOffset/byteLength honouring, JSON hex-wrap parity with Buffer, the rejection matrix across four element kinds × three types, the top-level-TypedArray-no-coercion case, and a real-Postgres round-trip. The new code matches local conventions ($ERR_INVALID_ARG_VALUE, unprotected Buffer/JSON/instanceof Date already used throughout this file). The removed Buffer.isBuffer arm in the default: case is fully subsumed by the new top-of-function guard. No outstanding reviewer comments.
|
Updated 7:18 PM PT - Jul 28th, 2026
❌ @robobun, your commit c11121b has some failures in 🧪 To try this PR locally: bunx bun-pr 36241That installs a local version of the PR into your bun-36241 --bun |
There was a problem hiding this comment.
LGTM — all four items from the earlier review rounds are addressed (\x prefix now runs through arrayEscape, bare ArrayBuffer/SharedArrayBuffer covered, JSONB named in the rejection message, SAB test added), and the deliberate throw for binary elements in non-binary array types was discussed and justified.
Checked that the BYTEA literal now reaches byteain as \x… after array_in unescapes the doubled backslash, and that the docker round-trip in sql.test.ts asserts the actual bytes.
Confirmed serializeArray's top-level path is behavior-preserving for Buffer (still returned unchanged) and plain arrays; only non-Buffer views switch from .map to Array.from.
Verified Buffer.from(value.buffer, value.byteOffset, value.byteLength) is exercised by the offset/length subarray + DataView test.
Extended reasoning...
Overview
Targeted fix in src/js/internal/sql/postgres.ts to arrayValueSerializer and serializeArray: any ArrayBufferView / ArrayBuffer / SharedArrayBuffer element is now hex-encoded for BYTEA / JSON / JSONB arrays and rejected with ERR_INVALID_ARG_VALUE otherwise, replacing the old path that recursed via TypedArray.prototype.map and coerced serialized strings back into the element type (silently zeroing the bytes). The \x prefix is routed through arrayEscape so array_in unescapes back to \x for byteain. serializeArray uses Array.from for a top-level TypedArray so per-element strings are not coerced. New scripted-backend test file asserts the exact Bind literal for each view/buffer variant plus a docker-gated round-trip in sql.test.ts.
Security risks
None. sql.array output is a text-format bind parameter, not interpolated SQL; arrayEscape still handles \ and " for the array-literal syntax and hex output is [0-9a-f]*. No new external input parsing.
Level of scrutiny
Medium — user-facing serialization on a data-integrity path. The PR has been through two prior review passes here plus CodeRabbit; every thread is resolved. The one behavioral tightening (Buffer/TypedArray elements now throw in TEXT/INT/etc. arrays instead of emitting a bare hex string or accidentally-correct nested numerics) was explicitly raised and the author gave a reasoned justification: the old output was an undocumented byproduct of the exact coercion bug being removed, disagreed with the sibling Buffer path, and now fails loud before I/O with the type named in the message.
Other factors
The scripted-backend tests assert the exact wire literal and were shown to fail on main and pass on the branch in both debug+ASAN and release. The docker round-trip is what caught the original single-backslash bug. Buffer.from(view.buffer, view.byteOffset, view.byteLength) is covered by a dedicated offset/length test, and bare ArrayBuffer/SharedArrayBuffer each have a positive BYTEA case and a rejection case. Top-level serializeArray behavior is unchanged for plain arrays and top-level Buffer (still short-circuits to return values). SharedArrayBuffer is always defined in Bun so the unguarded instanceof is fine.
|
#41301 overlaps with this PR in If #41301 merges first, this PR needs a rebase. In #41301 an untyped array has |
This branch has not been deployed
2 participants
What
sql.array(values, type)silently zeroed any non-BufferArrayBufferViewelement.No error; the row is written with the wrong data.
Float32Arrayelements became{{NaN,NaN}}andBigInt64ArraythrewFailed to parse String to BigInt.Why
arrayValueSerializertreated every non-BufferArrayBuffer.isViewas a nested array dimension and recursed withvalue.map(...).TypedArray.prototype.mapreturns the same typed-array species, so each per-element string serialization was coerced back into the element type:'"1"'->NaN->0forUint8Array,NaNforFloat32Array. ADataViewelement has no.length, so it became{}.serializeArrayhad the same.mapcoercion for a top-level typed array.Fix
arrayValueSerializernow checksArrayBuffer.isViewfirst. Any view element (Buffer,Uint8Array,DataView,Float32Array, ...) is hex-encoded in aBYTEAor JSON/JSONB array and rejected withERR_INVALID_ARG_VALUEfor every other element type, so the silent corruption becomes a clear error andUint8ArraymatchesBufferbyte for byte inbytea[]. The typed-array nested-dimension arm is gone from the element dispatch.serializeArraybuilds per-element strings into a plainArrayviaArray.fromfor a top-level typed array so nothing is coerced.Tests
test/js/sql/postgres-array-typedarray-serialize.test.tsscripts a v3 backend that records the Bind parameter and asserts the exact literal for theBYTEA/JSON cases and the rejection for everything else (fails onmain, passes here). ABYTEAround-trip withUint8ArrayandBufferelements is added to the container-backedsql.arraysuite intest/js/sql/sql.test.ts.[review] gate passed · iteration 6 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 6
evidence per changed file