lumeland / lume

🔥 Static site generator for Deno 🦕
https://lume.land
MIT License
1.79k stars 77 forks source link

Use minimum system permissions necessary #261

Closed cmoog closed 1 year ago

cmoog commented 1 year ago

I noticed that the default dev script invokes itself with full permissions over the entire system. Ideally, Lume should function fine if invoked with the following minimum permissions.

$ deno run --allow-net=localhost:3000 --allow-read=./ --allow-write=./_site lume/task.ts
oscarotero commented 1 year ago

It's not so easy, for several reasons:

I'd like to simplify this in the near future, when this proposal is implemented: https://github.com/denoland/deno/issues/12763 so you can configure the permissions in the Deno config file.

cmoog commented 1 year ago

This depends on the site configuration.

Point taken. Then the minimum permissions for most projects would be --allow-net=localhost --allow-read=./ --allow-write=./. If some plugin needs to make other network requests, the user should need to explicitly allow that.

deno eval has implicit all permissions (it's like --allow-all).

After reading this issue, I can't help but feel the current behavior is a big mistake. If my understanding is correct, the lack of support for import maps in deno run means the only ergonomic way to invoke "framework" CLIs is without any sandboxing. This erases a major benefit of Deno's security model.

And to preempt, I don't think this is bikeshedding. Given that Lume is plugin-based, it's plausible that there will be a long tail of unaudited, untrusted plugins. Why should plugins have exec permissions?

Potential (temporary) solution

I haven't thought about this too much... but this came across my mind as a potential solution. This way, only one line of code runs with global permissions, then the task.ts process itself is sandboxed.

{
  "importMap": "import_map.json",
  "tasks": {
    "lume": "deno run --allow-read=./ --allow-write=./ --allow-net=localhost $(deno eval \"console.log(JSON.parse(Deno.readTextFileSync('import_map.json')).imports['lume/'])\")task.ts",
    "build": "deno task lume",
    "serve": "deno task lume -s"
  }
}

Of course this would also require changes to ci.ts and task.ts to remove the unnecessary self-invocation.

oscarotero commented 1 year ago

task.ts and ci.ts works as wrappers to execute the real CLI script (cli.ts). This wrapper fixes different issues. So, restricting permissions to these files has no effect to cli.ts(AFAIK, permissions are not propagated between childprocesses: a file with only --allow-run could run a script passing the --allow-all argument).

The use cases are so variable that this is the reason I've decided to run cli.ts with --allow-all, for example:

Restricting the permissions right now can break many sites. I think it's better to let the users configure the permisisions once Deno team implements permission configuration in the deno.json file. In Lume 2.0 I'm planning to remove these wrapper files and depend only on deno.json that will be mandatory (unlike now, that both deno.json and import_map.json files are optional).

If you're concerned about that, the correct way to apply permissions is by running the cli.ts file directly. You can create a task like this:

{
  "importMap": "import_map.json",
  "tasks": {
    "lume": "deno run --unstable --allow-read=./ --allow-write=./ --allow-net=localhost https://lume.land/x/lume@1.11.4/cli.ts",
    "build": "deno task lume",
    "serve": "deno task lume -s"
  }
}

This conversation makes me think that a Permissions section in the documentation website would be really useful for users that want to restrict these permissions.

bryophyta commented 1 year ago

This conversation makes me think that a Permissions section in the documentation website would be really useful for users that want to restrict these permissions.

This sounds like a great idea! It will also be useful for raising awareness of permissions considerations among users who wouldn't otherwise be thinking about it.

I don't think I know enough about Deno's permissions API or Lume's requirements to write this myself, but I'd be very happy to help with writing this documentation if anyone feels like collaborating on it.

oscarotero commented 1 year ago

Permissions are explained here: https://lume.land/docs/advanced/permissions/ Closing this.