Wikimedia Bugzilla is closed!

Hey guys, I took an initial look through this. Other than following the style guide (I think someone pointed that out somewhere else too), and being updated to prevent the php warnings, just a few general comments:

* Overall the .js really needs to follow the recommendations in https://www.mediawiki.org/wiki/DOM-based_XSS. Since the .js is only loaded on that one special page, it's probably not exploitable as is, but it would be much safer to avoid innerHTML, etc.
* Follow https://www.mediawiki.org/wiki/Security_for_developers, especially "Escape as close to the output as possible, so that the reviewer can easily verify that it was done."
** Using the Html and Xml classes to build the html will make it easier to see that the code is secure.
* You have a lot of functions that pass around raw html, and there aren't comments saying what when parameters are expected to be escaped already, vs the function handling the escaping. Please try to document that.

https://gerrit.wikimedia.org/r/#/c/161807/ Addresses point 1
More yet to come.

Chris,
Can you explain the points you made regarding escaping? This extension really just outputs data from the DB with no user input except a few options which should be validated by the FormOptions. I don't understand why is their any need to escape the HTML.

Most of the code here is borrowed (and restructured) from core MW classes like ChangesListSpecialPage, SpecialRecentChanges and similar ones so if you could provide examples from them it would be helpful.

Hi Nischay,

Let me try to illustrate this with an examples.

SpecialRecentActivityFeed.php: line 280 has

$changeLine = $this->makeChangesLine( $list, $rc, $order );
if ( $changeLine !== false ) {
	$rclistOutput .= "<div style=\"margin-left:20px;\">$changeLine </div>";
}

Looking at just this, it's hard to tell if this is safe, or what you intended to do for escaping.

To show your escaping intent, a better way to write this would be,

$rclistOutput .= Html::rawElement( 'div', array( 'style' => 'margin-left:20px;' ), $rclistOutput );

so it's clear to future developers that $rclistOutput is intended to be safe. Additionally, anyone wanting to add another div attributes later won't be tempted to put an unescaped value directly into the html.

When reviewing this line, I'll wonder if $rclistOutput is correctly escaped, so to my last point, makeChangesLine should be commented with something like:
@returns string An HTML snippet safe for output

That way it's easy for me to check that function to see that it produces a safe html string, and later when someone edits the function, they know the expectation is that the function is handling the escaping.

Does that make sense?

> Most of the code here is borrowed (and restructured) from core MW classes
> like ChangesListSpecialPage, SpecialRecentChanges and similar ones so if you
> could provide examples from them it would be helpful.

I haven't tracked down which pieces you brought in, but in general, if you can call or inherit from core functions/classes, that makes everything easier for long term maintenance.