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

[Linkgoron]

This PR adds support for file urls in fetch

refs: https://github.com/nodejs/node/issues/42003

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

[Linkgoron]

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]

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]

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]

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]

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]

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]

Asked about this in Deno btw: https://github.com/denoland/deno/issues/11925#issuecomment-1043217870

[KhafraDev]

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]

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]

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]

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]

I would prefer to leave this to https://github.com/wintercg/fetch/issues/4 instead of implementing a non-spec compliant solution.