Skip to content

Fixed escaping of space characters as query params on signer_v4#7660

Merged
quartzmo merged 1 commit intogoogleapis:masterfrom
cfactolerin:master
Sep 17, 2020
Merged

Fixed escaping of space characters as query params on signer_v4#7660
quartzmo merged 1 commit intogoogleapis:masterfrom
cfactolerin:master

Conversation

@cfactolerin
Copy link
Copy Markdown
Contributor

@cfactolerin cfactolerin commented Sep 15, 2020

Issue ticket opened: #7655

Context on this PR:

  • Change any + characters in the query params to %XX UTF-8 style

@cfactolerin
Fix issues when escaping query parameters with spaces specially when …
90cf55c 
…doing response-content-disposition. The + char that CGI.escape does not work during decoding on the live api
@cfactolerin cfactolerin requested a review from a team September 15, 2020 03:22
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 15, 2020
@quartzmo quartzmo self-assigned this Sep 15, 2020
@quartzmo quartzmo added the api: storage Issues related to the Cloud Storage API. label Sep 15, 2020
@quartzmo
Copy link
Copy Markdown
Member

quartzmo commented Sep 15, 2020

It appears to me that the test expectation updates are necessary because of a space ( ) in the values of the query string parameter response-content-disposition. The value attachment; filename was encoded as attachment%3B+filename before this PR, but is now encoded as attachment%3B%20filename.

@frankyn Does this look OK to you?

@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 15, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 15, 2020
@quartzmo
Copy link
Copy Markdown
Member

quartzmo commented Sep 15, 2020

@frankyn What do you think about adding conformance test coverage for spaces in file names?

@quartzmo
Copy link
Copy Markdown
Member

quartzmo commented Sep 16, 2020

Thanks @frankyn for the PR to add space character coverage to conformance tests.

@cfactolerin I'd like to wait for the above PR to be merged, then I'll copy it into this repo, and let you know to rebase this branch. Thanks!

@quartzmo quartzmo mentioned this pull request Sep 17, 2020
Merged
@quartzmo quartzmo merged commit 2180747 into googleapis:master Sep 17, 2020
@quartzmo quartzmo mentioned this pull request Sep 17, 2020
Merged
@quartzmo
Copy link
Copy Markdown
Member

quartzmo commented Sep 17, 2020

@cfactolerin Thank you again for this contribution! I decided to merge this PR first and follow it with #7804. Hopefully your change will be released Monday 2020-09-21.

@quartzmo
Copy link
Copy Markdown
Member

quartzmo commented Sep 22, 2020

Released in v1.29.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@quartzmo quartzmo quartzmo approved these changes

Assignees

@quartzmo quartzmo

Labels

api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cfactolerin @quartzmo @yoshi-kokoro