fix(config): tolerate non-cloneable values when cloning config - #1771
fix(config): tolerate non-cloneable values when cloning config#1771danielroe wants to merge 1 commit into
Conversation
commit: |
📝 WalkthroughWalkthroughThis change adds Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 1
🤖 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/utils.ts`:
- Around line 90-94: Update the deep-copy logic around the copy target to
preserve the input object's prototype by creating it with Object.create(input's
prototype). Replace the for-in iteration with Object.keys(input), recursively
copy each own property, and define properties on the target so an own __proto__
key remains data rather than changing the prototype.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bf8d4d8-ceda-416f-aa61-9b0dbfe8ca9d
📒 Files selected for processing (3)
src/config.tssrc/utils.tstest/unit/config.spec.ts
| const copy: Record<string, any> = {} | ||
| seen.set(input, copy) | ||
| for (const key in input) { | ||
| copy[key] = deepCopy((input as Record<string, any>)[key], seen) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve null prototypes and own __proto__ keys.
Line 90 always creates {}. Line 93 assigns an own __proto__ key through the inherited setter. A null-prototype object therefore changes prototype, and a JSON-derived __proto__ property is not copied as data.
Create the target with Object.create(proto). Iterate own keys with Object.keys. Define each property instead of assigning it.
Proposed fix
- const copy: Record<string, any> = {}
+ const copy: Record<string, any> = Object.create(proto)
seen.set(input, copy)
- for (const key in input) {
- copy[key] = deepCopy((input as Record<string, any>)[key], seen)
+ for (const key of Object.keys(input as Record<string, any>)) {
+ Object.defineProperty(copy, key, {
+ value: deepCopy((input as Record<string, any>)[key], seen),
+ enumerable: true,
+ writable: true,
+ configurable: true,
+ })
}🤖 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/utils.ts` around lines 90 - 94, Update the deep-copy logic around the
copy target to preserve the input object's prototype by creating it with
Object.create(input's prototype). Replace the for-in iteration with
Object.keys(input), recursively copy each own property, and define properties on
the target so an own __proto__ key remains data rather than changing the
prototype.
1 participant
🔗 Linked issue
npmx-dev/npmx.dev#3094
overlaps with nuxt/nuxt#35574
📚 Description
this should be a bit safer