"Bawolff" changed the status of MediaWiki.r111263 to "fixme" and commented it. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/111263#c30787
Old Status: new > New Status: fixme Commit summary for MediaWiki.r111263: initial import of new extension EtherpadLite Bawolff's comment: Some comments: function efEtherpadLiteParser_Initialize( &$parser ) Doing the & in &$parser isn't needed anymore as far as I am aware. (OTOH, it doesn't hurt anything either) The <tt>empty</tt> function is generally discouraged in the coding conventions. $wgEtherpadLiteDefaultPadUrl = "http://www.example.com/p/";; One alternative possibility is to do <code>$wgEtherpadLiteDefaultPadUrl = null</code> instead as default, and when you're about to display the pad, check for null and yell at user to configure the extension if the variable is null. $wgEtherpadLiteShowControls = 'true'; ... I'm not really a fan of using a string true. It would be better to use a boolean true value, and then convert that to a string when making the iframe url parameters. $padId = ( !empty( $args['pad-id'] ) ) ? $args['pad-id'] : "" ; ... There's no validation of user input here. I'm not sure how much evil can be accomplished via the url parameters. Worst case scenario would be of course if etherpad had an xss vulnrability in terms of its url parameters (Of course you have to allow most input here because pads can be named everything, but should at least check for & since those separate url parameters. ( !empty( $args['pad-url'] ) ) ? $args['pad-url'] : $wgEtherpadLiteDefaultPadUrl this is kind of scary, an evil user can then embed an iframe to an arbitrary site. This could perhaps be used to do some kind of clickjacking attack against some other site, track users of a wiki, other evil stuff etc. This is also an XSS attack by doing something like <nowiki><eplite pad-url="javascript:alert(1)//"/></nowiki> + "id" => "epframe$padId", Minor issue, but user could potentially make an id that is not a valid one. (HTML has some restrictions on ids if i recall). Also what if someone embeds the same pad twice (ids must be unique)? $userName = $wgUser->getName(); No garuntee the user who parsed the page is the one viewing the page. Either need to call $parser->disableCache(), add the username process at some later stage (like use js, or possibly some hook that runs right before outputting the page). "style" => "width:$width;height:$height", CSS needs to be sanitized (There's a method somewhere in the Sanitizer for that i think). Certain browsers can be caused to execute js via certain css rules. return array( $output, 'noparse' => true, 'isHTML' => true ); That's the return value format for parser functions not parser tags. Parser tags you just return the html as a string. (There is an extended return value system for parser tags where you can specify markerType for example, but the element names of the array are different from the parser function return value) $wgExtensionCredits['other'][] Generally this would probably be considered a "hook" extension not an "other" extension. Cheers. p.s. You should perhaps coordinate with NeilK. If i recall he was interested in etherpad <-> mediawiki stuff at one point. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
