Repository navigation
Consolidate SSH security, shim hardening, and restored terminal replay fixes - #15116
Conversation
…ath-shim # Conflicts: # CLI/CMUXCLI+Events.swift # Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/EventStreamFailure.swift # Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/EventStreamFailureTests.swift
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cmux's shared OpenSSH control socket lived at /tmp/cmux-ssh-<uid>-%C. Move it to ~/.cmux/ssh, which cmux creates with mode 0700 and checks is owned by the user and not writable by anyone else, along with every directory above it. When no such directory is available, or the home path is too long for a socket, cmux adds no connection-sharing defaults. A ControlPath pointing at an older cmux's /tmp socket is rewritten to the private one, or to none, even with ControlMaster=no. Those /tmp paths are no longer treated as cmux-owned, so cmux never checks, reuses or removes them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThis pull request updates SSH and Unix-socket transport, file access, browser proxy authentication, relay policies, terminal links and clipboard reads, shell integration, and cloud replay handling. It also adds related validation and tests. ChangesRemote access and transport
Proxy and relay behavior
Terminal and shell behavior
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to Fix wait-for channel isolation and log-file protections, then replace the bounded test polling before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The changes add protections across several remote-access paths, but one local-file opening decision depends on an unverified guarantee about which terminal actions remote output can produce. No local-file bypass has been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (8 errors, 1 warning)
✅ Passed checks (16 passed)
Full details: Cmux Swift Actor IsolationExplanation The production diff adds Resolution Declare the new logger as Full details: Cmux Swift Blocking RuntimeExplanation The diff adds production timing-based synchronization. Resolution Replace the daemon socket retry sleep with event-driven directory/socket readiness plus a cancellation-aware deadline. Replace the periodic Full details: Cmux Swift Package BoundariesExplanation The new Resolution Extract Full details: Cmux Swift LoggingExplanation The new production log declaration in Resolution Declare the logger as Full details: Cmux User-Facing Error PrivacyExplanation The PR violates the user-facing error privacy rule by exposing a user ID (UID) in an error message visible to end users. Violation identified: In Resolution Remove the UID from the error message in Full details: Cmux Full InternationalizationExplanation The PR adds new user-facing text without complete internationalization. The five new Resolution Add translated catalog entries for all 20 supported locales for Full details: Cmux Architecture RethinkExplanation The replay repair introduces a production timing workaround. Resolution Make replay application and resize completion one explicit state transition owned by the terminal/session coordinator. Have Full details: Cmux No Test Or Debug Seam In Production SourceExplanation The PR adds test-only seams in production Swift source. Resolution Remove the test-only ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
|
All contributors have signed the CLA ✍️ ✅ |
The baked-VM daemon forward carries daemon RPC without a credential of its own. The new tests drive start() through a fake ssh that honors -L and expect the local end to be a Unix socket in a directory only the current user can open, removed on stop and when ssh exits. The socket-forward path now honors the transport executable test seam. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The loopback SOCKS5 and HTTP CONNECT listener that serves the ssh workspace browser accepts any local client. Add a per-tunnel BrowserProxyCredential and an optional session seam, then test that a handshake without it is refused before a daemon stream opens and that the credentialed handshake still relays bytes. The Network.framework cases drive URLSession through ProxyConfiguration, the stack WKWebView uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A clipboard read the terminal program starts should ask in a window sheet whatever the unsafe-paste setting, and reject when there is no window. It should get the pasteboard's plain text only, while a native paste keeps files and images. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI failure attributionCI failed on
Matched log linesNot re-run automatically: Written by |
The baked-VM ssh transport forwarded the daemon socket to a loopback TCP port. Daemon RPC on that path carries no credential of its own, so the local end now lives in a fresh mkdtemp directory (0700) under the per-user temporary directory, falling back to /private/tmp. ssh binds it with StreamLocalBindMask=0177 and StreamLocalBindUnlink=yes ahead of any configured options. The client accepts the socket only when its peer runs as the current user, and removes the directory on stop or when ssh exits. The remote side of the forward is unchanged (same direct-streamlocal channel to /run/cmuxd-remote.sock), so deployed VMs need no update. ssh creates the socket only after it authenticates, so the connect now retries a missing or not-yet-listening socket while ssh runs, for up to five seconds after the startup grace period. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The loopback SOCKS5 and HTTP CONNECT listener for the ssh workspace browser now requires the credential its tunnel minted. SOCKS5 only accepts username/password authentication (RFC 1929), CONNECT needs a matching Proxy-Authorization: Basic header, and both compare in constant time before any daemon stream opens. A refused client gets 05 FF, 01 01 or 407 and the connection closes. The broker mints a fresh credential per tunnel start and hands it to the tunnel and the BrowserProxyEndpoint. The browser applies it to its WKWebsiteDataStore SOCKS5 and CONNECT proxy configurations, and the favicon session moves from the legacy proxy dictionary, which cannot carry a credential, to a credentialed ProxyConfiguration. Descriptions of the credential and endpoint are redacted, and workspace.remote.status keeps reporting the endpoint without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A clipboard read the terminal program starts (for example OSC 52 under Ghostty's default clipboard-read = ask) now always asks in a window sheet, and is rejected when there is no window, instead of being approved unasked. The sheet uses clipboard access wording rather than the paste wording. Such a read also takes only the pasteboard's plain-text flavor. It never prepares, saves or uploads Finder files or images, for any pane kind, and never becomes input to a remote tmux mirror pane. Native paste gestures keep the full pasteboard, including image and file upload. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and remote status details Remote relay callers can still reach surface.resume.set/get/clear, request a notification reply field, and read the full remote status payload. These tests fail until the relay schema and coordinator narrow those paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arding Covers the stale-socket preflight, the CmuxCore batch builders and the coordinator's exec, bootstrap, scp and reverse-relay argv: each must put the destination after `--`, and batch runs must turn off agent, X11 and port forwarding ahead of the configured options. Also covers shell quoting of values that end in, or contain, a line terminator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…her user
Adds the shared checks the fix will use (a socket peer-uid check, an
append opener that refuses symlinks, hard links and foreign owners, and
an owned marker-file reader) with their own tests, injectable seams at
each client connect and at the debug log path, and failing regression
tests for the behavior the fix must provide:
- the control socket probe sends nothing to a server run by another user;
- the remote relay forwards nothing to such a local socket;
- the cloud CLI bridge writes no password or request to it;
- the debug event log does not write through a symlink or hard link and
creates its file with mode 0600;
- the zsh PR watcher writes no file at a shared /tmp name and does not
use a state directory that was replaced with a symlink.
Red, before the fix:
swift test --disable-index-store --package-path Packages/macOS/CmuxControlSocket \
--filter SocketTransportProbeCommandTests
-> 7 tests, 1 failed (probeCommandSendsNothingToAServerRunningAsAnotherUser:
response "PONG", commandReceived true)
swift test --disable-index-store --package-path Packages/macOS/CmuxRemoteWorkspace \
--filter "RemoteCLIRelayServerTests|RemoteDaemonProxyTunnelCloudCLITests"
-> 19 tests, 2 failed (relay wrote 177 bytes; bridge threw nothing
and wrote 159 bytes)
swift test --disable-index-store --package-path Packages/macOS/CMUXDebugLog \
--filter DebugEventLogFileSafetyTests
-> 3 tests, 3 failed (symlink and hard-link targets appended to;
new file mode 0644)
python3 tests/test_shell_pr_watch_private_state.py
-> 4 tests, 3 failed (force signal, cache and debug log wrote through
/tmp symlinks)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
…runs Shell-word quoting now uses one byte-comparing helper (POSIXShellWord) instead of per-file `^…$` regexes, which NSString's ICU matcher lets match before a trailing line terminator. Validation regexes that gate ssh, scp and shell arguments anchor with `\z`. Background ssh and scp argv built by cmux put `--` ahead of the destination. The interactive ssh command cannot, so its input paths (CLI VM usernames, workspace context, restored snapshots) reject a destination that starts with `-`. Batch runs that never become a ControlMaster turn off agent and X11 forwarding, and drop configured port forwards where the run has no forward of its own. The daemon carrier keeps forwarding because persistent remote shells inherit its SSH_AUTH_SOCK and DISPLAY, and runs that can create the shared master keep the configured settings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CLI, the control-socket probe, the remote relay and the cloud CLI bridge now read the listening process's user from the connected socket and refuse to write anything to a socket served by another user. The CLI also lstats the socket path so a link someone else planted is never followed. The debug event log opens its /tmp file with O_NOFOLLOW and only appends to a regular, single-link file this user owns. The zsh PR watcher regression test from the previous commit is dropped from this change and left as a follow-up. The probe test's server now answers only when it received a command, so a refusing probe no longer raises SIGPIPE in the test process. Green: - swift test --disable-index-store --package-path Packages/macOS/CmuxControlSocket --filter SocketTransportProbeCommandTests: 7 tests passed - swift test --disable-index-store --package-path Packages/macOS/CmuxRemoteWorkspace --filter "RemoteCLIRelayServerTests|RemoteDaemonProxyTunnelCloudCLITests": 19 tests passed - swift test --disable-index-store --package-path Packages/macOS/CMUXDebugLog --filter DebugEventLogFileSafetyTests: 3 tests, 0 failures Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… relay The relay no longer forwards surface.resume.get/set/clear, so a remote session cannot read, write or clear a Mac-side resume command, and command-bearing params are denied on every method with no exceptions. A relayed notification is delivered with the relay origin, no reply affordance and the remote destination in its title; reply_shape is out of the relay schema. workspace.remote.status and the terminal_session_* lifecycle replies carry only enabled, state and connected in remote for a relay caller. 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>
…socket Move the CLI's socket discovery probe into CmuxFoundation as UnixSocketConnectProbe and route workspace.remote.configure's local_socket_path through ControlWorkspaceRemoteLocalSocketPath, both unchanged in behavior. The new tests expect the probe to refuse a listener running as another user and the forwarding path to be the app's own control socket; they fail here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge receipt for
Labeled |
github-actions
Bot
added
the
merged-unverified
c4900c2 docs: CI routing is minis first; never send a lane straight to Blacksmith (manaflow-ai#15563) 7538f00 ci: keep the kept build when it and the local seed both recompile the app (manaflow-ai#15552) 217ef13 ci: fingerprint package interfaces and shadow the app-compile skip (manaflow-ai#15551) ad7906a Consolidate SSH security, shim hardening, and restored terminal replay fixes (manaflow-ai#15116) ff12362 Fix pane drop target rendering in the wrong pane (manaflow-ai#15550) ed49329 ci: report healthy Aqua console sessions accurately (manaflow-ai#15504) # Conflicts: # .github/workflows/ci-guards.yml # .github/workflows/ci-macos.yml
With cmuxTests compiling again, the changed-suites job runs tests that could not run on main since #15116 and #15550: four remote tmux mirror topology tests see one pane too many and two browser drop-preview tests find no overlay. Disable them against #15564 so ci-status can go green. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
main no longer compiles after this merge@austinywang: after Evidence: https://github.com/manaflow-ai/cmux/actions/runs/36566630813/job/109400047856 Nothing blocks merging meanwhile. A fix-forward (or, failing that, a revert) is attempted automatically unless an open pull request already fixes this. main_compile_attribution.py: post-merge, nothing here gates a merge. |
* fix(tests): compile cmuxTests again after #15116 and #15550 main's app-host test target no longer compiles (every PR's compile admission fails, e.g. job 109346405399): - #15116 added initialTerminalIsRemote to TabManager's makeWorkspaceForCreation and addWorkspaceIfActive; three test overrides still had the old signature (does not override). - #15116's RemoteTmux harness captured self in a closure before every stored property was set, which fails definite initialization. - #15550's test passed a zero-argument closure as frameForZone, which takes the zone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test: disable six app-host tests that fail on main (#15564) With cmuxTests compiling again, the changed-suites job runs tests that could not run on main since #15116 and #15550: four remote tmux mirror topology tests see one pane too many and two browser drop-preview tests find no overlay. Disable them against #15564 so ci-status can go green. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
6093e59 test(cloud-vm): cover missing attach address b639b65 fix(iroh-v2): omit bearer device attribution c9a74d6 ci: E2E picker routes owned labels by the online runners, not CI_OWNED_POOL_SLOTS (manaflow-ai#15582) 194ae87 fix(tests): compile cmuxTests again after manaflow-ai#15116 and manaflow-ai#15550 (manaflow-ai#15561) 8bfc872 ci: live runners decide root, gui and side routing; CI_OWNED_POOL_SLOTS is the fallback only (manaflow-ai#15572) 1516426 ci: route hardcoded Blacksmith labels through the runner variables (manaflow-ai#15592) # Conflicts: # .github/workflows/cmux-cloud-cli.yml # .github/workflows/cmux-tui.yml # .github/workflows/ios-e2e.yml
The relay's local socket and the policy fixture's listener now set SO_NOSIGPIPE, so a write to a socket that was shut down or whose peer hung up fails with EPIPE instead of killing a host process that hasn't ignored SIGPIPE. The app only ignores it through Ghostty's startup, so package tests and any other embedder had no protection. Darwin refuses the option once the peer is gone, so both set it before connecting or listening. Accepted sockets inherit it from the listener, as in the two sibling fixtures #15116 fixed. On a socket-creation failure the relay closes the descriptor and reports the existing "failed to create local relay socket" error, so the app's behavior is unchanged. Refs #15488 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`wait-for -L`/`-U` keep their lock at /tmp/cmux-wait-for-<name>.lock, the shared world-writable pattern manaflow-ai#15116 removed for signals: another user can pre-create the lock to block a channel forever, or plant a symlink at it. These cases pin the lock beside the signals in the private per-user directory: exclusive until unlocked, a blocked locker wakes on unlock, no symlink is followed, distinct names get distinct files, and a group-writable directory is refused. Red: the fixture does not compile yet, because TmuxWaitForSignal has no lock API (lockPath, lock, unlock). This is a compile-level red, not a behavioral one: the CLI's lock path needs a live socket to reach. python3 tests/test_cli_tmux_wait_for_private_dir.py _Claude Code on behalf of Adriano Machado (@ammachado)_ _This was generated by an AI agent and may contain inaccuracies. Please verify before relying on it._
…the clipboard Since manaflow-ai#15116, cmux drops every OSC 52 clipboard write from a remote terminal: cmux ssh mirror panes and cmux ssh-tmux mirror panes. That is the right default, but it also removes the only copy path agent TUIs have on a remote host. OpenCode, Codex and Claude Code capture the mouse, select text themselves, and copy it with OSC 52; without a display server on the remote they have nothing else to write to. Before 0.65.0 these copies reached the Mac clipboard (manaflow-ai#18324, and the OSC 52 half of manaflow-ai#17472). Add terminal.trustedClipboardWriteHosts, a list of ssh_config-style host globs, empty by default. A remote surface records the SSH hosts its output comes from (the destination and, for a brokered connection, the HostName it reaches, matched the way terminal.uploadCommands.hostPattern is). The Ghostty write_clipboard_cb admits an OSC 52 write from such a surface when one of its hosts matches a trusted pattern. Local terminals and Cloud writer shims keep their existing behavior, and OSC 52 clipboard reads stay denied. The gate reads the setting on every write, so a cmux.json edit applies on the next copy without restarting. Remote exec PTYs that still run ssh inside a local Ghostty PTY carry no host and stay blocked.
1 participant
Summary
This PR consolidates cmux’s SSH, relay, local-state, terminal-restore, and shim hardening into one reviewable change. SSH control masters now live in a private per-user directory and are separated whenever route or host-key policy differs. Remote exec and restored terminal surfaces retain their remote origin before OSC 52 callbacks can run, so remote output cannot overwrite the local clipboard.
It also hardens daemon and relay authorization, forwarding defaults, proxy credentials, local socket peer checks, shell shims, debug logs, wait-for state, remote links, browser proxy access, and Cloud/SSH replay restoration. The latest main merge is included through
79f62d7026a.Security fixes
ControlMaster=noandControlPath=noneacross repeated option merges; host-key and handshake policy changes cannot reuse a weaker%Cmaster.isRemoteTerminalbefore runtime callbacks are active.Validation
scripts/verify-local.py --allpassed 15/15 checks.The combined PR’s two macOS lanes are still pending; I’m waiting for those required checks to finish before calling the PR fully green.
Changelog