Change 893064 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/CheckUser@master] [WIP] Enable checkusers to reveal all IPs used by a temporary user

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

Change 893064 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Enable checkusers to reveal all IPs used by a temporary user

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

I am finding performance slow if the temporary user has a lot of edits on the page (50+).

For example for 200 edits, my Chromium tab crashes.

@STran Are you experiencing the same thing?

I also wonder if appending too many revision IDs to the /rest.php/checkuser/v0/temporaryaccount/<username>/revisions/ will hit URL length limits. I don't think I have seen it happen yet, but it seems possible.

FYI, I have been creating lots of edits locally with:

for i in {1..<num edits>}; do docker compose exec mediawiki sh -c "echo foo$i | php maintenance/run.php edit.php --user='*Unregistered <num>' Main_Page"; done

Change 902057 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/CheckUser@master] Improve the race conditions present

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

I tried to reproduce locally and could not. Maybe @Tchanders could be the tiebreaker. Regardless, I have my suspicions that doing a lot of edits could lead to performance problems. Thalia and I chatted about it and I've got a patch with some changes. Maybe it would help? Could you try it out and see if it improves the issue at all?

I'm also not sure about the url limits but fwiw, there could be a use case where there are too many edits shown and the api won't return all of them because it hit the return count limit and we won't support that use case.

In T327946#8717407, @STran wrote:

I tried to reproduce locally and could not. Maybe @Tchanders could be the tiebreaker. Regardless, I have my suspicions that doing a lot of edits could lead to performance problems. Thalia and I chatted about it and I've got a patch with some changes. Maybe it would help? Could you try it out and see if it improves the issue at all?

Thanks @STran, that is significantly quicker. I even tried revealing 500 revisions, which I assume is the maximum anyone could do.

I'm also not sure about the url limits but fwiw, there could be a use case where there are too many edits shown and the api won't return all of them because it hit the return count limit and we won't support that use case.

I wasn't thinking about that, but it is a good point. Setting $wgCheckUserMaximumRowCount to 2 didn't make a difference. Is it supported by rest.php/checkuser/v0/temporaryaccount/<username>/revisions?

I was just thinking about servers rejecting a URL that is too long.

I also notice it makes the same API request with each revealed IP address. Could it do it once? Or perhaps it doesn't matter as the response is cached.

In T327946#8717967, @dom_walden wrote:

In T327946#8717407, @STran wrote:

I wasn't thinking about that, but it is a good point. Setting $wgCheckUserMaximumRowCount to 2 didn't make a difference. Is it supported by rest.php/checkuser/v0/temporaryaccount/<username>/revisions?

Looking at the source code, it doesn't seem to apply this value. This can be done easily and I've made a patch for it which is at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/902096

In T327946#8718017, @Dreamy_Jazz wrote:

Looking at the source code, it doesn't seem to apply this value. This can be done easily and I've made a patch for it which is at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/902096

Thanks.

In T327946#8717967, @dom_walden wrote:

In T327946#8717407, @STran wrote:

I'm also not sure about the url limits but fwiw, there could be a use case where there are too many edits shown and the api won't return all of them because it hit the return count limit and we won't support that use case.

I wasn't thinking about that, but it is a good point. Setting $wgCheckUserMaximumRowCount to 2 didn't make a difference. Is it supported by rest.php/checkuser/v0/temporaryaccount/<username>/revisions?

Actually, on Special:RecentChanges I can get it to make a request with 501 revisions (but not more?), which returns a 400 error with Too many values supplied for parameter "ids". The limit is 500. and no IPs are revealed. But if we don't support that I guess it is OK.

In T327946#8718472, @dom_walden wrote:

Actually, on Special:RecentChanges I can get it to make a request with 501 revisions (but not more?), which returns a 400 error with Too many values supplied for parameter "ids". The limit is 500. and no IPs are revealed. But if we don't support that I guess it is OK.

I think that's because the default limit is 500 for users who have high rate limits and 50 for those without this ability. My patch raises both these limits to the configuration amount (default 5000).

I can see a need for the limit to be higher than 500 (such as on Special:RecentChanges), but maybe 5,000 (by default) is too large. Perhaps instead the maximum could be a number that would not cause the URL to be too long.

In T327946#8717967, @dom_walden wrote:

In T327946#8717407, @STran wrote:

I tried to reproduce locally and could not. Maybe @Tchanders could be the tiebreaker. Regardless, I have my suspicions that doing a lot of edits could lead to performance problems. Thalia and I chatted about it and I've got a patch with some changes. Maybe it would help? Could you try it out and see if it improves the issue at all?

Thanks @STran, that is significantly quicker. I even tried revealing 500 revisions, which I assume is the maximum anyone could do.

