lib,src: drop --experimental-network-imports #53822
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
There was a problem hiding this comment.
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?)
GeoffreyBooth
added
module
|
An alternative here might be to just drop the security model itself, as opposed to the entire feature. Has that been considered? |
|
That is:
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:
What do you think? |
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. |
|
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. |
|
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. |
RafaelGSS
force-pushed
the
remove-network-import
branch
from
a9e93e6
to
fd75cb4
Compare
RafaelGSS
added
request-ci
github-actions
bot
removed
the
request-ci
aduh95
added
commit-queue
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
nodejs-github-bot
removed
the
commit-queue
|
Landed in 5ac969f |
PR-URL: #53822 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: TODO
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
Network imports were removed, so remove the comment. Refs: #53822
PR-URL: #53822 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
This PR reverts (not cleanly) #36328 and consequently, the
--expiremental-network-importis 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:
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