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

vm: harden module type checks #52162

Merged
merged 1 commit into from
Mar 22, 2024
Merged

vm: harden module type checks #52162

merged 1 commit into from
Mar 22, 2024

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 20, 2024 •
edited
Loading

Check if the value returned from user linker function is a null-ish
value.

validateInternalField should be preferred when checking this
argument to guard against null-ish this.

Co-authored-by: Mike Ralphson mike.ralphson@gmail.com

Replaces #47583

@legendecas @MikeRalphson
vm: harden module type checks
11bdfcd 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <mike.ralphson@gmail.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Mar 20, 2024
H4ad
H4ad approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be marked as minor since is changing the error?

@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57867/

@legendecas legendecas added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 21, 2024
@anonrig
Copy link
Member

anonrig commented Mar 21, 2024 •
edited
Loading

I think this is a semver-major since the error code is changing.

@anonrig anonrig added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 21, 2024
@legendecas
Copy link
Member Author

legendecas commented Mar 21, 2024 •
edited
Loading

@anonrig I think this is a semver-major since the error code is changing.

Personally, I would not consider changing errors thrown from Object.getOwnPropertyDescriptor(SourceTextModule.prototype, 'status').get.call(undefined) to be a breaking change, as long as the error type is TypeError to indicate it is an invalid API call in development. For instance, Web APIs that perform strict this checks only throw a TypeError (not even a DOMException), e.g. Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call(undefined).

The code of the error thrown from module.link(() => notModule) is not changed in this PR, which I would find changing error code could be a breaking change.

@legendecas legendecas removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 21, 2024
@targos
Copy link
Member

targos commented Mar 21, 2024

I think this is a semver-major since the error code is changing.

I disagree. vm modules are experimental.

@anonrig anonrig added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 21, 2024
@legendecas legendecas added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 22, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 22, 2024
@nodejs-github-bot nodejs-github-bot merged commit d1d5da2 into nodejs:main Mar 22, 2024
78 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d1d5da2

@legendecas legendecas deleted the module/brand-check branch March 22, 2024 09:48
anonrig pushed a commit to anonrig/node that referenced this pull request Mar 25, 2024
@legendecas @MikeRalphson @anonrig
vm: harden module type checks
b86113a 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <mike.ralphson@gmail.com>
PR-URL: nodejs#52162
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@legendecas @MikeRalphson @rdw-msft
vm: harden module type checks
7b0ba4f 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <mike.ralphson@gmail.com>
PR-URL: nodejs#52162
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
Merged
@marco-ippolito
Copy link
Member

Can you create a manual backport for v20? It doesn't land cleanly

@marco-ippolito marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label May 21, 2024
@legendecas legendecas mentioned this pull request May 22, 2024
Open
@legendecas
Copy link
Member Author

Submitted #53109

@legendecas legendecas added the backport-open-v20.x Indicate that the PR has an open backport label May 22, 2024
marco-ippolito pushed a commit to legendecas/node that referenced this pull request Jun 12, 2024
@legendecas @marco-ippolito
vm: harden module type checks
f9c85f3 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <mike.ralphson@gmail.com>
PR-URL: nodejs#52162
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
# Conflicts:
#	lib/internal/vm/module.js
legendecas added a commit to legendecas/node that referenced this pull request Jun 18, 2024
@legendecas @MikeRalphson
vm: harden module type checks
5081f84 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <mike.ralphson@gmail.com>
PR-URL: nodejs#52162
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jun 18, 2024
@legendecas @MikeRalphson @joyeecheung
vm: harden module type checks
fc4db31 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <mike.ralphson@gmail.com>
PR-URL: nodejs#52162
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@H4ad H4ad H4ad approved these changes

Assignees
No one assigned
Labels
backport-open-v20.x Indicate that the PR has an open backport backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@legendecas @nodejs-github-bot @anonrig @targos @marco-ippolito @H4ad