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

Add the Addressable gem to more responsibly parse uris. #337

Closed
wants to merge 2 commits into from

Conversation

dgiunta
Copy link
Contributor

@dgiunta dgiunta commented Apr 3, 2014

This pull request attempts to resolve the concerns raised in this issue: #336

In a nutshell, the issue is that the URI library—specifically the URI.escape method—does not accept all possible, and valid URIs. The particular problem I was having when I raised the issue was that I had users uploading files with file names that included square brackets.

A lot of google searching later, and I found the following:

  1. URI.escape / URI.encode are both deprecated.
  2. Documentation on the URI module is actually quite horrible.
  3. A commenter here suggested the Addressable gem as the best solution as results from that gem conform to RFC 3986, RFC 3987, and RFC 6570 (level 4), whereas URI does not.

I also found looking in the carrierwave source this pull-request and commit: https://github.com/carrierwaveuploader/carrierwave/pull/1013/files#diff-323aa4ff2c24aebdbdd5f21fb1195886R75 which attempt to solve the exact same problem.

Personally, I don't think that's the best solution as it's still using the deprecated URI.encode method.

I'm not sure how you feel about introducing another dependency for the project, but it looks like that might be the best solution.

I think that the Addressable gem might be nice to integrate throughout the gem, but rather than including that work into this pull request, I tried to localize my implementation to just this one particular place that solves this one particular problem, hence my returning the result of URI.parse, instead of converting those URI.parse's into Addressable::URI.parse's throughout.

Thoughts?

@bitflingr
Copy link

I too am running into this. I have a image resize proxy server using Dragonfly and i sometimes run into crazy uri's. I addressed them as needed, adding them to the list of things to look for. Since the upgrade to 1.0.3 I am getting 404's for a particular url.

http://images.lifescript.com/Media/9/1/B/%7B91B6CE27-CF10-476D-86C4-56ABFAF3A41E%7DMilk_article.jpg

decoded it looks like http://images.lifescript.com/Media/9/1/B/{91B6CE27-CF10-476D-86C4-56ABFAF3A41E}Milk_article.jpg

Note the '{ }'.

However in my logging i am passing the following url
http://images.lifescript.com/Media/9/1/B/%7B91B6CE27-CF10-476D-86C4-56ABFAF3A41E%7DMilk_article.jpg

When wiresharking i see a request for http://images.lifescript.com/Media/9/1/B/%257B91B6CE27-CF10-476D-86C4-56ABFAF3A41E%257DMilk_article.jpg

Which it should not need to do since '%' are totally fine.

@dgiunta
Copy link
Contributor Author

dgiunta commented Apr 6, 2014

@bitflingr I saw the same behavior happening locally as well, where properly escaped characters were getting double-escaped when coming through dragonfly's parse_uri method.

@markevans
Copy link
Owner

thanks for this and sorry for the delay.
I must admit, I was reluctant to pull in another gem dependency but addressable does seem to be better than ruby std lib (which really needs fixing!) and pretty solid and I'd rather not manually add square brackets etc. as escaping URLs actually seems like it's actually more complicated than a simple regexp substitution.
I've merged this in via cherry-pick.
@dgiunta just to confirm, since using Addressable has it ironed out all the url problems?
@bitflingr the double escaping issue should have been fixed in 1.0.4 https://github.com/markevans/dragonfly/blob/master/History.md#104-2014-04-11 though any other escaping problems are hopefully solved with this pull request

@dgiunta
Copy link
Contributor Author

dgiunta commented Jul 2, 2014

@markevans Yes, all url problems have been resolved successfully after adding the addressable gem.

@markevans
Copy link
Owner

cool it'll be part of the next release then - cheers

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.

3 participants