lib,src: drop --experimental-network-imports

[RafaelGSS]

This PR reverts (not cleanly) #36328 and consequently, the --expiremental-network-import is removed.

In the past month, we have been assessing several experimental features. Recently, we have received reports regarding issues with the Network Import feature. Unfortunately, this feature is lacking a champion, and the documentation is not clear enough to define security expectations or boundaries. This makes it difficult to review the reports, as we are unsure of how to assess them.

I want to emphasize that this doesn't mean Node.js does not support the exploration of http imports. I will quote @joyeecheung as:

"The security model it's based on (no access to other builtins to run untrusted code) is just not going to work out in the current Node.js architecture without a huge amount of refactoring, vetting and gatekeeping that nobody is doing and nobody seems to be committed to do either
Support for http import alone isn't really a problem, but that particular security model is just not realistic to implement or maintain on top of the existing code; The only way you could do it is to rewrite Node.js from scratch, then that's Deno
Even (redacted) leaks a stream that can be a file stream, it's going to be a whack-a-mole"

It's also worth it to mention that experimental features are being evaluated at nodejs/next-10#285.

I'm pinging the initial authors of this PR for awareness: @bmeck @jasnell @ljharb @jsumners @JakobJingleheimer

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[joyeecheung]

Tests that make use of --experimental-network-imports should also be removed otherwise CI would complain...

On a side note, for the kind of security promises this feature aims to provide, the amount of tests written in the past 4 years seems way too small.....(<300 lines in total?)

[mcollina]

lgtm

[GeoffreyBooth]

Please let #53619 land first if you don’t mind.

[jsumners]

I am in support, but for clarity, my only contributions to the original issue were:

• Https imports #36328 (comment)
• Https imports #36328 (comment)
• Https imports #36328 (comment)

[guybedford]

An alternative here might be to just drop the security model itself, as opposed to the entire feature. Has that been considered?

[guybedford]

That is:

• Removing the security restrictions might be as simple a PR as this PR that does the full feature drop
• Adding new security models in future remains entirely viable, as a future effort to be championed

Whereas if we fully remove, it may send a message for those unaware and haven't read the full PR description that this feature is entirely removed.

[RafaelGSS]

That is:

• Removing the security restrictions might be as simple a PR as this PR that does the full feature drop
• Adding new security models in future remains entirely viable, as a future effort to be championed

Whereas if we fully remove, it may send a message for those unaware and haven't read the full PR description that this feature is entirely removed.

However, this contradicts the Node.js security policy. We believe that all features, even experimental ones, should be secure or, at the very least, have all limitations documented. For further information, we have discussed insecure features at nodejs/security-wg#1274.

Even if we document these issues as known limitations, they will eventually be classified as bugs. I doubt that they will be fixed, so having unmaintained code with known bugs is bad for Node.js development in general.

We can follow a quick-deprecation cycle if you feel strong about it, which is:

• semver-minor - doc-deprecate
• semver-minor - runtime deprecation
• semver-minor - remove the feature

What do you think?

[guybedford]

We believe that all features, even experimental ones, should be secure or, at the very least, have all limitations documented.

A CSP-style security model for network imports would be great to see, including integrity checking, rather than a parent importer context security model. And at the very least, an allow list of permitted origins.

I just wondered if the benefit of keeping it around is that it makes us consider how to support this as the loader APIs change in ways that may make it harder to implement. But I'm not asking for changes to the overall approach.

If the security stance is not to land experimental features without a security model then certainly let's go ahead with this removal for now.

[joyeecheung]

I think implementing CSP could also be quite a challenge on the current Node.js architecture - in Node.js the concept of origin basically has never really existed. All of the scripts or modules are compiled with an opaque origin, all the contexts share the same security token, there are holes everywhere. It can just be a new kind of whack-a-mole if we want to implement this security model in the existing codebase, instead of rewriting Node.js from scratch. If we want to implement that we need serious commitment from some party to deal with the security triaging, bug fixing and releases needed, otherwise it's going to consume our limited security workforce that could've been better spent again (which is what this feature is doing now) and hurt the security of the project as a whole.

[guybedford]

To be clear - I'm not holding this up, was just curious to have the discussion. Please don't let the above discussion stop this from progressing @RafaelGSS.