From c5607b86dee3ac56bf8d36128649638bed79d17f Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Fri, 20 Sep 2024 11:48:44 -0700 Subject: [PATCH 1/6] Make the HTTP transport behavior consistent between WinHTTP and libCurl by disabling automatically following redirects on Windows. --- sdk/core/azure-core/CHANGELOG.md | 2 ++ .../azure-core/src/http/winhttp/win_http_transport.cpp | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 446901c050..0c0beee9e8 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Make the HTTP transport behavior consistent between WinHTTP and libCurl by disabling automatically following redirects on Windows. + ### Other Changes ## 1.14.0-beta.2 (2024-09-12) diff --git a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp index 7172786d0c..89e350f126 100644 --- a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp +++ b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp @@ -1043,6 +1043,16 @@ _detail::WinHttpRequest::WinHttpRequest( } } + DWORD disableRedirects = WINHTTP_DISABLE_REDIRECTS; + if (!WinHttpSetOption( + m_requestHandle.get(), + WINHTTP_OPTION_DISABLE_FEATURE, + &disableRedirects, + sizeof(disableRedirects))) + { + GetErrorAndThrow("Error while disabling redirects."); + } + // Set the callback function to be called whenever the state of the request handle changes. m_httpAction = std::make_unique<_detail::WinHttpAction>(this); From 941b88e071c0c7afee75ac41807ab09f05b8a9b5 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Fri, 20 Sep 2024 11:51:14 -0700 Subject: [PATCH 2/6] Re-enable APC test. --- .../azure-identity/test/ut/azure_pipelines_credential_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp b/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp index 07167abc7c..35c69e6a04 100644 --- a/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp @@ -644,7 +644,7 @@ TEST(AzurePipelinesCredential, InvalidServiceConnectionId_LIVEONLY_) } } -TEST(AzurePipelinesCredential, DISABLED_InvalidSystemAccessToken_LIVEONLY_) +TEST(AzurePipelinesCredential, InvalidSystemAccessToken_LIVEONLY_) { std::string tenantId = Environment::GetVariable("AZURESUBSCRIPTION_TENANT_ID"); std::string clientId = Environment::GetVariable("AZURESUBSCRIPTION_CLIENT_ID"); From 3bfead6e88e45e89a39126527b889ca4725911de Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Fri, 20 Sep 2024 23:39:29 -0700 Subject: [PATCH 3/6] Fix casing in changelog. --- sdk/core/azure-core/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 0c0beee9e8..b3a84ac34c 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -8,7 +8,7 @@ ### Bugs Fixed -- Make the HTTP transport behavior consistent between WinHTTP and libCurl by disabling automatically following redirects on Windows. +- Make the HTTP transport behavior consistent between WinHTTP and libcurl by disabling automatically following redirects on Windows. ### Other Changes From e10e85eb1a54221daf1040af2f7b1c30e6300656 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Mon, 23 Sep 2024 00:05:55 -0700 Subject: [PATCH 4/6] Add http bin based unit test and fix typo in doc comment for curl. --- .../src/http/curl/curl_connection_private.hpp | 5 ++--- .../test/ut/transport_adapter_base_test.cpp | 15 +++++++++++++++ .../test/ut/transport_adapter_base_test.hpp | 8 ++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 7c360d3fbc..1ddd0eae2e 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -33,10 +33,9 @@ typedef struct x509_store_ctx_st X509_STORE_CTX; namespace Azure { namespace Core { namespace _detail { /** - * @brief Unique handle for WinHTTP HINTERNET handles. + * @brief Unique handle for CURL handles. * - * @note HINTERNET is declared as a "void *". This means that this definition subsumes all other - * `void *` types when used with Azure::Core::_internal::UniqueHandle. + * @note CURL is declared as a "void". * */ template <> struct UniqueHandleHelper diff --git a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp index cf667e04b4..bebecdf9eb 100644 --- a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp +++ b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp @@ -389,6 +389,21 @@ namespace Azure { namespace Core { namespace Test { t1.join(); } + TEST_P(TransportAdapter, dontAutoFollowRedirects) + { + // We dont expect the transport adapter to follow redirects automatically to this url. + std::string redirectToUrl = AzureSdkHttpbinServer::ResponseHeaders("foo=bar"); + + Azure::Core::Http::Request request( + Azure::Core::Http::HttpMethod::Get, + Azure::Core::Url(AzureSdkHttpbinServer::RedirectTo(redirectToUrl))); + + auto response = m_pipeline->Send(request, Context{}); + EXPECT_EQ(response->GetStatusCode(), Azure::Core::Http::HttpStatusCode::Found); + EXPECT_TRUE(response->GetHeaders().find("location") != response->GetHeaders().end()); + EXPECT_EQ(response->GetHeaders().at("location"), redirectToUrl); + } + TEST_P(TransportAdapter, cancelRequest) { Azure::Core::Url hostPath(AzureSdkHttpbinServer::Delay() + "/2"); // 2 seconds delay on server diff --git a/sdk/core/azure-core/test/ut/transport_adapter_base_test.hpp b/sdk/core/azure-core/test/ut/transport_adapter_base_test.hpp index 5429def0e6..a7d8cab89f 100644 --- a/sdk/core/azure-core/test/ut/transport_adapter_base_test.hpp +++ b/sdk/core/azure-core/test/ut/transport_adapter_base_test.hpp @@ -38,6 +38,14 @@ namespace Azure { namespace Core { namespace Test { { return Schema() + "://" + Host() + "/status/" + std::to_string(statusCode); } + inline static std::string ResponseHeaders(std::string responseHeaders) + { + return Schema() + "://" + Host() + "/response-headers?" + responseHeaders; + } + inline static std::string RedirectTo(std::string redirectToUrl) + { + return Schema() + "://" + Host() + "/redirect-to?url=" + redirectToUrl; + } inline static std::string Host() { return std::string(_detail::AzureSdkHttpbinServer); } inline static std::string Schema() { return std::string(_detail::AzureSdkHttpbinServerSchema); } }; From 7ad87d88b5ae837a76fc4bb360c6f9b071b95e7b Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Mon, 23 Sep 2024 00:16:16 -0700 Subject: [PATCH 5/6] Update test name to avoid 'dont' --- sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp index bebecdf9eb..e8d60f375f 100644 --- a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp +++ b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp @@ -389,7 +389,7 @@ namespace Azure { namespace Core { namespace Test { t1.join(); } - TEST_P(TransportAdapter, dontAutoFollowRedirects) + TEST_P(TransportAdapter, redirectsNotFollowed) { // We dont expect the transport adapter to follow redirects automatically to this url. std::string redirectToUrl = AzureSdkHttpbinServer::ResponseHeaders("foo=bar"); From f3f25a8b2421b61ed428a6ae0fe5385cad0823cd Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Mon, 23 Sep 2024 11:38:19 -0700 Subject: [PATCH 6/6] Fix typo. --- sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp index e8d60f375f..4daf33ac84 100644 --- a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp +++ b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp @@ -391,7 +391,7 @@ namespace Azure { namespace Core { namespace Test { TEST_P(TransportAdapter, redirectsNotFollowed) { - // We dont expect the transport adapter to follow redirects automatically to this url. + // We don't expect the transport adapter to follow redirects automatically to this url. std::string redirectToUrl = AzureSdkHttpbinServer::ResponseHeaders("foo=bar"); Azure::Core::Http::Request request(