feat(fetch): add support for file urls in fetch #1239

Linkgoron · 2022-02-17T09:19:30Z

This PR adds support for file urls in fetch

refs: nodejs/node#42003

According to the fetch spec, file urls are "left as an exercise for the reader."
https://fetch.spec.whatwg.org/#scheme-fetch

benjamingr · 2022-02-17T16:26:55Z

lib/fetch/index.js

@@ -882,7 +883,7 @@ async function schemeFetch (fetchParams) {
    case 'file:': {
      // For now, unfortunate as it is, file URLs are left as an exercise for the reader.


remove comment?

That's from the spec itself, other comments also include the text from the spec itself.

benjamingr · 2022-02-17T16:27:35Z

lib/fetch/index.js

+async function fileFetch (fetchParams) {
+  const { request } = fetchParams
+  const url = requestCurrentURL(request)
+  const context = this


why is context just not this?

that's the same style that's used in other methods, so I wanted to keep it the same.

benjamingr · 2022-02-17T16:27:50Z

lib/fetch/index.js

+  const url = requestCurrentURL(request)
+  const context = this
+  if (request.method !== 'GET') {
+    return makeNetworkError(`Fetching files only supports the GET method. Received ${request.method}`)


What do browsers do with HEAD?

I looked at what Deno does, I even kept the message the same. I assume that Chrome behaves in a similar way, but I'll check it out.

benjamingr · 2022-02-17T16:29:05Z

lib/fetch/index.js

+        response.aborted = true
+        stream.destroy(new AbortError())
+      } else {
+        stream.destroy(new TypeError('terminated'))


Not specific to these PR but these errors should have codes?

(That seems orthogonal though)

I looked at what's done in other methods, so changes might need to get made in other places as well.

benjamingr · 2022-02-17T16:29:48Z

test/fetch/client-fetch.js

+    t.ok(err.cause)
+    t.equal(err.cause.code, 'ENOENT')
+  }
+})


test AbortSignal?

yes, tests are the main thing that's missing and why I've put this as a draft (also need to see what to do with headers)

benjamingr

Good progress

mcollina

We need a way to control the security of this. There are quite a few possible attacks when reading files from disk on servers.

My opinion is that we should aim to a "close by default" approach vs "open by default".

I would recommend we add a "FileSystemEnabledAgent" that can direct requests to disk in case we need it to be. This entity will be responsible to configure what paths could be served.

Linkgoron · 2022-02-17T17:09:42Z

We need a way to control the security of this. There are quite a few possible attacks when reading files from disk on servers.

My opinion is that we should aim to a "close by default" approach vs "open by default".

I would recommend we add a "FileSystemEnabledAgent" that can direct requests to disk in case we need it to be. This entity will be responsible to configure what paths could be served.

This kind of change would make this less portable (which was one of the reasons why this was requested), as I assume that other implementation don't have such constructions (I'll check them out). Is going out of spec and adding a flag or forcing localURLsOnly when using file urls an option? Or were you thinking of somehow configuring everything before-hand and having some kind of global configuration for fetch (does something like that exist already, I couldn't find any)?

benjamingr · 2022-02-17T17:22:30Z

We need a way to control the security of this. There are quite a few possible attacks when reading files from disk on servers.

So your concern is that we get an external URL from the user and they send over a file url and that lets them somehow access/manipulate data on the server?

mcollina · 2022-02-17T17:22:55Z

This kind of change would make this less portable (which was one of the reasons why this was requested), as I assume that other implementation don't have such constructions (I'll check them out). Is going out of spec and adding a flag or forcing localURLsOnly when using file urls an option?

That's not correct. In the browser this is managed by CSP and others. Unfortunately Node.js is not a browser and we have to deal with more strict security constraints, so I don't think this can be enabled by default without checks.

mcollina · 2022-02-17T17:23:31Z

So your concern is that we get an external URL from the user and they send over a file url and that lets them somehow access/manipulate data on the server?

Exactly that. Moreover I might want to send a URL to read a file that is restricted in a given folder.

Linkgoron · 2022-02-17T17:26:30Z

This kind of change would make this less portable (which was one of the reasons why this was requested), as I assume that other implementation don't have such constructions (I'll check them out). Is going out of spec and adding a flag or forcing localURLsOnly when using file urls an option?

That's not correct. In the browser this is managed by CSP and others. Unfortunately Node.js is not a browser and we have to deal with more strict security constraints, so I don't think this can be enabled by default without checks.

Yes, of course. What I meant was that I didn't think the fetch method had such parameters. I edited my message after you already quoted me, where I asked if you meant that we should add some kind of global configuration for fetch that would allow such behaviour?

mcollina · 2022-02-17T17:46:21Z

We usually include those configuration inside the Dispatcher system. That's the right place where the retrieving of file should happen and where the checks should be.

benjamingr · 2022-02-17T17:52:19Z

Asked about this in Deno btw: denoland/deno#11925 (comment)

KhafraDev · 2022-02-17T21:39:54Z

I think there should also be some warning that this behavior can change at any times; once (or if - file uris have been left to the implementation for years now) the spec includes steps for handling file:// urls, this will have to be rewritten and it's very likely that the behavior will change.

I believe that only Firefox and Deno handle file urls at the moment as well, which means it's not technically spec compliant and not widely available.

Jamesernator · 2022-09-18T02:06:49Z

While it's not a web option, could we just have an option to fetch to allow file urls? Browsers would simply ignore such an option so it would still be possible to write a compatible subset of fetch that works in both for loading package resources:

// file.js
const myResource = new URL("./resource.txt", import.meta.url);
 
