Repository navigation
cmux ssh: keep resume bindings, notification replies and local status off the relay - #15145
austinywang wants to merge 4 commits into
Conversation
… 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>
… 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>
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (17)
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 ✍️ ✅ |
CI failure attributionCI passes on Written by |
Dogfood tours of
|
Catch-up merge by scripts/ci/catch_up_pr.py (RFC #14631). Merged by scripts/merge-main.sh: origin/main at f7ee3bc. Resolved conflicts: - Resources/Localizable.xcstrings: xcstrings key-level union Catch-up-previous-head: f206129 Catch-up-base: f7ee3bc Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # Resources/Localizable.xcstrings
This branch has not been deployed
1 participant
Summary
The
cmux sshCLI relay forwarded more than any remote flow needs. After this change, a process on the remote host can no longer:surface.resume.setbindings and redactedresume_bindingfromsurface.list. Still,surface.resume.set/get/clearwere in the relay schema, andsetcarried the only exemption from the command-parameter deny. A relayedgetreturned the full binding, command included, and a relayedcleardeleted it. All three are out of the schema now, and command-bearing params (initial_command,command,tmux_start_command,pane_start_command) are denied on every method with no exceptions.ControlSurfaceResumeTarget.registeredBindingstill returnsnilfor relay callers, as a second layer.notification.create_for_targetlosesreply_shapefrom the relay schema. The coordinator also ignores a relayedreply_shape. The app delivers the notification with the.sshRelay(ownerWorkspaceID:)origin, which clamps click, agent context, sound override and hooks, and it prefixes the title with the remote destination (for exampledev-box: Build finished) so a remote can't pass its notification off as a local one.workspace.remote.statusand theterminal_session_launching/connected/endreplies now carry onlyenabled,stateandconnectedinremote. The destination, proxy, local ports and daemon details stay on the Mac. Nothing on the remote side reads the fuller payload. Local callers get the full payload, same as before.The remote CLI (
daemon/remote) never sendsreply_shapeorsurface.resume.*. On the Mac,cmux notifyaddsreply_shapeonly with--reply. So normal remotenotify, status and session flows are unchanged.Relay authorization analysis
surface.resume.set) and the notification reply path, which could have typed input into a local pane.Squatting relay listener
We also looked at whether a process on the remote host could bind the relay port before
cmux sshdoes and impersonate the Mac. This PR changes nothing here:A squatter running as the same uid could already read the token file. A proof from the server would need a protocol change that deployed remotes wouldn't enforce.
Overlap with open PRs
#14929 and #14915 also edit
RemoteRelayRoutingSchema,RemoteRelayAuthorizationPolicyanddaemon/remote/README.md, so whichever lands second will need a textual merge. They don't conflict in behavior:agent.hook.enqueueand doesn't forwardsurface.resume.*through the relay.Testing
The failing tests were committed first, in cee4bef. The fix is in 459905c.
swift test --package-path Packages/macOS/CmuxRemoteWorkspace --filter RemoteRelayNarrowingswift test --package-path Packages/macOS/CmuxControlSocket --filter RemoteRelayNarrowingOn 459905c, these broader package runs also passed:
swift test --package-path Packages/macOS/CmuxRemoteWorkspace: 160 tests in 26 suites.swift test --package-path Packages/macOS/CmuxControlSocket --filter "Remote|Notification|Resume|Workspace": 117 tests in 14 suites.python3 scripts/verify-local.py --affected origin/main --swift-changed origin/mainpassed all 15 selected checks: swift-syntax, xcstrings, localization, the project, test-wiring and package-group checks, launch-policy, remote-tmux-waits and feature-flags.Not run locally, because the build disk was nearly full: the app build,
cmuxTestsand a tagged-build dogfood. That covers the app-side changes inTerminalController+ControlNotificationContext.swiftandTerminalNotificationLiveRetargetDelivery.swift. It also covers the updatedRemoteResumeBindingTestsKiro case, which now expectsremote_relay_method_denied. CI covers the app build and app tests. Until those pass, the app-side changes are unverified.Localization audit:
notification.remoteRelay.hostFallback("Remote host") has all 9 macOS locales.notification.remoteRelay.title(%1$@: %2$@) has a format omission record inscripts/localization-allowed-omissions.json.python3 scripts/localization_catalog.py checkreported 0 parity errors../scripts/localize-changesalso reported 0 parity errors. It raised one human-attention flag onControlCommandCoordinator+Notification.swift. That's a false positive from an existing doc comment that mentionsString(localized:); the file has no new user-facing strings.Changelog
Changed: Remote SSH sessions can no longer read or change Mac-side resume commands, request replies to their notifications, or read local connection details through the cmux CLI relay; their notifications now show the remote host in the title
Checklist
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Keeps resume bindings, notification reply fields, and local connection details off the
cmux sshrelay, so a remote host can no longer read or change Mac-side resume commands, request a reply through a notification, or read local ports, proxy, and daemon details.surface.resume.set/get/clearfrom the relay schema; command-bearing params are now denied on every method with no exceptions.notification.create_for_targetignorereply_shape; the app delivers such notifications with the relay origin, no reply affordance, and the remote destination in the title.workspace.remote.statusand theterminal_session_*lifecycle replies carry onlyenabled,state, andconnected; local callers keep the full payload.The new relay-narrowing tests pass after the fix; the recent merges from
mainonly reconciledLocalizable.xcstrings. #14929 and #14915 also edit the relay schema and policy files, so whichever lands second will need a textual merge.Written for commit 7f7004b. Summary will update on new commits.