Skip to content

fix(s3): preserve question marks in copy source keys #1713

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.

Sign up for GitHub

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

Open
Tomoya0k wants to merge 1 commit into
base: main
Choose a base branch
from
+100 −38
Open

fix(s3): preserve question marks in copy source keys #1713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
80 changes: 43 additions & 37 deletions src/main/java/io/github/hectorvent/floci/services/s3/S3Controller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1738,21 +1738,13 @@ private Response handleCopyObject(String copySource, String destBucket, String d
String contentType, HttpHeaders httpHeaders) {
// copySource format: /bucket/key or bucket/key, where key is URL-encoded
String source = copySource.startsWith("/") ? copySource.substring(1) : copySource;

// URL decode the entire source first, then split
String decodedSource;
try {
decodedSource = URLDecoder.decode(source, StandardCharsets.UTF_8);
} catch (IllegalArgumentException e) {
throw new AwsException("InvalidArgument", "Invalid copy source: " + copySource, 400);
}
int slashIndex = decodedSource.indexOf('/');

int slashIndex = source.indexOf('/');
if (slashIndex <= 0) {
throw new AwsException("InvalidArgument", "Invalid copy source: " + copySource, 400);
}
String sourceBucket = decodedSource.substring(0, slashIndex);
String pathAfterBucket = decodedSource.substring(slashIndex + 1);
ParsedCopySource sourceObject = parseCopySourceObject(pathAfterBucket);
String sourceBucket = decodeCopySourceComponent(source.substring(0, slashIndex), copySource);
ParsedCopySource sourceObject = parseCopySourceObject(source.substring(slashIndex + 1), copySource);
String copyContentEncoding = toPersistedContentEncoding(httpHeaders.getHeaderString("Content-Encoding"));
String copyContentDisposition = httpHeaders.getHeaderString("Content-Disposition");
String copyCacheControl = httpHeaders.getHeaderString("Cache-Control");
Expand Down Expand Up @@ -1800,20 +1792,12 @@ private Response handleUploadPartCopy(String copySource, String destBucket, Stri
// copySource format: /bucket/key or bucket/key, where key is URL-encoded
String source = copySource.startsWith("/") ? copySource.substring(1) : copySource;

// URL decode the entire source first, then split.
String decodedSource;
try {
decodedSource = URLDecoder.decode(source, StandardCharsets.UTF_8);
} catch (IllegalArgumentException e) {
throw new AwsException("InvalidArgument", "Invalid copy source: " + copySource, 400);
}
int slashIndex = decodedSource.indexOf('/');
int slashIndex = source.indexOf('/');
if (slashIndex <= 0) {
throw new AwsException("InvalidArgument", "Invalid copy source: " + copySource, 400);
}
String sourceBucket = decodedSource.substring(0, slashIndex);
String pathAfterBucket = decodedSource.substring(slashIndex + 1);
ParsedCopySource sourceObject = parseCopySourceObject(pathAfterBucket);
String sourceBucket = decodeCopySourceComponent(source.substring(0, slashIndex), copySource);
ParsedCopySource sourceObject = parseCopySourceObject(source.substring(slashIndex + 1), copySource);
String copySourceRange = httpHeaders.getHeaderString("x-amz-copy-source-range");
String eTag = s3Service.uploadPartCopy(destBucket, destKey, uploadId, partNumber,
sourceBucket, sourceObject.objectKey(), sourceObject.versionId(), copySourceRange,
Expand Down Expand Up @@ -2653,42 +2637,64 @@ private Response handlePutBucketWebsite(String bucket, byte[] body) {
}

/**
* Splits the {@code CopyObject}/{@code UploadPartCopy} copy-source remainder into S3 object key and
* optional source {@code versionId}.
* Splits the raw {@code CopyObject}/{@code UploadPartCopy} copy-source remainder into decoded S3 object
* key and optional source {@code versionId}.
* <ul>
* <li><b>Input:</b> decoded {@code x-amz-copy-source} with bucket already removed (substring after
* <li><b>Input:</b> raw {@code x-amz-copy-source} with bucket already removed (substring after
* the {@code '/'} that follows the bucket). Both {@code handleCopyObject} and
* {@code handleUploadPartCopy} compute this as {@code pathAfterBucket}.</li>
* <li><b>Key:</b> substring before the first {@code '?'} if any; keys may contain more {@code '/'}
* segments.</li>
* <li><b>{@code versionId}:</b> first {@code versionId} query pair, when present (raw value after
* {@code '='}). Other query pairs are ignored.</li>
* <li><b>Key:</b> the decoded full path unless a trailing query string contains {@code versionId=}.</li>
* <li><b>{@code versionId}:</b> first decoded {@code versionId} query pair, when present. Other query
* pairs are ignored and do not cause the key to be truncated.</li>
* </ul>
*
* @param pathAfterBucket object key alone, or key with query (for example {@code dir/k.txt?versionId=uuid})
* @param pathAfterBucket raw object key alone, or key with query (for example
* {@code dir/k.txt?versionId=uuid})
* @return key without trailing query plus {@code versionId} value, or {@code null} version when absent
*/
private ParsedCopySource parseCopySourceObject(String pathAfterBucket) {
private ParsedCopySource parseCopySourceObject(String pathAfterBucket, String originalCopySource) {
int queryStart = pathAfterBucket.indexOf('?');
if (queryStart < 0) {
return new ParsedCopySource(pathAfterBucket, null);
return new ParsedCopySource(decodeCopySourceComponent(pathAfterBucket, originalCopySource), null);
}
String objectKey = pathAfterBucket.substring(0, queryStart);

String query = pathAfterBucket.substring(queryStart + 1);
String versionId = extractVersionId(query, originalCopySource);
if (versionId == null) {
return new ParsedCopySource(decodeCopySourceComponent(pathAfterBucket, originalCopySource), null);
}

String objectKey = decodeCopySourceComponent(pathAfterBucket.substring(0, queryStart), originalCopySource);
return new ParsedCopySource(objectKey, versionId);
}

private String extractVersionId(String query, String originalCopySource) {
if (!query.contains("versionId=")) {
return null;
}

String versionId = null;
for (String pair : query.split("&")) {
int eq = pair.indexOf('=');
if (eq <= 0) {
continue;
}
String name = pair.substring(0, eq);
String value = pair.substring(eq + 1);
String name = decodeCopySourceComponent(pair.substring(0, eq), originalCopySource);
String value = decodeCopySourceComponent(pair.substring(eq + 1), originalCopySource);
if ("versionId".equals(name)) {
versionId = value;
break;
}
}
return new ParsedCopySource(objectKey, versionId);
return versionId;
Comment on lines +2671 to +2689

@greptile-apps greptile-apps Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 versionId silently dropped when key contains a raw ? alongside ?versionId=

When a copy-source header contains both a raw (unencoded) ? in the key and a trailing ?versionId=X — e.g. /bucket/dir/file?.txt?versionId=abc — parseCopySourceObject finds the first ? at the ? inside the key. The query string becomes txt?versionId=abc, which does contain versionId=. extractVersionId then splits on & and gets the single token txt?versionId=abc; its name (everything before the first =) is txt?versionId ≠ "versionId", so it returns null. The fallback then decodes the entire pathAfterBucket (including ?versionId=abc) as the key, silently discarding the version selector. The recommended AWS approach is to percent-encode ? as %3F in the key portion, which the new code handles correctly — worth documenting in the Javadoc on parseCopySourceObject.

}

private String decodeCopySourceComponent(String value, String originalCopySource) {
try {
return URLDecoder.decode(value, StandardCharsets.UTF_8);
} catch (IllegalArgumentException e) {
throw new AwsException("InvalidArgument", "Invalid copy source: " + originalCopySource, 400);
}
}

private record ParsedCopySource(String objectKey, String versionId) {
Expand Down
58 changes: 57 additions & 1 deletion src/test/java/io/github/hectorvent/floci/services/s3/S3IntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,62 @@ void copyObjectWithNonAsciiKeySucceeds() {

@Test
@Order(21)
void copyObjectWithQuestionMarkInSourceKeySucceeds() {
String sourceBucket = "copy-question-source-bucket";
String destBucket = "copy-question-dest-bucket";
String srcKey = "folder/file with question ?.txt";
String encodedSrcKey = "folder/file%20with%20question%20%3F.txt";

given().put("/" + sourceBucket).then().statusCode(200);
given().put("/" + destBucket).then().statusCode(200);

given()
.contentType("text/plain")
.body("copy test")
.when()
.put("/" + sourceBucket + "/" + srcKey)
.then()
.statusCode(200);
Comment on lines +395 to +401

@greptile-apps greptile-apps Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 PUT stores source object at wrong key due to raw ? in URL path

REST Assured (backed by Apache HttpClient) treats ? as the query-string delimiter when building the request URI, not as a literal path character. The PUT call .put("/" + sourceBucket + "/" + srcKey) where srcKey = "folder/file with question ?.txt" will send a request with path /copy-question-source-bucket/folder/file with question and query string txt. The server stores the object at key folder/file with question (without ?.txt). Both subsequent copy operations then look up folder/file with question ?.txt, find no object, and would return NoSuchKey — the test would fail at the copy-source assertions, not validate the fix. Use the already-defined encodedSrcKey variable for the PUT path so the ? is sent percent-encoded as %3F and the key is stored correctly.


given()
.header("x-amz-copy-source", "/" + sourceBucket + "/" + encodedSrcKey)
.when()
.put("/" + destBucket + "/copied/encoded.txt")
.then()
.statusCode(200)
.body(containsString("CopyObjectResult"));

given()
.header("x-amz-copy-source", "/" + sourceBucket + "/" + srcKey)
.when()
.put("/" + destBucket + "/copied/raw.txt")
.then()
.statusCode(200)
.body(containsString("CopyObjectResult"));

given()
.when()
.get("/" + destBucket + "/copied/encoded.txt")
.then()
.statusCode(200)
.body(equalTo("copy test"));

given()
.when()
.get("/" + destBucket + "/copied/raw.txt")
.then()
.statusCode(200)
.body(equalTo("copy test"));

given().delete("/" + sourceBucket + "/" + srcKey);
given().delete("/" + destBucket + "/copied/encoded.txt");
given().delete("/" + destBucket + "/copied/raw.txt");
given().delete("/" + sourceBucket);
given().delete("/" + destBucket);
}
Comment on lines 384 to +438

@greptile-apps greptile-apps Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 UploadPartCopy path has no regression coverage for ? in source key

The PR description states the same parsing fix was applied to both CopyObject and UploadPartCopy to keep them aligned. Only CopyObject has a regression test here. Per AGENTS.md, any behavior affecting AWS compatibility should have automated test coverage. Adding a corresponding UploadPartCopy scenario (initiate multipart upload → uploadPartCopy with a percent-encoded ? in the key → complete) would verify the handleUploadPartCopy path stays correct as well.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


@Test
@Order(22)
void copyObjectWithMalformedEncodedSourceReturns400() {
given()
.header("x-amz-copy-source", "/test-bucket/%ZZinvalid")
Expand All @@ -394,7 +450,7 @@ void copyObjectWithMalformedEncodedSourceReturns400() {
}

@Test
@Order(22)
@Order(23)
void copyObjectWithEmptyBucketReturns400() {
given()
.header("x-amz-copy-source", "/key-only-no-bucket")
Expand Down