Repository navigation
Conversation
An explicit tls object that only carries tuning knobs (lowMemoryMode,
requestCert, rejectUnauthorized, secureOptions, passphrase, serverName,
ca) but no cert/key previously started an HTTPS listener with no server
identity: server.url reported https:, plain HTTP clients got ECONNRESET,
and TLS clients hung after ClientHello because no ServerHello could be
sent. The same keys at the top level (the legacy v0.2 flattened shape)
triggered the same broken listener via the back-compat read of the serve
options object.
SSLConfig::from_generated sets its "any TLS option present" flag for
each of these knobs, so the parser returned Some(SSLConfig) with
cert/key still empty and the listener was created in TLS mode.
Bun.serve now checks SSLConfig::has_server_identity() after parsing:
* tls: {...} / tls: [{...}] without a cert or key throws at construction
time, since the user explicitly asked for TLS but the server could
never complete a handshake.
* top-level keys without a cert or key are ignored so the server stays
plain HTTP, preserving the v0.2 back-compat only for the {cert, key}
shape it existed for.
|
Reproduced with CI build #82108 finished 194/196 green;
Flaky-tagged warnings ( |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesAdded server-identity detection to TLS identity validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:16 PM PT - Jul 25th, 2026
❌ @robobun, your commit a861719 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35795That installs a local version of the PR into your bun-35795 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits, I also checked: the explicit drop(ssl_config) before each new throw mirrors the existing SNI-error path and is leak-free (SSLConfig::Drop runs deinit); the top-level back-compat branch correctly drops the parsed config via scope exit when has_server_identity() is false; and tls: [{cert, key}, {serverName: "x"}] (SNI entry with only serverName) now throws at the new guard rather than reaching the later serverName check — still correct since a cert-less SNI entry is the same dead-handshake class.
Extended reasoning...
Two nits are posted inline covering the OR-vs-AND semantics of has_server_identity() and the unaddressed Bun.listen sibling. Both are pre-existing behavior, not regressions. This note records the additional paths examined so a later pass need not re-derive them.
has_server_identity() now requires (cert || certFile) && (key || keyFile)
so a tls object with only one half of the pair is rejected too, and the
error message is rephrased to read correctly whether zero or one of the
two is present. Covers {cert}-only and {key}-only in the test block.
![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 `@test/js/bun/http/bun-serve-ssl.test.ts`:
- Around line 6-73: Expand the TLS test matrix around noIdentity to include
partial certFile/keyFile combinations, asserting that explicit tls objects and
arrays reject incomplete identities with the existing error. Add successful
Bun.serve cases for complete file-backed certFile/keyFile credentials and valid
mixed inline/file credentials, preferably using test.each while preserving the
existing top-level legacy and tls:{} coverage.
🪄 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: 9419d461-34a3-45bd-94a6-9f11b3f76830
📒 Files selected for processing (3)
src/http/ssl_config.rssrc/runtime/server/ServerConfig.rstest/js/bun/http/bun-serve-ssl.test.ts
The "multiple missing server name" test used key-only entries, which now throw the new "must specify both cert and key" error before the SNI serverName check it is meant to exercise. Give each entry the harness tls.cert/tls.key so both pass has_server_identity() and the second entry reaches the serverName validation.
There was a problem hiding this comment.
All three earlier findings are addressed (the ||→&& in has_server_identity, the Bun.listen exclusion now stated in the description, and the serve.test.ts SNI fixture). Deferring for a maintainer to sign off on the user-facing behavior change — an explicit tls: object without both cert and key now throws where it previously started a (dead) listener.
What was reviewed:
has_server_identity()requires(cert || certFile) && (key || keyFile); the threeServerConfig::from_jscall sites (array entry, single object, legacy top-level) and their throw-vs-ignore split.drop(ssl_config)before eachreturn Errmatches the existing SNI-error pattern just below;SSLConfig: Drophandles the C-string frees.- Test matrix: 11 no-identity shapes × 3 placements,
tls: {}stays HTTP, top-level{cert, key}still enables HTTPS, and the updatedserve.test.tsfixture reaches the SNI check via the harnesstlsimport. server.reload()reuses the sameServerConfig::from_js, so the new validation applies there too — consistent with the intent.
Extended reasoning...
Overview
Adds SSLConfig::has_server_identity() (a 2-line predicate over cert/cert_file_name and key/key_file_name) and wires it into the three TLS-parsing sites in ServerConfig::from_js: array entries and single tls: objects throw tls object must specify both "cert" and "key" to start a TLS server; the legacy v0.2 top-level scan silently drops the parsed config so the server stays plain HTTP. ~75 lines of new tests in bun-serve-ssl.test.ts cover 11 no-identity shapes across all three placements plus back-compat guards, and one existing serve.test.ts fixture is updated to carry a full cert+key so it still reaches the SNI-serverName check.
Security risks
None introduced. The change is a fail-closed validation: it rejects configurations that previously produced an HTTPS listener incapable of completing a handshake. It does not touch BoringSSL context creation, certificate verification, or any handshake path — only the JS-side option parser.
Level of scrutiny
Medium. The code change is small and mechanical (one predicate + three guards following the local drop(ssl_config); return Err(...) idiom). What warrants human sign-off is the API contract change: Bun.serve({ tls: { requestCert: true }, ... }) and every other tuning-only or half-identity tls object now throws at construction time. That is unambiguously better than the previous dead-listener behavior, but it is a deliberate tightening on a widely-used public API and per repo convention a maintainer should confirm the throw (vs. warn / silently downgrade to HTTP) is the desired UX.
Other factors
- All three of my earlier inline findings were addressed in follow-up commits (6f17404, 063a7fb, e18e604, be735f4) and the threads are resolved. The
Bun.listensibling is intentionally excluded with rationale in the PR description. - The updated
serve.test.tsfixture usestls.key/tls.certfrom the existingharnessimport (line 14), so no new import is needed. server.reload()sharesServerConfig::from_js(server_body.rs:2322), so the guard applies uniformly there — expected and consistent.- The bug-hunting system found nothing on this revision.
These two snippets showed tls:{...} without a cert or key, which now
throws at construction. The adjacent Passphrase and CA examples on the
same page already include key/cert alongside the highlighted field.
There was a problem hiding this comment.
No issues found. All prior review feedback (has_server_identity() AND vs OR, the broken serve.test.ts SNI fixture, the tls.mdx doc snippets, and the Bun.listen scope note) has been addressed. Deferring to a human because this tightens user-facing Bun.serve validation — configs that previously started a (dead) HTTPS listener now throw at construction — and the top-level-ignore vs explicit-tls:-throw split is a small API decision worth a maintainer sign-off.
Checked this run: traced certFile/keyFile through from_generated — both set requires_custom_request_ctx → any = true, so from_js returns Some and the guard fires; handle_path's access() probe passes for the jsonwebtoken PEM fixtures used in the new tests. Grepped the test suite for other Bun.serve({ tls: {tuning-only} }) shapes that would newly throw — none beyond the already-fixed serve.test.ts SNI fixture; the many tls: { rejectUnauthorized: false } hits are all client-side (fetch/WebSocket).
Extended reasoning...
Overview
Adds SSLConfig::has_server_identity() (4-line predicate: (cert || certFile) && (key || keyFile)) and wires it into three sites in ServerConfig::from_js: the tls: [...] array loop, the single tls: {...} object branch, and the v0.2 top-level back-compat block. The first two throw tls object must specify both "cert" and "key" to start a TLS server; the third silently ignores so a top-level lowMemoryMode: true no longer arms a certificate-less HTTPS listener. Updates one existing serve.test.ts fixture so it still reaches its SNI-serverName assertion, adds ~35 tests in bun-serve-ssl.test.ts covering 11 no-identity shapes × 3 placements plus two back-compat guards, and adds key/cert to two tls.mdx doc snippets that would otherwise now throw.
Security risks
None introduced. This is defensive validation in the TLS-server construction path — it fails closed where the previous behavior started a listener that could never complete a handshake. No new parsing of untrusted input; no widening of accepted config shapes.
Level of scrutiny
Moderate. The Rust change is ~20 lines and mechanically simple, but it's a user-facing behavioral change to Bun.serve in a TLS-adjacent path: configs that previously "started" (uselessly) now throw. The top-level path deliberately ignores rather than throws — a reasonable call given accidental key-name collisions with unrelated serve options, and explained in the inline comment — but it's an API-shape decision. Bun.listen is intentionally left out (shared SocketConfig path with Bun.connect, needs mode-aware placement); the PR description documents this per the "say so if excluded" rule.
Other factors
This PR has been through four prior automated review rounds, each producing a concrete fix: (1) has_server_identity() changed from OR to AND with {cert}-only / {key}-only test coverage; (2) Bun.listen exclusion documented; (3) the serve.test.ts "multiple missing server name" fixture given a full cert+key pair so it reaches its named assertion; (4) two tls.mdx snippets given key/cert. All threads are resolved. The bug-hunting pass this run found nothing new. Test coverage is thorough (matrix over shapes × placements, back-compat guards, file-path variants). CI on the previous run had a fleet stall (zero test lanes ran); build #82108 is the re-run to watch.
|
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. |
2 participants
An explicit
tlsobject that only carries tuning knobs (lowMemoryMode,requestCert,rejectUnauthorized,secureOptions,passphrase,serverName,ca) or only one half of the cert/key pair previously started an HTTPS listener with no usable server identity:server.urlreportedhttps:, plain HTTP clients gotECONNRESET, and TLS clients hung after ClientHello because no ServerHello could be sent. The same keys at the top level (the legacy v0.2 flattened shape) triggered the same dead listener via the back-compat read of the serve options object.Repro
Cause
SSLConfig::from_generatedsets its "any TLS option present" flag for each of these knobs, so the parser returnedSome(SSLConfig)withcert/keystill empty andServerConfigcreated the listener in TLS mode. The v0.2.1 back-compat block inServerConfig::from_jsruns the same parser over the top-level serve options object whentlsis absent, so a top-levellowMemoryMode: truetook the same path.Fix
SSLConfig::has_server_identity()reports whether the config carries both a certificate and a private key (inline PEM or file path).ServerConfig::from_jschecks it after parsing:tls: {...}/tls: [{...}]without both a cert and a key throws at construction time withtls object must specify both "cert" and "key" to start a TLS server.{ cert, key }shape it existed for.Bun.listenshares the underlying parser but is intentionally left out of this change: itsSocketConfigpath is shared withBun.connect(where cert-less TLS is valid) and already acceptstls: truefor both modes, so wiring the guard there needs a mode-aware placement plus a decision on whattls: trueshould mean for a listener. That belongs in a follow-up rather than bundled here.Verification
test/js/bun/http/bun-serve-ssl.test.tsgains atls option without a server identityblock covering 9 no-identity shapes (7 tuning-only plus{cert}-only and{key}-only) acrosstls: {...},tls: [{...}], and top-level placement, plus two back-compat guards (tls: {}stays HTTP, top-level{cert, key}still enables TLS). On the released binary 27 of the 29 new tests fail; with the fix all 45 tests in the file pass on both debug+ASAN and release builds.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts