-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Proposal: Local File Fetch #11925
Comments
I think it would be nice to at least have consistency with This would be particularly important if Deno ever supported something like service workers, in which case Deno's fetching on |
@Jamesernator internally, for local files, a content-type/media type/mime type is not assigned to modules, and it won't be with import assertions either. So there is no consistency to be had. Setting a content-type only assists a very limited set of use cases, and is inconsistent with the one reference implementation behaviour. All the other times when the content-type is set, it is never derived from the file extension. It is supplied by either a server or is part of the URL (in the case of data and blob URLs). Setting it by extension is problematic, especially since the extension |
To be clear, I'm not suggesting you recognize IANA types at all. But rather whenever Deno would interpret a file as having some (content) type, it should treat it as such consistently. i.e. If Deno treats
How does Deno intend to distinguish JSON modules from JS modules if not extensions? Note that the spec specifically forbids using the assertion itself to determine the module type, and for good reason, it completely defeats the point of the feature being an assertion. |
@Jamesernator Internally we have an enum called The key here is that the conversion from |
And to be pedantic,
Like we do other modules, derive the |
This is still fine. The observability comes up with something like service workers, and while I know this is still a proposed feature that might not happen, it would be nice to be consistent both internally and with the web. Just to give an example suppose we have the following module and a way to run it with a service worker (say // main.js
await import("./foo.js");
await fetch(new URL("./foo.js", import.meta.url)); // sw.js
self.addEventListener("fetch", (fetchEvent) => {
fetchEvent.respondWith(async () => {
const response = await fetch(fetchEvent.request);
if (fetchEvent.request.destination === "worker") {
if (response.headers.get('content-type') === "text/javascript") {
// Send this info off for logging
systemLog.log(`Fetched JS worker: ${ fetchEvent.request.url }`);
} else if (response.headers.get('content-type') === "application/json") {
// This makes no sense, log an error
systemLog.error(`Tried to load JSON as worker ${ fetchEvent.request.url }`);
}
}
return response;
});
}); Now consider 3 ways of using
As currently stated, the first one would behave differently to the second two, as Deno is essentially doing the following (simplified based on what you've described): // This is very simplified
const extensionMap = {
"ts": "typescript",
"wasm": "wasm",
"js": "javascript",
"json": "json",
}
const contentTypeMap = {
"text/javascript": "javascript",
"text/typescript": "typescript",
"application/wasm": "wasm",
"application/json": "json",
};
async function resolveModule(url) {
// Note that in this condition we completely ignore the headers
if (url.protocol === "file:") {
const extension = url.pathname.split(".").pop();
const mediaType = extensionMap[extension];
return { mediaType, module: await fetch(url).then(res => res.arrayBuffer()) };
} else if (url.protocol === "http:" || "https:") {
const response = await fetch(url);
const mediaType = contentTypeMap[response.headers.get('content-type')];
return { mediaType, module: await response.arrayBuffer() };
}
// etc
} Notice that on the web OR if This is fairly annoying, as while it is work-around-able (i.e. in this situation the service worker could DUPLICATE Deno's resolution logic), it would be more ideal if Deno put this resolution logic higher up so that service workers don't need to know how Deno will handle it. Now obviously this issue is primarily concerned with service workers, but decisions made now may affect the ability for service workers to correctly integrate with Deno later. |
I think you are trying to solve problems that don't exist. Content via |
The key point here is that Ideally if Deno does ever support service workers such that fetches are observable, then Deno would also trigger a fetch event for And yes like I've said this issue is basically contingent on Deno implementing service workers, but the point is that module loading on the web and within Deno would lead to observably different behaviour inside a service worker depending on whether it was loaded in Deno vs on the web. Service workers are already really tricky to get right even ignoring platform differences, the more platform differences there are the harder it would be to correctly utilize service worker features to write service worekrs that are polymorphic between Deno and the Web. Now perhaps polymorphic service workers, or service workers at all, is not a goal Deno wants to pursue, but decisions such as this issue WILL affect the behaviour of, and ability to write code for, service workers if Deno were to support them later. Now I don't know if these things even would be a small or large problem, the difficultly here is it's really hard to evaluate what working with service workers for Deno would be like without an implementation, as such I feel like it would be considerably safer to be as closer to typical web behaviour as possible so that surprises don't come up later. But it would sad if Deno implemented Having said that I'm not familiar enough with Deno's policy on breaking backwards compatibility either, it may be the case that Deno is happy to break code that does exceedingly fragile things, or Deno might be more cautious like the Web is. I don't know where Deno lies, but if Deno does lie towards the more cautious side like the Web does with backwards compatibility, then these sort've things really need to considered with a fine-tooth comb to ensure future compatibility. Of course if Deno doesn't have as cautious an approach as the Web with breaking backwards compatibility over time, then basically all of my comments here can be disregarded as Deno won't be as constrained to revise these sort've small decisions in future. |
Again, you are solving problems that don't exist. There is no one working on service workers anytime soon, the core team isn't going to work on it, and we would strongly caution anyone else wanting to work on it, because there are some complex issues that need to be addressed, including the fact that the way we handle imports of modules and fetch are two entirely different code paths. If service workers did come, imported file URLs would work in the same way they do today. |
Hey, security concerns were raised when implementing this for Node's fetch. What does Deno do when I:
Isn't this a problem? What is the expected way to use this safely? |
@benjamingr |
@nayeemrmn yes but it's a lot more likely to accept user input to |
@benjamingr Local file fetch only requires If you pass a URL to your app unfiltered, then there is probably a pretty big issue in your system. You can prevent local file fetches by just checking that the scheme is only |
If you write a server that proxies data or a server that access's a URL and assesses something about it (e.g. accessibility) then there is a good chance you accept URLs for user input (happy to share more use cases if that helps).
Sure though
The question is whether requiring anyone using I'm not sure I know the answer it's just a concern raised I wanted to bring with you since people have expressed concern over this in Node.js. |
I'm sure the concern still exists here in some form but Deno has always recommended that people use allow lists with |
Totally agree @benjamingr. It's an insane choice to allow fetching file URIs. There are many, many circumstances where this is a major security vulnerability. Here are just a few:
WTF! How on earth did this pass? #12545 should be reverted. Imagine if Chrome or FireFox let random websites fetch anything from your hard drive. The Deno experience should not be insecure by default. Deno permissions don't even help this, because of course you have to allow Deno to read your app config, thereby making it fetchable with file URIs. Why was this solution not good enough?: https://deno.land/x/file_fetch@0.2.0 It was proposed before the merge. It makes Deno FS calls when you supply a file URI. If you want to do weird fetch stuff, you should create a library to wrap fetch. It was already done. Now that it's merged the opposite is true: we will have to create a custom "secure" fetch wrapper function in all our projects. EDIT: Here it is: /**
* Mitigates Deno's vulnerable fetch implementation.
* https://github.com/denoland/deno/issues/11925#issuecomment-1678084346
*/
const safeFetch: typeof fetch = (...args) => {
const url = getUrl(args[0]);
if (url.startsWith('https://')) {
return fetch(...args);
} else {
throw new Error('Invalid URL');
}
};
function getUrl(value: string | URL | Request): string {
if (typeof value === 'string') return value;
if (value instanceof URL) return value.toString();
if (value instanceof Request) return value.url;
throw new Error('Invalid value');
}
export { safeFetch }; EDIT2: See also: https://gitlab.com/soapbox-pub/mostr/-/merge_requests/66/diffs |
Allowing file system access through the fetch API in Deno is a significant security concern, and I would strongly urge you to reconsider this patch. Permitting file system access in this manner exposes sensitive files and directories to unauthorized access. This is particularly concerning because the fetch API is traditionally associated with retrieving resources over a network, not direct access to the local file system. In Deno, strict security measures are enforced through a secure sandbox environment, which necessitates explicit permission for file system access. This recent change bypasses these controls, leading to a conflict with Deno's underlying security philosophy. If accessing files through this approach does not require the user to pass additional permissions to the Deno compiler, it violates the promised secure sandboxing and undermines the trust users have placed in Deno's security model. |
I created a import { safeFetch } from 'https://gitlab.com/soapbox-pub/deno-safe-fetch/-/raw/develop/mod.ts';
// Use it normally:
const response = await safeFetch('https://example.com');
// This throws an error:
const file = await safeFetch('file:///etc/passwd'); Replace fetch globally: import 'https://gitlab.com/soapbox-pub/deno-safe-fetch/-/raw/develop/load.ts'; |
@alexgleason I think your concerns are the result of a misunderstanding of both Prior to landing this PR,
Unless you already do filtering on the With the introduction of The module you present ( Finally, I'd like to mention that our behaviour is entirely compliant with the Fetch spec - grep "file: URLs are left as an exercise for the reader". |
@elaine-jackson You are misunderstanding. Deno requires |
Well, thanks for letting me know, but how is that acceptable? EDIT: I updated the |
@ry Have you seen this? Because I think this is the setup to a talk called "Things I Regret About Deno" |
@alexgleason Your mitigation does not solve the problem by the way, because I can just have a DNS entry on a custom domain that points to 127.0.0.1, or CNAMEs to another internal endpoint, or anything else. You can not protect against localhost access attacks using text based filtering of the hostname. I think you have fundamentally misunderstood what the security model of the As the conversation has veered off the original topic, I'd urge you to open a GitHub discussion if you'd like to discuss this further. Please also take note of our code of conduct (https://deno.co/coc) - discussion should be civil, and there is seldom a "right" answer to a technical question. Every design or implementation choice carries a trade-off and numerous costs. |
Context
Currently,
fetch()
does not allowfile://
schemes to be fetched. The inability to do this becomes a significant usability issue, making code less portable. It has become quite common in Deno to write isomorphic code that usesimport.meta.url
as a base for accessing a resource when writing server code. Currently a user would have to code path to determine if the resource they are trying to access is local or on the network at a code level.It was requested in #2150 is the 10th most 👍 open issue at the time of this writing.
That being said, the web platform does not support it, because of the security considerations and undefined behavior. Specifically the Fetch Living Standard says:
The specification does not prohibit
file
protocols, but the entire behavior is undefined.And Node.js libraries providing the
fetch()
API have decided not to implement local file URLs, partly because of the undefined behavior and the security concerns that come with Node.js having a trust-by-default model.Firefox
Firefox is the only mainstream browser to support local "file://" URLs well and would be the only one considered as setting precedence with local file
fetch()
.Since Firefox 67, by default, local files create their own unique opaque origin, instead of sharing an origin (ref). This means the only file that can be fetched is the file itself. For example scripts that come from
file:///example/test.html
can only dofetch("file:///example/test.html")
orfetch("./test.html")
. Every other one will displayCross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at
in the console and throw a network error.If you change the config option
privacy.file_unique_origin
tofalse
, then you can fetch local files from pages that have a local origin. There are the observed behaviours of fetching local files this way:window.location
window.location.origin
is opaque, irrespective of it being unique per file or not.HEAD
returns with a body.Cross-Origin Request Blocked
error is logged to the console, and a network error is thrown. (I suspect the behaviour around theCross-Origin Request Blocked
is part of the security mitigation to limit the ability of scripts to try to detect if theprivacy.file_unique_origin
istrue
orfalse
).Since you have to do advanced configuration to be able to enable this, it feels like its behaviours shouldn't overly influence, as most code in the wild wouldn't expect to be able to fetch local files.
Solution
Scheme Fetch
Building upon 4.2. Scheme fetch and switch on the current URL's scheme, and running the associated steps:
↪
"file"
GET
, then return a network error.op_open_async
with an options argument of path set to the result ofpathFromURL()
for the url and options set to read to true.FileStream
constructed with therid
from result.OK
.stream
.Body used
TBC steps to close the
FileStream
.FileStream
An internal class which encapsulates a success result of
op_open_async
and performsop_read_async
to read and enqueue chunks.The
readable
should have a similar chunk queuing strategy to reading the body of a network request, in that it uses"byte"
type and thepull()
algorithm which provides back pressure on the stream.Considerations
op_open_async
and be surfaced in a meaningful way. Because this builds upon the Fetch specification, any error condition, including security or non-found (for like a blob URL) return network errors.window.location
and worker.location
, then we could consider it, but usingimport.meta.url
is the only viable near-term solution.HEAD
requests, etc. could be added in the future. It feels appropriate to simply focus on a minimum-viable solution.content-length
header is not set, as the implementation reads the file in a streaming fashion, meaning that the length of the content can change as the content is being read. Therefore consumers who need to "forward" the content length will need to calculate the length through some other mechanism. (Also, note that this header is not set on Firefox when fetching local files).The text was updated successfully, but these errors were encountered: