postgres: throw ERR_INVALID_ARG_TYPE for null bytes in connection params#32352
Conversation
The Rust port's null-byte rejection at PostgresSQLConnection.rs:1158 threw a plain code-less Error via throw_invalid_arguments_fmt (a thin wrapper over self.throw). MySQL (JSMySQLConnection.rs:506) and the Zig reference (PostgresSQLConnection.zig:704 throwInvalidArguments) both throw ERR_INVALID_ARG_TYPE TypeError; this brings Postgres in line. Message text is unchanged. The companion SSL_CTX-failure fix from #31664 is already in place via the shared ConnectionCtorArgs from #32135 — verified by the new ERR_BORINGSSL test, which passes without any Rust change. throw_invalid_arguments_fmt had this as its only caller, so the trait method goes too.
|
Updated 2:59 PM PT - Jun 15th, 2026
❌ @robobun, your commit 8c9b180 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32352That installs a local version of the PR into your bun-32352 --bun |
|
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 (1)
Walkthrough
ChangesRemove throw_invalid_arguments_fmt and update Postgres call site
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 `@test/js/sql/sql.test.ts`:
- Around line 12777-12790: The test.concurrent.each matrix in the null-byte
rejection test is incomplete. Currently it only covers ["username", "password",
"database"] but omits "path", which should also be validated for null-byte
rejection according to the same contract. Add "path" to the array of field names
being tested in the test.concurrent.each call to ensure complete coverage of all
parameters that reject null bytes.
🪄 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: cdf70011-be27-4919-935c-afbe47497ab7
📒 Files selected for processing (3)
src/sql_jsc/jsc.rssrc/sql_jsc/postgres/PostgresSQLConnection.rstest/js/sql/sql.test.ts
💤 Files with no reviewable changes (1)
- src/sql_jsc/jsc.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line swap to the canonical throw_invalid_arguments plus dead-code removal; the two inline nits are comment-only and don't block.
Extended reasoning...
Overview
This PR changes a single error-throwing call in src/sql_jsc/postgres/PostgresSQLConnection.rs:1158 from the local throw_invalid_arguments_fmt wrapper (which threw a plain code-less Error) to the inherent JSGlobalObject::throw_invalid_arguments (which throws an ERR_INVALID_ARG_TYPE TypeError). The now-orphaned trait method and its import are removed from src/sql_jsc/jsc.rs. A new describe block in test/js/sql/sql.test.ts adds six no-server tests covering null-byte rejection, SSL_CTX failure, bad tls type, and simple-query-with-params rejection.
Security risks
None. The change tightens an existing input-validation error to carry the correct error code and prototype; the rejection logic itself (NUL-byte check on credentials) is unchanged. No new code paths, no auth/crypto/permission surface touched.
Level of scrutiny
Low. The Rust delta is a one-line method swap to a widely-used inherent helper (20+ existing callers across the codebase follow the exact Err(global.throw_invalid_arguments(format_args!(...))) pattern), plus mechanical dead-code removal verified by grep — throw_invalid_arguments_fmt has zero remaining references and JSGlobalObjectSqlExt is no longer used in PostgresSQLConnection.rs. The test additions are self-contained and don't touch existing tests.
Other factors
The bug-hunting system flagged two nits, both about test comment text: a stale file-path reference (connection_args.rs vs ConnectionCtorArgs.rs) and bug-history narration that CLAUDE.md routes to the PR description. Both are comment-only with zero runtime effect and are surfaced as inline comments for the author to address; they don't warrant holding the merge. The PR description documents cargo check clean, 6/6 tests passing, and a discriminating stash-probe against unfixed main.
|
@robobun adopt |
There was a problem hiding this comment.
LGTM — both nits addressed; straightforward swap to the proper ERR_INVALID_ARG_TYPE helper plus dead-code cleanup and tests.
Extended reasoning...
Overview
This PR swaps a single call site in src/sql_jsc/postgres/PostgresSQLConnection.rs from throw_invalid_arguments_fmt (a thin local wrapper over self.throw, which produces a plain code-less Error) to the inherent JSGlobalObject::throw_invalid_arguments, which routes through JscError::INVALID_ARG_TYPE.fmt(...) to produce a proper ERR_INVALID_ARG_TYPE TypeError. The now-orphaned throw_invalid_arguments_fmt trait method and its impl are removed from src/sql_jsc/jsc.rs, along with the now-unused JSGlobalObjectSqlExt as _ import. A new server-free describe block in test/js/sql/sql.test.ts covers the null-byte rejection error shape, the SSL_CTX-failure ERR_BORINGSSL path, non-boolean/object tls rejection, and parameterized simple-query rejection.
Security risks
None. The behavioral change is strictly in the error shape (name/code/prototype) of an already-existing rejection path; the rejection condition and message text are unchanged. No new inputs are accepted, no auth/crypto/permission logic is touched.
Level of scrutiny
Low. The Rust diff is a one-line substitution at a single call site plus removal of a now-dead helper (verified no remaining callers via grep). I confirmed throw_invalid_arguments → to_invalid_arguments → JscError::INVALID_ARG_TYPE at src/jsc/JSGlobalObject.rs:231,1461, and that it returns JsError so the new Err(...) wrapping is correct. The test additions are self-contained, server-free (use an unreachable 127.0.0.1:1 so failures are guaranteed to be pre-connect validation), and follow existing patterns in the file.
Other factors
Both nits I raised on the previous revision were addressed in commit 7c9c6a8: the test comment now correctly references ConnectionCtorArgs.rs / QueryCtorArgs.rs, and the bug-history trailing clauses were removed from both comments per CLAUDE.md. The CodeRabbit suggestion to add path to the test matrix was reasonably answered with an in-comment explanation that the JS layer drops path via existsSync before it reaches the native parser, so it can't be exercised the same way. The PR description documents cargo check clean and 6/6 test pass with a discriminating stash-probe against unfixed main.
There was a problem hiding this comment.
LGTM — thanks for addressing the comment nits; the error-type swap is straightforward and matches the MySQL/Zig reference.
Extended reasoning...
Overview
This PR changes the Postgres null-byte credential rejection in PostgresSQLConnection.rs:1158 from throwing a plain code-less Error (via the SQL-local throw_invalid_arguments_fmt wrapper, which just called self.throw) to throwing a proper ERR_INVALID_ARG_TYPE TypeError via the inherent JSGlobalObject::throw_invalid_arguments. The now-orphaned throw_invalid_arguments_fmt trait method is removed from JSGlobalObjectSqlExt, and the trait import is dropped from PostgresSQLConnection.rs (verified no other trait methods are used there). A new server-free test block in sql.test.ts covers the null-byte rejection, SSL_CTX failure (ERR_BORINGSSL), invalid tls type, and parameterized simple-query rejection.
I verified JSGlobalObject::throw_invalid_arguments → to_invalid_arguments → JscError::INVALID_ARG_TYPE.fmt(...), which is the correct error code/type. I also confirmed via grep that throw_invalid_arguments_fmt has no remaining callers and that PostgresSQLConnection.rs uses no other JSGlobalObjectSqlExt methods (err_out_of_range, sql_vm, sql_vm_ptr), so the import removal is safe.
Security risks
None. The validation logic itself is unchanged — null bytes were already rejected; only the error type changes from Error to TypeError with a code property. No new input handling, no auth/crypto/permission surface.
Level of scrutiny
Low. This is a ~10-line mechanical Rust change (swap one throw helper for another with the correct error code, delete dead code) plus ~70 lines of new tests that don't require a server. The behavior change is purely cosmetic from a security standpoint and brings the Rust Postgres port to parity with both the MySQL adapter and the Zig reference, as documented.
Other factors
- All three prior review comments (my two nits about file-path references and bug-history narration, plus CodeRabbit's
pathmatrix suggestion) were addressed in commit7c9c6a8f— the diff now citesConnectionCtorArgs.rs / QueryCtorArgs.rscorrectly, the "previous port threw…" clauses are gone, and an explanatory note covers whypathisn't in the test matrix (JS layer drops it viaexistsSyncbefore native). - PR description reports
cargo check -p bun_sql_jscclean and 6/6 tests passing, with a discriminating stash-probe confirming the new assertions fail against the unfixed Rust build. - The Buildkite ❌ prompted an empty
ci: retriggercommit, consistent with flake; the actual diff is small enough that any real failure would be unrelated to this change. - A maintainer (
alii) has adopted the PR.
|
Diff is ready (6/6 |
…32415) Extends the existing postgres-only `rejects tls that is neither a boolean nor an object` test (added in #32352) to mysql via `test.each`. The native constructor throws synchronously on a non-boolean/non-object `tls`, and `createPooledConnectionHandle` defers `onClose` via `process.nextTick` (since #32145) so the pending query rejects cleanly instead of hanging. This locks that in for both adapters. No code change; 2/2 pass on `bun bd test`.
3 participants
What this does
The Rust port's null-byte rejection for
username/password/database/pathatPostgresSQLConnection.rs:1158threw a plain code-lessErrorviathrow_invalid_arguments_fmt(a thin wrapper overself.throw). MySQL (JSMySQLConnection.rs:506) and the Zig reference (PostgresSQLConnection.zig:704throwInvalidArguments) both throwERR_INVALID_ARG_TYPETypeError; this brings Postgres in line. Message text is unchanged.The companion SSL_CTX-failure fix from #31664 is already in place via the shared
ConnectionCtorArgsfrom #32135 — verified by the newERR_BORINGSSLtest, which passes onmainwithout any Rust change.throw_invalid_arguments_fmthad this as its only caller, so the trait method is removed too.Extracted from #31664 (closed).
Verification
cargo check -p bun_sql_jsc: clean.bun bd test sql.test.ts -t 'shared createInstance validation': 6 pass / 0 fail.USE_SYSTEM_BUN=1passes (system bun is pre-Rust-port Zig, which was correct), so the load-bearing check is a stash-probe against unfixedmain's Rust build — null-byte rejection there gives{name: 'Error', code: undefined, instanceof TypeError: false}, failing the newERR_INVALID_ARG_TYPEandinstanceof TypeErrorassertions.