"Bawolff" posted a comment on MediaWiki.r111263. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/111263#c30794
Commit summary for MediaWiki.r111263: initial import of new extension EtherpadLite Bawolff's comment: <pre> The Html:rawElement was mentioned as a good way to sanitize here https://www.mediawiki.org/wiki/Security_checklist_for_developers#Output_.28API.2C_CSS.2C_JavaScript.2C_HTML.2C_XML.2C_etc..29 , so I thought, it is enough to use this method. </pre> That page was misleading (I changed it). Html::rawElement makes sure your attributes are encoded (So people don't do crap like <code><nowiki>width='foo" ><script>doEvil()...'</nowiki></code>. They don't look at the actual contents of the attribute. Hence, style attributes have to be further filtered with Sanitizer::checkCss (or Sanitizer::validateAttributes whic will do stuff for some other attributes in addition to style ) if the CSS comes from the user. Sanitizer::cleanUrl doesn't do a whole lot for security in the context of your extension (It basically just normalizes entity references to stop people from getting around spam blacklists, which to be honest I'm not even sure spam blacklists would be checked for a parser hook attribute). The most scary part is allowing users to override the pad url - embedding iframe to a potentially evil site opens up users to at the very least phising attacks (Because it's embedded into the page, looks like part of the page, evil person could set up a website asking for users passwords, etc. wfAppendUrl would probably help somewhat with security of building the url parameters, since it would prevent someone from passing a parameter to the other server other then the one's that you explicitly want them to (Of course in general, this extension is working on the assumption that the etherpad server is not evil, so the url paramter validation is not super-critical...). The best thing though would to check what all the valid values for those parameters could be, and make sure the user cannot pass in anything but those. p.s. I just noticed there is a typo: $wgEtherpadLiteDefaultHeigth Should be $wgEtherpadLiteDefaultHeight Cheers. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
