Skip to content

fix(config): use Object.hasOwn instead of in operator in unsetConfigValueAtPath#99826

Closed
zenglingbiao wants to merge 4 commits into
openclaw:mainfrom
zenglingbiao:fix/prototype-pollution-config-paths
Closed

fix(config): use Object.hasOwn instead of in operator in unsetConfigValueAtPath#99826
zenglingbiao wants to merge 4 commits into
openclaw:mainfrom
zenglingbiao:fix/prototype-pollution-config-paths

Conversation

@zenglingbiao

@zenglingbiao zenglingbiao commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Use Object.hasOwn() instead of the in operator in unsetConfigValueAtPath() to prevent prototype chain pollution when checking whether a config key exists before deletion.

What Problem This Solves

When calling unsetConfigValueAtPath(root, ["foo", "toString"]) on a config object that does not have a toString key, the function returns true (claiming success) because "toString" in cursor matches Object.prototype.toString.

The function lies to its caller — it reports "key found and deleted" when the key never existed. This can cause config management tools (CLI config unset, config merge logic) to silently skip error handling for non-existent keys.

Code evidence: In src/config/config-paths.ts:63, if (!(leafKey in cursor)) checks the prototype chain. cursor is a plain PathNode object (Record<string, unknown>), and leafKey comes from user-provided dot-notation paths like foo.toString.

Root Cause

  • The in operator checks the entire prototype chain, not just own properties
  • Object.hasOwn() (ES2022+) correctly checks only the object's own properties
  • Object.hasOwn is already used 15+ times across the codebase — this was a missed instance

Why This Fix

Evidence

  • Before: /config unset foo.toString reports "Config value unset" even though toString never existed as a config key — the in operator finds Object.prototype.toString
  • After: /config unset foo.toString correctly reports "No config value found"
  • Root cause: if (!(leafKey in cursor)) at config-paths.ts:63 traverses the prototype chain; "toString" in {} is true
  • All 4 prototype keys tested: toString, constructor, valueOf, hasOwnProperty — all correctly return false
  • Existing own-key deletion and parent pruning: unaffected

Real Behavior Proof

Before (on main)

$ node --import tsx proof.mjs
FAIL non-existent "toString" returns false (got: true, owns "toString": false)
FAIL non-existent "constructor" returns false (got: true, owns "constructor": false)
FAIL non-existent "valueOf" returns false (got: true, owns "valueOf": false)
FAIL non-existent "hasOwnProperty" returns false (got: true, owns "hasOwnProperty": false)
PASS existing "toString" deleted successfully
PASS "toString" no longer own property
PASS sibling key "bar" preserved
PASS parent "foo" preserved after no-op unset
PASS sibling "bar" preserved after no-op unset

4 FAILED

After (with fix)

$ node --import tsx proof.mjs
PASS non-existent "toString" returns false (got: false, owns "toString": false)
PASS non-existent "constructor" returns false (got: false, owns "constructor": false)
PASS non-existent "valueOf" returns false (got: false, owns "valueOf": false)
PASS non-existent "hasOwnProperty" returns false (got: false, owns "hasOwnProperty": false)
PASS existing "toString" deleted successfully
PASS "toString" no longer own property
PASS sibling key "bar" preserved
PASS parent "foo" preserved after no-op unset
PASS sibling "bar" preserved after no-op unset

ALL PASS

Tests and Validation

  • Regression test added: src/config/config-paths.test.ts covers 4 prototype keys + own-key deletion + parent pruning
  • Fix verified with standalone node --import tsx proof script (see above)
  • Focused vitest proof: node scripts/run-vitest.mjs src/config/config-paths.test.ts — 1 file, 6 tests passed
  • Diff: 1 line changed in source + regression test
  • Proof: 4 prototype keys (toString/constructor/valueOf/hasOwnProperty) all change from FAIL → PASS, regression tests pass

@clawsweeper

clawsweeper Bot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 1:46 AM ET / 05:46 UTC.

Summary
The PR replaces the leaf in check in unsetConfigValueAtPath with Object.hasOwn and adds regression tests for prototype-named config keys.

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

Reproducibility: yes. Current main source plus a read-only Node expression show the helper can treat inherited names such as toString as present even when they are not own config keys.

Review metrics: none identified.

