Skip to content

Commit 9210930

Browse files
Laonelvagaerg
authored andcommitted
Fix query parameters encoding in rewritten target requests
Fixes request rewriting when query parameters contain a value that matches a percent-encoded value (e.g. `%2F`). Previously, such values were not encoded when adding query parameters to the rewritten request, as the method used internally by `JerseyUriBuilder.queryParam` to encode values (UriComponent.contextualEncode) is recognizing percent-encoded values to avoid double-encoding. This led to changing the original request, and subsequently the remote denying the request with `SignatureDoesNotMatch` error. To fix this, we can encode all query parameter values before passing them to the URI builder, as we know that they require encoding.
1 parent 2ab4bb4 commit 9210930

3 files changed

Lines changed: 35 additions & 1 deletion

File tree

trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/rest/TrinoS3ProxyClient.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@
6060
import static java.lang.annotation.ElementType.PARAMETER;
6161
import static java.lang.annotation.RetentionPolicy.RUNTIME;
6262
import static java.util.Objects.requireNonNull;
63+
import static org.glassfish.jersey.uri.UriComponent.Type.QUERY_PARAM;
64+
import static org.glassfish.jersey.uri.UriComponent.encode;
6365

6466
public class TrinoS3ProxyClient
6567
{
@@ -238,7 +240,7 @@ private static String buildRemoteHost(URI remoteUri)
238240
private static UriBuilder uriBuilder(MultiMap queryParameters)
239241
{
240242
UriBuilder uriBuilder = UriBuilder.newInstance();
241-
queryParameters.forEachEntry(uriBuilder::queryParam);
243+
queryParameters.forEachEntry((name, value) -> uriBuilder.queryParam(name, encode(value, QUERY_PARAM)));
242244
return uriBuilder;
243245
}
244246

trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/AbstractTestProxiedRequests.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,33 @@ public void testPathsNeedingEscaping()
251251
internalClient.deleteBucket(r -> r.bucket(bucket));
252252
}
253253

254+
@Test
255+
public void testPathsNeedingEscapingAsQueryParam()
256+
{
257+
String bucket = "escapes-query";
258+
String targetBucket = requestRewriteController.getTargetBucket(bucket, "");
259+
remoteClient.createBucket(r -> r.bucket(targetBucket));
260+
internalClient.putObject(r -> r.bucket(bucket).key("a=1/b=2/1"), RequestBody.fromString("something"));
261+
internalClient.putObject(r -> r.bucket(bucket).key("a=1%2Fb=2/2"), RequestBody.fromString("else"));
262+
263+
assertThat(listFilesInS3Bucket(remoteClient, targetBucket, requestRewriteController.getTargetKey(bucket, "a=1/b=2")))
264+
.containsExactlyInAnyOrder(requestRewriteController.getTargetKey(bucket, "a=1/b=2/1"));
265+
// Provide target key prefix - rewriting query parameters is not supported
266+
assertThat(listFilesInS3Bucket(internalClient, bucket, requestRewriteController.getTargetKey(bucket, "a=1/b=2")))
267+
.containsExactlyInAnyOrder(requestRewriteController.getTargetKey(bucket, "a=1/b=2/1"));
268+
269+
assertThat(listFilesInS3Bucket(remoteClient, targetBucket, requestRewriteController.getTargetKey(bucket, "a=1%2Fb=2")))
270+
.containsExactlyInAnyOrder(requestRewriteController.getTargetKey(bucket, "a=1%2Fb=2/2"));
271+
// Provide target key prefix - rewriting query parameters is not supported
272+
assertThat(listFilesInS3Bucket(internalClient, bucket, requestRewriteController.getTargetKey(bucket, "a=1%2Fb=2")))
273+
.containsExactlyInAnyOrder(requestRewriteController.getTargetKey(bucket, "a=1%2Fb=2/2"));
274+
275+
internalClient.deleteObject(r -> r.bucket(bucket).key("a=1/b=2/1"));
276+
internalClient.deleteObject(r -> r.bucket(bucket).key("a=1%2Fb=2/2"));
277+
assertThat(listFilesInS3Bucket(internalClient, bucket)).isEmpty();
278+
internalClient.deleteBucket(r -> r.bucket(bucket));
279+
}
280+
254281
@Test
255282
public void testKeyOrBucketDoesNotExist()
256283
{

trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/testing/TestingUtil.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,11 @@ public static List<String> listFilesInS3Bucket(S3Client storageClient, String bu
150150
return storageClient.listObjects(request -> request.bucket(bucket)).contents().stream().map(S3Object::key).collect(toImmutableList());
151151
}
152152

153+
public static List<String> listFilesInS3Bucket(S3Client storageClient, String bucket, String prefix)
154+
{
155+
return storageClient.listObjects(request -> request.bucket(bucket).prefix(prefix)).contents().stream().map(S3Object::key).collect(toImmutableList());
156+
}
157+
153158
@SuppressWarnings("UnstableApiUsage")
154159
public static String sha256(String content)
155160
{

0 commit comments

Comments
 (0)