Page MenuHomePhabricator

CheckUser improvements for security patch T207094 / r539643
Closed, ResolvedPublic

Description

Per @Umherirrender's public gerrit comments, we should address the following issues related to the deployed/backported security patch for the PermanentlyPrivate bug T207094:

  1. Revision::newFromId - line 1787 - "This seems uncached, at least it is unbatched. This mixed database queries in the formatting code and may slow done the whole page"
  2. Exception - line 1807 - "There are some maintenance scripts which can lead to this situation. It is also possible that the delete or undelete is in process and finished between both queries."
  3. $rev->userCan - line 1812 - "userCan needs a user to avoid $wgUser"
  4. Linker::commentBlock - line 1815 - "There is Linker::revComment which handle these message and styling for such messages (grey + strike)"

Also, test coverage apparently decreased for this patch, so we might want to add/update tests accordingly.

Event Timeline

sbassett triaged this task as Medium priority.Sep 30 2019, 5:18 PM

@sbassett Is Security planning to take on this ticket? Anti-Harassment team is planning to work on making some improvements to checkuser starting next quarter so we could look into this.

@Niharika - I think this can be open to whomever has the cycles to work on it. If Anti-Harassment would like to work on these issues, that's great. To be clear - these are just some non-blocking issues that were suggested as improvements for a currently-deployed security patch.

sbassett added a parent task: Restricted Task.Oct 8 2019, 6:35 PM

Change 541606 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/CheckUser@master] Load RevisionRecord in bulk for audience check

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

Change 541606 abandoned by Umherirrender:
Load RevisionRecord in bulk for audience check

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

Change 824874 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] Batch process and format revision comments using preprocessResults

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

Change 824874 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Batch process and format revision comments using preprocessResults

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

The associated patch has been merged and this addressed all the 4 points. Issues with a drop in coverage can be addressed in T175920.