Repository navigation
fix(tls): preserve servernames set before connecting - #197
Merged
Merged
Conversation
Read the pending socket servername when preparing TLS options, preserving explicit empty names while continuing to suppress implicit host SNI. Add Node-compatible server-observed direct and address-family-selection regressions.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Hi! I'm the It looks like you correctly set up a CI job that uses the autofix.ci GitHub Action, but the autofix.ci GitHub App has not been installed for this repository. This means that autofix.ci unfortunately does not have the permissions to fix this pull request. If you are the repository owner, please install the app and then restart the CI workflow! 😃 |
autofix.ciKeep the existing public-name fallback while prioritizing pending setServername state. Cover false and zero names with explicit contexts, and apply the CI import ordering.
Promote the two existing pre-connect SNI tests now that pending names reach the handshake, and remove their obsolete inherited-gap comments. Their assertions are unchanged.
steipete
added a commit
that referenced
this pull request
Oct 24, 2026
…al design (#198) This reverts f45fff0 (#195), resolving overlap with #196 without restoring its Error.cause serialization changes. The fixed small-payload Worker postMessage gate measured a median paired ratio of 1.053 against a maximum 1.03, with 9 of 10 pairs slower. Remove the per-container bookkeeping pending a design that charges only large-Buffer paths. Preserve the #197 pre-connect SNI fix and the historical #195 changelog entry, with a new backout note.
steipete
added a commit
that referenced
this pull request
Oct 24, 2026
…al design (#198) This reverts f45fff0 (#195), resolving overlap with #196 without restoring its Error.cause serialization changes. The fixed small-payload Worker postMessage gate measured a median paired ratio of 1.053 against a maximum 1.03, with 9 of 10 pairs slower. Remove the per-container bookkeeping pending a design that charges only large-Buffer paths. Preserve the #197 pre-connect SNI fix and the historical #195 changelog entry, with a new backout note.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A bare
tls.TLSSocketwithsetServername("set.before.connect")called beforeconnect()sent no SNI. The server receivedfalse, while Node.js 26 and stock Bun nightly received the explicit name. Both direct dialing andautoSelectFamilywere affected.net.Socket.connect()replaces the publicservernamefield with its options. TLS already records pending names in a private socket field, but the fork adapter stopped reading it. Read that field first, preserving an explicitly cleared empty name and the existing omission behavior for absent/null names and falsy names with an explicit secure context. Connection-host and shared-context names remain excluded.The October 16 sync (
2c9435668e6, #177) reapplied #171 while dropping this private-field lookup from its upstream parent. The current oven-sh#45260 patch already retains the lookup. The fix stays in the Node TLS adapter; native transport ownership is unaffected.The portable fixture now covers pending-name replacement, empty names, family selection, and falsy explicit-context controls. Promote the two existing pre-connect SNI expected failures to ordinary tests with their assertions unchanged. Update the Node compatibility note and changelog.
Validation on head
c8e312cdd8380d8951726a7138000367bb6ca2af:ef37c31de3, promoted pre-connect testsNative CI and executable artifacts. Both native selections include the full hostname-verification file,
node-tls-connect, TLS server, wrapped-socket, and upgrade suites. The downloaded executable's synthetic merge tree exactly matches the PR head tree.