nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.07k stars 529 forks source link

fetch: Enable fetch via file URL (under flag) #2751

Open micheleriva opened 7 months ago

micheleriva commented 7 months ago

What is the problem this feature will solve?

When trying to perform a fetch request to a file:// schema, fetch fails with the following error:

cause: Error: not implemented... yet...

Looking at the WHATWG specs, it seems like there is no specification for this schema:

"file" For now, unfortunate as it is, file: URLs are left as an exercise for the reader.

When in doubt, return a network error.

and undici follows this behaviour correctly.

There are many use cases where fetching a local file using fetch would be handy, and this is particularly true for WebAssembly modules.

Right now, libraries like wasm-pack (a Rust to WASM lib) have to perform different builds for browsers and Node.js, since Node.js is not able to fetch the compiled WASM binary from the file system.

Other runtimes and browsers handle this without any particular problem, but except for Deno (which has its own security model), I do understand the security implications of fetching a file from the filesystem.

What is the feature you are proposing to solve the problem?

With the introduction of the new Node.js permission APIs, we could enable fetch on the file system under an option, such as allowFileUrl: true.

That way, the user could explicitly enable fetching a local file, acknowledging the security implications that come with it.

About the actual implementation, I see that WinterCG also hasn't specified fetch for URLs with a file:// schema: https://fetch.spec.wintercg.org. Therefore, I'd also ask if that's the case to discuss how actually to implement this - if interesting for anyone.

What alternatives have you considered?

No response

mcollina commented 7 months ago

I’m ok in supporting this under a allowFileUrl option in fetch.

aduh95 commented 7 months ago

Related: https://github.com/wintercg/fetch/issues/5

mcollina commented 7 months ago

Moving to Undici repository

metcoder95 commented 7 months ago

What would be the shape of it, and its considerations?

From what I've seen from the issue shared by @aduh95 the main security points seem valid; Chromium seems to have some initial implementation.

All assuming its behind a flag, ofc

KhafraDev commented 7 months ago

If we implement this we will deviate from literally every other platform. We should either let wintercg handle it, or let the spec handle it. -1

micheleriva commented 7 months ago

@KhafraDev I agree with you that it’d be optimal to have wintercg to formalize this. What do you mean by “will deviate from literally every other platform”? Other runtimes (Bun and Deno for sure) implement a this already

KhafraDev commented 7 months ago

From a quick glance:

  1. Bun allows fetching file:// urls with any method, Deno throws on non-GET calls.
  2. Chrome only allows this for extensions, if users opt-in to it.
  3. Firefox and Safari do not implement this.

If we implement our own implementation (ie. with a non-standard option), that's another implementation that differs from every browser and every server environment. No implementation is spec compliant because there is no spec, which will also cause issues when a spec is written, and these platforms now have to break their users to match it.

There are also a number of edge cases that haven't been decided on https://github.com/wintercg/fetch/issues/5#issuecomment-1397365354


I'm quite confused as to your mention of wasm-pack

Right now, libraries like wasm-pack (a Rust to WASM lib) have to perform different builds for browsers and Node.js, since Node.js is not able to fetch the compiled WASM binary from the file system.

Browsers aren't able to do this either. But from looking at the code, they instead download the binary from a url, which will work in node and browsers, ignoring CORs errors. Maybe you could expand on this?

micheleriva commented 7 months ago

No implementation is spec compliant because there is no spec, which will also cause issues when a spec is written, and these platforms now have to break their users to match it.

This is very true, but that's also the purpose of experimental APIs. I personally think that missing APIs cause more damage to the user than experimental APIs.


Browsers aren't able to do this either. But from looking at the code, they instead download the binary from a url, which will work in node and browsers, ignoring CORs errors. Maybe you could expand on this?

Correct. Right now, if you use wasm-pack, you have to run at least three compilation processes:

  1. For browsers and runtimes. Since the mainstream runtimes support fetching from a file, they can share the same bindings built for the browsers.
  2. ESM bindings for Node.js, which doesn't support fetch for local files
  3. CJS bindings for Node.js, which doesn't support fetch for local files

The only real blocker I found to enable a single compilation process, it's by allowing Node.js to fetch the compiled WASM binaries from the filesystem at runtime.

I wouldn't focus too much on the wasm-pack use case only, though. I'm using Deno a lot and being able to avoid calling server-side methods (such as fs.readFileSync for Node.js) allows you to write code once and use it both on the frontend and the backend, which is kinda handy.

I understand the spec is basically non-existent, yet, as I said earlier, an experimental API is far better than no API at all to me.

But of course, that's a personal opinion and I'm happy to discuss

KhafraDev commented 7 months ago

But we aren't missing any apis, we implement fetching file urls exactly as the spec tells us to (throw an error). I would claim that Deno and Bun, by implementing this, cause much more damage in the long run (in the style of mootools or others that added methods to Array/Object prototypes, preventing the tc39 from adding certain methods).

I'm also still not sure how fetching file urls can be used in both browsers and runtimes, but that doesn't seem relevant.

aduh95 commented 7 months ago

I'm also still not sure how fetching file urls can be used in both browsers and runtimes, but that doesn't seem relevant.

tbf it doesn't need to be supported in browsers to improve interop, the use case I have in mind is fetch(new URL('./some/relative/path', import.meta.url)), where import.meta.url would be a HTTPS URL in browsers and a file: URL on runtimes.

KhafraDev commented 7 months ago

Thanks for the use case, makes sense now! Interop with other server environments is precisely the point of WinterCG. In the past we've followed Deno, which has only ever caused issues for us (namely security vulnerabilities, edge cases, more edge cases, other issues). But the proposed solution is not interoperable with other runtimes as it requires explicitly opting in to it, for every request - the other two runtimes just work. As in, you would have to change your code for it to work in node, whereas you don't for other runtimes, nor will you when/if it's ever added into the spec.

I mentioned earlier, but Bun isn't entirely interoperable with Deno, and there are also a number of questions about the api that WinterCG should solve before we even consider implementing it.