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

BREAKING CHANGE Remove --allow-run, --allow-plugin #4340

Closed
wants to merge 1 commit into from

Conversation

ry
Copy link
Member

@ry ry commented Mar 12, 2020

Use --allow-all instead. This is both simpler and better reflects what
these permissions imply.

Sets up check_all() for PRs like #4202 to use.

Use --allow-all instead. This is both simpler and better reflects what
these permissions imply.
@hayd
Copy link
Contributor

hayd commented Mar 12, 2020

👎 with this change you can't support run prefixes/conditions e.g. --allow-run=ls would be completely different to --allow-all. This was discussed at length in another issue...

@Soremwar
Copy link
Contributor

@ry Is this temporary?

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Mar 12, 2020

Besides what is described by @hayd, I also have concerns about the meaning of --allow-all magically including other permissions that is not explicitly expressed by some --allow-* flag (at least it should be named as something else like --no-sandbox (or even --unsafe to scare people away) or similar)

@brandonkal
Copy link
Contributor

Honestly I don't see the motivation behind this change. I'd rather see a allow-run whitelist. Requiring all programs that need to run a subprocess have no restrictions in place because of the possibility of invoking an interpreter is wrong.

As I mentioned in the original issue, a prime example of this is --allow-run=helm. If I specify that, I am sure the TypeScript config program itself cannot read environment variables or muck around with the filesystem. In that case, it is more secure because the only privileged actions can be performed by the helm binary which has a known subset of actions (namely pulling and caching repos). But beyond security, there is a better guarantee that the only inputs are those given to the TypeScript program.

I would instead suggest that the breaking change be that --allow-run requires a whitelist. --allow-run=* would be valid and an appropriate console warning would be given that allowing any subprocess is similar to --allow-all if an interpreter is invoked.

@ry
Copy link
Member Author

ry commented Mar 13, 2020

Ok. A lot of negative but valuable feedback, I will retract this.

My worry is that --allow-run does not sufficiently communicate to users that "all bets are off". From The runtime's perspective we can no longer ensure any security. That said, I can see how --allow-run with a whitelist could be useful.

I like @kevinkassimo's idea of --unsafe too.

@ry ry closed this Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants