fix(config): compare prototype key guards literally so analysis can see them#1425
Conversation
The guards added in #1415 are effective — Object.prototype is never touched — but they sit at the top of the function and delegate to a Set lookup, which CodeQL cannot follow. Three prototype-pollution alerts stayed open on src/core/config-schema.ts after that PR merged, on exactly the code it hardened. Each key segment is now compared literally in the loop that performs the write. Same behavior for every input, including the empty path and nested creation; the check is simply local to the danger and visible to a reader and to the analyzer. Also tightens a feedback test that matched the issue URL with a substring, which CodeQL rated high. It now parses the URL and compares origin and pathname, so a lookalike host cannot satisfy the assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNested configuration writes and deletes now perform inline unsafe-key checks before mutation, with regression coverage for unchanged objects and prototype safety. The feedback output test parses the logged URL and validates its exact GitHub origin and pathname. ChangesConfiguration guards
Feedback URL validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/config-schema.ts`:
- Around line 138-154: Update the path-writing logic around the key iteration in
src/core/config-schema.ts:138-154 to preflight every segment for __proto__,
constructor, and prototype before creating or mutating intermediate objects,
while retaining the inline write-site guard for static analysis. Update
.changeset/analyzer-visible-key-guards.md:5-7 only after this change so its
“behavior is unchanged” claim accurately reflects the removal of partial
mutations.
- Around line 169-186: Update the final-key branch of deleteNestedValue to
verify that key is an own property of current before deleting and returning
true; inherited properties such as toString must return false without reporting
a deletion. Preserve the existing handling for missing keys and nested
traversal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 588a2285-790d-4e08-a904-91bac03c10fe
📒 Files selected for processing (3)
.changeset/analyzer-visible-key-guards.mdsrc/core/config-schema.tstest/commands/feedback.test.ts
| for (let i = 0; i < keys.length; i++) { | ||
| const key = keys[i]; | ||
| if (key === '__proto__' || key === 'constructor' || key === 'prototype') { | ||
| return false; | ||
| } | ||
|
|
||
| if (i === keys.length - 1) { | ||
| if (key in current) { | ||
| delete current[key]; | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| if (current[key] === undefined || current[key] === null || typeof current[key] !== 'object') { | ||
| return false; | ||
| } | ||
| current = current[key] as Record<string, unknown>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check for an own property before reporting deletion success.
key in current includes inherited properties. For example, deleteNestedValue({}, 'toString') returns true even though no configuration key was removed, causing src/commands/config.ts to save unchanged data and report a successful unset.
Proposed fix
- if (key in current) {
+ if (Object.prototype.hasOwnProperty.call(current, key)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (let i = 0; i < keys.length; i++) { | |
| const key = keys[i]; | |
| if (key === '__proto__' || key === 'constructor' || key === 'prototype') { | |
| return false; | |
| } | |
| if (i === keys.length - 1) { | |
| if (key in current) { | |
| delete current[key]; | |
| return true; | |
| } | |
| return false; | |
| } | |
| if (current[key] === undefined || current[key] === null || typeof current[key] !== 'object') { | |
| return false; | |
| } | |
| current = current[key] as Record<string, unknown>; | |
| for (let i = 0; i < keys.length; i++) { | |
| const key = keys[i]; | |
| if (key === '__proto__' || key === 'constructor' || key === 'prototype') { | |
| return false; | |
| } | |
| if (i === keys.length - 1) { | |
| if (Object.prototype.hasOwnProperty.call(current, key)) { | |
| delete current[key]; | |
| return true; | |
| } | |
| return false; | |
| } | |
| if (current[key] === undefined || current[key] === null || typeof current[key] !== 'object') { | |
| return false; | |
| } | |
| current = current[key] as Record<string, unknown>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/config-schema.ts` around lines 169 - 186, Update the final-key
branch of deleteNestedValue to verify that key is an own property of current
before deleting and returning true; inherited properties such as toString must
return false without reporting a deletion. Preserve the existing handling for
missing keys and nested traversal.
The first attempt at making the guard analyzer-visible moved the check
into the write loop, which changed behavior: a path whose unsafe segment
came after a safe one created the intermediate objects for the safe prefix
before bailing out. `setNestedValue({a:'x'}, 'b.constructor.c', v)` left
behind `b: {}` where the previous implementation wrote nothing.
A differential run against the implementation on main caught it — 47,782
mismatches in 400,000 cases. The literal comparisons stay, but they now
scan the whole path before any mutation, so a rejected key leaves the
object untouched. Re-run of the same comparison: 0 mismatches.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The implementation fixes the partial-write regression, but the committed tests do not guard it: I moved the unsafe-segment check back into the write loop, reproduced { a: 'x', b: {} } from setNestedValue({ a: 'x' }, 'b.constructor.c', 'v'), and all 78 config-schema tests still passed. Please add a focused regression asserting that a later unsafe segment leaves a prepopulated target completely unchanged; then this is ready to approve.
The fix landed without a test that would fail if the guard moved back into the write loop, so the regression could return unnoticed. The existing prototype-pollution tests only assert Object.prototype, and every path they use starts with an unsafe segment on an empty object, so no intermediate object is created before the guard trips. Adds cases that put the unsafe segment after a safe one and assert the whole target, plus one for a trailing unsafe segment, where the debris is a re-parented prototype rather than an extra key and a structural comparison alone would miss it. Verified by reintroducing the regression: 5 of the new assertions fail against the buggy build and pass against the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Approved at exact head 3ade52b. The new late-unsafe-segment cases pin the no-partial-mutation contract: the exact earlier write-loop implementation now fails four focused assertions, while the fixed code passes all 107 config/feedback tests and the full exact-head CI, security, and CodeQL matrix.
2 participants
Status: Follow-up to #1415, found while sanity-checking
mainafter the merge.What was wrong
#1415 guarded the config key helpers, and the guards work —
Object.prototypeis never touched. But the check delegates to a helper doing aSetlookup, which CodeQL cannot follow as a sanitizer.The result: three prototype-pollution alerts stayed open on
src/core/config-schema.tsafter #1415 merged, pointing at the exact code that PR hardened. Anyone reading the security dashboard sees "prototype pollution in shipped source" — the thing we told #1414 was fixed.How it was fixed
The segments are compared literally, in the same function:
The check still covers the whole path before anything is written, so a rejected key leaves the object untouched.
Also tightens
test/commands/feedback.test.ts, which matched the issue URL with.includes(...)— CodeQL rated that high. It now parses the URL and comparesoriginandpathname, so a lookalike host cannot satisfy the assertion.A regression caught in review
The first attempt moved the guard into the write loop. That changed behavior: a path whose unsafe segment follows a safe one created intermediate objects for the safe prefix before bailing.
A differential run against the implementation on
maincaught it: 47,782 mismatches in 400,000 cases. The current form scans the whole path first and re-runs clean.Proof
main__proto__.polluted,constructor.prototype.x,featureFlags.__proto__,prototype.yNotes
js/shell-command-injection-from-environmentalerts remain, all intest/whereexecSyncinterpolates a constant test path. They do not ship. Converting them toexecFileSyncis mechanical but touches many call sites, so it deserves its own PR.🤖 Generated with Claude Code
Summary by CodeRabbit
https://github.com/Fission-AI/OpenSpec/issues/newURL origin/path.