Patch for review:

Tested locally using the following steps running mariadb with replication:

• Made a test edit
• Gave myself suppressor rights
• Suppressed the edit
• Loaded Special:ApiSandbox
• Used the following query:

{
	"action": "query",
	"format": "json",
	"list": "checkuser",
	"curequest": "edits",
	"cutarget": "Exampleuser-who-made-the-edit",
	"cutoken": "Exampletoken"
}

• Verified that the "user" for the edit shows the username as I have rights to see it
• Removed suppressor rights from myself
• Ran the API query again
• Verified that the "user" for the edit is shown as "(username removed)" as I do not have rights to see it

In T318166#8248278, @Dreamy_Jazz wrote:

T318166.patch1 KBDownload

+2

The patch looks good, though I didn't test locally. Thanks, @Dreamy_Jazz. We should be able to get this deployed during the next security window.

this is now deployed in production on php-1.40.0-wmf.5

Tested on testwiki using wmf.8 following the steps:

• Made a test edit
• Got granted OS and CU by steward on testwiki
• Suppressed the test edit
• Opened Special:ApiSandbox
• Ran a check on Dreamy Jazz (myself) using the get edits mode and pressing the auto-fill token button
• Verified that I could see my username
• Had OS removed from me on testwiki
• Ran the same check
• Verified that I could not see my username (instead showing username hidden)
• Had OS granted again to remove the test suppression
• Had CU and OS removed.

I can provide screenshots of the results (these results were designed to only include my edits) but will leave them not published here because this will go public eventually. The test was successful and so the patch worked.

For items stored in the logging table https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/856930 is open to add a new column called cuc_log_id that would store the log ID for any associated entry. This would then be used to find the RevisionDeletion status on the entry in the logging table and would address this issue for logged items.

Change 879073 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] SECURITY: Do not show suppressed usernames on edits in the API

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

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

[mediawiki/extensions/CheckUser@REL1_39] SECURITY: Do not show suppressed usernames on edits in the API

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

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

[mediawiki/extensions/CheckUser@REL1_38] SECURITY: Do not show suppressed usernames on edits in the API

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

Change 879541 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_39] SECURITY: Do not show suppressed usernames on edits in the API

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

Change 879543 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_38] SECURITY: Do not show suppressed usernames on edits in the API

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

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

[mediawiki/extensions/CheckUser@REL1_35] SECURITY: Do not show suppressed usernames on edits in the API

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

Change 879569 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_35] SECURITY: Do not show suppressed usernames on edits in the API

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

@Dreamy_Jazz is there a separate ticket for log actions? perhaps we should open one for that and then close these?

and is the change here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/866684 not sufficient

In T318166#8520985, @Mstyles wrote:

@Dreamy_Jazz is there a separate ticket for log actions? perhaps we should open one for that and then close these?

There doesn't yet exist separate tickets for log events - I kept them together as the attack vector is very similar. I wouldn't oppose creating new tickets, but as these stand they have checkboxes for progress on fixing log actions in the task description. I guess as this task contains information about log actions not respecting revision deletion status, the new tasks won't need to be hidden from public view (as the cat is out of the bag).

In T318166#8521010, @Mstyles wrote:

and is the change here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/866684 not sufficient

No. This only added the tables that would allow storing the log ID to look up revision deletion status, but did not add any support for reading from them or writing to them nor fix the issue at hand here. There are https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/876360 and https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/877182 (which are yet to be reviewed) that only adds code to write to the new tables when the config is set. Reading from these tables and switching to writing/reading on WMF wikis would be needed before a fix would work. I'm not yet fully sure how to fix log actions as I'm not sure how the code will read from the tables.

Everything before "Move log entries in cu_changes to cu_private_event" in T324907 will need to be done before a fix would work for log entries.

One thing I would note is that there would be no foolproof method to match items in the cu_changes table to the logging table. My intention is to just move these to the table created for events that only CheckUser's can see. This means that even if this was kept private until the fix was in place, 3 months would have to pass before WMF production CheckUser had no leakage.

@Mstyles @Dreamy_Jazz - I'd support the idea of creating a separate task or tasks to track the related log events issue. It makes things more granular, which is generally a good idea, and won't block us form resolving these tasks as disclosed security issues, as they just went out with the latest supplemental security release at T318974.

Okay. I'll look at creating separate tasks.

In T318166#8521571, @Dreamy_Jazz wrote:

Re-closing as separate tickets filed.

Thanks!