Policy: add secret and auth conformance checks#81974
Conversation
|
Codex review: needs real behavior proof before merge. Latest ClawSweeper review: 2026-05-22 19:45 UTC / May 22, 2026, 3:45 PM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: not applicable. this is a feature PR rather than a bug report. The behavior is reviewable from the PR source, tests, docs, and proof comments, but the current head still needs real CLI proof. PR rating Rank-up moves:
What the crustacean ranks mean
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 Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Land only after a maintainer accepts the opt-in secret/auth Policy semantics and the contributor adds redacted current-head Do we have a high-confidence way to reproduce the issue? Not applicable: this is a feature PR rather than a bug report. The behavior is reviewable from the PR source, tests, docs, and proof comments, but the current head still needs real CLI proof. Is this the best way to solve the issue? Yes for the owner boundary: implementing this as config-level Policy plugin conformance over existing SecretRef/auth metadata is the maintainable direction. Merge readiness still depends on current-head proof and maintainer acceptance of the policy semantics. Label justifications:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 63545693a0fa. |
4b344b0 to
8a8bb1a
Compare
|
Restacked this branch onto current upstream/main and pushed the follow-ups needed by validation:
I also updated the PR body with the required Real behavior proof fields. |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
78e5eb3 to
ac8f697
Compare
0920c77 to
cd5518c
Compare
galiniliev
left a comment
There was a problem hiding this comment.
This PR adds Policy plugin evidence and doctor/policy-check findings for redacted secret/auth provenance. That is the right ownership boundary for this config-level conformance work, but the new scanner currently misses several schema-backed SecretInput surfaces, so secrets.disallowInline is not reliable yet.
Findings
- [P1] Secret scanner misses configured provider request SecretInput fields.
ConfiguredProviderRequestallows inline secrets inrequest.auth.value,request.tls.ca/cert/key/passphrase, proxy TLS fields, and arbitraryrequest.headers.*, andModelProviderSchemaexposes that request object undermodels.providers.*.request. The new recursive scanner only records keys matching the hardcoded name list here, so a policy withsecrets.disallowInline: truewill not report inline TLS keys, passphrases, header auth values, or custom header secrets. That leaves real OpenClaw config secret inputs outside the evidence and attestation payload. Please drive this from the schema/secret-target contract or explicitly cover the provider request SecretInput paths, and add regression tests for at leastmodels.providers.openai.request.auth.value,models.providers.openai.request.tls.key, andmodels.providers.openai.request.headers.Authorization.
Bug/behavior: new policy conformance feature for config-derived secret/auth provenance.
Affected surface: Policy plugin doctor --lint / policy check evidence and findings, plus attestation hashes.
Provenance: N/A, feature PR.
Best-fix verdict: keep this in the Policy plugin, but make the secret input discovery complete for the config contract before merge.
Reviewed code paths: extensions/policy/src/policy-state.ts, extensions/policy/src/doctor/register.ts, extensions/policy/src/doctor/register.test.ts, src/config/types.provider-request.ts, and src/config/zod-schema.core.ts.
| function isSecretInputKey(key: string): boolean { | ||
| const normalized = key.toLowerCase(); | ||
| return ( | ||
| normalized === "apikey" || |
There was a problem hiding this comment.
This key-name heuristic misses several real SecretInput fields from the config contract. ConfiguredProviderRequest exposes auth.value, TLS ca/cert/key/passphrase, proxy TLS, and arbitrary headers.* as SecretInput, and model providers carry that under models.providers.*.request. With the current allowlist, inline values at those paths are not added to evidence, so secrets.disallowInline: true and attestation hashes do not cover them. Please drive discovery from the schema/secret-target contract or explicitly handle these request paths, with regressions for request.auth.value, request.tls.key, and request.headers.Authorization.
There was a problem hiding this comment.
Addressed in signed commit 1e187e7d1b240b73be147f6c120b90e1e35bc721.
What changed:
- Added explicit secret-input path coverage for provider request fields:
request.headers.*,request.auth.value,request.tls.{ca,cert,key,passphrase}, and proxy TLS. - Added matching coverage for the existing audio request/header surfaces and MCP env plugin config surfaces.
- Kept secret evidence based on the raw config file when available so
${ENV}SecretRef-style markers are not misclassified after env substitution. - Added regressions for
models.providers.openai.request.auth.value,models.providers.openai.request.tls.key,models.providers.openai.request.headers.Authorization, and raw${OPENAI_API_KEY}handling.
Proof run:
git diff --checknode scripts/run-vitest.mjs extensions/policy/src/doctor/register.test.ts extensions/policy/src/cli.test.ts --run --reporter=dot-> 93 tests passednode node_modules/@typescript/native-preview/bin/tsgo.js -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test-policy-81974-galin3.tsbuildinfo/mnt/c/src/claws-hapi/.agents/skills/autoreview/scripts/autoreview --mode branch --reviewer codex --fallback-reviewer none-> no accepted/actionable findings
CI on the pushed head is green for preflight, check-dependencies, and Real behavior proof. Please re-review when you have a chance.
galiniliev
left a comment
There was a problem hiding this comment.
This PR adds Policy evidence and findings for secret/auth provenance, which is the right surface for enforcing inline-secret and auth-profile conformance. The latest update closes the model-provider request gap, but the same SecretInput-bearing request contract is still only partially covered for media config.
Bug/behavior: policy conformance can miss inline media provider request secrets.
Affected surface: Policy plugin secret evidence/attestation scanning.
Provenance: N/A, feature coverage in this PR.
Best-fix verdict: keep this in the Policy plugin, but align SecretInput discovery with the config/schema secret-target contract or add the missing media request patterns plus regressions before merge.
| /^models\.providers\.[^.]+\.request\.(?:proxy\.)?tls\.(?:ca|cert|key|passphrase)$/.test( | ||
| joined, | ||
| ) || | ||
| /^tools\.media\.audio\.request\.headers\.[^.]+$/.test(joined) || |
There was a problem hiding this comment.
[P1] Cover all media ConfiguredProviderRequest paths. This still only covers root tools.media.audio.request.*. The same request contract is also available on tools.media.models[], tools.media.image, tools.media.video, and each section's models[] entries through MediaUnderstandingRuntimeFields, so inline values like tools.media.models[].request.headers.Authorization, tools.media.image.request.auth.value, or tools.media.video.request.tls.key are not recorded in evidence. That means secrets.disallowInline and workspace attestation can report clean while inline media request secrets remain in config. Please either drive discovery from the config/secret-target contract or add the missing media request patterns with regressions for shared models and image/video section requests.
There was a problem hiding this comment.
Fixed in 676fdc7.
Changed the secret scanner from one-off provider/media regexes to a configured provider request helper and applied it to the media request categories:
- tools.media.models[].request.*
- tools.media.audio.request.* and tools.media.audio.models[].request.*
- tools.media.image.request.* and tools.media.image.models[].request.*
- tools.media.video.request.* and tools.media.video.models[].request.*
Added regressions for shared media model headers, audio model proxy TLS passphrase, image request auth value, and video request TLS key, with assertions that evidence/findings do not leak inline values.
Proof:
- git diff --check
- node scripts/run-vitest.mjs extensions/policy/src/doctor/register.test.ts extensions/policy/src/cli.test.ts --run --reporter=dot
- node node_modules/@typescript/native-preview/bin/tsgo.js -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test-policy-81974-media-requests.tsbuildinfo
- /mnt/c/src/claws-hapi/.agents/skills/autoreview/scripts/autoreview --mode local -> clean, no accepted/actionable findings
|
Pushed signed commit 72edf5b with the lint-only cleanup after restoring the original combined #81974 shape. Verification:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
Addressed the latest SecretInput coverage blocker on current head Behavior addressed: Policy secret evidence now explicitly covers configured-provider
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Superseded by the corrected proof comment below. The previous body was corrupted by shell command substitution while posting. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Rebased on current Behavior addressed: refreshed root and plugin
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
Superseded by the corrected proof comment below. The previous body was corrupted by shell command substitution while posting. |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 Reason: re-review requires an open issue or PR. |
Summary
Adds Policy 1.0 secret provenance and auth-profile conformance checks with redacted config evidence.
Logical base: merged #80783. This PR intentionally stays config-level only: it reads existing OpenClaw config and reports policy conformance findings through
doctor --lint; it does not add runtime enforcement and it does not take on secrets management.Policy syntax
{ "secrets": { "requireManagedProviders": true, "denySources": ["exec"], "allowInsecureProviders": false }, "auth": { "profiles": { "requireMetadata": ["provider", "mode"], "allowModes": ["api_key", "token"] } } }Safety
This is policy conformance, not secrets regulation or storage management.
doctor --lintreads config-level declarations, emits redacted findings, and leaves credential stores and runtime behavior to the existing auth/credential systems.Real behavior proof
Behavior addressed: config-level Policy conformance for redacted secret provenance and auth-profile metadata/mode rules.
Real environment tested: WSL checkout at
/root/src/openclaw-81974-policyon Ubuntu-24.04; PR headd72baa8115b12041a1c532479e919480c46eda57is signed and GitHub-verified.Exact steps or command run after this patch:
git push fork policy-81974-config:policy-secrets-auth-provenanceOPENCLAW_CONFIG_PATH=/tmp/policy-proof-81974/openclaw.json timeout 180 node scripts/run-node.mjs doctor --lint --json --severity-min errorpnpm exec oxfmt extensions/policy/src/doctor/register.ts extensions/policy/src/policy-state.tsgit diff --checkpnpm run lint:extensions:bundlednode scripts/run-vitest.mjs extensions/policy/src/doctor/register.test.ts extensions/policy/src/cli.test.ts --run --reporter=dotnode node_modules/@typescript/native-preview/bin/tsgo.js -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test-policy-81974-original-lint.tsbuildinfoEvidence after fix: the real
doctor --lintCLI run loaded a temp workspace policy and config, ran 37 checks, and returned four redacted policy errors:{ "ok": false, "checksRun": 37, "checksSkipped": 0, "findings": [ { "checkId": "policy/auth-profile-unapproved-mode", "ocPath": "oc://openclaw.config/auth/profiles/oauth", "requirement": "oc://policy.jsonc/auth/profiles/allowModes", "message": "Auth profile 'oauth' uses mode 'oauth' outside the policy allowlist." }, { "checkId": "policy/secrets-denied-provider-source", "ocPath": "oc://openclaw.config/secrets/providers/command", "requirement": "oc://policy.jsonc/secrets/denySources", "message": "Secret provider 'command' uses denied source 'exec'." }, { "checkId": "policy/secrets-insecure-provider", "ocPath": "oc://openclaw.config/secrets/providers/vault", "requirement": "oc://policy.jsonc/secrets/allowInsecureProviders", "message": "Secret provider 'vault' enables insecure posture: allowInsecurePath." }, { "checkId": "policy/secrets-unmanaged-provider", "ocPath": "oc://openclaw.config/models/providers/anthropic/apiKey", "requirement": "oc://policy.jsonc/secrets/requireManagedProviders", "message": "SecretRef uses unmanaged provider 'missing'." } ] }Observed result after fix: the CLI reported the expected secret/auth conformance findings through
doctor --lintwithout printing private secret values.What was not tested: no live Gateway was launched; this is config-level policy conformance proof only.
Related