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

Jump to bottom

Fix regression in storage url signing of objects with slashes in names #1348

Merged
merged 2 commits into from
Oct 28, 2016
Merged

Fix regression in storage url signing of objects with slashes in names #1348

merged 2 commits into from
Oct 28, 2016

Conversation

mziccard
Copy link
Contributor

This replaces #1347 and fixes #1346

@ostronom I took the liberty to cherry-pick your commits and apply some fixes, namely:

  • Squash and rebase on top of master
  • Handle ? character: UrlEscapers.urlFragmentEscaper() does not escape ?. Even though the use of ? in blob names is discouraged (see here) we better handle it properly
  • Add slashes and special characters to signUrl integration tests

@ostronom @lesv can you have a look?

@mziccard mziccard added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: storage Issues related to the Cloud Storage API. labels Oct 28, 2016
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Oct 28, 2016
@mziccard mziccard mentioned this pull request Oct 28, 2016
Closed
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 84.286% when pulling e5743e4 on mziccard:signurl-regression into 0221851 on GoogleCloudPlatform:master.

@mziccard
Copy link
Contributor Author

@mickeyreiss this should fix the regression, feel free to comment on :)

lesv
lesv approved these changes Oct 28, 2016
View reviewed changes
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

LGTM - assuming my questions are incorrect.

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java
@@ -528,7 +528,8 @@ public URL signUrl(BlobInfo blobInfo, long duration, TimeUnit unit, SignUrlOptio
if (blobInfo.getName().startsWith("/")) {
path.setLength(path.length() - 1);
}
path.append(UrlEscapers.urlPathSegmentEscaper().escape(blobInfo.getName()));
String escapedName = UrlEscapers.urlFragmentEscaper().escape(blobInfo.getName());
path.append(escapedName.replace("?", "%3F"));

This comment was marked as spam.

Sign in to view

google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java
@@ -1323,7 +1323,8 @@ public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException
String blobName = "/a" + specialChar + "b";
URL url =
storage.signUrl(BlobInfo.newBuilder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS);
String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName);
String escapedBlobName =
UrlEscapers.urlFragmentEscaper().escape(blobName).replace("?", "%3F");

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java
@@ -1193,7 +1193,7 @@ public void testWriteChannelExistingBlob() throws IOException {

@Test
public void testGetSignedUrl() throws IOException {
String blobName = "test-get-signed-url-blob";
String blobName = "test-get-signed-url-blob/with/slashes/and?special=char*";

This comment was marked as spam.

Sign in to view

mickeyreiss
mickeyreiss approved these changes Oct 28, 2016
View reviewed changes
Copy link

@mickeyreiss mickeyreiss left a comment

Choose a reason for hiding this comment

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

👍 Thanks for the admirably fast turnaround!

google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java
@@ -1323,7 +1323,8 @@ public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException
String blobName = "/a" + specialChar + "b";
URL url =
storage.signUrl(BlobInfo.newBuilder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS);
String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName);
String escapedBlobName =
UrlEscapers.urlFragmentEscaper().escape(blobName).replace("?", "%3F");

This comment was marked as spam.

Sign in to view

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 84.382% when pulling 988c1c4 on mziccard:signurl-regression into 0221851 on GoogleCloudPlatform:master.

@lesv
Copy link
Contributor

lesv commented Oct 28, 2016

LGTM

@mziccard mziccard merged commit 76c3a48 into googleapis:master Oct 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesv lesv lesv approved these changes

@mickeyreiss mickeyreiss mickeyreiss approved these changes

@ostronom ostronom ostronom approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: no This human has *not* signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression in storage url signing
6 participants
@mziccard @googlebot @coveralls @lesv @ostronom @mickeyreiss