@STran Unfortunately, I find after applying this change the pair reveal no longer works for me. Clicking the "Show IP" does not respond, without any errors in the browser console.

In T327946#8720496, @dom_walden wrote:

In T327946#8717967, @dom_walden wrote:

In T327946#8717407, @STran wrote:

I tried to reproduce locally and could not. Maybe @Tchanders could be the tiebreaker. Regardless, I have my suspicions that doing a lot of edits could lead to performance problems. Thalia and I chatted about it and I've got a patch with some changes. Maybe it would help? Could you try it out and see if it improves the issue at all?

Thanks @STran, that is significantly quicker. I even tried revealing 500 revisions, which I assume is the maximum anyone could do.

@STran Unfortunately, I find after applying this change the pair reveal no longer works for me. Clicking the "Show IP" does not respond, without any errors in the browser console.

@dom_walden This should be fixed now, and the patch merged, so I think it's ready for testing again

Change 902057 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Improve the race conditions present

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

In T327946#8718495, @Dreamy_Jazz wrote:

In T327946#8718472, @dom_walden wrote:

Actually, on Special:RecentChanges I can get it to make a request with 501 revisions (but not more?), which returns a 400 error with Too many values supplied for parameter "ids". The limit is 500. and no IPs are revealed. But if we don't support that I guess it is OK.

I think that's because the default limit is 500 for users who have high rate limits and 50 for those without this ability. My patch raises both these limits to the configuration amount (default 5000).

I can see a need for the limit to be higher than 500 (such as on Special:RecentChanges), but maybe 5,000 (by default) is too large. Perhaps instead the maximum could be a number that would not cause the URL to be too long.

Thanks for figuring all this out, and for the patch. I'm tempted to do a bit of research before changing the limits. It looks like we'd already be hitting the URL length limit for Edge/IE (on the order of 2k characters) with a limit of 500 revision IDs. Perhaps we could monitor the number of revisions asked for (assuming that high numbers from other browsers would still make it through) and based on that data decide whether we need to (a) do something more to limit URL length and/or (b) do something more to allow larger numbers of revisions to be requested.

Here's a task for that: T333185: Monitor the number of revisions requested via temporary account revision API

Testing IP reveal on:

• Revision history
• Special:Contributions
• Special:Log
• Special:RecentChanges
  • Both versions
  • Including Live Updates
• Special:AbuseLog
• Diff
• Page information

For a CheckUser and non-CheckUser.

Skins:

• Vector 2010
• Vector 2022
• Minerva

Browsers:

• Firefox 102
• Chromium 111

As far as I could tell the IP revealed for the revision is correct (I used self-verifying data where the title of the page edited was the same as the IP which edited it).

Because this feature is only testable locally, I could not test it on Safari or a mobile. I might ask someone else if they can do this for me.

Some questions for @STran:

If there are over 50 revisions for a user on a page, attempting to reveal a single IP (as a non-CheckUser) won't work, because you hit the API limit. Will this be fixed by T333299?

Should we do any error handling? For example, if you are blocked and try to reveal an IP, it appears as if nothing happens.

The enhanced (Group results by page) Special:RecentChanges can have two "Show IP" buttons for the same revision (e.g. the two IPs revealed in the screenshot below). This means the API request contains a duplicate revision ID. I don't know if this is something we have to take into account if we try to work out an appropriate limit to send in the API request.

Will this line always work if there is no matching revId, because the response from the API is (sometimes) an associative array. Perhaps this case won't actually occur in practice.

var ip = response.ips[ revId || 0 ];

In order:

1. If you hit a limit I'm not sure what the solution is yet. The problem is captured in T333190: What should happen if clicking on an IP reveal button results in an API error?, which captures the API limit and the blocked user use case.

2. We probably should de-dupe and it won't be hard to clean up the batch before we send it over. I can do it as part of T333299: Stop API from returning excess IPs on user request since that makes some changes to when we'll be batching.

3. afaik it's always an associative array? At least, if it's using that endpoint.

@Tchanders I noticed that the task description said that:

On all other pages admins and checkusers will be able to reveal all IPs for a given temp account

However, the current code only seems to do this for checkusers. Has there been a change in direction?

In T327946#8745581, @Dreamy_Jazz wrote:

@Tchanders I noticed that the task description said that:

On all other pages admins and checkusers will be able to reveal all IPs for a given temp account

However, the current code only seems to do this for checkusers. Has there been a change in direction?

There has been a bit of back-and-forth discussion about this, but it's been decided that this will be the behaviour for anyone with the right to reveal IP addresses - see T334113: Enable IP multireveal for all users.

(You're right though that there was a discrepancy here between the task description and the implementation)

Thanks for the update.