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

Patch bug in stack-utils npm package #6451

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Oct 13, 2022

Lockdown breaks Ava under certain versions of NodeJS. It turns out to be due to a bug in the stack-utils npm package which 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.

It is unclear which version of Node starts manifesting the problem, but at least v16.16.0 has it. The problem manifests as Ava test failures being reported unhelpfully -- whereas one should get a stack backtrace from the point in the test file where the failure happened, usually along with a nice diff between expected and actual result values from things like t.deepEqual, instead you get an "Unable to serialize error" exception with a stack trace from somewhere deep in the bowels of Ava. The problem was the stack-utils npm package (which Ava uses) failing a property assignment to Object.prototype (which should have been a simple property assignment to an ordinary property of an ordinary object but wasn't because of the override mistake) in an environment where Object.prototype is frozen because of lockdown.

Without this patch (or, the eventual hoped for fix to the stack-utils npm package itself), debugging Ava test failures is grossly inhibited if you are using one of the newer versions of Node that manifest the problem.

See tapjs/stack-utils#70

@FUDCo FUDCo added the bug Something isn't working label Oct 13, 2022
@FUDCo FUDCo requested a review from kriskowal October 13, 2022 21:54
@FUDCo FUDCo self-assigned this Oct 13, 2022
erights added a commit to erights/stack-utils that referenced this pull request Oct 13, 2022
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
Copy link
Member

erights commented Oct 13, 2022

PR for stack-utils now at tapjs/stack-utils#71

(If they don't respond to an issue, they often will to a pull request. We'll see)

@erights
Copy link
Member

erights commented Oct 13, 2022

See also endojs/endo#576 (comment)

@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label Oct 18, 2022
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
@mergify mergify bot merged commit 5e5ae42 into master Oct 18, 2022
@mergify mergify bot deleted the patch-for-stack-utils-bug branch October 18, 2022 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants