Bun.serve: require key+cert when a tls object is passed

[robobun]

What

Bun.serve's TLS-presence detection treats tuning fields (requestCert, rejectUnauthorized, lowMemoryMode, ciphers, minVersion/maxVersion, sessionTimeout, ...) as peers of key/cert when deciding whether to stand up a TLS listener, and never requires key/cert at all. Both directions of that are wrong:

// fail-OPEN: quietly serves plaintext http even though `tls` was given
Bun.serve({ tls: {}, fetch: () => new Response("ok") });
// url.protocol === "http:", plaintext fetch works

// cert-less https: listens as https:// but every handshake aborts
// with TLSV1_ALERT_INTERNAL_ERROR
Bun.serve({ tls: { requestCert: true }, fetch: () => new Response("ok") });

// the v0.2.1 legacy reader re-parses the *top-level* object, so a
// stray tuning field there also arms a cert-less https:// listener
Bun.serve({ lowMemoryMode: true, fetch: () => new Response("ok") });

// node parity: Node listens as TLS and every handshake fails closed;
// we listened as plaintext http
require("node:https").createServer({}, handler).listen(0);

Root cause

SSLConfig::from_generated computes any = passphrase || dhParams || ... || rejectUnauthorized.is_some() || requestCert || secureOptions || ... and returns Some whenever any tuning field is set. ServerConfig then arms TLS on Some (cert-less https:) and silently drops to plaintext on None (tls:{}). The legacy top-level reader applies the same check to the whole serve-options object.

Fix

Identity material is the only thing that arms TLS.

