Fix regression in storage url signing of objects with slashes in names by mziccard · Pull Request #1348 · googleapis/google-cloud-java · GitHub
Skip to content

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

Merged
mziccard merged 2 commits into
googleapis:masterfrom
mziccard:signurl-regression
Oct 28, 2016
Merged

Fix regression in storage url signing of objects with slashes in names#1348
mziccard merged 2 commits into
googleapis:masterfrom
mziccard:signurl-regression

Conversation

@mziccard

@mziccard mziccard commented Oct 28, 2016

Copy link
Copy Markdown
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

googlebot commented Oct 28, 2016

Copy link
Copy Markdown

@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

coveralls commented Oct 28, 2016

Copy link
Copy Markdown

Coverage Status

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

@mziccard

mziccard commented Oct 28, 2016

Copy link
Copy Markdown
Contributor Author

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

lesv
lesv approved these changes Oct 28, 2016
View reviewed changes

@lesv lesv left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java
}
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

Comment thread google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java
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

Comment thread google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java Outdated
@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

@mickeyreiss mickeyreiss left a comment

Copy link
Copy Markdown

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!

Comment thread google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java
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

coveralls commented Oct 28, 2016

Copy link
Copy Markdown

Coverage Status

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

@lesv

lesv commented Oct 28, 2016

Copy link
Copy Markdown
Contributor

@mziccard mziccard merged commit 76c3a48 into googleapis:master Oct 28, 2016
chingor13 pushed a commit that referenced this pull request Jan 22, 2026
@lqiu96
feat: next release from main branch is 2.18.0 (#1348)
576a30c
chingor13 pushed a commit that referenced this pull request Feb 24, 2026
@release-please
chore(main): release 1.21.1-SNAPSHOT (#1348)
ab87281 
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
chingor13 pushed a commit that referenced this pull request Mar 12, 2026
@release-please
chore(main): release 1.21.1-SNAPSHOT (#1348)
1366fa4 
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
suztomo pushed a commit to suztomo/google-cloud-java that referenced this pull request Mar 23, 2026
@olavloite
chore: add copyright header to expected query in test (googleapis#1348)
1ff73e9 
Fixes the build error in googleapis#1345

This test failure is caused by the recent update that ensures that PostgreSQL-dialect queries also send all comments to Cloud Spanner, which means that the expected query in this test has changed.
meltsufin pushed a commit that referenced this pull request Apr 29, 2026
@release-please
chore(main): release 1.120.22-SNAPSHOT (#1348)
47e4a74 
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
meltsufin pushed a commit that referenced this pull request May 1, 2026
@release-please
chore(main): release 1.120.22-SNAPSHOT (#1348)
203c5a1 
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@ostronom ostronom ostronom approved these changes
@mickeyreiss mickeyreiss mickeyreiss approved these changes
@lesv lesv lesv approved these changes
Reviewers whose approvals may not affect merge requirements

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