Last modified: 2014-07-17 21:44:43 UTC
https://www.mediawiki.org/wiki/Extension:Petition
From bug 65849 comment 4: "Greg: We don't have a particular date (lots of other variables) but it would be good to have the extension deployed by the middle of June."
At an architectural level, there's a couple of things that concern me: * It seems like a violation of least privilege / separation of duty that those who can view results always get 100% of them, and have to do their own analysis to figure out which petition was signed. * If we get hit with "spam" (obviously not visible to the public, so low value to the spamer.. but a user could easily write javascript to submit the form 10M times), there's no way to delete it other than deleting rows in the DB. That seems like it will come back to bite us. * Similarly, if we notice abuse, the extension doesn't respect user blocks. * The extension doesn't integrate with abusefilter / spam blacklist or Checkuser. The spam ones, again, we probably don't need. If someone starts submitting death threats in the petition comments, then we will want Checkuser integration. Some specific implementation things that should get fixed: * Petition::petitionRender() - $htmlOut .= wfMessage('petition-num-signatures', $numberOfSignatures)->text(); should be escaped or parsed. Since it's coming from a count in the db, there's no xss in this code, but it's more likely to prevent in the future if it does that now. * Make sure when you export to csv, values starting with '=' should get escaped (most spreadsheet applications will automatically interpret it as an equation and run). * Since you don't set any caching headers, by default the CSV shouldn't get cached, but you may want to be explicit and say it shouldn't be cached. * (performance) Trust Aaron's input over mine, but it seems like you would want to count(pt_id) instead of count(*), so the count comes from index on innodb tables.
Change 139126 had a related patch set uploaded by Pcoombe: When getting number of signatures, count on indexed column https://gerrit.wikimedia.org/r/139126
Change 139285 had a related patch set uploaded by Pcoombe: Escape equals signs in csv output https://gerrit.wikimedia.org/r/139285
Change 139286 had a related patch set uploaded by Pcoombe: Escape number of signatures https://gerrit.wikimedia.org/r/139286
(In reply to Chris Steipp from comment #2) > At an architectural level, there's a couple of things that concern me: > * It seems like a violation of least privilege / separation of duty that > those who can view results always get 100% of them, and have to do their own > analysis to figure out which petition was signed. I'm not sure this is such a big deal for us, since there's only plans for one petition at the moment. Being able to filter the output by petition would be nice, but doesn't strike me as a security issue (unless we want to introduce per-petition rights, which will get complicated) > * If we get hit with "spam" (obviously not visible to the public, so low > value to the spamer.. but a user could easily write javascript to submit the > form 10M times), there's no way to delete it other than deleting rows in the > DB. That seems like it will come back to bite us. Based on this and feedback elsewhere I'm going to add rate-limiting, which should mitigate this. > * Similarly, if we notice abuse, the extension doesn't respect user blocks. It's going to be installed on the wikimediafoundation.org site, where editing is locked down so the only current blocks are of former staff. I think abuse is unlikely, but agree that it would be good to have some way to stop it just in case, and will look into adding a check for blocks. > * The extension doesn't integrate with abusefilter / spam blacklist or > Checkuser. The spam ones, again, we probably don't need. If someone starts > submitting death threats in the petition comments, then we will want > Checkuser integration. I guess we'll need this in order to determine who to block if it gets to that. Is there documentation somewhere on how to integrate with Checkuser?
(In reply to Peter Coombe from comment #6) > > * The extension doesn't integrate with abusefilter / spam blacklist or > > Checkuser. The spam ones, again, we probably don't need. If someone starts > > submitting death threats in the petition comments, then we will want > > Checkuser integration. > I guess we'll need this in order to determine who to block if it gets to > that. Is there documentation somewhere on how to integrate with Checkuser? I really need to make nicer instructions for it, but here's how AbuseFilter does it: https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FAbuseFilter.git/bafcde688fc12aefab2741c9b050264f71852227/AbuseFilter.class.php#L1006
Change 142737 had a related patch set uploaded by Pcoombe: Prevent blocked users from signing petitions https://gerrit.wikimedia.org/r/142737
Change 144074 had a related patch set uploaded by Pcoombe: Add rate limiting https://gerrit.wikimedia.org/r/144074
Okay, I believe I've fixed for all the issues raised here. Code reviews would be appreciated!
Change 139126 merged by jenkins-bot: When getting number of signatures, count on indexed column https://gerrit.wikimedia.org/r/139126
Change 139285 merged by jenkins-bot: Escape equals signs in csv output https://gerrit.wikimedia.org/r/139285
Change 139286 merged by jenkins-bot: Escape number of signatures https://gerrit.wikimedia.org/r/139286
Change 142737 merged by Aaron Schulz: Prevent blocked users from signing petitions https://gerrit.wikimedia.org/r/142737
Change 144074 merged by jenkins-bot: Add rate limiting https://gerrit.wikimedia.org/r/144074
Chris: It looks like all of the related patches here are merged, has this now passed the security review?
Hi Peter, I don't see patches or comments specifically saying why you haven't addressed: * The extension doesn't integrate with abusefilter / spam blacklist or Checkuser. The spam ones, again, we probably don't need. If someone starts submitting death threats in the petition comments, then we will want Checkuser integration. * Since you don't set any caching headers, by default the CSV shouldn't get cached, but you may want to be explicit and say it shouldn't be cached. I think at least CheckUser integration is needed before this goes into production, unless we have a good reason not to. Same for the caching headers.
Change 146970 had a related patch set uploaded by Pcoombe: Disable caching of petition CSV download https://gerrit.wikimedia.org/r/146970
It looks like the bugzilla notification didn't work for some reason, but https://gerrit.wikimedia.org/r/#/c/145435/ (now merged) added logging and Checkuser integration. There's a private log visible to users with the view-petition-data right which records users/IP addresses who signed a petition. CU isn't actually going to be much use on wikimediafoundation.org because of its restrictions on account creation (if a malefactor has an account there, then we have bigger problems!) and there are currently no checkusers there. But I added the integration in case it becomes useful or other sites want it.
Thanks!
Change 146970 merged by jenkins-bot: Disable caching of petition CSV download https://gerrit.wikimedia.org/r/146970
