Skip to content

fix: populate localAddress on connect handler Socket#6688

Merged
fhanau merged 1 commit intomainfrom
fix/connect-handler-remote-address
Apr 30, 2026
Merged

fix: populate localAddress on connect handler Socket#6688
fhanau merged 1 commit intomainfrom
fix/connect-handler-remote-address

Conversation

@fhanau
Copy link
Copy Markdown
Contributor

@fhanau fhanau commented Apr 28, 2026

Clone of #6647 (already approved) – see that PR for context.

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.

@fhanau fhanau requested review from a team as code owners April 28, 2026 20:05
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 28, 2026

I've now thoroughly reviewed all the changed files. Let me assess:

Summary of changes:

  1. Added a localAddress parameter to Socket constructor, setupSocket, threaded through startTls and both handleProxyStatus overloads.
  2. In ServiceWorkerGlobalScope::connect, forwards the host (CONNECT authority) as localAddress to the setupSocket call.
  3. Outbound paths (connectImplNoOutputLock, Hyperdrive::connect) pass kj::none for localAddress — no behavior change.
  4. Test coverage via service-binding round-trip test asserting strict equality.
  5. Tail worker test adjustments for the new test case.

Review findings:

  1. Correctness: The plumbing is consistent — every call site of setupSocket passes localAddress, every handleProxyStatus and startTls path propagates it. The kj::mv(host) in global-scope.c++ is safe since host is a kj::String parameter (owned).

  2. Backward compatibility: The localAddress field previously always resolved to undefined in JS. Now it resolves to a string for inbound connect-handler sockets. The connect handler itself requires the experimental compat flag, so the blast radius is minimal. No compat flag needed.

  3. Memory/GC safety: localAddress is kj::Maybe<kj::String> (C++ type, not JS heap), so it correctly doesn't need visitForGc. It also isn't in visitForMemoryInfo, but neither is remoteAddress — pre-existing oversight.

  4. API design: Using localAddress rather than remoteAddress for the inbound handler is semantically correct — from the handler's perspective, the CONNECT authority is the address the peer asked to connect to on this end.

  5. Test quality: The test exercises the service-binding path specifically (which forwards the authority verbatim) and asserts strict equality. Good red-green verification in the PR description.

I don't see any actionable issues.

LGTM

github run

@fhanau fhanau force-pushed the fix/connect-handler-remote-address branch from 37a605c to b1fc711 Compare April 28, 2026 20:43
@fhanau fhanau requested a review from dom96 April 29, 2026 20:08
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.
@fhanau fhanau force-pushed the fix/connect-handler-remote-address branch from b1fc711 to 258bbb9 Compare April 30, 2026 14:26
@fhanau fhanau merged commit e5e9cf4 into main Apr 30, 2026
32 of 34 checks passed
@fhanau fhanau deleted the fix/connect-handler-remote-address branch April 30, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants