From 7c60ab46643d3190d1734678e085bc304c5f7813 Mon Sep 17 00:00:00 2001 From: EnokMan <416828041@qq.com> Date: Wed, 23 Oct 2019 22:19:27 +0800 Subject: [PATCH] refactor DenoPermissions.check_net & resolve_addr (#3182) --- cli/ops/net.rs | 16 ++------ cli/ops/tls.rs | 17 +++------ cli/permissions.rs | 65 ++++++++++++++----------------- cli/resolve_addr.rs | 93 +++++++++++++++------------------------------ cli/state.rs | 4 +- core/modules.rs | 2 +- 6 files changed, 70 insertions(+), 127 deletions(-) diff --git a/cli/ops/net.rs b/cli/ops/net.rs index b3450222ba..1c5aa6edd4 100644 --- a/cli/ops/net.rs +++ b/cli/ops/net.rs @@ -70,13 +70,9 @@ fn op_dial( let args: DialArgs = serde_json::from_value(args)?; assert_eq!(args.transport, "tcp"); // TODO Support others. - // TODO(ry) Using format! is suboptimal here. Better would be if - // state.check_net and resolve_addr() took hostname and port directly. - let address = format!("{}:{}", args.hostname, args.port); + state.check_net(&args.hostname, args.port)?; - state.check_net(&address)?; - - let op = resolve_addr(&address).and_then(move |addr| { + let op = resolve_addr(&args.hostname, args.port).and_then(move |addr| { TcpStream::connect(&addr) .map_err(ErrBox::from) .and_then(move |tcp_stream| { @@ -141,13 +137,9 @@ fn op_listen( let args: ListenArgs = serde_json::from_value(args)?; assert_eq!(args.transport, "tcp"); - // TODO(ry) Using format! is suboptimal here. Better would be if - // state.check_net and resolve_addr() took hostname and port directly. - let address = format!("{}:{}", args.hostname, args.port); + state.check_net(&args.hostname, args.port)?; - state.check_net(&address)?; - - let addr = resolve_addr(&address).wait()?; + let addr = resolve_addr(&args.hostname, args.port).wait()?; let listener = TcpListener::bind(&addr)?; let local_addr = listener.local_addr()?; let resource = resources::add_tcp_listener(listener); diff --git a/cli/ops/tls.rs b/cli/ops/tls.rs index a0f4197baa..569b5a1f67 100644 --- a/cli/ops/tls.rs +++ b/cli/ops/tls.rs @@ -55,23 +55,19 @@ pub fn op_dial_tls( _zero_copy: Option, ) -> Result { let args: DialTLSArgs = serde_json::from_value(args)?; - - // TODO(ry) Using format! is suboptimal here. Better would be if - // state.check_net and resolve_addr() took hostname and port directly. - let address = format!("{}:{}", args.hostname, args.port); let cert_file = args.cert_file; - state.check_net(&address)?; + state.check_net(&args.hostname, args.port)?; if let Some(path) = cert_file.clone() { state.check_read(&path)?; } - let mut domain = args.hostname; + let mut domain = args.hostname.clone(); if domain.is_empty() { domain.push_str("localhost"); } - let op = resolve_addr(&address).and_then(move |addr| { + let op = resolve_addr(&args.hostname, args.port).and_then(move |addr| { TcpStream::connect(&addr) .and_then(move |tcp_stream| { let local_addr = tcp_stream.local_addr()?; @@ -189,13 +185,10 @@ fn op_listen_tls( let args: ListenTlsArgs = serde_json::from_value(args)?; assert_eq!(args.transport, "tcp"); - // TODO(ry) Using format! is suboptimal here. Better would be if - // state.check_net and resolve_addr() took hostname and port directly. - let address = format!("{}:{}", args.hostname, args.port); let cert_file = args.cert_file; let key_file = args.key_file; - state.check_net(&address)?; + state.check_net(&args.hostname, args.port)?; state.check_read(&cert_file)?; state.check_read(&key_file)?; @@ -204,7 +197,7 @@ fn op_listen_tls( .set_single_cert(load_certs(&cert_file)?, load_keys(&key_file)?.remove(0)) .expect("invalid key or certificate"); let acceptor = TlsAcceptor::from(Arc::new(config)); - let addr = resolve_addr(&address).wait()?; + let addr = resolve_addr(&args.hostname, args.port).wait()?; let listener = TcpListener::bind(&addr)?; let local_addr = listener.local_addr()?; let resource = resources::add_tls_listener(listener, acceptor); diff --git a/cli/permissions.rs b/cli/permissions.rs index f57732589d..1dd2eb1e25 100644 --- a/cli/permissions.rs +++ b/cli/permissions.rs @@ -208,28 +208,19 @@ impl DenoPermissions { } } - pub fn check_net(&self, host_and_port: &str) -> Result<(), ErrBox> { - let msg = &format!("network access to \"{}\"", host_and_port); + pub fn check_net(&self, hostname: &str, port: u16) -> Result<(), ErrBox> { + let msg = &format!("network access to \"{}:{}\"", hostname, port); match self.allow_net.get_state() { PermissionAccessorState::Allow => { self.log_perm_access(msg); Ok(()) } _state => { - let parts = host_and_port.split(':').collect::>(); - if match parts.len() { - 2 => { - if self.net_whitelist.contains(parts[0]) { - true - } else { - self - .net_whitelist - .contains(&format!("{}:{}", parts[0], parts[1])) - } - } - 1 => self.net_whitelist.contains(parts[0]), - _ => panic!("Failed to parse origin string: {}", host_and_port), - } { + if self.net_whitelist.contains(hostname) + || self + .net_whitelist + .contains(&format!("{}:{}", hostname, port)) + { self.log_perm_access(msg); Ok(()) } else { @@ -438,26 +429,26 @@ mod tests { }); let domain_tests = vec![ - ("localhost:1234", true), - ("deno.land", true), - ("deno.land:3000", true), - ("deno.lands", false), - ("deno.lands:3000", false), - ("github.com:3000", true), - ("github.com", false), - ("github.com:2000", false), - ("github.net:3000", false), - ("127.0.0.1", true), - ("127.0.0.1:3000", true), - ("127.0.0.2", false), - ("127.0.0.2:3000", false), - ("172.16.0.2:8000", true), - ("172.16.0.2", false), - ("172.16.0.2:6000", false), - ("172.16.0.1:8000", false), + ("localhost", 1234, true), + ("deno.land", 0, true), + ("deno.land", 3000, true), + ("deno.lands", 0, false), + ("deno.lands", 3000, false), + ("github.com", 3000, true), + ("github.com", 0, false), + ("github.com", 2000, false), + ("github.net", 3000, false), + ("127.0.0.1", 0, true), + ("127.0.0.1", 3000, true), + ("127.0.0.2", 0, false), + ("127.0.0.2", 3000, false), + ("172.16.0.2", 8000, true), + ("172.16.0.2", 0, false), + ("172.16.0.2", 6000, false), + ("172.16.0.1", 8000, false), // Just some random hosts that should err - ("somedomain", false), - ("192.168.0.1", false), + ("somedomain", 0, false), + ("192.168.0.1", 0, false), ]; let url_tests = vec![ @@ -502,8 +493,8 @@ mod tests { assert_eq!(*is_ok, perms.check_net_url(&u).is_ok()); } - for (domain, is_ok) in domain_tests.iter() { - assert_eq!(*is_ok, perms.check_net(domain).is_ok()); + for (host, port, is_ok) in domain_tests.iter() { + assert_eq!(*is_ok, perms.check_net(host, *port).is_ok()); } } } diff --git a/cli/resolve_addr.rs b/cli/resolve_addr.rs index b783444d86..5a4c9d54b4 100644 --- a/cli/resolve_addr.rs +++ b/cli/resolve_addr.rs @@ -1,5 +1,4 @@ // Copyright 2018-2019 the Deno authors. All rights reserved. MIT license. -use crate::deno_error; use deno::ErrBox; use futures::Async; use futures::Future; @@ -7,21 +6,17 @@ use futures::Poll; use std::net::SocketAddr; use std::net::ToSocketAddrs; -/// Go-style network address parsing. Returns a future. -/// Examples: -/// "192.0.2.1:25" -/// ":80" -/// "[2001:db8::1]:80" -/// "198.51.100.1:80" -/// "deno.land:443" -pub fn resolve_addr(address: &str) -> ResolveAddrFuture { +/// Resolve network address. Returns a future. +pub fn resolve_addr(hostname: &str, port: u16) -> ResolveAddrFuture { ResolveAddrFuture { - address: address.to_string(), + hostname: hostname.to_string(), + port, } } pub struct ResolveAddrFuture { - address: String, + hostname: String, + port: u16, } impl Future for ResolveAddrFuture { @@ -32,26 +27,14 @@ impl Future for ResolveAddrFuture { // The implementation of this is not actually async at the moment, // however we intend to use async DNS resolution in the future and // so we expose this as a future instead of Result. - match split(&self.address) { - None => Err(deno_error::invalid_address_syntax()), - Some(addr_port_pair) => { - // I absolutely despise the .to_socket_addrs() API. - let r = addr_port_pair.to_socket_addrs().map_err(ErrBox::from); - r.and_then(|mut iter| match iter.next() { - Some(a) => Ok(Async::Ready(a)), - None => panic!("There should be at least one result"), - }) - } - } - } -} - -fn split(address: &str) -> Option<(&str, u16)> { - address.rfind(':').and_then(|i| { - let (a, p) = address.split_at(i); // Default to localhost if given just the port. Example: ":80" - let addr = if !a.is_empty() { a } else { "0.0.0.0" }; + let addr: &str = if !self.hostname.is_empty() { + &self.hostname + } else { + "0.0.0.0" + }; + // If this looks like an ipv6 IP address. Example: "[2001:db8::1]" // Then we remove the brackets. let addr = if addr.starts_with('[') && addr.ends_with(']') { @@ -60,13 +43,14 @@ fn split(address: &str) -> Option<(&str, u16)> { } else { addr }; + let addr_port_pair = (addr, self.port); + let r = addr_port_pair.to_socket_addrs().map_err(ErrBox::from); - let p = p.trim_start_matches(':'); - match p.parse::() { - Err(_) => None, - Ok(port) => Some((addr, port)), - } - }) + r.and_then(|mut iter| match iter.next() { + Some(a) => Ok(Async::Ready(a)), + None => panic!("There should be at least one result"), + }) + } } #[cfg(test)] @@ -77,36 +61,19 @@ mod tests { use std::net::SocketAddrV4; use std::net::SocketAddrV6; - #[test] - fn split1() { - assert_eq!(split("127.0.0.1:80"), Some(("127.0.0.1", 80))); - } - - #[test] - fn split2() { - assert_eq!(split(":80"), Some(("0.0.0.0", 80))); - } - - #[test] - fn split3() { - assert_eq!(split("no colon"), None); - } - - #[test] - fn split4() { - assert_eq!(split("deno.land:443"), Some(("deno.land", 443))); - } - - #[test] - fn split5() { - assert_eq!(split("[2001:db8::1]:8080"), Some(("2001:db8::1", 8080))); - } - #[test] fn resolve_addr1() { let expected = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 80)); - let actual = resolve_addr("127.0.0.1:80").wait().unwrap(); + let actual = resolve_addr("127.0.0.1", 80).wait().unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn resolve_addr2() { + let expected = + SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(0, 0, 0, 0), 80)); + let actual = resolve_addr("", 80).wait().unwrap(); assert_eq!(actual, expected); } @@ -114,7 +81,7 @@ mod tests { fn resolve_addr3() { let expected = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 1), 25)); - let actual = resolve_addr("192.0.2.1:25").wait().unwrap(); + let actual = resolve_addr("192.0.2.1", 25).wait().unwrap(); assert_eq!(actual, expected); } @@ -126,7 +93,7 @@ mod tests { 0, 0, )); - let actual = resolve_addr("[2001:db8::1]:8080").wait().unwrap(); + let actual = resolve_addr("[2001:db8::1]", 8080).wait().unwrap(); assert_eq!(actual, expected); } } diff --git a/cli/state.rs b/cli/state.rs index 950bdba703..ca64e6d6c3 100644 --- a/cli/state.rs +++ b/cli/state.rs @@ -336,8 +336,8 @@ impl ThreadSafeState { } #[inline] - pub fn check_net(&self, host_and_port: &str) -> Result<(), ErrBox> { - self.permissions.check_net(host_and_port) + pub fn check_net(&self, hostname: &str, port: u16) -> Result<(), ErrBox> { + self.permissions.check_net(hostname, port) } #[inline] diff --git a/core/modules.rs b/core/modules.rs index 6f71537a68..85de79cca5 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -1021,7 +1021,7 @@ mod tests { let result = recursive_load.poll(); assert!(result.is_ok()); assert!(result.ok().unwrap().is_not_ready()); - let l = loads.lock().unwrap();; + let l = loads.lock().unwrap(); assert_eq!( l.to_vec(), vec![