Skip to content

Keep remote-initiated opens off this Mac's local network - #15148

Closed
austinywang wants to merge 5 commits into
mainfrom
remote-url-open-guard
Closed

austinywang wants to merge 5 commits into
mainfrom
remote-url-open-guard

Conversation

@austinywang

@austinywang austinywang commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Tightens what a remote machine can make this Mac do without a click.

  • Remote url-open. A url-open from a cloud or SSH machine can no longer hand a loopback, private, link-local, multicast or local-name URL (localhost, *.local, *.internal, single-label names, legacy IPv4 spellings such as 0x7f.1, and mapped IPv6) to the default browser.
    • SSH loopback routes still open in the cmux browser, where they reach the remote machine. For a remote request they never fall back to the system browser, because there they would reach this Mac's own port.
    • A refusal returns false, so the remote side prints the URL instead.
    • Public https:// opens are unchanged, including OAuth and device-login pages.
    • Clicked links are unchanged.
  • Private-address routing. When several SSH machines share 127.0.0.1, a browser URL now goes only to the machine that owns the browser, instead of the first machine the catalog returns.
    • This applies to both BrowserPanel.navigate and rebindCloudRouteIfNeeded.
    • The owner is the browser's cloud route, then the machine that owns its workspace.
  • Cloud notification subtitle. A producer-supplied subtitle still names the machine, as "detail on machine". Line breaks, control characters and bidi overrides are removed, and the detail and machine name are length-capped. reply_shape and click actions were already clamped for cloud rows, so they are unchanged.

The decisions live in CmuxCore and CmuxNotifications value types, which the app calls:

  • PrivateNetworkHostPolicy
  • RemoteLinkOpenPolicy
  • PrivateAddressRouteSelector
  • RemoteMachineNotificationSubtitle

Related: #12373, #10827 and #7397 (related, no overlap).

Follow-ups (not in this PR)

  • Remote file:// and path links that fall through to local files. This covers:
    • canResolveTerminalPathsAgainstLocalFilesystem should treat isLocal != false as remote;
    • the dock terminalLinkIsRemoteTerminal gap.
  • Resolving public DNS names before a remote-initiated open. PrivateNetworkHostPolicy.requiresAddressLookup marks which names need it. DNS rebinding remains after that.
  • Carrying the source machine to dock and split browsers, so a dock browser can route an SSH loopback URL. With no owner, loopback URLs are no longer routed to an arbitrary machine.
  • The notification title is still shown as the producer sent it.
  • App-level regression tests for the coordinator and notification wiring.

Testing

Red commit 99a24db852 has the tests and stubs that keep the old behavior. Green commit 70da666651 has the fix. The same focused commands were run on both:

  • swift test --package-path Packages/macOS/CmuxCore --filter 'PrivateNetworkHostPolicyTests|RemoteLinkOpenPolicyTests|PrivateAddressRouteSelectorTests'
    • Red: 20 tests in 3 suites failed with 59 issues.
    • Green: 20 tests in 3 suites passed.
  • swift test --package-path Packages/macOS/CmuxNotifications --filter RemoteMachineNotificationSubtitleTests
    • Red: 6 tests failed with 6 issues.
    • Green: 6 tests passed.
  • python3 scripts/verify-local.py --affected origin/main --swift-changed origin/main passed 4/4 checks: swift-syntax, wire-app-sources, package-groups and feature-flags.

Not run locally, because the machine's disk was nearly full:

  • the native app build;
  • cmuxTests;
  • dogfooding.

The app-side wiring in these files has not been compiled locally; CI is the first compile:

  • TerminalLinkOpenCoordinator
  • CloudGuestURLService
  • CloudNotificationLocalDelivery
  • BrowserPanel

Localization audit: no new user-facing strings. The subtitle reuses cloudNotification.subtitle.machine.

