Allows port to be specified as an environment variable (#963)

Co-authored-by: Garrick Aden-Buie <[email protected]>
Co-authored-by: Barret Schloerke <[email protected]>

NAMESPACE

@@ -116,7 +116,6 @@ importFrom(lifecycle,deprecated)
 importFrom(magrittr,"%>%")
 importFrom(rlang,"%||%")
 importFrom(rlang,missing_arg)
-importFrom(stats,runif)
 importFrom(stats,setNames)
 importFrom(utils,file.edit)
 importFrom(utils,installed.packages)

NEWS.md

@@ -1,7 +1,8 @@
 # plumber (development version)
 
-* Added support for graphic devices provided by ragg and svglite (@thomasp85
-#964)
+* Fixes #956, allowing a port to be specified as an environment variable. User-provided ports must be between 1024 and 49151 (following [IANA guidelines](https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml)) and may not be a known unsafe port. plumber will now throw an error if an invalid port is requested. (@shikokuchuo @gadenbuie #963)
+
+* Added support for graphic devices provided by ragg and svglite (@thomasp85 #964)
 
 # plumber 1.2.2
 

R/find-port.R

@@ -1,47 +1,77 @@
+# Ports that are considered unsafe by Chrome
+# http://superuser.com/questions/188058/which-ports-are-considered-unsafe-on-chrome
+# https://github.com/rstudio/shiny/issues/1784
+unsafePorts <- function() {
+  asNamespace("httpuv")[["unsafe_ports"]]
+}
 
-# Exclude unsafe ports from Chrome https://src.chromium.org/viewvc/chrome/trunk/src/net/base/net_util.cc?view=markup#l127
-portBlacklist <- c(0, 3659, 4045, 6000, 6665, 6666, 6667, 6668, 6669)
 
-#' Get a random port between 3k and 10k, excluding the blacklist. If a preferred port
-#' has already been registered in .globals, use that instead.
-#' @importFrom stats runif
-#' @noRd
-getRandomPort <- function(){
-  port <- 0
-  while (port %in% portBlacklist){
-    port <- round(runif(1, 3000, 10000))
-  }
-  port
+randomPort <- function(..., n = 10) {
+  tryCatch({
+    port <- httpuv::randomPort(..., n = n)
+  }, httpuv_unavailable_port = function(e) {
+    stop(
+      "Unable to start a Plumber server. ",
+      paste0("We were unable to find a free port in ", n, " tries.")
+    )
+  })
+  return(port)
 }
 
-#' Find a port either using the assigned port or randomly search 10 times for an available
-#' port. If a port was manually assigned, just return it and assume it will work.
-#' @noRd
-findPort <- function(port){
-  if (missing(port) || is.null(port)){
-    if (!is.null(.globals$port)){
-      # Start by trying the .globals$port
-      port <- .globals$port
-    } else {
-      port <- getRandomPort()
+portIsAvailable <- function(port) {
+  tryCatch(
+    {
+      randomPort(min = port, max = port)
+      TRUE
+    },
+    error = function(e) {
+      FALSE
     }
+  )
+}
 
-    for (i in 1:10){
-      tryCatch(srv <- httpuv::startServer("127.0.0.1", port, list(), quiet = TRUE), error=function(e){
-        port <<- 0
-      })
-      if (port != 0){
-        # Stop the temporary server, and retain this port number.
-        httpuv::stopServer(srv)
-        .globals$port <- port
-        break
-      }
-      port <- getRandomPort()
+#' Find a port either using the assigned port or randomly search 10 times for an
+#' available port. If a port was manually assigned, just return it and assume it
+#' will work.
+#' @noRd
+findPort <- function(port = NULL) {
+  if (is.null(port)) {
+    # Try to use the most recently used _random_ port
+    if (
+      (!is.null(.globals$last_random_port)) &&
+        portIsAvailable(.globals$last_random_port)
+    ) {
+      return(.globals$last_random_port)
     }
+
+    # Find an available port
+    port <- randomPort()
+
+    # Save the random port for future use
+    .globals$last_random_port <- port
+    return(.globals$last_random_port)
+  }
+
+  port_og <- port
+
+  if (rlang::is_character(port)) {
+    port <- suppressWarnings(as.integer(port))
+  }
+
+  if (!rlang::is_integerish(port, n = 1, finite = TRUE) || port != port_og) {
+    stop("Port must be an integer value, not '", port_og, "'.")
+  }
+
+  port <- as.integer(port)
+
+  # Ports must be in [1024-49151]
+  # https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml
+  if (port < 1024 || port > 49151) {
+    stop("Port must be an integer in the range 1024 to 49151 (inclusive).")
   }
 
-  if (port == 0){
-    stop("Unable to start a Plumber server. Either the port specified was unavailable or we were unable to find a free port.")
+  if (port == 0 || port %in% unsafePorts()) {
+    stop("Port ", port, " is an unsafe port. Please choose another port.")
   }
 
   port

tests/testthat/test-find-port.R

@@ -1,24 +1,39 @@
 context("find port")
 
 test_that("ports can be randomly found", {
-  foundPorts <- NULL
-
-  for (i in 1:50){
-    p <- getRandomPort()
-    expect_gte(p, 3000)
-    expect_lte(p, 10000)
-
-    foundPorts <- c(foundPorts, p)
-  }
+  foundPorts <- unlist(lapply(1:50, function(i) {
+    p <- randomPort()
+    # Use findPort to verify the port's value
+    findPort(p)
+  }))
 
   # It's possible we got a collision or two, but shouldn't have many.
   expect_gt(length(unique(foundPorts)), 45)
 })
 
 test_that("global port used if available", {
-  .globals$port <- 1234
-  expect_equal(findPort(), 1234)
-  rm("port", envir = .globals)
+  .globals$last_random_port <- 12345
+  expect_equal(findPort(), 12345)
+  rm("last_random_port", envir = .globals)
+})
+
+test_that("integer type is returned", {
+  expect_type(findPort(), "integer")
+  expect_equal(findPort("8000"), 8000L)
+  expect_equal(findPort(8000.00000), 8000L)
+})
+
+test_that("throws if provided non-integerish port", {
+  expect_error(findPort("blue"))
+  expect_error(findPort(8000.0001))
+  expect_error(findPort(8000:8002))
+})
+
+test_that("throws for invalid ports", {
+  expect_error(findPort(800)) # in 0-1024
+  expect_error(findPort(123456)) # out of range
+  expect_error(findPort(0)) # unsafe
+  expect_error(findPort(6666)) # unsafe
 })
 
 test_that("finds a good port and persists it", {
@@ -27,27 +42,28 @@ test_that("finds a good port and persists it", {
   p <- findPort()
 
   # Persisted
-  expect_equal(.globals$port, p)
+  expect_equal(.globals$last_random_port, p)
 
   # Check that we can actually start a server
   srv <- httpuv::startServer("127.0.0.1", p, list())
 
   # Cleanup
-  rm("port", envir = .globals)
+  rm("last_random_port", envir = .globals)
   httpuv::stopServer(srv)
 })
 
 test_that("we don't pin to globals$port if it's occupied", {
   testthat::skip_on_cran()
 
-  srv <- httpuv::startServer("127.0.0.1", 1234, list())
-  .globals$port <- 1234
+  ex_p <- 12345
+  srv <- httpuv::startServer("127.0.0.1", ex_p, list())
+  .globals$last_random_port <- ex_p
 
   p <- findPort()
 
   # It should shuffle to find another port.
-  expect_true(p != 1234)
+  expect_true(p != ex_p)
 
-  rm("port", envir = .globals)
+  rm("last_random_port", envir = .globals)
   httpuv::stopServer(srv)
 })

tests/testthat/test-shared-secret.R

@@ -3,7 +3,7 @@ context("shared secret")
 test_that("requests with shared secrets pass, w/o fail", {
   options(`plumber.sharedSecret`="abcdefg")
 
-  pr <- pr()
+  pr <- pr() %>% pr_set_debug(FALSE)
   pr$handle("GET", "/", function(){ 123 })
   req <- make_req("GET", "/", pr = pr)