fix(agent): pin the agent's secure-storage dir so no keychain login leaks - #1191
fix(agent): pin the agent's secure-storage dir so no keychain login leaks#1191posthog[bot] wants to merge 1 commit into
Conversation
…eaks The agent subprocess got an empty CLAUDE_CONFIG_DIR, which closed the file half of a stored Claude login. The macOS keychain half stayed open whenever a user shell set CLAUDE_SECURESTORAGE_CONFIG_DIR: that var sits outside the ANTHROPIC_*/CLAUDE_CODE_* namespace the wizard strips, and the binary derives the keychain service name from it. Both spawn sites now take one env pair from isolatedAgentCredentialEnv(), which points CLAUDE_CONFIG_DIR and CLAUDE_SECURESTORAGE_CONFIG_DIR at the same per-run temp dir. The keychain lookup then names an item that does not exist, so it finds nothing. Generated-By: PostHog Desktop Task-Id: 658d6c4a-2e6b-4936-9845-77c4fa863fd2
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). Resolved comments: 1 left for you |
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a Context Mill branch:
Add Results will be posted here when complete. |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| // Per-run empty config + secure-storage dirs, so no stored Claude | ||
| // login reaches the gateway and 401s the run. See stored-login.ts. | ||
| ...isolatedAgentCredentialEnv(), |
There was a problem hiding this comment.
Fresh config directories break resumed tutorial sessions
Why we think it's a valid issue
- Checked: the pinned SDK itself. Downloaded and unpacked
@anthropic-ai/claude-agent-sdk@0.3.169(the version pinned atpackage.json:35) and read its typings and bundle, rather than reasoning from the snippet. - Found: the SDK ties resume to the config dir.
sdk.d.tsdocumentspersistSessionas "When false, disables session persistence to disk. Sessions will not be saved to ~/.claude/projects/ and cannot be resumed later. @default true", and thesessionStoredoc says "the subprocess still writes to CLAUDE_CONFIG_DIR". Insdk.mjsthe config root resolves as(process.env.CLAUDE_CONFIG_DIR ?? join(homedir(), ".claude")).normalize("NFC"), and lookup failures raiseSession ${id} not found in project directory/Session ${id} not found (no projects directory). - Found:
isolatedAgentCredentialEnv()callsmkdtempSyncon every invocation (stored-login.ts:57-60,stored-login.ts:90). The PR's own test asserts this at__tests__/stored-login.test.ts:129-131— "returns a distinct directory on each call". So prompt 2 spawns against a different, empty root than prompt 1 wrote its transcript into. - Found: the resume path is a shipped feature, not a corner.
McpSuggestedPromptsScreen.tsx:412passesresumeSessionId: currentSessionIdRef.current, whichmcp-prompt-streaming.ts:309turns intoresume.MAX_PROMPT_RUNS = 5(McpSuggestedPromptsScreen.tsx:121) with a dedicatedPhase.FollowUp, so every tutorial user who picks a second prompt hits it. The code states the intent it no longer delivers:mcp-prompt-streaming.ts:202-205and:298-300describe resumed follow-ups keeping prior turns as context. - Found: no mitigation applies. The SDK's resume-materialization temp dir (
claude-resume-*) only runs on thesessionStorepath, which this call site does not use. - Found: the root cause landed one commit earlier.
git diff FETCH_HEAD HEADshows main already hadCLAUDE_CONFIG_DIR: createIsolatedAgentConfigDir()here, added by919e7d1(fix(agent): isolate CLAUDE_CONFIG_DIR so a stored Claude login cannot 401 the run #1180, v2.71.0). This PR replaces that line with the two-var spread atmcp-prompt-streaming.ts:372. - Impact: every follow-up prompt in the MCP tutorial resumes a session id whose transcript is unreachable. The run either errors — surfaced to the user through the
errorchunk atmcp-prompt-streaming.ts:400-403andMcpSuggestedPromptsScreen.tsx:430-433— or silently starts fresh and drops the conversation the follow-up suggestions refer to. The trigger and the consequence are both concrete and deterministic, so this clears the bar even though the PR only re-expresses the line. It is the right moment to fix, because the change edits that exact line and adds a test that locks in the per-call fresh directory.
Issue description
The SDK stores resumable sessions under CLAUDE_CONFIG_DIR because persistSession defaults to true. This call creates a new directory for every prompt, including prompts with resumeSessionId. The resumed process cannot find the first prompt's transcript. Follow-up prompts can fail or lose their prior context.
Suggested fix
Create one isolated credential directory for each tutorial conversation. Reuse both environment values for resumed prompts. Create a new directory only when the user starts a new conversation. Add a test that checks path reuse for follow-ups and path replacement after reset.
Prompt to fix with AI (copy-paste)
## Context
@src/lib/agent/mcp-prompt-streaming.ts#L370-372
<issue_description>
The SDK stores resumable sessions under `CLAUDE_CONFIG_DIR` because `persistSession` defaults to `true`. This call creates a new directory for every prompt, including prompts with `resumeSessionId`. The resumed process cannot find the first prompt's transcript. Follow-up prompts can fail or lose their prior context.
</issue_description>
<issue_validation>
- **Checked:** the pinned SDK itself. Downloaded and unpacked `@anthropic-ai/claude-agent-sdk@0.3.169` (the version pinned at `package.json:35`) and read its typings and bundle, rather than reasoning from the snippet.
- **Found:** the SDK ties resume to the config dir. `sdk.d.ts` documents `persistSession` as "When false, disables session persistence to disk. Sessions will not be saved to ~/.claude/projects/ and cannot be resumed later. @default true", and the `sessionStore` doc says "the subprocess still writes to CLAUDE_CONFIG_DIR". In `sdk.mjs` the config root resolves as `(process.env.CLAUDE_CONFIG_DIR ?? join(homedir(), ".claude")).normalize("NFC")`, and lookup failures raise `Session ${id} not found in project directory` / `Session ${id} not found (no projects directory)`.
- **Found:** `isolatedAgentCredentialEnv()` calls `mkdtempSync` on every invocation (`stored-login.ts:57-60`, `stored-login.ts:90`). The PR's own test asserts this at `__tests__/stored-login.test.ts:129-131` — "returns a distinct directory on each call". So prompt 2 spawns against a different, empty root than prompt 1 wrote its transcript into.
- **Found:** the resume path is a shipped feature, not a corner. `McpSuggestedPromptsScreen.tsx:412` passes `resumeSessionId: currentSessionIdRef.current`, which `mcp-prompt-streaming.ts:309` turns into `resume`. `MAX_PROMPT_RUNS = 5` (`McpSuggestedPromptsScreen.tsx:121`) with a dedicated `Phase.FollowUp`, so every tutorial user who picks a second prompt hits it. The code states the intent it no longer delivers: `mcp-prompt-streaming.ts:202-205` and `:298-300` describe resumed follow-ups keeping prior turns as context.
- **Found:** no mitigation applies. The SDK's resume-materialization temp dir (`claude-resume-*`) only runs on the `sessionStore` path, which this call site does not use.
- **Found:** the root cause landed one commit earlier. `git diff FETCH_HEAD HEAD` shows main already had `CLAUDE_CONFIG_DIR: createIsolatedAgentConfigDir()` here, added by `919e7d1` (#1180, v2.71.0). This PR replaces that line with the two-var spread at `mcp-prompt-streaming.ts:372`.
- **Impact:** every follow-up prompt in the MCP tutorial resumes a session id whose transcript is unreachable. The run either errors — surfaced to the user through the `error` chunk at `mcp-prompt-streaming.ts:400-403` and `McpSuggestedPromptsScreen.tsx:430-433` — or silently starts fresh and drops the conversation the follow-up suggestions refer to. The trigger and the consequence are both concrete and deterministic, so this clears the bar even though the PR only re-expresses the line. It is the right moment to fix, because the change edits that exact line and adds a test that locks in the per-call fresh directory.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Create one isolated credential directory for each tutorial conversation. Reuse both environment values for resumed prompts. Create a new directory only when the user starts a new conversation. Add a test that checks path reuse for follow-ups and path replacement after reset.
</potential_solution>
There was a problem hiding this comment.
Confirmed this is a real bug against the current head, and I'm escalating it for a human decision rather than fixing it unattended.
What happens: the MCP tutorial resumes sessions for follow-up prompts (it carries the previous run's session id forward as resumeSessionId, up to 5 prompts per session). But each run is handed a brand-new, empty per-run config directory — the isolation helper creates a fresh temp dir on every call, and a test in this PR deliberately locks that in ("a distinct directory on each call"). Because the SDK keeps resumable transcripts under that config directory, a resumed follow-up looks for the first prompt's transcript in a directory that never held it. The result is either a "session not found" error surfaced to the user or a silent fresh start that drops the conversation the follow-up suggestions refer to. The root cause actually shipped in v2.71.0 with the original config-dir isolation, but this PR edits the exact line, so it's a reasonable place to address it.
Why it needs a human: fixing it means reconciling two goals that currently conflict — per-run credential isolation wants a fresh, empty directory, while session resume wants a stable directory for the whole conversation. The natural fix is one isolated directory per tutorial conversation (created when the user picks a new prompt, reused for follow-ups, reset on a new conversation), which requires threading that directory through the screen → services → streaming runner, changing the isolation helper's contract, and updating its locking test. Two things put it beyond an unattended fix: (1) its correctness — that resume actually recovers the transcript once the directory is stable — can only be proven by running the real Agent SDK against a resumed session, which this pass can't do; and (2) the helper is credential-leak-prevention code, so changing how its directory is created and reused shouldn't be done without a human confirming it doesn't reopen a leak path.
Decision needed: adopt the per-conversation stable isolated directory (and confirm via a real resumed-session run that it fixes resume without reintroducing a stored-login leak), or decide the tutorial shouldn't resume at all (drop resume here and accept each prompt as standalone).
Problem
CLAUDE_CONFIG_DIR, so the binary finds no.credentials.jsonor.claude.json.CLAUDE_SECURESTORAGE_CONFIG_DIRsits outside theANTHROPIC_*/CLAUDE_CODE_*namespacesanitizeAgentSubprocessEnvstrips, so a value in the user's shell survives into the subprocess and points the keychain lookup back at the real login item.Changes
isolatedAgentCredentialEnv()returns both vars set to the same fresh per-run temp dir; both spawn sites spread it.~/.claude/.credentials.json,~/.claude.jsonCLAUDE_CONFIG_DIRisolationCLAUDE_SECURESTORAGE_CONFIG_DIRset in the user's shellTest plan
pnpm build && pnpm test && pnpm fix— 2586 tests pass.@anthropic-ai/claude-agent-sdk@0.3.169) against a local HTTP probe that logs auth headers, with a planted stored credential:Probe results
authorization: Bearer <gateway>andx-api-key: <stored key>→ the gateway 401sauthorization: Bearer <gateway>onlyauthorization: Bearer <gateway>only; the subprocess runs normally and writes its scratch state into the temp dirThis is why the failure is a 401 and not a silent bill: a stored login travels as
x-api-key, which the binary resolves separately from theANTHROPIC_AUTH_TOKENthe wizard injects, and the gateway accepts only its own token.Note
Scope check, because it narrows the linked report. Reading the SDK binary shows the keychain lookup is path-scoped after all — the service name carries a hash of the config dir — so the shipped
CLAUDE_CONFIG_DIRfix already covered most macOS users as a side effect. What remains is theCLAUDE_SECURESTORAGE_CONFIG_DIRoverride above, plus the risk of leaning on undocumented SDK behaviour across upgrades. The error-screen numbers in the report predate v2.71.0, so they cannot yet show how much is left; worth re-reading them a week after this ships.LLM context
Investigated by decompiling the pinned SDK's macOS binary to confirm the credential resolution order, then reproducing header-by-header against a local probe server. Considered and rejected: adding
CLAUDE_SECURESTORAGE_CONFIG_DIRto the namespace strip inagent-env-isolation.ts— unsetting it makes the binary fall back to the default keychain item, which is the login being defended against, so the var has to be pinned rather than removed. Also left alone: the shared spawn-site env recipe, which would be a wider refactor than this fix needs.Created with PostHog Desktop from this inbox report.