GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||
GeezerVison is showing github.com — lib,src: drop --experimental-network-imports by RafaelGSS · Pull Request #53822 · nodejs/node Conversation
|
|
Review requested:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
joyeecheung
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 3 reactions
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
GeoffreyBooth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let #53619 land first if you don’t mind.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
jsumners
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in support, but for clarity, my only contributions to the original issue were:
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
An alternative here might be to just drop the security model itself, as opposed to the entire feature. Has that been considered? |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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? |
All reactions
-
👍 2 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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. |
All reactions
-
👍 2 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
joyeecheung
commented
Jul 22, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
|
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. |
All reactions
-
👍 3 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
a9e93e6 to
fd75cb4
Compare
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
Refs: #53822 PR-URL: #54118 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Network imports were removed, so remove the comment. Refs: #53822 PR-URL: #54146 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Network imports were removed, so remove the comment. Refs: #53822 PR-URL: #54146 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #55111 Fixes: #55091 Refs: #53822 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Network imports were removed, so remove the comment. Refs: #53822 PR-URL: #54146 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #55111 Fixes: #55091 Refs: #53822 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #55111 Fixes: #55091 Refs: #53822 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* chore: bump Node.js to v22.9.0 * build: drop base64 dep in GN build nodejs/node#52856 * build,tools: make addons tests work with GN nodejs/node#50737 * fs: add fast api for InternalModuleStat nodejs/node#51344 * src: move package_json_reader cache to c++ nodejs/node#50322 * crypto: disable PKCS#1 padding for privateDecrypt nodejs-private/node-private#525 * src: move more crypto code to ncrypto nodejs/node#54320 * crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey nodejs/node#50234 * src: shift more crypto impl details to ncrypto nodejs/node#54028 * src: switch crypto APIs to use Maybe<void> nodejs/node#54775 * crypto: remove DEFAULT_ENCODING nodejs/node#47182 * deps: update libuv to 1.47.0 nodejs/node#50650 * build: fix conflict gyp configs nodejs/node#53605 * lib,src: drop --experimental-network-imports nodejs/node#53822 * esm: align sync and async load implementations nodejs/node#49152 * esm: remove unnecessary toNamespacedPath calls nodejs/node#53656 * module: detect ESM syntax by trying to recompile as SourceTextModule nodejs/node#52413 * test: adapt debugger tests to V8 11.4 nodejs/node#49639 * lib: update usage of always on Atomics API nodejs/node#49639 * test: adapt test-fs-write to V8 internal changes nodejs/node#49639 * test: adapt to new V8 trusted memory spaces nodejs/node#50115 * deps: update libuv to 1.47.0 nodejs/node#50650 * src: use non-deprecated v8::Uint8Array::kMaxLength nodejs/node#50115 * src: update default V8 platform to override functions with location nodejs/node#51362 * src: add missing TryCatch nodejs/node#51362 * lib,test: handle new Iterator global nodejs/node#51362 * src: use non-deprecated version of CreateSyntheticModule nodejs/node#50115 * src: remove calls to recently deprecated V8 APIs nodejs/node#52996 * src: use new V8 API to define stream accessor nodejs/node#53084 * src: do not use deprecated V8 API nodejs/node#53084 * src: do not use soon-to-be-deprecated V8 API nodejs/node#53174 * src: migrate to new V8 interceptors API nodejs/node#52745 * src: use supported API to get stalled TLA messages nodejs/node#51362 * module: print location of unsettled top-level await in entry points nodejs/node#51999 * test: make snapshot comparison more flexible nodejs/node#54375 * test: do not set concurrency on parallelized runs nodejs/node#52177 * src: move FromNamespacedPath to path.cc nodejs/node#53540 * test: adapt to new V8 trusted memory spaces nodejs/node#50115 * build: add option to enable clang-cl on Windows nodejs/node#52870 * chore: fixup patch indices * chore: add/remove changed files * esm: drop support for import assertions nodejs/node#54890 * build: compile with C++20 support nodejs/node#52838 * deps: update nghttp2 to 1.62.1 nodejs/node#52966 * src: parse inspector profiles with simdjson nodejs/node#51783 * build: add GN build files nodejs/node#47637 * deps,lib,src: add experimental web storage nodejs/node#52435 * build: add missing BoringSSL dep * src: rewrite task runner in c++ nodejs/node#52609 * fixup! build: add GN build files * src: stop using deprecated fields of v8::FastApiCallbackOptions nodejs/node#54077 * fix: shadow variable * build: add back incorrectly removed SetAccessor patch * fixup! fixup! build: add GN build files * crypto: fix integer comparison in crypto for BoringSSL * src,lib: reducing C++ calls of esm legacy main resolve nodejs/node#48325 * src: move more crypto_dh.cc code to ncrypto nodejs/node#54459 * chore: fixup GN files for previous commit * src: move more crypto code to ncrypto nodejs/node#54320 * Fixup Perfetto ifdef guards * fix: missing electron_natives dep * fix: node_use_node_platform = false * fix: include src/node_snapshot_stub.cc in libnode * 5507047: [import-attributes] Remove support for import assertions https://chromium-review.googlesource.com/c/v8/v8/+/5507047 * fix: restore v8-sandbox.h in filenames.json * fix: re-add original-fs generation logic * fix: ngtcp2 openssl dep * test: try removing NAPI_VERSION undef * chore(deps): bump @types/node * src: move more crypto_dh.cc code to ncrypto nodejs/node#54459 * esm: remove unnecessary toNamespacedPath calls nodejs/node#53656 * buffer: fix out of range for toString nodejs/node#54553 * lib: rewrite AsyncLocalStorage without async_hooks nodejs/node#48528 * module: print amount of load time of a cjs module nodejs/node#52213 * test: skip reproducible snapshot test on 32-bit nodejs/node#53592 * fixup! src: move more crypto_dh.cc code to ncrypto * test: adjust emittedUntil return type * chore: remove redundant wpt streams patch * fixup! chore(deps): bump @types/node * fix: gn executable name on Windows * fix: build on Windows * fix: rename conflicting win32 symbols in //third_party/sqlite On Windows otherwise we get: lld-link: error: duplicate symbol: sqlite3_win32_write_debug >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:47987 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_sleep >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48042 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_is_nt >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48113 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_unicode >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48470 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_unicode_to_utf8 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48486 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_mbcs_to_utf8 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48502 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_mbcs_to_utf8_v2 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48518 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_mbcs >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48534 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_mbcs_v2 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48550 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj * docs: remove unnecessary ts-expect-error after types bump * src: move package resolver to c++ nodejs/node#50322 * build: set ASAN detect_container_overflow=0 nodejs/node#55584 * chore: fixup rebase * test: disable failing ASAN test * win: almost fix race detecting ESRCH in uv_kill libuv/libuv#4341
Reviewers
+1 more reviewer
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
12 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.