-
-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Fix to be compat with the JS override mistake #71
Conversation
JavaScript has a misfeature often called the "override mistake". In an assignment such as ```js res.constructor = true; ``` if `res` does not yet have its own `constructor` property, but inherits one that this assignment would override (as is the intention here), but the property that would be overridden is a non-writable data property, then the assignment fails. Hardened JS and similar frameworks for securing JS routinely freeze all the primordial objects, which causes their data properties to become non-configurable, non-writable. Also, the TC53 JS standard for embedded devices standardizes on Hardened JS, which will also cause this problem. The XS JS engine for embedded devices use the Hardened JS configuration by default on embedded devices. Object literals and classes override inherited properties without problem because they use JS's "define" semantics rather than JS's peculiar "assign" semantics. You can also do so manually via `Object.defineProperty`, as this PR does to repair this issue. See also tapjs#70 Agoric/agoric-sdk#6451
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the package aready requires node 10, this should be fine.
It'd be awesome to run tests with SES as well, to prevent regressions.
Hi @ljharb , sorry this took so long. I agree that adding a Hardened JavaScript (aka SES) test would be great. But please let's get this in first. Thanks. Btw, I only added |
Nah this seems sufficient to me, nobody should be monkeying with it anyways. |
All nine failing checks seem to be the same mysterious problem: "no cwd is stripped off". I have no idea what this is about. Could it possibly be related to this PR? |
That looks unrelated - specifically, it's expecting a forward slash but receiving a backslash on Windows. It probably needs to use |
Great, thanks! In that case, what's the ETA on merging this PR? Is it waiting on anything else? |
Yes - I'm waiting for one of the regular contributors to this repo to review it. |
Landed and published. Thanks! |
JavaScript has a misfeature often called the "override mistake". In an assignment such as
if
res
does not yet have its ownconstructor
property, but inherits one that this assignment would override (as is the intention here), but the property that would be overridden is a non-writable data property, then the assignment fails. Hardened JS and similar frameworks for securing JS routinely freeze all the primordial objects, which causes their data properties to become non-configurable, non-writable. Also, the TC53 JS standard for embedded devices standardizes on Hardened JS, which will also cause this problem. The XS JS engine for embedded devices use the Hardened JS configuration by default on embedded devices.Object literals and classes override inherited properties without problem because they use JS's "define" semantics rather than JS's peculiar "assign" semantics. You can also do so manually via
Object.defineProperty
, as this PR does to repair this issue.See also
#70
Agoric/agoric-sdk#6451