ALL POSTSAjaxTables
<p>So there are some JS functions for loading a page into a table on demand and displaying it at WoWWiki that I'm not sure who the author is (but pretty sure it's Celess22) that I'd like to port over to dev wiki, but maybe someone could look at it before I do.
</p><p>Here are the relevant pages:
</p>
(Edited by Fandyllic)
<div class="quote"><i>Fandyllic wrote:
Exactly why I wanted someone to look at it. As for the vulnerability aspect, it would be more dangerous if the code could be injected without affecting change history, so I'm not super worried. <p>Any good alternatives? I didn't see anything on dev. </p> </i></div>
<p>It can be injected simply by adding a changing the underlying code of a template and inserting the <script></script> tags to it. It could be written in such a way that the page appears to be the same. </p><p>I'm actually not exactly sure what functionality you'd like that differs from the mw.collapsible class. Do you just want to hide the [show] [hide] links, and make the table clickable? </p><p>I think we can probably clean up the code to make it usable before adding it here, especially since we have an active volunteer dev here who can always give advice to remove any security issues with it. </p><p>P.S. This wiki is (as far as I know) also part of the security code-review Javascript A/B test. So we can ultimately always get staff to conduct a security review of the code. </p>
Exactly why I wanted someone to look at it. As for the vulnerability aspect, it would be more dangerous if the code could be injected without affecting change history, so I'm not super worried. <p>Any good alternatives? I didn't see anything on dev. </p> </i></div>
<p>It can be injected simply by adding a changing the underlying code of a template and inserting the <script></script> tags to it. It could be written in such a way that the page appears to be the same. </p><p>I'm actually not exactly sure what functionality you'd like that differs from the mw.collapsible class. Do you just want to hide the [show] [hide] links, and make the table clickable? </p><p>I think we can probably clean up the code to make it usable before adding it here, especially since we have an active volunteer dev here who can always give advice to remove any security issues with it. </p><p>P.S. This wiki is (as far as I know) also part of the security code-review Javascript A/B test. So we can ultimately always get staff to conduct a security review of the code. </p>
(Edited by Dessamator)
<p>I had a quick go at clean up, chnaging the code style slightly and running it through jshint. One odd thing I spotted was it seems to have an external dependency somewhere (see
external dependency?), so that needs to be resolved before moving it here.
</p><p>I'm a little confused what the issue is with the performance - there's nothing particularly bad, nor is it very long. Is there any chance of the author being able to expand on what the concerns might be if they're still around? If the concern is due to it all being imported I use a complicated lazy loading setup on RS Wiki to only load what's needed, but that's a reaction to having 20-30 imported scripts,s some of them rather large, so it's not something that's necessary for every wiki. It's about being sensible in what you are loading.
</p><p>I have no idea what the security issues are, but I haven't looked too closely at what's going on and I haven't checked them template at all.
</p>
/*jshint jquery:true, browser:true, devel:true, camelcase:true, curly:false, undef:true, bitwise:true, eqeqeq:true, forin:true, immed:true, latedef:true, newcap:true, noarg:true, unused:true, regexp:true, strict:true, trailing:true, maxcomplexity:10 */
//FIXME: this is absurdly expensive atm for all the pages not used on
function wwAjaxTables() {
// patching in changes to table sorting and alt rows
function ajaxTableInit() {
var ts = window.ts = window.ts || {};
ts.ahClass = /class="ajaxHide"/gim;
ts.crlf = /\r\n/g;
ts.image_path = '';
ts.image_none = '';
ts.makeSortable = function (table) {
var $table = $(table),
firstRow;
if ($table.find("tr").length) {
firstRow = !!$table.find("th").length ?
$table.find("tr:has(th)").eq(0) :
$table.find("tr").eq(0);
}
if (!firstRow) {
return;
}
var ts = window.ts;
firstRow.children(":not('.unsortable')")
.append(
' ' +
'<a href="javascript:;" class="sortheader" onclick="ts_resortTable(this); return false;">' +
'<span class="sortarrow">' +
'<img src="' + ts.image_path + ts.image_none + '" alt="↓"/>' +
'</span>' +
'</a>'
);
};
}
// attempts to 'late bind' all the mw, wikia, and ww infrastructure
// would have had at page load time
function getTableData(tablePage, tableNum) {
$("body")
.bind("ajaxSend", function () {
$(this).css("cursor", "wait");
})
.bind("ajaxComplete", function () {
$(this).css("cursor", "auto");
});
$.get('http://' + location.hostname + '/' + tablePage + '?action=render', function (data) {
if (data) {
var ts = window.ts;
data = data
.replace(ts.crlf, "")
.replace(ts.ahClass, 'class="ajaxHide-active"')
.replace('class="darktable"', "");
$("#ajaxTable" + tableNum).find("td").eq(0)
.html(data)
.find("table.sortable").each(function () {
ts.makeSortable($(this));
});
$("#stl" + tableNum).html(
'[<a href="/' + tablePage + '?action=edit">edit</a>] ' +
'[<a href="javascript:;" id="htl' + tableNum + '" onClick="hideTable(' + tableNum + ');">hide</a>]'
);
// external dependency?
ttMouseOver(0);
}
});
}
function hideTable(tableNum) {
$("#ajaxTable" + tableNum).find("tr").eq(1).hide();
$("#htl" + tableNum).click(function () {
showTable(tableNum);
});
$("#htl" + tableNum).text("show");
}
function showTable(tableNum) {
$("#ajaxTable" + tableNum).find("tr").eq(1).show();
$("#htl" + tableNum).click(function () {
hideTable(tableNum);
});
$("#htl" + tableNum).text("hide");
}
function loadTableData(tableNum) {
var thisTable = document.getElementById("ajaxTable" + tableNum);
var loadPage = thisTable.className.substring(thisTable.className.indexOf("targetPage-") + 11);
getTableData(loadPage, tableNum);
}
function addAjaxDisplayLink() {
$("#WikiaArticle table.ajax").each(function (i) {
$(this).attr("id", "ajaxTable" + i);
$(this).find("td").eq(1).parent().hide();
$(this).find("td").eq(0).parent().show();
if (this.getElementsByTagName("th").length) {
this.getElementsByTagName("th")[0].innerHTML = '<span style="float:right;" id="stl' + i + '"></span>' + this.getElementsByTagName("th")[0].innerHTML;
}
if ($(this).find("td").eq(0).hasClass("showLinkHere")) {
$(this).find("td").eq(0).html($(this).find("td").eq(0).html().replace("[link]", '<a href="javascript:;" onClick="window.ts.loadTableData(' + i + ')">').replace("[/link]", "</a>"));
} else {
$("#stl" + i).html('[<a href="javascript:;" onClick="window.ts.loadTableData(' + i + ')">show data</a>]');
}
});
}
ajaxTableInit();
window.ts.loadTableData = loadTableData;
addAjaxDisplayLink();
}
// needs to represent the 'bare minimum' needed to evaluate if needs to load
function wwAjaxTablesInit() {
if ($("table.ajax").length) {
wwAjaxTables();
}
}
$(wwAjaxTablesInit);
(Edited by Cqm)
<p>Oh, and as an addendum to the JS review thing, it's only set up for MediaWiki pages afaik. There's talk of moving certain JS to that ns, e.g. FastDelete, AjaxRC, or anything fully protected right now, but I'm not sure what the overall design is as far as the JS library, that's been discussed in various places, or the codeeditor protected scripts goes.
</p>
(Edited by Cqm)
<p>I didn't read through it completely, but the part that sparked my concern was here:
</p>
<div dir="ltr" class="mw-geshi mw-content-ltr"><div class="javascript source-javascript">
$.<span class="kw1">get</span><span class="br0">(</span><span class="st0">'http://'</span> <span class="sy0">+</span> location.<span class="me1">hostname</span> <span class="sy0">+</span> <span class="st0">'/'</span> <span class="sy0">+</span> tablePage <span class="sy0">+</span> <span class="st0">'?action=render'</span><span class="sy0">,</span> <span class="kw1">function</span> <span class="br0">(</span>data<span class="br0">)</span> <span class="br0">{</span>
<span class="kw1">if</span> <span class="br0">(</span>data<span class="br0">)</span> <span class="br0">{</span>
<span class="kw1">var</span> ts <span class="sy0">=</span> window.<span class="me1">ts</span><span class="sy0">;</span>
data <span class="sy0">=</span> data.<span class="me1">replace</span><span class="br0">(</span>ts.<span class="me1">crlf</span><span class="sy0">,</span> <span class="st0">""</span><span class="br0">)</span>.<span class="me1">replace</span><span class="br0">(</span>ts.<span class="me1">ahClass</span><span class="sy0">,</span> <span class="st0">'class="ajaxHide-active"'</span><span class="br0">)</span>.<span class="me1">replace</span><span class="br0">(</span><span class="st0">'class="darktable"'</span><span class="sy0">,</span> <span class="st0">""</span><span class="br0">)</span><span class="sy0">;</span>
<span class="co1">//SECURITY ISSUE???</span>
$<span class="br0">(</span><span class="st0">"#ajaxTable"</span> <span class="sy0">+</span> tableNum<span class="br0">)</span>.<span class="me1">find</span><span class="br0">(</span><span class="st0">"td"</span><span class="br0">)</span>.<span class="me1">eq</span><span class="br0">(</span><span class="nu0">0</span><span class="br0">)</span>.<span class="me1">html</span><span class="br0">(</span>data<span class="br0">)</span><span class="sy0">;</span></div></div>
<p>I see that you rewrote it to reduce the clutter. But the author didn't check any of the variables being passed nor did they sanitize the "data" before simply dumping it to the page.
</p><p>The external dependency is here (http://wowwiki.wikia.com/wiki/MediaWiki:Common.js):
</p>
// check to see if it is active then do it
function ttMouseOver(foo) {
if (tooltipsOn && getCookie("wiki-tiploader") != "no") {
$("#WikiaArticle").mouseover(hideTip);
$("#WikiaArticle").append('<div id="tfb" class="htt"></div><div id="templatetfb" class="htt"><div>');
$tfb = $("#tfb");
$ttfb = $("#templatetfb");
$htt = $("#tfb,#templatetfb");
if (foo == 1) {
$("#WikiaArticle span.ajaxttlink").each(ttBind);
}
$("#WikiaArticle span.tttemplatelink").mouseover(showTemplateTip).mouseout(hideTemplateTip).mousemove(moveTip);
}
}
<p>It seems to show tooltips on mouseover, and also for some strange reason gets the cookie maybe so someone can opt-out of tooltips?
</p><p>That part of the code might not be necessary for this script, at least not in its current implementation.
</p>
(Edited by Dessamator)
The data is coming from MediaWiki which automatically sanitizes such things to my knowledge.
(Edited by Shining-Armor)
<div class="quote"><i>Shining-Armor wrote:
The data is coming from MediaWiki which automatically sanitizes such things to my knowledge.</i></div>
<p>That's possible. Although it is a bad practice to simple pass on information obtained through an ajax call without double checking if that's really what you want or need. </p>
The data is coming from MediaWiki which automatically sanitizes such things to my knowledge.</i></div>
<p>That's possible. Although it is a bad practice to simple pass on information obtained through an ajax call without double checking if that's really what you want or need. </p>
(Edited by Dessamator)
<div class="quote"><i>
<p>Dessamator wrote:
</p>
<div class="quote">Shining-Armor wrote:
The data is coming from MediaWiki which automatically sanitizes such things to my knowledge.</i></div>
<p>That's possible. Although it is a bad practice to simple pass on information obtained through an ajax call without double checking if that's really what you want or need. </p> </div> <p>It appears that there is a standard way to do it so double checking isn't much needed. </p>
The data is coming from MediaWiki which automatically sanitizes such things to my knowledge.</i></div>
<p>That's possible. Although it is a bad practice to simple pass on information obtained through an ajax call without double checking if that's really what you want or need. </p> </div> <p>It appears that there is a standard way to do it so double checking isn't much needed. </p>
(Edited by Shining-Armor)
Thanks for all the review. I will see if I can find out the external dependency and apply recommended fixes before I move it over.
(Edited by Fandyllic)
<p>@Dessamator
data in this context is completely parsed and sanitised by the MediaWiki parser which is more or less bullet proof in terms of security. I haven't seen any security patches to it in recent months in any of the releases despite an external security review and specialised security team at Wikimedia. Anything that was major enough would likely be backported anyway even though it's for different version. I'm not sure how many changes have been made to it since 1.19 was released, but given how archaic it still is and recent work on Parsoid I wouldn't be surprised if it's more or less the same as the current version of core.
</p><p>There's much point in checking the result either as due to how it's been requested it'll just throw an error. It's a slightly dirty way of obtaining the HTML of a page, but it's fairly common and pretty well tested nonetheless.
</p>
(Edited by Cqm)
<div class="quote"><i>Cqm wrote:
@Dessamator
<p>That may be true, but it is better to be safe than sorry. According to MediaWiki's security guidelines [1] : </p><p></p><blockquote>Any content that MediaWiki generates can be a vector for XSS attacks.</blockquote> <p>Although it might not a problem for wikia since all pages are basically text, wikimedia's newer version has different content models for pages, and different rules apply to them. </p><p>Anyway, I do agree that it is a small problem, and once we remove the tooltip code and cookie tracker stuff from the script, it is probably good enough to store here. </p>
@Dessamator
data in this context is completely parsed and sanitised by the MediaWiki parser which is more or less bullet proof in terms of security. I haven't seen any security patches to it in recent months in any of the releases despite an external security review and specialised security team at Wikimedia. Anything that was major enough would likely be backported anyway even though it's for different version. I'm not sure how many changes have been made to it since 1.19 was released, but given how archaic it still is and recent work on Parsoid I wouldn't be surprised if it's more or less the same as the current version of core.
<p>There's much point in checking the result either as due to how it's been requested it'll just throw an error. It's a slightly dirty way of obtaining the HTML of a page, but it's fairly common and pretty well tested nonetheless.
</p>
</i></div><p>That may be true, but it is better to be safe than sorry. According to MediaWiki's security guidelines [1] : </p><p></p><blockquote>Any content that MediaWiki generates can be a vector for XSS attacks.</blockquote> <p>Although it might not a problem for wikia since all pages are basically text, wikimedia's newer version has different content models for pages, and different rules apply to them. </p><p>Anyway, I do agree that it is a small problem, and once we remove the tooltip code and cookie tracker stuff from the script, it is probably good enough to store here. </p>
(Edited by Dessamator)