Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a WebSocket-to-TCP bridge in the DAP subsystem so remote debugging can connect via ws:///wss:// to the runner’s DAP server (e.g., through Dev Tunnels using GitHub auth), while the internal DAP server continues to speak DAP-over-TCP.
Changes:
- Add
WebSocketDapBridgeto translate WebSocket text frames ↔ DAP TCP (Content-Length framed) messages. - Update
DapDebuggerstartup/shutdown to expose the tunnel port via the WebSocket bridge and move the internal DAP listener to an ephemeral local port. - Add L0 coverage for the bridge and for DapDebugger WebSocket connectivity (including “pre-upgraded” WebSocket streams).
Show a summary per file
| File | Description |
|---|---|
| src/Runner.Worker/Dap/WebSocketDapBridge.cs | New bridge implementation handling upgrade detection, handshake, and bidirectional message pumping. |
| src/Runner.Worker/Dap/DapDebugger.cs | Starts/stops the bridge and uses an internal ephemeral TCP port for the actual DAP server when the bridge is enabled. |
| src/Test/L0/Worker/WebSocketDapBridgeL0.cs | New L0 tests validating forwarding, rejection paths, message size limits, and disposal behavior. |
| src/Test/L0/Worker/DapDebuggerL0.cs | Extends L0 tests to validate DapDebugger can accept DAP initialize via WebSocket and pre-upgraded WebSocket streams. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 7
| return null; | ||
| } | ||
|
|
||
| var webSocketKey = headers["Sec-WebSocket-Key"]; |
There was a problem hiding this comment.
Sec-WebSocket-Key like this is just for now right, cause at the moment its' untrusted right?
There was a problem hiding this comment.
We're actually authed at the devtunnel layer, this is just meant to be protocol-compliant, that's part of the initial handshake.
There was a problem hiding this comment.
yeah that makes sense, tunnel layer handling auth is good. just thinking since we're going for protocol compliance anyway, rfc 6455 expects the key to be a base64 encoded 16 byte nonce so a quick format check might be nice to have. would also guard against any weird edge cases with cr/lf in the value messing with the response framing
| if (messageStream.Length + result.Count > MaxInboundMessageSize) | ||
| { | ||
| Trace.Warning($"WebSocket message exceeds maximum allowed size of {MaxInboundMessageSize} bytes, closing connection"); | ||
| await source.CloseAsync( |
There was a problem hiding this comment.
Could we cancel sessionCts before initiating the close, or signal the session teardown path in HandleClientAsync to handle the graceful close instead?
| } | ||
|
|
||
| var prefixKind = ClassifyIncomingStreamPrefix(initialBytes); | ||
| if (prefixKind == IncomingStreamPrefixKind.PreUpgradedWebSocket) |
There was a problem hiding this comment.
this probs for dev tunnels right? jsut thinking something could easily skip e.g. by sending like 0x81 0x85 ..., maybe something like a flag allowPreUpgraded: true to Configure(), and the
bridge only accepts that byte-sniff path when the flag, that still has a limitation though if the relay is active but a bit more secure at least. I think tho if doing secrets or diff ports in the future it would address this anyway
There was a problem hiding this comment.
I got this all in place while I was failing to figure out connectivity going through dev tunnels yeah 😄 I think this is also covered by auth at dev tunnel layer?
|
|
||
| try | ||
| { | ||
| await completedTask; |
There was a problem hiding this comment.
can we skip the re-await of completedTask and go straight to Task.WhenAll after sessionCts.Cancel(), currently if completedTask faults with something other than OperationCanceledException (e.g., IOException from a peer disconnect), this rethrows and we never reach the Task.WhenAll below?
| } | ||
| } | ||
|
|
||
| if (contentLength < 0) |
There was a problem hiding this comment.
in DAP isn't there a big error if the content-length is wrong, so should we not clear the entire buffer so the next message starts clean, or (probably better) treat this as a fatal protocol error and tear down the session?
| var bridgePort = GetFreePort(); | ||
|
|
||
| var bridge = new WebSocketDapBridge(); | ||
| bridge.Initialize(hc); |
There was a problem hiding this comment.
should we try catch the configure start and start?
| var headers = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
| while (true) | ||
| { | ||
| var line = await ReadLineAsync(handshakeStream, cancellationToken); |
There was a problem hiding this comment.
individual header lines are capped at 8 KB but theres no cap on the count, so someone could just spam thousands of small headers within the handshake timeout. maybe a maxHeaderCount (64ish?) to be safe?
| { | ||
| Trace.Info("Stopping WebSocket DAP bridge"); | ||
| var shutdownTask = _webSocketBridge.ShutdownAsync(); | ||
| if (await Task.WhenAny(shutdownTask, Task.Delay(5_000)) != shutdownTask) |
There was a problem hiding this comment.
small thing, if the 5s timeout fires shutdownTask is still running but we drop the reference and null the bridge. if it eventually faults that goes unobserved. might be nice to have a ContinueWith to log if it fails or something, just so we dont silently lose errors
| } | ||
| } | ||
|
|
||
| private static bool TryParseDapMessage(List<byte> buffer, out byte[] messageBody) |
There was a problem hiding this comment.
nit: this gets allocated on every call to TryParseDapMessage, could just be a private static readonly byte[] field
This adds a bridge converting messages to/from TCP <-> WS so that we can connect to the DAP server through
wss://directly and using GitHub credentials out of the box with Dev Tunnels.https://github.com/github/c2c-actions/issues/9831