refactor DenoPermissions.check_net & resolve_addr (#3182)

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(&args.hostname, args.port)?;
-  // state.check_net and resolve_addr() took hostname and port directly.
-  let address = format!("{}:{}", args.hostname, args.port);
 
-  state.check_net(&address)?;
+  let op = resolve_addr(&args.hostname, args.port).and_then(move |addr| {
-
-  let op = resolve_addr(&address).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(&args.hostname, args.port)?;
-  // state.check_net and resolve_addr() took hostname and port directly.
-  let address = format!("{}:{}", args.hostname, args.port);
 
-  state.check_net(&address)?;
+  let addr = resolve_addr(&args.hostname, args.port).wait()?;
-
-  let addr = resolve_addr(&address).wait()?;
   let listener = TcpListener::bind(&addr)?;
   let local_addr = listener.local_addr()?;
   let resource = resources::add_tcp_listener(listener);

cli/ops/tls.rs

@@ -55,23 +55,19 @@ pub fn op_dial_tls(
   _zero_copy: Option<PinnedBuf>,
 ) -> Result<JsonOp, ErrBox> {
   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);

cli/permissions.rs

@@ -208,28 +208,19 @@ impl DenoPermissions {
     }
   }
 
-  pub fn check_net(&self, host_and_port: &str) -> Result<(), ErrBox> {
+  pub fn check_net(&self, hostname: &str, port: u16) -> Result<(), ErrBox> {
-    let msg = &format!("network access to \"{}\"", host_and_port);
+    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::<Vec<&str>>();
+        if self.net_whitelist.contains(hostname)
-        if match parts.len() {
+          || self
-          2 => {
-            if self.net_whitelist.contains(parts[0]) {
-              true
-            } else {
-              self
             .net_whitelist
-                .contains(&format!("{}:{}", parts[0], parts[1]))
+            .contains(&format!("{}:{}", hostname, port))
-            }
+        {
-          }
-          1 => self.net_whitelist.contains(parts[0]),
-          _ => panic!("Failed to parse origin string: {}", host_and_port),
-        } {
           self.log_perm_access(msg);
           Ok(())
         } else {
@@ -438,26 +429,26 @@ mod tests {
     });
 
     let domain_tests = vec![
-      ("localhost:1234", true),
+      ("localhost", 1234, true),
-      ("deno.land", true),
+      ("deno.land", 0, true),
-      ("deno.land:3000", true),
+      ("deno.land", 3000, true),
-      ("deno.lands", false),
+      ("deno.lands", 0, false),
-      ("deno.lands:3000", false),
+      ("deno.lands", 3000, false),
-      ("github.com:3000", true),
+      ("github.com", 3000, true),
-      ("github.com", false),
+      ("github.com", 0, false),
-      ("github.com:2000", false),
+      ("github.com", 2000, false),
-      ("github.net:3000", false),
+      ("github.net", 3000, false),
-      ("127.0.0.1", true),
+      ("127.0.0.1", 0, true),
-      ("127.0.0.1:3000", true),
+      ("127.0.0.1", 3000, true),
-      ("127.0.0.2", false),
+      ("127.0.0.2", 0, false),
-      ("127.0.0.2:3000", false),
+      ("127.0.0.2", 3000, false),
-      ("172.16.0.2:8000", true),
+      ("172.16.0.2", 8000, true),
-      ("172.16.0.2", false),
+      ("172.16.0.2", 0, false),
-      ("172.16.0.2:6000", false),
+      ("172.16.0.2", 6000, false),
-      ("172.16.0.1:8000", false),
+      ("172.16.0.1", 8000, false),
       // Just some random hosts that should err
-      ("somedomain", false),
+      ("somedomain", 0, false),
-      ("192.168.0.1", 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() {
+    for (host, port, is_ok) in domain_tests.iter() {
-      assert_eq!(*is_ok, perms.check_net(domain).is_ok());
+      assert_eq!(*is_ok, perms.check_net(host, *port).is_ok());
     }
   }
 }

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.
+/// Resolve network address. Returns a future.
-/// Examples:
+pub fn resolve_addr(hostname: &str, port: u16) -> ResolveAddrFuture {
-/// "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 {
   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,14 +43,15 @@ 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(':');
+    r.and_then(|mut iter| match iter.next() {
-    match p.parse::<u16>() {
+      Some(a) => Ok(Async::Ready(a)),
-      Err(_) => None,
+      None => panic!("There should be at least one result"),
-      Ok(port) => Some((addr, port)),
-    }
     })
   }
+}
 
 #[cfg(test)]
 mod tests {
@@ -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);
   }
 }

cli/state.rs

@@ -336,8 +336,8 @@ impl ThreadSafeState {
   }
 
   #[inline]
-  pub fn check_net(&self, host_and_port: &str) -> Result<(), ErrBox> {
+  pub fn check_net(&self, hostname: &str, port: u16) -> Result<(), ErrBox> {
-    self.permissions.check_net(host_and_port)
+    self.permissions.check_net(hostname, port)
   }
 
   #[inline]

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![