Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

erights
Copy link
Contributor

@erights erights commented Oct 13, 2022

JavaScript has a misfeature often called the "override mistake". In an assignment such as

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
#70
Agoric/agoric-sdk#6451

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
Copy link
Member

@ljharb ljharb left a 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.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Nov 7, 2022

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 configurable because that's all you asked for. Should I also add writable and enumerable?

@erights erights requested a review from ljharb November 7, 2022 03:53
@ljharb
Copy link
Member

ljharb commented Nov 7, 2022

Nah this seems sufficient to me, nobody should be monkeying with it anyways.

@erights
Copy link
Contributor Author

erights commented Nov 7, 2022

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?

@ljharb
Copy link
Member

ljharb commented Nov 7, 2022

That looks unrelated - specifically, it's expecting a forward slash but receiving a backslash on Windows. It probably needs to use path.sep or path.join() instead of a string with /, but that shouldn't block this PR.

@erights
Copy link
Contributor Author

erights commented Nov 8, 2022

but that shouldn't block this PR.

Great, thanks! In that case, what's the ETA on merging this PR? Is it waiting on anything else?

@ljharb
Copy link
Member

ljharb commented Nov 8, 2022

Yes - I'm waiting for one of the regular contributors to this repo to review it.

@ljharb ljharb requested review from bcoe and isaacs November 8, 2022 00:47
@isaacs isaacs self-assigned this Nov 8, 2022
@isaacs isaacs closed this in 0e170d3 Nov 8, 2022
@isaacs
Copy link
Member

isaacs commented Nov 8, 2022

Landed and published. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants