node:https: fail closed when createServer has no key or cert

[robobun]

What

https.createServer({}) with no key and no cert serves plaintext HTTP on the "https" listener. Node fails closed: it still builds a TLS listener, every handshake fails, and nothing is ever served in the clear.

import https from "node:https";
import net from "node:net";

const srv = https.createServer({}, (req, res) => res.end("SECRET:" + req.url));
srv.listen(0, "127.0.0.1", () => {
  const c = net.connect(srv.address().port, "127.0.0.1", () =>
    c.write("GET /secret HTTP/1.1\r\nHost: h\r\nConnection: close\r\n\r\n"));
  let buf = "";
  c.on("data", d => (buf += d));
  c.on("close", () => console.log(JSON.stringify(buf.slice(0, 30))));
});

// node:  ""                         <- nothing served in the clear
// bun:   "HTTP/1.1 200 OK\r\n..."    <- full response in cleartext

This matters because the misconfiguration class "key/cert didn't make it into the options object" (a typo'd option name, an undefined from a failed file read, a conditionally-assembled config) turns an intended https endpoint into a working cleartext http endpoint. It passes a naive "server is up and answers" health check, so nothing catches it.

Cause

node:https aliased Server/createServer straight to node:http:

Server: http.Server,
createServer: http.createServer,

http.Server only marks the server as TLS when it sees truthy key/cert/ca in the options. With none present, this[tlsSymbol] stays null, and Bun.serve({ tls: null }) starts a plain HTTP listener. Because https.createServer and http.createServer were the same function, the https layer could not tell "user wanted HTTPS" from "user wanted HTTP".

Fix

Node's https.Server extends tls.Server, so it is always a TLS listener regardless of cert material. This PR gives node:https its own Server class that forces the TLS path on before delegating to http.Server. With no cert the server becomes a certificate-less TLS listener whose handshakes all fail, matching Node, instead of a cleartext HTTP listener.

http.Server's TLS-option parsing is hoisted out of the else branch so the forced flag is honored even when the options argument is omitted or is a function (new https.Server(), https.createServer(requestListener)). http.createServer keeps its cleartext default for callers that pass no cert material.

Verification

test/js/node/http/node-https-server.test.ts covers it: a cleartext client gets nothing back, the TLS handshake fails, and http.createServer({}) still serves cleartext as a control. Fails on the released binary, passes with this change.

Out of scope

The report also notes that a garbage key/cert PEM is not rejected synchronously at createServer time (Node throws ERR_OSSL_PEM_NO_START_LINE; Bun surfaces it as a deferred 'error' at listen()). That is a separate timing/compat issue that also affects tls.createServer, which builds its native SSL context lazily at listen. It is not a fail-open security problem (the handshake still fails with bad PEM), so it is left for a focused follow-up.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 2 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: 43e68cc7-b2b4-40b6-94d1-fb0752700f6b

📥 Commits

Reviewing files that changed from the base of the PR and between 4aaae86 and 3c70124.

📒 Files selected for processing (3)

• src/js/node/_http_server.ts
• src/js/node/https.ts
• test/js/node/http/node-https-server.test.ts

Walkthrough

Changes

HTTPS TLS server behavior

Layer / File(s)	Summary
TLS mode construction src/js/node/_http_server.ts, src/js/node/https.ts	Certificate-less HTTPS servers retain TLS mode. The dedicated constructor preserves TLS options, ALPN handling, and validation.
HTTPS factory and export wiring src/js/node/https.ts	https.createServer() uses the dedicated Server constructor. https.Server references that constructor.
HTTPS behavior validation test/js/node/http/node-https-server.test.ts	Tests cover certificate-less TLS, valid credentials, cleartext rejection, HTTP behavior, handshake errors, and constructor identity.

Possibly related PRs

• oven-sh/bun#36707: Updates the same HTTP server TLS handling and adds SNI callback support.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fail-closed HTTPS behavior when createServer receives no key or certificate.
Description check	✅ Passed	The description explains the change, cause, fix, verification, and scope, including tests and compatibility behavior.
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.

---

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

[robobun]

^{Updated 4:52 AM PT - Aug 4th, 2026}

✅ @robobun, your commit 3c70124f4acc4e3bc29cc8662162c0065d9dbd0c passed in Build #88766! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 33539

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

bun-33539 --bun

[github-actions]

This PR may be a duplicate of:

1. node:https: expose tls.Server API on https.Server #31095 - Also creates a distinct https.Server class that sets isTlsSymbol = true to prevent plaintext fallback when no cert/key is supplied
2. node:https: add addContext and other tls.Server methods to https.Server #32435 - Same isTlsSymbol = true fix in a new https.Server class, with additional addContext/SNI support layered on top

🤖 Generated with Claude Code

[robobun]

Heads up on the duplicate-detection flag: #32435 already contains this exact fail-closed change. Its https.Server constructor seeds this[tlsSymbol] ??= { requestCert: false, rejectUnauthorized: false } so a cert-less https.createServer({}) builds a TLS listener instead of serving plaintext, same root cause and fix as here. That PR bundles it with addContext/SNI/setSecureContext support and some native uWS changes.

#31095 creates an https.Server class too but does not force the TLS path on, so it does not address the plaintext fallback.

This PR (#33539) is the minimal, security-only version: it isolates just the fail-closed fix (a distinct https.Server class that forces isTlsSymbol, plus hoisting the TLS-option parse so the flag is honored when options are omitted) with no new API surface or native changes. If #32435 is on track to land, this is redundant and can be closed in its favor; if a focused fix for the security issue is preferable to merge sooner, this is that. Happy to go either way.

[claude]

I didn't find any correctness issues, but this changes fail-open → fail-closed TLS behavior and makes https.Server a distinct class, so a maintainer should sign off (and pick between this and #32435).

Extended reasoning...

Overview

This PR stops https.createServer({}) from silently serving plaintext HTTP when no key/cert are provided. It does so by giving node:https its own Server class that pre-sets isTlsSymbol before delegating to http.Server, and hoisting the TLS-option parse in _http_server.ts out of the else branch so the pre-set flag is honored even when options is omitted or is a function. A new test file exercises three call shapes, verifies cleartext requests get nothing back, verifies the TLS handshake fails with an alert (not WRONG_VERSION_NUMBER), and includes a control that http.createServer({}) still serves plaintext.

Security risks

The change itself is a security fix (fail-closed instead of fail-open on cert misconfiguration), and I don't see it introducing new exposure — http.createServer keeps its plaintext default, and the hoisted option reads are all no-ops on the {} fallback. But it is squarely in TLS/HTTPS listener-setup code, which is exactly the kind of path the approval guidelines say to leave for a human.

Level of scrutiny

Medium-high. The logic is small and the mechanism is clear (isTlsSymbol → tlsSymbol → Bun.serve({ tls })), but it changes public API identity (https.Server !== http.Server, instanceof semantics) and flips the default behavior of a misconfigured-but-previously-working call pattern. That's an intentional Node-compat correction, but it's the kind of behavioral shift a maintainer should be aware of.

Other factors

Two open PRs (#31095, #32435) overlap with this one, and #32435 contains the same fail-closed fix bundled with SNI/addContext work — robobun already flagged that a maintainer needs to choose which to land. Tests look solid (event-driven, no sleeps, control case included). No CODEOWNERS coverage on these files.

[robobun]

Status: rebased onto main at 3c70124. Folded main's ALPN-defaulting createServer into the new https.Server class so new https.Server(...) gets the same ALPN defaults as createServer(...). All review threads resolved (trimmed the new comments, added new https.Server and ALPN coverage to the test).

Locally: test/js/node/http/node-https-server.test.ts is 17/17 on the debug build, 12/17 fail on canary 2190ef1a3 (fail-before holds); all 59 test-https-*.js node tests pass on debug.

[coderabbitai]

Actionable comments posted: 3

🤖 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/node/_http_server.ts`:
- Around line 324-325: Shorten the explanatory comment blocks near the PKCS#12
handling in the HTTPS server implementation to one concise line each, including
the blocks around the visible comment and the referenced locations. Retain only
durable, non-obvious implementation context and remove workaround rationale or
Node parity explanations.

In `@test/js/node/http/node-https-server.test.ts`:
- Around line 107-110: Extend the https.Server tests to cover direct
construction with new https.Server(options, requestListener), including listener
behavior. Add ALPN cases verifying the default ALPNProtocols value ["http/1.1"],
preservation of an explicit ALPNProtocols value, and storage of an ALPNCallback
on the created instance.
- Around line 42-47: The comment above the HTTPS test is overly detailed;
replace it with one concise line stating the invariant that missing TLS
key/certificate material must not create a cleartext HTTP listener. Remove the
historical behavior and failure-context explanation while preserving the test
logic.

🪄 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: 57997737-a94b-4bcc-8a34-4176a99731cb

📥 Commits

Reviewing files that changed from the base of the PR and between af63b8a and 4aaae86.

📒 Files selected for processing (3)

• src/js/node/_http_server.ts
• src/js/node/https.ts
• test/js/node/http/node-https-server.test.ts

[claude]

I didn't find any bugs — the fail-closed mechanism (pre-setting isTlsSymbol before delegating to http.Server, with the TLS-option parse hoisted so the flag is honored on all argument shapes) checks out, and the test matrix is thorough. Deferring to a human because this is security-sensitive TLS server code and there's a merge-order decision to make vs. #32435, which bundles the same fix with SNI/addContext support.

What was reviewed:

• Verified the _http_server.ts dedent is behavior-preserving for plain http.Server: this[tlsSymbol] = null at line 312 makes the removed else redundant, and options = {} on the function/null paths means the hoisted TLS reads are all no-ops.
• Confirmed http.Server never resets isTlsSymbol, so the value pre-set by https.Server survives into the normalizeServerTls branch.
• Checked the double validateObject/spread between https.Server and http.Server is harmless.

Extended reasoning...

Overview

Three files: src/js/node/https.ts gains a real Server class (previously aliased to http.Server) that forces this[isTlsSymbol] = true before delegating, folds in the ALPN-defaulting logic that used to live only in createServer, and wires createServer to it. src/js/node/_http_server.ts hoists the TLS-option parsing block out of the else branch so the pre-set isTlsSymbol is honored even when options is a function or omitted; the diff is almost entirely a two-column dedent plus one new one-line comment. test/js/node/http/node-https-server.test.ts is new and covers the fail-closed invariant across four constructor shapes, a positive control (valid cert still serves over TLS), a negative control (http.createServer({}) stays plaintext), instanceof identity, and ALPN defaults for both createServer() and new Server().

Security risks

The change is in the security-hardening direction — it converts a fail-open (misconfigured https.createServer serves plaintext) into a fail-closed (cert-less TLS listener whose handshakes all fail), matching Node. I traced the mechanism end to end: https.Server sets isTlsSymbol → http.Server.$call runs, line 312 resets tlsSymbol (not isTlsSymbol) → the hoisted parse leaves key/cert undefined → if (this[isTlsSymbol]) at line 373 is true → tlsSymbol becomes a normalizeServerTls({key: undefined, cert: undefined, requestCert: false, rejectUnauthorized: false, ...}) object → listen() reads it as truthy tls and hands it to the native layer, which starts a TLS listener with no cert. The residual risk is the usual one for TLS-path edits: an unforeseen interaction with the native Bun.serve({ tls: {...} }) path when key/cert are undefined. The test's _ALERT_ assertion and the reported 59/59 pass on test-https-*.js node tests give reasonable confidence, but a human familiar with the uSockets TLS layer should confirm.

Level of scrutiny

High — this is the node:https server constructor and it changes when a listener is TLS vs. plaintext. That said, the actual code delta is small (~30 net new lines in https.ts, a dedent in _http_server.ts) and the direction is strictly safer than before.

Other factors

All prior review threads (comment-cop, coderabbit, my own comment-style note) are resolved in 3c70124. The duplicate-PR bot flagged #32435 as carrying the same fix bundled with addContext/SNI support and native uWS changes; the author acknowledged this and offered to close in its favor. That's a maintainer call — landing this focused security fix now vs. waiting for the larger PR — which is another reason to defer rather than approve.