Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Https imports #36328

Merged
merged 1 commit into from Feb 10, 2022
Merged

Https imports #36328

merged 1 commit into from Feb 10, 2022

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Nov 30, 2020

This allows for HTTPS (not HTTP) imports to work with the ESM module loader. This PR is only an initial PR for incremental progress behind a flag.

Supported concerns
  • Disabled by default - requires --experimental-https-modules flag
  • Redirects - act like web
  • Selective HTTP support - only works on loopback
  • Debugging HTTPS on a local machine is painful - use loopback + http:
  • Cannot access node: builtins - banned anything except http: and https: deps.
Punted concerns
  • Offline cache - punted to follow on work
  • Integrity checks - left as follow on work via policies
  • CORS - not supported
  • CA - TBD follow on work
  • Cookies - TBD follow on work
  • MIME parsing - wontfix / still broken since we don't have a real parser in core, I'm not gonna try to get mime landed anytime soon due to pushbacks
Documentation
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@bmeck bmeck added wip Issues and PRs that are still a work in progress. esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team. labels Nov 30, 2020
@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

@nodejs/modules-active-members I couldn't find a sane way to refactor the getFormat hook in this PR. We should figure out a way to make it not do 2 requests.

@vdeturckheim
Copy link
Member

Interesting, would there be a way to statically know what an app uses? Right now Sqreen parses the content of the diverse node_modules and we consider it as "good enough" as a source of truth.

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

@vdeturckheim due to import() using an expression, not unless there is no usage without a literal. A policy file could enforce a static codebase though.

@vdeturckheim
Copy link
Member

@bmeck true, I often kind of think that import() is somehow the eval() of modules and could have a flag to be disabled but this would be a totally different topic.
I like the policy idea, it sounds pretty elegant.

@jkrems
Copy link
Contributor

jkrems commented Nov 30, 2020

I couldn't find a sane way to refactor the getFormat hook in this PR. We should figure out a way to make it not do 2 requests.

This may a new motivation to revive #35524. I think that should fix the issue (but it's potentially disruptive).

@benjamingr
Copy link
Member

benjamingr commented Nov 30, 2020

I think this is really cool and it would be cool if this loader (optionally?) cached resources (according to HTTP headers, like browsers do). I think this is what you mean by "Offline cache" :]

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

@jkrems an alternative is to have a resource cache in our default loader rather than doing everything at time of request, we can drop the resource once it ref counts down to 0 or some other metric.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree to have this enabled by default. It’s really unsafe to do, and it is detrimental to the developer experience.

(I do not have much time to explain why it’s completely unsafe to do this, I’ll try to write a long write up later if needed).

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

@mcollina I'm not entirely opposed to flagging this (such as requiring it to be in a policy file listing), but if this leads to users consistently enabling them by default it would be good to understand why HTTPS (not HTTP) is insecure from your perspective so we can mitigate any issues.

@jasnell
Copy link
Member

jasnell commented Nov 30, 2020

@mcollina... as this is still a draft and being actively discussed, I'm not sure the "-1"/Request Changes is helpful yet.

@bmeck ... the security concerns on this really come down to the general risk of running untrusted code downloaded over arbitrary internet connections. For instance, I could fairly easily write a script that returns one bit of javascript during development but replaces that with a malicious script when accessed from a production server, without the developer ever knowing that there's been a change. Putting this behind a flag for now while we figure out the threat model and various mitigations definitely makes sense and would be right thing to do.

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

@jasnell I've stated a security model document, though due to the lack of safety in using core's HTTPS a lot will inevitably be labeled as out of scope of this feature and must be secured by other means like policies. I would note that event-stream did the same kind of replacement workflow on files locally and do not consider this attack vector novel by adding HTTPS. Node's mitigation for that attack surface is to use a policy and the same would be true here.

@mcollina
Copy link
Member

It is insecure because there is no signature of the original file. Anybody that can take control of a domain name can take control of a server.