Changelog

  • Changed: Links a remote machine opens without a click stay off this Mac's local network, and remote notification subtitles always name their machine.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Keeps links a remote machine opens without a click off this Mac's loopback and local network, routes private-address browser URLs to the machine that owns the browser, and stops remote terminal content from opening files on this Mac.

  • A remote-initiated url-open no longer hands a loopback, private, link-local, multicast, or local-name URL (localhost, *.local, *.internal, single-label names, legacy IPv4 spellings) to the default browser.
  • SSH loopback routes still open in the cmux browser, where they reach the remote machine, and never fall back to the system browser for a remote request. A refusal returns false, so the remote side prints the URL instead; public https:// opens and clicked links are unchanged.
  • When several SSH machines share 127.0.0.1, a browser URL now goes only to the machine that owns the browser, in both BrowserPanel.navigate and rebindCloudRouteIfNeeded.
  • Only a click in a terminal known to run on this Mac opens a local file. SSH, cloud, and unplaced terminals, plus any open a remote machine asks for, never fall back to this Mac's filesystem, so apps and scripts on this Mac don't launch from remote content. Scrollback exports Ghostty wrote on this Mac still open locally.
  • A remote notification's subtitle always names its machine; line breaks, control characters, and bidi overrides are removed, and the detail and machine name are length-capped.

Written for commit 2fda0cb. Summary will update on new commits.

Review in cubic

austinywang and others added 2 commits September 27, 2026 21:16
Adds failing regression tests for:
- PrivateNetworkHostPolicy: loopback, private, link-local and legacy IPv4 spellings
- RemoteLinkOpenPolicy: remote-initiated opens never hand private or loopback URLs to the default browser
- PrivateAddressRouteSelector: machines sharing 127.0.0.1 route only to the browser's owner
- RemoteMachineNotificationSubtitle: an explicit subtitle still names the machine

The sources are stubs that keep today's behavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A url-open that a remote machine starts without a click no longer hands a
  loopback, private, link-local or local-name URL to the default browser.
  SSH loopback routes stay in the cmux browser and are never opened in the
  system browser for a remote request; refusals return false so the remote
  side prints the URL instead.
- Browser private-address routing picks the machine that owns the browser
  when several SSH machines share 127.0.0.1, instead of the first match.
- A cloud notification's explicit subtitle still names its machine, with line
  breaks, control characters and bidi overrides removed and lengths capped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 5492c056-580a-4d05-b452-bacbe69acfe3

📥 Commits

Reviewing files that changed from the base of the PR and between ce5cb45 and 2fda0cb.

📒 Files selected for processing (22)
  • Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/PrivateAddressRouteSelector.swift
  • Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/PrivateNetworkHostPolicy.swift
  • Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/RemoteLinkDestinations.swift
  • Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/RemoteLinkOpenPolicy.swift
  • Packages/macOS/CmuxCore/Tests/CmuxCoreTests/PrivateAddressRouteSelectorTests.swift
  • Packages/macOS/CmuxCore/Tests/CmuxCoreTests/PrivateNetworkHostPolicyTests.swift
  • Packages/macOS/CmuxCore/Tests/CmuxCoreTests/RemoteLinkOpenPolicyTests.swift
  • Packages/macOS/CmuxNotifications/Sources/CmuxNotifications/RemoteMachineNotificationSubtitle.swift
  • Packages/macOS/CmuxNotifications/Tests/CmuxNotificationsTests/RemoteMachineNotificationSubtitleTests.swift
  • Sources/Cloud/CloudGuestURLService.swift
  • Sources/Cloud/CloudNotificationLocalDelivery.swift
  • Sources/DockSplitStore+TerminalLinkOpening.swift
  • Sources/GhosttyTerminalView.swift
  • Sources/Panels/BrowserPanel+CloudConnection.swift
  • Sources/Panels/BrowserPanel.swift
  • Sources/TerminalLinkOpenCoordinator.swift
  • Sources/TerminalLinkOpenRequest.swift
  • Sources/Workspace+RemoteTmuxControlTopology.swift
  • cmuxTests/RemoteTmuxNotificationLifecycleTests.swift
  • cmuxTests/SSHTuiMigrationTests.swift
  • cmuxTests/TerminalLinkLocationAndDockTests.swift
  • cmuxTests/TerminalLinkOpenCoordinatorTests.swift

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

