Skip to content

Allowing keys with leading slashes for S3 #1264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 28, 2019
Merged
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
5 changes: 5 additions & 0 deletions .changes/next-release/bugfix-AmazonS3-1f528cd.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"category": "Amazon S3",
"type": "bugfix",
"description": "Allows S3 to be used with object keys that have a leading slash \"/myKey\""
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public final class SimpleTypePathMarshaller {
* so that it preserves the path structure.
*/
public static final XmlMarshaller<String> GREEDY_STRING =
new SimplePathMarshaller<>(ValueToStringConverter.FROM_STRING, PathMarshaller.GREEDY);
new SimplePathMarshaller<>(ValueToStringConverter.FROM_STRING, PathMarshaller.GREEDY_WITH_SLASHES);
Copy link
Contributor

Choose a reason for hiding this comment

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

What services are affected by this change? How certain are we that they wouldn't be broken by this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only effects XML services of which none of them currently use the greedy path marshaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than S3 that is.


public static final XmlMarshaller<Void> NULL = (val, context, paramName, sdkField) -> {
throw new IllegalArgumentException(String.format("Parameter '%s' must not be null", paramName));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ public abstract class PathMarshaller {
*/
public static final PathMarshaller GREEDY = new GreedyPathMarshaller();

/**
* Marshaller for greedy path labels that allows leading slahes. Value is not URL encoded and
* replaced in the request URI.
*/
public static final PathMarshaller GREEDY_WITH_SLASHES = new GreedyLeadingSlashPathMarshaller();

private PathMarshaller() {
}

Expand Down Expand Up @@ -68,4 +74,14 @@ public String marshall(String resourcePath, String paramName, String pathValue)
}
}

private static class GreedyLeadingSlashPathMarshaller extends PathMarshaller {

@Override
public String marshall(String resourcePath, String paramName, String pathValue) {
Validate.notEmpty(pathValue, "%s cannot be empty.", paramName);
return resourcePath.replace(String.format("{%s+}", paramName),
SdkHttpUtils.urlEncodeIgnoreSlashes(pathValue));
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,27 @@ public class ApacheHttpRequestFactory {

public HttpRequestBase create(final HttpExecuteRequest request, final ApacheHttpRequestConfig requestConfig) {
URI uri = request.httpRequest().getUri();
HttpRequestBase base = createApacheRequest(request, uri.toString());

HttpRequestBase base = createApacheRequest(request, sanitizeUri(uri));
addHeadersToRequest(base, request.httpRequest());
addRequestConfig(base, request.httpRequest(), requestConfig);

return base;
}

/**
* The Apache HTTP client doesn't allow consecutive slashes in the URI. For S3
* and other AWS services, this is allowed and required. This methods replaces
* any occurrence of "//" in the URI path with "/%2F".
*
* @param uri The existing URI with double slashes not sanitized for Apache.
* @return a new String containing the modified URI
*/
private String sanitizeUri(URI uri) {
String newPath = uri.getPath().replace("//", "/%2F");
return uri.toString().replace(uri.getPath(), newPath);
}

private void addRequestConfig(final HttpRequestBase base,
final SdkHttpRequest request,
final ApacheHttpRequestConfig requestConfig) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright 2010-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/
package software.amazon.awssdk.services.s3;

import static software.amazon.awssdk.testutils.service.S3BucketUtils.temporaryBucketName;

import java.nio.charset.StandardCharsets;
import org.junit.BeforeClass;
import org.junit.Test;
import software.amazon.awssdk.core.sync.RequestBody;

public class KeysWithLeadingSlashIntegrationTest extends S3IntegrationTestBase {

private static final String BUCKET = temporaryBucketName(KeysWithLeadingSlashIntegrationTest.class);
private static final String KEY = "/stupidkeywithillegalleadingslashthatsucks";
Copy link
Contributor

Choose a reason for hiding this comment

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

No passive aggression here. Nope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a random key name I swear.

private static final byte[] CONTENT = "Hello".getBytes(StandardCharsets.UTF_8);

@BeforeClass
public static void setUp() throws Exception {
S3IntegrationTestBase.setUp();
createBucket(BUCKET);
}

@Test
public void putObject_KeyWithLeadingSlash_Succeeds() {
s3.putObject(r -> r.bucket(BUCKET).key(KEY), RequestBody.fromBytes(CONTENT));
String retrievedKey = s3.listObjects(r -> r.bucket(BUCKET)).contents().get(0).key();

assert(retrievedKey).equals(KEY);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -487,24 +487,6 @@
}
}
},
{
"description": "Input with greedy label in path, leading slash removed",
"given": {
"input": {
"NonGreedyPathParam": "pathParamValue",
"GreedyPathParam": "/foo/bar/baz"
}
},
"when": {
"action": "marshall",
"operation": "OperationWithGreedyLabel"
},
"then": {
"serializedAs": {
"uri": "/2016-03-11/operationWithGreedyLabel/pathParamValue/foo/bar/baz"
}
}
},
{
"description": "Operation with null members that have the idempotent trait are autofilled.",
"when": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,24 @@
}
}
}
},
{
"description": "Input with greedy label in path, leading slash removed",
"given": {
"input": {
"NonGreedyPathParam": "pathParamValue",
"GreedyPathParam": "/foo/bar/baz"
}
},
"when": {
"action": "marshall",
"operation": "OperationWithGreedyLabel"
},
"then": {
"serializedAs": {
"uri": "/2016-03-11/operationWithGreedyLabel/pathParamValue/foo/bar/baz"
}
}
}
// TODO This is a post process customization for API Gateway
// {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,24 @@
}
}
}
},
{
"description": "Input with greedy label in path, leading slash removed",
"given": {
"input": {
"NonGreedyPathParam": "pathParamValue",
"GreedyPathParam": "/foo/bar/baz"
}
},
"when": {
"action": "marshall",
"operation": "OperationWithGreedyLabel"
},
"then": {
"serializedAs": {
"uri": "/2016-03-11/operationWithGreedyLabel/pathParamValue//foo/bar/baz"
}
}
}
// TODO this is not possible, payloads can only be structures or blobs. Only S3 utilizes this
// {
Expand Down