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.