Page MenuHomePhabricator

Follow-up comments from the security review
Closed, ResolvedPublic

Description

Review and act on the followup comments from the security review:

[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.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 12 2019, 11:53 AM

Change 495892 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/WikimediaEditorTasks@master] Tweak targetPassed queries for performance

https://gerrit.wikimedia.org/r/495892

Change 495895 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/WikimediaEditorTasks@master] Hygiene: Update column description comment

https://gerrit.wikimedia.org/r/495895

Change 495896 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/WikimediaEditorTasks@master] Hygiene: Add quotes to timestamp() calls in comparison conditional strings

https://gerrit.wikimedia.org/r/495896

Mholloway triaged this task as Medium priority.Mar 12 2019, 2:56 PM

Change 495895 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEditorTasks@master] Hygiene: Update column description comment

https://gerrit.wikimedia.org/r/495895

Change 495896 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEditorTasks@master] Hygiene: Add quotes to timestamp() calls in comparison conditional strings

https://gerrit.wikimedia.org/r/495896

Change 496216 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/WikimediaEditorTasks@master] Tweak: Use FOR_PUBLIC audience constant when checking reverted users

https://gerrit.wikimedia.org/r/496216

Change 496216 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEditorTasks@master] Tweak: Use FOR_PUBLIC audience constant when checking reverted users

https://gerrit.wikimedia.org/r/496216

Jhernandez closed this task as Resolved.Mar 14 2019, 3:15 PM

Change 495892 had a related patch set uploaded (by Krinkle; owner: Michael Holloway):
[mediawiki/extensions/WikimediaEditorTasks@master] Tweak targetPassed queries for performance

https://gerrit.wikimedia.org/r/495892

Change 495892 abandoned by Mholloway:
Tweak targetPassed queries for performance

Reason:
superseded by Ia7454c497e82857aa50922e8b1b9656446c92b97

https://gerrit.wikimedia.org/r/495892