Repository navigation
feat(desktop): add authenticated remote port forwarding - #7922
burnmandont wants to merge 26 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the new Effect services (DesktopPortForwardManager, TcpForwardTicketStore, TcpForwardBridge) and their call sites against the service conventions. Service shape, make/layer exports, namespace imports, and dependency acquisition all look right; the findings below are about tagged-error catching and error modeling in the new code.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Reviewed the changed web UI surfaces (DesktopPortForwardControl, DesktopPortForwardsSettings, the ui/select prop addition, and the ChatHeader/ConnectionsSettings call sites). Five findings, mostly around the shared overlay layer, the Button icon-color indirection, and duplicated treatment across the two new port-forward surfaces.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Reviewed the new port-forwarding services against the Effect service conventions. The findings from the previous run (catchTags usage, TcpForwardTargetConnectError structural fields, TcpForwardTicketInvalidError reason/cause, optional cause on DesktopPortForwardError) are all resolved. Two remaining items on error modeling and predicates are noted inline.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
UI consistency review of the new port-forwarding surfaces. Two concrete issues in the changed web UI; everything else (primitive usage, size="xs" route select in ConnectionsSettings, shared useDesktopPortForwards/portForwardConnectionSummary extraction, --control-icon-color indirection on the header trigger) now follows the existing contracts.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One convention finding on the new port-forward renderer helpers. Everything else in the new Effect service code (DesktopPortForwardManager, TcpForwardTicketStore, TcpForwardBridge) follows the canonical Context.Service / make / layer layout, structured Schema.TaggedErrorClass failures, and Effect.catchTags recovery.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Two consistency findings in the new desktop port-forwarding UI (both in apps/web/src/components/settings/DesktopPortForwardsSettings.tsx, one of which also applies to the popover twin). Everything else — Button/Select/Input/Popover/Tooltip composition, the --control-icon-color indirection on the header trigger, the shared useDesktopPortForwards hook, and the new route Select in ConnectionsSettings — matches the existing primitives and call-site conventions.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Reviewed the new port-forwarding UI (DesktopPortForwardControl, DesktopPortForwardsSettings, the shared useDesktopPortForwards/presentation helpers, and the connection-method Select in ConnectionsSettings). The earlier findings on badge/icon-color indirection, hardcoded addresses, label spacing, arrow spacing, and the duplicated hook logic are resolved. One remaining consistency gap: the port inputs accept out-of-range values with no visible feedback, unlike the existing port field on the same settings page.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Reviewed the new Effect services in this slice (TcpForwardTicketStore, TcpForwardBridge, DesktopPortForwardManager, the desktop IPC methods, and the client-runtime route/authorization changes). Service definition order, make/layer exports, namespace imports, Effect.catchTags usage, and the schema predicates in desktopPortForwardAuthorization.ts all match the conventions. Two error-context findings below.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Reviewed the new Effect services and their call sites (TcpForwardTicketStore, TcpForwardBridge, DesktopPortForwardManager, registry/route-transition changes). Two remaining error-modeling items noted inline; imports, service/make/layer shape, catchTags usage, and predicate exports look consistent with the conventions.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One themed-chrome finding in the chat header trigger. Prior findings on these files (settings status alignment, missing port aria-invalid feedback) still apply but are not re-posted.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One layout finding in the new port-forwarding settings form; details inline. Previously reported findings on these files that are still open (status alignment in SettingsRow, the duplicated create/stop/subscription logic shared with DesktopPortForwardControl, and the repeated count-badge treatment) are not re-posted here.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Reviewed the changed web UI surfaces (DesktopPortForwardControl, DesktopPortForwardsSettings, the new route Select in ConnectionsSettings, and the ChatHeader insertion) against the shared primitives and this page's existing form conventions.
Resolved since the last run: the themed-toolbar hook (data-toolbar-control) is now present on the header trigger, the badge count is exposed through the trigger's accessible name, the stray z-70 select positioner override is gone, and the settings validation messages no longer sit inside sm:items-end grid cells.
Two remaining findings are noted inline: the popover's port validation messages are not associated with their inputs (so they are never announced), and the new field labels diverge from the field-label treatment used by every other form on this settings page.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Reviewed the changed web UI surfaces (DesktopPortForwardControl, DesktopPortForwardsSettings, the new route selector in ConnectionsSettings). The label treatment, port-field validation wiring, badge treatment, and primitive usage now match the surrounding conventions. One remaining pending-state inconsistency noted inline.
Posted via Macroscope — UI Consistency
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. Macroscope could not determine whether this PR modifies its approvability configuration, so the PR was not approved automatically. A PR that may change the rules that govern approval is never approved automatically. |
99c51ee to
78b7ec9
Compare
78b7ec9 to
986953d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 986953d. Configure here.
| ), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Session expiry sleep overflows timer
High Severity
expired sleeps until the parent session expiresAt from verifyWebSocketToken, which uses the session row TTL (30 days). That delay exceeds the 32-bit setTimeout limit (~24.8 days), so the sleep can fire immediately, win the race against runBridge, and tear down every new forward right after connect.
Triggered by learned rule: Clamp computed setTimeout delays to 2^31-1 to prevent overflow
Reviewed by Cursor Bugbot for commit 986953d. Configure here.
|
Note 🤖 GPT-5.6 Sol responding on behalf of Theo Closing this PR after an automated pass over open pull requests. Adds a remote port-forwarding architecture across 70 files and depends on #7921. |
2 participants
Depends on #7921. Until that PR lands, GitHub includes its three routing commits in this comparison; the port-forwarding feature itself is the seven commits after
a14598386.What Changed
Why
Remote development often exposes an environment-local HTTP server, database, or debugger that the user needs on their desktop. This provides that path without exposing an unauthenticated listener or introducing another external tunnel dependency.
This is intentionally one complete cross-surface feature PR: contracts, server transport, client authorization, Electron lifecycle, and desktop UI must agree for the feature to be usable and capability-gated during version skew.
Security and Lifecycle
127.0.0.1.127.0.0.1and::1.terminal:operate; active bridges follow parent-session revocation and expiration.UI Changes
Adds a port-forward control to the desktop chat header and management under Settings → Connections. The feature is desktop-only; mobile receives only compatible shared contracts and no forwarding UI.
Screenshots will be added before this draft is marked ready for review.
Verification
git diff --checkpasses against the routing branch.Checklist
Prepared with GPT-5.6-Sol via the Codex harness in T3 Code.
Note
Add desktop remote port forwarding, multi-route environments, and SSH runtime reuse
/api/auth/tcp-forward-ticket, and the desktop app bridges local TCP sockets to remote targets. UI controls added to settings and chat header.EnvironmentRegistrynow tracks and publishes multiple available routes (e.g., Relay, SSH) per environment. Users can switch active routes from the settings UI.routesarray toConnectionCatalogDocument; legacy documents fall back to treatingtargetsas routes. SSH script changes (buildRemoteLaunchScript,buildRemotePairingScript) alter remote shell behavior and reuse logic.Macroscope summarized 986953d.
Note
High Risk
Touches authentication (single-use tickets, session revocation, DPoP expiry), a new WebSocket-to-TCP bridge, and connection-catalog persistence. Mis-handling tickets, destinations, or route selection could expose remote loopback services or drop live sessions.
Overview
Adds desktop-only TCP port forwarding: a local
127.0.0.1listener is bridged to a remote loopback port over a dedicated authenticated WebSocket. The server advertisestcpPortForwarding, issues destination-bound single-use tickets (/api/auth/tcp-forward-ticket,terminal:operate), and dials127.0.0.1then::1. Electron owns listeners, credit-based framing, connection limits, and cleanup; the renderer authorizes each accepted socket through the current prepared connection.UI: cable control in the chat header and a Connections settings panel to start/stop runtime-only forwards. Route changes keep the listener but close in-flight streams so reconnects use the newly selected path.
Also persists multiple routes per environment (selected
targetsplusroutes). Users can switch T3 Connect vs SSH (and similar) from settings after a prepare-and-verify step; failed switches leave the live session unchanged. SSH prepare can reuse a cached bearer instead of always pairing.Reviewed by Cursor Bugbot for commit 986953d. Bugbot is set up for automated code reviews on this repo. Configure here.