Fix overly aggressive remote image url escaping in specific instances #2457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
courtsimas
commented
Mar 14, 2020
| @@ -40,7 +40,9 @@ def download(url, remote_headers = {}) | |||
| def process_uri(uri) | |||
| uri_parts = uri.split('?') | |||
| encoded_uri = Addressable::URI.parse(uri_parts.shift).normalize.to_s | |||
| encoded_uri << '?' << Addressable::URI.encode(uri_parts.join('?')).gsub('%5B', '[').gsub('%5D', ']') if uri_parts.any? | |||
| encoded_uri << '?' << Addressable::URI.encode(uri_parts.join('?')). | |||
| gsub('%5B', '[').gsub('%253A%252F%252F', '%3A%2F%2F'). | |||
There was a problem hiding this comment.
This is specifically targeting for the :// portion of the https:// or http:// or some-protocol:// and removing the double escape. This, coupled with the change in the next line, is what fixes the issue.
|
Also related to #2473, but different |
|
Ping. Anyone using Vimeo or imgix for mashups (overlays) defining the secondary image in a query param can’t use carrierwave in this manner. |
update from upstream repo
mshibuya
pushed a commit
that referenced
this pull request
Jan 17, 2021
|
@mshibuya since your commit doesn't actually seem to address the issue my PR addressed (albeit, maybe mine wasn't the best approach, but it worked), what's an easy way of overriding the |
|
Dude I apologize. You’re right. Thanks man. |
This was referenced Mar 11, 2021
joemsak
pushed a commit
to Kadenze/carrierwave
that referenced
this pull request
Mar 27, 2021
joemsak
pushed a commit
to Kadenze/carrierwave
that referenced
this pull request
Mar 27, 2021
Fixes carrierwaveuploader#2456, Closes carrierwaveuploader#2457, Fixes carrierwaveuploader#2472, Closes carrierwaveuploader#2473, Fixes carrierwaveuploader#2505, Fixes carrierwaveuploader#2517, Closes carrierwaveuploader#2518
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2456 - which is an issue where in specific instances, when the remote download is trying to parse and escape the remote absolute image url, it can improperly escape some characters that are actually needed in order for the image/download to work - specifically when the image source is actually in the query string, and not in the base portion of the path.
You'll see this type of pattern semi-often; where the image path/source is actually defined in the query parameter in on-the-fly overlays/mashups of images (which imgix supports) and in the way Vimeo's thumbnail images work as well (among others).
The related issue at #2456 has more information and an example.