fix(compartment-mapper): mapNodeModules should not crawl ESM modules from CJS entrypoints

[boneskull]

Description

This changes the logic in mapNodeModules() so that when it reads the package.json of the entrypoint, it checks for type === 'module'. If this is set, then import is added to the conditions. If it is not set, then require is added to the conditions. The conditions always contain default; this has not changed.

This prevents CJS modules from referencing modules in the CompartmentMapDescriptor which they would not otherwise load (and in fact cannot load for some value of "cannot").

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

This may impact users, but it is a bug fix.

Testing Considerations

The tests verify this is working for both module systems.

Compatibility Considerations

n/a

Upgrade Considerations

Updated NEWS.md.

[naugtur]

did you mean dual-cjs?

[boneskull]

idk, doesn't really matter

[naugtur]

I've created an exploratory e2e test to see how the nested trees of cjs vs esm would work

#3006

there's a few more permutations to try.

[naugtur]

Comparing against node.js is one thgin, but do we also want to compare against some bundlers and tsc?

[kriskowal]

For the sake of capturing notes,

I think this is interesting. Node.js, unlike Endo-as-writ, supports two concurrent module systems. The legacy, bottom tier module system is always synchronous and only hosts CJS. The top tier, like Endo, hosts both CJS and ESM. The ramp up is dynamic import, with a fully qualified module specifier (module- and package-relative imports won’t work). The ramp down is modules.createRequire, which contextualizes module- and package relative imports to a fully qualified module specifier (import.meta.url). Endo isn’t in a great position for implementing either of these ramps because portability demands neutrality about fully qualified URLs, but we’ve discussed ways to bolt these features on the side for LavaMoat.

I believe the top tier always has import and the bottom tier always has require conditions. We could conceivably create parallel compartments for the tiers, but I don’t like what that implies about eval twin hazards. I’d much go where the puck is headed: ESM and CJS coëxisting on the top tier. I could also see tacking on a legacy mode where there’s only the bottom tier (require and CJS only), and that could be another mapNodeModules variant, or we could use an 'auto-entry' mode to sense which tier to use based on the module type of the entry, like Node.js.

[boneskull]

Yeah, based on talking it over with @kriskowal and his comments here, I am not really convinced this is "correct", nor am I convinced it actually "fixes" anything. The current behavior seems kind of weird to me, so I think Something should be done. This is likely not the Something that should be done.

[naugtur]

This PR doesn't address the issue identified here:
#3055

It does go from an error Unexpected import statement in CJS module to unexpected end of input tho, so maybe it's close.