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(cli): shorthands for --allow-* flags #19549

Closed
wants to merge 1 commit into from

Conversation

samyosm
Copy link

@samyosm samyosm commented Jun 18, 2023

As per #19530, adds the following short flags:

short long
-E --allow-env
-F --allow-ffi
-T --allow-hrtime
-N --allow-net
-R --allow-read
-X --allow-run
-S --allow-sys
-W --allow-write

@CLAassistant
Copy link

CLAassistant commented Jun 18, 2023

CLA assistant check
All committers have signed the CLA.

@samyosm samyosm marked this pull request as ready for review June 18, 2023 20:55
@aapoalas aapoalas requested review from ry and dsherret July 4, 2023 18:17
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm against this because it degrades the permission system by obscuring the meaning of the permission flags.

@samyosm
Copy link
Author

samyosm commented Jul 4, 2023

How could it not? All shorthands do; you can't preserve much meaning with a single letter. They only allow those that are aware of them to be quicker and more productive. E.g. ls -A instead of ls --almost-all.

@dsherret
Copy link
Member

dsherret commented Jul 4, 2023

How could it not? All shorthands do; you can't preserve much meaning with a single letter.

Exactly. That's a huge issue for people who aren't aware of what the shorthand flags mean because this relates to Deno's security model. Looking at a deno command on the command line should be easily auditable for what permissions are being allowed.

@samyosm
Copy link
Author

samyosm commented Jul 4, 2023

I'd argue that shorthands are as easily auditable as their longer form the more people use them. That's why some people aren't even aware of ls --all. Once people get a hand of shorthands they will prefer to use them. This is a similar situation to vim or tailwind.

@dsherret
Copy link
Member

dsherret commented Jul 4, 2023

I'd argue that shorthands are as easily auditable as their longer form the more people use them. That's why some people aren't even aware of ls --all. Once people get a hand of shorthands they will prefer to use them. This is a similar situation to vim or tailwind.

I disagree with the ls, vim, and tailwind comparison because those don't expand a security sandbox.

I think it's less auditable because it requires looking up or knowing what each shorthand does. It also requires people to be on their toes about new shorthand flags that could open up the security sandbox. It's especially harmful to beginners who might accidentally open their security sandbox for scripts they don't want to because they never realized Deno even had short non-descriptive flags that opened the security sandbox and they just copy and pasted some command.

@samyosm
Copy link
Author

samyosm commented Jul 4, 2023

I guess you are right. I'll wait the ending words of #19530 before closing this PR.

@catuhana
Copy link
Contributor

I don't think this should land as stated by reviewer, but if its going to happen somehow, instead of reserving new ones I think these short flags make more sense:

Short Long
-Ae --allow-env
-Af --allow-ffi
-At --allow-hrtime
-An --allow-network
-Ar --allow-read
-Ax --allow-run
-As --allow-sys
-Aw --allow-write

@bartlomieju
Copy link
Member

Thank you for the PR @samyosm, but as it stands there's no support for this features in the core team. We are exploring alternative ways of making it more convenient to specify which permissions to apply including #12763. I'm going to close it without a merge.

@bartlomieju bartlomieju closed this Sep 3, 2023
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