Last modified: 2014-08-14 18:59:10 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T68805, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 66805 - Security review for mediawiki/extensions/FundraisingChart
Security review for mediawiki/extensions/FundraisingChart
Status: NEW
Product: MediaWiki extensions
Classification: Unclassified
FundraisingChart (Other open bugs)
master
All All
: Normal normal (vote)
: ---
Assigned To: Chris Steipp
:
Depends on:
Blocks: 65834
  Show dependency treegraph
 
Reported: 2014-06-18 18:05 UTC by Sherah Smith
Modified: 2014-08-14 18:59 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Sherah Smith 2014-06-18 18:05:08 UTC
Security review request for FundraisingChart extension. Depends on bug #65834
Comment 1 Sherah Smith 2014-07-23 23:34:56 UTC
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!
Comment 2 Greg Grossmeier 2014-07-24 15:13:23 UTC
For some reason this was assigned to fr-tech and not Chris. That's probably a cause for delay :)
Comment 3 Chris Steipp 2014-07-28 17:12:54 UTC
What domain is this going to be deployed on?
Comment 4 Sherah Smith 2014-07-28 17:15:17 UTC
(In reply to Chris Steipp from comment #3)
> What domain is this going to be deployed on?

collab and meta
Comment 5 Chris Steipp 2014-08-14 01:47:21 UTC
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?
Comment 6 Chris Steipp 2014-08-14 18:59:10 UTC
(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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links