Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions ext/node/ops/tls_wrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,11 @@ struct TLSWrapInner {
/// is freed, so in-flight write callbacks can avoid a dangling deref.
alive: Rc<Cell<bool>>,

/// Set to true by JS when the TLSSocket is being destroyed/closed.
/// Checked after handshake callback to prevent sending application
/// data when checkServerIdentity fails.
closing: bool,

// Error string (like Node's error_)
error: Option<String>,

Expand Down Expand Up @@ -899,6 +904,7 @@ impl TLSWrapInner {
bytes_read: 0,
bytes_written: 0,
alive: Rc::new(Cell::new(true)),
closing: false,
error: None,
verify_error: Arc::new(std::sync::Mutex::new(None)),
pending_client_config: None,
Expand Down Expand Up @@ -934,12 +940,36 @@ impl TLSWrapInner {
return;
}
TLSWrapInner::do_enc_out_action(ptr, enc_action);

// After handshake completes, the JS callback (onhandshakedone ->
// onConnectSecure) has run. If the connection was accepted (e.g.
// checkServerIdentity passed), drain any pending cleartext that
// was buffered before the handshake. If JS destroyed the socket
// (identity check failed), the closing flag prevents sending.
if result.handshake_done && !(*ptr).closing {
Comment thread
bartlomieju marked this conversation as resolved.
(*ptr).cycling = true;
(*ptr).clear_in();
let enc_action2 = (*ptr).enc_out_collect();
(*ptr).cycling = false;
TLSWrapInner::do_enc_out_action(ptr, enc_action2);
}
}
}

/// Feed pending cleartext into rustls writer.
/// Mirrors Node's TLSWrap::ClearIn().
fn clear_in(&mut self) {
// Don't feed application data to rustls until the handshake is
// complete and JS has confirmed the connection (via onhandshakedone).
// In Node.js, the SSL_write for app data effectively happens after
// the handshake callback because OpenSSL fires the info callback
// synchronously during SSL_read. Without this gate, app data would
// be encrypted and sent before checkServerIdentity can reject the
// connection.
if !self.established || self.closing {
return;
}

let Some(ref mut conn) = self.tls_conn else {
return;
};
Expand Down Expand Up @@ -2155,6 +2185,15 @@ impl TLSWrap {
0
}

/// Mark the TLSWrap as closing. Called synchronously from JS
/// TLSWrap.close() so the Rust side knows not to send buffered
/// application data after a handshake callback rejects the connection.
#[fast]
fn set_closing(&self) {
let inner = unsafe { &mut *self.inner.as_mut_ptr() };
inner.closing = true;
}

/// Destroy the SSL connection. Tears down the TLS state without
/// re-entering JS (no write-completion callbacks).
#[nofast]
Expand Down
8 changes: 6 additions & 2 deletions ext/node/polyfills/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,10 @@ Object.setPrototypeOf(TLSSocket.prototype, net.Socket.prototype);
Object.setPrototypeOf(TLSSocket, net.Socket);

tlsWrap.TLSWrap.prototype.close = function close(cb) {
// Signal to Rust that this TLSWrap is being closed, so it won't
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick sanity-check question — is TLSWrap.close() the only path where the socket can be destroyed after a failed handshake-callback identity check?

Reading the flow: JS onhandshakedoneonConnectSecurecheckServerIdentity throws → caught somewhere → socket.destroy(err)Socket._destroythis._handle.close(cb) where _handle is the TLSWrap → enters this close method → setClosing().

Is that the only path? If JS rejects via some other mechanism (e.g., a 'secureConnect' listener that calls socket.destroy() outside the onConnectSecure handler, or an explicit tlsSocket.destroy() from user code) does that still go through TLSWrap.close() synchronously, or could it bypass and leave closing: false while the post-handshake drain runs?

Not blocking — the checkServerIdentity test should exercise the documented path — but worth confirming there's no alternative destroy entry point that skips setClosing().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- TLSWrap.close() is the only path. Every destroy route goes through Socket._destroy which calls this._handle.close(cb), and for a TLSSocket, _handle is always the TLSWrap instance, so it always dispatches to TLSWrap.prototype.close which calls setClosing() synchronously before anything else.

Even the user-code case (socket.destroy() from a 'secureConnect' listener or anywhere else) goes through the same Writable.destroy -> _destroy -> _handle.close() path -- there's no alternative entry point that could skip it.

_destroySsl() exists separately but it only calls destroySsl() (Rust teardown()), not close() -- and it's only called from TLSWrap.prototype.close itself (via the done callback) or from _tlsError cleanup, never as a standalone destroy path.

// send buffered application data after a failed identity check.
this.setClosing();

let ssl;
if (this._owner) {
ssl = this._owner.ssl;
Expand All @@ -301,7 +305,7 @@ tlsWrap.TLSWrap.prototype.close = function close(cb) {

if (this._parentWrap) {
if (this._parentWrap._handle === null) {
queueMicrotask(done);
nextTick(done);
return;
}

Expand All @@ -313,7 +317,7 @@ tlsWrap.TLSWrap.prototype.close = function close(cb) {
}

// Defer so callers can register "close" listeners after destroy().
queueMicrotask(done);
nextTick(done);
};

TLSSocket.prototype._wrapHandle = function (wrap, handle) {
Expand Down
14 changes: 9 additions & 5 deletions ext/node/polyfills/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1839,15 +1839,19 @@ Socket.prototype._destroy = function (exception, cb) {
}
}

this._handle.close(() => {
this._handle.onread = _noop;
this._handle = null;
this._sockname = undefined;
const handle = this._handle;
// Call cb first so the stream's error emission (via nextTick) is
// scheduled before the handle close callback. This ensures 'error'
// fires before 'close', matching Node.js behavior.
this._handle = null;
this._sockname = undefined;
cb(exception);
handle.close(() => {
handle.onread = _noop;

debug("emit close");
this.emit("close", isException);
});
cb(exception);
} else {
cb(exception);
nextTick(_emitCloseNT, this);
Expand Down
6 changes: 6 additions & 0 deletions tests/node_compat/config.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -1732,6 +1732,7 @@
"parallel/test-https-agent-unref-socket.js": {},
"parallel/test-https-agent.js": {},
"parallel/test-https-byteswritten.js": {},
"parallel/test-https-client-checkServerIdentity.js": {},
"parallel/test-https-close.js": {},
"parallel/test-https-connect-address-family.js": {},
"parallel/test-https-connecting-to-http.js": {
Expand All @@ -1756,7 +1757,12 @@
"parallel/test-https-server-options-server-response.js": {},
"parallel/test-https-server-request-timeout.js": {},
"parallel/test-https-set-timeout-server.js": {},
"parallel/test-https-simple.js": {},
"parallel/test-https-socket-options.js": {},
"parallel/test-https-strict.js": {
"ignore": true,
"reason": "NODE_TLS_REJECT_UNAUTHORIZED=0 does not fully suppress cert verification errors"
},
"parallel/test-https-timeout-server-2.js": {},
"parallel/test-https-timeout.js": {},
"parallel/test-https-truncate.js": {},
Expand Down
Loading