Skip to content

fix: restore redacted SecretRef id during config.set round-trip#54848

Closed
CoCoManYY wants to merge 2 commits into
openclaw:mainfrom
CoCoManYY:fix/restore-secret-ref-id
Closed

fix: restore redacted SecretRef id during config.set round-trip#54848
CoCoManYY wants to merge 2 commits into
openclaw:mainfrom
CoCoManYY:fix/restore-secret-ref-id

Conversation

@CoCoManYY

@CoCoManYY CoCoManYY commented Mar 26, 2026

Copy link
Copy Markdown

Summary

Fix asymmetric redaction/restore for SecretRef objects in config.set.

  • redactSecretRefId preserves the SecretRef structure and only redacts the id field, but restoreRedactedValues has no corresponding logic to restore it.
  • This causes config.set to fail with "File secret reference id must be an absolute JSON pointer" when the config contains file-based SecretRef objects (e.g. models.providers.*.headers.* or gateway.auth.token).

Changes

  • Add restoreSecretRefId in redact-snapshot.secret-ref.ts (symmetric to redactSecretRefId)
  • Detect SecretRef with redacted id in both restoreRedactedValuesWithLookup and restoreRedactedValuesGuessing before recursing into objects

Reproduction

  1. Configure a model provider with file-based SecretRef for headers or apiKey: 
    "headers": {
  "x-api-key": {
    "source": "file",
    "provider": "my-secret-provider",
    "id": "/my_secret_key"
  }
}
  2. Call config.get → returns { source: "file", provider: "my-secret-provider", id: "__OPENCLAW_REDACTED__" }
  3. Pass the result back to config.set → fails validation: 
    File secret reference id must be an absolute JSON pointer (example: "/providers/openai/apiKey"), or "value" for singleValue mode.

Root Cause

During redaction, redactSecretRefId does SecretRef-aware redaction — only replaces id, preserving source and provider. But during restoration, the code recurses into the object and encounters id: "__OPENCLAW_REDACTED__" at a path like models.providers.*.headers.x-api-key.id, which doesn't match isSensitivePath() patterns, so the sentinel value is left as-is and fails schema validation.

Test plan

  • Existing tests pass
  • config.get → config.set round-trip with file SecretRef no longer fails validation
  • Plain string secrets still restore correctly
  • SecretRef with non-redacted id (e.g. env var placeholder) passes through unchanged

🤖 AI-assisted PR — code reviewed and tested manually.

fix: restore redacted SecretRef id during config.set round-trip
cc3c00f 
`redactSecretRefId` preserves the SecretRef object structure and only
redacts the `id` field, but `restoreRedactedValues` has no corresponding
logic to detect and restore it. When a config obtained from `config.get`
is passed back to `config.set`, the redacted `id` (__OPENCLAW_REDACTED__)
fails JSON Pointer validation with:

  "File secret reference id must be an absolute JSON pointer"

Add `restoreSecretRefId` (symmetric to `redactSecretRefId`) and call it
in both `restoreRedactedValuesWithLookup` and
`restoreRedactedValuesGuessing` before recursing into objects, so that
SecretRef objects survive a Web UI / API round-trip unmodified.
@CoCoManYY CoCoManYY requested a review from a team as a code owner March 26, 2026 03:09
@greptile-apps

greptile-apps Bot commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a symmetric redaction/restore bug for SecretRef objects in the config.set round-trip. During redaction, redactSecretRefId preserved the SecretRef object structure but only replaced the id field with the sentinel. The corresponding restore logic had no SecretRef-aware handling, so it would recurse into the object and fail to restore id: "__OPENCLAW_REDACTED__" because the sub-path (e.g. models.providers.*.headers.x-api-key.id) didn't match any isSensitivePath() pattern, causing a schema validation error.\n\nChanges:\n- Adds restoreSecretRefId in redact-snapshot.secret-ref.ts — symmetric to the existing redactSecretRefId, it replaces a sentinel-valued id with the original value.\n- Applies the new check in all three object-traversal code paths inside restoreRedactedValuesWithLookup (both the lookup.has and !matched branches) and restoreRedactedValuesGuessing, intercepting SecretRef objects before they get recursed into.\n- The guard correctly short-circuits only when isSecretRefShape is true on both incoming and original, and id equals the sentinel — all other cases fall through to the existing recursive logic.

Confidence Score: 5/5

This PR is safe to merge — the fix is minimal, surgical, and correctly symmetric to the existing redaction logic.

The new restoreSecretRefId function mirrors redactSecretRefId exactly: it only replaces id when it equals the sentinel, and leaves everything else untouched. All three restore code paths are updated consistently. Edge cases (non-redacted id, missing/null original, original not a SecretRef) all fall through to the pre-existing logic unchanged.

No files require special attention.

Reviews (1): Last reviewed commit: "fix: restore redacted SecretRef id durin..." | Re-trigger Greptile

@joshavant joshavant mentioned this pull request Mar 31, 2026
Merged
@joshavant

joshavant commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

Thanks @CoCoManYY for the contribution on SecretRef id restoration.

This is now fully covered by #58044 (merged), which landed the round-trip restore fix in the broader hardened implementation and test set.

Closing this PR as superseded by #58044.

@joshavant joshavant closed this Mar 31, 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

size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@CoCoManYY @joshavant