Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #8864 (closed defect: wontfix)

Opened 2 years ago

Last modified 2 years ago

[PATCH] strip_links doesn't filter multi-links

Reported by: hawe Assigned to: core
Priority: high Milestone: 1.2.4
Component: ActionPack Version: edge
Severity: normal Keywords: strip_links
Cc: lifofifo

Description

strip_links doesn't filter subsequent links or links without matching close tags (</a>) like these: 

"<a href='http://www.holy-angel.com/'><a href='http://www.attacker.com/'>Test</a></a>"
"<a href='http://www.holy-angel.com/'>Test"

This can make a XSS attack work. A quick solution is to remove the opening and closing tags separately.

Attachments

text_helper_strip_links.diff (124 bytes) - added by hawe on 07/03/07 19:16:06.
text_helper_test_strip_links.diff (222 bytes) - added by hawe on 07/03/07 19:24:10.
fix_strip_links.patch (1.4 kB) - added by lifofifo on 07/03/07 22:51:32.
Use valid regular expression and make all tests pass/implement new test cases
fix_strip_links_token_ff.patch (2.1 kB) - added by lifofifo on 07/04/07 00:41:22.
Use tokenizer and handle <href="link"> tags

Change History

07/03/07 19:16:06 changed by hawe

  • attachment text_helper_strip_links.diff added.

07/03/07 19:24:10 changed by hawe

  • attachment text_helper_test_strip_links.diff added.

07/03/07 19:32:38 changed by lifofifo

You should make your patch from top level or may be actionpack/ using svn diff.

07/03/07 19:48:23 changed by lifofifo

  • priority changed from normal to high.

07/03/07 20:00:12 changed by lifofifo

  • cc set to lifofifo.

07/03/07 22:50:50 changed by lifofifo

The current changes suggested by hawe breaks existing test cases.

I've refactored suggested regular expression and have also implemented a few test cases.

07/03/07 22:51:32 changed by lifofifo

  • attachment fix_strip_links.patch added.

Use valid regular expression and make all tests pass/implement new test cases

07/04/07 00:24:21 changed by lifofifo

After playing around for a while, I found out that firefox treats 

<href="http://whatever.com" onClick="alert()">something

as a link! I guess now that we're dealing with more than one type of html tags, it's better to use html tokenizer to make things simpler and protect against any weird things like this. So I'm submitting a new patch for the same.

Thanks.

07/04/07 00:41:22 changed by lifofifo

  • attachment fix_strip_links_token_ff.patch added.

Use tokenizer and handle <href="link"> tags

07/04/07 09:13:05 changed by hawe

Thanks for your updates. That's the bad thing about blacklists, you always come up with a new hack, this one will produce a proper link: 

assert_equal "all <b>day</b> long", strip_links("<<a>a href='hello'>all <b>day</b> long<</A>/a>")

This is because of the limitation of the tokenizer to parse only correct XHTML (same thing for sanitize and strip_tags), but, yes, a regular expression would fail here, too.

Quick solution: repeatedly apply the filter on the output string in order to remove newly created links.

07/18/07 11:17:59 changed by lifofifo

  • status changed from new to closed.
  • resolution set to wontfix.

Use of whitelist plugin is suggested in cases like this. http://weblog.techno-weenie.net/2006/9/3/white-listing-plugin-for-rails