Bun.SQL(postgres): send plaintext startup when sslmode=prefer and server declines TLS

[robobun]

Problem

sslmode=prefer is libpq's documented default: "first try an SSL connection; if that fails, try a non-SSL connection". Postgres ships with ssl=off, so a stock docker run postgres answers the 8-byte SSLRequest with N. Bun then never writes anything else on the socket and the connection idles until connectionTimeout:

$ bun -e 'await new Bun.SQL("postgres://postgres@127.0.0.1:5432/postgres?sslmode=prefer",{max:1,connectionTimeout:3})`SELECT 1`'
PostgresError: Connection timeout after 3s
 code: "ERR_POSTGRES_CONNECTION_TIMEOUT"

A wire capture shows the client sends the SSLRequest, the server replies N, and the client goes quiet.

Cause

In PostgresRequest::on_data, the b'N' arm with TlsStatus::MessageSent correctly fails for require/verify-*, but for prefer it only did connection.tls_status.set(TlsStatus::SslNotAvailable) and continued the read loop. Nothing ever called start() (the StartupMessage writer) again: start() is only reached from on_open (already past) and from setup_tls() (the S arm).

The continue also kept dispatching any bytes that arrived in the same read as the N, so a server that wrote N + ErrorResponse in one segment had that ErrorResponse surfaced as the connection error even though no StartupMessage had been sent. That is the message-injection shape libpq hardened against in CVE-2021-23222.

Fix

src/sql_jsc/postgres/PostgresRequest.rs: in the N+prefer arm, call connection.start() to write the plaintext StartupMessage and return Ok(()) so any trailing bytes in the same read are discarded (mirrors the S arm, which also return Ok(())s after setup_tls()).

src/sql_jsc/postgres/PostgresSQLConnection.rs: widen start() to pub(crate).

test/js/sql/postgres-tls-ctx-leak.test.ts: the mock server was packing N + AuthenticationOk + ReadyForQuery in a single write and relying on the old continue to dispatch them; switch it to the real two-round-trip exchange.

Verification

Against a real local Postgres with ssl=off:

prefer:  [{"x":1}]                         # was ERR_POSTGRES_CONNECTION_TIMEOUT
disable: [{"x":1}]
require: ERR_POSTGRES_TLS_NOT_AVAILABLE

New tests in test/js/sql/tls-sql.test.ts:

• sslmode=prefer falls back to a plaintext startup when the server declines TLS: mock server answers N, asserts wire == [SSLRequest, StartupMessage, Query] and the query returns a row.
• sslmode=prefer discards bytes that arrive alongside the 'N' SSLRequest answer: mock server writes N + ErrorResponse in one segment, asserts the injected error does not surface and the plaintext startup proceeds.

Both fail on main (first with ERR_POSTGRES_CONNECTION_TIMEOUT and wire == [SSLRequest], second with ERR_POSTGRES_SERVER_ERROR: injected before startup) and pass with this change.

Fixes #36887

[robobun]

^{Updated 12:05 PM PT - Jul 7th, 2026}

❌ @robobun, your commit a6521e3 has some failures in Build #69933 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33666

That installs a local version of the PR into your bun-33666 executable, so you can run:

bun-33666 --bun

[github-actions]

This PR may be a duplicate of:

1. fix(sql): TypeError tls must be a boolean or an object + fix SSL/TLS behaviour to match postgres.js standard #26503 - Also fixes sslmode=prefer falling back to plaintext startup when the server declines TLS, with mock server tests covering the same SSLRequest → N → StartupMessage flow

🤖 Generated with Claude Code

[robobun]

Not a duplicate of #26503. That PR targets the pre-port Zig files (PostgresSQLConnection.zig), carries merge conflicts, and bundles a default-sslmode change plus verify-ca/verify-full rework alongside the fallback fix. This PR is the two-line N-arm fix in the current Rust path (PostgresRequest.rs) with wire-level tests, and leaves the default sslmode alone.

[coderabbitai]

Walkthrough

The Postgres client now falls back to plaintext startup after an SSL refusal in non-strict sslmode, and the tests cover the updated wire sequence plus handling of extra bytes during the transition.

Changes

Postgres TLS Fallback Fix

