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

Cannot redefine a property on vm's globalThis #47799

Closed
domenic opened this issue May 1, 2023 · 2 comments · Fixed by #51602
Closed

Cannot redefine a property on vm's globalThis #47799

domenic opened this issue May 1, 2023 · 2 comments · Fixed by #51602
Assignees
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@domenic
Copy link
Contributor

domenic commented May 1, 2023

Version

v20.0.0

Platform

Microsoft Windows NT 10.0.22621.0 x64

Subsystem

vm

What steps will reproduce the bug?

const vm = require("vm");
const context = vm.createContext();

const window = vm.runInContext("this", context);

Object.defineProperty(window, "x", { value: "1", configurable: true });
Object.defineProperty(window, "x", { value: "2", configurable: true });

console.log(window.x);

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Outputs "2"

What do you see instead?

Outputs "1"

Additional information

This reproduces in every version I've tested back to v16.20.0.

This does not reproduce if you replace window in the above with context, i.e. it's specific to what happens when you pull out the inner globalThis value. (However, pulling out that value is crucial for jsdom's use cases.)

@targos
Copy link
Member

targos commented May 1, 2023

@nodejs/vm

@targos targos added the vm Issues and PRs related to the vm subsystem. label May 1, 2023
@fhinkel
Copy link
Member

fhinkel commented Jan 29, 2024

My gut feeling is that this is an upstream bug.

As a work around, adding writable: true in addition to configurable results in the expected updated value for window.x.

@fhinkel fhinkel self-assigned this Jan 30, 2024
fhinkel added a commit to fhinkel/node that referenced this issue Jan 30, 2024
fhinkel added a commit to fhinkel/node that referenced this issue Jan 30, 2024
fhinkel added a commit to fhinkel/node that referenced this issue Jan 30, 2024
Object.defineProperty allows to change the value for non-writable
properties if they are configurable. We missed that case when checking if a
property is read-only.

Fixes: nodejs#47799
fhinkel added a commit to fhinkel/node that referenced this issue Jan 30, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: nodejs#47799
fhinkel added a commit to fhinkel/node that referenced this issue Jan 31, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: nodejs#47799
nodejs-github-bot pushed a commit that referenced this issue Feb 1, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: #47799
PR-URL: #51602
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Feb 9, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: nodejs#47799
PR-URL: nodejs#51602
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Feb 15, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: #47799
PR-URL: #51602
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: nodejs#47799
PR-URL: nodejs#51602
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: #47799
PR-URL: #51602
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Object.defineProperty allows to change the value for
non-writable properties if they are configurable. We
missed that case when checking if a
property is read-only.

Fixes: #47799
PR-URL: #51602
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants