Repository navigation
Fix SSH browser loopback fetches across ports - #3820
Merged
Merged
Conversation
An SSH browser page can navigate through the loopback proxy alias, but JavaScript inside that page still emits literal localhost URLs. The regression test loads one remote-alias browser session and verifies that two arbitrary loopback ports from the same page context need to be translated to the proxy alias while HTTPS remains untouched. Constraint: The repro must cover the browser-pane path without requiring a live SSH host in unit tests. Rejected: Testing source text for the injected script | project policy requires runtime behavior, not grep-style assertions. Confidence: high Scope-risk: narrow Directive: Keep this as the first commit in the issue-3819 stack so CI can show the test failing before the fix. Tested: Not run locally; this is the intentionally failing regression-test commit. Not-tested: Full cmux-unit suite; will use CI per workflow constraints.
The SSH browser proxy already multiplexes arbitrary remote ports once requests use the loopback alias. Page JavaScript was outside that boundary after the aliasing change, so fetches and other runtime APIs kept targeting local-machine localhost and never reached the remote daemon proxy. This injects a document-start bridge only on cmux loopback alias pages. It rewrites HTTP and WebSocket localhost-family URLs to the same alias host while preserving arbitrary ports, paths, queries, and localhost subdomains. The existing proxy request and response rewriters then map those requests back to remote loopback and keep CORS/cookie headers aligned. Constraint: Do not hardcode common dev ports; the route must remain lazy and per request. Rejected: Reopening tunnels for a fixed frontend/backend port pair | users run arbitrary ports and the proxy already supports per-request multiplexing. Rejected: Reverting alias navigation wholesale | WebKit loopback proxy bypass was the reason the alias exists. Confidence: medium Scope-risk: moderate Directive: Any future remote-browser loopback routing change must cover page-initiated requests, not only top-level omnibar navigations. Tested: node --check on the injected JavaScript body. Not-tested: Local xcodebuild/cmux-unit; CI will run the regression test per workflow constraints.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoteLoopbackProxyAlias centralizes loopback-host detection and provides runtimeBridgeScriptSource JS. BrowserPanel injects that script at document start for all frames. BrowserPanel host-rewriting functions delegate to RemoteLoopbackProxyAlias. Tests and a wait helper validate multiple loopback URL rewrite cases. ChangesRemote Loopback URL Rewriting Bridge
Sequence Diagram(s)sequenceDiagram
participant WebView as WKWebView
participant Config as WKWebViewConfiguration
participant Bridge as JS Bridge
participant Page as Web Page
participant Alias as Alias Host
WebView->>Config: configureWebViewConfiguration()
Config->>WebView: addUserScript(Bridge) atDocumentStart
Page->>Bridge: network call to http://localhost:PORT/...
Bridge->>Alias: rewrite host -> cmux-loopback.localtest.me and forward
Alias-->>Page: response via rewritten host
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
Contributor
Greptile SummaryFixes cross-port loopback fetch/XHR/WebSocket/EventSource failures in SSH browser panes by injecting a document-start JavaScript bridge (
Confidence Score: 5/5Safe to merge; the bridge is correctly self-gated to the alias host and leaves https/wss untouched. The new RemoteLoopbackRuntimeBridge is a clean, well-scoped JS injection with correct self-gating, deterministic static let initialization, and a regression test covering the exact failure scenario. The loopback-host logic consolidation removes duplication without changing semantics. The only gap is missing U+2028/U+2029 escape sequences in javaScriptStringLiteral, which is theoretical given all current inputs are static ASCII hostnames. No files require special attention; RemoteLoopbackRuntimeBridge.swift is the only new production file and its escaping helper has a minor defensive gap noted inline. Important Files Changed
Sequence DiagramsequenceDiagram
participant Page as Page JS (alias host)
participant Bridge as RemoteLoopbackRuntimeBridge
participant WK as WKWebView / WebKit
participant Proxy as SOCKS/CONNECT Proxy
participant Remote as SSH Remote Server
Note over WK,Bridge: atDocumentStart (forMainFrameOnly: false)
WK->>Bridge: inject runtimeBridgeScriptSource
Bridge-->>Page: monkey-patch fetch / XHR / WebSocket / EventSource
Page->>Bridge: fetch("http://localhost:8000/api")
Bridge->>Bridge: rewriteLoopbackURL → "http://cmux-loopback.localtest.me:8000/api"
Bridge->>WK: nativeFetch("http://cmux-loopback.localtest.me:8000/api")
WK->>Proxy: CONNECT cmux-loopback.localtest.me:8000
Proxy->>Remote: forward to 127.0.0.1:8000
Remote-->>Proxy: response
Proxy-->>WK: response
WK-->>Page: fetch resolved
Reviews (6): Last reviewed commit: "Preserve TLS hostnames in remote loopbac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2017
to
+2021
| normalizedHost === '::1' | ||
| ) { | ||
| return aliasHost; | ||
| } | ||
| const suffix = '.localhost'; |
Contributor
There was a problem hiding this comment.
wss: connections are silently not rewritten
The rewriteLoopbackURL guard exits early for any protocol other than http: and ws:, which means wss://localhost:… WebSocket connections are left pointing at the literal loopback address and will fail the same way http://localhost fetches did before this fix. Any dev server that exposes WSS for HMR or streaming (e.g. Vite over HTTPS, Next.js with --experimental-https) would break silently — no rewrite, no proxy, connection refused — without the bridge reporting an error. The PR covers four APIs but leaves wss: as a blind spot even though EventSource and plain ws: are fully handled.
Contributor
Author
There was a problem hiding this comment.
Fixed in daaf40d by allowing wss loopback URLs through the same alias rewrite path as ws, preserving arbitrary ports for encrypted WebSocket dev servers.
— Claude Code
Comment on lines
+1984
to
+1986
| static var remoteLoopbackRuntimeBridgeScriptSource: String { | ||
| """ | ||
| (() => { |
Contributor
There was a problem hiding this comment.
static var computes the entire 130-line string on every access — should be static let
All other script-source declarations in this class (telemetryHookBootstrapScriptSource, dialogTelemetryHookBootstrapScriptSource, etc.) are static let. The only reason remoteLoopbackRuntimeBridgeScriptSource is a computed static var is the \(remoteLoopbackProxyAliasHost) interpolation, but that value is itself a static let constant. The whole body can be a static let that evaluates once at first use, matching the established pattern.
Suggested change
| static var remoteLoopbackRuntimeBridgeScriptSource: String { | |
| """ | |
| (() => { | |
| static let remoteLoopbackRuntimeBridgeScriptSource: String = { | |
| """ | |
| (() => { |
Contributor
Author
There was a problem hiding this comment.
Fixed in daaf40d by changing the bridge source to a static let initializer so it matches the surrounding script constants.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmuxTests/GhosttyConfigTests.swift`:
- Line 1340: The loop waiting for the WKWebView should not rely on webView.url
when the content is loaded via loadHTMLString(baseURL:); update the waiting
condition in the loop that currently checks "while webView.isLoading ||
webView.url == nil" to only check webView.isLoading, i.e., replace the OR check
with a single isLoading check so the wait is robust for loadHTMLString(baseURL:)
usage and avoids depending on webView.url.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2823-2829: The injected loopback bridge is currently limited to
main frames by configuration.userContentController.addUserScript(...) using
WKUserScript(..., forMainFrameOnly: true); change this to inject into all frames
by setting forMainFrameOnly to false so same-origin iframes get the rewrite and
proxying; the bridge already self-gates on frame hostname inside
Self.remoteLoopbackRuntimeBridgeScriptSource, so widening the injection is safe
and will no-op for third-party frames.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 394f9c8f-3b9d-43e0-8ec3-5d6a47b047c2
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/GhosttyConfigTests.swift
Review feedback found three valid gaps in the first fix: the bridge did not run inside alias-host subframes, encrypted WebSocket localhost URLs stayed outside the proxy alias path, and the WKWebView test helper depended on loadHTMLString updating webView.url. This keeps the same lazy per-request proxy architecture, but widens the document-start bridge to all frames with the existing hostname self-gate, includes wss URLs in WebSocket-safe rewrites, and makes the test wait only on WebView loading state. Constraint: Do not inject telemetry-style globals into third-party frames; this bridge still returns before mutating globals unless the frame host is the cmux loopback alias. Rejected: Leaving wss unaliased | TLS WebSocket dev servers would hit the same local-loopback failure path as the original fetch bug. Confidence: medium Scope-risk: narrow Directive: Keep the bridge host-gated if injection remains all-frame. Tested: node --check on the updated injected JavaScript body. Not-tested: Local xcodebuild/cmux-unit; PR CI and dispatched CI are used per workflow constraints.
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 `@Sources/Panels/BrowserPanel.swift`:
- Around line 1984-2107: Extract the bridge JS and the localhost-alias mapping
into a new helper (e.g., RemoteLoopbackBridge or LoopbackAliasPolicy) so
BrowserPanel no longer embeds the large JS blob or duplicate alias logic: move
the static let remoteLoopbackRuntimeBridgeScriptSource and the loopback host
mapping logic (currently referenced via remoteLoopbackProxyAliasHost and any
loopbackAliasHost/normalizeHost helpers) into that helper, expose a single
canonical scriptSource string and a Swift-native alias mapping API (e.g.,
RemoteLoopbackBridge.scriptSource and
RemoteLoopbackBridge.rewriteHost(_:)/loopbackAliasHost(_:)), and update
BrowserPanel to call those symbols instead of keeping its own JS string or
duplicate rules. Ensure the helper is used by both the injected JS path and any
native rewrite logic so there is one source of truth.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5086ffb5-4db2-4523-aeea-13ff5590197a
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/GhosttyConfigTests.swift
BrowserPanel should install the remote browser runtime bridge, not own the alias policy that navigation rewriting, header rewriting, and JavaScript runtime rewriting all depend on. Moving the loopback predicate and bridge source into RemoteLoopbackProxyAlias keeps the remote-browser proxy contract in one helper without changing the already validated runtime behavior. Constraint: Reviewer requested a single owner for the loopback alias bridge after the functional fix was already green. Rejected: Leave the WK bridge blob in BrowserPanel | duplicates the alias policy next to navigation rewriting and makes future port-scope regressions easier to reintroduce. Confidence: high Scope-risk: narrow Directive: Keep remote localhost alias decisions in RemoteLoopbackProxyAlias so browser navigation and in-page APIs cannot drift. Tested: Extracted bridge JavaScript with sed, checked it with node --check, and executed the multi-port rewrite harness in Node. Not-tested: Local XCTest per project policy; PR CI will run the macOS unit suite.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/RemoteLoopbackProxyAlias.swift`:
- Around line 82-97: The JS hard-coded loopback list in the
runtimeBridgeScriptSource should be derived from the Swift exactLoopbackHosts
constant to avoid drift: replace the literal array/conditions inside the
loopbackAliasHost JS function with an interpolated representation of
exactLoopbackHosts (and optionally canonicalLoopbackHost for the suffix) when
building runtimeBridgeScriptSource so the JS string literal is generated from
the Swift constants; locate the JS snippet inside runtimeBridgeScriptSource and
inject a joined/escaped version of exactLoopbackHosts (and the suffix) instead
of the hard-coded 'localhost', '127.0.0.1', '0.0.0.0', '::1' values so any
future changes to exactLoopbackHosts automatically propagate to the embedded
script.
- Around line 99-119: Add an inline comment inside the rewriteLoopbackURL
function (near the protocol allowlist check that tests parsed.protocol !==
'http:' && parsed.protocol !== 'ws:' && parsed.protocol !== 'wss:') explaining
why wss: is rewritten but https: is not: note that tests (e.g.,
testRemoteWorkspaceRuntimeBridgeAliasesMultipleLoopbackPortsFromSamePage)
exercise this asymmetry, that WebSocket upgrade paths (wss) are proxied/handled
differently by the runtime so rewriting works for HMR/WS traffic while plain
HTTPS requests are intentionally left unchanged to avoid interfering with normal
HTTPS routing/certificate semantics, and reference loopbackAliasHost as the
rewriting hook; place the comment immediately above or inline with the protocol
check so future maintainers see the rationale where the decision is implemented.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68c1b807-3590-4ef6-a7f6-5dd98bff326d
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftSources/RemoteLoopbackProxyAlias.swift
The injected bridge should not grow a second hard-coded list after moving ownership into RemoteLoopbackProxyAlias. Generating the JavaScript loopback set from the Swift constants keeps the native predicate and page-runtime bridge aligned, while documenting the tested wss/https protocol split at the allowlist. Constraint: CodeRabbit requested removing the remaining drift point between Swift loopback constants and the generated runtime bridge. Rejected: Leave the JavaScript loopback list literal in place | future host additions would still require two edits and reintroduce policy drift. Confidence: high Scope-risk: narrow Directive: Update exactLoopbackHosts only; the bridge script derives its exact-host allowlist from that constant. Tested: Extracted bridge JavaScript with sed, checked it with node --check, and executed the multi-port rewrite harness in Node. Not-tested: Local XCTest per project policy; PR CI will rerun macOS unit tests.
Adding local setup inside the static bridge-script closure changed it from a single-expression closure to a multi-statement closure. Swift then requires an explicit return for the generated script string, which the CI debug build caught. Constraint: Must fix CI without running local xcodebuild directly. Confidence: high Scope-risk: narrow Tested: Extracted bridge JavaScript with sed, checked it with node --check, and executed the multi-port rewrite harness in Node. Not-tested: Local XCTest per project policy; PR CI will rerun.
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 `@Sources/RemoteLoopbackProxyAlias.swift`:
- Around line 55-189: The file mixes loopback-host policy with a large injected
JS runtime bridge; extract the static runtimeBridgeScriptSource and its helper
javaScriptStringLiteral out of RemoteLoopbackProxyAlias into a dedicated browser
runtime helper (e.g., create a new type RemoteLoopbackRuntimeBridge or
BrowserRuntimeBridge) and move the JS string and escaping logic there; update
RemoteLoopbackProxyAlias to reference
RemoteLoopbackRuntimeBridge.runtimeBridgeScriptSource (adjust access level from
private/static as needed), remove the helper from the enum, and run/update any
callers/tests to import/use the new helper so networking/policy code stays
isolated from platform bridge code.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ddc37b1e-96f6-410d-94d9-cd67e4d4e2c8
📒 Files selected for processing (1)
Sources/RemoteLoopbackProxyAlias.swift
The browser SSH loopback bridge had grown inside the alias policy type, which made the policy owner also responsible for a large WebKit runtime script. Split the runtime injection script into its own helper while keeping the alias constants as the single source of truth used by both Swift routing and injected JavaScript. Constraint: Preserve the existing remote loopback alias invariant and avoid direct xcodebuild per project build policy. Rejected: Leave the bridge on RemoteLoopbackProxyAlias | keeps alias policy and WebKit runtime script ownership mixed. Confidence: high Scope-risk: narrow Directive: Keep loopback host policy in RemoteLoopbackProxyAlias and browser runtime API shims in RemoteLoopbackRuntimeBridge. Tested: Node bridge syntax and rewrite harness for localhost 3000/8000, subdomain localhost, ws, wss, and https; plutil lint of project.pbxproj; git diff --check. Not-tested: Local XCTest and app build per workflow; PR CI and final reload.sh launch will verify.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 1ba943d. Configure here.
The runtime bridge can safely alias cleartext HTTP and WebSocket URLs because the proxy can remap those hostnames before the remote request is interpreted. TLS-bearing schemes carry hostname expectations into certificate validation, so the bridge now leaves https and wss loopback URLs unchanged and documents that boundary. Constraint: Avoid changing SNI/certificate expectations for localhost development certificates. Rejected: Rewrite wss like ws | it reaches the remote port but changes the certificate hostname to the alias. Confidence: high Scope-risk: narrow Directive: Do not add TLS schemes to the runtime alias allowlist without a certificate/SNI strategy. Tested: Node bridge syntax and rewrite harness for http 3000/8000, api.localhost, ws rewrite, wss passthrough, and https passthrough; git diff --check. Not-tested: Local XCTest and app build per workflow; PR CI and final reload.sh launch will verify.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
origin/mainin a tagged dev build launched with./scripts/reload.sh --tag repro-3819-main --launch.http:and cleartextws:runtime URLs to the existing SSH loopback alias while preserving arbitrary ports and*.localhostsubdomains.https:andwss:URLs on their original hostnames to avoid changing SNI/certificate validation semantics.Reproduction Evidence
Tagged main build:
repro-3819-main.SSH workspace:
cmux ssh austins-macbook-pro, with remote servers on127.0.0.1:3000and127.0.0.1:8000.Direct browser-pane navigations worked in isolation:
http://localhost:8000/renderedbackend-8000http://localhost:3000/renderedfrontend-3000-plainAfter replacing the 3000 page with:
fetch('http://localhost:8000/') .then(r => r.text()) .then(t => document.body.innerText = t)the page stayed at
pending fetch.Exact browser pane DevTools Console errors captured during the main-branch repro:
The cmux browser CLI also reported:
Regression Source
Regressing commit:
8b2edfe2c7be2c835eddfd525946db2478bcbee2, PR #3764,Allow HTTP localhost subdomains in browser, merged 2026-05-09 01:44 UTC.That change introduced the remote loopback proxy alias path for top-level browser navigations and proxy header rewriting. The missing path was page-initiated browser runtime requests: JavaScript still requested literal
localhost, which WebKit treated as local-machine loopback instead of the SSH workspace proxy target.Checked PR #3801 as requested; it is still open and not merged into
main, so it is not the regression on currentmain.Architecture
The fix keeps the existing lazy per-request proxy architecture. It does not hardcode common dev ports and does not allocate one tunnel per port. Pages loaded through
cmux-loopback.localtest.meget a document-start bridge that maps localhost-family runtime URLs to the same alias host. The existing SOCKS/CONNECT proxy and request/response rewriters then route each arbitrary port back to remote loopback.Ownership is split by responsibility:
RemoteLoopbackProxyAliasowns the loopback alias policy and Swift-native host mapping.RemoteLoopbackRuntimeBridgeowns the injected WebKit runtime bridge script generated from that policy.BrowserPanelonly installs the bridge and delegates host mapping.Test Evidence
6901f5ae8f1b96b8ca63cc72386bf14fcb1c5e81.a83009a6b90f5ea86dbb369919eda7e9d414766c.a38ae275c8345ba6157e05ddede60243a8e5dded.The test-only branch (
issue-3819-browser-ssh-multi-port-test-only) failed as expected before the fix. CI run: https://github.com/manaflow-ai/cmux/actions/runs/25622448205. The macOS unit log showed the new bridge test failing with:The fixed PR branch passes on head
a38ae275c8345ba6157e05ddede60243a8e5dded: 18 passed, 0 failed, 0 pending, 7 skipped. Passing checks include CodeRabbit, Greptile, Cursor Bugbot, activation-session, CircleCI macOS debug/release builds, CircleCI macOS unit tests, remote-daemon-tests, web typecheck, web DB migrations, workflow guard, Vercel, and Socket.Local lightweight validation run before pushing the final review fix:
http://localhost:3000,http://localhost:8000,http://api.localhost:8000,ws://localhost:5173,wss://localhost:5173passthrough, andhttps://localhost:9443passthrough.git diff --checkpassed.plutil -lint GhosttyTabs.xcodeproj/project.pbxprojpassed after addingRemoteLoopbackRuntimeBridge.swift.