3,307
pages
ALL POSTSBadge script 
<p>Hello everyone!
</p><p>I've written script, which is an alternative of custom achievements (except automating process). It allows to create new badges and give them to users. All badges are on their own module in rail, which is showing on user page and message wall.
</p><p>Why am I asking for help with reviewing? Script's config is out of .js page - I'm keeping all data in json format on Project:Medal page, because all editing like add badge to user or create new badge will not be passed through the JS review and we're not wasting our time for waiting. But script can have security holes - that's what I'm asking you to check. There is my script - User:Kopcap94/badge.js. Also you can check out it on my sandbox - w:c:ru.siegenax.
</p><p>Thanks in advance!
</p>
(Edited by administrators)
<p>I noticed that nobody reviewed the script, and so you've published it. But there a couple of possible security exploits:
</p>
- You set the data without sanitizing it:
var imgLink = $that.find('.MedalListLinkImage').val();
var titleNew = $that.find('.MedalListTitle').val();
var nameNew = $that.find('.MedalListName').val();
var prevName = $that.find('.MedalListName').attr('data-prev');
$that.find('.MedalImagePreview img')
.attr('src', imgLink)
.attr('title', titleNew)
.attr('data-prev', nameNew);
<p>Read through these to get acquainted with mediawiki best practices:
</p>
<p>Given these attack vectors I highly doubt Wikia will allow this code to run in any wikia.
</p>
(Edited by Dessamator)
1
<p>You can't set something like 'onclick', 'onload', 'onsomething' via .attr(). That's why I've used this instead of
</p>
$('<div class="MedalImagePreview" />').append('<img src="' + imgLink + '" />');
<p>Moreover, getting data via .val() will give me a string that can't do anything bad. BUT .attr() can be dangerous if we're using it for 'href'. So I've solve this problem via
</p>
mw.config.get('wgServer') + '/wiki/' + MedalSettings.module_info
(Edited by Kopcap94)
<div class="quote"><i>Kopcap94 wrote:
You can't set something like 'onclick', 'onload', 'onsomething' via .attr(). That's why I've used this instead of
<p>See : </p> <p>Well, that's true for the input but not the output. Basically you validate the input and escape the output. From a brief look at your script it seems that the attr() and prop() are being used to espace. But they are all over the place, it might make more sense to use a function for that purpose especially considering that you're extracting text from an external source, and one small overlooked mistake may allow an exploit. </p><p>You might consider using this library for those purposes. </p>
You can't set something like 'onclick', 'onload', 'onsomething' via .attr(). That's why I've used this instead of
$('<div class="MedalImagePreview" />').append('<img src="' + imgLink + '" />');
<p>Moreover, getting data via .val() will give me a string that can't do anything bad. BUT .attr() can be dangerous if we're using it for 'href'. So I've solve this problem via
</p>
mw.config.get('wgServer') + '/wiki/' + MedalSettings.module_info
</i></div><p>See : </p> <p>Well, that's true for the input but not the output. Basically you validate the input and escape the output. From a brief look at your script it seems that the attr() and prop() are being used to espace. But they are all over the place, it might make more sense to use a function for that purpose especially considering that you're extracting text from an external source, and one small overlooked mistake may allow an exploit. </p><p>You might consider using this library for those purposes. </p>
(Edited by Dessamator)
1
<p>I know about mw.html.escape function, but I've got some problems with it. About to create specific function to control all 'escapes' - good idea but here can be some problems with making it suitable for all cases. I'd prefer to stay at like it's now since I've covered all attr actions.
</p>
(Edited by Kopcap94)
2
<p>
.attr() isn't for event based attributes, that's what .on() is for, e.g. .on('click', ...), or it's shorthand version, .click().
</p><p>To re-iterate Dessamator's point, you should always escape user input before outputting it, or validate it if there's a set of pre-specified values is can take. .text() is suitable for this, .attr() is not and has never claimed to be.
</p>
(Edited by Cqm)
1
<p>Ah, ok, I will fix it.
</p>
(Edited by Kopcap94)
0
nice
(Edited by Jerricks)