fix(relay): carry binary tunnel WS frames as base64#4066
Conversation
The host-service tunnel client UTF-8-decoded binary WS frames from its local loopback socket via String(buffer), corrupting PTY output bytes (9016225 made PTY output binary end-to-end). The relay then forwarded the mangled string as a text frame, and the renderer's JSON.parse fell through to "[terminal] invalid server payload". Add an optional encoding: "base64" discriminator to TunnelWsFrame. tunnel-client base64-encodes ArrayBuffer frames; relay decodes and emits a real binary WS frame to the browser. Renderer's existing ArrayBuffer fast path handles it unchanged. Deploy relay first, then desktop canary picks up the host-service tunnel-client change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request adds binary WebSocket frame support by extending the tunnel protocol with an optional ChangesWebSocket Binary Frame Support
Sequence DiagramsequenceDiagram
participant LocalWs as Local WebSocket
participant HostService as Host Service (tunnel-client)
participant Tunnel as Tunnel (protocol)
participant Relay as Relay (relay.ts)
participant RemoteWs as Remote WebSocket
LocalWs->>HostService: send(ArrayBuffer)
HostService->>HostService: detect binary, base64-encode
HostService->>Tunnel: ws:frame { data: base64, encoding: "base64" }
Tunnel->>Relay: forward ws:frame
Relay->>Relay: sees encoding="base64", decode to bytes
Relay->>RemoteWs: send(ArrayBuffer)
RemoteWs-->>LocalWs: binary delivered
LocalWs->>HostService: send(string)
HostService->>Tunnel: ws:frame { data: string }
Tunnel->>Relay: forward ws:frame
Relay->>RemoteWs: send(string)
RemoteWs-->>LocalWs: text delivered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes corrupted PTY output over cross-host relay tunnels by base64-encoding binary WebSocket frames from the host-service tunnel client, decoding them back to a binary buffer in the relay, and delivering a real binary WS frame to the browser renderer's existing
Confidence Score: 3/5Safe to merge after adding a typeof guard before the Buffer.from decode in the relay; the core binary-forwarding logic is correct. One P1 finding: the relay's Buffer.from(msg.data as string, 'base64') call has no type guard and can throw uncaught, potentially disconnecting the host. One P2 finding: silent drop of unrecognised data types in the tunnel-client onmessage handler. The P1 caps confidence at 4, and the fact that it sits on the relay's critical message-dispatch path (no try/catch) pulls it to 3. apps/relay/src/tunnel.ts — the new Buffer.from decode path needs a typeof msg.data !== 'string' guard before use.
|
| Filename | Overview |
|---|---|
| packages/shared/src/tunnel-protocol.ts | Adds optional encoding?: "base64" discriminator to the shared TunnelWsFrame interface; clean, minimal change that correctly extends both the TunnelRequest and TunnelResponse union types. |
| packages/host-service/src/tunnel/tunnel-client.ts | Sets binaryType = "arraybuffer" and base64-encodes binary frames before forwarding to relay; the fix is correct but the onmessage handler silently drops data that is neither string nor ArrayBuffer. |
| apps/relay/src/tunnel.ts | Decodes base64-encoded frames from the host and forwards them as real binary WS frames; Buffer.from(msg.data as string, "base64") lacks a type guard and can throw uncaught if msg.data is not a string. |
Sequence Diagram
sequenceDiagram
participant Terminal as Host Terminal (PTY)
participant LocalWS as Local Loopback WS
participant TunnelClient as TunnelClient (host-service)
participant Relay as Relay (apps/relay)
participant Browser as Renderer (browser)
Note over Terminal,Browser: Host to Browser (PTY output - binary path, this PR)
Terminal->>LocalWS: send Uint8Array (binary WS frame)
LocalWS->>TunnelClient: onmessage(ArrayBuffer) binaryType=arraybuffer
TunnelClient->>Relay: JSON ws:frame encoding:base64
Relay->>Browser: clientWs.send(Buffer) binary WS frame
Browser->>Browser: event.data instanceof ArrayBuffer PTY output renders
Note over Browser,Terminal: Browser to Host (keystrokes - text path, unchanged)
Browser->>Relay: JSON ws:frame data:keystroke
Relay->>TunnelClient: sendWsFrame data string
TunnelClient->>LocalWS: localWs.send(message.data)
LocalWS->>Terminal: keystroke delivered
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/src/tunnel/tunnel-client.ts:196-210
**Silent drop of unrecognised data types**
If `event.data` is neither a `string` nor an `ArrayBuffer`, the frame is silently discarded with no log entry. With `binaryType = "arraybuffer"` this case is currently unreachable in practice, but if the runtime or environment ever returns a `Buffer`/`Blob`, the PTY output will disappear without a trace. An `else` log helps diagnose future regressions.
### Issue 2 of 2
apps/relay/src/tunnel.ts:178-179
**Missing type guard before decode**
`msg.data` is typed as `unknown` at runtime. The `as string` cast is unchecked, so a malformed frame where `encoding` is `"base64"` but `data` is `null` or a number causes `Buffer.from` to throw a `TypeError`. Since `handleMessage` has no surrounding try/catch, the exception propagates uncaught and can disconnect the host's relay connection. A `typeof` guard before the call prevents this.
Reviews (1): Last reviewed commit: "fix(relay): carry binary tunnel WS frame..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| localWs.onmessage = (event) => { | ||
| this.send({ type: "ws:frame", id: request.id, data: String(event.data) }); | ||
| const data = event.data; | ||
| if (typeof data === "string") { | ||
| this.send({ type: "ws:frame", id: request.id, data }); | ||
| return; | ||
| } | ||
| if (data instanceof ArrayBuffer) { | ||
| this.send({ | ||
| type: "ws:frame", | ||
| id: request.id, | ||
| data: Buffer.from(data).toString("base64"), | ||
| encoding: "base64", | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
If event.data is neither a string nor an ArrayBuffer, the frame is silently discarded with no log entry. With binaryType = "arraybuffer" this case is currently unreachable in practice, but if the runtime or environment ever returns a Buffer/Blob, the PTY output will disappear without a trace. An else log helps diagnose future regressions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/tunnel/tunnel-client.ts
Line: 196-210
Comment:
**Silent drop of unrecognised data types**
If `event.data` is neither a `string` nor an `ArrayBuffer`, the frame is silently discarded with no log entry. With `binaryType = "arraybuffer"` this case is currently unreachable in practice, but if the runtime or environment ever returns a `Buffer`/`Blob`, the PTY output will disappear without a trace. An `else` log helps diagnose future regressions.
How can I resolve this? If you propose a fix, please make it concise.| if (msg.encoding === "base64") { | ||
| clientWs.send(Buffer.from(msg.data as string, "base64")); |
There was a problem hiding this comment.
msg.data is typed as unknown at runtime. The as string cast is unchecked, so a malformed frame where encoding is "base64" but data is null or a number causes Buffer.from to throw a TypeError. Since handleMessage has no surrounding try/catch, the exception propagates uncaught and can disconnect the host's relay connection. A typeof guard before the call prevents this.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/relay/src/tunnel.ts
Line: 178-179
Comment:
**Missing type guard before decode**
`msg.data` is typed as `unknown` at runtime. The `as string` cast is unchecked, so a malformed frame where `encoding` is `"base64"` but `data` is `null` or a number causes `Buffer.from` to throw a `TypeError`. Since `handleMessage` has no surrounding try/catch, the exception propagates uncaught and can disconnect the host's relay connection. A `typeof` guard before the call prevents this.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/relay/src/tunnel.ts`:
- Around line 177-183: The code assumes msg.data is a string before
decoding/sending which can throw on malformed JSON; before using clientWs.send
or Buffer.from check that typeof msg.data === "string" (and for "base64"
encoding wrap Buffer.from(...) in a try/catch to handle invalid base64), log or
drop the frame if the type/decoding fails, and keep the existing
clientWs?.readyState === 1 guard; reference clientWs, msg.encoding and msg.data
in tunnel.ts to find where to add the runtime guard and error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa1eb99e-056b-4f57-aa52-6bbb168e47a8
📒 Files selected for processing (3)
apps/relay/src/tunnel.tspackages/host-service/src/tunnel/tunnel-client.tspackages/shared/src/tunnel-protocol.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
handleMessage runs uncaught; a malformed frame with non-string data would throw at Buffer.from and could tear down the host's tunnel connection. Drop frames whose data isn't a string.
Three fixes since v0.2.6: - relay tunnel: carry binary WS frames as base64 so PTY output renders through cross-host workspaces (no more `[terminal] invalid server payload`) (#4066) - host-service: read remote URLs from git config instead of `git remote -v` (#4065) - cli: auto-refresh OAuth access token using the refresh token (#4069) Push cli-v0.2.7 after this lands to fire the release pipeline.
1 participant
Summary
[terminal] invalid server payloadand never render PTY output. Root cause is a version skew with901622573(PTY output bytes end-to-end): the host-service tunnel client atpackages/host-service/src/tunnel/tunnel-client.ts:196UTF-8-decoded binary WS frames from its local loopback socket viaString(buffer), the relay forwarded the mangled string as a text frame, and the renderer's JSON.parse fell through to the error label.encoding: "base64"discriminator toTunnelWsFrame. tunnel-client now setsbinaryType = "arraybuffer"and base64-encodes binary frames; relay decodes andclientWs.send(buffer)emits a real binary WS frame. The renderer's existingevent.data instanceof ArrayBufferfast path picks it up unchanged.Trace of the loss (one PTY byte)
packages/host-service/src/terminal/terminal.ts:410,822— host-service sendsUint8Array. ✓packages/host-service/src/tunnel/tunnel-client.ts:193-196— loopback WS receives a Buffer;String(event.data)mangles it. ← originpackages/host-service/src/tunnel/tunnel-client.ts:110— mangled string JSON-stringified to relay.apps/relay/src/tunnel.ts:175-177— relayclientWs.send(string)— text frame, no binary path.apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:239-249— JSON.parse fails →[terminal] invalid server payload.Rollout
Relay and desktop ship together — no backwards compat needed.
cd apps/relay && fly deploy -a superset-relay(no CI auto-deploy for the relay).Test plan
cd apps/relay && bun run dev) with two desktops pointed at it viaRELAY_URL=http://localhost:8080.[terminal] invalid server payloadis gone and PTY output renders.vim,htop,ls --color=always,printf '\xff\xfe\x00\x01'.Summary by cubic
Fixes corrupted PTY output by sending binary tunnel WebSocket frames as base64 through the relay, restoring cross-host terminal rendering. Also guards against malformed frames to avoid relay crashes; keystrokes remain unchanged.
Bug Fixes
encoding: "base64"toTunnelWsFrameinpackages/shared.packages/host-servicetunnel client: setbinaryType = "arraybuffer", base64-encode binary frames, and includeencoding: "base64".apps/relay: decode base64 frames andsendas real binary; text frames unchanged.apps/relay: dropws:framemessages with non-stringdatabefore decode to prevent tunnel teardown on malformed frames.Migration
apps/relayfirst; desktop canary picks up the tunnel client change. No back-compat required.Written for commit ad1445a. Summary will update on new commits.
Summary by CodeRabbit