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

Inconsistency between #image_tag and #link_to in escaping URIs #2016

Closed
ujifgc opened this Issue Apr 1, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@ujifgc
Member

ujifgc commented Apr 1, 2016

/padrino-framework$ bundle console
2.1.0 :001 > include Padrino::Helpers::TagHelpers
2.1.0 :002 > include Padrino::Helpers::AssetTagHelpers
2.1.0 :003 > include Padrino::Helpers::OutputHelpers
2.1.0 :004 > puts(image_tag 'image name.jpeg')
<img src="/images/image%20name.jpeg?1459513767" />
2.1.0 :005 > puts(link_to 'image title', 'image name.jpeg')
<a href="image name.jpeg">image title</a>

As you can see, #image_tag escapes the space char in image URI while #link_to does not.

Per rfc3986 space cannot be a part of proper URI.

Fixing this for #link_to will break applications that manually encode URIs to conform with the RFC and W3C standards.

Breaking this for #image_tag will not break applications but will introduce non-valid image URIs.

@ujifgc ujifgc added this to the 0.14.0 milestone Apr 1, 2016

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Apr 28, 2016

Member

Hey, @padrino/core-members do you have an opinion on this? I would like to fix it one way or another in 0.14 and have a warning on the behavior change in the next 0.13.

Member

ujifgc commented Apr 28, 2016

Hey, @padrino/core-members do you have an opinion on this? I would like to fix it one way or another in 0.14 and have a warning on the behavior change in the next 0.13.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 1, 2016

Member

@ujifgc My 2 cents: I think fixing this for link_to is the best option for 0.14 as you said with warnings about this on next release.

Member

nesquena commented May 1, 2016

@ujifgc My 2 cents: I think fixing this for link_to is the best option for 0.14 as you said with warnings about this on next release.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 2, 2016

Member

Do you think we can use SafeBuffer#html_safe flag to allow crafty users to sneak in manually escaped URIs if they want to? I feel like there's gonna be some apps that fixed this manually.

Member

ujifgc commented May 2, 2016

Do you think we can use SafeBuffer#html_safe flag to allow crafty users to sneak in manually escaped URIs if they want to? I feel like there's gonna be some apps that fixed this manually.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jun 4, 2016

Member

I implemented a proper link escaper for modern internet document links based on https://github.com/ruby-rdf/rdf gem: https://github.com/padrino/padrino-framework/blob/link-escaper/padrino-support/lib/padrino-support/utils/link_escaper.rb

The thing is, there is virtually no difference between this and just doing String#gsub(' ', '%20') since modern Unicode IRIs are so liberal and cases when you are trying to escape a binary string in a document link are very fringe.

The test examples are here: https://github.com/padrino/padrino-framework/blob/link-escaper/padrino-support/test/test_link_escaper.rb

I'm tending to just scrap this escaper and use simple gsub. It covers 99.9% of common IRI syntax violations (space) and does not invade pre-encoded links.

Member

ujifgc commented Jun 4, 2016

I implemented a proper link escaper for modern internet document links based on https://github.com/ruby-rdf/rdf gem: https://github.com/padrino/padrino-framework/blob/link-escaper/padrino-support/lib/padrino-support/utils/link_escaper.rb

The thing is, there is virtually no difference between this and just doing String#gsub(' ', '%20') since modern Unicode IRIs are so liberal and cases when you are trying to escape a binary string in a document link are very fringe.

The test examples are here: https://github.com/padrino/padrino-framework/blob/link-escaper/padrino-support/test/test_link_escaper.rb

I'm tending to just scrap this escaper and use simple gsub. It covers 99.9% of common IRI syntax violations (space) and does not invade pre-encoded links.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 5, 2016

Member

I tend to agree, better to keep it simpler. @namusyaka any thoughts?

Member

nesquena commented Jun 5, 2016

I tend to agree, better to keep it simpler. @namusyaka any thoughts?

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Jun 6, 2016

Member

Great, I would use the feature in Padrino

Member

namusyaka commented Jun 6, 2016

Great, I would use the feature in Padrino

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