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

lib: fix Module check when monkey patching #50109

Closed
wants to merge 1 commit into from
Closed

lib: fix Module check when monkey patching #50109

wants to merge 1 commit into from

Conversation

RafaelGSS
Copy link
Member

Apparently, the last security release broke the coverage of reports of nyc. I'm still waiting for a reproducible example so I can create a test for that, but I am confident this should fix the problem.

Fixes: istanbuljs/nyc#1530

cc: @bmeck

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 10, 2023
GeoffreyBooth
GeoffreyBooth reviewed Oct 10, 2023
View reviewed changes
lib/internal/modules/helpers.js

/** @type {RequireFunction} */
let require;
if (redirects) {
if (mod instanceof Module !== true) {
throw new ERR_INVALID_ARG_TYPE('mod', 'Module', mod);
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t seem correct; there’s another reference to mod in the else branch farther down; why would we validate the input only for this branch and not that one?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was added specifically to solve 15bced0bde. Since this check didn't exist previously I think it's safe to ignore the other branches.

Copy link
Member

Choose a reason for hiding this comment

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

Well at the very least it looks like a mistake; if I were refactoring this, I would instinctively move the check to the top of the function. There should be a comment explaining why mod is always defined for if (redirects) but only sometimes defined otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

What's missing is a test that would fail if this check was at the top of the function.

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth
Copy link
Member

Longer term, nyc needs to migrate to the module customization hooks rather than monkey patching. We do not and will not support monkey patching Node internals. The hooks are the public API that can enable code coverage.

@github-actions github-actions bot mentioned this pull request Oct 11, 2023
Open
30 tasks
@jeremymeng jeremymeng mentioned this pull request Oct 17, 2023
Open
@jeremymeng
Copy link

Any updates! We don't want to have to migrate to another code coverage tool.

@RafaelGSS
Copy link
Member Author

So, turns out it's not a nyc bug, but an esm one. See: istanbuljs/nyc#1530 (comment)

Please note if you want this to be fixed quickly, consider doing it yourself or sponsoring someone to do so. Also note, that the commit that introduced this behavior was a security fix. There's no way to revert that.

I will try to look at esm source code this week to have this fixed, but it really depends on the bandwidth I'll have.

@jeremymeng
Copy link

@RafaelGSS Thank you so much! This is worse as esm doesn't seem to be actively maintained any more. Appreciate you looking into the issue.

@RafaelGSS
Copy link
Member Author

So, esm is definitely monkey patching makeRequireFunction and Module, and as with any monkey patching that's subject to break due to not using the public API properly.

Considering esm repository is now achieved, I'm tempted to just close this PR. Unfortunately, it has been done in a security release (which per policy can land semver-major).

If someone creates a minimal reproducible code (non-library dependency) I might come up with a solution

@GeoffreyBooth
Copy link
Member

Unfortunately, it has been done in a security release

What does this mean?

Nyc should be able to migrate to the module customization hooks. That's not a quick fix but it's the long term solution.

@RafaelGSS
Copy link
Member Author

What does this mean?

I meant it seems the "patch" would be considered a semver-major PR if that wasn't fixing a vulnerability.

Nyc should be able to migrate to the module customization hooks. That's not a quick fix but it's the long term solution.

:s/nyc/esm

@GeoffreyBooth
Copy link
Member

:s/nyc/esm

Well, you’re saying that esm is unmaintained, but also its entire purpose is to transpile ESM to CommonJS as it goes through the CommonJS loader; it wouldn’t really “migrate” to the module hooks so much as be replaced by them. nyc shouldn’t need esm at all now that it can use the load hook to do its work.

@RafaelGSS
Copy link
Member Author

Well, you’re saying that esm is unmaintained, but also its entire purpose is to transpile ESM to CommonJS as it goes through the CommonJS loader; it wouldn’t really “migrate” to the module hooks so much as be replaced by them. nyc shouldn’t need esm at all now that it can use the load hook to do its work.

nyc isn't the problem here as I said in #50109 (comment). The issue was created using -r esm to run tests (nyc mocha ...) and the reporter (and others) thought that it was a nyc behaviour, but it's not. There's nothing wrong with nyc -- so far.

@RafaelGSS RafaelGSS closed this Oct 20, 2023
@GeoffreyBooth
Copy link
Member

The issue was created using -r esm to run tests

Ah, I think I misunderstood. I thought esm was a dependency of nyc; you’re saying that it wasn’t, that the user was bringing it in explicitly? That changes things.

@jeremymeng
Copy link

nyc isn't the problem here

It could be the interaction between nyc and esm. Without nyc, mocha -r esm runs the test without any error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments

@targos targos targos left review comments

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nyc + esm is broken in latest NodeJS versions
5 participants
@RafaelGSS @nodejs-github-bot @GeoffreyBooth @jeremymeng @targos