As a community we had some significant issues regarding security, from left-pad to event-stream. Adding this functionality would make us 10x more vulnerable to these kind of a attack because there would be no central entity that could intervene.

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

@mcollina can you clarify how policies aren't that intervention/mitigation?

@mcollina
Copy link
Member

@mcollina... as this is still a draft and being actively discussed, I'm not sure the "-1"/Request Changes is helpful yet.

The plan at the top highlight the fact that this will be enabled by default and it needs consensus to make users disable this. I do not agree with that approach, hence my comment. I'm happy to change my mind if it can be made safe to do so - however I do not think it is possible at all to make this safe.

@jasnell
Copy link
Member

jasnell commented Nov 30, 2020

@bmeck .. for discussion.. from your checklist:

Can be disabled - NEEDS CONSENSUS

Initially I would mark this explicitly opt-in. Assuming we flip that default later, there should definitely be a way to explicitly opt-out on the process level.

Offline cache - NEEDS CONSENSUS
established file location / configuration
ability to eject/override cache features agreed upon
cache semantics agreed upon

Definitely a much larger discussion and one that we need to have with the package manager folks at the table. Not only would we need to figure out the caching semantics, we need to figure out the semantics around what happens if a require('https...') script does a require('some-local-script'). What if those things end up being different versions? I think what we need to do is take a moment to draw up a list of What Ifs and see if we can easily answer those.

Integrity checks - WIP

I'm going to sound like a broken record but as I've been saying for years now we 100% need this. Adding https imports would only make that need more pronounced.

policies should cover? needs tests for protocol => domain => / separator => URL
certificate authorities
Cookies - NEEDS CONSENSUS
should this even be allowed / same-origin meaning, agreed upon

Blocklist / Allow list of origins at the very least.
Same-origin is going to be hard. I can imagine a case where require('https...') effectively becomes its own realm, where any of the code running within is isolated in it's own context, is only permitted to use either require('{built-in}') or require('https...') within the same origin or following origin policies, etc.

Cookies are a much more difficult matter. My knee jerk reaction is that we should not support cookies.

Redirects - WONTFIX / banned
redirecting across protocols is... insane
they work significantly different than files, making dumping to disk break in odd ways
they make import.meta.url mean something very different by no longer having a 1-1 mapping for Web compat
meaning for policies is very complicated (need to add aliasing config to policies if supported)

This I'm less convinced about and need to think through more. My knee jerk reaction is that not supporting redirects in some way is very anti-web and therefore a bad thing but that's the Standards Wonk side of me speaking. Will stew on this one.

MIME - FIXME
MIME parser is... still blocked? I'm not willing to go through the consensus/ anguish to move that forward; too much blood lost already to the great consensus gods and it feels like I will cry (not in a joking manner) if I try to land it again.

I think I may be able to take this over for you but it would realistically be a 2021 project.

Selective HTTP support - NEEDS CONSENSUS

I think we can safely just say no to http modules.

Debugging HTTPS on a local machine is painful, should have some way to do local dev.

This mechanism should allow for locally aliasing of https modules anyway, making it so that even if require('https...') is used, if there is a local alias established for it, the local one is used. That should address at least the immediate issue. However, the point here still stands: we'll need to provide observability into what is happening during load. We do have keylog support already built in to core. We'll want to make sure we have command line options to allow keylogging for Node.js' own module loader connections.

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

@mcollina we need to understand why you consider it unsafe given current features and mitigations for these workflows that already exist. I don't think a blanket statement that it is unsafe and implication that it cannot be safe is helpful.

@jasnell
Copy link
Member

jasnell commented Nov 30, 2020

@bmeck ... I'm going to take a bit more time to dig through your write ups and code and think it through before responding much more as I don't want to just churn the conversation here. Hopefully others will do the same. I'll definitely take on the MIME module work tho if that would make things easier for you.

@mcollina
Copy link
Member

The attack vector is the same of left-pad and event-stream. There are plenty of write ups around on what happened. The mitigations that npm put in place are not feasible without a central/federated authority or strong cryptography.

