From 8faea195969223e3d74b86eb3f2cb8f097de087a Mon Sep 17 00:00:00 2001 From: Jon Harmon Date: Fri, 7 Feb 2025 12:50:39 -0600 Subject: [PATCH 1/4] Less-zealous redirect_uri normalization Closes #646 I also tweaked a snapshot test that was coming up as changed on my Windows machine. --- NEWS.md | 1 + R/oauth-flow-auth-code.R | 4 ++-- tests/testthat/test-curl.R | 15 ++++++++++----- tests/testthat/test-oauth-flow-auth-code.R | 9 +++++++++ 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/NEWS.md b/NEWS.md index cfc3b0cf4..467263ef8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,6 +7,7 @@ * `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). +* `req_oauth_auth_code()` no longer adds trailing "/" characters to well-formed `redirect_uri` values (@jonthegeek, #646). # httr2 1.1.0 diff --git a/R/oauth-flow-auth-code.R b/R/oauth-flow-auth-code.R index 7abda7434..a381b0279 100644 --- a/R/oauth-flow-auth-code.R +++ b/R/oauth-flow-auth-code.R @@ -209,7 +209,7 @@ normalize_redirect_uri <- function(redirect_uri, port = deprecated(), error_call = caller_env()) { - parsed <- url_parse(redirect_uri) + old <- parsed <- url_parse(redirect_uri) if (lifecycle::is_present(host_name)) { lifecycle::deprecate_warn( @@ -250,7 +250,7 @@ normalize_redirect_uri <- function(redirect_uri, } list( - uri = url_build(parsed), + uri = if (identical(old, parsed)) redirect_uri else url_build(parsed), localhost = localhost, can_fetch_code = can_fetch_oauth_code(redirect_uri) ) diff --git a/tests/testthat/test-curl.R b/tests/testthat/test-curl.R index a9cff6ad8..ae4b45c3a 100644 --- a/tests/testthat/test-curl.R +++ b/tests/testthat/test-curl.R @@ -204,11 +204,16 @@ test_that("can read from clipboard", { rlang::local_interactive() clipr::write_clip("curl 'http://example.com' \\\n -H 'A: 1' \\\n -H 'B: 2'") - expect_snapshot({ - curl_translate() - # also writes to clip - clipr::read_clip() - }) + expect_snapshot( + { + curl_translate() + # also writes to clip + clipr::read_clip() + }, + transform = function(x) { + grep("\\\\r", x, value = TRUE, invert = TRUE) + } + ) }) test_that("encode_string2() produces simple strings", { diff --git a/tests/testthat/test-oauth-flow-auth-code.R b/tests/testthat/test-oauth-flow-auth-code.R index 9d0da0b15..944e837a1 100644 --- a/tests/testthat/test-oauth-flow-auth-code.R +++ b/tests/testthat/test-oauth-flow-auth-code.R @@ -88,6 +88,15 @@ test_that("old args are deprecated", { }) +test_that("valid urls aren't changed", { + # Allow tests to run when is_hosted_session() is TRUE. + local_mocked_bindings(is_hosted_session = function() FALSE) + + original_uri <- "http://localhost:8080" + normalized_uri <- normalize_redirect_uri(original_uri) + expect_identical(normalized_uri$uri, original_uri) +}) + # ouath_flow_auth_code_parse ---------------------------------------------- test_that("forwards oauth error", { From bfcdf531afe0c0592a4f079973f43fcfae43575b Mon Sep 17 00:00:00 2001 From: Jon Harmon Date: Fri, 7 Feb 2025 14:57:42 -0600 Subject: [PATCH 2/4] More precise transform. --- tests/testthat/test-curl.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-curl.R b/tests/testthat/test-curl.R index ae4b45c3a..87cb31f32 100644 --- a/tests/testthat/test-curl.R +++ b/tests/testthat/test-curl.R @@ -211,7 +211,7 @@ test_that("can read from clipboard", { clipr::read_clip() }, transform = function(x) { - grep("\\\\r", x, value = TRUE, invert = TRUE) + grep('"\\\\r"\\s*$', x, value = TRUE, invert = TRUE) } ) }) From 0fb92882d66c72e7e7f41e1e80bfe06cefabba46 Mon Sep 17 00:00:00 2001 From: Jon Harmon Date: Sat, 8 Feb 2025 08:05:45 -0600 Subject: [PATCH 3/4] Revert transform. Will move to a new issue/PR. --- tests/testthat/test-curl.R | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/testthat/test-curl.R b/tests/testthat/test-curl.R index 87cb31f32..a9cff6ad8 100644 --- a/tests/testthat/test-curl.R +++ b/tests/testthat/test-curl.R @@ -204,16 +204,11 @@ test_that("can read from clipboard", { rlang::local_interactive() clipr::write_clip("curl 'http://example.com' \\\n -H 'A: 1' \\\n -H 'B: 2'") - expect_snapshot( - { - curl_translate() - # also writes to clip - clipr::read_clip() - }, - transform = function(x) { - grep('"\\\\r"\\s*$', x, value = TRUE, invert = TRUE) - } - ) + expect_snapshot({ + curl_translate() + # also writes to clip + clipr::read_clip() + }) }) test_that("encode_string2() produces simple strings", { From 9d51f497bf79a42da44bc9abee35c05171825c8a Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sat, 8 Feb 2025 08:09:21 -0600 Subject: [PATCH 4/4] Polish tests --- tests/testthat/test-oauth-flow-auth-code.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-oauth-flow-auth-code.R b/tests/testthat/test-oauth-flow-auth-code.R index 944e837a1..5a5afc4cf 100644 --- a/tests/testthat/test-oauth-flow-auth-code.R +++ b/tests/testthat/test-oauth-flow-auth-code.R @@ -88,13 +88,13 @@ test_that("old args are deprecated", { }) -test_that("valid urls aren't changed", { +test_that("urls left as is if not changes needed", { # Allow tests to run when is_hosted_session() is TRUE. local_mocked_bindings(is_hosted_session = function() FALSE) original_uri <- "http://localhost:8080" normalized_uri <- normalize_redirect_uri(original_uri) - expect_identical(normalized_uri$uri, original_uri) + expect_equal(normalized_uri$uri, original_uri) }) # ouath_flow_auth_code_parse ---------------------------------------------- @@ -134,6 +134,7 @@ test_that("external auth code sources are detected correctly", { test_that("auth codes can be retrieved from an external source", { skip_on_cran() + local_mocked_bindings(sys_sleep = function(...) {}) req <- local_app_request(function(req, res) { # Error on first, and then respond on second