node:https: add addContext and other tls.Server methods to https.Server - #32435
node:https: add addContext and other tls.Server methods to https.Server#32435robobun wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SNI (Server Name Indication) context support to Bun's HTTPS server by introducing a ChangesHTTPS Server SNI Context Support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:25 PM PT - Aug 16th, 2026
❌ @robobun, your commit 2233ef2 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32435That installs a local version of the PR into your bun-32435 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![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 `@src/js/node/https.ts`:
- Around line 151-157: The error messages in both the getTicketKeys and
setTicketKeys methods contain a typo where "implented" should be spelled as
"implemented". Update the Error messages in both Server.prototype.getTicketKeys
and Server.prototype.setTicketKeys to correct the spelling from "Not implented
in Bun yet" to "Not implemented in Bun yet".
🪄 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: a1bf7bd6-4259-40b8-af3d-7a43ec5bed54
📒 Files selected for processing (4)
src/js/internal/http.tssrc/js/node/_http_server.tssrc/js/node/https.tstest/js/node/http/node-https-server-context.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
7b35335 to
a028e90
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/https.ts`:
- Around line 84-91: The validation checks in the tlsOptionsFromContext function
and related TLS option validations use truthiness checks (if cert, if key, if
ca, if passphrase) that allow invalid falsy values like 0, false, and empty
strings to bypass type validation. Replace these truthiness checks with explicit
null and undefined checks instead. For example, change "if (passphrase && typeof
passphrase !== 'string')" to "if (passphrase != null && typeof passphrase !==
'string')" for the passphrase validation on line 89, and apply the same pattern
to all other TLS option validations including cert, key, and ca. Also apply this
same fix pattern to the similar validation logic mentioned at lines 140-153.
In `@src/js/node/tls.ts`:
- Around line 622-635: The function tlsStringToProtocolVersion() returns 0 for
invalid version strings, but this invalid result is not validated after the
conversion. In the newNativeSecureContext() function around lines 743-747 and in
the TLSSocket path around lines 1790-1801, add validation checks after each call
to tlsStringToProtocolVersion() that verify the returned value is not 0. If the
result is 0 (indicating an invalid protocol version string was passed), throw an
ERR_TLS_INVALID_PROTOCOL_VERSION error instead of silently using the invalid
value, to match Node.js behavior and prevent invalid strings like "invalid" from
being treated as valid protocol versions.
In `@src/runtime/server/server_body.rs`:
- Around line 2553-2564: The H3 SNI registration error handling in the
`Self::HAS_H3` block is incomplete because it only returns an error when
`add_server_name_with_options` fails AND `global.has_exception()` is false. This
causes the function to silently continue when an exception is already pending.
Remove the `&& !global.has_exception()` condition from the if statement so that
any failure from `add_server_name_with_options().is_err()` always returns an
error. Additionally, add a separate check after the if block to return
`Err(JsError::Thrown)` if `global.has_exception()` is true at that point,
mirroring the H1 error path pattern and ensuring no failures are swallowed.
In `@test/js/node/http/node-https-server-context.test.ts`:
- Around line 23-30: The peerCN function establishes an SNI connection but only
checks the peer certificate, not whether the correct route was actually selected
by SNI. Enhance the function to send an actual HTTP GET request over the TLS
socket after the secureConnect event completes, read and return the response
body instead of just the certificate CN, and ensure the caller asserts this
response matches the expected body for the a.example.com SNI-selected route.
This will prove the test fails when SNI routing is not working correctly.
🪄 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: 3e0185b7-de54-473b-b108-47eea20c3ae6
📒 Files selected for processing (7)
src/js/internal/http.tssrc/js/node/_http_server.tssrc/js/node/https.tssrc/js/node/tls.tssrc/runtime/node/node_http_binding.rssrc/runtime/server/server_body.rstest/js/node/http/node-https-server-context.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 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/https.ts`:
- Around line 84-91: The validation checks in the tlsOptionsFromContext function
and related TLS option validations use truthiness checks (if cert, if key, if
ca, if passphrase) that allow invalid falsy values like 0, false, and empty
strings to bypass type validation. Replace these truthiness checks with explicit
null and undefined checks instead. For example, change "if (passphrase && typeof
passphrase !== 'string')" to "if (passphrase != null && typeof passphrase !==
'string')" for the passphrase validation on line 89, and apply the same pattern
to all other TLS option validations including cert, key, and ca. Also apply this
same fix pattern to the similar validation logic mentioned at lines 140-153.
In `@src/js/node/tls.ts`:
- Around line 622-635: The function tlsStringToProtocolVersion() returns 0 for
invalid version strings, but this invalid result is not validated after the
conversion. In the newNativeSecureContext() function around lines 743-747 and in
the TLSSocket path around lines 1790-1801, add validation checks after each call
to tlsStringToProtocolVersion() that verify the returned value is not 0. If the
result is 0 (indicating an invalid protocol version string was passed), throw an
ERR_TLS_INVALID_PROTOCOL_VERSION error instead of silently using the invalid
value, to match Node.js behavior and prevent invalid strings like "invalid" from
being treated as valid protocol versions.
In `@src/runtime/server/server_body.rs`:
- Around line 2553-2564: The H3 SNI registration error handling in the
`Self::HAS_H3` block is incomplete because it only returns an error when
`add_server_name_with_options` fails AND `global.has_exception()` is false. This
causes the function to silently continue when an exception is already pending.
Remove the `&& !global.has_exception()` condition from the if statement so that
any failure from `add_server_name_with_options().is_err()` always returns an
error. Additionally, add a separate check after the if block to return
`Err(JsError::Thrown)` if `global.has_exception()` is true at that point,
mirroring the H1 error path pattern and ensuring no failures are swallowed.
In `@test/js/node/http/node-https-server-context.test.ts`:
- Around line 23-30: The peerCN function establishes an SNI connection but only
checks the peer certificate, not whether the correct route was actually selected
by SNI. Enhance the function to send an actual HTTP GET request over the TLS
socket after the secureConnect event completes, read and return the response
body instead of just the certificate CN, and ensure the caller asserts this
response matches the expected body for the a.example.com SNI-selected route.
This will prove the test fails when SNI routing is not working correctly.
🪄 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: 3e0185b7-de54-473b-b108-47eea20c3ae6
📒 Files selected for processing (7)
src/js/internal/http.tssrc/js/node/_http_server.tssrc/js/node/https.tssrc/js/node/tls.tssrc/runtime/node/node_http_binding.rssrc/runtime/server/server_body.rstest/js/node/http/node-https-server-context.test.ts
🛑 Comments failed to post (4)
src/js/node/https.ts (1)
84-91:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFalsy values bypass TLS option type validation.
Line 89 and Lines 140-153 use truthiness checks (
if (value && typeof value !== ...)), so invalid falsy values like0,false, and""pass validation and get written into TLS config.Suggested fix
- if (passphrase && typeof passphrase !== "string") { + if (passphrase !== undefined && passphrase !== null && typeof passphrase !== "string") { throw $ERR_INVALID_ARG_TYPE("options.passphrase", "string", passphrase); } @@ - if (passphrase && typeof passphrase !== "string") { + if (passphrase !== undefined && passphrase !== null && typeof passphrase !== "string") { throw $ERR_INVALID_ARG_TYPE("options.passphrase", "string", passphrase); } @@ - if (servername && typeof servername !== "string") { + if (servername !== undefined && servername !== null && typeof servername !== "string") { throw $ERR_INVALID_ARG_TYPE("options.servername", "string", servername); } @@ - if (secureOptions && typeof secureOptions !== "number") { + if (secureOptions !== undefined && secureOptions !== null && typeof secureOptions !== "number") { throw $ERR_INVALID_ARG_TYPE("options.secureOptions", "number", secureOptions); }Also applies to: 139-154
🤖 Prompt for 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. In `@src/js/node/https.ts` around lines 84 - 91, The validation checks in the tlsOptionsFromContext function and related TLS option validations use truthiness checks (if cert, if key, if ca, if passphrase) that allow invalid falsy values like 0, false, and empty strings to bypass type validation. Replace these truthiness checks with explicit null and undefined checks instead. For example, change "if (passphrase && typeof passphrase !== 'string')" to "if (passphrase != null && typeof passphrase !== 'string')" for the passphrase validation on line 89, and apply the same pattern to all other TLS option validations including cert, key, and ca. Also apply this same fix pattern to the similar validation logic mentioned at lines 140-153.src/js/node/tls.ts (1)
622-635:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
# Check the actual state of src/js/node/tls.ts around the flagged lines sed -n '620,650p' src/js/node/tls.ts sed -n '740,750p' src/js/node/tls.ts sed -n '1785,1805p' src/js/node/tls.tsRepository: oven-sh/bun
Length of output: 2129
🏁 Script executed:
# Find where DEFAULT_MIN_VERSION and DEFAULT_MAX_VERSION are defined and exported rg 'DEFAULT_MIN_VERSION|DEFAULT_MAX_VERSION' src/js/node/tls.ts -B2 -A2Repository: oven-sh/bun
Length of output: 2635
🏁 Script executed:
# Check if these constants are exported and writable rg 'export.*DEFAULT_MIN_VERSION|export.*DEFAULT_MAX_VERSION' src/js/node/tls.tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look at the Node.js TLS spec behavior for invalid protocol versions # Check what the native layer expects rg 'tlsStringToProtocolVersion|TLS1_VERSION' src/js/node/tls.ts -B2 -A2 | head -60Repository: oven-sh/bun
Length of output: 1193
🏁 Script executed:
# Check if there are tests for invalid TLS versions find test -name "*.test.ts" -o -name "*.test.js" | xargs rg -l "tls|TLS" | head -5Repository: oven-sh/bun
Length of output: 242
🏁 Script executed:
# Find test files specifically for node:tls fd -e test.ts -e test.js | rg tlsRepository: oven-sh/bun
Length of output: 1663
🏁 Script executed:
# Look at the node-tls-create-secure-context-args test head -100 test/js/node/tls/node-tls-create-secure-context-args.test.tsRepository: oven-sh/bun
Length of output: 3074
🏁 Script executed:
# Search for tests that explicitly test invalid version strings rg 'TLSv|minVersion|maxVersion' test/js/node/tls/node-tls-create-secure-context-args.test.ts -A3 -B1 | head -80Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there's validation in the native layer by searching for error handling rg 'minVersion|maxVersion' src/runtime -A2 -B2 | head -100Repository: oven-sh/bun
Length of output: 1497
🏁 Script executed:
# Look for what happens when 0 is passed as version rg 'set_min_proto_version|set_max_proto_version|minVersion.*0' src/ -r -A2 -B2 | head -80Repository: oven-sh/bun
Length of output: 1165
🏁 Script executed:
# Search for how the native layer handles minVersion/maxVersion, especially 0 rg 'ssl_min_version|ssl_max_version' src/ -A3 -B3 | head -100Repository: oven-sh/bun
Length of output: 5931
🏁 Script executed:
# Check if there's any validation when a version is 0 rg 'minVersion.*0|maxVersion.*0' src/runtime -B2 -A2 | head -60Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for the actual native code that sets protocol versions rg 'SSL_CTX_set_min_proto_version|SSL_CTX_set_max_proto_version' src/ -B3 -A3 | head -80Repository: oven-sh/bun
Length of output: 915
🏁 Script executed:
# Search for what happens with the 0 return value from tlsStringToProtocolVersion rg 'tlsStringToProtocolVersion' src/js/node/tls.ts -A5 -B5Repository: oven-sh/bun
Length of output: 1370
🏁 Script executed:
# Check Node.js behavior: does it validate when you set tls.DEFAULT_MIN_VERSION? # Look at existing tests for setting these defaults rg 'DEFAULT_MIN_VERSION|DEFAULT_MAX_VERSION' test/js/node/tls/ -B3 -A3Repository: oven-sh/bun
Length of output: 1178
🏁 Script executed:
# Check if the native layer actually has defaults for when 0 is passed rg 'ssl_min_version.*0|ssl_max_version.*0' src/http/ -B5 -A5Repository: oven-sh/bun
Length of output: 681
🏁 Script executed:
# Search for where BoringSSL gets called with these versions rg 'SSL_CTX_set_min_proto_version|SSL_CTX_set_max_proto_version' src/ --type cpp --type rs -B2 -A2 | head -60Repository: oven-sh/bun
Length of output: 82
🏁 Script executed:
# Check what Node.js actually does - look at the github link in the comment rg 'secureProtocolToVersionRange|Node seeds the protocol' src/js/node/tls.ts -B5 -A15Repository: oven-sh/bun
Length of output: 3637
🏁 Script executed:
# Check if there are any tests setting invalid version strings to DEFAULT_MIN/MAX_VERSION rg 'DEFAULT_MIN_VERSION|DEFAULT_MAX_VERSION' test/js/node/tls -A10 | grep -A10 'invalid\|throw\|error'Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look at Node.js source to understand what happens with invalid strings # Check if Node validates the version string when DEFAULT_MIN_VERSION is set # First, let's see what the actual behavior is supposed to be node -e "const tls = require('tls'); tls.DEFAULT_MIN_VERSION = 'invalid'; console.log(tls.DEFAULT_MIN_VERSION); const ctx = tls.createSecureContext(); console.log('created context');" 2>&1 || trueRepository: oven-sh/bun
Length of output: 825
🏁 Script executed:
# Since we can't run Node.js in this environment, check what the review expects # Let's see if there's any validation elsewhere in the code rg 'ERR_TLS_INVALID_PROTOCOL_VERSION' src/Repository: oven-sh/bun
Length of output: 1096
🏁 Script executed:
# Look at what happens when 0 is actually passed to the native layer # Check the C++ bindings that consume these values fd -e cpp -e h | xargs rg 'ssl_min_version|ssl_max_version' 2>/dev/null | head -40Repository: oven-sh/bun
Length of output: 305
🏁 Script executed:
# Check the exact lines referenced in the review to see current state sed -n '622,635p' src/js/node/tls.tsRepository: oven-sh/bun
Length of output: 338
🏁 Script executed:
# Check if validation is already present in the file rg '\$ERR_TLS_INVALID_PROTOCOL_VERSION' src/js/node/tls.ts -B5 -A2Repository: oven-sh/bun
Length of output: 700
🏁 Script executed:
# Get the full context of where validation happens sed -n '740,765p' src/js/node/tls.tsRepository: oven-sh/bun
Length of output: 1059
🏁 Script executed:
# Find VALID_TLS_VERSIONS definition rg 'VALID_TLS_VERSIONS' src/js/node/tls.ts -B5 -A5Repository: oven-sh/bun
Length of output: 1344
🏁 Script executed:
# Check the full context of the newNativeSecureContext function to see where validation happens sed -n '700,780p' src/js/node/tls.tsRepository: oven-sh/bun
Length of output: 3847
🏁 Script executed:
# Check the third location mentioned in "Also applies to: 743-747, 1790-1801" sed -n '1785,1810p' src/js/node/tls.tsRepository: oven-sh/bun
Length of output: 737
Validate translated TLS protocol versions; invalid DEFAULT_MIN/MAX_VERSION strings silently degrade bounds.
When
tlsStringToProtocolVersion()receives an invalid version string (including fromDEFAULT_MIN_VERSION/DEFAULT_MAX_VERSION), it returns0. The current code innewNativeSecureContext()(lines 743–747) converts these strings to numeric protocol versions but never validates the result. If a developer setstls.DEFAULT_MIN_VERSIONto an invalid string (e.g.,tls.DEFAULT_MIN_VERSION = "invalid"), andcreateSecureContext()is called without explicitminVersion, the invalid string silently converts to0and disables the version bound instead of throwing.Node.js rejects this with
ERR_TLS_INVALID_PROTOCOL_VERSIONat context creation time. Add validation after each string-to-number conversion to match that behavior:Suggested fix
} else { minVersion = tlsStringToProtocolVersion(optMinVersion ?? DEFAULT_MIN_VERSION); maxVersion = tlsStringToProtocolVersion(optMaxVersion ?? DEFAULT_MAX_VERSION); + if (minVersion === 0) throw $ERR_TLS_INVALID_PROTOCOL_VERSION(String(optMinVersion ?? DEFAULT_MIN_VERSION), "minimum"); + if (maxVersion === 0) throw $ERR_TLS_INVALID_PROTOCOL_VERSION(String(optMaxVersion ?? DEFAULT_MAX_VERSION), "maximum"); }Also applies to lines 1790–1801 in the TLSSocket path.
🤖 Prompt for 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. In `@src/js/node/tls.ts` around lines 622 - 635, The function tlsStringToProtocolVersion() returns 0 for invalid version strings, but this invalid result is not validated after the conversion. In the newNativeSecureContext() function around lines 743-747 and in the TLSSocket path around lines 1790-1801, add validation checks after each call to tlsStringToProtocolVersion() that verify the returned value is not 0. If the result is 0 (indicating an invalid protocol version string was passed), throw an ERR_TLS_INVALID_PROTOCOL_VERSION error instead of silently using the invalid value, to match Node.js behavior and prevent invalid strings like "invalid" from being treated as valid protocol versions.src/runtime/server/server_body.rs (1)
2553-2564:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate H3 SNI registration failures instead of falling through.
When the H3
add_server_name_with_optionscall fails whileglobal.has_exception()is already true, this condition is false and the method continues toset_routes()and returns success with a pending exception. Mirror the H1 error path and returnErr(JsError::Thrown)whenever the H3 registration fails or leaves an SSL/JS error pending. As per coding guidelines, “Never swallow a failure or signal success on one.”🐛 Proposed fix
if Self::HAS_H3 { if let Some(h3_app) = self.h3_app { - if bun_opaque::opaque_deref_mut(h3_app) + if bun_opaque::opaque_deref_mut(h3_app) .add_server_name_with_options(z, &ssl_opts) .is_err() - && !global.has_exception() { + if !global.has_exception() && !super::throw_ssl_error_if_necessary(global) { + return Err(global.throw(format_args!( + "Failed to add serverName \"{}\" for HTTP/3", + bstr::BStr::new(server_name.to_bytes()) + ))); + } + return Err(JsError::Thrown); + } + if super::throw_ssl_error_if_necessary(global) { + return Err(JsError::Thrown); + } - return Err(global.throw(format_args!( - "Failed to add serverName \"{}\" for HTTP/3", - bstr::BStr::new(server_name.to_bytes()) - ))); - } } }🤖 Prompt for 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. In `@src/runtime/server/server_body.rs` around lines 2553 - 2564, The H3 SNI registration error handling in the `Self::HAS_H3` block is incomplete because it only returns an error when `add_server_name_with_options` fails AND `global.has_exception()` is false. This causes the function to silently continue when an exception is already pending. Remove the `&& !global.has_exception()` condition from the if statement so that any failure from `add_server_name_with_options().is_err()` always returns an error. Additionally, add a separate check after the if block to return `Err(JsError::Thrown)` if `global.has_exception()` is true at that point, mirroring the H1 error path pattern and ensuring no failures are swallowed.Source: Coding guidelines
test/js/node/http/node-https-server-context.test.ts (1)
23-30:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExercise the SNI-selected route, not only the
Hostheader.
fetch("https://127.0.0.1:...")does not usea.example.comas TLS SNI; theHostheader is sent after TLS context selection. This assertion can pass through the default route even if the new SNI domain never gets routes installed. Reusetls.connect({ host: "127.0.0.1", port, servername: "a.example.com" }), send an HTTP request over that socket, and assert the response body. As per coding guidelines, “Prove the test fails for the RIGHT reason” and “Every assertion must be able to fail, and assert the strongest invariant.”🧪 Proposed test helper shape
async function peerCN(port: number, servername?: string) { const socket = tls.connect({ host: "127.0.0.1", port, servername, rejectUnauthorized: false }); const errored = once(socket, "error"); await Promise.race([once(socket, "secureConnect"), errored.then(([e]) => Promise.reject(e))]); const cert = socket.getPeerCertificate(); socket.destroy(); return cert.subject?.CN; } + +async function httpsBodyViaSNI(port: number, servername: string) { + const socket = tls.connect({ host: "127.0.0.1", port, servername, rejectUnauthorized: false }); + const errored = once(socket, "error").then(([e]) => Promise.reject(e)); + try { + await Promise.race([once(socket, "secureConnect"), errored]); + socket.write(`GET / HTTP/1.1\r\nHost: ${servername}\r\nConnection: close\r\n\r\n`); + + const chunks: Buffer[] = []; + socket.on("data", chunk => chunks.push(chunk)); + await Promise.race([once(socket, "end"), once(socket, "close"), errored]); + + const response = Buffer.concat(chunks).toString("utf8"); + return response.slice(response.indexOf("\r\n\r\n") + 4); + } finally { + socket.destroy(); + } +}- const res = await fetch(`https://127.0.0.1:${port}/`, { - tls: { rejectUnauthorized: false, checkServerIdentity: () => undefined }, - headers: { Host: "a.example.com" }, - }); - expect(await res.text()).toBe("ok"); + expect(await httpsBodyViaSNI(port, "a.example.com")).toBe("ok");Also applies to: 95-99
🤖 Prompt for 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. In `@test/js/node/http/node-https-server-context.test.ts` around lines 23 - 30, The peerCN function establishes an SNI connection but only checks the peer certificate, not whether the correct route was actually selected by SNI. Enhance the function to send an actual HTTP GET request over the TLS socket after the secureConnect event completes, read and return the response body instead of just the certificate CN, and ensure the caller asserts this response matches the expected body for the a.example.com SNI-selected route. This will prove the test fails when SNI routing is not working correctly.Source: Coding guidelines
|
Re the four CodeRabbit findings that failed to post inline (review 4517716418), addressed in 34b011c:
Declined with reasons:
|
|
CI build 63164: 284 passed, 2 failed. Neither failure touches this diff:
The new Ready for review. |
baa7f22 to
23ef95d
Compare
462eff6 to
3ede0f2
Compare
|
Rebased on main and squashed to one commit (3ede0f2). Conflicts resolved in All 7 tests in |
d42fcd3 to
20921b0
Compare
|
CI note (kept current in place): Head Build #99491 for it: every lane that has finished is green except two Local, on the rebased debug+ASAN build: Nothing left to do here from my side; ready for review. |
20921b0 to
8ff6c5b
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
8e9dce9 to
4bf7ca1
Compare
There was a problem hiding this comment.
I reviewed the latest revision (4bf7ca1) and found no issues — all three prior findings (the OwnedString wrap, the setSecureContext validate-before-commit ordering, and the unhandled closed promise) are addressed in the current diff. Given the scope — a new native binding, SSL_CTX lifecycle changes in App.h/openssl.c, and ~100 lines of Rust in add_sni_context with a manual SSL_CTX_free — a human look at the native memory-safety and TLS changes is still warranted.
What was reviewed:
add_sni_context's probe-before-remove ordering: the probe SSL_CTX is freed on success, and every error path returns beforeremove_server_namemutates stateus_internal_ssl_ctx_clear_sni_userdataclears the ex_data slot so keep-alive connections on a removed SNI fall back to the default router instead of dereferencing the freed one; covered by the keep-alive re-add testsetSecureContextnow validates every option before replacingthis[tlsSymbol]in one assignment; covered by the invalid-key test- The keep-alive test's
closedpromise is now in the firstPromise.race, so no unhandled rejection on early failure
Extended reasoning...
Overview
This PR makes https.Server a distinct subclass of http.Server and wires addContext, setSecureContext, getTicketKeys, and setTicketKeys onto it. It spans 11 files across four layers: JS built-ins (https.ts, _http_server.ts, internal/http.ts, tls.ts), a new Rust binding (node_http_binding.rs → server_body.rs::add_sni_context), a restored uWS FFI wrapper (App.rs::remove_server_name), and C/C++ changes in vendored uSockets/uWS (openssl.c, libusockets.h, App.h) that add us_internal_ssl_ctx_clear_sni_userdata and call it from removeServerName to prevent a UAF on the per-domain router when a keep-alive connection outlives its SNI context. A 271-line test file exercises pre- and post-listen SNI registration, hostname re-add, malformed-PEM rejection, keep-alive across re-add, and setSecureContext atomicity.
Security risks
The PR is squarely in TLS/SNI territory: it constructs SSL_CTX objects from user-supplied key/cert/CA material, registers them on a running server, and normalizes requestCert/rejectUnauthorized for both the default and per-hostname contexts. The requestCert/rejectUnauthorized normalization in tlsOptionsFromContext and setSecureContext mirrors normalizeServerTls (rejectUnauthorized only applies when requestCert is true), and the post-listen path passes apply_client_cert_policy = true to match listen-time entries. I did not spot a way to weaken verification relative to what Bun.serve's tls-array path already does, but a maintainer familiar with #36174's client-cert-policy semantics should confirm the parity.
Level of scrutiny
High. This is not a mechanical change: it introduces a new native entry point that mutates a live uWS SSL app, adds an unsafe { SSL_CTX_free(probe) } call in Rust, and patches vendored C++ (App.h::removeServerName) to fix a use-after-free that this PR's own re-add path would otherwise expose. The C/C++ change is small and well-commented, and the keep-alive test covers it, but changes to vendored uSockets/uWS memory management are exactly the kind of thing REVIEW.md flags for maintainer sign-off.
Other factors
The PR has been through three prior automated review rounds; each finding (a WTFStringImpl ref leak, a partial-write-on-throw in setSecureContext, a bare .toThrow(), and an unhandled-rejection edge in the keep-alive test) was fixed and is verified in the current diff. The comment-cop bot's flags were addressed by shortening comments. Test coverage is thorough for the JS-visible surface. The two known limitations (no SecureContext object support, post-listen setSecureContext deferred to next listen()) are documented in the description and match tls.Server's current behavior. Deferring to a human is appropriate given the native/TLS surface, not because of any specific unresolved concern.
|
Follow-up in 1c01f2e after a self-review of the rebased diff, which turned up two things the rebase had left behind:
Six tests added for these ( |
1c01f2e to
6259011
Compare
6259011 to
1bcd34d
Compare
a15fb2a to
f623106
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs this pass. The two nits from the previous round (ArrayPrototypeSplice capture, and rolling back the SNI entry on the H3 failure path / dropping the dead post-success error-queue checks in add_sni_context) are both addressed in f623106. Because this touches TLS/SNI plumbing end to end — SSL_CTX lifecycle across openssl.c/App.h (including the keep-alive-connection UAF guard in removeServerName), a new native binding into a running uWS app, and the shared server-TLS option pipeline — a human look would still be worthwhile.
What was reviewed:
add_sni_contexterror paths: probe-before-remove ordering, H3 rollback, no route-less SNI entry left registered on any exit.App.h::removeServerNameclearing the per-SNI ex_data slot so live keep-alive sockets fall back to the default router instead of the freed one.serverTlsFromOptionsextraction from_http_server.ts— verified the moved block is byte-equivalent modulo thealwaysTlsgate;normalizeServerTlsstill applied on every path.kSNIContextsdedup + thekRealListentls-array assembly (default entry always first, primordial-safe splice/push).
Extended reasoning...
Overview
This PR makes https.Server a real subclass of http.Server and wires addContext/setSecureContext/get/setTicketKeys onto it, matching Node's tls.Server inheritance. It spans 14 files: the JS layer (https.ts, _http_server.ts, a new shared serverTlsFromOptions helper in internal/tls.ts, the kSNIContexts symbol, an empty-hostname guard on tls.Server#addContext), a new native binding (httpServerAddServerName → add_sni_context in server_body.rs), the restored uws_remove_server_name FFI in App.rs, and a memory-safety change in the C/C++ usockets/uWS layer (us_internal_ssl_ctx_clear_sni_userdata + its call site in App.h::removeServerName). Fifteen new tests plus one added to node-tls-context.test.ts.
Security risks
The change is squarely in TLS territory: per-SNI SSL_CTX construction, client-certificate policy propagation (requestCert/rejectUnauthorized carried through normalizeServerTls and apply_client_cert_policy), and swapping SNI contexts on a running server. The App.h hunk is a UAF guard — a keep-alive connection accepted under a since-removed SNI entry would otherwise dereference the freed per-domain HttpRouter* via us_socket_server_name_userdata(); the fix nulls the ex_data slot so HttpContext falls back to the default router. That is the right shape and is covered by the "re-add does not break keep-alive connections" test, but it is exactly the kind of native lifetime change a maintainer should sign off on. I did not find a path where an https server's ca alone starts requiring client certificates (the normalizeServerTls defaults are preserved on every entry point), nor a way for a rejected setSecureContext to leave a half-applied config.
Level of scrutiny
High. This is not a mechanical change: it introduces a new native entry point that mutates a live uWS SSL app, restores a previously-removed FFI symbol, refactors the constructor's TLS option pipeline into a shared helper used by three call sites, and adds C code that manipulates BoringSSL ex_data on an SSL_CTX still referenced by open sockets. It has been through eleven review iterations here with every finding addressed, and the test coverage is thorough (pre-/post-listen, repeated hostnames, pfx, minVersion clearing, invalid-option atomicity, keep-alive across re-add), but the surface area and the native memory-safety component put it outside what an automated review should approve on its own.
Other factors
All prior inline findings from earlier passes are marked resolved with matching fix commits, and the current diff reflects the last two (primordial splice; add_sni_context no longer has post-success error exits that could strand a route-less SNI entry, and the H3 failure path now unregisters the TCP entry it just added). The _http_server.ts diff is a clean move of the option block into internal/tls.ts plus the new kSNIContexts fan-out at listen time. The two documented limitations (no SecureContext-object input, post-listen setSecureContext deferring to next listen()) match tls.Server's current behavior in Bun and are called out in the description.
https.Server was a direct alias for http.Server, so none of the tls.Server prototype methods (addContext, setSecureContext, getTicketKeys, setTicketKeys) were available on servers created via https.createServer(). In Node.js https.Server extends tls.Server which provides these. Make https.Server its own class extending http.Server (the ALPN defaulting that https.createServer did moves into the constructor, so new https.Server() gets it too) and implement addContext by buffering SNI contexts and passing them to Bun.serve as the tls array on listen; when called after listen, a new native binding (httpServerAddServerName) registers the SNI context on the running uWS SSL app, with the same per-serverName client-certificate policy the listen-time entries get. Repeated addContext for the same hostname replaces the previous context (last wins) instead of throwing, and the new SSL_CTX is validated before the old entry is removed. uWS removeServerName now clears the per-domain SNI userdata so keep-alive connections fall back to the default router rather than dereferencing the freed one. setSecureContext updates the default TLS config before listen and applies the same requestCert/rejectUnauthorized normalization as normalizeServerTls. getTicketKeys/setTicketKeys are stubbed the same way they are on tls.Server. Fixes #12157 Fixes #24474 Co-authored-by: Sam <sam.shridhar1950f@gmail.com>
…them setSecureContext wrote each option into the live TLS config as it went, so a later option failing validation left the server with a partially updated config (for example a new cert paired with the previous key). Validate everything first and replace the config in one step, the way tls.Server does. Pin the malformed-PEM addContext assertion to the PEM error and shorten the comments around the SNI registration sequence.
…xt and setSecureContext
addContext() and setSecureContext() validated their own short list of
options, so everything http.Server's constructor has since learned to
accept (pfx, minVersion/maxVersion, secureProtocol, ciphers) was silently
dropped: a pfx context registered with no certificate, and a version pin
given to setSecureContext() was ignored while one set in the constructor
survived it. Move that option handling into one internal/tls helper used
by the constructor and both methods. https.Server marks itself TLS before
the constructor runs, so it always gets a full config and no longer needs
a hand-made one; setSecureContext() rebuilds the config from the options
it was given and keeps only the server's requestCert/rejectUnauthorized,
as in Node.
addContext("") now throws ERR_TLS_REQUIRED_SERVER_NAME up front instead
of failing the later listen() (or, after listen, filing an entry on the
SNI tree root that nothing can match), and the native binding rejects an
empty name the same way Listener::add_server_name does.
tls.Server#addContext() gets the same up-front check; it used to accept
the empty name and fail later, at listen() or in the native binding.
After add_server_name_with_options() succeeded, add_sni_context() still had two exits (the error-queue checks after the add and after domain()) that returned before set_routes(), leaving the hostname registered with an empty router so every request for it was closed without a response. Both checks were dead anyway: building the context clears the error queue first and domain() makes no TLS calls. Drop them, and when the HTTP/3 registration fails, remove the entry just added to the TCP app so nothing is left half-registered. Also use the captured Array.prototype.splice in addContext(), like the push next to it.
f623106 to
2233ef2
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues; the earlier findings from prior runs have all been addressed. Because it's a ~800-line change spanning TLS/SNI handling (security-sensitive), a new native binding, and SSL_CTX/router lifetime management in uWS C/C++ and Rust, a human look is still warranted.
What was reviewed:
add_sni_contexterror paths — no exit leaves a route-less SNI entry registered; H3 failure now rolls back the TCP entry.removeServerNameUAF fix in App.h — the ex_data slot is cleared so keep-alive connections fall back to the default router; covered by the keep-alive re-add test.serverTlsFromOptionsrefactor — verified byte-for-byte equivalence with the constructor logic it replaces in_http_server.ts;setSecureContextvalidate-then-assign ordering.- Primordial captures (
ArrayPrototypeSplice/Push) and theERR_TLS_REQUIRED_SERVER_NAMEguard applied to bothhttps.Serverandtls.Serversiblings.
Extended reasoning...
Overview
This PR makes https.Server a distinct subclass of http.Server and wires addContext/setSecureContext/getTicketKeys/setTicketKeys to it (fixing #12157/#24474). It touches 14 files across four layers: built-in JS modules (src/js/node/https.ts, _http_server.ts, tls.ts, internal/tls.ts, internal/http.ts), a new Rust native binding (node_http_binding.rs → server_body.rs::add_sni_context), the uWS Rust FFI (App.rs restoring remove_server_name), and uWS/usockets C/C++ (App.h::removeServerName clearing SNI ex_data, openssl.c adding us_internal_ssl_ctx_clear_sni_userdata). The constructor's TLS-option validation is extracted into a shared serverTlsFromOptions helper so the constructor, addContext, and setSecureContext cannot drift. 15 new tests plus a sibling test in node-tls-context.test.ts.
Security risks
TLS/SNI is security-sensitive by nature. Specific surfaces I looked at: (1) the per-SNI client-certificate policy — add_sni_context passes apply_client_cert_policy = true matching the listen-time array path, and setSecureContext preserves the server's requestCert/rejectUnauthorized rather than letting a certificate swap silently drop mTLS enforcement (covered by a test); (2) empty-hostname rejection so an empty name never registers on the SNI tree root; (3) NUL-byte rejection before CString::new; (4) the UAF hazard in App.h::removeServerName where live keep-alive connections holding a ref on the removed SSL_CTX would otherwise dereference the freed per-domain router — the new us_internal_ssl_ctx_clear_sni_userdata clears the ex_data slot so those connections fall back to the default router. I did not find a way for user input to bypass validation or leave a half-registered SNI entry.
Level of scrutiny
High. This is not a mechanical change: it adds a new native FFI entry point, manipulates SSL_CTX refcounts and per-domain HttpRouter lifetimes in C++, and reorders remove/add on a running server. The refactor of _http_server.ts's TLS-option handling into internal/tls.ts is behavior-preserving on inspection but shifts ~100 lines of load-bearing validation. The keep-alive-across-re-add UAF fix in App.h is exactly the kind of change that benefits from a second pair of eyes on the ownership model (pendingServerNames is the single owner; listeners hold borrowed pointers; live SSL* objects hold their own SSL_CTX ref via SSL_set_SSL_CTX).
Other factors
The PR has been through 12 iterations with multiple prior review rounds; every earlier finding (dead error-queue checks after a successful add, ArrayPrototypeSplice primordial, unhandled-rejection in the keep-alive test, sibling tls.Server#addContext guard) has been addressed and all threads are resolved. Test coverage is thorough (pre/post-listen, repeated hostnames, invalid material rollback, pfx/minVersion, client-cert policy retention, keep-alive re-add). The known limitations (SecureContext objects not accepted, post-listen setSecureContext deferred to next listen()) are documented in the description and match tls.Server's current behavior. Given the breadth across native TLS plumbing, deferring to a human reviewer rather than auto-approving.
1 participant
What does this PR do?
Fixes #12157.
Fixes #24474.
https.Serverwas a direct alias forhttp.Server, so none of thetls.Servermethods (addContext,setSecureContext,getTicketKeys,setTicketKeys) were available on servers created viahttps.createServer(). In Node.js,https.Serverextendstls.Server, which provides these.Reproduction
Fix
https.Serveris now its own class extendinghttp.Server(sohttpsServer instanceof http.Serveris stilltrue, andhttp.Serverdoes not gain these methods).addContext(hostname, context)accepts the same options as the constructor (key/cert/ca, pfx, minVersion/maxVersion, secureProtocol, ciphers, per-context requestCert/rejectUnauthorized; an empty hostname throwsERR_TLS_REQUIRED_SERVER_NAMElike Node) and buffers the context. Whenlisten()is called, the buffered SNI contexts are passed toBun.servevia thetlsarray so eachserverNameis matched via SNI. When called afterlisten(), a new native binding (httpServerAddServerName) registers the SNI context on the running uWS SSL app viauws_add_server_name_with_optionsand reinstalls routes for that domain.setSecureContext(options)rebuilds the default TLS config used atlisten()from the options given (an omitted option is cleared, as in Node), keeping the server'srequestCert/rejectUnauthorized; like Node, providingcaalone does not start requiring a client certificate. Nothing is applied unless every option validated.addContext()andsetSecureContext()share one option pipeline (serverTlsFromOptionsininternal/tls, extracted fromhttp.Server's constructor), so the three cannot drift apart again.https.Servermarks itself TLS beforehttp.Serverruns, so it gets a real config even when constructed without certificates.getTicketKeys()/setTicketKeys()are stubbed the same way as ontls.Server. Also fixed the existing"implented"typo in thetls.Serverstubs.Verification
New tests at
test/js/node/http/node-https-server-context.test.ts:https.Serverexposes all four methods and is anhttp.ServersubclassaddContextbeforelisten(): connecting with SNI"a.example.com"and"b.example.com"returns the respective per-hostname certs (CNagent1/agent3); an unknown SNI falls through to the default cert (CNagent2)addContextafterlisten(): same SNI behavior on a running server, plus an HTTPS request still reaches the request handlersetSecureContextbeforelisten()replaces the default certsetSecureContext({key, cert, ca})on a server created with no initial TLS options does not require a client certificateaddContext("")throws before and afterlisten(), andpfxworks throughaddContext()(both phases) andsetSecureContext()setSecureContext()appliesminVersion, clears aminVersionthe constructor had set, rejects an unknownsecureProtocolwithout applying anything, and keeps the constructor's client certificate policy (a client with a certificate from the configured CA is served the new certificate, one without is refused)All tests fail on
main(TypeError: server.addContext is not a function) and pass with this change. Existingtest/js/node/tls/node-tls-context.test.ts,test/js/node/tls/node-tls-server.test.ts,test/js/node/http/node-https-checkServerIdentity.test.ts, and thetest-https-*Node parallel tests still pass.Supersedes #31095 by @sam-shridhar1950f, which introduced the same
https.Serversubclass split with method stubs (credited with aCo-authored-bytrailer on the commit); this PR additionally wiresaddContextto the underlying SNI mechanism, both at listen time and on a running server, so the contexts actually select per-hostname certificates. Makinghttps.Servera distinct class also fixes theinstanceofconfusion from #31125 (supertest <= 6.1.6 and@astrojs/nodetreat everyhttp.Serveras HTTPS on Bun).Known limitations (unchanged from the original review)
addContext()/setSecureContext()take plain{ key, cert, ca, ... }objects; atls.createSecureContext()result is not accepted, becauseBun.serve's TLS config is built from PEM material and cannot adopt an existingSSL_CTX. Supporting it needs native plumbing shared withtls.Serverand is left as a follow-up.setSecureContext()afterlisten()only affects the nextlisten(), the same as Bun'stls.Servertoday; there is no uWS primitive for swapping a running app's default context.addContext()afterlisten()does take effect.Rebase notes (current main)
https.createServer()on main had grown the ALPN defaulting from Node'shttps.Serverconstructor; that logic now lives in the newServerconstructor (as in Node), andcreateServer()returnsnew Server(...), sonew https.Server()gets the same defaults.apply_client_cert_policy = truetoadd_server_name_with_options, matching what the listen-timetlsarray entries get since Bun.serve: honor requestCert/rejectUnauthorized on per-serverName tls entries #36174, so a context'srequestCert/rejectUnauthorizedbehave the same whether it was added before or afterlisten().App::remove_server_name(theuws_remove_server_namebinding, removed as unused in Remove ~39k lines of dead Rust across the workspace #35002) is restored since it now has a caller.https.getinstead of parsing raw bytes, since the server now sends thewriteHead(); end("ok")body chunked.instanceofcoverage from node:https: expose tls.Server API on https.Server #31095 is carried over as a test here (http.createServer()is not anhttps.Server,http.Serverdoes not gainaddContext).setSecureContext()used to update the live config in place (a rejected option left a half-applied config; the test for it fails against that version withKEY_VALUES_MISMATCHat listen), and both methods still used the option list from before node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488, sopfx,minVersion/maxVersion,secureProtocolandcipherswere silently dropped (apfxcontext registered with no certificate). Both are fixed by the shared helper above;ERR_TLS_REQUIRED_SERVER_NAMEis added toErrorCode.tsfor the empty-hostname case, and the native binding rejects an empty name likeListener::add_server_namedoes.tls.Server#addContext()gets the same up-front check (it used to accept""and fail later atlisten()), covered intest/js/node/tls/node-tls-context.test.ts.add_server_name_with_optionsthe post-listen path had two dead error-queue checks that returned beforeset_routes()(which would have left the hostname registered with an empty router); they are removed, and a failed HTTP/3 registration now removes the entry it just added to the TCP app.tls.Server#addContext()and theaddContextdedup loop (ArrayPrototypeSplice) were tidied in the same rounds.test/js/node/http/node-https-server-context.test.ts(15/15, all fail on main),node-http.test.ts,node-https-checkServerIdentity.test.ts,node-http-agent-tls-options.test.mts,node-tls-context.test.ts,node-tls-server.test.ts,bun-serve-ssl.test.ts, and all 76test-https-*Node parallel tests; the only failures seen also fail on main in this environment (proxy env vars,localhostresolving to::1). Also 15/15 on Windows x64 (debug build). TheminVersiontest reads the refused handshake through a try/catch helper rather thanexpect().rejects: the latter spins a nested event loop, which on Windows hits a pre-existing usockets crash that reproduces on main with a plainhttps.createServer({ minVersion })(the libuv backend never setstick_depth, so a nested tick frees the socket whose callback is still on the stack); reported separately.[review] gate passed · iteration 12 · 14 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 12
evidence per changed file