I don't have time right now for a long write-up, I'll try to get back as soon as I can.


What would make this safe is giving authors a way to cryptographically sign the published files, and let developers validate those files against said keys that are fetched off-band.


Overall I would recommend developing this as a module/loader and then propose its addition to Node.js.

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

What would make this safe is giving authors a way to cryptographically sign the published files, and let developers validate those files against said keys that are fetched off-band.

I don't understand this. I am vehemently against code signing on the developer if it is done improperly and have met over the years with a few certificate authorities about the complexity of doing this. This reduces down to the same workflow as TLS where you pull the keys off the trusted CA which is out of band by nature then verify. What advantage do we get here? Even if they upload a personal key such as using PGP the revocation mechanism and hijacking or repurposing under a different key are all possible.

Overall I would recommend developing this as a module/loader and then propose its addition to Node.js.

What is "this"? I'd note that a variety of the mitigations are not possible in user land loaders (such as policy integration) and loader have repeatedly been stalled out by Node's process so I doubt they will move forward any time soon. I spent a lot of time on loaders and trying to move them forward and don't think they are worth the push back from the consensus model we use in Node core.

@wperron
Copy link

wperron commented Nov 30, 2020

Hey all, so I brought up some points recently on Twitter related to this proposal, so I thought I'd re-articulate them here. Keep in mind I've never contributed to Node.js, this is just my opinion as a long-time user and a contributor to Deno.

TL;DR:

Importing from npm isn't inherently more secure than importing from a raw URL, so let's work to make the whole ecosystem more secure, regardless of how packages are imported.

URL Imports Aren't Inherently Less Secure

First I want to address the biggest concern that always comes up when talking about URL imports: "It's insecure." The argument is usually summed up similarly to what @jasnell said earlier in this thread: "I could fairly easily write a script that returns one bit of javascript during development but replaces that with a malicious script when accessed from a production server, [...]"

It's true, but it implies that the current way of importing is somehow more secure, and that it is more secure by virtue of not explicitely using a URL. However, packages hosted on npm are vulnerable to this problem as well. Not even a month ago, there was another report of malicious code being distributed in an npm package.

We've become accustomed to assume that central registries are somehow more secure, but it's simply not the case. Packages are still fetched from a remote source and malicious code can still make its way into npm. Once we accept that reality, URL imports become much less scary (or central registries become much more scary, depending on how you look at it).

The concerns raised here are all very valid; We just have to remember that they are issues that also affect the current import model, and so I don't it's fair to block this proposal on the grounds that it's "less secure".

