Fix regression in storage url signing of objects with slashes in names #1348
Conversation
mziccard
added
type: bug
|
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
added
the
cla: no
|
@mickeyreiss this should fix the regression, feel free to comment on :) |
lesv
left a comment
lesv
left a comment
There was a problem hiding this comment.
LGTM - assuming my questions are incorrect.
| } | ||
| 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.
This comment was marked as spam.
Sorry, something went wrong.
| 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.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @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.
This comment was marked as spam.
Sorry, something went wrong.
mickeyreiss
left a comment
mickeyreiss
left a comment
There was a problem hiding this comment.
👍 Thanks for the admirably fast turnaround!
| 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.
This comment was marked as spam.
Sorry, something went wrong.
|
LGTM |
6 participants
This replaces #1347 and fixes #1346
@ostronom I took the liberty to cherry-pick your commits and apply some fixes, namely:
?character:UrlEscapers.urlFragmentEscaper()does not escape?. Even though the use of?in blob names is discouraged (see here) we better handle it properlysignUrlintegration tests@ostronom @lesv can you have a look?