Improve redaction (#660)

* Always redact `Authorization()`. Fixes #649.
* Support dynamic dots in `req_headers_redacted()`  (#647)
* Refactor code
* Refactor tests and add a bunch more.

Author: hadley

NEWS.md

@@ -1,5 +1,7 @@
 # httr2 (development version)
 
+* `req_headers()` always redacts `Authorization` (#649).
+* `req_headers_redacted()` supports dynamic dots (#647)
 * `resp_stream_sse()` now automatically retrieves the next event if the current event contains no data. The data is now returned as a single string (#650).
 * `aws_v4_signature()` now works if url contains query parameters (@jeffreyzuber, #645).
 

R/req-auth.R

@@ -30,7 +30,7 @@ req_auth_basic <- function(req, username, password = NULL) {
   password <- check_password(password)
 
   username_password <- openssl::base64_encode(paste0(username, ":", password))
-  req_headers(req, Authorization = paste0("Basic ", username_password), .redact = "Authorization")
+  req_headers(req, Authorization = paste0("Basic ", username_password))
 }
 
 #' Authenticate request with bearer token
@@ -57,5 +57,5 @@ req_auth_basic <- function(req, username, password = NULL) {
 req_auth_bearer_token <- function(req, token) {
   check_request(req)
   check_string(token)
-  req_headers(req, Authorization = paste("Bearer", token), .redact = "Authorization")
+  req_headers(req, Authorization = paste("Bearer", token))
 }

R/req-headers.R

@@ -14,8 +14,8 @@
 #'   * Use `NULL` to reset a value to httr2's default
 #'   * Use `""` to remove a header
 #'   * Use a character vector to repeat a header.
-#' @param .redact Headers to redact. If `NULL`, the default, the added headers
-#'   are not redacted.
+#' @param .redact A character vector of headers to redact. The Authorization
+#'   header is always redacted.
 #' @returns A modified HTTP [request].
 #' @export
 #' @examples
@@ -63,16 +63,15 @@
 #' req_secret |> req_dry_run()
 req_headers <- function(.req, ..., .redact = NULL) {
   check_request(.req)
-
-  headers <- list2(...)
-  header_names <- names2(headers)
   check_character(.redact, allow_null = TRUE)
 
-  redact_out <- attr(.req$headers, "redact") %||% .redact %||% character()
-  redact_out <- union(redact_out, .redact)
-  .req$headers <- modify_list(.req$headers, !!!headers)
+  redact_prev <- attr(.req$headers, "redact")
+  header_names <- names(list2(...))
+  .req$headers <- modify_list(.req$headers, ...)
 
-  attr(.req$headers, "redact") <- redact_out
+  .redact <- union(.redact, "Authorization")
+  .redact <- .redact[tolower(.redact) %in% tolower(header_names)]
+  attr(.req$headers, "redact") <- sort(union(.redact, redact_prev))
 
   .req
 }
@@ -82,6 +81,6 @@ req_headers <- function(.req, ..., .redact = NULL) {
 req_headers_redacted <- function(.req, ...) {
   check_request(.req)
 
-  dots <- list(...)
-  req_headers(.req, !!!dots, .redact = names(dots))
+  headers <- list2(...)
+  req_headers(.req, !!!headers, .redact = names(headers))
 }

man/req_headers.Rd

@@ -1,4 +1,15 @@
-# can control which headers to redact
+# is case insensitive
+
+    Code
+      req
+    Message
+      <httr2_request>
+      GET http://example.com
+      Headers:
+      * a: "<REDACTED>"
+      Body: empty
+
+# checks input types
 
     Code
       req_headers(req, a = 1L, b = 2L, .redact = 1L)

tests/testthat/_snaps/req-headers.md

@@ -1,3 +1,7 @@
 testthat::set_state_inspector(function() {
   list(connections = getAllConnections())
 })
+
+expect_redacted <- function(req, expected) {
+  expect_equal(attr(req$headers, "redact"), expected)
+}

tests/testthat/helper.R

@@ -3,12 +3,14 @@ test_that("can send username/password", {
   password <- "p"
   req1 <- request_test("/basic-auth/:user/:password")
   req2 <- req1 %>% req_auth_basic(user, password)
+  expect_redacted(req2, "Authorization")
 
   expect_error(req_perform(req1), class = "httr2_http_401")
   expect_error(req_perform(req2), NA)
 })
 
 test_that("can send bearer token", {
   req <- req_auth_bearer_token(request_test(), "abc")
+  expect_redacted(req, "Authorization")
   expect_equal(req$headers, structure(list(Authorization = "Bearer abc"), redact = "Authorization"))
 })

tests/testthat/test-req-auth.R

@@ -20,18 +20,46 @@ test_that("can add repeated headers", {
   expect_equal(resp$headers$a, c("a,b"))
 })
 
+# redaction ---------------------------------------------------------------
+
 test_that("can control which headers to redact", {
-  expect_redact <- function(req, expected) {
-    expect_equal(attr(req$headers, "redact"), expected)
-  }
+  req <- request("http://example.com")
+  expect_redacted(req_headers(req, a = 1L, b = 2L), character())
+  expect_redacted(req_headers(req, a = 1L, b = 2L, .redact = "a"), "a")
+  expect_redacted(req_headers(req, a = 1L, b = 2L, .redact = c("a", "b")), c("a", "b"))
+})
+
+test_that("only redacts supplied headers", {
+  req <- request("http://example.com")
+  expect_redacted(req_headers(req, a = 1L, b = 2L, .redact = "d"), character())
+})
+
+test_that("redaction preserved across calls", {
+  req <- request("http://example.com")
+  req <- req_headers(req, a = 1L, .redact = "a")
+  req <- req_headers(req, a = 2)
+  expect_redacted(req, "a")
+})
+
+test_that("req_headers_redacted redacts all headers", {
+  req <- request("http://example.com")
+  expect_redacted(req_headers_redacted(req, a = 1L, b = 2L), c("a", "b"))
+})
 
+test_that("is case insensitive", {
   req <- request("http://example.com")
-  expect_redact(req_headers(req, a = 1L, b = 2L), character())
-  expect_redact(req_headers(req, a = 1L, b = 2L, .redact = c("a", "b")), c("a", "b"))
-  expect_redact(req_headers(req, a = 1L, b = 2L, .redact = "a"), "a")
+  req <- req_headers(req, a = 1L, .redact = "A")
+  expect_redacted(req, "A")
+  expect_snapshot(req)
+})
 
-  expect_redact(req_headers_redacted(req, a = 1L, b = 2L), c("a", "b"))
+test_that("authorization is always redacted", {
+  req <- request("http://example.com")
+  expect_redacted(req_headers(req, Authorization = "X"), "Authorization")
+})
 
+test_that("checks input types", {
+  req <- request("http://example.com")
   expect_snapshot(error = TRUE, {
     req_headers(req, a = 1L, b = 2L, .redact = 1L)
   })