Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions src/aws-cpp-sdk-core/source/http/HttpRequest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,6 @@ namespace Aws
// CRT sigv4 don't any encoding if double encoding is off, need to encode the path before passing to CRT.
const URI& uri = m_uri;
Aws::StringStream ss;
Aws::StringStream port;
if (uri.GetScheme() == Scheme::HTTP && uri.GetPort() != HTTP_DEFAULT_PORT)
{
port << ":" << uri.GetPort();
}
else if (uri.GetScheme() == Scheme::HTTPS && uri.GetPort() != HTTPS_DEFAULT_PORT)
{
port << ":" << uri.GetPort();
}

// Note: GetURLEncodedPathRFC3986 does legacy mode by default, which
// leaves some reserved delimeter characters unencoded (things like
Expand All @@ -88,8 +79,11 @@ namespace Aws
// practice however CRT will never support legacy mode and will
// always need modern encoding, so be explicit here about which mode
// we use.
ss << SchemeMapper::ToString(uri.GetScheme()) << SEPARATOR << uri.GetAuthority() << port.str()
<< ((uri.GetPath() == "/") ? "" : uri.GetURLEncodedPath())

// Set path to origin-form (path + query only) per RFC 9112 §3.2.1.
// aws-c-http converts to absolute-form only when sending through a
// forward proxy via aws_http_rewrite_uri_for_proxy_request.
ss << (uri.GetPath().empty() ? "/" : uri.GetURLEncodedPath())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetPath() adds a / before every segment, and adds one more when there are no segments. Every possible result therefore starts with / and the result will never be empty. This can be simplified to ss << uri.GetURLEncodedPath() << uri.GetQueryString();

<< uri.GetQueryString();

request->SetPath(Aws::Crt::ByteCursorFromCString(ss.str().c_str()));
Expand Down
81 changes: 81 additions & 0 deletions tests/aws-cpp-sdk-core-tests/http/HttpClientTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <aws/core/http/standard/StandardHttpRequest.h>
#include <aws/core/client/ClientConfiguration.h>
#include <aws/core/utils/logging/LogMacros.h>
#include <aws/crt/http/HttpRequestResponse.h>
#include <future>
#include <chrono>

Expand Down Expand Up @@ -385,4 +386,84 @@ TEST_F(CURLHttpClientTest, TestHttpRequestWorksFine)
#endif // ENABLE_CURL_CLIENT
#endif // ENABLE_HTTP_CLIENT_TESTING
#endif // NO_HTTP_CLIENT

#endif // DISABLE_DNS_REQUIRED_TESTS

// Test ToCrtHttpRequest conversion (no network required)
#include <aws/testing/AwsCppSdkGTestSuite.h>
#include <aws/core/http/standard/StandardHttpRequest.h>
#include <aws/crt/http/HttpRequestResponse.h>

class HttpRequestConversionTest : public Aws::Testing::AwsCppSdkGTestSuite
{
};

// Test that ToCrtHttpRequest produces origin-form request-target (path + query only)
TEST_F(HttpRequestConversionTest, ToCrtHttpRequestOriginForm)
{
// Test with HTTP scheme and non-default port
{
Aws::Http::URI uri("http://example.com:9000/bucket/key?uploadId=abc&partNumber=1");
Aws::Http::Standard::StandardHttpRequest request(uri, Aws::Http::HttpMethod::HTTP_PUT);

auto crtRequest = request.ToCrtHttpRequest();
ASSERT_NE(nullptr, crtRequest);

// The path should be origin-form: /path?query
// NOT absolute-form: http://example.com:9000/path?query
auto pathOpt = crtRequest->GetPath();
ASSERT_TRUE(pathOpt.has_value());
Aws::String path(reinterpret_cast<const char*>(pathOpt->ptr), pathOpt->len);
EXPECT_EQ("/bucket/key?uploadId=abc&partNumber=1", path);
EXPECT_FALSE(path.find("http://") == 0);
EXPECT_FALSE(path.find("https://") == 0);
}

// Test with HTTPS scheme and default port
{
Aws::Http::URI uri("https://s3.amazonaws.com/my-bucket/my-key");
Aws::Http::Standard::StandardHttpRequest request(uri, Aws::Http::HttpMethod::HTTP_GET);

auto crtRequest = request.ToCrtHttpRequest();
ASSERT_NE(nullptr, crtRequest);

auto pathOpt = crtRequest->GetPath();
ASSERT_TRUE(pathOpt.has_value());
Aws::String path(reinterpret_cast<const char*>(pathOpt->ptr), pathOpt->len);
EXPECT_EQ("/my-bucket/my-key", path);
EXPECT_FALSE(path.find("http://") == 0);
EXPECT_FALSE(path.find("https://") == 0);
}

// Test with query string
{
Aws::Http::URI uri("https://storage.example.com:443/path/to/file?query=value&foo=bar");
Aws::Http::Standard::StandardHttpRequest request(uri, Aws::Http::HttpMethod::HTTP_POST);

auto crtRequest = request.ToCrtHttpRequest();
ASSERT_NE(nullptr, crtRequest);

auto pathOpt = crtRequest->GetPath();
ASSERT_TRUE(pathOpt.has_value());
Aws::String path(reinterpret_cast<const char*>(pathOpt->ptr), pathOpt->len);
EXPECT_EQ("/path/to/file?query=value&foo=bar", path);
EXPECT_FALSE(path.find("http://") == 0);
EXPECT_FALSE(path.find("https://") == 0);
}

// Test with root path
{
Aws::Http::URI uri("http://localhost:8080/");
Aws::Http::Standard::StandardHttpRequest request(uri, Aws::Http::HttpMethod::HTTP_HEAD);

auto crtRequest = request.ToCrtHttpRequest();
ASSERT_NE(nullptr, crtRequest);

auto pathOpt = crtRequest->GetPath();
ASSERT_TRUE(pathOpt.has_value());
Aws::String path(reinterpret_cast<const char*>(pathOpt->ptr), pathOpt->len);
// Root path should be "/"
EXPECT_EQ("/", path);
EXPECT_FALSE(path.find("http://") == 0);
}

@sbaluja sbaluja Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add cases for the following paths whose behavior changes?

  • Root path with a query: https://host/?list-type=2 → /?list-type=2. The old code produced https://host?list-type=2.
  • Trailing slash: https://host/bucket/prefix/ → /bucket/prefix/.
  • Characters that need encoding in the key, e.g. a:b c+d → /bucket/a%3Ab%20c%2Bd.
  • Aws::Http::SetPreservePathSeparators(true) with a key that starts with /, which should produce //key. Reset it to false afterwards so it doesn't leak into other tests.

also, each case should be a separate TEST_F instead of scoped blocks in one test, so a failure points at the specific case

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to its own file, e.g. http/HttpRequestTest.cpp. HttpClientTest.cpp is about the HTTP clients and is almost entirely behind DISABLE_DNS_REQUIRED_TESTS, which is why the includes end up repeated mid-file here. http/*.cpp is globbed, so no CMake change is needed.

}