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
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

added net tests and parsing for urls

  • Loading branch information...
afinch7 committed May 1, 2019
commit a4bf99847b624e2f4a917058e5b9dcabd34a06b1
@@ -229,11 +229,19 @@ impl DenoPermissions {
}
}

pub fn check_net(&self, domain_name: &str) -> DenoResult<()> {
pub fn check_net(&self, url_str: &str) -> DenoResult<()> {
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@ry

ry May 7, 2019

Collaborator

I don't think check_net should take a URL. You can add a new method which checks a URL, but, for example, Deno.dial() does not use URLs.

This comment has been minimized.

Copy link
@afinch7

afinch7 May 8, 2019

Author Contributor

I'm going to refactor this to origin(host/domain + optional port) for now. I still think we may want to move to full urls at some point for more parsing consistency, but I'll save that for another PR. For dial() it wouldn't be that hard to just add the protocol on for a full url I.E. tcp://localhost:1234.

This comment has been minimized.

Copy link
@ry

ry May 8, 2019

Collaborator

Hm... Ok - let's consider that in a separate PR - this one is large enough as it is.

This comment has been minimized.

Copy link
@hayd

hayd May 8, 2019

Contributor

One reason url might be useful is restricting libs, e.g. deno.land/std or deno.land/x/oak (rather than everything in the registry deno.land/x/).

This comment has been minimized.

Copy link
@hayd

hayd May 8, 2019

Contributor

Thinking about it host+port is probably sufficient for requests, I was confusing this with imports which is less relevant here.

match self.allow_net.get_state() {
PermissionAccessorState::Allow => Ok(()),
state => {
match self.net_whitelist.contains(domain_name) {
let parsed_url_result = url::Url::parse(url_str);
let whitelist_result = match parsed_url_result {
Ok(parsed_url) => match parsed_url.host() {
Some(host) => self.net_whitelist.contains(&host.to_string()),
None => false,
},
Err(_) => false,
};
match whitelist_result {
true => Ok(()),
false => {
match state {
@@ -242,7 +250,7 @@ impl DenoPermissions {
PermissionAccessorState::Ask => {
match self.try_permissions_prompt(&format!(
"network access to \"{}\"",
domain_name
url_str
)) {
Err(e) => Err(e),
Ok(v) => {
@@ -434,22 +442,72 @@ mod tests {
..Default::default()
});

// Inside of /a/specific and /a/specific/dir/name
perms.check_read("/a/specific/dir/name").unwrap();
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@ry

ry May 7, 2019

Collaborator

Nit: Rather than doing .unwrap() (which works) I think it would be more clear if you did

assert!(perms.check_read("/a/specific/dir/name").is_ok());

or

assert_eq!(Ok(()), perms.check_read("/a/specific/dir/name"));
perms.check_write("/a/specific/dir/name").unwrap();

// Inside of /a/specific but outside of /a/specific/dir/name
perms.check_read("/a/specific/dir").unwrap();
perms.check_write("/a/specific/dir").unwrap();

// Inside of /a/specific and /a/specific/dir/name
perms.check_read("/a/specific/dir/name/inner").unwrap();
perms.check_write("/a/specific/dir/name/inner").unwrap();

// Inside of /a/specific but outside of /a/specific/dir/name
perms.check_read("/a/specific/other/dir").unwrap();
perms.check_write("/a/specific/other/dir").unwrap();

// Exact match with /b/c
perms.check_read("/b/c").unwrap();
perms.check_write("/b/c").unwrap();

// Sub path within /b/c
perms.check_read("/b/c/sub/path").unwrap();
perms.check_write("/b/c/sub/path").unwrap();

// Inside of /b but outside of /b/c
perms.check_read("/b/e").unwrap_err();
perms.check_write("/b/e").unwrap_err();

// Inside of /a but outside of /a/specific
perms.check_read("/a/b").unwrap_err();
perms.check_write("/a/b").unwrap_err();
}

#[test]
fn check_domains() {
let perms = DenoPermissions::from_flags(&DenoFlags {
net_whitelist: vec![
"localhost".to_string(),
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju May 3, 2019

Contributor

Can you add a case with port? Eg. localhost:4500

"deno.land".to_string(),
"127.0.0.1".to_string(),
].to_vec(),
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@ry

ry May 7, 2019

Collaborator

.to_vec() unnecessary

no_prompts: true,
..Default::default()
});

// http and https for the same domain should pass
perms.check_net("http://localhost/test/url").unwrap();
perms.check_net("https://localhost/test/url").unwrap();
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@ry

ry May 7, 2019

Collaborator

This seems very odd that it operates on URLs. I think it should only be passed a host.


// Correct domain should pass but typo should not
perms
.check_net("https://deno.land/std/http/file_server.ts")
.unwrap();
perms
.check_net("https://deno.lands/std/http/file_server.ts")
.unwrap_err();

// Correct ipv4 address should succeed but type should not
perms.check_net("https://127.0.0.1").unwrap();
perms.check_net("https://127.0.0.2").unwrap_err();
// TODO(afinch7) This currently succeeds but we may want to
// change this behavior in the future.
perms.check_net("https://127.0.0.1:3000").unwrap();

// Completely different hosts should also fail
perms.check_net("https://somedomain/").unwrap_err();
perms.check_net("https://192.168.0.1/").unwrap_err();
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.