Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 1.2.3

Bug #9585 BC break with Wikilink renderer
Submitted: 2006-12-08 17:12 UTC
From: yunosh Assigned: justinpatrin
Status: Bogus Package: Text_Wiki (version 1.2.0RC1)
PHP Version: Irrelevant OS:
Roadmaps: (Not assigned)    
Subscription  


 [2006-12-08 17:12 UTC] yunosh (Jan Schneider)
Description: ------------ The Wikilink renderer is escaping urls now which didn't happen in older Text_Wiki versions and is a BC break. Test script: --------------- http://dev.horde.org/~jan/WikiLink.phpt

Comments

 [2006-12-08 19:34 UTC] justinpatrin (Justin Patrin)
Thanks for the report but fixing XSS attacks is not a BC break. Text_Wiki should have been escaping this all along in the Xhtml renderer. Think about converting to Plaintext or Latex or PDF. If you choose one of these with pre-escaped links then you'll have invalid URLs in your output.
 [2006-12-08 21:09 UTC] yunosh (Jan Schneider)
This is a BC break. Period. Sane programmers take care of escaping user input, thus my example. This is not the job of a library, but of the application using the library. You are punishing all developers of applications using Text_Wiki (and their users) that already have taken care of security measures.
 [2006-12-08 21:20 UTC] justinpatrin (Justin Patrin)
Since Text_Wiki allows converting to multiple output formats the same input should work for all output formats. Unfortunately Text_Wiki was open to XSS attacks. It is no longer open to these attacks, as it should be. If you don't want translation you can turn it off.
 [2006-12-08 21:46 UTC] yunosh (Jan Schneider)
Analyzing the changes in the renderer I noticed that you simply overshoot the XSS protection. In Text_Wiki_Render_Xhtml_Wikilink::token(), you don't need to pass the whole built url to textEncode(). It is sufficient to pass the content that's potentially user input, like $page, $anchor and $text. $href, which is always provided by the developer, should not, or rather must not be escaped. You have to trust the developer who uses Text_Wiki that he is not passing XSS code in the configuration parameters.
 [2006-12-08 21:54 UTC] justinpatrin (Justin Patrin)
Since Text_Wiki is putting that URL in the html it is up to Text_Wiki to encode as appropriate. The developer should now have to know where their input will end up in order to escape properly. In addition, it makes no sense to force every developer who uses Text_Wiki to pre-escape values. Text_Wiki should take in the *real* URL and give you the same URL back however it needs to.
 [2006-12-08 23:12 UTC] chagenbu at php dot net (Chuck Hagenbuch)
It's still a BC break; you're saying that people who _were_ avoiding XSS attacks now have no way to write code that is secure while still _working_ with both Text_Wiki < 1.2 and with the new version or later. I'm all for closing security holes but it's these kinds of BC breaks that cause people to fork PEAR packages.
 [2006-12-08 23:37 UTC] justinpatrin (Justin Patrin)
If you had reported this bug instead of working around it, it could have been fixed earlier. Breaking a hack which worked around a bug is not a BC break. I'm sorry that your code now has to be changed. Be happy that Text_Wiki now escapes its output properly. You only have to fix this in your code once.