Skip to content

fix(config): use Object.hasOwn instead of in operator in restoreOriginalValueOrThrow#99152

Merged
steipete merged 3 commits into
openclaw:mainfrom
zenglingbiao:fix/prototype-pollution-redact-snapshot
Jul 3, 2026
Merged

fix(config): use Object.hasOwn instead of in operator in restoreOriginalValueOrThrow#99152
steipete merged 3 commits into
openclaw:mainfrom
zenglingbiao:fix/prototype-pollution-redact-snapshot

Conversation

@zenglingbiao

@zenglingbiao zenglingbiao commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Use Object.hasOwn() instead of the in operator in restoreOriginalValueOrThrow() to prevent prototype chain pollution when restoring redacted config values.

What Problem This Solves

When a config key named after an Object.prototype property (e.g., toString) is redacted and needs restoration, restoreOriginalValueOrThrow uses key in original to check if the original value exists. On a plain object without that key, "toString" in original returns true (matching Object.prototype.toString), causing the function to return a native function instead of the expected config value.

Code evidence: In src/config/redact-snapshot.ts:541, if (params.key in params.original) traverses the prototype chain. params.original is a Record<string, unknown> plain object, and params.key comes from redacted config paths.

Root Cause

Why This Fix

Evidence

  • Before: restoreRedactedValues({toString: REDACTED}, {}, hints) returns ok=true with a function value
  • After: correctly returns ok=false with a proper error

Real Behavior Proof

Before (on main)

$ node --import tsx proof.mjs
FAIL: restoreRedactedValues returned ok=true
  restored value type: function
  got FUNCTION from prototype
PASS: normal key restoration still works
SOME FAILED

After (with fix)

$ node --import tsx proof.mjs
PASS: restoreRedactedValues correctly rejected missing toString key
  error: Sentinel value "__OPENCLAW_REDACTED__" in key toString is not valid as real data
PASS: normal key restoration still works
ALL PASS

Tests and Validation

  • Regression test added to existing src/config/redact-snapshot.test.ts covering 4 prototype keys
  • Normal key restoration: unaffected
  • Diff: 1 line changed in source + regression test

@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 6:35 AM ET / 10:35 UTC.

Summary
The PR replaces params.key in params.original with Object.hasOwn(params.original, params.key) in config redaction restore and adds a regression test for prototype-named missing keys.

PR surface: Source 0, Tests +10. Total +10 across 2 files.

Reproducibility: yes. Source inspection shows current main accepts inherited prototype keys through params.key in params.original, and the PR body provides before/after terminal output for the toString restore case.

Review metrics: none identified.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] Required CI is not fully green on the PR head: QA Smoke CI is failing, though the diff-specific config checks inspected here are green and no code finding ties that failure to this patch.

Maintainer options:

  1. Decide the mitigation before merge
    Land the narrow helper-level own-property check with the focused restore regression once the normal required merge gates are satisfied.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] No repair lane is needed because the patch has no actionable review findings; maintainers only need normal merge-gate handling.

Security
Cleared: The diff does not touch dependencies, workflows, lockfiles, or secret storage, and the config-redaction change tightens own-property restoration instead of weakening a boundary.

Review details

Best possible solution:

Land the narrow helper-level own-property check with the focused restore regression once the normal required merge gates are satisfied.

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

Yes. Source inspection shows current main accepts inherited prototype keys through params.key in params.original, and the PR body provides before/after terminal output for the toString restore case.

Is this the best way to solve the issue?

Yes. restoreOriginalValueOrThrow is the shared restoration owner for both lookup and guessing paths, so changing that helper to Object.hasOwn is narrower than caller-side filtering or null-prototype object reconstruction.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f84bcdb4d79c.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes before/after terminal output for the real restore path, and maintainer discussion adds exact-head focused test proof after the test-only cleanup.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes before/after terminal output for the real restore path, and maintainer discussion adds exact-head focused test proof after the test-only cleanup.

Label justifications:

  • P2: This fixes a real config redaction restore bug with limited blast radius and a narrow production change.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes before/after terminal output for the real restore path, and maintainer discussion adds exact-head focused test proof after the test-only cleanup.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes before/after terminal output for the real restore path, and maintainer discussion adds exact-head focused test proof after the test-only cleanup.
Evidence reviewed

PR surface:

Source 0, Tests +10. Total +10 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 1 0
Tests 1 10 0 +10
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 11 1 +10

What I checked:

