Proposed patch:

In T356183#9510883, @Dreamy_Jazz wrote:

Proposed patch:

T356183.patch7 KBDownload

CR+1 - code looks good to me. I think this is likely fine to deploy for now.

Thanks @Dreamy_Jazz and @sbassett

I won't be able to deploy this today, but if @Tchanders agrees with this I could deploy this tommorrow and see what happens.

In T356183#9514358, @Dreamy_Jazz wrote:

I won't be able to deploy this today, but if @Tchanders agrees with this I could deploy this tommorrow and see what happens.

That should be fine if you want to do it during the backport window or just sometime before or after the train. Would likely be best to have you quickly test on a debug server or post-deploy.

The patch looks good to me, and works on local testing - happy to +2.

It occurs to me that this is a breaking change for anyone who is using the API directly. Should we give them a heads up somehow? Alternatively I don't mind considering direct use of the API unsupported (it's very tailored to the UI anyway), in which case maybe we can document this on https://www.mediawiki.org/wiki/Extension:IPInfo or somewhere.

...Oh, the patch didn't apply cleanly to the latest master (I had to add a couple of lines manually) though it didn't look like there were actually conflicts. Just mentioning in case it needs a rebase.

In T356183#9518332, @Tchanders wrote:

...Oh, the patch didn't apply cleanly to the latest master (I had to add a couple of lines manually) though it didn't look like there were actually conflicts. Just mentioning in case it needs a rebase.

The old patch from T356183#9510883 seems to apply fine to wmf/1.42.0-wmf.16 but not master and wmf/1.42.0-wmf.17.

git diff origin/wmf/1.42.0-wmf.16..origin/wmf/1.42.0-wmf.17 -- src/Rest/Handler/IPInfoHandler.php shows some minor formatting/line-count changes, but as you noted, no substantive conflicts.

Patch with minor merge conflicts addressed that should now apply to master and wmf/1.42.0-wmf.17:

In T356183#9519296, @sbassett wrote:

Patch with minor merge conflicts addressed that should now apply to master and wmf/1.42.0-wmf.17:

01-T356183-rev2.patch7 KBDownload

This doesn't apply to production due to a missing line of code. I've applied the following to wmf/1.42.0-wmf.17:

I've deployed the security patches to both wmf.17 and wmf.16.

Now this is deployed on production, we can go ahead and upload this to Gerrit as the IP Info extension is not bundled.

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

[mediawiki/extensions/IPInfo@master] SECURITY: Use CSRF token in IPInfo REST APIs

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

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

[mediawiki/extensions/IPInfo@REL1_41] SECURITY: Use CSRF token in IPInfo REST APIs

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

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

[mediawiki/extensions/IPInfo@REL1_40] SECURITY: Use CSRF token in IPInfo REST APIs

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

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

[mediawiki/extensions/IPInfo@REL1_39] SECURITY: Use CSRF token in IPInfo REST APIs

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

Change 998367 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@REL1_39] SECURITY: Use CSRF token in IPInfo REST APIs

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

Change 998322 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@REL1_41] SECURITY: Use CSRF token in IPInfo REST APIs

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

Change 998324 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@REL1_40] SECURITY: Use CSRF token in IPInfo REST APIs

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

Change 998327 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] SECURITY: Use CSRF token in IPInfo REST APIs

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

When this is made public we can also make T310393: IP Info log can be used to deanonymize user public too as that task is also about the same issue of not having a CSRF token.

@Dreamy_Jazz wrote:

01-T356183-rev2.patch7 KBDownload

This doesn't apply to production due to a missing line of code. I've applied the following to wmf/1.42.0-wmf.17:

01-T356183-rev2.patch7 KBDownload

Huh, strange. Not sure what happened there.

I've deployed the security patches to both wmf.17 and wmf.16.

Thanks.

When this is made public we can also make T310393: IP Info log can be used to deanonymize user public too as that task is also about the same issue of not having a CSRF token.

We can make it public now.

Suggested QA steps (designed for a local wiki):

1. Install the IP Info extension, if required
2. Add the following to your LocalSettings.php

$wgGroupPermissions['sysop']['ipinfo'] = true;
$wgGroupPermissions['sysop']['ipinfo-view-full'] = true;
$wgGroupPermissions['sysop']['ipinfo-view-log'] = true;
$wgGroupPermissions['*']['move'] = true;

