Skip to content

feat: browser-based remote control for v2 desktop terminals - #4345

Merged
AviPeltz merged 16 commits into
mainfrom
remote-terminal-implement
May 11, 2026
Merged

AviPeltz merged 16 commits into
mainfrom
remote-terminal-implement

Conversation

@AviPeltz

@AviPeltz AviPeltz commented May 10, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Adds an end-to-end remote control feature for v2 desktop terminal panes. A user clicks Share on a terminal, gets an auto-copied URL, and anyone opening the link in a browser sees the live terminal output and can type into it.

  • Desktop: new Share button on v2 terminal panes (radio icon → pulsing "live" badge dropdown with Copy/Stop)
  • Cloud tRPC: remoteControl.{create,get,revoke,listForWorkspace,expireStale} — mints a short user JWT for the relay POST and only inserts the cloud row after the host returns a token. Stores SHA-256 hash; the raw HMAC is never persisted.
  • Host-service: HMAC token mint/verify + in-memory session registry + expiry sweep, /remote-control/:sessionId WebSocket route, attachTerminalViewer fan-out on TerminalSession (256 KB tail ring, output sequence, viewer set)
  • Relay: /hosts/:hostId/remote-control/* skips the user-JWT gate; the per-session HMAC validated by the host is the credential. Existing /trpc/* and other host paths still require user JWT.
  • Web viewer: /agents/remote-control/[sessionId] page + RemoteTerminal client component using xterm.js + addon-fit, mirroring the desktop's default Ember "dark" theme (palette, font stack, font size, scrollback) so the browser feels native.
  • DB: new v2_remote_control_sessions table + remote_control_session_{mode,status} enums, with a unique index on token_hash and cascade-deletes from org/host/workspace.
  • Shared: @superset/shared/remote-control-protocol pinning the wire format (frames, capabilities, limits).

Two modes: command (read + structured commands only) and full (read + raw input + resize + commands). Capabilities are enforced per message; per-viewer rate limits (200 input/s, 10 resize/s) and a 4-viewer-per-session cap. Default TTL 4h, max 24h. Either side can revoke; revoke fans out to all viewers and closes their sockets. The PTY never leaves the host.

Out of scope (tracked as future work):

  • Viewer-count fan-out on attach/detach (only the connecting viewer sees the count)
  • Host→cloud heartbeat for last_connected_at / viewer_count columns
  • Host-driven resize push to viewers (viewer's size wins in full mode)
  • Rebuilding the in-memory session registry across host-service restarts (viewers see session-not-found and the desktop user re-shares)

Test plan

  • Run the migration on a fresh Neon branch (bun run --cwd packages/db drizzle-kit migrate)
  • bun dev from root + bun run --cwd apps/relay dev (relay isn't in the default bun dev filter)
  • Toggle Settings → Security → "Allow remote workspaces to access this device via relay" so the host-service receives RELAY_URL
  • Open a v2 workspace, open a terminal pane, run something with live output (top, for i in {1..1000}; do echo $i; sleep 1; done)
  • Click the new radio icon → toast "Remote control link copied" → live badge replaces the icon
  • Paste the link in another browser → terminal output streams within ~1s
  • Type q (or arbitrary input) in the browser → desktop terminal reflects it
  • Click Stop on either side → web tab shows "Revoked" overlay within ~1s; cloud row goes to status='revoked'
  • Inspect v2_remote_control_sessions: token_hash is a SHA-256 hex, never the raw token
  • bun run typecheck and bun run lint are green; new session-manager unit tests pass (bun test packages/host-service/src/terminal/remote-control)

Summary by cubic

Adds browser-based remote control for v2 desktop terminals. The Share button is gated by WEB_REMOTE_CONTROL_ACCESS and adds Open in browser; anonymous viewers now work in production via a public page and CSP updates.

  • Bug Fixes

    • Viewer auth and controls: remoteControl.get is a public mutation (token in POST body) with constant‑time token‑hash compare; returns a generic 401 for missing/invalid tokens and compares against a dummy hash when absent to prevent probing. Refuses revoked/expired rows and promotes just‑expired to expired. Anonymous viewers can stop sessions via remoteControl.revokeWithToken({ sessionId, token }).
    • Access checks: protected revoke and listForWorkspace require host membership; cloud revoke stays org‑scoped and idempotent.
    • Input and stability: added raw pty.writeBytes to avoid non‑ASCII corruption; unified WS cleanup; zod‑validates client messages; caches auth at handshake to cut per‑message cost; debounces viewer resize broadcasts with a 200ms trailing delay to stay under the resize rate limit and avoid the “rate‑limited” banner.
    • UX: desktop Share button hydrates every 30s, disables Copy when the link isn’t recoverable, and shows a one‑time hint in command mode; Share is gated by WEB_REMOTE_CONTROL_ACCESS and adds Open in browser; the viewer page moved to (public) and is allowed in the web proxy for anonymous access.
    • Robustness and relay: tail buffer keeps the tail of oversized PTY chunks; host clamps TTL; relay redacts remoteControlToken (and token) and still runs tunnel‑presence checks with Fly replay; prod CSP allows the relay wss:// origin; host‑revoke failures now surface; mintToken calls time out after 5s; on cloud insert failure we revoke host sessions and wrap DB errors with TRPCError; adds structured logs for orphan cleanup.
    • CI: Sherif ignores @xterm/addon-fit and @xterm/xterm to allow the intentional desktop (6.x beta) vs web (5.x stable) version split.
  • Migration

    • Run DB migration 0050_add_v2_remote_control_sessions.
    • Set HOST_SERVICE_SECRET so the host-service can mint/verify tokens.
    • For local dev, run the relay and confirm the remote-control path is reachable.

Written for commit a95b19e. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Added terminal remote control sharing capability, allowing users to create secure sessions and share access to terminal control via a unique link
    • New web-based terminal viewer with full input control, resize support, and real-time output synchronization
    • Session management features including creation, revocation, and viewer tracking with automatic expiration
  • Dependencies

    • Added xterm.js terminal emulator library

Review Change Stack

AviPeltz added 2 commits May 9, 2026 21:44
Adds an end-to-end remote control flow: a desktop user clicks Share on a
v2 terminal pane, gets a one-time URL, and anyone opening the link in a
browser sees the live terminal output and can type into it.

Pieces:
- v2_remote_control_sessions table + remote_control_session_{mode,status}
  enums, with a unique index on token_hash. Cloud row stores the SHA-256
  hash only; the host-service mints and verifies the HMAC-signed token
  end-to-end so a leaked DB row cannot grant new sessions.
- Shared protocol module (packages/shared/remote-control-protocol)
  pinning the wire format, capabilities, and limits.
- Host-service: session manager (token mint/verify, registry, expiry
  sweep), /remote-control/:sessionId WS route, attachTerminalViewer fan-
  out on TerminalSession (256 KB tail ring, output sequence, viewer
  set), and terminal.remoteControl.{mintToken,revoke,listActive} tRPC.
- Cloud tRPC: remoteControl.{create,get,revoke,listForWorkspace,
  expireStale}; mints a short user JWT for the relay POST and inserts
  the row only after the host returns a token.
- Relay: lets /hosts/:hostId/remote-control/* skip the user-JWT gate;
  the per-session HMAC validated by the host is the credential.
- Desktop UI: TerminalRemoteControlButton on v2 terminal panes (radio
  icon -> live badge dropdown with copy/stop) wired through pane
  registry. Host-service spawn now propagates HOST_SERVICE_SECRET into
  the child env so the secret derivation is stable.
- Web viewer: /agents/remote-control/[sessionId] page + RemoteTerminal
  client component (xterm.js + addon-fit, mobile-only key toolbar,
  status badge, copy/stop controls).

Out of scope: viewer-count fan-out, host->cloud heartbeat for
last_connected_at, host-driven resize push, and rebuilding the in-
memory session registry across host-service restarts (viewers see
session-not-found and the desktop user re-shares).
Pulls the xterm options and color palette from the desktop's default
"dark" (Ember) theme so the browser viewer renders identical font, font
size, scrollback, ANSI colors, cursor, and selection. The chrome (header,
buttons, banners, mobile toolbar) now uses the same warm-dark surface
tones (#151110/#1a1716/#2a2827) and matches the ANSI accent colors for
status badges and the destructive Stop button.

`vtExtensions` (kittyKeyboard) and `scrollbar` are intentionally omitted
because they only exist on the desktop's xterm beta build; the stable
web release does not type them.
@coderabbitai

coderabbitai Bot commented May 10, 2026 •

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Implements end-to-end terminal remote control: shared protocol constants and message types, PostgreSQL schema migration for sessions, host-service in-memory session/token manager with HMAC signing and expiry sweep, WebSocket viewer route with rate limiting, cloud and host tRPC routers, relay API gateway routing changes, desktop and web UI components for session lifecycle management.

Changes

Terminal Remote Control Feature

Layer / File(s) Summary
Protocol & Shared Types
packages/shared/src/remote-control-protocol.ts, packages/shared/package.json
Defines remote-control protocol: constants (tail bytes, TTL bounds, rate limits, max viewers), mode/status literals, capability mapping, client/server message interfaces, error codes, revoke reasons, and token claim shape.
Database Schema & Enums
packages/db/src/schema/enums.ts, packages/db/src/schema/schema.ts, packages/db/drizzle/0050_add_v2_remote_control_sessions.sql, packages/db/drizzle/meta/_journal.json
Adds remote-control-session enums and PostgreSQL table with session identity, token hash, expiration/revocation tracking, viewer count, foreign keys (organization, host, workspace, terminal, users), and indexes; migration journal updated.
Feature Flag
packages/shared/src/constants.ts
Adds WEB_REMOTE_CONTROL_ACCESS feature flag to gate desktop terminal share-control session creation.
Host Service Configuration
packages/host-service/src/app.ts, packages/host-service/src/serve.ts, apps/desktop/src/main/host-service/index.ts
Wires hostServiceSecret into app config; conditionally initializes remote-control secret, starts expiry sweep, registers WebSocket route, and adds shutdown cleanup; desktop entry point passes secret from environment.
Session & Token Management
packages/host-service/src/terminal/remote-control/session-manager.ts
Implements in-memory session registry: derives SHA-256 secret from base secret, mints/verifies HMAC-SHA256 tokens with claim structure, registers sessions with token-hash mismatch revocation, authenticates via constant-time token comparison, tracks viewers with capacity enforcement, notifies revocation listeners, revokes by session/terminal/all, and runs periodic expiry sweep.
Session Manager Tests
packages/host-service/src/terminal/remote-control/session-manager.test.ts
Unit tests covering token mint/verify round-trip, tampered signature rejection, expiry handling, secret rotation invalidation, sessionId mismatch, viewer cap enforcement, revoke listener callbacks, terminal-scoped sweeps, and expiry-sweep behavior.
Terminal Viewer Abstraction
packages/host-service/src/terminal/terminal.ts
Exports TerminalViewerListener/TerminalViewerSnapshot/TerminalViewerHandle interfaces; adds bounded tail-ring output buffering (capped by REMOTE_CONTROL_TAIL_BYTES), output sequencing, and per-viewer callbacks (onData/onTitle/onResize/onExit); implements attachTerminalViewer() returning a handle with detach/sendInput/resize/runCommand/getSnapshot; updates PTY output and exit handling to notify viewers; disposes remote-control sessions on terminal shutdown.
WebSocket Route & Viewer Connection
packages/host-service/src/terminal/remote-control/route.ts
Registers GET /remote-control/:sessionId WebSocket: per-connection session authentication via token, attaches terminal viewer with listener-based data forwarding, sends hello/snapshot/exit/revoked messages, validates inbound messages with Zod discriminated union, enforces per-mode capabilities and token-bucket rate limits (input/runCommand shared; resize separate), and performs idempotent cleanup on close/error.
Host Service tRPC Endpoints
packages/host-service/src/trpc/router/terminal/remote-control.ts, packages/host-service/src/trpc/router/terminal/terminal.ts
Host-side tRPC router: mintToken (checks terminal exists, mints and registers session), revoke (revokeSession with reason), listActive (list sessions); terminal router wires remoteControl sub-router.
Relay API Gateway Routing
apps/relay/src/index.ts
Conditionally routes /hosts/:hostId/remote-control/* to skip user JWT and host-access middleware (validates only hostId), while preserving tunnel-presence/maybeReplay behavior; redacts remoteControlToken and token query parameters in request logs.
Cloud Backend tRPC Router
packages/trpc/src/router/remote-control/remote-control.ts, packages/trpc/src/router/remote-control/index.ts, packages/trpc/src/root.ts
Cloud remoteControl router: create (protected, validates workspace/host membership, calls relay to mint host token, persists active session row with hashed token), get (public, SHA-256 verifies token with constant-time compare, gates wsUrl behind effective-active status), revoke (protected, marks revoked and notifies host), revokeWithToken (public, token-only revoke), listForWorkspace (protected, returns 50 newest sessions), expireStale (protected, marks elapsed sessions as expired), statuses (protected, returns enum values); root router and re-exports updated.
Web Viewer & Page Components
apps/web/package.json, apps/web/src/app/(public)/agents/remote-control/[sessionId]/..., apps/web/next.config.ts, apps/web/src/proxy.ts
Adds xterm dependencies; RemoteTerminal component (xterm + FitAddon, authenticates via session.get, opens token-authenticated WebSocket, handles server messages, sends periodic pings, forwards input base64-encoded in full mode with view-only hint otherwise, forwards resizes only in full mode, manages MobileToolbar for control sequences); RemoteTerminalLoader (reads token from location.hash, updates on hashchange, renders placeholder/unavailable/terminal states); page unwraps sessionId via React.use(); CSP extended with relay WebSocket origin; proxy routes bypass auth for /agents/remote-control/; barrel exports for page structure.
Desktop Terminal Pane UI
apps/desktop/src/renderer/routes/.../TerminalRemoteControlButton/..., apps/desktop/src/renderer/routes/.../TerminalHeaderExtras/..., apps/desktop/src/renderer/routes/.../usePaneRegistry/...
TerminalRemoteControlButton component with feature flag gating, background hydration (listForWorkspace with abort), state phases (loading/inactive/creating/active/revoking), start/stop/copy-link flows, loading spinner, tooltip, and active dropdown with live badge and menu actions (open, copy, stop); TerminalHeaderExtras accepts workspaceId prop and renders button before logs button; pane registry wires workspaceId; re-export for button module.

🎯 4 (Complex) | ⏱️ ~60 minutes

🐰 A protocol takes flight,
Sessions mint and viewers wake,
Share your terminal, bright! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.17% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: browser-based remote control for v2 desktop terminals' accurately and concisely summarizes the primary change—adding a new remote control feature for browser-based terminal access. It is clear, specific, and reflects the main functionality added.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description is comprehensive and well-structured, covering all major components of the remote control feature with clear implementation details and test plan.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch remote-terminal-implement

@greptile-apps

greptile-apps Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements an end-to-end browser-based remote control feature for v2 desktop terminals, spanning the full stack: a new DB table, cloud tRPC router, host-service HMAC token mint/verify + in-memory session registry, relay auth bypass, and a Next.js web viewer using xterm.js.

  • Host-service: HMAC-signed tokens are minted per-session, verified with timing-safe comparison, and backed by a 256 KB tail ring buffer and in-memory viewer fan-out with a 4-viewer cap and per-viewer rate limits (200 input/s, 10 resize/s).
  • Cloud tRPC: remoteControl.{create,get,revoke,listForWorkspace,expireStale} — mints a short-lived JWT for the relay POST, inserts the DB row only after the host returns a token, and stores only the SHA-256 hash.
  • Web viewer: /agents/remote-control/[sessionId] page with RemoteTerminal client component calls remoteControl.get (a protectedProcedure) to resolve the WebSocket URL — this blocks unauthenticated viewers from connecting, which is at odds with the "anyone with the link" intent.

Confidence Score: 3/5

The feature is not safe to merge without resolving the authentication gap — unauthenticated viewers cannot reach the WebSocket endpoint because the get call requires a Superset session.

The core host-service logic (HMAC signing, timing-safe verification, rate limiting, expiry sweep, revoke fan-out) is solid. The relay bypass and DB migration are correct. Two issues stand out: the viewer web page calls a protected tRPC endpoint to resolve the WebSocket URL, which silently blocks anyone without a Superset account from ever seeing the terminal; and the onError WebSocket handler omits the removeViewer and revoke-listener teardown that onClose performs, which can permanently consume viewer slots from the 4-viewer cap when connections drop via error.

RemoteTerminal.tsx (auth-gated wsUrl fetch) and route.ts (incomplete onError cleanup) need the most attention before merge.

Security Review

  • Token exposure in URL: The raw HMAC bearer token is embedded as a plain query parameter in the share URL (buildWebUrl), making it visible in browser history, server/proxy access logs, and Referer headers for its full TTL (up to 24 h). Moving it to the URL fragment would eliminate server-side exposure.
  • Relay auth bypass scope: All paths under /hosts/:hostId/remote-control/* bypass user-JWT validation. Only the single /remote-control/:sessionId WebSocket route is registered on the host-service, so other paths 404; the bypass is wider than strictly necessary but carries no practical escalation risk under the current route layout.
  • Timing-safe comparison: Token hash comparison uses crypto.timingSafeEqual correctly throughout — no timing oracle.
  • No raw token persistence: Only the SHA-256 hash is stored in the database; the raw HMAC is returned once and never persisted.

Important Files Changed

Filename Overview
packages/host-service/src/terminal/remote-control/route.ts WebSocket route for viewer connections — correct HMAC auth and rate-limiting on open, but onError handler is missing removeViewer/unsubscribeRevoke cleanup, and per-message full re-auth is costly.
packages/host-service/src/terminal/remote-control/session-manager.ts In-memory session registry with HMAC mint/verify, timing-safe comparison, expiry sweep, and revoke fan-out — well implemented.
apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx Browser viewer component — calls a protectedProcedure to resolve wsUrl, blocking unauthenticated viewers from connecting despite the intent to support "anyone with the link".
packages/trpc/src/router/remote-control/remote-control.ts Cloud tRPC router for session lifecycle — org-scoped, correct DB operations; raw bearer token in share URL is a security hygiene concern.
apps/relay/src/index.ts Relay middleware: intentionally bypasses user-JWT gate for /remote-control/* paths, delegating auth to the host-service HMAC check — correctly implemented.
packages/db/drizzle/0048_add_v2_remote_control_sessions.sql Migration adds v2_remote_control_sessions table with enums, FK cascade deletes, and unique index on token_hash — correct.
packages/shared/src/remote-control-protocol.ts Wire protocol types and constants shared between host-service and web viewer — clean, no issues.
packages/host-service/src/trpc/router/terminal/remote-control.ts Host-side tRPC handlers for mintToken and revoke — correctly validates terminal existence before minting, registers session atomically.

Sequence Diagram

sequenceDiagram
    participant Desktop as Desktop (host-service)
    participant Cloud as Cloud tRPC
    participant Relay
    participant Browser as Browser (viewer)

    Desktop->>Cloud: remoteControl.create(workspaceId, terminalId, mode)
    Cloud->>Cloud: mint short-lived user JWT (300s)
    Cloud->>Relay: POST /hosts/:routingKey/trpc — mintToken
    Relay->>Desktop: forward (user JWT verified by relay)
    Desktop->>Desktop: HMAC-sign token, registerSession in-memory
    Desktop-->>Cloud: token, tokenHash, expiresAt
    Cloud->>Cloud: INSERT v2_remote_control_sessions (tokenHash only)
    Cloud-->>Desktop: sessionId, webUrl, wsUrl
    Desktop->>Desktop: copy webUrl to clipboard
    Browser->>Cloud: remoteControl.get(sessionId) [requires Superset session]
    Cloud-->>Browser: wsUrl, mode, status
    Browser->>Relay: WS upgrade to remote-control route
    Note over Relay: user-JWT gate bypassed for remote-control paths
    Relay->>Desktop: forward WebSocket
    Desktop->>Desktop: verify HMAC token + timing-safe hash check
    Desktop-->>Browser: hello + snapshot frames
    loop Live output
        Desktop-->>Browser: data frames (base64)
        Browser->>Desktop: input or resize (full mode only)
    end
    Browser->>Cloud: remoteControl.revoke(sessionId)
    Cloud->>Cloud: "UPDATE status=revoked"
    Cloud->>Desktop: terminal.remoteControl.revoke (best-effort)
    Desktop->>Desktop: revokeSession — fan-out revoked frame to all viewers
Loading
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx:112-139
**Viewer requires Superset authentication to resolve `wsUrl`**

`trpcClient.remoteControl.get` is a `protectedProcedure` that demands a valid user session. An unauthenticated visitor who opens the share link will receive a 401 before the WebSocket connection is even attempted, making the feature non-functional for the "anyone opening the link" use-case described in the PR. The raw token in the URL already proves the viewer was given access by the session owner; either embed the `wsUrl` in the share URL so the client component can skip the `get` call, or add a token-gated unprotected lookup procedure that accepts the raw `remoteControlToken` and returns only the WebSocket endpoint.

### Issue 2 of 4
packages/host-service/src/terminal/remote-control/route.ts:342-351
**`onError` skips `removeViewer` and revoke-listener cleanup**

`onError` nulls out `ctx.viewer` but never calls `removeViewer(sessionId, ctx.viewerSocket)` or `ctx.unsubscribeRevoke()`. If the underlying node-ws adapter fires `onError` without a subsequent `onClose` (possible on abrupt socket tear-down), the `ViewerSocket` object stays in the session's `viewers` Set indefinitely. Because `REMOTE_CONTROL_MAX_VIEWERS` is only 4, a handful of such orphaned error states can permanently block new viewers from joining until the host-service restarts. The revoke listener also leaks, so the dead socket will receive close attempts for the session's entire remaining lifetime. `onClose` should be the single cleanup path, and `onError` should delegate to it — or at minimum the same cleanup block should be duplicated here.

### Issue 3 of 4
packages/host-service/src/terminal/remote-control/route.ts:252-257
**Full HMAC re-verification on every inbound WebSocket message**

`authenticateSession` is called for each message, which re-runs `verifyRemoteControlToken` (an HMAC-SHA256 computation) plus a second SHA-256 hash of the entire token string to compare against the stored `tokenHash`. At the allowed rate of 200 input messages per second this is 400 hash operations per second per viewer, purely to recheck expiry. The HMAC and stored-hash checks only need to run once at handshake time (already done in `onOpen`). Subsequent messages can guard against expiry with a lightweight `session.expiresAt <= nowSec()` comparison using the value already present in the in-memory `ActiveSession`.

### Issue 4 of 4
packages/trpc/src/router/remote-control/remote-control.ts:50-54
**Raw bearer token exposed in the share URL query string**

`buildWebUrl` embeds the full raw token as a plain query parameter. This means it appears in browser history, server access logs, proxy logs, and HTTP `Referer` headers whenever the viewer navigates away from the page. The DB correctly stores only the SHA-256 hash, but the raw token in the URL is a bearer credential valid for up to 24 hours. Consider passing it in the URL fragment (after `#`) so it is never sent to any server and stays out of logs and `Referer` headers. The `page.tsx` already reads it from `searchParams` server-side, which would need to move to a client-side read, but that is straightforward for a client component.

Reviews (1): Last reviewed commit: "style(web): match desktop terminal theme..." | Re-trigger Greptile

@github-actions

github-actions Bot commented May 10, 2026 •

Copy link
Copy Markdown
Contributor

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) ✅ View Branch
Vercel API (Vercel) ✅ Open Preview
Vercel Web (Vercel) ✅ Open Preview
Vercel Marketing (Vercel) ✅ Open Preview
Vercel Admin (Vercel) ✅ Open Preview
Vercel Docs (Vercel) ✅ Open Preview

Preview updates automatically with new commits

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (6)
packages/host-service/src/terminal/remote-control/session-manager.ts (3)

326-340: 💤 Low value

Doc nit: onRevoke fires asynchronously, not synchronously, when the session is missing.

The comment says "fire synchronously" but queueMicrotask schedules the listener for the next microtask tick. Also, hard-coding "manual" for an already-gone session can mislead consumers (the session may have actually expired or been revoked for another reason). Consider either passing a dedicated reason (e.g., a new "unknown"/"already-gone" variant) or updating the comment to clarify the intent.


155-160: 💤 Low value

Unreachable try/catch around base64UrlDecode.

Buffer.from(s, "base64") does not throw on malformed input — it silently ignores invalid characters — so this try/catch will never trigger. The downstream timingSafeEqual length check (line 166) will catch a malformed signature anyway.

♻️ Proposed cleanup
-	let providedSig: Buffer;
-	try {
-		providedSig = base64UrlDecode(sigB64);
-	} catch {
-		return { ok: false, reason: "malformed" };
-	}
+	const providedSig = base64UrlDecode(sigB64);
🤖 Prompt for 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.

In `@packages/host-service/src/terminal/remote-control/session-manager.ts` around
lines 155 - 160, Remove the unreachable try/catch around base64UrlDecode:
directly assign providedSig = base64UrlDecode(sigB64) (no try/catch) and rely on
the existing timingSafeEqual length check to detect malformed signatures; ensure
the subsequent length comparison/timingSafeEqual logic in the same function
still returns { ok: false, reason: "malformed" } when lengths differ.

68-80: 💤 Low value

Prefer Node's native base64url encoding.

Node ≥16 supports 'base64url' directly on Buffer, which removes the need for the manual +/= substitution and padding math in a security-sensitive path.

♻️ Proposed simplification
-function base64UrlEncode(buf: Buffer | string): string {
-	const b = typeof buf === "string" ? Buffer.from(buf, "utf8") : buf;
-	return b
-		.toString("base64")
-		.replace(/\+/g, "-")
-		.replace(/\//g, "_")
-		.replace(/=+$/, "");
-}
-
-function base64UrlDecode(s: string): Buffer {
-	const padded = s + "=".repeat((4 - (s.length % 4)) % 4);
-	return Buffer.from(padded.replace(/-/g, "+").replace(/_/g, "/"), "base64");
-}
+function base64UrlEncode(buf: Buffer | string): string {
+	const b = typeof buf === "string" ? Buffer.from(buf, "utf8") : buf;
+	return b.toString("base64url");
+}
+
+function base64UrlDecode(s: string): Buffer {
+	return Buffer.from(s, "base64url");
+}
🤖 Prompt for 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.

In `@packages/host-service/src/terminal/remote-control/session-manager.ts` around
lines 68 - 80, Replace the custom base64 URL encode/decode logic in
base64UrlEncode and base64UrlDecode with Node's built-in 'base64url' support:
use Buffer.from(...).toString('base64url') in base64UrlEncode and Buffer.from(s,
'base64url') in base64UrlDecode, removing the manual +/_, = replacements and
padding math; keep the functions' names/signatures intact so callers of
base64UrlEncode and base64UrlDecode continue to work.
packages/host-service/src/terminal/remote-control/session-manager.test.ts (1)

234-236: ⚡ Quick win

Make the expiry-sweep test deterministic.

This assertion currently depends on a 20 ms interval firing within a 60 ms wall-clock sleep, which is easy to miss on a busy CI worker. Poll the session state until it flips, or expose a one-shot sweep entrypoint for tests so this coverage stays stable.

🤖 Prompt for 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.

In `@packages/host-service/src/terminal/remote-control/session-manager.test.ts`
around lines 234 - 236, The test's reliance on timing
(startRemoteControlExpirySweep(20) then sleep 60ms) is flaky; change the test to
deterministically wait for the session expiry by polling the session state or by
calling a one-shot sweep helper; specifically modify the test around
startRemoteControlExpirySweep and stopRemoteControlExpirySweep to either (a)
poll the SessionManager/session object until the expired flag or expected state
change is observed (with a short timeout), or (b) add and call a new
singleRunExpirySweep() (or similar) function on the same module that executes
one sweep synchronously for tests, then assert the session state immediately.
Ensure you reference and update the test assertions that currently follow the
sleep to use the polling result or the one-shot sweep output.
apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx (1)

440-478: ⚡ Quick win

Extract MobileToolbar into its own component folder.

MobileToolbar is a self-contained component co-located in RemoteTerminal.tsx. Per project convention, each component should live in its own folder with a barrel export. Move it to components/MobileToolbar/MobileToolbar.tsx with an index.ts re-export, alongside RemoteTerminal/.

As per coding guidelines: "Organize components with one folder per component: ComponentName/ComponentName.tsx + index.ts barrel export".

🤖 Prompt for 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.

In
`@apps/web/src/app/`(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx
around lines 440 - 478, Move the MobileToolbar function out of
RemoteTerminal.tsx into its own component folder named MobileToolbar with the
file MobileToolbar.tsx exporting the component as default (or a named export)
and add an index.ts barrel that re-exports it; update RemoteTerminal.tsx to
import MobileToolbar from the new barrel (e.g. from "components/MobileToolbar")
and remove the in-file MobileToolbar declaration; ensure the prop type
MobileToolbarProps is moved or re-exported as needed so RemoteTerminal still
passes onSend correctly and that any styling/utility imports used by
MobileToolbar are updated in its new file.
packages/trpc/src/router/remote-control/remote-control.ts (1)

142-188: ⚡ Quick win

Compensate for orphaned host sessions if the cloud insert fails.

The host mint at lines 142–163 succeeds before the DB insert at lines 166–177. If the insert throws (constraint violation, transient outage, network blip after the host responded), the host holds a live session keyed to a tokenHash the cloud never persisted — visible to any holder of minted.token until natural expiry, and invisible to listForWorkspace/revoke. A try/catch that best-effort calls terminal.remoteControl.revoke on the host before re-throwing would avoid the orphan.

🛡️ Proposed compensation
-const expiresAt = new Date(minted.expiresAt * 1000);
-await dbWs.insert(v2RemoteControlSessions).values({
-  id: sessionId,
-  organizationId,
-  hostId: host.machineId,
-  workspaceId: workspace.id,
-  terminalId: input.terminalId,
-  createdByUserId: userId,
-  mode: input.mode,
-  status: "active",
-  tokenHash: minted.tokenHash,
-  expiresAt,
-});
+const expiresAt = new Date(minted.expiresAt * 1000);
+try {
+  await dbWs.insert(v2RemoteControlSessions).values({
+    id: sessionId,
+    organizationId,
+    hostId: host.machineId,
+    workspaceId: workspace.id,
+    terminalId: input.terminalId,
+    createdByUserId: userId,
+    mode: input.mode,
+    status: "active",
+    tokenHash: minted.tokenHash,
+    expiresAt,
+  });
+} catch (err) {
+  // Best-effort: revoke the orphan host session so the minted token is unusable.
+  await relayMutation<{ sessionId: string }, unknown>(
+    { relayUrl: env.RELAY_URL, hostId: routingKey, jwt, timeoutMs: 5000 },
+    "terminal.remoteControl.revoke",
+    { sessionId },
+  ).catch((revokeErr) => {
+    console.warn(
+      "[remote-control] failed to revoke orphan host session:",
+      revokeErr instanceof Error ? revokeErr.message : String(revokeErr),
+    );
+  });
+  throw err;
+}
🤖 Prompt for 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.

In `@packages/trpc/src/router/remote-control/remote-control.ts` around lines 142 -
188, The DB insert of the minted session can fail after
relayMutation("terminal.remoteControl.mintToken") returns, leaving an orphaned
host session; wrap the dbWs.insert(v2RemoteControlSessions).values(...) call in
a try/catch and in the catch best-effort call relayMutation with
"terminal.remoteControl.revoke" (using routingKey, sessionId and
minted.tokenHash/token as appropriate) to revoke the host-side session before
re-throwing the original error; ensure you reference the minted, sessionId and
routingKey variables and do not suppress the original exception if revoke itself
fails (log or ignore revoke errors but still throw the insert error).
🤖 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/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/TerminalRemoteControlButton.tsx:
- Around line 31-84: Hydrate and keep the remote-share state in sync with the
backend by querying remoteControl (e.g., remoteControl.listForWorkspace or
remoteControl.get) on mount and when workspaceId changes, and update the local
state variables phase and active accordingly; specifically, inside the component
initialize by calling apiTrpcClient.remoteControl.listForWorkspace/get to find
any live session for terminalId and setActive({sessionId, webUrl, expiresAt})
and setPhase("active") if found, clear state if none or expired, and handle
remote revocations by refreshing or re-querying after stopShare or on a
periodic/ subscription update so the UI shows stop/copy affordances when a
session exists and clears them when the session is expired/ revoked (ensure
startShare and stopShare still update backend and then refresh the hydrated
state after success/failure).

In `@apps/relay/src/index.ts`:
- Around line 185-195: The route middleware for app.use("/hosts/:hostId/*", ...)
currently bypasses authMiddleware for paths under /remote-control/* which skips
the replay/tunnel-ownership checks (maybeReplay and local tunnel presence),
causing incorrect behavior across instances; refactor by extracting the
replay/availability logic from authMiddleware into a separate function (e.g.,
maybeReplayOrCheckTunnelAvailability or reuse maybeReplay()) and invoke that
function in this middleware before deciding to call authMiddleware or next(),
ensuring the /remote-control branch still runs the replay/tunnel-ownership
checks (call maybeReplay(...) or the new helper and enforce tunnel-presence
checks) then proceed with c.set("hostId", hostId) and next() as before.

In
`@apps/web/src/app/`(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx:
- Around line 249-253: The terminal's onData handler (term.onData) silently
drops keystrokes when meta.mode !== "full", so surface a one-time read-only hint
to the user; modify RemoteTerminal to show a faint message when mode is not
"full" (e.g., write a dim inline note into the xterm buffer via term.writeln or
render a transient DOM overlay/header) and ensure it only appears once per
session (track with a local flag like hasShownReadOnlyHint) so normal input
handling (sendClientMessage, bytesToBase64) remains unchanged when meta.mode ===
"full".

In `@packages/host-service/src/terminal/remote-control/route.ts`:
- Around line 240-316: The onMessage handler currently JSON.parses into
RemoteControlClientMessage without runtime validation and calls
ctx.viewer.resize(parsed.cols, parsed.rows) and
ctx.viewer.runCommand(parsed.command) unguarded; add a Zod discriminated-union
schema for RemoteControlClientMessage and validate the parsed payload (rejecting
and sendError on validation failure) before the switch, and wrap the
ctx.viewer.resize(...) and ctx.viewer.runCommand(...) calls in try/catch blocks
similar to the input branch so runtime type errors are caught and reported via
sendError; reference the onMessage handler, RemoteControlClientMessage,
ctx.viewer.resize, ctx.viewer.runCommand, and ensure rate-limit checks remain
unchanged.

In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 548-555: sendInput currently converts bytes to a latin1 string
then calls session.pty.write, which later re-encodes as UTF-8 and corrupts
high-bytes; instead send raw bytes to the PTY: change sendInput to forward the
Buffer directly (construct the Buffer from bytes.buffer/byteOffset/byteLength)
and call a binary-aware API on the PTY (e.g., session.pty.writeBuffer or update
makeDaemonPty.write to accept a Buffer/Uint8Array and write it to the daemon
without re-encoding). Update the PTY implementation (makeDaemonPty.write) to
accept and pass-through Buffer/Uint8Array data verbatim rather than doing
Buffer.from(data, "utf8"), so non-ASCII/pasted bytes are preserved.
- Around line 437-447: pushToTailRing currently pushes an entire incoming bytes
chunk even when it's larger than REMOTE_CONTROL_TAIL_BYTES, then immediately
discards it leaving the snapshot empty; fix by truncating the chunk before
pushing so only its last REMOTE_CONTROL_TAIL_BYTES are kept: inside
pushToTailRing, if copy.byteLength > REMOTE_CONTROL_TAIL_BYTES replace copy with
copy.slice(copy.byteLength - REMOTE_CONTROL_TAIL_BYTES) (preserving the tail),
then push that sliced copy to session.tailRing and update session.tailRingBytes
as before so the existing trimming loop can operate correctly on the
already-truncated chunk.

In `@packages/host-service/src/trpc/router/terminal/remote-control.ts`:
- Around line 12-18: The ttlSec field is only checked as positive in the
mintTokenInput schema so a caller can request >86,400s; update the host-side
validation to enforce the 24-hour cap: change mintTokenInput.ttlSec to a
z.number().int().positive().max(86400).optional() (or if you prefer,
validate/clamp inside mintRemoteControlToken) and ensure mintRemoteControlToken
rejects or clamps any ttlSec > 86400 before minting the HMAC token so the host
remains the final authority.

In `@packages/trpc/src/router/remote-control/remote-control.ts`:
- Around line 239-246: Tighten the revoke UPDATE by narrowing its WHERE clause:
when calling dbWs.update(v2RemoteControlSessions).set(...) for input.sessionId,
add checks for organizationId (matching the orgId you validated in the prior
findFirst) and for status = 'active' so the UPDATE only affects sessions owned
by that organization and only active sessions; this prevents race-condition
overwrites and preserves original revokedAt/revokedByUserId for
already-revoked/expired rows (keep using input.sessionId and userId in the set
payload).

---

Nitpick comments:
In
`@apps/web/src/app/`(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx:
- Around line 440-478: Move the MobileToolbar function out of RemoteTerminal.tsx
into its own component folder named MobileToolbar with the file
MobileToolbar.tsx exporting the component as default (or a named export) and add
an index.ts barrel that re-exports it; update RemoteTerminal.tsx to import
MobileToolbar from the new barrel (e.g. from "components/MobileToolbar") and
remove the in-file MobileToolbar declaration; ensure the prop type
MobileToolbarProps is moved or re-exported as needed so RemoteTerminal still
passes onSend correctly and that any styling/utility imports used by
MobileToolbar are updated in its new file.

In `@packages/host-service/src/terminal/remote-control/session-manager.test.ts`:
- Around line 234-236: The test's reliance on timing
(startRemoteControlExpirySweep(20) then sleep 60ms) is flaky; change the test to
deterministically wait for the session expiry by polling the session state or by
calling a one-shot sweep helper; specifically modify the test around
startRemoteControlExpirySweep and stopRemoteControlExpirySweep to either (a)
poll the SessionManager/session object until the expired flag or expected state
change is observed (with a short timeout), or (b) add and call a new
singleRunExpirySweep() (or similar) function on the same module that executes
one sweep synchronously for tests, then assert the session state immediately.
Ensure you reference and update the test assertions that currently follow the
sleep to use the polling result or the one-shot sweep output.

In `@packages/host-service/src/terminal/remote-control/session-manager.ts`:
- Around line 155-160: Remove the unreachable try/catch around base64UrlDecode:
directly assign providedSig = base64UrlDecode(sigB64) (no try/catch) and rely on
the existing timingSafeEqual length check to detect malformed signatures; ensure
the subsequent length comparison/timingSafeEqual logic in the same function
still returns { ok: false, reason: "malformed" } when lengths differ.
- Around line 68-80: Replace the custom base64 URL encode/decode logic in
base64UrlEncode and base64UrlDecode with Node's built-in 'base64url' support:
use Buffer.from(...).toString('base64url') in base64UrlEncode and Buffer.from(s,
'base64url') in base64UrlDecode, removing the manual +/_, = replacements and
padding math; keep the functions' names/signatures intact so callers of
base64UrlEncode and base64UrlDecode continue to work.

In `@packages/trpc/src/router/remote-control/remote-control.ts`:
- Around line 142-188: The DB insert of the minted session can fail after
relayMutation("terminal.remoteControl.mintToken") returns, leaving an orphaned
host session; wrap the dbWs.insert(v2RemoteControlSessions).values(...) call in
a try/catch and in the catch best-effort call relayMutation with
"terminal.remoteControl.revoke" (using routingKey, sessionId and
minted.tokenHash/token as appropriate) to revoke the host-side session before
re-throwing the original error; ensure you reference the minted, sessionId and
routingKey variables and do not suppress the original exception if revoke itself
fails (log or ignore revoke errors but still throw the insert error).
🪄 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: 725c98c0-8e4e-4a47-b0ac-e70f9ba6c822

📥 Commits

Reviewing files that changed from the base of the PR and between be63d1d and b2b2495.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (28)
  • apps/desktop/src/main/host-service/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/TerminalHeaderExtras.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/TerminalRemoteControlButton.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx
  • apps/relay/src/index.ts
  • apps/web/package.json
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/index.ts
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/page.tsx
  • packages/db/drizzle/0048_add_v2_remote_control_sessions.sql
  • packages/db/drizzle/meta/0048_snapshot.json
  • packages/db/drizzle/meta/_journal.json
  • packages/db/src/schema/enums.ts
  • packages/db/src/schema/schema.ts
  • packages/host-service/src/app.ts
  • packages/host-service/src/serve.ts
  • packages/host-service/src/terminal/remote-control/route.ts
  • packages/host-service/src/terminal/remote-control/session-manager.test.ts
  • packages/host-service/src/terminal/remote-control/session-manager.ts
  • packages/host-service/src/terminal/terminal.ts
  • packages/host-service/src/trpc/router/terminal/remote-control.ts
  • packages/host-service/src/trpc/router/terminal/terminal.ts
  • packages/shared/package.json
  • packages/shared/src/remote-control-protocol.ts
  • packages/trpc/src/root.ts
  • packages/trpc/src/router/remote-control/index.ts
  • packages/trpc/src/router/remote-control/remote-control.ts

Comment thread apps/relay/src/index.ts Resolved

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 29 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/trpc/src/router/remote-control/remote-control.ts">

<violation number="1" location="packages/trpc/src/router/remote-control/remote-control.ts:53">
P2: The raw HMAC token (valid up to 24 h) is placed in the URL query string, which means it will appear in server access logs, browser history, intermediary proxy logs, and `Referer` headers when the viewer navigates away. Use a URL fragment (`#`) instead of `?` so the credential stays client-side and is never transmitted to any server. The `page.tsx` currently reads it from `searchParams` (server-side); shift that read to the client component.</violation>

<violation number="2" location="packages/trpc/src/router/remote-control/remote-control.ts:200">
P2: Missing status guard before revoking: if the session is already `revoked` or `expired`, this overwrites the original `revokedAt`/`revokedByUserId` audit data and makes an unnecessary relay call. Add a check after fetching the row.</violation>
</file>

<file name="apps/relay/src/index.ts">

<violation number="1" location="apps/relay/src/index.ts:190">
P1: Remote-control bypass skips `fly-replay` routing, so viewer WebSocket connections will fail in multi-instance deployments when the request lands on an instance that doesn't own the host tunnel. Add replay handling for the remote-control path (without the auth/access checks) before calling `next()`.</violation>
</file>

<file name="packages/host-service/src/terminal/remote-control/route.ts">

<violation number="1" location="packages/host-service/src/terminal/remote-control/route.ts:313">
P2: Wrap `ctx.viewer.runCommand(parsed.command)` in a try-catch to match the `sendInput` error handling. Both call `session.pty.write()` under the hood, which can throw if the pty is closed or errored.</violation>
</file>

<file name="apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx">

<violation number="1" location="apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx:112">
P1: `trpcClient.remoteControl.get.query` is a `protectedProcedure` requiring a valid user session. An unauthenticated visitor opening the share link will receive a 401 before the WebSocket is attempted, breaking the "anyone with the link" use-case. The raw HMAC token already proves the viewer was authorized by the session owner. Either embed the `wsUrl` in the share URL directly (avoiding this call), or add an unprotected token-gated lookup that accepts the `remoteControlToken` and returns only the WebSocket endpoint.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/relay/src/index.ts Resolved
Resolves 11 findings from greptile + coderabbit review on the
remote-control feature:

- #1 (P1): `remoteControl.get` is now `publicProcedure` and accepts the
  raw token, hashing it for constant-time comparison against the row's
  `tokenHash`. Anonymous viewers can resolve `wsUrl` without a Superset
  session — the share link itself is the credential.
- #10 (Major): the host-side `sendInput` no longer round-trips bytes
  through a latin1 string before `pty.write` re-encodes them as UTF-8
  (which corrupted any byte ≥ 0x80). Adds `pty.writeBytes` that
  forwards a `Uint8Array` straight to the daemon.
- #2: a single `cleanup()` helper now handles `onClose` and `onError`,
  removing the viewer from the session's set, detaching the handle, and
  unsubscribing the revoke listener idempotently. Fixes a leak where
  abrupt teardown could orphan up to four `MAX_VIEWERS` slots until host
  restart.
- #8: client WebSocket payloads are validated via a zod discriminated
  union before dispatch; `resize` and `runCommand` are wrapped in
  try/catch like `input` was.
- #5: `TerminalRemoteControlButton` hydrates from
  `remoteControl.listForWorkspace` on mount and refreshes every 30s, so
  the live badge survives remounts and reflects backend revocation /
  expiry. The original `webUrl` is unrecoverable after `create` (the
  cloud only stores `tokenHash`), so Copy Link is disabled when we
  don't hold it.
- #3: handshake-time auth result is cached on the WS context; per-
  message handling just compares `expiresAt` against `now` instead of
  re-running HMAC + SHA-256 at 200/s/viewer.
- #4: the bearer token is now passed in the URL fragment
  (`#remoteControlToken=…`), not the query string. The fragment never
  reaches the server, never appears in `Referer` headers, and stays out
  of access logs and history. A new `RemoteTerminalLoader` client
  component reads `location.hash` after mount.
- #7: the web viewer writes a one-time dim hint into xterm when the
  user types in `command` mode so silent drops are explained.
- #9: oversized PTY chunks (> 256 KB in one event) now have their tail
  preserved instead of being pushed-and-immediately-shifted out of the
  ring, which would have left late-joining viewers with an empty
  snapshot.
- #11: host-side mintToken schema now `.min(MIN_TTL).max(MAX_TTL)`,
  matching `mintRemoteControlToken`'s internal clamp.
- #12: revoke `UPDATE` adds `organizationId` and `status='active'` to
  the `WHERE` so re-revoke is idempotent and cannot transition an
  `expired` row to `revoked`.

Skipped: #6 (relay replay/tunnel-ownership) — the existing host proxy
paths don't call `maybeReplay` either, so this PR doesn't regress the
single-region behavior. Multi-region replay is a broader gap tracked
separately.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
packages/host-service/src/terminal/remote-control/route.ts (2)

381-397: 💤 Low value

runCommand shares inputBucket — confirm intent.

runCommand consumes from ctx.inputBucket (sized by REMOTE_CONTROL_INPUT_RATE_PER_SEC), so an input-heavy viewer can starve runCommand and vice-versa, and the user-facing message "Command rate limit exceeded" is actually reporting input-bucket exhaustion. If that's the intended unified write-budget design, ignore; otherwise consider a dedicated runCommandBucket or relabel the error.

🤖 Prompt for 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.

In `@packages/host-service/src/terminal/remote-control/route.ts` around lines 381
- 397, The review points out that runCommand currently consumes from the shared
ctx.inputBucket (sized by REMOTE_CONTROL_INPUT_RATE_PER_SEC), which can cause
runCommand and other input actions to contend; decide whether to keep the
unified budget or separate them: if you want a dedicated budget, add a new
bucket (e.g., runCommandBucket) and use consume(runCommandBucket) in the
runCommand case; if you intend the shared budget, keep ctx.inputBucket but
change the sendError call to a clearer message (or relabel the error code) so
the user sees that the command failed due to the shared input rate limit. Ensure
you update the placement where consume is called and any related bucket
initialization (and reference runCommand, ctx.inputBucket, consume,
REMOTE_CONTROL_INPUT_RATE_PER_SEC, and sendError) accordingly.

245-265: 💤 Low value

Set ctx.viewerSocket only after addViewer succeeds.

When the viewer cap is hit, ctx.viewerSocket has already been assigned (line 254), so cleanup() will call removeViewer(sessionId, viewerSocket) on a socket that was never registered. It's almost certainly a no-op in the manager, but the cleaner ordering avoids the wasted call and the implicit assumption about removeViewer tolerating unknown sockets.

♻️ Proposed reorder
-					ctx.viewerSocket = viewerSocket;
 					const added = addViewer(sessionId, viewerSocket);
 					if (!added.ok) {
 						console.warn(
 							`[remote-control] viewer cap reached for ${sessionId}`,
 						);
 						sendError(ws, "max-viewers", "Maximum viewers reached");
 						handle.detach();
 						ctx.viewer = null;
 						ws.close(1013, "max-viewers");
 						return;
 					}
+					ctx.viewerSocket = viewerSocket;
🤖 Prompt for 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.

In `@packages/host-service/src/terminal/remote-control/route.ts` around lines 245
- 265, The current code assigns ctx.viewerSocket before calling addViewer,
causing cleanup() to call removeViewer with an unregistered socket when
addViewer fails; instead, create the local viewerSocket object (still named
viewerSocket), call addViewer(sessionId, viewerSocket) first, and only if
added.ok set ctx.viewerSocket = viewerSocket; keep the error branch unchanged
except avoid touching ctx.viewerSocket (or set it to null only after a
successful add/remove path), and ensure cleanup() continues to check
ctx.viewerSocket truthiness before calling removeViewer; update references
around addViewer, viewerSocket, ctx.viewerSocket, removeViewer, and cleanup
accordingly.
🤖 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 `@packages/trpc/src/router/remote-control/remote-control.ts`:
- Around line 190-202: The insert of the session row
(dbWs.insert(v2RemoteControlSessions).values({...})) can fail after the host has
already registered the token (minted), leaving a dangling host-side token; wrap
that insert in a try/catch and on error call the same best-effort revoke RPC
used elsewhere (e.g., terminal.remoteControl.revoke) with the same identifiers
(sessionId, organizationId, host.machineId, workspace.id, input.terminalId,
userId) to remove the token from the host; ensure the catch logs the failure and
does not rethrow so the host revoke is attempted regardless.
- Around line 167-188: The relayMutation call in the create flow (calling
"terminal.remoteControl.mintToken") doesn’t set a timeout, so a slow/stuck relay
can hang the operation; update the relayMutation invocation in the create path
to include a bounded timeoutMs (match the revoke path, e.g. timeoutMs: 5000) in
the options object passed to relayMutation so the mintToken call fails fast and
surfaces an error to the user.

---

Nitpick comments:
In `@packages/host-service/src/terminal/remote-control/route.ts`:
- Around line 381-397: The review points out that runCommand currently consumes
from the shared ctx.inputBucket (sized by REMOTE_CONTROL_INPUT_RATE_PER_SEC),
which can cause runCommand and other input actions to contend; decide whether to
keep the unified budget or separate them: if you want a dedicated budget, add a
new bucket (e.g., runCommandBucket) and use consume(runCommandBucket) in the
runCommand case; if you intend the shared budget, keep ctx.inputBucket but
change the sendError call to a clearer message (or relabel the error code) so
the user sees that the command failed due to the shared input rate limit. Ensure
you update the placement where consume is called and any related bucket
initialization (and reference runCommand, ctx.inputBucket, consume,
REMOTE_CONTROL_INPUT_RATE_PER_SEC, and sendError) accordingly.
- Around line 245-265: The current code assigns ctx.viewerSocket before calling
addViewer, causing cleanup() to call removeViewer with an unregistered socket
when addViewer fails; instead, create the local viewerSocket object (still named
viewerSocket), call addViewer(sessionId, viewerSocket) first, and only if
added.ok set ctx.viewerSocket = viewerSocket; keep the error branch unchanged
except avoid touching ctx.viewerSocket (or set it to null only after a
successful add/remove path), and ensure cleanup() continues to check
ctx.viewerSocket truthiness before calling removeViewer; update references
around addViewer, viewerSocket, ctx.viewerSocket, removeViewer, and cleanup
accordingly.
🪄 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: 80a3ffe3-08ad-4007-a078-b1b5d4a4a2b1

📥 Commits

Reviewing files that changed from the base of the PR and between b2b2495 and 25aae3c.

📒 Files selected for processing (9)
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/TerminalRemoteControlButton.tsx
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminalLoader.tsx
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/index.ts
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/page.tsx
  • packages/host-service/src/terminal/remote-control/route.ts
  • packages/host-service/src/terminal/terminal.ts
  • packages/host-service/src/trpc/router/terminal/remote-control.ts
  • packages/trpc/src/router/remote-control/remote-control.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/index.ts
  • packages/host-service/src/trpc/router/terminal/remote-control.ts
  • apps/web/src/app/(agents)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 9 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/trpc/src/router/remote-control/remote-control.ts">

<violation number="1" location="packages/trpc/src/router/remote-control/remote-control.ts:220">
P1: `remoteControl.get` now carries a bearer token but is still implemented as a tRPC query, so the token is serialized into URL query params by `httpBatchLink` and can leak via request logs.</violation>

<violation number="2" location="packages/trpc/src/router/remote-control/remote-control.ts:245">
P1: `get` returns `wsUrl`/`routingKey` even for revoked/expired sessions; this weakens cloud-side revocation as an attach gate when host revoke propagation is delayed or fails.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Resolves four further findings on PR #4345:

- High: redact `remoteControlToken` (and any `token` query param) from the
  relay's request logger. The viewer has to put the bearer on the WS
  upgrade URL because browser WebSockets can't carry custom headers, and
  Hono's default `logger()` would otherwise spill the raw token into Fly
  logs / Sentry breadcrumbs.

- High: when the bypass for `/hosts/:hostId/remote-control/*` skips the
  user-JWT `authMiddleware`, still run the tunnel-presence check + Fly
  `maybeReplay`. Previously a viewer landing on a relay instance that
  doesn't own the destination tunnel would get a hard failure instead
  of a `fly-replay` to the right region/instance.

- Medium: the web viewer's Stop button now calls a new public
  `remoteControl.revokeWithToken({ sessionId, token })` mutation. The
  protected `revoke` requires a Superset session, which anonymous share
  recipients don't have, so the previous wiring silently 401'd. The
  bearer token IS the credential — anyone holding it has the same
  authority as whoever they got the link from. Constant-time SHA-256
  match against `tokenHash`, then revoke + best-effort host tear-down.

- Medium: `revoke` (protected) and `listForWorkspace` now require host
  membership in addition to active org membership, matching the gate
  on `create`. Otherwise an org member who isn't on the host could
  enumerate or revoke other people's share sessions.

The shared host-revoke flow is factored into `callHostRevoke()` so both
the protected and public revoke paths use the same best-effort tear-
down.
…mationFrame

ResizeObserver can fire at refresh-rate (~60Hz) during a window-drag,
which immediately trips the host's REMOTE_CONTROL_RESIZE_RATE_PER_SEC
= 10 rate limit and surfaces a spurious "rate-limited" banner in the
viewer during normal use. Coalesce to one fit+broadcast per animation
frame and cancel any pending frame in the cleanup so we don't
fit+send after the WS is gone.
…limit banner

The RAF coalescing from the previous commit didn't actually help —
ResizeObserver already fires once per layout, so RAF was a no-op for
sustained window-drag, and the host's 10/s bucket still drained in
~166ms and tripped the "rate-limited" error.

Switch to a trailing 200ms debounce: keep calling `fit()` per event so
local rendering stays responsive, but only fire the host-side resize
message once after the user stops dragging. 5 Hz worst case, well
under the host's 10/s cap.
Desktop's terminal-runtime uses the @XTerm beta 6.x track for
kittyKeyboard + scrollbar options; the new web remote-control viewer
uses stable 5.x because the beta is too churny for a fresh feature.
The split is intentional and documented in the viewer's source, so
tell Sherif to ignore those two deps rather than force-aligning to
either track.
@AviPeltz
AviPeltz merged commit f407aea into main May 11, 2026
16 checks passed
MocA-Love added a commit to MocA-Love/superset that referenced this pull request May 29, 2026
Add the remote-control protocol, host-service session manager/route/tRPC surface, cloud remoteControl router, relay bypass for remote-control WebSockets, and the web viewer page.

Wire the desktop terminal header to the feature-flagged share button while preserving the fork terminal runtime and current xterm dependency set. Drizzle migration files were not edited; generate/apply the v2_remote_control_sessions migration on a Neon branch before release.

This branch was previously deployed

1 inactive deployment
preview — a95b19ec Deployed May 11, 2026 by AviPeltz via Deploy Admin #5197
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.

1 participant