<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>

<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);

<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>

<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>

The data is coming from MediaWiki which automatically sanitizes such things to my knowledge.

<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>

<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>

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.

<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>

<div class="quote"><i>Cqm wrote:
@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>