Repository navigation
Emit computed property name for __proto__ in generated object literals - #526
Tejas Lamba (TejasLamba2006) wants to merge 1 commit into
Conversation
A JSON object with an own __proto__ field was emitted as a quoted
property name ("__proto__": value), which JavaScript interprets as a
prototype setter instead of a data property. Now emits ["__proto__"]
(a computed property name) for this one key, which correctly defines it
as an own data property. Other keys are unchanged. Fixes microsoft#497.
| it("evaluates an object with __proto__ as an own data property", async () => { | ||
| const program = JSON.parse('{"@steps":[{"@func":"first","@args":[{"__proto__":"value"}]}]}'); | ||
| const result = await evaluateJsonProgram(program, async (_func, args) => args[0]); | ||
| assert.ok(Object.prototype.hasOwnProperty.call(result, "__proto__")); | ||
| assert.equal((result as Record<string, unknown>)["__proto__"], "value"); | ||
| }); |
robgruen
left a comment
There was a problem hiding this comment.
Thanks Tejas Lamba (@TejasLamba2006) for this focused fix. The production change is nicely scoped and looked sound in the local checks.
My belt-and-suspenders testing habit is showing a little here, but could we have the regression tests execute the generated module, rather than only checking its text? The interpreter already preserves this field, so the interpreter-only assertion does not directly cover the code-generation bug.
I'd like to see string, object, and null values covered, checking that the generated result has an own __proto__ data property, preserves the serialized data, and matches the interpreter result. Please keep execution limited to controlled test fixtures; production code should continue generating text as before.
JOJO (@moneymaker26754-crypto), thank you for the earlier work in draft #497, which already includes the same fix and these behavioral regression tests. Could you both coordinate the overlapping contributions? If the work is consolidated into one PR, let's preserve credit for both authors, including the implementation and regression coverage.
Since #497 is itself an open draft PR rather than an issue, "Related to #497" would describe the relationship more accurately than "Fixes #497".
I'm requesting changes for the regression coverage and coordination above, not a larger rewrite. Thanks to you both for working on this.
|
Makes sense to consolidate onto #497 since the same fix is already there. JOJO (@moneymaker26754-crypto) I can add the behavioral regression tests (executing the generated module with string, object, and null proto values) to your branch if that helps, or you can grab them from here. Let me know what works best. Happy to close this one once #497 has the coverage sorted. |
A JSON object with an own
__proto__field was emitted as a quoted property name, which JavaScript interprets as a prototype setter instead of a data property. Now emits["__proto__"](a computed property name) for this one key, which correctly defines it as an own data property. Other keys are unchanged. Fixes #497.