Repository navigation
fix(bg): revalidate process identity before signals - #1937
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughChangesBackground session process safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
3. Findings
High
H1 — Live-but-unreadable session becomes permanently un-killable (availability regression).
killBackgroundSession (bg.ts:859-867) calls authorizeBackgroundSessionSignal, which throws on unreadable/mismatch (bg.ts:845-850). Because the throw precedes markKilled, a session whose live identity cannot be confirmed is never killed and the operator is told to "re-run openclaude ps and retry." But the identity can stay unreadable indefinitely:
getProcessCommand(genericProcessUtils.ts:94-107) collapses every error tonull;verifyBackgroundSessionProcessIdentitythen mapscommand == nullto'unreadable'(bgRegistry.ts:611-613). Any failure to read the command line — Windowspowershell.exe Get-CimInstancedenied/missing,psdenied (e.g., another user's process or a restricted container/PID namespace), or proc momentarily unavailable — yieldsunreadable.getBackgroundSessionProcessLiveness(bgRegistry.ts:630-636) classifies any non-ESRCHprocess.kill(pid,0)error as'unreadable', which also refuses the kill.
The prior killHandler used isProcessRunning(pid) (old genericProcessUtils.ts:20-28), which caught all errors as "not running" and killed the process. The new code is stricter and can leave a legitimately-owned, still-alive session un-killable. This is most likely on Windows (powershell/elevation mismatch) but identical failure modes exist on Unix (permission-denied ps, PID namespaces).
Recommended fix: distinguish "process exists but its command line is unreadable" from identity mismatch. For a live PID (liveness === 'alive') whose command line merely can't be introspected, prefer a best-effort kill (possibly with a warning) rather than a hard refuse — keep 'mismatch' as the strict fail-closed case. At minimum, allow signaling when liveness is alive even if the command is unreadable.
Medium
M1 — Injected identity options are not wrapped, bypassing fail-closed and leaking raw text.
getBackgroundSessionProcessLiveness wraps signalProcess in try/catch (bgRegistry.ts:631-636), but three equivalent injection points are not wrapped:
options.verifySessionIdentityinverifySelectedBackgroundSessionIdentity(bg.ts:824-826) — a throwing verifier propagates raw, never becoming the controlledunverifiedProcessError.options.getProcessCommandinverifyBackgroundSessionProcessIdentity(bgRegistry.ts:607-608) — called bare (no try/catch), unlike the wrappedsignalProcess.options.isProcessAliveingetBackgroundSessionProcessLiveness(bgRegistry.ts:625-627) — called bare.
If any injected probe throws, the 'unreadable' → refuse classification is bypassed and the raw (potentially sensitive) error text escapes. The production default path is safe (the default getProcessCommand/isProcessRunning are internally guarded), but the public option surface advertises exactly this misuse and contradicts the PR's stated "fail closed on unreadable" guarantee (wave1-failure A/B/C, wave2-contract F3).
Recommended fix: wrap verifySessionIdentity, readCommand, and options.isProcessAlive call sites in try/catch that map exceptions to 'unreadable' (mirroring the signalProcess guard).
M2 — Identity is argv-only, a weak/forgeable authenticator.
commandLineMatchesBackgroundSession (bgRegistry.ts:578-589) is the sole discriminator and matches only contiguous whole-token runs of the stored session.command (or the session UUID). This correctly defeats substring and interleaved-token collisions (#1770), but:
- A PID reused by any process with identical argv (another openclaude session launched with the same prompt/flags, or an attacker re-exec'ing the same image) yields a confident
matches→ a wrong-but-"justified" kill (wave1-skepticF2). - The contiguous-run matcher returns true for a stored argv that is a prefix of an unrelated line (
bgRegistry.ts:565-575); a crafted leading argv could in theory produce a falsematches(wave1-skepticF5, Low likelihood).
The PR's own description concedes the residual kernel-level TOCTOU (verification and signaling are two separate syscalls). argv is attacker-controllable and cannot be the only anchor.
Recommended fix (defense in depth, not blocking): capture a harder-to-spoof property at spawn time (process start time from /proc/<pid>/stat, or a per-session nonce on argv) and additionally target the process group (setpgid + kill(-pgid, sig)) so a reused PID in a different group is not signalled.
M3 — Test coverage/robustness gaps on the security-critical path.
killHandler(the actual CLI entry point,bg.ts:917-931) is exercised only indirectly via the lower-levelkillBackgroundSession; there is no end-to-endkillHandlertest (wave2-claimsF1).- The real errno classification (
ESRCH→not-running,EPERM→unreadable) is only tested via injected mock errors, never via a realNodeJS.ErrnoException(wave2-claimsF2). bgRegistry.test.ts:993-1003assertsaliveChecks === 2, coupling the test to the double-liveness-probe implementation detail; a legitimate single-probe refactor would keep behavior but break the count (wave2-claimsF3, Medium).- Several refusal assertions are substring-only (
toThrow('refused to signal an unverified process')) and one usescalls.slice(0,2)instead of an exact-array check (bg.test.ts:552,672,693,722,817;wave2-claimsF4/F5). None let a security bug through given the suite as a whole, but they reduce regression sensitivity. - No
win32test asserts missing-pid →'not-running'(see H1/M1 cross-platform risk).
Low
L1 — Latent infinite-recursion footgun (currently unreachable). The isProcessAlive built in terminateBackgroundSessionProcessTree (bg.ts:806) is pid => getLiveness(pid) !== 'not-running', and getLiveness → getBackgroundSessionProcessLiveness(pid, options), which consults options.isProcessAlive if present (bgRegistry.ts:625-627). Today the verification closures capture the original options (without the recursive closure), so it is unreachable — but a future refactor that spreads the augmented options into verification would deadlock the CLI (wave2-failure F1). Fix: have getBackgroundSessionProcessLiveness ignore options.isProcessAlive, or assert it is never forwarded.
L2 — command == null vs ''. bgRegistry.ts:611 treats only null/undefined as unreadable. If an injected getProcessCommand returned '', commandLineMatchesBackgroundSession('', session) returns false → mismatch (not unreadable). The production getProcessCommand can't return '' (result ? result.trim() : null), so this is unreachable in practice; hardening: if (command == null || command === '') (wave2-skeptic F5).
L3 — mismatch conflated into stale status. refreshBackgroundSessionStatuses maps both not-running and mismatch to stale (bgRegistry.ts:490). Because every actual signal goes through a fresh authorizeBackgroundSessionSignal that throws on mismatch, this is display-only and safe, but a live-but-wrong-PID process shows as stale rather than something more diagnostic (wave1-state F3).
L4 — verifyBeforeSignal declared return type omits that it throws. The signature Promise<'matches' | 'not-running'> (bg.ts:728-732) doesn't express the mismatch/unreadable → throw path. TypeScript-legal and all callers handle throws, but the type is imprecise (wave2-failure F2, info).
L5 — Double liveness probe = 2 kill(pid,0) syscalls per identity check per running session on every status refresh (bgRegistry.ts:604 + :609). Intended and fail-safe (it is what makes natural-exit-during-lookup return not-running), but doubles syscall overhead; acceptable (wave2-skeptic F1).
|
Follow-up to review 4681028808:
Validation: focused background-session tests, typecheck, and build pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cli/bgRegistry.test.ts (1)
1005-1016: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually assert
getProcessCommandwas skipped.The comment/intent ("command lookup must not run without confirmed liveness") isn't enforced by an assertion — the test only checks
result.state === 'unreadable', which would also pass ifgetProcessCommandran and threw (since that's caught and also maps to'unreadable'). Track a call count like the "exit during command lookup" test does at line 993-1002, to actually prove the short-circuit.✅ Suggested strengthening
it('treats an access-denied liveness probe as unreadable', () => { + let commandCalls = 0 const result = verifyBackgroundSessionProcessIdentity(session, { signalProcess: () => { throw Object.assign(new Error('access denied'), { code: 'EPERM' }) }, getProcessCommand: () => { + commandCalls++ throw new Error('command lookup must not run without confirmed liveness') }, }) expect(result.state).toBe('unreadable') + expect(commandCalls).toBe(0) })🤖 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 `@src/cli/bgRegistry.test.ts` around lines 1005 - 1016, Strengthen the access-denied test around verifyBackgroundSessionProcessIdentity by tracking getProcessCommand invocations with a call counter, then assert the counter remains zero alongside the existing unreadable-state assertion. Preserve the current throwing stub so the test verifies command lookup is skipped after the EPERM liveness failure.Source: Path instructions
src/cli/bgRegistry.ts (1)
481-490: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTernary chain over identity state isn't exhaustive at the type level.
processState === 'matches' ? 'running' : processState === 'unreadable' ? 'unknown' : 'stale'silently folds both'not-running'and'mismatch'into'stale', and any future state added toBackgroundSessionProcessIdentity['state']would fall into'stale'without a compiler error. Aswitchwith exhaustive cases (or a lookup map keyed by the full union) would catch missing mappings at compile time.♻️ Exhaustive mapping alternative
- const nextStatus: BackgroundSessionStatus = - processState === 'matches' - ? 'running' - : processState === 'unreadable' - ? 'unknown' - : 'stale' + const nextStatus: BackgroundSessionStatus = ((): BackgroundSessionStatus => { + switch (processState) { + case 'matches': + return 'running' + case 'unreadable': + return 'unknown' + case 'not-running': + case 'mismatch': + return 'stale' + } + })()🤖 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 `@src/cli/bgRegistry.ts` around lines 481 - 490, Replace the nested ternary assigning nextStatus with an exhaustive switch or full-union lookup over processState in the surrounding background session status flow. Explicitly map every current BackgroundSessionProcessIdentity state, including 'matches', 'unreadable', 'not-running', and 'mismatch', while preserving their intended statuses; ensure future union members produce a compile-time error rather than defaulting to 'stale'.src/cli/bg.ts (1)
807-816: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the unreachable
pidguard.terminateBackgroundProcessTree()is only invoked here withsession.pid, so this callback never sees a descendant PID.verifySelectedBackgroundSessionIdentity()already covers the PID match, so the extra branch can be removed.🤖 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 `@src/cli/bg.ts` around lines 807 - 816, Remove the unreachable pid comparison and its unverifiedProcessError branch from the verifyBeforeSignal callback; retain the verifySelectedBackgroundSessionIdentity(session, options) call and return authorizeBackgroundSessionSignal(session, identity).
🤖 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.
Outside diff comments:
In `@src/cli/bg.ts`:
- Around line 807-816: Remove the unreachable pid comparison and its
unverifiedProcessError branch from the verifyBeforeSignal callback; retain the
verifySelectedBackgroundSessionIdentity(session, options) call and return
authorizeBackgroundSessionSignal(session, identity).
In `@src/cli/bgRegistry.test.ts`:
- Around line 1005-1016: Strengthen the access-denied test around
verifyBackgroundSessionProcessIdentity by tracking getProcessCommand invocations
with a call counter, then assert the counter remains zero alongside the existing
unreadable-state assertion. Preserve the current throwing stub so the test
verifies command lookup is skipped after the EPERM liveness failure.
In `@src/cli/bgRegistry.ts`:
- Around line 481-490: Replace the nested ternary assigning nextStatus with an
exhaustive switch or full-union lookup over processState in the surrounding
background session status flow. Explicitly map every current
BackgroundSessionProcessIdentity state, including 'matches', 'unreadable',
'not-running', and 'mismatch', while preserving their intended statuses; ensure
future union members produce a compile-time error rather than defaulting to
'stale'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5535b99-acb8-433e-94aa-465c9d30b50d
📒 Files selected for processing (4)
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.tssrc/cli/bg.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.tssrc/cli/bg.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.tssrc/cli/bg.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/cli/bgRegistry.test.tssrc/cli/bg.test.ts
🪛 ast-grep (0.44.1)
src/cli/bg.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (10)
src/cli/bg.ts (2)
820-847: LGTM! Exceptions fromverifySessionIdentityare correctly sanitized into a generic refusal without leaking callback error details, and the reported identity is cross-checked against the expected session/pid before being trusted.
863-878: 🎯 Functional CorrectnessNo issue:
authorizeBackgroundSessionSignalalready throws on'unreadable', sokillBackgroundSessioncannot fall through tomarkKilledon that path.> Likely an incorrect or invalid review comment.src/cli/bgRegistry.ts (2)
591-627: LGTM! The liveness → command-read → re-check-liveness sequence correctly closes the TOCTOU window between confirming the process is alive and reading/matching its command line, and exceptions from the injected command reader are safely downgraded to'unreadable'/latest liveness.
629-649: LGTM! Liveness classification correctly distinguishesESRCH(not-running) from other errno cases (unreadable), and injectedisProcessAlivethrows are contained.src/cli/bgRegistry.test.ts (1)
961-1045: LGTM! Good coverage of the identity state matrix (not-running/matches/mismatch/unreadable), exception sanitization for both liveness and command probes, and whitespace/empty command handling.src/cli/bg.test.ts (5)
522-532: LGTM!
558-588: LGTM! Confirms fail-closed behavior on identity mismatch (no signal, no mark-killed) and that sensitive session data isn't leaked into the refusal message.
771-800: LGTM! Sequencing assertion (verify → SIGTERM → sleep → verify → SIGKILL → sleep) is a solid, precise regression guard for the pre-escalation revalidation behavior.
802-820: LGTM! Directly exercisesterminateBackgroundSessionProcessTree's rejection of stale verifier results for another session/pid, with no signaling.
823-847: LGTM! ConfirmskillBackgroundSessionsanitizes a throwing verifier without leaking details and performs no signal/mark side effects, consistent with the sanitization logic inverifySelectedBackgroundSessionIdentity.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Do not re-signal terminal background-session records
src/cli/bg.ts:869
killBackgroundSessionnow verifies and terminates every resolved record, butrefreshBackgroundSessionStatusesdeliberately leaves terminal records (stale,killed,exited, andfailed) untouched. Consequently, after an old terminal record's PID is reused by a later OpenClaude invocation with the same session ID or stored command,openclaude kill <old-id>can classify that new process as a match and send it SIGTERM/SIGKILL. The previous handler only signaledrunningrecords, so this newly turns historical metadata into a destructive target. Preserve the terminal-status gate before verification/signaling (while keeping any desired idempotent metadata update), and add a PID-reuse regression test for a terminal record.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
3 participants
Summary
Implementation
Tests
Risk
Refs #1770
Summary by CodeRabbit
Bug Fixes
Tests