User talk:Redekopmark

Re: XSS extension check
You asked Johnduhart for an xss review of your extension, I noticed it and thought I'd do a bit of a full review of it: Daniel Friesen (Dantman) 01:00, 3 January 2012 (UTC)
 * Instead of having a mp3.php file you should have it installed inside of extensions/MiniMp3/MiniMp3.php instead. If your extension gets into svn this will also make things easier since you can just drop a copy of the .swf file in there as well.
 * You should avoid using $wgScriptPath inside of the description message. Especially given good extensions in svn put their messages inside of an .i18n.php file for translation where that won't exactly be available.
 * Don't use wgExtensionFunctions to register your tag, use.
 * That foreach to loop over args is completely unnecessary. You can simply use . (IMHO $params would be a better var name than $argv)
 * Drop the ?> from the end of the file, there's a good reason our code style has it omitted.
 * Don't use $wgUploadPath for the download link, you should provide a config variable that can be used to point to the proper url where that file would reside. Either that or make use of and have the file put inside of your extension folder.
 * As you asked, yes you do in fact have an XSS vulnerability inside of your extension. The bg and color parameters can be used for an xss attack. The way you use $input inside of that error message also looks potentially hazardous, though I can't quite understand what situation that $input would even have a proper value.
 * To properly escape everything you should make some changes:
 * You're best off making use of our Html:: class to build your markup instead of trying to concatenate everything. For example use  to build those 's. You can use Html::rawElement for the few things like the where you do in fact want to nest more html building methods inside of it.
 * Your error div should be built using.
 * To build those FlashVars I suggest making use of our wfArrayToCGI method it will take a clean array and turn it into a properly urlencoded string in the format you want and then passing it to Html::element will have the &'s properly html escaped, all nice and implicitly. Something like this:  (of course with more clean indentation; make sure you drop the urlencode from the   when you do this since wfArrayToCGI will do the urlencoding for you).

Since you updated the extension I thought I might follow up: Daniel Friesen (Dantman) 08:05, 4 January 2012 (UTC)
 * Don't use $wgParser when using ParserFirstCallInit, it defeats the purpose. Your callback is given a parser instance as an argument, so just add a $parser argument and call setHook on that.
 * The XSS and most of the other issues still stand.


 * ✅ move file to extensions/MiniMp3/MiniMp3.php instead.
 * ✅ You should avoid using $wgScriptPath inside of the description message.
 * ✅ Don't use wgExtensionFunctions, use.
 * ✅ That foreach to loop over args is completely unnecessary.
 * ✅ Drop the ?> from the end of the file.
 * ✅ Don't use $wgUploadPath for the download link.
 * ✅ XSS - To properly escape everything you should make some changes:
 * ✅ use our Html:: class for params
 * use Html::rawElement for the few things like the
 * ✅ use of our wfArrayToCGI method


 * updated
 * ✅ Don't use $wgParser when using ParserFirstCallInit
 * The XSS and most of the other issues still stand.
 * redekopmark 04:41, 5 January 2012 (UTC)

Good improvements. isn't particularly necessary,  is enough given there's nothing else in the string. It's good practice to never leave something unescaped when you don't have a reason to. In this case it's best not to leave $flashFile unescaped inside your tag. I suggest using Html::rawElement there (nest the rest of your pieced together html inside of the third argument and you can omit the for the closing parenthesis). Since you added a media handler you may also want to consider combining your code into some common code instead of duplicating it. $flashFile should also be "$wgExtensionAssetsPath/MiniMp3/player_mp3_mini.swf". Daniel Friesen (Dantman) 05:00, 5 January 2012 (UTC)