fix: populate localAddress on the connect handler's Socket#6647
fix: populate localAddress on the connect handler's Socket#6647xortive wants to merge 1 commit intocloudflare:mainfrom
localAddress on the connect handler's Socket#6647Conversation
| const payload = | ||
| info.remoteAddress === EXPECTED_AUTHORITY | ||
| ? 'OK' | ||
| : `BAD:${info.remoteAddress}`; |
There was a problem hiding this comment.
I honestly think this should be localAddress here.
That fits socket semantics more imo.
@kentonv wdyt?
There was a problem hiding this comment.
Yeah, on the server side, "remoteAddress" should be the client's address, not the server's address.
There was a problem hiding this comment.
Remember the socket object here is not the same socket object that the client holds. It's a socket representing the other end of the connection. So local vs. remote are swapped.
There was a problem hiding this comment.
Switched to localAddress. Threaded a localAddress parameter through Socket / setupSocket and forward host into it on the ingress path; outbound continues to populate remoteAddress as before.
| // info.remoteAddress matches the authority string the caller supplied to | ||
| // fetcher.connect(...). Writes 'OK' on match, 'BAD:<actual>' on mismatch so | ||
| // the client's assertion failure points directly at the observed value. | ||
| export class RemoteAddressEndpoint extends WorkerEntrypoint { |
There was a problem hiding this comment.
Can we update the existing tests at src/workerd/api/tests/connect-handler-test.js for this instead of adding a new test that duplicates a lot of the code from it? I think that should be cleaner and easier to maintain in the long run
There was a problem hiding this comment.
Folded into the existing test. Added a localAddressViaServiceBinding case in connect-handler-test.js that goes through a service binding (TARGET) to a new LocalAddressEndpoint (added to connect-handler-test-proxy.js), with strict equality between the authority passed to fetcher.connect(...) and the localAddress observed on the server side. The TCP listener path can't give that strict-equality guarantee since workerd's TcpListener forwards the listener's bound address (e.g. *:8081) as the CONNECT authority rather than the client's connect() string, so the service-binding path is what actually exercises the fix end-to-end. Removed the separate connect-handler-local-address-test files.
ca473a0 to
a182141
Compare
remoteAddress on the connect handler's SocketlocalAddress on the connect handler's Socket
|
LGTM |
a182141 to
4a3822b
Compare
|
last push was to cleanup the tests as @fhanau suggested |
fhanau
left a comment
There was a problem hiding this comment.
Thank you for updating the test, LGTM
Merging this PR will improve performance by 38.55%
Performance Changes
Comparing Footnotes
|
ServiceWorkerGlobalScope::connect was dropping the CONNECT authority when constructing the ingress Socket, causing both socket.opened.remoteAddress and socket.opened.localAddress to resolve to undefined on the connect handler side. Plumb a localAddress parameter through Socket/setupSocket and forward the host parameter into it so JS callers see the exact authority string that was passed to fetcher.connect(...). From the handler's perspective the CONNECT authority is the local address on this side of the tunnel (the address the peer asked to connect to), not the remote address, so populate localAddress rather than remoteAddress. Outbound sockets continue to populate remoteAddress as before; localAddress remains empty for outbound since we have no useful value to expose. Extend the existing connect-handler-test with a localAddressViaServiceBinding case that asserts strict equality between the authority passed to fetcher.connect(...) and the localAddress observed on the server side.
4a3822b to
37a605c
Compare
|
Merged in #6688. |
Summary
connect()handler,socket.opened.localAddress(and previouslyremoteAddress) currently always resolves toundefinedbecauseServiceWorkerGlobalScope::connectdiscards the CONNECT authority when constructing the ingressSocket.localAddressparameter throughSocket/setupSocketand forward thehostparameter into it. JS callers now observe the exact authority string that was passed tofetcher.connect("host:port"), on the correct field.connect-handler-testwith alocalAddressViaServiceBindingcase that asserts strict equality between the authority passed tofetcher.connect(...)and thelocalAddressobserved on the server side.Semantics: why
localAddress, notremoteAddressOn the ingress (handler) side of a CONNECT tunnel, the authority the caller targeted (e.g.
"example.com:1234") represents the address on this side of the socket — the endpoint the peer asked to connect to. That's the local address from the handler's perspective, not the remote address. The outboundconnect()side continues to populateremoteAddressas before;localAddressremains empty on outbound since there is no useful value to expose.SocketInfo.localAddresspreviously carried a comment saying it "will always remain empty" — updated to reflect the new behavior.Changes
src/workerd/api/sockets.h:localAddressto theSocketconstructor parameter list and as a member.localAddressparameter to thesetupSocketfree-function declaration.SocketInfo.localAddresscomment.src/workerd/api/sockets.c++:localAddressthroughsetupSocket,Socket::startTls, and bothSocket::handleProxyStatusoverloads.src/workerd/api/global-scope.c++:kj::mv(host)into the newlocalAddressargument ofsetupSocket.src/workerd/api/hyperdrive.c++:kj::none /* localAddress */(outbound, unchanged behavior).TARGETservice binding to the existingconnect-handler-testworker pointing at a newLocalAddressEndpointdefined alongside the existingConnectProxy/ConnectEndpointinconnect-handler-test-proxy.js.localAddressViaServiceBindingcase toconnect-handler-test.jsthat callsenv.TARGET.connect("example.com:1234")and asserts the server reports back exactly that string asinfo.localAddress.The TCP listener path in workerd's
TcpListenerforwards the listener's bound address (e.g.*:8081) as the CONNECT authority rather than the client'sconnect()string, so the existing TCP-listener tests cannot give a strict-equality guarantee against the caller-supplied authority. The service-binding path goes throughWorkerEntrypoint::connectand forwards the authority verbatim, which is what end-to-end exercises this fix.Verification
Red-green
global-scope.c++forwarding reverted,localAddressViaServiceBindingfails withActual: "OK:undefined"— confirming the test catches the bug. The other two existing test cases (connectHandler,connectHandlerProxy) keep passing since they don't depend onlocalAddress.Broader suite
bazel test //src/workerd/api/... --nocache_test_results: 906/912 pass. The 6 failures (worker-loader-test:pythonBasicsanddns-nodejs-test) are pre-existing on cleanmain(verified by stashing the change and rerunning before any code modifications) — they fail due to environmental TLS cert chain / DNS issues on the developer machine and are unrelated to this change.Non-goals
domain/ SNI parameter — incoming CONNECT with TLS is still rejected upstream inworker-entrypoint.c++.SocketInfo's public interface: bothremoteAddressandlocalAddressfields already exist on the JS surface;localAddresswas simply never populated before.connect()behavior is preserved:remoteAddresscontinues to carry the target address;localAddressremains empty on outbound.