From d37d48c938c6303d80ea6a570dac7d61ba09382f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 6 Feb 2025 18:45:36 -0600 Subject: [PATCH] Improve redaction * Always redact `Authorization()`. Fixes #649. * Support dynamic dots in `req_headers_redacted()` (#647) * Refactor code * Refactor tests and add a bunch more. --- NEWS.md | 2 ++ R/req-auth.R | 4 +-- R/req-headers.R | 21 +++++++------- man/req_headers.Rd | 4 +-- tests/testthat/_snaps/req-headers.md | 13 ++++++++- tests/testthat/helper.R | 4 +++ tests/testthat/test-req-auth.R | 2 ++ tests/testthat/test-req-headers.R | 42 +++++++++++++++++++++++----- 8 files changed, 69 insertions(+), 23 deletions(-) diff --git a/NEWS.md b/NEWS.md index 4d51ee38c..369b16200 100644 --- a/NEWS.md +++ b/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). diff --git a/R/req-auth.R b/R/req-auth.R index 64593495f..906551b8e 100644 --- a/R/req-auth.R +++ b/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)) } diff --git a/R/req-headers.R b/R/req-headers.R index 3d099cc83..a430837f9 100644 --- a/R/req-headers.R +++ b/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)) } diff --git a/man/req_headers.Rd b/man/req_headers.Rd index c78d5cdbb..0781afc70 100644 --- a/man/req_headers.Rd +++ b/man/req_headers.Rd @@ -20,8 +20,8 @@ and their values. \item Use a character vector to repeat a header. }} -\item{.redact}{Headers to redact. If \code{NULL}, the default, the added headers -are not redacted.} +\item{.redact}{A character vector of headers to redact. The Authorization +header is always redacted.} } \value{ A modified HTTP \link{request}. diff --git a/tests/testthat/_snaps/req-headers.md b/tests/testthat/_snaps/req-headers.md index d5315a008..e35012fd4 100644 --- a/tests/testthat/_snaps/req-headers.md +++ b/tests/testthat/_snaps/req-headers.md @@ -1,4 +1,15 @@ -# can control which headers to redact +# is case insensitive + + Code + req + Message + + GET http://example.com + Headers: + * a: "" + Body: empty + +# checks input types Code req_headers(req, a = 1L, b = 2L, .redact = 1L) diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R index 128daafd4..8611329a2 100644 --- a/tests/testthat/helper.R +++ b/tests/testthat/helper.R @@ -1,3 +1,7 @@ testthat::set_state_inspector(function() { list(connections = getAllConnections()) }) + +expect_redacted <- function(req, expected) { + expect_equal(attr(req$headers, "redact"), expected) +} diff --git a/tests/testthat/test-req-auth.R b/tests/testthat/test-req-auth.R index 9e44389a1..5d378c103 100644 --- a/tests/testthat/test-req-auth.R +++ b/tests/testthat/test-req-auth.R @@ -3,6 +3,7 @@ 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) @@ -10,5 +11,6 @@ test_that("can send username/password", { 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")) }) diff --git a/tests/testthat/test-req-headers.R b/tests/testthat/test-req-headers.R index 1b7f38830..74918d03a 100644 --- a/tests/testthat/test-req-headers.R +++ b/tests/testthat/test-req-headers.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) })