3. While logged out (and not using a temporary account) make a few testing edits and move a page
4. Log into an account with the sysop group
5. Go to Special:Preferences and check the preference under the IP Information section
6. Open the history page for the page you edited in step 3
7. Open Developer tools and go to the Network tab
8. Click on the information icon next to the IP address (as shown below):

8. Verify that a dropdown menu appears
9. Go to the Network tab and find a request where the name / title is an integer followed by ?dataContext=popup such as in the following image (the full URL should be something like rest.php/ipinfo/v0/revision/335?dataContext=popup&language=en-gb)

10. Click on this request and go to the Payload tab
11. Verify that the Request payload includes a token such as the following:

12. Load Special:Log
13. Click on the information icon next to the IP address used to move the page
14. Verify that a dropdown menu appears
15. Go to the Network tab and find a request where the name / title is an integer followed by ?dataContext=popup such as in the following image (the full URL should be something like rest.php/ipinfo/v0/log/335?dataContext=popup&language=en-gb)

16. Click on this request and go to the Payload tab
17. Make sure that the Request Payload includes a token such as in the following:

18. Load the contributions page of one of the IP addresses used to make the edit (it is fine if only one IP address was used)
19. Open the IP Information section
20. Verify that no error occurs
21. In the Network tab find the request with the name / title that includes ?dataContext=infobox such as the following (the URL should be something like rest.php/ipinfo/v0/revision/339?dataContext=infobox&language=en-gb):

22. Click on this and find the Request Payload and verify that a token is present such as in the following screenshot:

Noting I dropped the security patch from this task from the train this week (1.42.0-wmf.18), still applied to previous weeks.

Patch was failing to apply, presumably because it's now merged in master. If this is incorrect, please let me know/flag someone on the blocker task for this week: T354436

In T356183#9536504, @thcipriani wrote:

Noting I dropped the security patch from this task from the train this week (1.42.0-wmf.18), still applied to previous weeks.

Patch was failing to apply, presumably because it's now merged in master. If this is incorrect, please let me know/flag someone on the blocker task for this week: T354436

Yes, this is correct. Thanks.

@Dreamy_Jazz I applied the below patch to rest.js to make it fail the first request but not the second, which I can confirm when looking at the browser's network tab. However, the popups and infoboxes still shows "The IP information could not be retrieved."

In T356183#9550359, @dom_walden wrote:

@Dreamy_Jazz I applied the below patch to rest.js to make it fail the first request but not the second, which I can confirm when looking at the browser's network tab. However, the popups and infoboxes still shows "The IP information could not be retrieved."

T356183_fail_first.patch518 BDownload

I can re-produce this too. It seems that I had misunderstood that the fail handler couldn't override the success status of the promise and when testing I had relied on seeing the network tab have a successful response for the second request. I will make a follow-up and thanks for finding this.

Moving back to 'In Progress' while I work on a follow-up to fix this.

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

[mediawiki/extensions/IPInfo@master] Make second successful request to REST API return a good promise

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

Change 1004121 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Make second successful request to REST API return a good promise

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

I see that a CSRF token is sent with the API request. If I try to make the API request with a different or no token, it returns an error:

{"errorKey":"rest-badtoken","messageTranslations":{"en":"The CSRF token provided is invalid."},"httpCode":403,"httpReason":"Forbidden"}

In T356183#9550359, @dom_walden wrote:

@Dreamy_Jazz I applied the below patch to rest.js to make it fail the first request but not the second, which I can confirm when looking at the browser's network tab. However, the popups and infoboxes still shows "The IP information could not be retrieved."

I cannot reproduce this bug anymore, either with the popup or infobox.

Test environment: local docker IP Info 0.0.0 (fc40803) 07:38, 20 February 2024.

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

[mediawiki/extensions/IPInfo@REL1_41] Make second successful request to REST API return a good promise

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

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

[mediawiki/extensions/IPInfo@REL1_40] Make second successful request to REST API return a good promise

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

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

[mediawiki/extensions/IPInfo@REL1_39] Make second successful request to REST API return a good promise

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

Change 1005078 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@REL1_40] Make second successful request to REST API return a good promise

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

Change 1005077 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@REL1_41] Make second successful request to REST API return a good promise

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

Change 1005079 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@REL1_39] Make second successful request to REST API return a good promise

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