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

Rework de/encoding in _sanitizePath method to be more careful #25

Merged
merged 3 commits into from
May 25, 2017
Merged

Conversation

jayeb
Copy link
Contributor

@jayeb jayeb commented May 24, 2017

This PR is intended to fix the two issues brought up in #24, both of which involve encoding errors brought about by ham-fistedly decoding all paths fed in to the _sanitizePath() private method before attempting to re-encode things properly based on whether we determined the path was a fully qualified URL or just a path fragment.

Instead of this clumsy approach, we're now evaluating the given path up front to determine which of the following categories it falls into. Each category is then handled slightly differently:

  • Fully-qualified, unencoded URLs. If the given path matches /^https?:\/\//, we assume its a fully-qualified, unencoded URL. All we need to do here is encode it as though it was a URI component (since it will be just a component, in the final imgix URL).
  • Fully-qualified, pre-encoded URLs. If the given path matches /^https?:%3A%2F%2F/, we assume its a fully-qualified, pre-encoded URL. In this case, we decode and re-encode it using decodeURIComponent and encodeURIComponent, to ensure that all characters in the string are actually encoded properly.
  • Path fragments. If the given path does not match either of the above patterns, we must assume that it's a boring path rather than a full URL. In this case, we decode and re-encode it as above, but this time we use decodeURI and encodeURI so that legal path characters such as / and @ aren't affected.

This PR also reorganizes the library's tests so they're focused on the private component methods that make up the public buildURL() method: _sanitizePath(), _buildParams(), and _signParams(). Testing these component functions separately allows us to untangle the various cases that need to be tested and gives us better confidence that all cases are being covered.

@mchatman Do you mind giving this a quick review?

@jayeb jayeb requested a review from mchatman May 24, 2017 20:44
@mchatman
Copy link

Looks great. Worked successfully with an URL that previously failed.

@jayeb jayeb merged commit 41f7057 into master May 25, 2017
@jayeb jayeb deleted the fix-24 branch May 25, 2017 15:57
@mixergtz
Copy link

mixergtz commented Jun 6, 2018

Hi @jayeb, thanks for the awesome job!

I'm wondering if there's some way to skip the double encoding you added in this PR, my use case is when you have an already encoded url like https://mysite.s3.amazonaws.com/My%20Product.jpg you'll get an extra-encoded url looking like https://mysite.s3.amazonaws.com/My%2520Product.jpg which won't be present in the server.

My current workaround is to decode the url before passing it to buildUrl

I'll be thankful to hear any thoughts from you on this matter.

@ericdeansanchez
Copy link
Contributor

🎉 This PR is included in version 0.3.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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