Page MenuHomePhabricator

Security review for `countCUstats.js`
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

This is a transparency report generator for Stewards/loginwiki. It accesses CU logs, parse how many CU actions have been performed by given Stewards, and present them in a wikitable format.

Description of how the tool will be used at WMF

It will generate reports for Stewards to publish the monthly report.

Dependencies

Action API?

Has this project been reviewed before?

None AFAIK

Working test environment

You'll need to be able to see CU logs on loginwiki or mimic CU logs to actually work with it.

Post-deployment

Other wishes (o/t)

Any ideas that could better the working of the script, old syntax cleanup, etc. would be certainly welcome.

Related Objects

Event Timeline

revi added a subscriber: sbassett.

Submitted per @sbassett.

MarcoAurelio renamed this task from Security Review For countCUstats.js to Security review for `countCUstats.js`.Jul 3 2019, 10:44 PM
MarcoAurelio updated the task description. (Show Details)
MarcoAurelio subscribed.

Ok, a quick review:

  1. In general, these kinds of user-scripts can become an especially ripe attack vector if they are 1) used by privileged users 2) are used by several users and loaded via one entry-point. The first case is certainly true, given the CU right required for queryCheckUserLog(). The second case isn't true so far as I'm only seeing one other user loading it, according to mwgrep. Still, something to be mindful of.
  2. getStewards() - fine, only pulling public data via the API.
  3. queryCheckUserLog() - fine, must have CU to view the CU log.
  4. appendToPage() - fine by itself, if text is trusted/sanitized. See next items below...
  5. getStatsTable() - even though we are pulling from a trusted source (mw API), it is a best practice to sanitize data at all sinks. Wrapping something like mw.html.escape() around entry.name and entry.count would be a good idea here.
  6. init() - again, even though we are pulling from a trusted source (mw API), it is a best practice to sanitize data at all sinks. Wrapping something like mw.html.escape() around username and data within the onError handler would be a good idea. totalCount, within the $.when.apply block, should be fine since it's locally-derived from numerical data and is also being passed to jquery.text().

Thanks @sbassett - Do we have any steward who understands JS and can implement the recomendations? @hoo perhaps, or can we ask a trusted individual (@Daimona maybe?) to help us?

Proposed fixed version:

