Skip to content

Commit 85352be

Browse files
L-ApplinL-Applin
authored
fix SigV4a signer incorrectly interpreting query params with '&' (#5383)
1 parent 30c0fe8 commit 85352be

File tree

3 files changed

+77
-5
lines changed

3 files changed

+77
-5
lines changed

‎.changes/next-release/bugfix-AWSSDKforJavav2-0daac40.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "AWS SDK for Java v2",
4+
"contributor": "",
5+
"description": "fix SigV4a signer incorrectly interpreting query params with '&'"
6+
}

‎core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/crt/internal/util/CrtHttpRequestConverter.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public static SdkHttpRequest toRequest(SdkHttpRequest request, HttpRequest crtRe
7979
}
8080

8181
builder.encodedPath(fullUri.getRawPath());
82-
String remainingQuery = fullUri.getQuery();
82+
String remainingQuery = fullUri.getRawQuery();
8383

8484
builder.clearQueryParameters();
8585
while (remainingQuery != null && !remainingQuery.isEmpty()) {
@@ -92,7 +92,9 @@ public static SdkHttpRequest toRequest(SdkHttpRequest request, HttpRequest crtRe
9292
queryValue = remainingQuery.substring(nextAssign + 1, nextQuery);
9393
}
9494

95-
builder.appendRawQueryParameter(queryName, queryValue);
95+
String decodedQueryValue = SdkHttpUtils.urlDecode(queryValue);
96+
String decodedQueryName = SdkHttpUtils.urlDecode(queryName);
97+
builder.appendRawQueryParameter(decodedQueryName, decodedQueryValue);
9698
} else {
9799
String queryName = remainingQuery;
98100
if (nextQuery >= 0) {

‎core/http-auth-aws/src/test/java/software/amazon/awssdk/http/auth/aws/crt/internal/signer/DefaultAwsCrtV4aHttpSignerTest.java

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,28 @@
3131
import static software.amazon.awssdk.http.auth.aws.crt.internal.util.CrtUtils.toCredentials;
3232
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.ChecksumUtil.readAll;
3333
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4FamilyHttpSigner.CHECKSUM_ALGORITHM;
34+
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4FamilyHttpSigner.SERVICE_SIGNING_NAME;
3435
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4aHttpSigner.AUTH_LOCATION;
3536
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4aHttpSigner.AuthLocation;
3637
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4aHttpSigner.CHUNK_ENCODING_ENABLED;
3738
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4aHttpSigner.EXPIRATION_DURATION;
3839
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4aHttpSigner.PAYLOAD_SIGNING_ENABLED;
40+
import static software.amazon.awssdk.http.auth.aws.signer.AwsV4aHttpSigner.REGION_SET;
41+
import static software.amazon.awssdk.http.auth.spi.signer.HttpSigner.SIGNING_CLOCK;
3942

43+
import java.io.ByteArrayInputStream;
44+
import java.net.URI;
4045
import java.time.Duration;
46+
import java.time.Instant;
47+
import java.util.List;
48+
import java.util.Map;
4149
import org.junit.jupiter.api.Test;
4250
import software.amazon.awssdk.crt.auth.signing.AwsSigningConfig;
4351
import software.amazon.awssdk.http.Header;
52+
import software.amazon.awssdk.http.SdkHttpMethod;
53+
import software.amazon.awssdk.http.SdkHttpRequest;
54+
import software.amazon.awssdk.http.auth.aws.TestUtils;
55+
import software.amazon.awssdk.http.auth.aws.signer.RegionSet;
4456
import software.amazon.awssdk.http.auth.spi.signer.AsyncSignRequest;
4557
import software.amazon.awssdk.http.auth.spi.signer.SignRequest;
4658
import software.amazon.awssdk.http.auth.spi.signer.SignedRequest;
@@ -118,6 +130,58 @@ public void sign_withQuery_shouldSignWithQueryParams() {
118130
assertThat(signedRequest.request().firstMatchingRawQueryParameter("X-Amz-Signature")).isPresent();
119131
}
120132

133+
@Test
134+
void sign_requestWithQueryEncodedParamValue_shouldEncodedValue() {
135+
AwsCredentialsIdentity credentials =
136+
AwsCredentialsIdentity.create("AKIDEXAMPLE", "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY");
137+
SignRequest<AwsCredentialsIdentity> request =
138+
SignRequest.builder(credentials)
139+
.request(SdkHttpRequest.builder()
140+
.method(SdkHttpMethod.POST)
141+
.port(443)
142+
.putHeader("x-amz-archive-description", "test test")
143+
.putHeader("Host", "demo.us-east-1.amazonaws.com")
144+
.encodedPath("/")
145+
.uri(URI.create("https://demo.us-east-1.amazonaws.com"))
146+
.appendRawQueryParameter("goodParam1", "123")
147+
.appendRawQueryParameter("badParam", "abc&xyz")
148+
.appendRawQueryParameter("goodParam2", "abc")
149+
.build())
150+
.payload(() -> new ByteArrayInputStream("{\"TableName\": \"foo\"}".getBytes()))
151+
.putProperty(REGION_SET, RegionSet.create("aws-global"))
152+
.putProperty(SERVICE_SIGNING_NAME, "demo")
153+
.putProperty(SIGNING_CLOCK, new TestUtils.TickingClock(Instant.ofEpochMilli(1596476903000L)))
154+
.putProperty(AUTH_LOCATION, AuthLocation.QUERY_STRING)
155+
.build();
156+
157+
SignedRequest signedRequest = signer.sign(request);
158+
Map<String, List<String>> queryParam = signedRequest.request().rawQueryParameters();
159+
assertThat(queryParam).doesNotContainKey("xyz");
160+
assertThat(queryParam).containsKeys("goodParam1", "badParam", "goodParam2");
161+
162+
assertThat(signedRequest.request().encodedQueryParameters())
163+
.isPresent()
164+
.get()
165+
.matches(str -> str.contains("badParam=abc%26xyz"));
166+
167+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("goodParam1"))
168+
.hasValue("123");
169+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("badParam"))
170+
.hasValue("abc&xyz");
171+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("goodParam2"))
172+
.hasValue("abc");
173+
174+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("X-Amz-Algorithm"))
175+
.hasValue("AWS4-ECDSA-P256-SHA256");
176+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("X-Amz-Credential"))
177+
.hasValue("AKIDEXAMPLE/20200803/demo/aws4_request");
178+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("X-Amz-Date")).hasValue("20200803T174823Z");
179+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("X-Amz-SignedHeaders"))
180+
.hasValue("host;x-amz-archive-description");
181+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("X-Amz-Region-Set")).hasValue("aws-global");
182+
assertThat(signedRequest.request().firstMatchingRawQueryParameter("X-Amz-Signature")).isPresent();
183+
}
184+
121185
@Test
122186
public void sign_withQueryAndExpiration_shouldSignWithQueryParamsAndExpire() {
123187
AwsCredentialsIdentity credentials =
@@ -222,7 +286,7 @@ public void sign_WithChunkEncodingTrue_DelegatesToAwsChunkedPayloadSigner() {
222286
SignedRequest signedRequest = signer.sign(request);
223287

224288
assertThat(signedRequest.request().firstMatchingHeader("x-amz-content-sha256"))
225-
.hasValue(STREAMING_AWS4_ECDSA_P256_SHA256_PAYLOAD);
289+
.hasValue(STREAMING_AWS4_ECDSA_P256_SHA256_PAYLOAD);
226290
assertThat(signedRequest.request().firstMatchingHeader(Header.CONTENT_LENGTH)).hasValue("353");
227291
assertThat(signedRequest.request().firstMatchingHeader("x-amz-decoded-content-length")).hasValue("20");
228292

@@ -244,7 +308,7 @@ public void sign_WithChunkEncodingTrueAndChecksumAlgorithm_DelegatesToAwsChunked
244308
SignedRequest signedRequest = signer.sign(request);
245309

246310
assertThat(signedRequest.request().firstMatchingHeader("x-amz-content-sha256"))
247-
.hasValue(STREAMING_AWS4_ECDSA_P256_SHA256_PAYLOAD_TRAILER);
311+
.hasValue(STREAMING_AWS4_ECDSA_P256_SHA256_PAYLOAD_TRAILER);
248312
assertThat(signedRequest.request().firstMatchingHeader(Header.CONTENT_LENGTH)).hasValue("554");
249313
assertThat(signedRequest.request().firstMatchingHeader("x-amz-decoded-content-length")).hasValue("20");
250314
assertThat(signedRequest.request().firstMatchingHeader("x-amz-trailer")).hasValue("x-amz-checksum-crc32");
@@ -268,7 +332,7 @@ public void sign_WithPayloadSigningFalseAndChunkEncodingTrueAndTrailer_Delegates
268332
SignedRequest signedRequest = signer.sign(request);
269333

270334
assertThat(signedRequest.request().firstMatchingHeader("x-amz-content-sha256"))
271-
.hasValue(STREAMING_UNSIGNED_PAYLOAD_TRAILER);
335+
.hasValue(STREAMING_UNSIGNED_PAYLOAD_TRAILER);
272336
assertThat(signedRequest.request().firstMatchingHeader(Header.CONTENT_LENGTH)).hasValue("62");
273337
assertThat(signedRequest.request().firstMatchingHeader("x-amz-decoded-content-length")).hasValue("20");
274338
assertThat(signedRequest.request().firstMatchingHeader("x-amz-trailer")).hasValue("x-amz-checksum-crc32");

0 commit comments

Comments
 (0)