Should we handle this like a security patch? IMO the risk is low given that:

• This is really only used on WMF wikis
• The users with access to this tool already have signed ANPDP

Given the lots of recent modifications, I'd propose to handle this publicly but merge and backport quickly

The current approach in CheckuserLog is that suppressed usernames are struck out and italicized. That should probably be replicated here?

In T411366#11434462, @XXBlackburnXx wrote:

The current approach in CheckuserLog is that suppressed usernames are struck out and italicized. That should probably be replicated here?

Yes that would be the approach to take

Untagging Security-Team for now, once there's a patch ready, we can help deploy it and/or add it to the next supplemental core security release.

next supplemental security release.

CheckUser is a bundled extension, so AFAIK it would need to made in the main release

In T411366#11443613, @Dreamy_Jazz wrote:

next supplemental security release.

CheckUser is a bundled extension, so AFAIK it would need to made in the main release

Ah, yes, it has been there since 1.44.

@sbassett thoughts on this comment? @kostajh has already said this is a okay way forward

In T411366#11430422, @Dreamy_Jazz wrote:

Should we handle this like a security patch? IMO the risk is low given that:

• This is really only used on WMF wikis
• The users with access to this tool already have signed ANPDP

Given the lots of recent modifications, I'd propose to handle this publicly but merge and backport quickly

In T411366#11451597, @Dreamy_Jazz wrote:

@sbassett thoughts on this comment? @kostajh has already said this is a okay way forward

In T411366#11430422, @Dreamy_Jazz wrote:

Should we handle this like a security patch? IMO the risk is low given that:

• This is really only used on WMF wikis
• The users with access to this tool already have signed ANPDP

Given the lots of recent modifications, I'd propose to handle this publicly but merge and backport quickly

This sounds low-risk enough IMO to push up to gerrit with a timely Wikimedia production deployment.

The code has stabilised enough that I think we can use a normal security patch

Proposed patch:

In T411366#11614673, @Dreamy_Jazz wrote:

Proposed patch:

T411366.patch12 KBDownload

CR+2

One non-blocking question, though:
Do we want the Investigate button to be enabled if all users in a case are hidden? In this situation, it's useless, as the current user cannot do anything about investigating the case. We don't have to do it right now, as it's not a security-related issue.

Should we add a isUserVisible() function in SuggestedInvestigationsCasesPager.php, would help avoid repeated code.

Will there be extra queries for every single user?

In T411366#11626684, @MPostoronca-WMF wrote:

Should we add a isUserVisible() function in SuggestedInvestigationsCasesPager.php, would help avoid repeated code.

Given that this is a security patch, I'd want to avoid the possibility of merge conflicts. If we add a helper method it would increase the risk that a rebase is required if we were to add a helper method in a similar area.

Additionally, I think the logic is simple enough here that a helper method would likely add more lines of code overall?

Will there be extra queries for every single user?

Yes, but there is not a way to batch load this information in MediaWiki as it stands.

Given this is a security patch and is the method followed by all other CheckUser interfaces (and interfaces in core etc.), I'd want to avoid adding code likely to MediaWiki core as part of a security patch which could merge conflict, and instead improve it if we think it's needed once the patch is made public and we don't have to worry about merge conflicts

In T411366#11626462, @mszwarc wrote:

One non-blocking question, though:
Do we want the Investigate button to be enabled if all users in a case are hidden? In this situation, it's useless, as the current user cannot do anything about investigating the case. We don't have to do it right now, as it's not a security-related issue.

Perhaps, though maybe it's better to show it as disabled? We probably want to ask Design about the correct approach. Happy to leave it as-is if you think it's not blocking

Thanks for the code review (both Marcin and Maxim)

In T411366#11626795, @Dreamy_Jazz wrote:

In T411366#11626462, @mszwarc wrote:

One non-blocking question, though:
Do we want the Investigate button to be enabled if all users in a case are hidden? In this situation, it's useless, as the current user cannot do anything about investigating the case. We don't have to do it right now, as it's not a security-related issue.

Perhaps, though maybe it's better to show it as disabled?

Yes, that was what I meant (that it's currently enabled and useless, so we might make it disabled).

In T411366#11626814, @mszwarc wrote:

In T411366#11626795, @Dreamy_Jazz wrote:

In T411366#11626462, @mszwarc wrote:

One non-blocking question, though:
Do we want the Investigate button to be enabled if all users in a case are hidden? In this situation, it's useless, as the current user cannot do anything about investigating the case. We don't have to do it right now, as it's not a security-related issue.

Perhaps, though maybe it's better to show it as disabled?

Yes, that was what I meant (that it's currently enabled and useless, so we might make it disabled).

Ah, yeah. I think I misread your comment to remove the button entirely. Thanks

I've deployed the patch to production.

@sbassett the issue is present in release 1.45. I want to ask if we still can publicly upload the patch now given that, or if we should wait for a security release? I still believe the risk is low because I don't see anyone using this feature except for WMF wikis, but given that CheckUser is bundled I want to check

Actually I see that this was discussed already. I'm going to go forward with making the patch public and backporting

I have uploaded the fix publicly and backported it to release 1.45.

I see suppressed usernames are replaced with (username removed).

I have raised a follow up bug T417868 that we might want to look at.

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

[mediawiki/extensions/CheckUser@master] SI: Disable Special:Investigate action if all users hidden

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

Change #1240641 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] SI: Disable Special:Investigate action if all users hidden

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