tls: deliver decrypted bytes before SSLWrapper answers close_notify (wss via CONNECT proxy reports 1006 on a server close) - #43198
Conversation
A wss:// WebSocket through a CONNECT proxy reported close code 1006 "Failed to write" when the server closed. A server that closes sends its Close frame and ends the TLS session behind it (ws.close() in Bun.serve does), so the frame and the close_notify usually arrive in one read. A direct wss:// connection reports the server's code. SSLWrapper::handle_reading called shutdown(false) as soon as SSL_read returned SSL_ERROR_ZERO_RETURN. That sent our close_notify before the data callback ran for the bytes decrypted in the same read. write_data refuses to write after that, so the WebSocket client could not echo the Close frame and failed the connection. Run the data callback first, then send the close_notify, then run the close callback. The uSockets path in openssl.c uses the same order.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change reorders SSL remote-close handling so buffered data and pending events are processed before shutdown. New TLS and WebSocket proxy tests verify event ordering and clean close handling when shutdown records arrive with application data. ChangesTLS shutdown handling
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The TLS shutdown ordering change preserves safe teardown and has targeted regression coverage for the affected event ordering. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status Reproduction, on 1.4.3-canary.1 (release) and on a debug build of main, Linux x64 and Windows x64: USE_SYSTEM_BUN=1 bun test test/js/web/websocket/websocket-proxy.test.ts -t "server close with the TLS close_notify"
PR: #43198 |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/uws/lib.rs— Every wss client that closes from inside the tunnel's last data callback now ends TLS without ever sending close_notify, where the base branch always sent it. After the fix the Close echo runs before shutdown(false), so the WebSocket client's clear_data() calls shutdown(true) at src/uws/lib.rs:1049 with sent_ssl_shutdown still false and takes the fast path (631-665), which queues close_notify into the wbio but never calls handle_writing; the guard at 1049 then returns before the deferred shutdown(false) at 1062 runs. Fix: the fast path must drain the write BIO (handle_writing) before trigger_close_callback, for every owner (WebSocket tunnel, fetch ProxyTunnel::close_raw, UpgradedDuplex::close).Extended reasoning...
Server-initiated close through a CONNECT proxy is the population this PR targets; it happens on every such close (20/20 per the PR). Trace: SSL_read returns 12 bytes then ZERO_RETURN; received_ssl_shutdown set (1035); data callback at 1047 delivers the Close frame. websocket_client.rs:1146 enqueue_encoded_bytes -> WebSocketProxyTunnel::write -> write_data (759) succeeds, SSL_write queues the echo, handle_traffic re-enters (1148) and flushes it. websocket_client.rs:1148-1150: send_buffer empty -> clear_data() -> WebSocketProxyTunnel::shutdown -> w.shutdown(true). At 596 sent_ssl_shutdown is false (the whole point of the fix), so the early-return branch is skipped and the fast path runs: SSL_shutdown twice (631,651) queues close_notify in the memory BIO, set_received_ssl_shutdown, trigger_close_callback (663), return. No handle_writing call anywhere on that path; the only drain is at 683 in the non-fast branch. Back in handle_reading, closed_notified is true so 1049 returns false; 1056-1064 never run. The queued close_notify is freed with the SSL. On the base, shutdown(false) ran before…
Verification: nit; acknowledged in diff (PR description only, not in code): "A fast shutdown queues close_notify but does not flush it ... So in this case the tunnel no longer writes a close_notify reply" — the bound stated there is accurate, but the description does not weigh what an origin that waits for the peer's close_notify does. Trigger: a wss origin behind a CONNECT proxy sends its Close frame and…
…ly over a Duplex node:tls over a Duplex uses the same SSLWrapper as the WebSocket proxy tunnel. The peer's last application data and its close_notify reach the engine in one chunk. The 'data' event must fire before the engine writes its close_notify reply to the transport, as in Node.
|
Reply to the two review findings. 1. Order test for 2. No close_notify when the owner closes from the last data callback. I did not change the fast path of
The correct place for this is the clean close of the WebSocket tunnel: a graceful |
Problem
wss://WebSocket through a CONNECT proxy reports close code 1006Failed to writewhen the server closes. A direct connection reports the code of the server. A server ends TLS behind its Close frame (ws.close()inBun.servedoes), so the frame and the close_notify arrive in one read.SSLWrapper::handle_reading(src/uws/lib.rs:1033). OnSSL_ERROR_ZERO_RETURNit sent our close_notify (shutdown(false)) before it ran the data callback for the bytes decrypted in the same read.write_data, andenqueue_encoded_bytes(src/http_jsc/websocket_client.rs:881) callsterminate(FailedToWrite).Fix
handle_readingruns the data callback first, thenshutdown(false), then the close callback.openssl.cuses this order for sockets with a file descriptor.test/js/web/websocket/websocket-proxy.test.ts(the four proxy cases fail without the fix) and one fornode:tlsover a Duplex intest/js/node/tls/node-tls-connect.test.ts. Alsotest/js/bun/http/proxy.test.ts, andnode-tls-namedpipes.test.tson Windows.Background
SSLWrapperis a TLS engine over memory BIOs. Its owners are the WebSocket proxy tunnel, the fetch proxy tunnel,node:tlsover a Duplex, and Windows named pipes.SSL_writefails on the side that sent it.CloseEvent.codeis the code of the received frame.Notes
Trace of the tunnel on main (
BUN_DEBUG_SSLWrapper=1 BUN_DEBUG_WebSocketProxyTunnel=1), frames and close_notify in one read:With this change:
How often. A
Bun.serveorigin that callsws.close(4001)through the plain CONNECT proxy of the test suite gives 1006 on 20 of 20 connections on the release build.The tests. Three cases use a
Bun.serveorigin that callsws.close(4001, "bye")from its message handler: direct,httpproxy,httpsproxy. They use the plain proxy of the suite, so the network decides if both TLS records arrive in one read. On the builds without the fix they did so on every run here (release and debug, Linux and Windows). Two more cases pin it. The client arms the proxy from itsopenhandler and sendsgo. A raw TLS origin answers withsocket.end(frames). The proxy holds the bytes until two complete TLS records are buffered (the frames, then the alert) and forwards them in one write. The assertion also checks the CONNECT request and that exactly one write with two records reached the client. 20 reruns of all five cases pass on the debug build.Not fixed here. These routes keep their current behavior. Each one needs its own change and test.
node:tlsover a Duplex or a Windows named pipe:socket.write()from thedatahandler for the last bytes fails withERR_SOCKET_CLOSED, and the peer never gets the bytes. Node v26.3.0 and Bun's file descriptor path deliver them. The write is refused insocket_body.rs, becauseSSLWrapper::is_shutdown()includesreceived_ssl_shutdown.us_internal_ssl_is_shut_downinopenssl.cchecks only the sent side. The follow-up is a write-side predicate for the wrapper. It needs the order from this PR first, becausewrite_datafailed onsent_ssl_shutdownbefore.close_dispatch_pending, andWebSocketProxyTunnel::on_closecallsfail(Ended)without a look at it. The client reports 1006Connection ended. The direct path honors it inhandle_close.WebSocketProxyTunnel::writemapsWantReadandWantWritetoConnectionClosed, so an echo during a TLS 1.2 renegotiation still gives 1006Failed to write(see also tls: fix renegotiation leaving the handshake state latched (lost drain, bogus ECONNRESET on close) #37094).handle_readingthat refuses a renegotiation closes without a flush of the bytes already decrypted in that read.ws.close()),clear_data()tears the tunnel down with a fast shutdown, which queues close_notify but does not flush it. The one-read flow is now the same. The follow-up is a gracefulshutdown(false)in the clean close of the tunnel, after the Close frame is flushed. The fast path itself cannot flush, because it is alsodestroy()fornode:tlsover a Duplex, where Node sends no close_notify (tls: close a destroyed TLS socket with a bare FIN, no close_notify #40412).Other owners of
SSLWrapper.node:tlsover a Duplex: thedataevent for the last bytes now fires before the close_notify reply is written to the Duplex. Before, it fired after. Node v26.3.0 emits the data first too. The new case innode-tls-connect.test.tspins this order with an in-memory Duplex pair.received_ssl_shutdownis still set before the data callback, sois_shutdown()is true there as before, andtunnel_poolablestill refuses to pool such a tunnel.shutdown(true)) now takes the normal fast shutdown path, not thesent_ssl_shutdownearly return. Both run the close callback, so the guard after the data callback still returns. A fast shutdown queues close_notify but does not flush it, as for every other fast shutdown. So in this case the tunnel no longer writes a close_notify reply. The WebSocket tunnel already behaves this way for a clientws.close().shutdown(true)stays reachable from a fatal read.response + corrupt TLS record in one packetinproxy.test.tscovers it.Suites run with the change. Linux debug build:
websocket-proxy.test.ts,websocket-proxy-close-reentrancy,websocket-proxy-tunnel-client-leak,websocket-proxy-tunnel-upgrade-leak,test-ws-bidir-proxy,first_party/ws/ws-proxy.test.ts,bun/http/proxy.test.ts(92 pass, includes the ASAN test from #31959),node-tls-connect,node-tls-upgrade,node-tls-duplex-close-throw-uaf,node-tls-duplex-write-throw-error-value,node-tls-socket-allow-half-open-option,renegotiation, and the Node parallel teststest-tls-js-stream,test-tls-inception,test-tls-destroy-stream,test-tls-socket-allow-half-open-option,test-tls-streamwrap-buffersize,test-http2-generic-streams. Windows x64 debug build:websocket-proxy.test.ts(four proxy cases fail before, all pass after) andnode-tls-namedpipes.test.ts(6 pass, same run time with and without the change).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/proxy.test.ts