Drop poisoned shell-wrapper restore commands#6012
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds trust gating that rejects shell-wrapper and poisoned agent-hook resume captures, uses only trusted launch captures when reconstructing resume/fork/startup commands and working directories, repairs loaded snapshots to drop untrusted resume bindings, and updates command-parsing helpers and tests. ChangesTrust filtering for shell-wrapper removal in session restore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 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 |
Greptile SummaryThis PR fixes a session-restore regression introduced in 0.64.15 where persisted agent-hook bindings shaped like
Confidence Score: 5/5Safe to merge — the changes are purely defensive filters on incoming persisted data, with clear fallbacks and regression tests for all new paths. No correctness issues were found in the new filter logic. Defense is applied at two independent layers (file load and workspace restore), both correctly using value-type transformations on Sendable structs without actor isolation concerns. The repair is idempotent and the in-memory snapshot is always the cleaned version regardless of whether the re-save succeeds. The three new regression tests cover the most important failure scenarios described in the PR. No files require special attention. The single-&& limitation in commandStartIndexAfterCwdGuard (already flagged in a prior review thread) is the only known edge case, and it was a pre-existing assumption that this PR does not worsen. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SessionPersistenceStore.loadOutcome] --> B[Decode AppSessionSnapshot]
B --> C[SessionSnapshotRepairer.repair]
C --> D{For each panel}
D --> E{resumeBinding present?}
E -- yes --> F[trustedForSessionRestore\nisPoisonedAgentHookShellWrapperResume?]
F -- poisoned --> G[Set resumeBinding = nil\ndidRepair = true]
F -- clean --> H[Keep resumeBinding]
D --> I{agent present?}
I -- yes --> J[repairedForSessionRestore\ntrustedLaunchCommandForSessionRestore?]
J -- untrusted --> K[Drop launchCommand\nReplace workingDirectory with fallback\ndidRepair = true]
J -- trusted --> L[Keep launchCommand]
C --> M{didRepair?}
M -- yes --> N[save repaired snapshot]
M -- no --> O[Return as-is]
N --> O
O --> P[Workspace.restoreSessionSnapshot]
P --> Q[createPanel from snapshot]
Q --> R[resumeBinding?.trustedForSessionRestore\ndefense-in-depth filter]
R --> S[restorableAgent?.resumeStartupCommand\ncalls trustedLaunchCommandForSessionRestore]
Reviews (2): Last reviewed commit: "Repair persisted session restore snapsho..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| nonisolated private static func agentHookBindingLooksLikeShellWrapperResume( | ||
| _ binding: SurfaceResumeBindingSnapshot | ||
| ) -> Bool { | ||
| guard binding.isAgentHookBinding else { return false } | ||
| let words = surfaceResumeShellWords(in: binding.command) | ||
| let commandStart = surfaceResumeCommandStartIndexAfterCwdGuard(words) | ||
| guard commandStart + 1 < words.endIndex else { return false } | ||
| let executable = (words[commandStart].value as NSString).lastPathComponent.lowercased() | ||
| let shells: Set<String> = ["sh", "bash", "zsh", "dash", "fish", "csh", "tcsh", "ksh"] | ||
| guard shells.contains(executable) else { return false } | ||
| let resumeWord = words[commandStart + 1].value | ||
| return resumeWord == "resume" || resumeWord == "--resume" || resumeWord.hasPrefix("--resume=") | ||
| } |
There was a problem hiding this comment.
fish included in shell set may cause false positives
fish is listed alongside traditional POSIX shells, but fish resume <session> is a plausible legitimate invocation for a user whose agent-hook command genuinely calls a Fish script named resume. Because agentHookBindingLooksLikeShellWrapperResume only fires for isAgentHookBinding bindings, the blast radius is narrow, but fish does not appear in the original poisoned binding pattern described in the PR (bash resume …) and adding it increases the heuristic surface without a concrete motivating case.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } | ||
|
|
||
| final class SocketListenerAcceptPolicyTests: XCTestCase { | ||
| func testPersistedAgentSnapshotDropsShellWrapperLaunchCommandWhenRenderingResume() { | ||
| let snapshot = SessionRestorableAgentSnapshot( | ||
| kind: .codex, | ||
| sessionId: "codex-session-123", | ||
| workingDirectory: "/tmp/repo", | ||
| launchCommand: AgentLaunchCommandSnapshot( | ||
| launcher: "codex", | ||
| executablePath: "bash", | ||
| arguments: [ | ||
| "bash", | ||
| "-c", | ||
| "payload=\"$1\"; shift; \"$@\" <\"$payload\" &", | ||
| ], | ||
| workingDirectory: "/tmp/dispatch-shell", | ||
| environment: ["CODEX_HOME": "/tmp/codex"], | ||
| capturedAt: 123, | ||
| source: "process" | ||
| ) | ||
| ) | ||
|
|
||
| XCTAssertEqual( | ||
| snapshot.resumeCommand, | ||
| "{ cd -- '/tmp/repo' 2>/dev/null || [ ! -d '/tmp/repo' ]; } && 'codex' 'resume' 'codex-session-123'" | ||
| ) | ||
| } | ||
|
|
||
| func testClaudeResumeCommandRoutesThroughWrapperInsteadOfCapturedRealBinary() { | ||
| // The captured launch executable is the real claude binary | ||
| // (CMUX_AGENT_LAUNCH_EXECUTABLE). Resuming with it directly bypasses |
There was a problem hiding this comment.
testPersistedAgentSnapshotDropsShellWrapperLaunchCommandWhenRenderingResume exercises SessionRestorableAgentSnapshot.resumeCommand filtering, but it lives inside SocketListenerAcceptPolicyTests. The two other new tests correctly land in SessionPersistenceTests. Moving this test keeps the file's grouping consistent and makes it easier to run the persistence test suite in isolation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 `@Sources/RestorableAgentSession.swift`:
- Around line 722-738: In repairedForSessionRestore, when
trustedLaunchCommandForSessionRestore is nil we currently backfill
repaired.workingDirectory with fallbackWorkingDirectory unconditionally; change
this so we preserve a nil cwd for registrations marked cwd == .ignore by
checking registration?.cwd == .ignore (or the equivalent enum case) before
assigning fallbackWorkingDirectory — if registration?.cwd == .ignore leave
repaired.workingDirectory as nil; keep existing logic using
Self.normalizedWorkingDirectory(...) and the trustedLaunchCommand branch intact.
In `@Sources/SessionPersistence.swift`:
- Around line 700-708: The helper commandStartIndexAfterCwdGuard currently only
recognizes a cwd guard terminated by "&&" and returns tokens.startIndex for the
other in-repo form; update it to also detect the "|| exit $?" guard (or the full
known prefix) and treat it the same as the "&&" case. Concretely, in
commandStartIndexAfterCwdGuard(_:), after confirming tokens.first is "{" or
"cd", check for a terminating operator of either "&&" or "||" (optionally
followed by "exit" and "$?") and when found return tokens.index(after:
thatOperatorIndex) so the resume command start is computed the same for both
guard spellings.
🪄 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: f109ed32-9a97-42ff-96f6-1a3c7be9e3c8
📒 Files selected for processing (4)
Sources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
| func repairedForSessionRestore(fallbackWorkingDirectory: String?) -> SessionRestorableAgentSnapshot { | ||
| let trustedLaunchCommand = trustedLaunchCommandForSessionRestore | ||
| var repaired = self | ||
| repaired.launchCommand = trustedLaunchCommand | ||
|
|
||
| let fallbackWorkingDirectory = Self.normalizedWorkingDirectory(fallbackWorkingDirectory) | ||
| if trustedLaunchCommand == nil { | ||
| if repaired.workingDirectory == nil | ||
| || Self.normalizedWorkingDirectory(repaired.workingDirectory) | ||
| == Self.normalizedWorkingDirectory(launchCommand?.workingDirectory) { | ||
| repaired.workingDirectory = fallbackWorkingDirectory | ||
| } | ||
| } else if repaired.workingDirectory == nil { | ||
| repaired.workingDirectory = Self.normalizedWorkingDirectory( | ||
| trustedLaunchCommand?.workingDirectory | ||
| ) ?? fallbackWorkingDirectory | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve .ignore by keeping the repaired snapshot cwd nil.
When trustedLaunchCommandForSessionRestore is nil, this repair path backfills workingDirectory from fallbackWorkingDirectory without checking registration?.cwd == .ignore. That reintroduces a saved cwd into snapshots that are supposed to restore from the caller’s current directory, and downstream restore code in this file explicitly relies on workingDirectory being nil for .ignore agents.
Possible fix
let trustedLaunchCommand = trustedLaunchCommandForSessionRestore
var repaired = self
repaired.launchCommand = trustedLaunchCommand
+
+ if registration?.cwd == .ignore {
+ repaired.workingDirectory = nil
+ return repaired
+ }
let fallbackWorkingDirectory = Self.normalizedWorkingDirectory(fallbackWorkingDirectory)Based on learnings: “Registrations with cwd: .ignore set resumeWorkingDirectory to nil, suppressing both the cwd guard in the resume command and the terminal working directory at placement time.”
🤖 Prompt for 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.
In `@Sources/RestorableAgentSession.swift` around lines 722 - 738, In
repairedForSessionRestore, when trustedLaunchCommandForSessionRestore is nil we
currently backfill repaired.workingDirectory with fallbackWorkingDirectory
unconditionally; change this so we preserve a nil cwd for registrations marked
cwd == .ignore by checking registration?.cwd == .ignore (or the equivalent enum
case) before assigning fallbackWorkingDirectory — if registration?.cwd ==
.ignore leave repaired.workingDirectory as nil; keep existing logic using
Self.normalizedWorkingDirectory(...) and the trustedLaunchCommand branch intact.
Source: Learnings
| static func commandStartIndexAfterCwdGuard(_ tokens: [String]) -> Int { | ||
| guard let first = tokens.first, | ||
| first == "{" || first == "cd" else { | ||
| return tokens.startIndex | ||
| } | ||
| guard let andIndex = tokens.firstIndex(of: "&&") else { | ||
| return tokens.startIndex | ||
| } | ||
| return tokens.index(after: andIndex) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle the repo’s || exit $? cwd guard here too.
This helper only skips guards terminated by &&. Sources/SessionRestoredTerminalCommandStore.swift already emits the other in-repo form, { cd -- <cwd> ...; } || exit $? before exec ..., so callers of commandStartIndexAfterCwdGuard will fall back to tokens.startIndex for those commands and parse { as the executable instead of the real resume command. Pattern-match both guard spellings (or the full known guard prefix) here so the canonical start index stays correct everywhere. Based on learnings from the supplied cross-file snippet: Sources/SessionRestoredTerminalCommandStore.swift:3-44 uses a || exit $? cwd guard that this helper does not currently recognize.
🤖 Prompt for 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.
In `@Sources/SessionPersistence.swift` around lines 700 - 708, The helper
commandStartIndexAfterCwdGuard currently only recognizes a cwd guard terminated
by "&&" and returns tokens.startIndex for the other in-repo form; update it to
also detect the "|| exit $?" guard (or the full known prefix) and treat it the
same as the "&&" case. Concretely, in commandStartIndexAfterCwdGuard(_:), after
confirming tokens.first is "{" or "cd", check for a terminating operator of
either "&&" or "||" (optionally followed by "exit" and "$?") and when found
return tokens.index(after: thatOperatorIndex) so the resume command start is
computed the same for both guard spellings.
1 participant
Summary
Fixes a real session restoration failure seen after upgrading from 0.64.14 to 0.64.15: persisted agent-hook resume bindings shaped like
bash resume <session> -c ...could still be trusted before the restored agent snapshot, producingbash: resume: No such file or directory.This PR now repairs persisted session snapshots at load time for all users. Old snapshots are normalized before restore sees them, poisoned shell-wrapper bindings are dropped, untrusted saved agent launch captures are stripped, and recovered agent working directories fall back to the terminal/panel/workspace directory. When a repair happens, the cleaned snapshot is saved back to disk so the upgrade is durable.
Verification
xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-rsres-green -only-testing:cmuxTests/SessionPersistenceTests/testLoadRepairsAndPersistsPoisonedAgentHookShellWrapperResumeBinding -only-testing:cmuxTests/SessionPersistenceTests/testLoadRepairsWrongForkAgentLaunchCommandAndRecoversWorkingDirectory -only-testing:cmuxTests/SessionPersistenceTests/testRestoreDropsPoisonedAgentHookShellWrapperResumeBindingAndUsesAgentSnapshot -only-testing:cmuxTests/SessionPersistenceTests/testRestoreDropsPoisonedAgentHookShellWrapperResumeBindingWithoutAgentSnapshot -only-testing:cmuxTests/SocketListenerAcceptPolicyTests/testPersistedAgentSnapshotDropsShellWrapperLaunchCommandWhenRenderingResume./scripts/reload-cloud.sh --tag rsfixhttp://127.0.0.1:17320/rsfix, confirmed thersfixdebug socket responds withlist-workspaces, and visually confirmed the app rendered.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Tests