Root-cause cluster
Relationship: canonical
Canonical: #99826
Summary: This PR is the active canonical fix for the unsetConfigValueAtPath inherited-key false positive; related items either duplicate this helper change or fix the same pattern elsewhere.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

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.

Next step before merge

  • [P2] No repair lane is needed; this PR already contains the focused fix and maintainer review or merge is the remaining action.

Security
Cleared: The diff narrows an object-key existence check and adds tests; it does not touch dependencies, workflows, credentials, or package resolution.

Review details

Best possible solution:

Land the narrow helper change with regression tests after normal maintainer review and required checks.

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

Yes. Current main source plus a read-only Node expression show the helper can treat inherited names such as toString as present even when they are not own config keys.

Is this the best way to solve the issue?

Yes. Fixing the shared helper's leaf existence check is narrower than caller-side special cases and matches sibling own-property config path helpers.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded config-helper correctness fix with limited blast radius and direct proof.
  • 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 (live_output): The PR body includes before/after terminal output from a real node --import tsx proof showing inherited-key false positives become false after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes before/after terminal output from a real node --import tsx proof showing inherited-key false positives become false after the fix.
Evidence reviewed

PR surface:

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

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

What I checked:

  • Repository policy read: Read the full root AGENTS.md and applied the config-sensitive review guidance; no scoped AGENTS.md exists under src/config. (AGENTS.md:1, 1b84316a91dc)
  • Current main root cause: Current main still checks if (!(leafKey in cursor)), so inherited names can be treated as present before deletion. (src/config/config-paths.ts:63, 1b84316a91dc)
  • User-visible caller: /config unset delegates to unsetConfigPath, and a false return is the path that produces the user-facing no-value-found response. (src/auto-reply/reply/config-mutations.ts:41, 1b84316a91dc)
  • Sibling own-property pattern: Strict config path deletion in the secrets path utility already uses Object.hasOwn for object leaf deletion, supporting the same fix shape here. (src/secrets/path-utils.ts:225, 1b84316a91dc)
  • Runtime behavior proof: A read-only Node expression confirmed toString in {bar:'value'} is true while Object.hasOwn is false, matching the PR's claimed false positive.
  • Latest release still has the bug: Tag v2026.6.11 still contains the same leafKey in cursor check, so this is not already shipped. (src/config/config-paths.ts:63, e085fa1a3ffd)

Likely related people:

  • steipete: GitHub commit metadata maps the original /config chat config update that introduced unsetConfigValueAtPath to this login. (role: introduced behavior; confidence: high; commits: 8b579c91a555; files: src/config/config-paths.ts, src/auto-reply/reply/config-mutations.ts, src/auto-reply/reply/commands-config.ts)
  • vincentkoc: Recent merged config command gating work touched the /config command surface that consumes this helper. (role: recent adjacent contributor; confidence: medium; commits: 08aa57a3de37; files: src/auto-reply/reply/commands-config.ts, src/auto-reply/reply/command-gates.ts)
  • zenglingbiao: Beyond this PR, this contributor authored a merged adjacent config own-property fix for restoreOriginalValueOrThrow. (role: recent adjacent contributor; confidence: medium; commits: 459b93e3ddaf; files: src/config/redact-snapshot.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: 🐚 platinum hermit Good normal PR readiness with ordinary 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 4, 2026
@zenglingbiao

zenglingbiao commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 4, 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.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 4, 2026
@zenglingbiao zenglingbiao mentioned this pull request Jul 4, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jul 4, 2026
Merged
@vincentkoc

vincentkoc commented Jul 4, 2026

Copy link
Copy Markdown
Member

Superseded by #99846, landed on main as 29814252e8e34f4bf9ecebf942d76e104cdb102b.

The landed repair preserves @zenglingbiao as commit author and completes the shared invariant across getConfigValueAtPath, setConfigValueAtPath, and unsetConfigValueAtPath. It also covers inherited parent/leaf data properties, accessors, and non-writable descriptors.

Final proof: 106 focused tests passed, Testbox pnpm check:changed passed, fresh autoreview was clean, and exact-head hosted CI completed with zero failures.

Closing this narrower duplicate in favor of the landed canonical repair.

@vincentkoc vincentkoc closed this Jul 4, 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

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.

2 participants

@zenglingbiao @vincentkoc