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

fetch() a local file #2150

Closed
phil294 opened this issue Apr 19, 2019 · 18 comments · Fixed by #12545
Closed

fetch() a local file #2150

phil294 opened this issue Apr 19, 2019 · 18 comments · Fixed by #12545
Labels
ext/fetch related to the ext/fetch feat new feature (which has been agreed to/accepted)

Comments

@phil294
Copy link

phil294 commented Apr 19, 2019

Coming from https://stackoverflow.com/q/51941064/3779853

Any plans to support await fetch("file:///./input.txt")? Like Jeremy, this is the first thing I'd intiuitively try to read a local file, given deno's philosophy of browser-like methods.

@ry
Copy link
Member

ry commented Apr 19, 2019

Seems reasonable to me.

@ry ry added this to the future milestone Apr 19, 2019
@zekth
Copy link
Contributor

zekth commented Apr 19, 2019

I will push a PR to fix this.

@Janpot
Copy link

Janpot commented Apr 22, 2019

is the behavior for fetch with file: urls standardized?

@phil294
Copy link
Author

phil294 commented Apr 22, 2019

Browsers cannot fetch local files in general, that would be a big security issue. Thats why the spec also only ever speaks about network or data/blob/about URLs. https://fetch.spec.whatwg.org/#url

node does not implement fetch at all.

@kevinkassimo
Copy link
Contributor

@Janpot I don't think so.

Also see the comments below from node-fetch maintainer (also a former Node.js TSC member) with respect to a similar issue:


Much thought have gone into this issue, for sure. I'd like to condense the most important reasons I believe URLs of file scheme should not be implemented in node-fetch. I would of course be thankful if @bitinn or @zkat would like to weigh in and offer their opinions here as well.

1. Accessing the local filesystem eases security breaches.

Take @jimmywarting's (admittedly anecdotal and contrived) use case:

I for once use node-fetch as a proxy, user can request any url they want with my api
wouldn't want them to get access to the filesystem...

Really, I cannot believe many people are using node-fetch as a proxy (I might be wrong of course, but IMO there are many much better ways to create a proxy in Node.js). But I find it easy to imagine people who are using a URL from an untrusted source (the network for example) and just plug it into fetch(). In many cases, that practice is fine. It would not be fine if file:///etc/passwd is accessed.

@stevenvachon proposed using an option to allow file URL scheme. That may solve this issue, but see my response below.

2. There is no recognized standards for interpretation of file-scheme URLs.

As outlined in sindresorhus/got#227 filed by @stevenvachon, WHATWG's Fetch Standard offers no discussion of how file URL schemes should be interpreted on different platforms. In fact, in the PR adding documentation for Node.js fs module file URL support, I explicitly recommended documenting how URLs are interpreted on different platforms, precisely because of the competing interpretations.

Now that file URL scheme is supported officially in Node.js, I foresee a de-facto standard being established at least in the Node.js community. But then in less than a month when Node.js 8.0.0 is released, users can use fs.readFile(new URL(fileURL), (err, buf) => { if (err) ... } ) (or any promisified variant), which I consider to be an improvement over fetch(fileURL, { allowFileScheme: true }).then(res => res.buffer()).then(buf => { ... }).catch(err => { ... });.

3. There is no precedent for support of file-scheme URLs in Node.js community.

None of the most popular URL-to-response libraries I can think of in under a minute do not support file-scheme URLs:

4. Browser fetch() does not support file-scheme URLs.

There are a lot of people who use node-fetch as a server-side polyfill to allow for isomorphic requests. It would be a surprising behavior if node-fetch has the capability to access the file system browsers' fetch() cannot. However, neither Chrome nor Firefox supports fetch() with file-scheme URLs. GitHub's fetch polyfill refuses to do so, also: JakeChampion/fetch#92 (comment)


@matthewadams

NB: I'm @matthewadams, not @matthew-andrews. Sorry for the noise, @matthew-andrews

Oops, apologies. Shouldn't have depended on the autocompletion.

I'm confused by your response. Please see swagger-api/swagger-js#1044 for justification.

What I meant to express is that Swagger should be aware of node-fetch's limitations, and decide if it wants to allow file URLs. I do not see any indication of such a decision made in the linked issue by the maintainers of Swagger, and if I were to design the Swagger API, I would allow both HTTP(S) URLs and a plain file path instead of resorting to file-scheme URLs.

Do you have a pointer for Node.js v8 supporting file:// URLs in fs?

Code was added in nodejs/node#10739. Docs were added in nodejs/node#12670.


@stevenvachon

I don't see why it can't be supported behind an option to enable it.

Adding an option that defaults to true would not change issue 1 above.

If it defaults to false, issues 1 and 4 would indeed be resolved. However I would prefer not to add to the list of non-spec options we have unless absolutely necessary. With the exception of size, the options we have all aim to reimplement browser defaults that users on Node.js may or may not want.

the spec is already broken here: https://github.com/bitinn/node-fetch/blob/master/LIMITS.md

This issue is not an issue of spec-compliance. Even if it is, as the steward of node-fetch, I would exercise my best decision when it comes to following the spec, which according to the reasons I outlined above leads me to the side of "against."


@Klortho

URL stands for Uniform Resource Locator, after all. Tim Berners Lee would be rolling over in his grave. (If he were dead, that is.)