• SSLConfig::has_identity_material() reports (key || keyFile) && (cert || certFile). ca/crl are trust, not identity.
• Bun.serve({ tls: <object> }) now throws a synchronous TypeError: tls option requires both "key" and "cert" (or "keyFile" and "certFile") when the object (or any tls: [...] entry) lacks identity material. The node:http wrapper path (onNodeHTTPRequest) is exempt so node:https keeps Node's fail-closed behaviour.
• tls: false/0/null/undefined and tls: [] stay plain HTTP and leave the legacy top-level reader active (unchanged; covered by the [v1.2.20] Binding multiple certs for SNI via Bun.serve crashes #21792 regression test).
• The legacy top-level reader only keeps a parsed config that carries identity material; a lone tuning field is inert and never arms or throws. A top-level key without cert (or vice versa) now throws the same TypeError.
• https.Server is now its own constructor that stamps the internal is-TLS marker and delegates to http.Server, and https.createServer just returns new Server(...) (matching Node's structure). An https server is always a TLS listener: with no key/cert every handshake now fails (like Node v26) instead of serving plaintext. Observable change: require("node:https").Server !== require("node:http").Server (it was previously the same constructor); https.Server still extends http.Server so instanceof holds.

Verification

test/js/bun/http/bun-serve-ssl.test.ts gains a tls must carry key+cert matrix (10 throw cases + legacy/falsey controls) and a node:https without key/cert is TLS fail-closed matrix (plaintext fetch rejected, TLS handshake errors, createServer and new Server both covered, key+cert control, instanceof check). Roughly 20 of the new tests fail on main and all pass with this change.

Fixture adjustments: serve-types.test.ts's tls: {} case now asserts the constructor error, and serve.test.ts's "SNI missing serverName" case carries cert alongside key so it still reaches the serverName check.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 5 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ea42ed7d-048a-4676-8e81-b7f4bfc5065e

📥 Commits

Reviewing files that changed from the base of the PR and between 916492f and 73b4fc2.

📒 Files selected for processing (7)

• src/http/ssl_config.rs
• src/js/node/_http_server.ts
• src/js/node/https.ts
• src/runtime/server/ServerConfig.rs
• test/integration/bun-types/fixture/serve-types.test.ts
• test/js/bun/http/bun-serve-ssl.test.ts
• test/js/bun/http/serve.test.ts

---

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

[robobun]

^{Updated 8:10 PM PT - Jul 25th, 2026}

❌ @robobun, your commit 73b4fc2 has some failures in Build #81897 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 35810

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

bun-35810 --bun

[github-actions]

This PR may be a duplicate of:

1. Bun.serve: reject tls objects that have no certificate #35795 - Also rejects tls objects without key/cert in Bun.serve, modifying the same core files (ssl_config.rs, ServerConfig.rs)

🤖 Generated with Claude Code

[robobun]

Re #35795: there is real overlap (same identity helper on SSLConfig, same throw-on-no-identity in ServerConfig, same top-level legacy gate), but this PR additionally covers two cases that one leaves open:

• Bun.serve({ tls: {} }): SSLConfig::from_js returns None for an empty object, so the if let Some(ssl_config) guard in Bun.serve: reject tls objects that have no certificate #35795 never fires and the server still listens as plain http:. Here require_identity(None) throws.
• https.createServer({...}) without key/cert: Bun.serve: reject tls objects that have no certificate #35795 makes this throw via Bun.serve, where Node listens as a TLS server whose every handshake fails. Here the onNodeHTTPRequest path is exempt from the throw and node:https stamps isTlsSymbol so the listener is always TLS (fail-closed, never plaintext).

It also updates the two existing tests whose fixtures used tls: {} / key-without-cert. Happy to rebase on top of #35795 or fold them together, whichever is easier to land.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/runtime/server/ServerConfig.rs:1406-1412 — The array path still fails open: Bun.serve({ tls: [{}], fetch }) serves plaintext http: with no error. SSLConfig::from_js({}) returns None (no fields → any=false), the None arm hits continue without calling require_identity, args.ssl_config stays None, and saw_tls_key=true blocks the legacy reader. Call require_identity(global, None)? in the None arm (or after the loop when the array was non-empty and ssl_config is still None) so the array branch matches the object branch.

  Extended reasoning...

  What the bug is

  The PR's stated invariant is that a tls object passed to Bun.serve "can never quietly downgrade to plaintext". The non-array branch enforces this — tls: {} now throws TypeError: tls option requires both "key" and "cert". But the sibling array branch does not: Bun.serve({ tls: [{}], fetch }) still listens as plaintext http: with no error, exactly the fail-open this PR set out to close.

  Code path

  Step through Bun.serve({ port: 0, tls: [{}], fetch: () => new Response("ok") }) in ServerConfig::from_js:

  1. arg.get_truthy(global, "tls") returns Some([{}]); saw_tls_key = true.
  2. tls.js_type().is_array() is true, value_iter.len == 1, so the loop runs once with item = {}.
  3. SSLConfig::from_js(vm, global, {}) → from_generated computes any = false (no passphrase, no dhParams, no serverName, reject_unauthorized.is_none(), request_cert == false, no secureOptions, no key/cert/ca/crl, …) and returns Ok(None) (src/runtime/socket/SSLConfig.rs:265).
  4. The None arm at ServerConfig.rs:1401-1407 hits continue — require_identity is never called. The comment there still reads "Backwards-compatibility; we ignored empty tls objects", which is precisely the stance this PR overrides for the object case.
  5. Loop ends with args.ssl_config == None.
  6. saw_tls_key == true, so the legacy top-level reader at line 1446 is skipped.
  7. args.ssl_config remains None → the server listens as plaintext http:.

  Compare the non-array branch at line 1433: require_identity(global, parsed.as_ref())? runs with parsed == None, is_node_http == false, cfg.is_some_and(...) == false → throws. The array branch's new guard at line 1409 (require_identity(global, Some(&ssl_config))?) only runs on the Some arm, so an entry that parses to None never reaches it.

  Why existing code doesn't prevent it

  The None arm was deliberately a silent skip pre-PR ("Backwards-compatibility; we ignored empty tls objects"). This PR removed that back-compat for tls: {} but left the same comment and the same continue in the parallel array arm. The new test matrix's only array case is [{ key: tlsCert.key, rejectUnauthorized: false }], which returns Some (because reject_unauthorized.is_some() sets any = true) and so reaches the Some-arm guard — [{}] is not covered.

  Impact

  Same security fail-open the PR description leads with, reachable via a documented input shape (tls accepts an array for SNI). A user who writes tls: [{}] — or tls: [tlsOpts] where tlsOpts is accidentally {} — gets a plaintext server instead of an error. Per REVIEW.md "Security checks fail closed and cover every path to the protected effect … enumerate every route to the effect … and enforce through one shared predicate" and "Fix the whole class in the same PR … parallel switch arms", the array branch is a sibling entry point that must carry the same guard.

  Fix

  In the None arm, replace continue with require_identity(global, None)?; continue; (which throws for direct Bun.serve callers and lets node:http through, matching the object branch). Alternatively, after the loop, if the array was non-empty and args.ssl_config is still None, call require_identity(global, None)?. Either way, add ["array with empty entry", [{}]] to the withoutIdentity test matrix in bun-serve-ssl.test.ts and drop or update the now-stale "Backwards-compatibility; we ignored empty tls objects" comment.

[claude]

The three earlier findings (mixed keyFile/cert sources, https.Server constructor path, tls: [{}] array gap) are all addressed and I didn't find anything new. Deferring to a human because this is a user-facing breaking change to TLS semantics — Bun.serve({ tls: {} }) now throws, top-level tuning fields go inert, and https.Server becomes its own class — plus #35795 covers overlapping ground and someone needs to pick which lands.

What was reviewed:

• has_identity_material predicate now accepts any key-source × any cert-source; legacy top-level fallback drops non-identity configs without leaking (SSLConfig::Drop runs).
• require_identity closure: is_node_http exemption reaches the single-object arm and arms cert-less TLS via SSLConfig::zero; array arm's post-loop None check closes the [{}] gap.
• https.Server wrapper: isTlsSymbol survives the { ...options } spread in _http_server.ts; call-without-new and function-arg forms handled; existing instanceof https.Server assertions in test-https-set-timeout-server.js still hold since createServer now constructs the subclass.

Extended reasoning...

Overview

The PR touches src/http/ssl_config.rs (new has_identity_material() predicate), src/runtime/server/ServerConfig.rs (new require_identity closure gating the three tls-parsing arms plus a saw_tls_key flag on the legacy fallback), src/js/node/https.ts (Server becomes a real constructor that stamps isTlsSymbol and delegates to http.Server; createServer is now a thin wrapper), src/js/node/_http_server.ts (reads the stamped symbol so an https server is TLS even with no key/cert), and three test files with a ~16-case matrix.

Security risks

The change closes a fail-open: previously Bun.serve({ tls: {} }) silently served plaintext and tls: { requestCert: true } stood up a cert-less https listener. Now direct Bun.serve throws synchronously and node:https arms a fail-closed TLS listener (Node parity). I checked that the is_node_http exemption is keyed on on_node_http_request being set, which only _http_server.ts's listen() supplies — a user calling Bun.serve directly can't reach the exempt path without also supplying a callable onNodeHTTPRequest. The legacy top-level reader now only keeps a config carrying identity material, so a stray lowMemoryMode: true at the top level no longer arms a broken https listener; the discarded SSLConfig is dropped and its C strings freed by Drop.

Level of scrutiny

High — this is TLS listener setup, and the PR changes three public behaviors: (1) Bun.serve now throws on tls objects without key+cert, (2) top-level tuning fields no longer arm TLS, (3) require('node:https').Server is now a distinct subclass rather than === http.Server. All three are defensible fixes but are breaking changes a maintainer should sign off on. There is also an overlapping PR (#35795) that a human needs to reconcile.

Other factors

All three of my earlier inline findings were fixed in 75583b7 and I re-verified each in the current diff. I spot-checked existing Node parallel tests that call https.Server(opts, ...) without new and that assert instanceof https.Server — the new constructor's if (!(this instanceof Server)) guard and $toClass(Server, "Server", http.Server) prototype chain keep those passing. get_truthy("tls") filters falsy values, so tls: false/tls: null leave saw_tls_key = false and the legacy fallback still runs (unchanged from before). The test matrix covers the throw cases, the [{}] array gap, both createServer and new Server entry points, and the instanceof relationship.

[claude]

No further findings after 73b4fc2 — the is_falsey() guard is back and covered by the new tls: false/0/null/undefined control cases. Deferring to a human for the API/compat decisions here (new synchronous throw on tls: {}, the intentional tls: [] carve-out, https.Server becoming its own constructor) and to reconcile with the overlapping #35795.

What was reviewed:

• require_identity reaches every tls: arm (object, array entry, post-loop empty-array-of-objects) and the legacy top-level partial-identity path; is_node_http exemption plumbs through to SSLConfig::zero so node:https still arms a fail-closed listener.
• has_identity_material accepts mixed key/keyFile × cert/certFile sources; tls: [] and tls: false leave saw_tls_key = false so top-level key/cert still arm HTTPS.
• _http_server.ts reads options[isTlsSymbol] on the object branch, sets this[tlsSymbol] via normalizeServerTls, and kRealListen passes that object as tls: alongside onNodeHTTPRequest, so is_node_http is true when the identity check runs.

Extended reasoning...

Overview

The PR changes when Bun.serve decides to arm a TLS listener. It adds SSLConfig::has_identity_material() / has_any_key() / has_any_cert(), wires a require_identity closure into every tls: arm of ServerConfig::from_js (single object, each array entry, post-loop for arrays whose entries all parsed to None, and the legacy v0.2.1 top-level reader for partial identity), and exempts the node:https path via is_node_http. On the JS side, https.Server becomes its own constructor that stamps an internal isTlsSymbol marker on the options so _http_server.ts builds a tls object even when no key/cert is present, which kRealListen then hands to Bun.serve alongside onNodeHTTPRequest. Tests cover the throw matrix, falsey/empty carve-outs, legacy top-level paths, and node:https fail-closed behavior for both createServer and new Server.

Security risks

This is squarely in TLS-arming territory. The direction of the change is fail-safe (turn a silent plaintext downgrade into a synchronous throw, and turn a plaintext node:https listener into a fail-closed TLS one), and I traced each arm to confirm the identity check is not bypassable via a truthy-but-identity-less shape after the latest commits. The remaining plaintext carve-outs (tls: [], tls: false/0/NaN/null/undefined) are the pre-existing behavior and are now covered by regression tests plus the #21792 test.

Level of scrutiny

High. Security-sensitive TLS gating with several sibling arms (object / array / legacy top-level / node:http), a five-round review history where each round found a real gap, an intentional API tightening that will throw on inputs that previously produced a running (if broken) server, and a change to the https.Server prototype chain. That combination warrants a maintainer signing off on the API surface even though the mechanics now look correct.

Other factors

Test coverage is thorough for the new behavior. Two things a human should weigh: the deliberate tls: [] = plain-HTTP carve-out (author documented the rationale and the #21792 regression test that pins it), and reconciliation with the overlapping PR #35795. The https.Server !== http.Server change matches Node but is a user-observable identity change that isn't called out in the PR body.

[robobun]

CI status for 73b4fc2 (build #81897, finished): the diff itself is green on every lane that built.

• All 6 Linux build-bun jobs timed out (aarch64, x64, x64-asan, aarch64-musl, x64-musl, aarch64-android), cascading 148 waiting test jobs. No compile error output; build-queue infra.
• test/regression/issue/20144/20144.test.ts and test/cli/run/no-orphans.test.ts were flaky on darwin (signal/process-reaping races, unrelated to TLS).

All darwin and Windows lanes built and ran: bun-serve-ssl.test.ts, serve.test.ts, serve-types.test.ts, the node:https parallel tests, and 21792.test.ts all passed there. Ready for review.

[robobun]

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.