fix(agents): harden host-managed Claude CLI auth path#61276
Conversation
d9c0d2e to
7eb2e04
Compare
7eb2e04 to
aeeca8d
Compare
Greptile SummaryThis PR hardens the host-managed Claude CLI auth path end-to-end: it scrubs inherited Claude env/config/plugin roots ( All changes are intentional security hardening with one P2 note: the Confidence Score: 5/5Safe to merge — all changes are intentional security hardening with no correctness regressions identified. No P0 or P1 findings. The single P2 note (unbounded image-cache growth) is an acknowledged design tradeoff documented with an inline comment. The env scrubbing, permission-mode normalisation, setting-sources enforcement, and migration model-allowlist replace logic are all correct. The PR's scoped serial-test evidence covers the touched path. src/agents/cli-runner/helpers.ts (no-op cleanup); extensions/anthropic/cli-shared.ts (new normalizer logic — reviewed and correct) Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/cli-runner/helpers.ts
Line: 258-259
Comment:
**No-op cleanup accumulates images indefinitely**
The content-addressed path is sound for prompt-cache stability, but `cleanup` is now a strict no-op — every unique image written to `openclaw-cli-images/` stays on disk forever with no eviction. Long-running sessions or workflows with varied images will grow this directory without bound. A background LRU sweep or max-age cull (e.g., files older than N days) would keep the cache bounded.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(agents): harden host-managed claude-..." | Re-trigger Greptile |
| const cleanup = async () => {}; | ||
| return { paths, cleanup }; |
There was a problem hiding this comment.
The content-addressed path is sound for prompt-cache stability, but cleanup is now a strict no-op — every unique image written to openclaw-cli-images/ stays on disk forever with no eviction. Long-running sessions or workflows with varied images will grow this directory without bound. A background LRU sweep or max-age cull (e.g., files older than N days) would keep the cache bounded.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner/helpers.ts
Line: 258-259
Comment:
**No-op cleanup accumulates images indefinitely**
The content-addressed path is sound for prompt-cache stability, but `cleanup` is now a strict no-op — every unique image written to `openclaw-cli-images/` stays on disk forever with no eviction. Long-running sessions or workflows with varied images will grow this directory without bound. A background LRU sweep or max-age cull (e.g., files older than N days) would keep the cache bounded.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eb2e042db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Provider auth migrations can intentionally replace the exact allowlist. | ||
| models: patchModels as NonNullable< | ||
| NonNullable<OpenClawConfig["agents"]>["defaults"] | ||
| >["models"], | ||
| }, |
There was a problem hiding this comment.
Preserve existing model allowlist when applying auth patches
applyProviderAuthConfigPatch now replaces agents.defaults.models with the patch payload whenever that field is present, which turns partial auth patches into destructive updates. Existing OAuth auth flows commonly return only provider-local/default entries (for example via buildOauthProviderAuthResult), so running auth for one provider can silently drop previously configured model aliases/allowlist entries from other providers instead of merging them. This is a behavior regression from the prior recursive merge and can unexpectedly remove models users still rely on after login.
Useful? React with 👍 / 👎.
Summary
openclaw models auth login --provider anthropic --method cli --set-default, provider-owned auth patch application, inherited Claude Code env/config roots, and repo-local Claude settings flowing into non-interactive runs.claude auth logincould end up with staleanthropic/*defaults/fallbacks, inherited parent routing/auth/config state, or repo-local.claudehooks/settings affecting host-managed CLI sessions.claude-clipath end-to-end: fixed migrated default-model replacement behavior, hardened provider-auth patch application, normalized malformed permission overrides, scrubbed inherited Claude env/config/plugin roots, forced--setting-sources user, and stabilized image hydration paths/extensions for prompt-cache reuse.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
claude-cli/*; the host-managed backend also trusted inherited Claude env/config state more than it should.models auth login, non-interactive migration, malformed backend security flags, inherited Claude env/config/plugin roots, and repo-local setting-source isolation.--print, and still loads project/local settings unless--setting-sourcesconstrains them.Regression Test Plan (if applicable)
extensions/anthropic/cli-migration.test.tsextensions/anthropic/cli-shared.test.tssrc/agents/cli-backends.test.tssrc/commands/models/auth.test.tssrc/commands/auth-choice.apply.plugin-provider.test.tssrc/plugins/provider-auth-choice-helpers.test.tssrc/gateway/gateway-cli-backend.live.test.tssrc/agents/cli-runner.helpers.test.ts--setting-sources user, and keep stable image hydration paths/extensions.src/agents/cli-auth-epoch.test.tsstill covers the auth-epoch/session invalidation seam adjacent to this path.User-visible / Behavior Changes
openclaw models auth login --provider anthropic --method cli --set-defaultnow cleanly rewrites migrated defaults toclaude-cli/*without leaving staleanthropic/*allowlist/fallback entries behind.--setting-sources user, so repo-local.claudeproject/local settings and hooks do not silently flow into non-interactive OpenClaw sessions..bin.Diagram (if applicable)
Security Impact (required)
No)Yes)No)Yes)Yes)Yes, explain risk + mitigation:--setting-sources userso repo-local.claudeproject/local settings and hooks do not silently affect non-interactive OpenClaw sessions.Repro + Verification
Environment
claude-cli/*via Anthropic CLI auth pathSteps
openclaw models auth login --provider anthropic --method cli --set-defaultwith existing Anthropic defaults/fallbacks.claude-clibackend with default and custom override configs, then run image hydration prep for repeated prompts.Expected
claude-cli/*without staleanthropic/*entries.claude-cliruns keep hardened env/config defaults and force--setting-sources user.Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm test:serial extensions/anthropic/cli-shared.test.ts src/agents/cli-backends.test.tspnpm test:serial extensions/anthropic/cli-migration.test.ts src/plugins/provider-auth-choice-helpers.test.ts extensions/anthropic/cli-shared.test.ts src/agents/cli-backends.test.ts src/commands/models/auth.test.ts src/commands/auth-choice.apply.plugin-provider.test.ts src/agents/cli-auth-epoch.test.ts src/gateway/gateway-cli-backend.live.test.ts src/agents/cli-runner.helpers.test.ts--permission-mode--setting-sourcesoverridessrc/agents/cli-runner.spawn.test.tsremains a local vitest hotspot and was not re-run to completionpnpm check/pnpm buildare still red on unrelated issues already visible on this treeReview Conversations
Compatibility / Migration
Yes)Nopublic contract change; hardened runtime defaults only`)No)Risks and Mitigations
cliBackends.claude-cli.argsoverride could rely on project/local Claude setting sources or malformed permission flags.--setting-sources userand valid--permission-modedefaults.pnpm build/pnpm checkfailures are called out explicitly below.Known Unrelated Local Failures
pnpm buildfails on this tree with unrelated unresolved import insrc/agents/tool-display.tsfor../../apps/shared/OpenClawKit/Sources/OpenClawKit/Resources/tool-display.json.pnpm check/pnpm tsgoalso fail on unrelated repo-wide issues outside this change, includingsrc/agents/pi-embedded-runner/google-prompt-cache*,src/docs/clawhub-plugin-docs.test.ts,src/gateway/reconnect-gating.test.ts,src/i18n/registry.test.ts, andsrc/ui-app-settings.agents-files-refresh.test.ts.AI-assisted