Yes, URL indeed stands for that. But it should come as no surprise that certain applications only support certain URL schemes. And speaking of Tim Berners-Lee, I would like to add a shameless plug to a particular tweet of mine. 😄

Originally posted by @TimothyGu in node-fetch/node-fetch#75 (comment)

@jimmywarting
Copy link
Contributor

fetch just create an unnecessary layer of complexity. if you intent to read some local file use the fs directly.
it's more clear if the fetch only have network permission. i think it would be confusing as to why i had to give filesystem access when all it should do is making request.

@est31
Copy link

est31 commented Apr 28, 2019

However, neither Chrome nor Firefox supports fetch() with file-scheme URLs.

That's news to me. Yes, Chrome disallows fetch-ing any file:// scheme based url, but Firefox definitely allows fetch('file://') as long as their same origin policy is followed.

@josephrocca
Copy link
Contributor

josephrocca commented Jun 2, 2019

And Chrome has the --allow-file-access-from-files flag which (as the name suggests) allows fetching file:// urls so long as the window itself is a file://.

  1. Browser fetch() does not support file-scheme URLs

I don't think this is a good reason to disallow it in deno. Deno's fetch shouldn't and doesn't behave exactly like browser fetch because in Deno the code is running on a machine that trusts it. That's why we don't need to worry about CORs header stuff, for example.

It would be a surprising behavior if fetch has the capability to access the file system browsers' fetch() cannot.

What about relative URLs? They're allowed in the browser, but disallowed in Deno. I think that's quite sane and unsurprising. Similarly, it it wouldn't be very surprising if the browser disallows file, but Deno allows them (again, because we trust the code). I think the fetch-as-proxy security example is a bit contrived.

  1. There is no precedent for support of file-scheme URLs in Node.js community.

But this is Deno! Viva la revolution! :)

Caveat to all of this is that I'm very new to Deno - so I basically have no idea what I'm talking about.


Separate, but tangentially related to this discussion: The native-file-system people over at WICG were recently discussing fetch characteristics (AbortSignal, streaming, etc.) as it pertains to local files: WICG/file-system-access#56

@Soremwar
Copy link
Contributor

Soremwar commented May 12, 2020

Should this be supported or just stick to the readJson module in the std/fs library?

@kitsonk
Copy link
Contributor

kitsonk commented May 13, 2020

fetch supporting local files is still desirable... it is easier to write isomorphic code that way.

@vwkd
Copy link
Contributor

vwkd commented May 19, 2020

Please don’t make fetch() browser incompatible. Especially not when a file system API already exists with fs.

Wasn’t it “If it’s just nice, don’t do it.”, or something along those lines what Ryan said?

Also there is the Native File System API coming out.

@kitsonk
Copy link
Contributor

kitsonk commented May 19, 2020

Supporting file:// URLs does not make fetch() browser incompatible. Being able to do something in Deno that you can't do in a browsers is not an incompatibility. Also, if you read the above comments, both Chrome and Firefox support file:// under certain conditions.

Assuming Native File System API get more traction, we would support that as well.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 15, 2020

I'm not sure why keeping this open is offending you.

Extending a capability of a browser standard to support situations that make sense for a server runtime is synergistic with the goals of Deno. We don't want to do it trivially or carelessly, but it has and can be done.

@callionica
Copy link

Can I suggest that fetch support for file:// URLs should be enabled, that it should respect the permissions given by —allow-read when accessing file:// URLs, and that the fetch implementation should support range requests on files. Thanks!

@nayeemrmn
Copy link
Collaborator

Ref https://fetch.spec.whatwg.org/#scheme-fetch.

-> "file"

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

When in doubt, return a network error.

@kitsonk kitsonk added feat new feature (which has been agreed to/accepted) ext/fetch related to the ext/fetch labels Nov 5, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Nov 5, 2020

The core team discussed this semi-recently, and the decision was to not make a decision about this right now.

@lowlighter
Copy link
Contributor

When using deno deploy we can fetch local files so unless there's some black magic behind this, it would make senses that it works on vanilla deno too

I can't seem to find a way to make an app capable of reading loading a local json that is compatible for both deploy and deno, because the first one don't have Deno.readFile and the other doesn't support file:// protocol in fetch

@satyarohith
Copy link
Member

When using deno deploy we can fetch local files so unless there's some black magic behind this, it would make senses that it works on vanilla deno too

I can't seem to find a way to make an app capable of reading loading a local json that is compatible for both deploy and deno, because the first one don't have Deno.readFile and the other doesn't support file:// protocol in fetch

deployctl uses https://deno.land/x/file_fetch to support fetching local files.

kitsonk added a commit to kitsonk/deno that referenced this issue Oct 26, 2021
kitsonk added a commit to kitsonk/deno that referenced this issue Oct 28, 2021
kitsonk added a commit to kitsonk/deno that referenced this issue Oct 28, 2021
kitsonk added a commit to kitsonk/deno that referenced this issue Oct 29, 2021
kitsonk added a commit to kitsonk/deno that referenced this issue Oct 29, 2021
kitsonk added a commit to kitsonk/deno that referenced this issue Nov 1, 2021
kitsonk added a commit that referenced this issue Nov 1, 2021
Closes #11925
Closes #2150

Co-authored-by: Bert Belder <bertbelder@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext/fetch related to the ext/fetch feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging a pull request may close this issue.