Likely related people:

  • steipete: Current-main blame for the restore helper and gateway config caller points to 1c7229dead, and he authored the PR's test-focus commit plus the maintainer deep-review proof for this exact head. (role: recent area contributor and reviewer; confidence: high; commits: 1c7229deadbf, d5700c0185d7; files: src/config/redact-snapshot.ts, src/gateway/server-methods/config.ts, src/config/redact-snapshot.restore.test.ts)
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.

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.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 2, 2026
@knoal

knoal commented Jul 2, 2026

Copy link
Copy Markdown

Onyx_bot Review

Bug confirmed. The restoreOriginalValueOrThrow function uses params.key in params.original — the in operator traverses the prototype chain, so toString in {} returns true, silently returning an inherited prototype method instead of throwing.

Fix confirmed. Object.hasOwn(params.original, params.key) checks own enumerable properties only — correct.

Test confirmed. Regression test iterates 4 prototype-key names against an empty original and asserts result.ok === false. Directly exercises the previously-broken path.

Assessment: Correct. 1-line fix, right API, adequate regression coverage. Ready to merge.

@steipete steipete self-assigned this Jul 3, 2026
@steipete
steipete requested a review from a team as a code owner July 3, 2026 10:07
@steipete

steipete commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Maintainer deep review and proof for exact head d5700c0185d751a3b909d5bc4d0a75f5e96d2276:

  • Best-fix verdict: yes. restoreOriginalValueOrThrow is the shared owner for sentinel restoration, and Object.hasOwn directly enforces the required invariant: only a value actually present in the original config may be restored. Caller-side filtering or null-prototype copies would be broader and leave other restore paths exposed.
  • Root-cause proof: on current main, a missing key such as toString still satisfies key in original through Object.prototype, so the restore path returns an inherited function. The PR rejects that state as a missing original value.
  • Improvement: moved the regression out of the broad snapshot suite into redact-snapshot.restore.test.ts, table-driven across toString, constructor, valueOf, and hasOwnProperty. Net test cleanup: 11 insertions / 16 deletions.
  • Focused proof: node scripts/run-vitest.mjs src/config/redact-snapshot.test.ts src/config/redact-snapshot.restore.test.ts — 2 files, 63 tests passed.
  • Quality: git diff --check passed. Fresh autoreview reported no accepted/actionable findings (patch is correct, 0.99).
  • Live-proof boundary: a source-Gateway attempt through the worktree TS launcher did not open a listening socket; no Gateway claim is made from that attempt. The PR's hosted Real behavior proof was green on the prior exact head; fresh exact-head hosted CI is running after the test-only cleanup.
  • Duplicate search: no competing restore fix found. fix(markdown-core): use Object.hasOwn instead of in operator in parseFrontmatterBlock #99129 and fix(config): use Object.hasOwn instead of in operator in unsetConfigValueAtPath #99148 address the same JavaScript operator pitfall in different owner functions and are not duplicates of this PR.

No config shape, persisted data, protocol, docs, or migration surface changes.

@github-actions github-actions Bot added the dependencies-changed PR changes dependency-related files label Jul 3, 2026
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: discord Channel integration: discord channel: imessage Channel integration: imessage channel: line Channel integration: line channel: matrix Channel integration: matrix channel: msteams Channel integration: msteams channel: signal Channel integration: signal channel: slack Channel integration: slack channel: telegram Channel integration: telegram channel: voice-call Channel integration: voice-call channel: whatsapp-web Channel integration: whatsapp-web channel: zalo Channel integration: zalo channel: zalouser Channel integration: zalouser app: android App: android app: ios App: ios app: web-ui App: web-ui gateway Gateway runtime extensions: diagnostics-otel Extension: diagnostics-otel extensions: memory-core Extension: memory-core labels Jul 3, 2026
@github-actions

github-actions Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Dependency graph guard cleared

This PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh /allow-dependencies-change comment after the guard blocks that new head SHA.

  • Current SHA: d5700c0185d751a3b909d5bc4d0a75f5e96d2276

@steipete
steipete force-pushed the fix/prototype-pollution-redact-snapshot branch from c8ed81f to d5700c0 Compare July 3, 2026 10:24
@openclaw-barnacle openclaw-barnacle Bot removed docs Improvements or additions to documentation channel: discord Channel integration: discord channel: imessage Channel integration: imessage labels Jul 3, 2026
@steipete

steipete commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Jul 3, 2026
Open
This was referenced Jul 4, 2026
Closed
Merged
Closed
@zenglingbiao zenglingbiao mentioned this pull request Jul 4, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jul 4, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zenglingbiao @knoal @steipete