Skip to content
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

fix: a resumable session without a Range header should be interpreted as 0 length #2182

Merged
merged 2 commits into from
Aug 30, 2023
Merged

fix: a resumable session without a Range header should be interpreted as 0 length #2182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98521f2
fix: a resumable session without a Range header should be interpreted…
BenWhitehead Aug 25, 2023
bf0fbb8
chore: review fixes
BenWhitehead Aug 29, 2023
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
4 changes: 2 additions & 2 deletions ...d-storage/src/main/java/com/google/cloud/storage/JsonResumableSessionFailureScenario.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ enum JsonResumableSessionFailureScenario {
BaseServiceException.UNKNOWN_CODE,
"dataLoss",
"Client side data loss detected. Bytes acked is more than client sent."),
SCENARIO_9(503, "backendNotConnected", "Ack less than bytes sent"),
QUERY_SCENARIO_1(503, "", "Missing Range header in response");
SCENARIO_9(503, "backendNotConnected", "Ack less than bytes sent");

private static final String PREFIX_I = "\t|< ";
private static final String PREFIX_O = "\t|> ";
Expand All @@ -79,6 +78,7 @@ enum JsonResumableSessionFailureScenario {
.or(matches("Content-Type"))
.or(matches("Range"))
.or(startsWith("X-Goog-Stored-"))
.or(matches("X-Goog-GCS-Idempotency-Token"))
Copy link
Member

Choose a reason for hiding this comment

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

What if the code collapsed to startsWith("X-Goog-")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to reduce the scope to prevent an accidental inclusion of a header that might contain a sensitive value.

If it were relaxed to x-goog- that could pull in object metadata fields which could have sensitive values.

Since these error messages go into peoples logs, we should only include what we know is safe.

.or(matches("X-GUploader-UploadID"));

private static final Predicate<Map.Entry<String, ?>> includeHeader =
Expand Down
7 changes: 5 additions & 2 deletions ...e-cloud-storage/src/main/java/com/google/cloud/storage/JsonResumableSessionQueryTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,11 @@ final class JsonResumableSessionQueryTask
long endOffset = range.endOffset();
return ResumableOperationResult.incremental(endOffset);
} else {
throw JsonResumableSessionFailureScenario.QUERY_SCENARIO_1.toStorageException(
uploadId, response);
// According to
// https://cloud.google.com/storage/docs/performing-resumable-uploads#status-check a 308
// response that does not contain a Range header should be interpreted as GCS having
// received no data.
return ResumableOperationResult.incremental(0);
}
} else {
HttpResponseException cause = new HttpResponseException(response);
Expand Down
10 changes: 3 additions & 7 deletions ...d-storage/src/test/java/com/google/cloud/storage/ITJsonResumableSessionQueryTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,6 @@ public void incompleteSession() throws Exception {
}
}

/**
* This is a hard failure from the perspective of GCS as a range header is a required header to be
* included in the response to a query upload request.
*/
@Test
public void incompleteSession_missingRangeHeader() throws Exception {
HttpRequestHandler handler =
Expand All @@ -156,9 +152,9 @@ public void incompleteSession_missingRangeHeader() throws Exception {
JsonResumableSessionQueryTask task =
new JsonResumableSessionQueryTask(httpClientContext, uploadUrl);

StorageException se = assertThrows(StorageException.class, task::call);
assertThat(se.getCode()).isEqualTo(503);
assertThat(se).hasMessageThat().contains("Range");
ResumableOperationResult<@Nullable StorageObject> result = task.call();
assertThat(result.getPersistedSize()).isEqualTo(0);
assertThat(result.getObject()).isNull();
}
}

Expand Down
19 changes: 19 additions & 0 deletions ...orage/src/test/java/com/google/cloud/storage/JsonResumableSessionFailureScenarioTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,25 @@ public void xGoogStoredHeadersIncludedIfPresent() throws IOException {
assertThat(storageException).hasMessageThat().contains("|< x-goog-stored-something: blah");
}

@Test
public void xGoogGcsIdempotencyTokenHeadersIncludedIfPresent() throws IOException {
HttpRequest req =
new MockHttpTransport()
.createRequestFactory()
.buildPutRequest(new GenericUrl("http://localhost:80980"), new EmptyContent());
req.getHeaders().setContentLength(0L);

HttpResponse resp = req.execute();
resp.getHeaders().set("X-Goog-Gcs-Idempotency-Token", "5").setContentLength(0L);

StorageException storageException =
JsonResumableSessionFailureScenario.SCENARIO_0.toStorageException(
"uploadId", resp, null, () -> null);

assertThat(storageException.getCode()).isEqualTo(0);
assertThat(storageException).hasMessageThat().contains("|< x-goog-gcs-idempotency-token: 5");
}

private static final class Cause extends RuntimeException {

private Cause() {
Expand Down