Wikimedia Bugzilla is closed!

So far, I've found two issues in the Vega library makes this extension undeployable on public wikis:

* Privacy violation with unrestricted xhr loads Anyone can put Something like <graph>{"width": 0, "height": 0, "data": [{"url": "//www.whatever.com/track.php"}] }</graph> and vega happily loads the url without user interaction.

* Stored XSS through the "filter" config. You can put arbitrary javascript in in the filter test, and it gets executed.

On a wiki where only users with interface editing rights can edit the graph config, this is fine as it doesn't do anything beyond what raw html can do. But are these issues fixable, if you're intending to deploy this on public wikis?

The new vega library is an improvement, but I think there's a flaw in how they did the domain comparison:

return vg.config.domainWhiteList.some(function(d) {
   return d === domain ||
     domain.lastIndexOf("."+d) === (domain.length - d.length - 1);
});

If "."+d doesn't exist in domain, lastIndexOf will return -1. So if d.length and domain.length are exactly the same length (but different), then -1 === -1, so the invalid domain would get through. I think you want to just take the substring of d which is the last domain.length characters, and then do a strict comparison.

thanks, good catch, fixing :)

Graph ext patch: https://gerrit.wikimedia.org/r/#/c/160369/

Upstream pull request: https://github.com/trifacta/vega/pull/217

Ok, the rest of it should be safe.

I would certainly encourage you to make the extension default to safe mode, and using the local wiki as the default whitelisted domain, so an admin has to explicitly enable insecure features. But since you're anxious to get this deployed, we can put it on wmf wikis as is, as long as we always configure the whitelist.

Chris, thanks, and the defaults have always been exactly as you describe - extension sets wgEnableGraphParserTag=false and wgGraphDataDomains=[] by default.

(In reply to Yuri Astrakhan from comment #6)
> Chris, thanks, and the defaults have always been exactly as you describe -
> extension sets wgEnableGraphParserTag=false and wgGraphDataDomains=[] by
> default.

Sorry, what I meant was in js/graph.js, you have 

if (vg.config.domainWhiteList) {
	vg.config.safeMode = true;
}

I'm not sure any js engines interpret [] == false, but something like this would feel safer to me (more fail safe):

vg.config.safeMode = true;
if ( vg.config.domainWhiteList === false ) {
	vg.config.safeMode = false;
}

Thx, please take a look at https://gerrit.wikimedia.org/r/#/c/164708/