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.

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

Use RFC 3986 query encoding #33

Merged
merged 2 commits into from
Jun 11, 2021
Merged

Conversation

dvic
Copy link
Contributor

@dvic dvic commented Jun 9, 2021

We stumbled upon this issue when we used the response-content-disposition query parameter (https://cloud.google.com/storage/docs/xml-api/reference-headers#responsecontentdisposition). Without this PR, the signed URL is rejected by Google. Upon further inspection we noticed that Google encoded the space character as %20, while the library encoded it as +.

This PR changes the query encoding to follow RFC 3986, which, among other things, means that spaces are encoded as %20 instead of +.

@coveralls
Copy link

coveralls commented Jun 9, 2021

Pull Request Test Coverage Report for Build 47219f3cc5012fef811aaa591cfa2aa593adb57c-PR-33

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 56b3c0c531151c2b69ee54b3cc12957b6d7ab732: 0.0%
Covered Lines: 84
Relevant Lines: 84

💛 - Coveralls

@mruoss
Copy link
Collaborator

mruoss commented Jun 10, 2021

I see... is this somehow related to #16?

@dvic
Copy link
Contributor Author

dvic commented Jun 10, 2021

I see... is this somehow related to #16?

Probably, I have not used v2 but I can imagine that the encoding should also be RFC3986 and not x-www-form-urlencoded (which is used by URI.encode_www_form/1).

@dvic
Copy link
Contributor Author

dvic commented Jun 10, 2021

I've checked and my tests fail when I use v2. The reason is that #16 missed a clause and the signature was not encoded at all. I pushed a commit that fixes this and changes the encoding to the more recommended RFC 3986 encoding.

@mruoss
Copy link
Collaborator

mruoss commented Jun 11, 2021

Perfect, thank you for your contribution, @dvic! 👍

@mruoss mruoss merged commit e1515a1 into alexandrubagu:master Jun 11, 2021
@atomkirk
Copy link

@dvic any chance you could share how you are calling GcsSignedUrl.generate_v4 and passing response-content-disposition? I'm trying all kinds of things and can't make it work

@dvic
Copy link
Contributor Author

dvic commented Jun 25, 2023

@atomkirk we use the following opts:

query_params: [
  "response-content-disposition": ~s[attachment; filename="#{filename}"],
]

@atomkirk
Copy link

interesting. I'm doing that exact same thing and it's not working. If i remove query_params then it creates a link that works. I'll keep digging. Thanks for the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants