Page MenuHomePhabricator

Security review for mediawiki/extensions/FundraisingChart
Closed, InvalidPublic

Description

Security review request for FundraisingChart extension. Depends on bug #65834


Version: master
Severity: normal

Details

Reference
bz66805

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:23 AM
bzimport set Reference to bz66805.

Any update for this? Fundraising is hoping to use this extension for our fiscal year-end report, which will be published at the end of August. Thanks!

For some reason this was assigned to fr-tech and not Chris. That's probably a cause for delay :)

What domain is this going to be deployed on?

(In reply to Chris Steipp from comment #3)

What domain is this going to be deployed on?

collab and meta

Hi Sherah,

It's a little difficult to do a thorough review because I keep hitting bugs with the version in gerrit, but there are a couple of design issues and some of the ways you're doing things I think are likely going to lead to security issues. Let me know if you want to schedule time to talk through these.

FundraisingChart.body.php

  • Urls should pull from https, or protocol relative links so https views aren't compromised
  • Not security but..
    • Adding bootstrap.css is messing with the page styles
    • Map charts mess up the page formatting because of stray <tr> tags

resources/js/fundraising_charts.js

  • There's a bad privacy leak in that you're not validating the urls you pull in from user-controlled tags on the page. A user can add wikitext to the page like "<div class="pieArea" id="Something" data-chartdata="http://www.google.com/">12345</div>" and your code will call out to google.
    • Because of that, there a few places that you're writing out

modules/ext.fundraisingChart.datamaps/datamaps.world.js

  • addLegend() - concatting html is usually bad. Either need to demonstrate sanitization or use dom objects.
  • updatePopup() - Setting .html() is dangerous. Sanitize before passing to popupTemplate, or in the template function. You should document where the sanitization is happening either way.

I'm still working on d3. I think it should be fine, but I noticed the included version is very old. Is someone going to keep it updated in case there are security issues in the library?

(In reply to Chris Steipp from comment #5)

I'm still working on d3. I think it should be fine, but I noticed the
included version is very old. Is someone going to keep it updated in case
there are security issues in the library?

D3 looks like it's pretty sane about how it does what it does. Should be fine to use.

This, and T67834, appear stalled. Is anything more needed/expected on this ticket? We will assume not if no response is given by August 18th, 2016.

demon subscribed.

Not needed/wanted anymore, best I can tell.