Skip to content

fix(models): omit plaintext api keys from models.json#84987

Open
junn-dev wants to merge 4 commits into
openclaw:mainfrom
junn-dev:codex/protect-agent-api-keys-11829
Open

fix(models): omit plaintext api keys from models.json#84987
junn-dev wants to merge 4 commits into
openclaw:mainfrom
junn-dev:codex/protect-agent-api-keys-11829

Conversation

@junn-dev

@junn-dev junn-dev commented May 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Fixes #11829.

This implements the Layer 1 hardening for models.json generation:

  • omit plaintext provider apiKey values before serializing agent-visible models.json
  • preserve non-secret auth markers such as known environment variable markers so marker-backed providers keep working
  • migrate legacy plaintext models.json keys into auth-profiles.json before redacting the generated file, so upgrades do not silently erase the only available credential
  • cover explicit providers, implicit providers, merge-mode existing providers, and legacy migration behavior

Real behavior proof:

  • ensureOpenClawModelsJson() was exercised through the existing models-config harness with a pre-existing models.json containing a plaintext provider key.
  • The regression verifies the regenerated models.json omits that plaintext key and the key is preserved in auth-profiles.json under custom-proxy:models-json.
  • The planner regression verifies plaintext keys from explicit, implicit, and existing merge-mode providers are absent from serialized output while OPENAI_API_KEY remains as a non-secret marker.

Validation:

  • corepack pnpm exec vitest run --reporter=dot src/agents/models-config.applies-config-env-vars.test.ts src/agents/models-config.skips-writing-models-json-no-env-token.test.ts -> 4 files passed, 26 tests passed
  • corepack pnpm exec oxlint src/agents/models-config.ts src/agents/models-config.plan.ts src/agents/models-config.applies-config-env-vars.test.ts src/agents/models-config.skips-writing-models-json-no-env-token.test.ts
  • git diff --check

TaskBounty: https://www.task-bounty.com/task/fix-security-roadmap-protecting-api-keys-from-agen-uej5sq

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 21, 2026
@clawsweeper

clawsweeper Bot commented May 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The PR redacts plaintext provider apiKey values from generated models.json and migrates legacy stored keys into auth profiles before writing the redacted file.

Reproducibility: yes. from source inspection: docs support models.json-only custom providers, while current planning skips before merge/redaction when no resolved providers exist. I did not run a local generation path because this review is read-only.

PR rating
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Summary: The PR has a useful direction but is not quality-ready because proof is mock-only and the patch misses a supported secret-bearing path.

Rank-up moves:

  • Cover and redact models.json-only custom providers with no config-backed provider present.
  • Add redacted real setup proof showing generated models.json omits plaintext keys and provider auth still works.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Real behavior proof
Needs real behavior proof before merge: The PR body reports unit/harness checks only; it needs redacted terminal output, logs, or another real setup artifact showing generated models.json redaction and continued provider auth, with private details removed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • A documented models.json-only custom provider can still keep a plaintext apiKey because the planner skips before the new redaction path when no providers are resolved from config, env, auth profiles, or discovery.
  • The PR body's proof is limited to unit/harness validation; reviewers still need redacted real setup output showing models.json redaction and continued provider auth.
  • Because this changes model credential storage, upgrade behavior for existing custom provider credentials needs explicit verification before merge.

Maintainer options:

  1. Fix existing-only redaction before merge (recommended)
    Route documented models.json-only providers through migration and redaction, then add focused coverage for that setup.
  2. Accept partial Layer 1 hardening
    Maintainers could land the narrower protection knowingly, but release notes should state that models.json-only custom providers are not covered yet.
  3. Pause for the roadmap issue
    If Layer 1 must cover every supported models.json credential path, keep this PR paused and continue the work under the linked security roadmap.

Next step before merge
Contributor action is needed for a code fix plus real behavior proof; this external PR should not be sent to automated repair while proof is mock-only.

Security
Needs attention: The patch improves secret handling but leaves a concrete supported models.json credential path unredacted.

Review findings

  • [P1] Redact models.json-only providers too — src/agents/models-config.ts:286-289
Review details

Best possible solution:

Keep the redaction boundary, but make existing models.json-only providers flow through the same migrate-then-redact rewrite path and prove the upgrade in a real setup.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: docs support models.json-only custom providers, while current planning skips before merge/redaction when no resolved providers exist. I did not run a local generation path because this review is read-only.

Is this the best way to solve the issue?

No, not yet: stripping provider apiKey before serialization is the right boundary, but this patch needs to handle existing-only models.json providers and include real behavior proof.

Label changes:

  • add P1: The PR targets model API key exposure to agents, a security-sensitive workflow affecting real users.
  • add merge-risk: 🚨 security-boundary: Merging this as-is could leave a supported plaintext models.json credential path exposed while appearing to complete Layer 1 redaction.

Label justifications:

  • P1: The PR targets model API key exposure to agents, a security-sensitive workflow affecting real users.
  • merge-risk: 🚨 security-boundary: Merging this as-is could leave a supported plaintext models.json credential path exposed while appearing to complete Layer 1 redaction.
  • rating: 🧂 unranked krab: Current PR rating is 🧂 unranked krab because proof is 🧂 unranked krab, patch quality is 🧂 unranked krab, and The PR has a useful direction but is not quality-ready because proof is mock-only and the patch misses a supported secret-bearing path.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports unit/harness checks only; it needs redacted terminal output, logs, or another real setup artifact showing generated models.json redaction and continued provider auth, with private details removed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Full review comments:

  • [P1] Redact models.json-only providers too — src/agents/models-config.ts:286-289
    The new migration runs before planning, but the planner still returns skip when resolved providers is empty on current main. Because docs allow custom providers to live only in models.json, that supported case gets a migrated auth profile while the original file keeps the plaintext apiKey, so Layer 1 is still bypassed. Please include existing-only providers in the redaction rewrite path and add a regression for that setup.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

Security concerns:

  • [medium] models.json-only providers remain exposed — src/agents/models-config.ts:286
    A custom provider stored only in models.json can still retain its plaintext apiKey because the new migration does not force a redacted rewrite when the planner skips for an empty resolved provider set.
    Confidence: 0.86

What I checked:

Likely related people:

  • @steipete: Auth/model config history shows repeated work on custom providers, models.json generation, merge helpers, and recent touches to the central files. (role: recent area contributor; confidence: high; commits: 082c872469f1, 79beb20ba2f3, 75e15216600b; files: src/agents/models-config.ts, src/agents/models-config.plan.ts, src/agents/models-config.merge.ts)
  • @joshavant: History ties this person to SecretRef-safe models.json persistence and provider secret hardening, which is the same boundary this PR changes. (role: SecretRef hardening contributor; confidence: high; commits: 8e20dd22d890, 36d2ae2a2235, 0bcb95e8fa5e; files: src/agents/models-config.ts, src/agents/models-config.providers.secrets.ts, src/agents/model-auth-markers.ts)
  • @vincentkoc: Recent history includes models.json readiness and embedded-run behavior, adjacent to the regeneration path this PR modifies. (role: adjacent recent contributor; confidence: medium; commits: 2b210703a3b9, d6d04f361e1c, f16ecd1dac5d; files: src/agents/models-config.ts, src/agents/models-config.plan.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 277a4b695264.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 21, 2026
@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@junn-dev

junn-dev commented May 21, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@junn-dev

junn-dev commented May 21, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Security Roadmap: Protecting API Keys from Agent Access

1 participant

@junn-dev