Fix diffJson silently dropping an own __proto__ key (#696) - #697
Conversation
canonicalize built its output with `{}`, so assigning a canonicalized
value to an own "__proto__" key invoked the Object.prototype __proto__
setter instead of creating a data property. The key was dropped, and
diffJson reported two objects that differ only in their __proto__
property as identical (a single unchanged "{}" hunk).
Build the canonicalized object with Object.create(null) so a "__proto__"
key is stored as an ordinary property. Add a regression test covering
diffJson over objects parsed from JSON with an own __proto__ property.
Fixeskpdecker#696There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:392ff6b95c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
ExplodingCabbage
left a comment
There was a problem hiding this comment.
Thank you!
Looks correct, comments are clear, and I have no problem with the AI co-authorship so long as it's disclosed and the substance of the PR is sound.
Doesn't seem to me like there are any other places in the codebase where similar changes are needed (based on a quick grep for {}).
I'll merge.
Fixes#696.
Problem
diffJsonreports two objects that differ only in an own__proto__property as identical:Root cause
canonicalize(src/diff/json.ts) builds its output object with{}. That object inheritsObject.prototype, socanonicalizedObj["__proto__"] = valueinvokes the__proto__setter instead of creating an own data property. The key is silently discarded, both sides canonicalize to{}, anddiffJsonsees no difference. Objects parsed from JSON (e.g.JSON.parse) can legitimately carry an own enumerable__proto__property, so this is silent data loss.Fix
Build the canonicalized object with
Object.create(null), so a"__proto__"key is stored as an ordinary property. This is the approach suggested (unverified) by the reporter in #696; this PR verifies it and adds test coverage.canonicalizeoutput is only everJSON.stringify-d or key-walked, both of which work on null-prototype objects, and the existing#canonicalizetests (which readObject.keys(...)) still pass.Test
Added a regression test in
test/diff/json.jsdiffing two JSON-parsed objects with an own__proto__property. Red-green verified: before the fix the assertion fails (single unchanged"{}"hunk); after, it produces the expected removed/added__proto__lines. The full mocha suite passes (one unrelated pre-existing timeout on the enormous-hunk perf test intest/patch/create.js, which passes with a higher--timeout);eslintis clean.Disclosure: prepared with AI assistance (Claude); I reviewed it and verified the red-green test and the full test suite.