Fix encoding/decoding of query parameters and cookies (#462)

* Fix #460 - now correctly url-decoding parameters

Also added relevant tests

* Fix #461 - now correctly url-encoding cookies

Also added relevant tests

* Fix broken sessions test not using parseCookies to read cookies

* Updated NEWS

* Removed @importFrom httpuv statements

NAMESPACE

@@ -31,7 +31,5 @@ import(stringi)
 importFrom(grDevices,dev.off)
 importFrom(grDevices,jpeg)
 importFrom(grDevices,png)
-importFrom(httpuv,decodeURI)
-importFrom(httpuv,encodeURI)
 importFrom(httpuv,runServer)
 importFrom(stats,runif)

NEWS.md

@@ -54,6 +54,8 @@ plumber 0.5.0
 
 ### Bug fixes
 
+* Fix URL-decoding of query parameters and URL-encoding/decoding of cookies. Both now use `httpuv::decodeURIComponent` instead of `httpuv::decodeURI`. (@antoine-sachet, [#462](https://github.com/trestletech/plumber/pull/462))
+
 * Fix bugs that prevented `do_provision` from deploying to DigitalOcean and updated to the latest `analogsea`.  ([#448](https://github.com/trestletech/plumber/pull/448/files))
 
 * Fixed bug where functions defined earlier in the file could not be found when `plumb()`ing a file.  ([#416](https://github.com/trestletech/plumber/pull/416))

R/cookie-parser.R

@@ -4,7 +4,6 @@ cookieFilter <- function(req){
   forward()
 }
 
-#' @importFrom httpuv decodeURI
 #' @noRd
 parseCookies <- function(cookie) {
   if (is.null(cookie) || nchar(cookie) == 0) {
@@ -23,7 +22,7 @@ parseCookies <- function(cookie) {
   }
 
   cookies <- vapply(cookieList, "[[", character(1), 2)
-  decodedCookies <- as.list(decodeURI(cookies))
+  decodedCookies <- as.list(httpuv::decodeURIComponent(cookies))
   cookieNames <- vapply(cookieList, "[[", character(1), 1)
   names(decodedCookies) <- cookieNames
   decodedCookies

R/query-string.R

@@ -27,7 +27,7 @@ parseQS <- function(qs){
     return(list())
   }
 
-  keys <- decodeURI(vapply(kv, "[[", character(1), 1)) # returns utf8 strings
+  keys <- httpuv::decodeURIComponent(vapply(kv, "[[", character(1), 1)) # returns utf8 strings
   if (any(Encoding(keys) != "unknown")) {
     # https://github.com/trestletech/plumber/pull/314#discussion_r239992879
     non_ascii <- setdiff(unique(Encoding(keys)), "unknown")
@@ -38,7 +38,7 @@ parseQS <- function(qs){
   }
 
   vals <- vapply(kv, "[[", character(1), 2)
-  vals <- decodeURI(vals) # returns utf8 strings
+  vals <- httpuv::decodeURIComponent(vals) # returns utf8 strings
 
   ret <- as.list(vals)
   names(ret) <- keys

R/response.R

@@ -58,10 +58,9 @@ removeCookieStr <- function(name, path, http = FALSE, secure = FALSE) {
   str
 }
 
-#' @importFrom httpuv encodeURI
 #' @noRd
 cookieToStr <- function(name, value, path, expiration=FALSE, http=FALSE, secure=FALSE){
-  val <- encodeURI(as.character(value))
+  val <- httpuv::encodeURIComponent(as.character(value))
   str <- paste0(name, "=", val, "; ")
 
   if (!missing(path)){

tests/testthat/test-cookies.R

@@ -7,9 +7,9 @@ skip_if_no_cookie_support <- function() {
 
 test_that("cookies are parsed", {
 
-  cookies <- parseCookies("spaced=cookie%20here; another=2")
+  cookies <- parseCookies("spaced=cookie%2C%20here; another=2")
   expect_equal(names(cookies), c("spaced", "another"))
-  expect_equal(cookies$spaced, "cookie here")
+  expect_equal(cookies$spaced, "cookie, here")
   expect_equal(cookies$another, "2")
 
   cookies <- parseCookies("a=zxcv=asdf; missingVal=; b=qwer=ttyui")
@@ -48,6 +48,7 @@ test_that("cookies can convert to string", {
 
   expect_equal(cookieToStr("abc", 123), "abc=123")
   expect_equal(cookieToStr("complex", "string with spaces"), "complex=string%20with%20spaces")
+  expect_equal(cookieToStr("complex2", "forbidden:,%/"), "complex2=forbidden%3A%2C%25%2F")
   expect_equal(cookieToStr("abc", 123, path="/somepath"), "abc=123; Path=/somepath")
   expect_equal(cookieToStr("abc", 123, http=TRUE, secure=TRUE), "abc=123; HttpOnly; Secure")
 

tests/testthat/test-querystring.R

@@ -9,6 +9,7 @@ test_that("query strings are properly parsed", {
 test_that("special characters in query strings are handled properly", {
   expect_equal(parseQS("?a=1+.#"), list(a="1+.#"))
   expect_equal(parseQS("?a=a%20b"), list(a="a b"))
+  expect_equal(parseQS('?a=%2C%2B%2F%3F%25%26'), list(a=",+/?%&"))
 })
 
 test_that("null an empty strings return empty list", {

tests/testthat/test-sessions.R

@@ -45,8 +45,7 @@ test_that("cookies are set", {
 
   cook <- res$headers[["Set-Cookie"]]
   expect_match(cook, "^plcook")
-  cook <- sub("^plcook=", "", cook, perl = TRUE)
-  cook <- sub(";.*", "", cook)
+  cook <- parseCookies(cook)$plcook
   expect_equal(decodeCookie(cook, asCookieKey(key)), list(abc = 1234))
 })