tls: rotate the live certificate on setSecureContext() and Bun.serve reload({tls}) - #33365
Open
robobun wants to merge 11 commits into
Open
tls: rotate the live certificate on setSecureContext() and Bun.serve reload({tls})#33365robobun wants to merge 11 commits into
robobun wants to merge 11 commits into
Claude / Claude Code Review
completed
Jul 29, 2026 in 25m 43s
Code review found 1 important issue
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/server/server_body.rs:2342-2351 |
Bun.serve reload({tls}) does not rotate the SNI-tree entry when serverName is set |
| 🟡 Nit | src/runtime/server/server_body.rs:2338-2358 |
Bun.serve reload({tls}) does not rotate the HTTP/3 (QUIC) listener's certificate |
| 🟡 Nit | src/uws_sys/ListenSocket.rs:112-124 |
Dead code: find_server_name_userdata wrapper added with no callers |
Annotations
Check failure on line 2351 in src/runtime/server/server_body.rs
claude / Claude Code Review
Bun.serve reload({tls}) does not rotate the SNI-tree entry when serverName is set
For `Bun.serve({ tls: { ..., serverName } })`, `reload({tls})` does not rotate the certificate for clients that send SNI matching `serverName` — the `node->ctx == old` check in `us_listen_socket_set_ssl_ctx` never holds because uWS's `addServerName` registers a *separate* `domainCtx` in the SNI tree, not the App's `sslCtx` that `ls->ssl_ctx` points at. So the SNI entry is misclassified as a caller-owned `addContext()` and skipped, and browsers/`fetch()` (which always send SNI) keep receiving the
Check warning on line 2358 in src/runtime/server/server_body.rs
claude / Claude Code Review
Bun.serve reload({tls}) does not rotate the HTTP/3 (QUIC) listener's certificate
The rotation only swaps `self.listener` (the TCP h1/h2 listen socket); `self.h3_app` / `self.h3_listener` are untouched, so `Bun.serve({http3: true, tls}).reload({tls: newTls})` serves the new certificate over h1/h2 and the retired one over h3 — the same expired-cert split this PR fixes for TCP. `on_reload_from_zig` already clears h3 routes a few lines up, so h3 is in scope for reload; per REVIEW.md's "fix the whole class / if a site is intentionally excluded, say so", worth either wiring a `us_
Check warning on line 124 in src/uws_sys/ListenSocket.rs
claude / Claude Code Review
Dead code: find_server_name_userdata wrapper added with no callers
The `find_server_name_userdata<T>()` wrapper (plus its FFI extern `us_listen_socket_find_server_name_userdata` and the `use core::ptr::NonNull` import added for its return type) has no callers anywhere in `src/` — only `set_ssl_ctx` is used by this PR. Per REVIEW.md ("Every line you add must be demonstrably live" / "Public items escape dead-code lints — grep for callers manually"), suggest dropping the wrapper, the extern, and the import.
Loading