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

Feat: Configure permission profiles to shorten task commands, like deno run --permission-profile main-script main.ts #26372

Open
brianpeiris opened this issue Oct 17, 2024 · 7 comments
Labels
suggestion suggestions for new features (yet to be agreed) task runner related to deno task

Comments

@brianpeiris
Copy link

brianpeiris commented Oct 17, 2024

Task commands for a simple application look like this:

{
  "tasks": {
    "start": "deno run --allow-env --allow-read --allow-run npm:concurrently 'deno run server' 'deno run bundle'",
    "bundle": "deno run --allow-env --allow-read --allow-run npm:esbuild src/main.ts --watch --bundle --outfile=public/main.bundle.js",
    "serve": "deno serve --allow-env --allow-sys --allow-net --allow-read --watch src/server.ts"
  }
}

As I'm sure others have noted, this is quite verbose and it's difficult to read or edit the end of the command, which is what I usually care about.

I'm proposing a new configuration section and a new CLI flag for deno run and deno serve, --permission-profile or -P, to define permission profiles for improved readability and reduced repetition:

{
  "tasks": {
    "start": "deno run -P tool npm:concurrently 'deno run server' 'deno run bundle'",
    "bundle": "deno run -P tool npm:esbuild src/main.ts --watch --bundle --outfile=public/main.bundle.js",
    "serve": "deno serve -P server --watch src/server.ts"
  },
  "permissionProfiles": {
    "tool": "--allow-env --allow-read --allow-run",
    "server": "--allow-env --allow-sys --allow-net --allow-read"
  }
}
@littledivy littledivy added feat new feature (which has been agreed to/accepted) task runner related to deno task labels Oct 18, 2024
@brianpeiris
Copy link
Author

Just noticed there is a similar proposal in #12763 with a lot of discussion, but I'd like to keep this open because I think it's a better approach in terms of simplicity and reusability. In my proposal, the profile names just serve as pointers to a set of flags, so you don't have to invent new configuration structures or syntax for specifying permission options. The implementation would also be simpler because you'd effectively just do a string substitution before running the command.

@ycmjason
Copy link

ycmjason commented Oct 19, 2024

I do think a combination of both ideas can be a good idea. I like #12763 proposal for allowing more structured way of defining permission, and I also like this proposal for being able to define mutiple permission presets.

So I am envisioning something like:

{
  "permissionProfiles": {
    "default": { // used when no --permission-profile is given?
      "allow-all": true,
      "allow-env": true,
      "allow-read": [ ".", "/tmp" ],
      "allow-write": ".",
      "allow-net": "deno.land,nest.land"
    },
    "server": {
      "allow-net": true
    }
  }
}

In my project, unit / integration and e2e tests all have different permissions to set. So this can really help with the situation!

@catuhana
Copy link
Contributor

If someone was to use permissionProfiles only for tasks, I think it would be unnecessary complexity. I feel maybe something like this could be good, where you don't need to use permissionProfiles if you're going to use it only for tasks.

{
  "permissionProfiles": {
    "test": {
      "allow-read": true
    }
  },
  "tasks": {
    "test": {
      "script": "deno test",
      "permissions": {
        "profile": "test" // Will override `allow-*` fields if specified.
        // "allow-*": <value> -- If no `profile` is specified, these could be used.
      }
    }
  }
}

or

{
  "tasks": {
    "test": ["deno test", {
      "permissions": {} // same as above
    }]
  }
}

I don't think these look the prettiest (also considering, deno's config is all flattened now, so first one doesn't actually fit the current schema), but I feel like this is a better approach.

@brianpeiris
Copy link
Author

Thanks for the comments. I still do prefer the command line flags, if only so that I don't have to learn a new way of specifying permissions and permission options, but I'd be happy with a compromise that includes multiple profiles. I also like the idea of a special "default" profile that is used when a profile is not specified with --permission-profile or -P.

@catuhana Can you clarify why you think the command line flag proposal would be unnecessary complexity? Is it just that you prefer to specify permissions along side the task, instead of a separate permissionProfiles section? I prefer profiles since they are likely to be reused, but I think you would still be allowed to specify individual permissions flags without a profile.

I can understand why people may prefer a json structure instead of command line flags. I'm not strongly against that, so it's up to everyone and the maintainers to decide, but I would still like to keep the idea of reusable profiles as well.

@ycmjason
Copy link

ycmjason commented Oct 20, 2024

In my project, the integration test task currently looks like:

    "test:integration": "deno test --lock --unsafely-ignore-certificate-errors=localhost --allow-net=\"localhost,127.0.0.1,8.8.8.8,[2001:4860:4860::8888]\" integration",

With just 4 hostnames in --allow-net has already made it quite unreadable imo. I am tempted to just do --allow-net if I were to add even more hostnames in the future, otherwise it will just become very hard to maintain.

So I think a structured way to define these permissions would be a nice way to promote more secure habbits.

In my project, I think it's essential to define the exact hostnames in --allow-net to ensure no other requests are made during the integration test.


Further, it might be a good idea not to limit profiles to only allow "permissions" (I assume these are all the --alow-* flags?) From the example that I've given, perhaps allowing any arbitrary flags for deno test / run would be incredibly useful?

@catuhana
Copy link
Contributor

@catuhana Can you clarify why you think the command line flag proposal would be unnecessary complexity? Is it just that you prefer to specify permissions along side the task, instead of a separate permissionProfiles section? I prefer profiles since they are likely to be reused, but I think you would still be allowed to specify individual permissions flags without a profile.

As I mentioned on my initial comment: If a user wants to use permission things only for tasks, having to create new permissionProfiles and using the flag is personally too much bother. We can still have permission profiles and use them, but we should not make it the only way of using permissions in other things like tasks.

@lucacasonato lucacasonato added suggestion suggestions for new features (yet to be agreed) and removed feat new feature (which has been agreed to/accepted) labels Oct 21, 2024
@Xiphe
Copy link

Xiphe commented Dec 2, 2024

Really like this idea. Just had a similar approach using configuration files instead of profiles: #27190 - I'm fine with either direction. Happy to help moving this forward <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion suggestions for new features (yet to be agreed) task runner related to deno task
Projects
None yet
Development

No branches or pull requests

6 participants