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

Permissions whitelist #2129

Merged
merged 20 commits into from May 8, 2019

Conversation

5 participants
@afinch7
Copy link
Contributor

commented Apr 16, 2019

Figured I would get the ball rolling on #2128
Still needs tests and some pattern matching.

@afinch7 afinch7 changed the title permissions whitelist [WIP] permissions whitelist Apr 16, 2019

@hayd

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

Is it possible to reuse --allow-net etc for this? To optionally pass args. Maybe using min_values(0)? clap-rs/clap#892

I think that's a cleaner API ... and benefits from no additional flags.

Show resolved Hide resolved cli/permissions.rs
Show resolved Hide resolved cli/permissions.rs
Show resolved Hide resolved cli/flags.rs Outdated
@ejsmith

This comment has been minimized.

Copy link

commented Apr 24, 2019

This looks great!

@afinch7 afinch7 force-pushed the afinch7:permissions_whitelist branch from c638e66 to df18869 Apr 24, 2019

@afinch7 afinch7 force-pushed the afinch7:permissions_whitelist branch from df18869 to bf63ae6 Apr 24, 2019

@afinch7 afinch7 changed the title [WIP] permissions whitelist permissions whitelist May 1, 2019

@afinch7 afinch7 changed the title permissions whitelist Permissions whitelist May 1, 2019

@afinch7 afinch7 force-pushed the afinch7:permissions_whitelist branch from 9f02f05 to 2a10fc0 May 1, 2019

@afinch7 afinch7 force-pushed the afinch7:permissions_whitelist branch from 2a10fc0 to a4bf998 May 1, 2019

@afinch7 afinch7 changed the title Permissions whitelist [WIP] Permissions whitelist May 2, 2019

afinch7 added some commits May 2, 2019

fmt

@afinch7 afinch7 force-pushed the afinch7:permissions_whitelist branch from c9495e3 to 7af6ea4 May 2, 2019

@bartlomieju
Copy link
Contributor

left a comment

@afinch7 I see you took approach with --allow-net=addr1,add2, after all I think it is more ergonomic than separate option 👍

Continuing discussion from previous comments, should we consider a blacklist?

deno --allow-net --deny-net=addr1,addr2

Allow all net connection except addr1 and add2

Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated

@afinch7 afinch7 changed the title [WIP] Permissions whitelist Permissions whitelist May 4, 2019

afinch7 added some commits May 4, 2019

fmt
@hayd

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@bartlomieju I think blacklist should be a follow up (personally I prefer using whitelists). --no-net or --deny-net seem okay, --no is more consistent with other args but semantically it's a little clunky.

@afinch7

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Got everything figured out. I had to make quite a few breaking changes to the way that file paths are resolved for file operations.

afinch7 added some commits May 7, 2019

Show resolved Hide resolved cli/ops.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
Show resolved Hide resolved js/read_dir_test.ts
@ry
Copy link
Collaborator

left a comment

Generally looks good and I'm in favor of this. Nice work!

I have two major requests:

  1. to de-dup the oft-repeated code block in ops.rs with a function
  2. that check_net continue to take a domain name instead of a URL

@afinch7 afinch7 force-pushed the afinch7:permissions_whitelist branch from 39fd1b0 to d418097 May 8, 2019

Show resolved Hide resolved cli/permissions.rs Outdated
@ry

ry approved these changes May 8, 2019

Copy link
Collaborator

left a comment

LGTM - nice work!

I suspect we're going to have to iterate on this for a while still, I think many people will already find this feature useful.

@ry ry merged commit 2edee33 into denoland:master May 8, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.