Maniphest T215048

Security review for the WikimediaEditorTasks extension
Closed, ResolvedPublic
Actions

Description

Project Information

Name of tool/project: mediawiki/extensions/WikimediaEditorTasks
Project home page: https://www.mediawiki.org/wiki/Extension:WikimediaEditorTasks
Name of team requesting review: Reading Infrastructure
Primary contact: Michael Holloway
Target date for deployment: 3/31/19
Link to code repository / patchset: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/WikimediaEditorTasks

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

Related Objects
Search...

Status	Assigned	Task
Resolved	• Mholloway	T212793 Build infrastructure required to support the Suggested Edits feature
Resolved	• Mholloway	T212795 Build infrastructure to track counts of qualifying edits for the Suggested Edits feature
Resolved	• Mholloway	T218136 Deploy Extension:WikimediaEditorTasks to production
Resolved	Bawolff	T215048 Security review for the WikimediaEditorTasks extension

Event Timeline

• Mholloway created this task.Feb 1 2019, 12:58 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 1 2019, 12:58 AM

• Mholloway added a parent task: T212795: Build infrastructure to track counts of qualifying edits for the Suggested Edits feature.Feb 1 2019, 1:01 AM

sbassett triaged this task as Medium priority.Feb 4 2019, 3:09 PM

• Jhernandez moved this task from Needs triage to Tracking on the Product-Infrastructure-Team-Backlog-Deprecated board.Feb 6 2019, 4:39 PM

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

• Mholloway added a project: WikimediaEditorTasks.Feb 24 2019, 6:17 PM

@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/

• charlotteportero assigned this task to Bawolff.Mar 5 2019, 6:13 PM

• charlotteportero edited projects, added Security-Team-Review-Active; removed deprecated-security-team-reviews.

• Charlotte added a project: Wikipedia-Android-App-Backlog.Mar 7 2019, 5:34 PM

• Charlotte moved this task from Needs Triage to Tracking on the Wikipedia-Android-App-Backlog board.

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.