I would even argue that URL imports have the potential to be more secure than traditional imports through npm, because it leverages two important aspects of the web: Authority and Visibility. It gives end-users the possibility to verify that they are imported from a trusted domain name and that that domain name has a valid SSL certificates that verifies that it is indeed that domain. It also allows to verify the whois information associated with that domain, potentially being much more explicit about when a domain or a package changes ownership. (As an aside: wouldn't you rather import express directly from https://expressjs.org than https://npmjs.org ?)

Those are all things that end users can implement themselves and include in their CI/CD pipelines and compliance tools. They've been the backbone of the internet for a long time. Browsers trust them. Users trust them. Why couldn't we?


Feedback On The Concerns Above

The other part of @jasnell comment - "[...] without the developer ever knowing that there's been a change." is very important; I think if URL imports are to go forward, they should be restricted to tagged versions, no "latest" allowed. deno.land/x uses the @ symbol to identify versions. While we don't support semver type versioning like package.json does, it's probably possible to implement in order to support the same version limiting already available in Node.

Local caching and interop with require("some-local-package") are also two good points. Now, this might just be me being naive about the complexity of the node_modules system, but we could simply cache url-imported modules in node_modules: It would make the old style of imports compatible with url imports and solve the issue of the local cache. Sure it wouldn't be compliant with http cache headers, but then again, node_modules already isn't and that's never been a problem in the past. In fact, I think there's a larger argument to be made here that being compliant with http cache headers don't make much sense on the server side.

Similarly, I agree with @jasnell with regards to the cookies support; I don't think it makes much sense here.

I'm less certain about redirects, they can be helpful for resolving versions when using tags like ^1.6 or something. Maybe restrict them to the same origin?

I don't see a reason to not include an allow/block function for specific origins, though I think we should also add a recommendation in the docs for operators to include those in firewall rules (hey if we're gonna take advantage of web mechanisms, let's go all the way!)

Now for a big one: integrity checks. Honestly, I don't know how much we can do there. Ultimately we can include checksum verification to make sure that the content the user receives is the same one the server said it was sending and that it wasn't tempered with. Beyond that, I don't there's much else that can be done. As I said earlier, it's already possible for legitimate users to upload malicious code to a legitimate platform. The best anyone can do here is certify that the content wasn't messed with by a man-in-the-middle.

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

I'm less certain about redirects, they can be helpful for resolving versions when using tags like ^1.6 or something. Maybe restrict them to the same origin?

They can be but the way the web is specified leads to issues if you do. Let us imagine:

  1. a function resolve = import.meta.resolve that resolves URLs for importing purposes.
  2. a url /react that redirects to /react/1.6

On the web await import(resolve('/react')) !== await import('/react') because one is cached at /react and the other at /react/1.6. This problem is so bad in ESM when it was possible to cause this via having both ESM and CJS that we have a whole docs section on it for Node https://nodejs.org/dist/latest/docs/api/packages.html#packages_dual_package_hazard

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

Per integrity checks that many people keep bringing up please read https://nodejs.org/dist/latest/docs/api/policy.html as they do already cover most of these concerns. However, like @wperron is bringing up, they assert integrity not if the resource is hostile or not.

@devsnek
Copy link
Member

devsnek commented Nov 30, 2020

how about gopher imports?

I think https imports are not great for security. I also think npm is not great for security. I'm fine with node supporting both. If one wants to argue that we should only be adding "secure" module systems moving forward, it would probably be a better use of time to discuss some sort of decentralised immutable protocol or something.

@guybedford
Copy link
Contributor

Restricting https imports to some top-level --unsecure flag that requires policy to be populated to disable might make sense. It would be nice to see what the overall workflow looks like there that it doesn't end up similar to Deno in making adding --unstable rote.

One ecosystem concern I have here is npm packages starting to ship imports to URLs in them. This risks an npm ecosystem that becomes dynamically attackable from these https vectors. It would be nice to explicitly disable support for that somehow in the resolution mechanism. Eg turning this feature off in the module graph once file-based package resolutions have been hit.

@bmeck
Copy link
Member Author

bmeck commented Nov 30, 2020

It would be nice to explicitly disable support for that somehow in the resolution mechanism. Eg turning this feature off in the module graph once file-based package resolutions have been hit.

Can you clarify this idea?

@GeoffreyBooth GeoffreyBooth added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Feb 10, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 10, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/36328
✔  Done loading data for nodejs/node/pull/36328
----------------------------------- PR info ------------------------------------
Title      Https imports (#36328)
Author     Bradley Farias  (@bmeck)
Branch     bmeck:https-import -> nodejs:master
Labels     semver-minor, esm, author ready, commit-queue-squash
Commits    12
 - esm: support https remotely and http locally under flag
 - fixup: rename `esm/fetch` → `esm/fetchModule`
 - fixup: explain purpose of new http(s) agent instead of the global agent
 - fixup: correct file name to conform to snake_case
 - fixup: add missing modules to bootstrap test
 - fixup: use require('buffer')
 - fixup: new linting rules
 - fixup: make test check for interfaces rather than assume ipv6 is avai…
 - fixup: lint
 - fixup: dtrace bootstrap check should only happen if dtrace exists
 - fixup: ci
 - fixup: ci
Committers 2
 - Geoffrey Booth 
 - Bradley Farias 
PR-URL: https://github.com/nodejs/node/pull/36328
Reviewed-By: Matteo Collina 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36328
Reviewed-By: Matteo Collina 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 30 Nov 2020 16:47:19 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36328#pullrequestreview-867134230
   ✔  - Geoffrey Booth (@GeoffreyBooth): https://github.com/nodejs/node/pull/36328#pullrequestreview-878270674
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-10T02:50:30Z: https://ci.nodejs.org/job/node-test-pull-request/42467/
- Querying data for job/node-test-pull-request/42467/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 36328
From https://github.com/nodejs/node
 * branch                  refs/pull/36328/merge -> FETCH_HEAD
✔  Fetched commits as 52b1904a0dd4..cdbba252057d
--------------------------------------------------------------------------------
Auto-merging doc/api/errors.md
Auto-merging doc/api/esm.md
Auto-merging lib/internal/errors.js
Auto-merging lib/repl.js
[master c3f50181bb] esm: support https remotely and http locally under flag
 Author: Bradley Farias 
 Date: Mon Nov 30 10:03:30 2020 -0600
 31 files changed, 935 insertions(+), 132 deletions(-)
 create mode 100644 lib/internal/modules/esm/fetch.js
 create mode 100644 lib/internal/modules/esm/formats.js
 create mode 100644 test/es-module/test-http-imports.mjs
[master 01543495ff] fixup: rename `esm/fetch` → `esm/fetchModule`
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Thu Feb 3 21:57:41 2022 +0100
 5 files changed, 11 insertions(+), 11 deletions(-)
 rename lib/internal/modules/esm/{fetch.js => fetchModule.js} (99%)
[master a79f3a4b13] fixup: explain purpose of new http(s) agent instead of the global agent
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Fri Feb 4 17:36:39 2022 +0100
 1 file changed, 11 insertions(+), 13 deletions(-)
[master 7b180db04f] fixup: correct file name to conform to snake_case
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Fri Feb 4 17:56:16 2022 +0100
 5 files changed, 4 insertions(+), 4 deletions(-)
 rename lib/internal/modules/esm/{fetchModule.js => fetch_module.js} (100%)
[master 0453e4a154] fixup: add missing modules to bootstrap test
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Fri Feb 4 17:56:40 2022 +0100
 1 file changed, 24 insertions(+), 15 deletions(-)
[master 8deca8052e] fixup: use require('buffer')
 Author: Geoffrey Booth 
 Date: Sun Feb 6 18:27:04 2022 -0800
 1 file changed, 1 insertion(+), 2 deletions(-)
[master 791a60ce3b] fixup: new linting rules
 Author: Geoffrey Booth 
 Date: Sun Feb 6 18:50:57 2022 -0800
 2 files changed, 9 insertions(+), 3 deletions(-)
[master b42683c098] fixup: make test check for interfaces rather than assume ipv6 is available
 Author: Bradley Farias 
 Date: Tue Feb 8 09:14:27 2022 -0600
 1 file changed, 30 insertions(+), 6 deletions(-)
[master d03922ba5d] fixup: lint
 Author: Bradley Farias 
 Date: Tue Feb 8 09:27:39 2022 -0600
 1 file changed, 4 insertions(+), 4 deletions(-)
[master 6692dadbef] fixup: dtrace bootstrap check should only happen if dtrace exists
 Author: Bradley Farias 
 Date: Tue Feb 8 12:06:40 2022 -0600
 1 file changed, 6 insertions(+), 1 deletion(-)
[master 1c671c49f0] fixup: ci
 Author: Bradley Farias 
 Date: Wed Feb 9 09:26:16 2022 -0600
 3 files changed, 8 insertions(+), 3 deletions(-)
[master 27383e3984] fixup: ci
 Author: Bradley Farias 
 Date: Wed Feb 9 13:51:30 2022 -0600
 1 file changed, 4 insertions(+), 2 deletions(-)
   ✔  Patches applied
There are 12 commits in the PR. Attempting to fixup everything into first commit.
[master 9cbf06a743] esm: support https remotely and http locally under flag
 Author: Bradley Farias 
 Date: Mon Nov 30 10:03:30 2020 -0600
 33 files changed, 1000 insertions(+), 149 deletions(-)
 create mode 100644 lib/internal/modules/esm/fetch_module.js
 create mode 100644 lib/internal/modules/esm/formats.js
 create mode 100644 test/es-module/test-http-imports.mjs
--------------------------------- New Message ----------------------------------
esm: support https remotely and http locally under flag

Co-authored-by: Jacob Smith
3012099+JakobJingleheimer@users.noreply.github.com
Co-authored-by: James M Snell jasnell@gmail.com
Co-authored-by: Jordan Harband ljharb@gmail.com
Co-authored-by: James Sumners james@sumners.email

PR-URL: #36328
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

[master 834b464a3f] esm: support https remotely and http locally under flag
Author: Bradley Farias bradley.meck@gmail.com
Date: Mon Nov 30 10:03:30 2020 -0600
33 files changed, 1000 insertions(+), 149 deletions(-)
create mode 100644 lib/internal/modules/esm/fetch_module.js
create mode 100644 lib/internal/modules/esm/formats.js
create mode 100644 test/es-module/test-http-imports.mjs
✖ 834b464a3f350c23c400b7b1b08e2790d74715e6
✖ 1:0 Co-authored-by must be a trailer co-authored-by-is-trailer
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 7:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/1821782609

Co-authored-by: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: James Sumners <james@sumners.email>
PR-URL: nodejs#36328
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@Trott
Copy link
Member

Trott commented Feb 10, 2022

Landed in ceadb47

@Trott Trott merged commit ceadb47 into nodejs:master Feb 10, 2022
@GeoffreyBooth
Copy link
Member

Thanks @Trott ! Congrats @JakobJingleheimer !

@TomasHubelbauer
Copy link

I'm incredibly excited for this, thanks for adding it! Any word on what will be the first version of Node this will land in and in which I can expect the CLI flag to work? Thanks!

@mrozbarry
Copy link

Would there be a good way to check an https import resource with a checksum?

@bmeck
Copy link
Member Author

bmeck commented Feb 10, 2022

@mrozbarry you can use policies for integrity checks https://nodejs.org/dist/latest-v17.x/docs/api/policy.html

@mrozbarry
Copy link

Unrelated to my previous comment, what if domains could be allowed/white-listed in a package.json? That could at least prevent imports from importing from some unknown domain. Maybe on first run, before cache, node could prompt "Do you trust domain.tld? [yN]", which would just add some sort of "allowedDomains": ["domain.tld"] to package.json. I think that could eliminate a collection of hijacking problems.

@bmeck
Copy link
Member Author

bmeck commented Feb 10, 2022

@mrozbarry that is possible in a policy

@mrozbarry
Copy link

@mrozbarry you can use policies for integrity checks https://nodejs.org/dist/latest-v17.x/docs/api/policy.html

It would be great if node could automatically handle generating these integrity checksums for new https resources if they don't exist already. I'm going to assume that most js scripts in the wild don't have their own checksums.

Also, is there a good standard in place for servers that don't want someone just importing their scripts? I'm assuming CORS won't really cover that, considering know doesn't necessarily have to have a concept of it's own server.

@bmeck
Copy link
Member Author

bmeck commented Feb 10, 2022

@mrozbarry please open new issues, this is a merged PR

@jsumners
Copy link
Contributor

I suggest locking as resolved.

@nodejs nodejs locked as resolved and limited conversation to collaborators Feb 10, 2022
@GeoffreyBooth GeoffreyBooth added notable-change PRs with changes that should be highlighted in changelogs. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 10, 2022
req.destroy();
res.destroy();
}
if (res.statusCode >= 300 && res.statusCode <= 303) {
Copy link
Contributor

@aduh95 aduh95 Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably think of adding support for HTTP status code 307 and 308 at some point.
EDIT: sorry for the noise, I didn't see the issue was locked.

@danielleadams

This comment was marked as outdated.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet