Page MenuHomeMiraheze

Bug exists in RemovePII extension whereby existing CheckUser log user data is retained for deleted/anonymized users
Closed, ResolvedPublic

Description

Bug: Noticed this as part of Trust and Safety investigation, but previously, with @Paladox' enhancements to the removepii.php maintenance script, certain CheckUser log actions relating to deleted and anonymized users were deleted. Now, though, as the old username is not renamed first in the CheckUser log, those log entries are retained.

Example for illustrative purposes (user and IP data redacted and replaced with example information of what I mean):

19:14, 6 September 2021, Doug (Miraheze) got users for 1234:5678::/32 (talk | block) (Trust and Safety—Terms of Use enforcement investigation)
19:13, 6 September 2021, Doug (Miraheze) got IP addresses for ExampleUser (talk | contribs | block) (Trust and Safety—Terms of Use enforcement investigation)

Note that even if with the removepii.php, the log action for getting users for a given IP address or IP address range (top line) was always retained, and should continue to be. The issue is the getting IP addresses for a given user (bottom line).

I'd suggest fixing this so that the CU log entries for the target are renamed, then remove those log entries immediately thereafter, if that makes sense.

Event Timeline

Universal_Omega moved this task from Backlog to Short Term on the MediaWiki (SRE) board.
Universal_Omega moved this task from Unsorted to Short Term on the Universal Omega board.

@Dmehus: Looking at it, it's only the log entry remaining. There should not be any data.

It should be first renamed, once the rename from RemovePII is done, as technically it is just the user ID that is being removed from database, not username, so as long as rename for user is done first, then it should be removing it from checkuser.

Oh, if it doesn't work with the extension the script doesn't either. RhinosF1 is right, the way its done now removes acting user not target user it seems.

@Dmehus: Looking at it, it's only the log entry remaining. There should not be any data.

@RhinosF1 That's correct. No CU data exists when searching for the deleted or renamed user. I guess it's more of a cosmetic thing thing, so we may not need to do anything?

It's just weird that before, the user was renamed and the CU log entries were removed. So if we wanted to replicate that functionality from the removepii.php script, I'd suggest a fix, but you're right, this can probably be lowered to normal or low priority, or closed as declined.

@Dmehus: I agree it's a bug. Just pointing out that it's simply the log retained.

@Dmehus: I agree it's a bug. Just pointing out that it's simply the log retained.

@RhinosF1 Yep, that's entirely fair.

https://github.com/miraheze/RemovePII/pull/20 - although I'm not certain it will work, and testing RemovePII is quite difficult (especially when needing to test CU which sysadmins don't have access to nor really should, and don't know if it's a good idea to add just for testing). I don't think it'll cause it to error though.

https://github.com/miraheze/RemovePII/pull/20 - although I'm not certain it will work, and testing RemovePII is quite difficult (especially when needing to test CU which sysadmins don't have access to nor really should, and don't know if it's a good idea to add just for testing). I don't think it'll cause it to error though.

Technically, you could add checkuser and checkuser-log permissions temporarily to sysadmin and test it on a test account, if you needed it, ideally linking to the Phabricator ticket in your log summary.

https://github.com/miraheze/RemovePII/pull/20 - although I'm not certain it will work, and testing RemovePII is quite difficult (especially when needing to test CU which sysadmins don't have access to nor really should, and don't know if it's a good idea to add just for testing). I don't think it'll cause it to error though.

Technically, you could add checkuser and checkuser-log permissions temporarily to sysadmin and test it on a test account, if you needed it, ideally linking to the Phabricator ticket in your log summary.

Yeah. Another thing for me personally is that I'm currently mobile until September 15th, so someone else will likely need to test or at least someone else deploy to test3 for me to test from there.

https://github.com/miraheze/RemovePII/pull/20 - although I'm not certain it will work, and testing RemovePII is quite difficult (especially when needing to test CU which sysadmins don't have access to nor really should, and don't know if it's a good idea to add just for testing). I don't think it'll cause it to error though.

Technically, you could add checkuser and checkuser-log permissions temporarily to sysadmin and test it on a test account, if you needed it, ideally linking to the Phabricator ticket in your log summary.

Yeah. Another thing for me personally is that I'm currently mobile until September 15th, so someone else will likely need to test or at least someone else deploy to test3 for me to test from there.

Ah, okay. True, perhaps @RhinosF1 or @Reception123 can test it on test3wiki when it's deployed there, adding the needed user rights on metawiki with the clear testing reason. Testing user account should be identifiably named, like "RhinosF1 (RemovePII test account)" or "Reception123 (RemovePII test account)". You'll need to perform the check on the test account prior to running RemovePII, though.

This task can probably be made public, I don't think it's a true security issue.

This task can probably be made public, I don't think it's a true security issue.

Probably can now, since there's not any non-public data in the task, assuming @Owen and @Reception123 have no objections.

https://phabricator.miraheze.org/T7995#161251

Regarding this, @Universal_Omega, what do you mean here? When we used the removepii.php script, the CheckUser log data for the bottom line is removed.

https://phabricator.miraheze.org/T7995#161251

Regarding this, @Universal_Omega, what do you mean here? When we used the removepii.php script, the CheckUser log data for the bottom line is removed.

Well not according to the code, the code for that is 100% exactly the same between the RemovePII extension and maintenance script.

John changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 3 2022, 13:07
John changed the edit policy from "Custom Policy" to "All Users".