const resourceText = await fetch(myResource, {
    // If file.js is loaded in Node.js then myResource will be some file:// url in which
    // case Node.js will see this option and allow it,
    // In browsers myResource will generally be a https:// url and will just ignore this
    // options proceeding per normal
    allowFileURLs: true,
}).then(res => res.text());

By having such an option, other fetch calls could be presumed to be "safe" (to the extent arbitrary fetches are actually safe in node).

Linkgoron · 2022-09-18T03:43:20Z

While it's not a web option, could we just have an option to fetch to allow file urls? Browsers would simply ignore such an option so it would still be possible to write a compatible subset of fetch that works in both for loading package resources:
// file.js
const myResource = new URL("./resource.txt", import.meta.url);
 
const resourceText = await fetch(myResource, {
    // If file.js is loaded in Node.js then myResource will be some file:// url in which
    // case Node.js will see this option and allow it,
    // In browsers myResource will generally be a https:// url and will just ignore this
    // options proceeding per normal
    allowFileURLs: true,
}).then(res => res.text());
By having such an option, other fetch calls could be presumed to be "safe" (to the extent arbitrary fetches are actually safe in node).

I agree that this feature is useful, and I'd be happy to get this into undici if we can agree on how to make everyone happy with its security.

mcollina · 2022-09-18T07:21:33Z

Go ahead and add this option, I think it's a good addition. I would use a more ugly-and-prefixed name for it to avoid any potential conflicts with whatwg. Anyhow, I'd recommend you to open an issue at wintercg/fetch.

KhafraDev · 2022-09-18T13:47:46Z

I would prefer to leave this to wintercg/fetch#5 instead of implementing a non-spec compliant solution.

Linkgoron force-pushed the fetch-file-url-support branch from e0525c3 to 1d0539c Compare February 17, 2022 12:12

add support for file urls in fetch

2b5a7c2

Linkgoron force-pushed the fetch-file-url-support branch from 1d0539c to 2b5a7c2 Compare February 17, 2022 12:26

benjamingr reviewed Feb 17, 2022

View reviewed changes

mcollina requested changes Feb 17, 2022

View reviewed changes

ronag force-pushed the main branch from c0f0e0d to ecae314 Compare August 17, 2022 15:09

KhafraDev mentioned this pull request Dec 9, 2022

Isomorphic local file fetching nodejs/node#45798

Closed

thescientist13 mentioned this pull request Dec 11, 2022

Agnostic Runtime ProjectEvergreen/greenwood#953

Open

ronag closed this Feb 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fetch): add support for file urls in fetch #1239

feat(fetch): add support for file urls in fetch #1239

Linkgoron commented Feb 17, 2022 •

edited

Loading

benjamingr Feb 17, 2022

Linkgoron Feb 17, 2022 •

edited

Loading

benjamingr Feb 17, 2022

Linkgoron Feb 17, 2022

benjamingr Feb 17, 2022

Linkgoron Feb 17, 2022 •

edited

Loading

benjamingr Feb 17, 2022

Linkgoron Feb 17, 2022 •

edited

Loading

benjamingr Feb 17, 2022

Linkgoron Feb 17, 2022 •

edited

Loading

benjamingr left a comment

mcollina left a comment

Linkgoron commented Feb 17, 2022 •

edited

Loading

benjamingr commented Feb 17, 2022

mcollina commented Feb 17, 2022

mcollina commented Feb 17, 2022

Linkgoron commented Feb 17, 2022 •

edited

Loading

mcollina commented Feb 17, 2022

benjamingr commented Feb 17, 2022

KhafraDev commented Feb 17, 2022

Jamesernator commented Sep 18, 2022 •

edited

Loading

Linkgoron commented Sep 18, 2022 •

edited

Loading

mcollina commented Sep 18, 2022

KhafraDev commented Sep 18, 2022

		@@ -882,7 +883,7 @@ async function schemeFetch (fetchParams) {
		case 'file:': {
		// For now, unfortunate as it is, file URLs are left as an exercise for the reader.

feat(fetch): add support for file urls in fetch #1239

feat(fetch): add support for file urls in fetch #1239

Conversation

Linkgoron commented Feb 17, 2022 • edited Loading

benjamingr Feb 17, 2022

Choose a reason for hiding this comment

Linkgoron Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

benjamingr Feb 17, 2022

Choose a reason for hiding this comment

Linkgoron Feb 17, 2022

Choose a reason for hiding this comment

benjamingr Feb 17, 2022

Choose a reason for hiding this comment

Linkgoron Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

benjamingr Feb 17, 2022

Choose a reason for hiding this comment

Linkgoron Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

benjamingr Feb 17, 2022

Choose a reason for hiding this comment

Linkgoron Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Linkgoron commented Feb 17, 2022 • edited Loading

benjamingr commented Feb 17, 2022

mcollina commented Feb 17, 2022

mcollina commented Feb 17, 2022

Linkgoron commented Feb 17, 2022 • edited Loading

mcollina commented Feb 17, 2022

benjamingr commented Feb 17, 2022

KhafraDev commented Feb 17, 2022

Jamesernator commented Sep 18, 2022 • edited Loading

Linkgoron commented Sep 18, 2022 • edited Loading

mcollina commented Sep 18, 2022

KhafraDev commented Sep 18, 2022

Linkgoron commented Feb 17, 2022 •

edited

Loading

Linkgoron Feb 17, 2022 •

edited

Loading

Linkgoron Feb 17, 2022 •

edited

Loading

Linkgoron Feb 17, 2022 •

edited

Loading

Linkgoron Feb 17, 2022 •

edited

Loading

Linkgoron commented Feb 17, 2022 •

edited

Loading

Linkgoron commented Feb 17, 2022 •

edited

Loading

Jamesernator commented Sep 18, 2022 •

edited

Loading

Linkgoron commented Sep 18, 2022 •

edited

Loading