Page MenuHomePhabricator

Security review for the WikimediaEditorTasks extension
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

The extension will provide the following functionality:

Contribution counts by defined type
  • Hook into PageSaveComplete and RollbackComplete events to count per-user and per-lang counts of edit and rollback events meeting defined criteria
  • Maintain a DB table of counts
  • Maintain a DB table of target counts met per user
  • Expose maintained data through a MediaWiki Action API module
Microcontribution suggestions
  • Generates microcontribution suggestions (for rollout, suggested Wikidata description edits) and serves them through an Action API module

Description of how the tool will be used at WMF

The tool will be used to support the App Editor Tasks project of the Wikimedia Apps team. Its primary consumers will be the official Wikipedia apps (initially, the Android app only).

Dependencies

Wikibase

Has this project been reviewed before?

No.

Working test environment

The extension is available via the wikimediaeditortasks role in MediaWiki-Vagrant.
Additionally, a development and testing environment is set up on Cloud VPS, consisting of wikidata-edit-counts.wmflabs.org and its client wikis, {en,es,fr,he,ru,zh}-edit-counts.wmflabs.org.

Post-deployment

Responsible team: Reading Infrastructure
Primary contact: Michael Holloway

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 1 2019, 12:58 AM
sbassett triaged this task as Medium priority.Feb 4 2019, 3:09 PM

FYI, my goal is to have the code substantially complete and ready for security review by the end of next week.

@Mholloway have you completed your work? we have a 30-day lead requirement for review as a reminder...

Mholloway renamed this task from Security review for the (WIP) WikimediaEditorTasks extension to Security review for the WikimediaEditorTasks extension.Feb 28 2019, 5:57 AM
Mholloway updated the task description. (Show Details)

@charlotteportero Sorry for the delay on this. The code is essentially complete (and recently merged), and ready for security review. (There is one patch[1] still outstanding, but it only involves querying the DB and persisting the intermediary results in another table, so I wouldn't expect it to have any meaningful security implications.)

I am out of office through the weekend, but happy to discuss the extension or walk through the code anytime beginning on Monday.

Thank you!

[1] https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikimediaEditorTasks/+/492526/

Bawolff closed this task as Resolved.Mar 12 2019, 10:55 AM

Overall: Looks good - Extension passes security review. There are a couple very small things though I would like to see changed.

Since this extension is adding database writes after a save, I think it would be a good idea to get the Performance-Team to give it a thumbs up before deployment.

Minor things

[Note: I include some other random stuff that crosses my mind as I read the code. Non-security stuff on this list is not officially part of the review. Feel free to ignore any of the non-security comments if you disagree with them]

  • wikimedia_editor_tasks_targets_passed documents the timestamp as a unix timestamp, where its actually a MW timestamp.
  • "CounterDao.php" line 208 & "CounterDao.php" line 239, 264 - [not exploitable] - in principle, IDatabase::timestamp() should be wrapped in IDatabase::addQuotes().
  • [not security] "CounterDao.php" line 244 (Along with other places that repeat the same pattern)- return (bool)$this->dbr->selectRowCount... At first glance this seems like a really inefficient way to detect whether at least one row exists. I'd suggest using IDatabase::selectField instead with a LIMIT of 1 [However, this isn't my area of expertise, so I may be missing something].
  • "Hooks.php" line 152 - $undidRev->getUser( RevisionRecord::RAW )->getId() - So in the event someone undo's a revision where the username is hidden, the counters still get reset. This is probably not a big deal since counters are private, but just as a precaution, could we use RevisionRecord::FOR_PUBLIC just in case. FOR_PUBLIC should be fine for 99.9999999% of the time (users with their name redacted usually aren't legit users anyhow), and it protects us in case a future version of the extension makes the counters public and someone figured out how to leak redacted usernames through this feature.