Layer / File(s)	Summary
Plaintext fallback on SSL refusal src/sql_jsc/postgres/PostgresRequest.rs, src/sql_jsc/postgres/PostgresSQLConnection.rs	When the server declines TLS under non-strict ssl_mode, the connection now calls the crate-visible start() method and returns early to begin plaintext startup.
Fault-injection tests for sslmode=prefer fallback test/js/sql/tls-sql.test.ts	Adds tests for the SSLRequest → StartupMessage → Query sequence after 'N', including a case where extra injected bytes are discarded.
TLS-refusal mock server stateful handshake test/js/sql/postgres-tls-ctx-leak.test.ts	Reworks the mock server to refuse SSL first, then send AuthenticationOk and ReadyForQuery on later client traffic, with updated commentary.

Possibly related PRs

• oven-sh/bun#32467: Uses the same Postgres TLS fault-injection test area and wire-frame helpers around SSL refusal handling.
• oven-sh/bun#33016: Also changes PostgresRequest::on_data to alter read handling during a connection-state transition.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the primary PostgreSQL SSL fallback change.
Description check	✅ Passed	The description explains the problem, cause, fix, verification steps, tests, and expected behavior, despite using different section headings than the template.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Thanks for extracting the helper — no further issues from me, but since this changes the TLS-vs-plaintext decision path in the Postgres driver I'd like a human to sign off.

Extended reasoning...

Overview

Two-line native change in src/sql_jsc/postgres/PostgresRequest.rs: in the b'N' arm of on_data while TlsStatus::MessageSent, replace continue with connection.start(); return Ok(()) so that sslmode=prefer sends a plaintext StartupMessage after the server declines TLS (matching libpq semantics) and drops any trailing bytes in the same read. start() is widened to pub(crate). The existing postgres-tls-ctx-leak mock server is updated to the real two-round-trip handshake, and two new wire-level tests in tls-sql.test.ts (now sharing runSslmodePreferAgainstNonSslServer, addressing my earlier nit) assert the SSLRequest → N → StartupMessage → Query sequence and that an ErrorResponse packed alongside the N is discarded.

Security risks

This is the code path that decides whether to proceed in plaintext or abort after TLS is refused. The change only affects the branch where ssl_mode is not require/verify-ca/verify-full — i.e. the user explicitly chose sslmode=prefer, whose documented contract is exactly "try TLS, fall back to plaintext". The existing test that tls: true / sslmode=require still refuses the downgrade with ERR_POSTGRES_TLS_NOT_AVAILABLE is untouched and still gates the strict path. The return Ok(()) (mirroring the S arm) additionally hardens against the CVE-2021-23222 message-injection shape by discarding bytes that arrive before the StartupMessage is sent. I don't see a downgrade or credential-exposure risk introduced here, but any edit to the TLS-negotiation state machine deserves human eyes.

Level of scrutiny

Medium-high: the diff is tiny and mirrors the sibling S arm's control flow, but it lives in a security-sensitive state machine (TLS negotiation for a database client that carries credentials). Per the approval guidelines I don't auto-approve changes in auth/crypto/TLS paths regardless of size.

Other factors

The bug-hunting pass found nothing. My one prior comment (extract the duplicated ~40-line mock-server body) was addressed in aa26ca4 and the thread is resolved. Test coverage is strong: wire-level assertions on the exact frame sequence, a negative test for injected trailing bytes, and the pre-existing tls: true refusal test still guards the strict path. There's a stale overlapping PR (#26503) against the old Zig code that a maintainer may want to close alongside this.

[claude]

Thanks for extracting the helper — the diff looks correct and well-tested to me, but since this changes the TLS-decline path to silently fall back to plaintext (and touches the CVE-2021-23222 injection surface), I'd like a human to sign off on the security-sensitive bits.

Extended reasoning...

Overview