1/*
2 * This script allows to count the number of times a steward has used CheckUser on a wiki.
3 * It was written in response in to [[Talk:Stewards/CheckUser_statistics_for_loginwiki#Update_required]]
4 * but can be used for other wikis too.
5 *
6 * To run the script, change the month as needed (see queryCheckUserLog() function) and run
7 * it on your browser console on Special:BlankPage.
8 *
9 * Caveats:
10 * - Only current stewards are counted. Previous stewards' stats have to be counted manually.
11 * - It assumes that the no. of CUs per steward is maximum 4000 (can be easily changed if needed).
12 * - It assumes that the max no. of stewards is 150 (also changeable)
13*/
14
15var api = new mw.Api();
16
17function getStewards() {
18 return api.get( {
19 format: 'json',
20 action: 'query',
21 list: 'globalallusers',
22 agugroup: 'steward',
23 agulimit: 150 // We probably won't have stewards more than this anytime soon
24 } );
25}
26
27function queryCheckUserLog( username ) {
28 return api.get( {
29 format: 'json',
30 list: 'checkuserlog',
31 culuser: username,
32 cullimit: 4000, // eh.. hopefully no one does more than this in a month
33 culdir: 'newer',
34 // Please don't change the format unless you know what you're doing or it might break
35 culfrom: new Date( '1 January 2016 UTC' ).toISOString(), // Start date
36 culto: new Date( '1 February 2016 UTC' ).toISOString() // End date
37 } );
38}
39
40function appendToPage( text ) {
41 // Note: text must be trusted!
42 $( '#mw-content-text' ).append( text + '<br/>' );
43}
44
45function getStatsTable( entries ) {
46 var html =
47 '<table class="wikitable sortable">\n'
48 + '<tr>'
49 + '<th> Username </th>'
50 + '<th> Count </th>'
51 + '</tr>\n';
52
53 $.each( entries, function( i, entry ) {
54 html += '<tr>'
55 + '<td>' + mw.html.escape( entry.name ) + '</td>'
56 + '<td>' + mw.html.escape( entry.count ) + '</td>'
57 + '</tr>\n';
58 } );
59
60 html += '</table>';
61
62 return html;
63}
64
65function init() {
66 getStewards().done( function( data ) {
67 var stewards = data.query.globalallusers,
68 stewardCount = stewards.length,
69 totalCount = 0,
70 stats = [];
71 promises = []; // promise for all query api requests
72
73 appendToPage( '<span id="cu-status">Querying... Please wait.</span>' );
74 var $status = $( '#cu-status' );
75
76 $.each( stewards, function( i, info ) {
77 var username = info.name;
78 var queryPromise = queryCheckUserLog( username );
79 promises.push( queryPromise );
80 queryPromise.done( function( data ) {
81 if ( !data.query ) {
82 onError( data );
83 }
84
85 var cuCount = data.query.checkuserlog.entries.length;
86 stats.push( {
87 name: username,
88 count: cuCount
89 } );
90 totalCount += cuCount;
91
92 } ).fail( function( data ) {
93 onError( data );
94 } );
95
96 function onError( data ) {
97 $status.text( 'Error occured in querying.' );
98 throw 'Error while querying ' + mw.html.escape( username ) + ' details: ' + JSON.stringify( data );
99 }
100
101 } );
102
103 $.when.apply( $, promises ).done( function() {
104 // Output table once we've the all stats data needed
105 $status.text( 'Total: ' + totalCount);
106 appendToPage( getStatsTable( stats ) );
107 mw.loader.using( 'jquery.tablesorter', function() {
108 $( 'table' ).tablesorter();
109 } );
110 });
111
112 } );
113}
114
115init();

Proposed fixed version: P8739

Diff for the patch available.

The script does not work. Exception from the console:

URL: https://login.wikimedia.org/wiki/Special:BlankPage?debug=true

Uncaught TypeError: s.replace is not a function
    at Object.escape (mediawiki.base.js?528b7:525)
    at Object.<anonymous> (<anonymous>:56:32)
    at Function.each (jquery.js?6fb03:355)
    at getStatsTable (<anonymous>:53:7)
    at Array.<anonymous> (<anonymous>:106:27)
    at fire (jquery.js?6fb03:3269)
    at Object.fireWith [as resolveWith] (jquery.js?6fb03:3399)
    at jquery.js?6fb03:3782
    at fire (jquery.js?6fb03:3269)
    at Object.fireWith [as resolveWith] (jquery.js?6fb03:3399)

@MarcoAurelio - weird, it looks like javascript thinks some instances of username and/or cuCount are objects as opposed to strings. Clearly getStatsTable() is expecting them to always be strings. Here's a fix for getStatsTable() I came up with, tested here. It type-checks the data and then JSON.stringify()'s any unexpected data types.

function getStatsTable( entries ) {
	var html =
        '<table class="wikitable sortable">\n'
        + '<tr>'
        + '<th> Username </th>'
        + '<th> Count </th>'
        + '</tr>\n';

    $.each( entries, function( i, entry ) {
    	var name = (typeof entry.name == "string") ? entry.name : JSON.stringify(entry.name);
        var count = (typeof entry.count == "string") ? parseInt(entry.count).toString() :
            (typeof entry.count == "number") ? entry.count.toString() : JSON.stringify(entry.count);
    	
        html += '<tr>'
            + '<td>' + mw.html.escape( name ) + '</td>'
            + '<td>' + mw.html.escape( count ) + '</td>'
            + '</tr>\n';
    } );

    html += '</table>';

    return html;
}

Final version, with authorship attributed to all contributors: https://meta.wikimedia.org/wiki/User:MarcoAurelio/SVU.js

I might also port a copy to my GitHub account as well.

Thanks, @MarcoAurelio (and everyone else). Resolving this for now.