lib: fix Module check when monkey patching

[RafaelGSS]

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]

Review requested:

• @nodejs/loaders

[GeoffreyBooth]

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?

[RafaelGSS]

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

[GeoffreyBooth]

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.

[targos]

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

[nodejs-github-bot]

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

[GeoffreyBooth]

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.

[jeremymeng]

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

[RafaelGSS]

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]

@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]

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]

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]

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]

: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]

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.

[GeoffreyBooth]

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]

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.