"Bawolff" posted a comment on MediaWiki.r111263. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/111263#c30861
Commit summary for MediaWiki.r111263: initial import of new extension EtherpadLite Bawolff's comment: You're getting closer, but there's still some issues. (From r111313) + + $sanitizedAttributes = Sanitizer::validateAttributes( $args, array ( "width", "height", "id", "src" ) ); You really shouldn't use validateAttributes for the individual width and height elements, unless you put them on an iframe as the old-style indvidual width and height attributes. (This is because the html width attribute is interpreted differently then the style attribute by web browsers, so different things are evil (or really, its only possible to do evil things with the style attribute...)). If you're going to use validateAttributes, run it on the fully constructed array of attributes ( <tt>$iframeAttributes</tt> ) so the Sanitizer see's exactly what attributes you're going to pass to Html::rawElement. Some other minor issues: $noColors = ! ( wfEtherpadLiteStringFromTestedBoolean( $args['show-colors'], $wgEtherpadLiteShowAuthorColors ) ); The ! mark operator working on a string result of wfEtherpadLiteStringFromTestedBoolean might do different things then what you expect. (Since <code>(!"false") === false</code> $args['id'] = ( isset( $args['id'] ) ) ? $args['id'] : ""; Any particular reason why you set an id attribute at all (instead of say a class attribute that could be non-unique and the same for all the iframes). Getting users to ensure the id is always unique is probably not ideal I'm still very nervous about allowing just any website as a source. The potential bad is as following (Even with nowiki, CR messes with my urls... pretend that is not using wiki external url syntax): <nowiki><eplite src="http://example.com/website-that-tells-user-they-have-been-logged-out-and-asks-for-password"; width="200px;border:none;overflow:hidden"/></nowiki> It's a lot less convincing if width is validated fully (You might want to consider also stripping semi-colons from width in addition to Sanitizer related stuff), but still, that could easily trick gullible users to revealing there password to some third party. If you really want to support multiple etherpad sites, perhaps the best way would be to have another config variable that has a list of alternative allowed etherpad websites in addition to the default. Last of all, some of the checks for various args (introduced in r111313) need to use isset to prevent <code><b>Notice</b>: Undefined index: show-colors in <b>/var/www/w/extensions/EtherpadLite/EtherpadLite.php</b> on line <b>116</b></code>. Cheers. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