The native change is two lines in src/sql_jsc/postgres/PostgresRequest.rs: in the b'N' arm of on_data while TlsStatus::MessageSent, replace continue with connection.start(); return Ok(()). This makes sslmode=prefer send a plaintext StartupMessage after the server declines TLS (matching libpq's documented semantics) instead of idling until connectionTimeout, and discards any bytes that arrived alongside the N. start() on PostgresSQLConnection is widened to pub(crate) to make it callable from the request module. Two new wire-level tests in test/js/sql/tls-sql.test.ts assert the exact frame sequence and the injection-discard behavior; test/js/sql/postgres-tls-ctx-leak.test.ts is updated to use the real two-round-trip handshake now that the client no longer dispatches trailing bytes in the same read as N.

Security risks

This is squarely in the TLS negotiation state machine. The change (a) causes sslmode=prefer to proceed in plaintext when the server answers N — which is the libpq contract, but does mean credentials go over the wire unencrypted in that mode; and (b) hardens the same arm against the CVE-2021-23222 message-injection shape by dropping trailing bytes. The require/verify-ca/verify-full arms are untouched and still fail closed. I don't see a downgrade regression — an explicit tls: true still maps to a strict mode per the existing "aborts the connection when the server declines TLS that was explicitly requested" test — but a human should confirm that prefer is the only mode that reaches this fallback and that the plaintext-downgrade UX is intentional for Bun.

Level of scrutiny

Medium-high. The Rust diff is tiny and mirrors the adjacent b'S' arm's return Ok(()) shape, and the tests are strong (exact wire assertions, both fail on main). But any change to whether a database client encrypts its startup/password exchange is security-sensitive by the repo's own review criteria, so I'm deferring rather than shadow-approving.

Other factors

• My earlier nit about the duplicated ~40-line mock-server body was addressed in aa26ca4 via runSslmodePreferAgainstNonSslServer(); that thread is resolved.
• The updated postgres-tls-ctx-leak.test.ts mock server is a required follow-on (the old single-write handshake relied on the removed continue), not scope creep.
• PR #26503 covers the same bug in the pre-port Zig path and bundles a default-sslmode change; a maintainer may want to decide how the two relate.
• No CODEOWNERS entry for src/sql_jsc/; the bug-hunting pass found nothing.

[claude]

Thanks for extracting the helper — the fix looks correct and well-tested, but since this is TLS negotiation / plaintext-fallback logic I'd like a human to sign off.

Extended reasoning...

Overview

Two-line fix in src/sql_jsc/postgres/PostgresRequest.rs: when the server answers the SSLRequest with 'N' and sslmode=prefer, call connection.start() to send the plaintext StartupMessage and return Ok(()) to discard any trailing bytes in the same read (mirroring the 'S' arm). start() is widened to pub(crate). An existing mock-server test in postgres-tls-ctx-leak.test.ts is updated to use the real two-round-trip exchange (it previously relied on the buggy continue), and two new wire-level tests are added in tls-sql.test.ts via a shared runSslmodePreferAgainstNonSslServer helper (extracted after my earlier nit).

Security risks

This is the TLS-vs-plaintext decision point for Postgres connections. The change is a hardening in two respects: (1) it makes sslmode=prefer actually work instead of hanging, without touching the require/verify-* guard that still fails closed; (2) the return Ok(()) drops bytes injected alongside the 'N' byte, which is the CVE-2021-23222 message-injection shape. I don't see a downgrade vector introduced here — the plaintext fallback only fires for prefer, which is documented to fall back — but any change to the encrypted/plaintext boundary of a DB driver deserves human eyes.

Level of scrutiny

Medium-high. The native diff is tiny and mechanically mirrors the adjacent 'S' arm, and the wire-level tests assert the exact frame sequence and that injected ErrorResponse bytes don't surface. But "when do we go plaintext" is exactly the class of code where a subtle mistake silently weakens security, so I'm deferring rather than shadow-approving.

Other factors

The bug-hunting pass found nothing. My prior review nit (duplicated ~40-line mock-server bodies) was addressed in aa26ca4. There's a flagged possibly-duplicate PR (#26503) targeting the old Zig path that the author has already responded to. CI build #69933 is in flight after two retrigger commits.

[robobun]

CI status: the only hard failure on builds #69915, #69921 and #69933 is the :darwin: 26 aarch64 - test-bun lane dying on buildkite-agent artifact download timed out after 120s before any test runs. That lane is failing the same way on 12 of the last 25 builds across unrelated branches, so it is Buildkite infrastructure, not this diff. The remaining reds are style: warning flaky-annotation entries (Windows hot-reload ENOENT, spawn-pipe-leak RSS threshold, complex-workspace install, Windows-aarch64 net/update_interactive) that pass on retry and are untouched by this change.

The changed files pass locally on both the debug/ASAN build and a release build, and fail with ERR_POSTGRES_CONNECTION_TIMEOUT / ERR_POSTGRES_SERVER_ERROR: injected before startup when src/ is reverted to main:

# with fix
bun bd test test/js/sql/tls-sql.test.ts test/js/sql/postgres-tls-ctx-leak.test.ts   # 6 pass
bun run build:release test test/js/sql/tls-sql.test.ts test/js/sql/postgres-tls-ctx-leak.test.ts   # 6 pass
# src/ reverted to origin/main
bun run build:release test test/js/sql/tls-sql.test.ts test/js/sql/postgres-tls-ctx-leak.test.ts   # 3 fail

Ready for review; needs a maintainer to look past the darwin-26-aarch64 artifact-download outage.