Fix S3CrtClient to use origin-form request-target by hippi345 · Pull Request #3957 · aws/aws-sdk-cpp

hippi345 · 2026-10-16T18:43:14Z

Issue #, if available:

Description of changes:

This PR fixes S3CrtClient to send origin-form request-targets (e.g., PUT /bucket/key HTTP/1.1) instead of absolute-form (e.g., PUT http://host/bucket/key HTTP/1.1) when connecting directly to origin servers.

Problem:
HttpRequest::ToCrtHttpRequest() was constructing absolute-form request-targets for all requests by prepending the scheme, authority, and port to the path. This violates RFC 9112 §3.2.1, which requires origin-form (path + query only) for direct connections to origin servers. While S3 and most S3-compatible servers accept absolute-form, security tools like CrowdSec's http-open-proxy scenario treat it as open-proxy probing, causing IP bans after a single failed request.

Solution:
Modified ToCrtHttpRequest() in src/aws-cpp-sdk-core/source/http/HttpRequest.cpp to set only the path and query string, removing the scheme, authority, and port components.

Proxy behavior is preserved:
The CRT HTTP layer (aws-c-http) handles proxy rewriting via aws_http_rewrite_uri_for_proxy_request (file: crt/aws-crt-cpp/crt/aws-c-http/source/proxy_connection.c, lines 931-1000). This function:

Parses the origin-form path we provide (line 942-949)
Rebuilds it with scheme, host, and port into absolute-form (lines 956-963)
Only when the request goes through a forward proxy (called via s_proxy_http_request_transform at line 1010)

This is per RFC 7230 §5.3.2, which requires absolute-form only for requests through forward proxies. Direct connections use origin-form.

Signing is not affected:
The CRT signers (SigV4/SigV4a via ToCrtHttpRequest) compute signatures over the canonical URI, which should be just the path component. The signing configuration includes the region and service name separately (see AWSAuthV4Signer.cpp lines 95-98), and the Host header is already set correctly. All 1088 core tests pass with this change.

Testing:
Added unit test HttpRequestConversionTest.ToCrtHttpRequestOriginForm that verifies:

Requests use origin-form (no scheme/authority in path)
Paths include query strings correctly
Root paths are handled properly

Test is placed outside DISABLE_DNS_REQUIRED_TESTS guard as it requires no network access.

Test output before fix (showing absolute-form bug):

Expected: "/bucket/key?uploadId=abc&partNumber=1"
Actual: "http://example.com:9000/bucket/key?uploadId=abc&partNumber=1"

[  FAILED  ] HttpRequestConversionTest.ToCrtHttpRequestOriginForm
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test

Test output after fix (origin-form, all tests pass):

[ RUN      ] HttpRequestConversionTest.ToCrtHttpRequestOriginForm
[       OK ] HttpRequestConversionTest.ToCrtHttpRequestOriginForm (0 ms)
[  PASSED  ] 1 test.

Full test suite: 1088 tests passed.

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Generated with AI assistance and reviewed by hippi345.

HttpRequest::ToCrtHttpRequest() was constructing an absolute-form request-target (e.g., "PUT http://host/path HTTP/1.1") for every request. This violates RFC 9112 §3.2.1, which requires origin-form (path + query only) when a client connects directly to an origin server. aws-c-http already handles proxy rewriting via aws_http_rewrite_uri_for_proxy_request, converting to absolute-form only when necessary. By passing origin-form here, we prevent security tools like CrowdSec's http-open-proxy scenario from flagging legitimate S3-compatible storage requests as proxy abuse. Fixes aws#3956

- Remove .changelog file (upstream has no such directory) - Move ToCrtHttpRequestOriginForm test outside DISABLE_DNS_REQUIRED_TESTS guard as it requires no network access

sbaluja · 2026-10-20T18:39:44Z

+            // 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())


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();

sbaluja · 2026-10-20T18:39:47Z

+        // Root path should be "/"
+        EXPECT_EQ("/", path);
+        EXPECT_FALSE(path.find("http://") == 0);
+    }


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

sbaluja · 2026-10-20T18:40:37Z

+        // Root path should be "/"
+        EXPECT_EQ("/", path);
+        EXPECT_FALSE(path.find("http://") == 0);
+    }


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.

Use GetURLEncodedPath() plus the query string. That path always starts with '/', so the empty-path fallback was unnecessary. Move the ToCrtHttpRequest cases into http/HttpRequestTest.cpp and give each one its own test, including root-plus-query, a trailing slash, reserved characters, and a preserved leading slash.

Put the cases in the repo clang-format style and leave HttpClientTest.cpp unchanged from main.

hippi345 · 2026-10-26T16:30:21Z

Thanks for the review. Updated:

Path is now GetURLEncodedPath() plus the query string. That path already always starts with /, including the root.
Split the cases into separate tests. Added /?list-type=2, /bucket/prefix/, /bucket/a%3Ab%20c%2Bd, and //key with SetPreservePathSeparators(true) (reset to false when the test ends).
Moved the tests to http/HttpRequestTest.cpp. HttpClientTest.cpp is back to the main version.

hippi345 added 2 commits October 16, 2026 18:15

Review fixes: remove changelog, move test outside DNS guard

5679182

- Remove .changelog file (upstream has no such directory) - Move ToCrtHttpRequestOriginForm test outside DISABLE_DNS_REQUIRED_TESTS guard as it requires no network access

hippi345 marked this pull request as ready for review October 16, 2026 19:01

sbaluja reviewed Oct 20, 2026

View reviewed changes

hippi345 added 2 commits October 26, 2026 14:19

Format the new HttpRequest tests

95ae28e

Put the cases in the repo clang-format style and leave HttpClientTest.cpp unchanged from main.

Merge branch 'main' into cursor/fix-s3crt-absolute-url-cd0a

eeccda8

Navigation Menu

Repository navigation

Fix S3CrtClient to use origin-form request-target - #3957

Fix S3CrtClient to use origin-form request-target#3957
hippi345 wants to merge 5 commits into
aws:mainfrom
hippi345:cursor/fix-s3crt-absolute-url-cd0a

hippi345 commented Oct 16, 2026

Uh oh!

sbaluja Oct 20, 2026

Uh oh!

sbaluja Oct 20, 2026 •

edited

Loading

Uh oh!

sbaluja Oct 20, 2026

Uh oh!

hippi345 commented Oct 26, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hippi345 commented Oct 16, 2026

Uh oh!

sbaluja Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

sbaluja Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbaluja Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

hippi345 commented Oct 26, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sbaluja Oct 20, 2026 •

edited

Loading