CI failure attribution

CI passes on 2fda0cbe63 (run 36485546627 attempt 2).

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's.

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Review: security review of this one, since blocking remote-initiated local network access is the whole point. The IP handling is genuinely good, but I do not think the PR closes the hole it says it closes. Not merging, one blocker below.

Blocker: hostnames are never resolved, so the guard only covers IP literals.

RemoteLinkOpenPolicy.swift:26 decides with !remoteInitiated || !hosts.isNonPublic(host: url.host ?? ""). For a DNS name, isNonPublic is purely lexical: PrivateNetworkHostPolicy.swift:21-32 sends it to isLocalName, which is a static suffix list (.local, .internal, .lan, .home.arpa, .localdomain, .localhost, plus the loopback proxy alias). No lookup happens anywhere.

The function that was supposed to cover this is right there in the same file. requiresAddressLookup(host:) at line 53, and the type's own doc comment at line 8 says "a public DNS name can still resolve to a private address, which requiresAddressLookup(host:) reports". It has six unit tests. It is called from nothing:

$ grep -rn "requiresAddressLookup" --include=*.swift .
.../PrivateNetworkHostPolicy.swift:8:   (doc comment)
.../PrivateNetworkHostPolicy.swift:53:  public func requiresAddressLookup(host: String) -> Bool {
.../PrivateNetworkHostPolicyTests.swift:57-62 (tests only)

So the concrete bypass: a remote guest goes through CloudGuestURLService.deliver() (Sources/Cloud/CloudGuestURLService.swift:97-134), which sets isRemoteInitiated: true and hands the URL to NSWorkspace.shared.open. It asks for http://127.0.0.1.nip.io:8080/, or http://169.254.169.254.nip.io/latest/meta-data/. Last label is io, not numeric, so classify returns .name, isLocalName is false, isNonPublic is false, and it opens. The browser then resolves it to 127.0.0.1 or to the link-local metadata address and connects. nip.io is a public wildcard DNS service, so this needs no attacker DNS infrastructure at all, and ordinary attacker-controlled DNS gets the same result with rebinding on top.

The test suite does not catch it because there is no test for a DNS name that needs resolution to classify correctly. Every hostname test uses a name that is already on the suffix list.

Fix direction, your call which: resolve the host and re-check the resolved addresses before a remote-initiated open, or refuse remote-initiated opens for any hostname requiresAddressLookup returns true for. The second is cruder but fails closed and is a much smaller change. Either way requiresAddressLookup should stop being dead code, and a 127.0.0.1.nip.io case belongs in RemoteLinkOpenPolicyTests.

Note that resolve-then-check is still TOCTOU against real rebinding, since the browser resolves again independently. If you want that closed properly it has to be an allowlist, not a denylist.

What I checked that is fine

The IPv4 range table at PrivateNetworkHostPolicy.swift:102-126 is complete: 0/8, 10/8, 100.64/10 CGNAT, 127/8, 169.254/16, 172.16/12, 192.0.0/24, 192.0.2/24, 192.168/16, 198.18/15, 198.51.100/24, 203.0.113/24, 224/4, 240/4. IPv6 unwraps IPv4-mapped and NAT64, folds 6to4 back into the IPv4 check, and by only treating 2000::/3 as public it denies ::1, fe80::/10 and fc00::/7 by exclusion.

Literal parsing tricks do not get through. classify re-derives the host itself with inet_aton/inet_pton instead of trusting a pre-parsed value, and it mirrors the WHATWG ends-in-a-number rule, so decimal, octal, hex and short-form literals, trailing dots and IPv6 zone IDs all classify the same way the browser will actually resolve them. Userinfo tricks like evil.com@127.0.0.1 are fine because URL.host already splits the authority. Non-ASCII and unparseable hosts fail closed.

Only one production call site sets isRemoteInitiated: true. The local click path at Sources/GhosttyTerminalView.swift:3573-3587 leaves it false and is separately gated by consumeTerminalLinkOpenAuthorization(), and clickedPrivateLinkStillOpens confirms a user clicking their own localhost link is unaffected.

One thing to keep an eye on, not a blocker

TerminalLinkOpenCoordinator's local-file branches route bare paths and file:// URLs to fileOpen.open(url) without ever consulting request.isRemoteInitiated; they only check terminalLinkIsRemoteTerminal. That is safe today purely because CloudGuestURLRequest.init?(data:) rejects any non-http(s) scheme at decode time, which is a different file and a different PR's invariant. If another remote-initiated caller shows up, or that decoder is loosened, a remote peer would be able to open arbitrary local files and nothing in these files would stop it. Cheap to harden now: fail closed in TerminalLinkOpenCoordinator.open on isRemoteInitiated for any non-http(s) scheme.

Fixed: nothing, this needs an author decision on the resolution strategy.

Left: the hostname blocker above. Happy to re-review once it is wired up.

* Test that remote terminal links never open a file on this Mac

A path or file URL from an SSH, cloud or unplaced terminal, and any
open a remote machine asks for, must not resolve against this Mac's
filesystem. The policy stub keeps today's behavior so these fail.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Never open a file on this Mac from a remote terminal link

Only a click in a terminal known to run on this Mac opens a local
file. SSH, cloud and unplaced terminals, and any open a remote machine
asks for, resolve on the remote machine or are refused, so apps and
scripts on this Mac never launch from remote content. The Dock now
treats unplaced and remote-machine terminals as remote. Scrollback
exports Ghostty wrote on this Mac still open locally.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Dogfood build of dae877431e1c7d97d9153b7c50caa625ae5c7081

cmux DEV pr-15148-dae87743.app

The link opens this exact commit in the cmux dev menu bar app. The build starts on each push and the page waits until it is ready; a newer push replaces it. It signs in against production, so Cloud or backend changes still need a tagged build with a development backend.

Dogfood tours of 2fda0cbe

browser-notifications-tour at 2fda0cbe: not run

skipped: CI built this head on a runner pool whose products the UI test Macs cannot load, and media never compiles one; gh workflow run pr-media.yml -f pr=&lt;n&gt; -f allow_compile=true does

modifier-clicks-tour at 2fda0cbe: not run

skipped: CI built this head on a runner pool whose products the UI test Macs cannot load, and media never compiles one; gh workflow run pr-media.yml -f pr=&lt;n&gt; -f allow_compile=true does

Tours are picked by the paths globs in dogfood/scenarios/*.json; a Dogfood-tours: a, b line in the description picks them instead (none turns this off). Look at every frame before merging: a green tour only means no step failed.

austinywang and others added 2 commits September 28, 2026 14:07
Catch-up merge by scripts/ci/catch_up_pr.py (RFC #14631).
Merged by scripts/merge-main.sh: origin/main at ce5cb45.

Catch-up-previous-head: dae8774
Catch-up-base: ce5cb45
The remote tmux lifecycle test still expected a path clicked in a
projected SSH-tmux pane to open the same path on this Mac through the
external file opener. Remote terminal links now never open a local
file, so the open is refused and nothing reaches the file opener or
the system opener.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
austinywang added a commit that referenced this pull request Sep 28, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Automatic catch-up couldn't merge main (217ef1338a6f): Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/RemoteLinkOpenPolicy.swift (added on both sides; needs a person), Packages/macOS/CmuxCore/Tests/CmuxCoreTests/RemoteLinkOpenPolicyTests.swift (added on both sides; needs a person), Sources/GhosttyTerminalView.swift (both sides changed the same lines), Sources/TerminalLinkOpenRequest.swift (both sides changed the same lines), cmuxTests/TerminalLinkOpenCoordinatorTests.swift (both sides changed the same lines). Nothing was pushed; merge it by hand. A new push or /catch-up tries again.

Label no-auto-catch-up to opt out · Catch-up run

@austinywang

Copy link
Copy Markdown
Contributor Author

Closing as superseded by merged PR #15116, which consolidated this SSH/security/local-state hardening into main.

@austinywang austinywang closed this Oct 8, 2026
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.

2 participants