Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

auto_link sanitize output #6485

Merged
merged 1 commit into from May 25, 2012

Conversation

Projects
None yet
4 participants
Contributor

homakov commented May 25, 2012

We should sanitize it to avoid javascript:// xss.

Contributor

homakov commented May 25, 2012

wow this PR looks ok, I finally found how to switch a branch to push in gh's interface :)

Contributor

homakov commented May 25, 2012

Owner

rafaelfranca commented May 25, 2012

@homakov do you mind add tests to it?

Owner

rafaelfranca commented May 25, 2012

Also, is this applied to 3.0+ too?

Owner

tenderlove commented May 25, 2012

We shouldn't need this above 3.0. I removed it in 3.1. :-)

Aaron Patterson
http://tenderlovemaking.com/

On May 25, 2012, at 7:02 AM, Rafael Mendonça Françareply@reply.github.com wrote:

Also, is this applied to 3.0+ too?


Reply to this email directly or view it on GitHub:
#6485 (comment)

Contributor

homakov commented May 25, 2012

@rafaelfranca are you proposing to test does "sanitize" method work fine(this is only addition in commit)? I bet it does and sanitize helper is tested )

Owner

rafaelfranca commented May 25, 2012

@homakov no I proposing to test that auto_link sanitize the content.

@tenderlove I see. Thank you for explain this.

tenderlove added a commit that referenced this pull request May 25, 2012

@tenderlove tenderlove merged commit f7cf745 into rails:3-0-stable May 25, 2012

Member

arunagw commented May 25, 2012

After this merge actionpack tests are going to fail

 1) Failure:
test_auto_link(TextHelperTest) [/Users/arunagw/checkouts/rails/actionpack/test/template/text_helper_test.rb:361]:
<"<p>Link <a href=\"http://www.rubyonrails.com\" target=\"_blank\">http://www.rubyonrails.com</a></p>"> expected but was
<"<p>Link <a href=\"http://www.rubyonrails.com\">http://www.rubyonrails.com</a></p>">.

  2) Failure:
test_auto_link_other_protocols(TextHelperTest) [/Users/arunagw/checkouts/rails/actionpack/test/template/text_helper_test.rb:454]:
<"<a href=\"file:///home/username/RomeoAndJuliet.pdf\">file:///home/username/RomeoAndJuliet.pdf</a>"> expected but was
<"<a>file:///home/username/RomeoAndJuliet.pdf</a>">.

  3) Failure:
test_auto_link_should_not_sanitize_input_when_sanitize_option_is_false(TextHelperTest) [/Users/arunagw/checkouts/rails/actionpack/test/template/text_helper_test.rb:443]:
<"<a href=\"http://www.rubyonrails.com?id=1&num=2\">http://www.rubyonrails.com?id=1&num=2</a>"> expected but was
<"<a href=\"http://www.rubyonrails.com?id=1&amp;num=2\">http://www.rubyonrails.com?id=1&num=2</a>">.

  4) Failure:
test_auto_link_with_options_hash(TextHelperTest) [/Users/arunagw/checkouts/rails/actionpack/test/template/text_helper_test.rb:527]:
<"Welcome to my new blog at <a href=\"http://www.myblog.com/\" class=\"menu\" target=\"_blank\">http://www.myblog.com/</a>. Please e-mail me at <a href=\"mailto:me@email.com\" class=\"menu\" target=\"_blank\">me@email.com</a>."> expected to be == to
<"Welcome to my new blog at <a class=\"menu\" href=\"http://www.myblog.com/\">http://www.myblog.com/</a>. Please e-mail me at <a class=\"menu\" href=\"mailto:me@email.com\" target=\"_blank\">me@email.com</a>.">.

Owner

rafaelfranca commented May 25, 2012

Hmmm. This is the why I asked for tests. Seems that sanitize remove the target and escapes the & in the URL.

Owner

tenderlove commented May 25, 2012

Oops. I guess I shouldn't have merged this. @homakov can you take a look please?

Contributor

homakov commented May 25, 2012

@arunagw oh, thank you! It caused problems with raw
it is happening due to forced sanitizing. I see two ways to go - do not mark returned string as html_safe - then you can raw it or sanitize
Or, more simple way to go(and more secure by architecture) - rollback and hardcode(whitelist) possible protocols.
I'm talking about

        AUTO_LINK_RE = %r{
            (?: ([\w+.:-]+:)// | www\. )
            [^\s<]+
          }x

we will whitelist that ([\w+.:-]+:) part using https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/vendor/html-scanner/html/sanitizer.rb#L100 in rails_autolink and in 3-0-stable

@tenderlove how do you like the solution? Whitelisting is always safe option to choose in my opinion.

Owner

tenderlove commented May 25, 2012

I think whitelisting is the best bet. Please do that!

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