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.

Sign up for GitHub

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

Jump to bottom

Avoid property override hazard #70

Open
FUDCo opened this issue Sep 28, 2022 · 1 comment
Open

Avoid property override hazard #70

FUDCo opened this issue Sep 28, 2022 · 1 comment

Comments

@FUDCo
Copy link

FUDCo commented Sep 28, 2022

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch stack-utils@2.0.5 for the project I'm working on.

Assignments to res.constructor trip over the JavaScript property override mistake, causing Ava test assertion failure results in some environments to become unable to be output. The problem can be reproduced if you freeze Object.prototype.

Here is the diff that solved my problem:

diff --git a/node_modules/stack-utils/index.js b/node_modules/stack-utils/index.js
index ed14bd3..ad9eeb1 100644
--- a/node_modules/stack-utils/index.js
+++ b/node_modules/stack-utils/index.js
@@ -161,7 +161,7 @@ class StackUtils {
     setFile(res, site.getFileName(), this._cwd);
 
     if (site.isConstructor()) {
-      res.constructor = true;
+      Object.defineProperty(res, 'constructor', { value: true });
     }
 
     if (site.isEval()) {
@@ -260,7 +260,7 @@ class StackUtils {
     setFile(res, file, this._cwd);
 
     if (ctor) {
-      res.constructor = true;
+      Object.defineProperty(res, 'constructor', { value: true });
     }
 
     if (evalOrigin) {

This issue body was partially generated by patch-package.
@FUDCo FUDCo mentioned this issue Sep 28, 2022
Closed
FUDCo added a commit to Agoric/agoric-sdk that referenced this issue Oct 13, 2022
@FUDCo
fix: patch bug in stack-utils npm package
42c2324 
Lockdown breaks Ava under certain versions of NodeJS.  It turns out to be due to
a bug in the `stack-utils` npm package with trips over the override mistake.
I've filed an issue with the maintainer of that package, but in the meantime
this patch deals with the problem.

See tapjs/stack-utils#70
@FUDCo FUDCo mentioned this issue Oct 13, 2022
Merged
FUDCo added a commit to Agoric/agoric-sdk that referenced this issue Oct 13, 2022
@FUDCo
fix: patch bug in stack-utils npm package
8436476 
Lockdown breaks Ava under certain versions of NodeJS.  It turns out to be due to
a bug in the `stack-utils` npm package with trips over the override mistake.
I've filed an issue with the maintainer of that package, but in the meantime
this patch deals with the problem.

See tapjs/stack-utils#70
@erights erights mentioned this issue Oct 13, 2022
Closed
erights added a commit to erights/stack-utils that referenced this issue Oct 13, 2022
@erights
Fix to be compat with the JS override mistake
d5db5ee 
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
@erights erights mentioned this issue Oct 13, 2022
Closed
@erights
Copy link
Contributor

erights commented Oct 13, 2022

PR now at #71
Please review

turadg pushed a commit to Agoric/agoric-sdk that referenced this issue Oct 18, 2022
@FUDCo @mergify
fix: patch bug in stack-utils npm package
7eceb12 
Lockdown breaks Ava under certain versions of NodeJS.  It turns out to be due to
a bug in the `stack-utils` npm package with trips over the override mistake.
I've filed an issue with the maintainer of that package, but in the meantime
this patch deals with the problem.

See tapjs/stack-utils#70
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@erights @FUDCo