Page MenuHomePhabricator

Adjust Item and Property Special Pages to not leak IPs when editing and IP masking is enabled
Closed, ResolvedPublic

Description

Problem:
We are currently leaking the IPs of users who are not logged in when making edits on Item and Property Special Pages.

With temporary accounts are enabled on the repo, we must not leak the IPs of users who are not logged in and add entry with their temporary account name instead to the edit histories etc.

Affected Item and Property SpecialPages

BDD
GIVEN a user who isn't logged in
AND Temporary Accounts are enabled
WHEN an edit is made on an Item and Property Special Page
THEN an entry with their temporary account name is added to the edit history of the Item

Acceptance criteria:

  • IP is not leaked for users editing on the Merge Item SpecialPage and IP masking is enabled on the repo
  • IP is not leaked for users editing on the Redirect an entity SpecialPage and IP masking is enabled on the repo

Details

SubjectRepoBranchLines +/-
mediawiki/extensions/WikibaseLexememaster+108 -56
mediawiki/extensions/Wikibasemaster+329 -825
mediawiki/extensions/Wikibasemaster+27 -34
mediawiki/extensions/Wikibasemaster+530 -82
mediawiki/extensions/Wikibasemaster+21 -48
mediawiki/extensions/WikibaseLexememaster+14 -3
mediawiki/coremaster+1 -0
mediawiki/extensions/WikibaseLexemeREL1_39+109 -56
mediawiki/extensions/WikibaseREL1_39+140 -101
mediawiki/extensions/WikibaseLexemeREL1_40+109 -56
mediawiki/extensions/WikibaseREL1_40+140 -100
mediawiki/extensions/WikibaseLexemeREL1_41+109 -56
mediawiki/extensions/WikibaseREL1_41+139 -99
mediawiki/extensions/WikibaseLexemewmf/1.42.0-wmf.18+108 -56
mediawiki/extensions/Wikibasewmf/1.42.0-wmf.18+143 -100
mediawiki/extensions/WikibaseLexemewmf/1.42.0-wmf.17+108 -56
mediawiki/extensions/Wikibasewmf/1.42.0-wmf.17+143 -100
mediawiki/extensions/Wikibasemaster+143 -100
Show related patches Customize query in gerrit

Event Timeline

Hm, this comes back to an annoyance I had noticed in T345064EntityRedirectCreatorInteractor wants to throw RedirectCreationException on error, but RedirectCreationException automatically prepends all error message keys with 'wikibase-redirect-'. So if the TempUserCreator fails to create a temporary user, I can’t just forward its error message – I have to create a new message like wikibase-redirect-cant-redirect-due-to-tempusercreator-error or whatever. This is quite annoying and causes unneeded extra work for our volunteer translators.

If we used the MediaWiki Status pattern instead, we could cleanly return messages with arbitrary message keys (and even with parameters!), and forward any bad Status returned by the TempUserCreator.

IMHO it’s worth making this change first.

Ugh, but reporting such a Status in the API response becomes a huge mess :(

The modern MediaWiki Action API error formats, which Wikibase doesn’t use (see below), all match Status well: both in the API and in Status, you can have a list of errors, each defined as a message with optional parameters that may be formatted in different ways (plain text, HTML, etc.). (The default format, errorformat=bc, only supports one error and throws away the other ones, but that format is no longer recommended.)

Wikibase’s own format for API errors, which predates the modern MediaWiki thing mentioned above (see T304945), also kinda supports multiple errors, but still needs a single / top-level error code to report. You can see this slight mismatch in EntitySavingHelper – it gets a Status from the EditEntity service, and then has to determine an error code to pass to Wikibase’s ApiErrorReporter; EntitySavingHelper::handleSaveStatus() does this by either picking an errorCode from the Status value, or by hard-coding certain error codes based on the errorFlags from the Status value. Here, the “value” field of the Status is effectively carrying additional error information between EditEntity and `EntitySavingHelper.

The MergeItems API (wbmergeitems) apparently invented another way to return multiple errors (to resolve T180296), which is to return an array of error strings in the extradata of the error. It gets those error strings by starting from a single exception and walking up the chain of nested exceptions / causes while collecting their messages. I don’t know whether these messages are typically localized or not.

The CreateRedirect API (wbcreateredirect), on the other hand, doesn’t support multiple errors. When handling a RedirectCreationException, it always uses that exception’s code as the error code; if the exception has a cause, it’ll use the cause’s message, otherwise the exception’s own message. Recursive causes are ignored as far as I can tell, and the exception’s message may not even be localized: for instance, you can see the error “Can't create redirect on non empty item $id”, which comes from hard-coded English text in ItemRedirectCreationInteractor, and not “Can't create redirect on non empty entity $1”, which would be the wikibase-redirect-origin-not-empty i18n message.

Okay, I think fixing the error reporting is too big to tackle in this task. I’ll see if I can get T304945 prioritized (as an Epic, I guess); in the meantime, let’s just add a custom error code and i18n message here after all.

Change 1002998 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Use EditEntity for ItemMergeInteractor

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

Change 1002999 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Use EditEntity for MergeLexemesInteractor

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

Change 1002998 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use EditEntity for ItemMergeInteractor

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

Change 1002950 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.17] Use EditEntity for ItemMergeInteractor

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

Change 1002951 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.17] Use EditEntity for MergeLexemesInteractor

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

Change 1002952 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.18] Use EditEntity for ItemMergeInteractor

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

Change 1002953 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.18] Use EditEntity for MergeLexemesInteractor

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

Change 1002999 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Use EditEntity for MergeLexemesInteractor

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

Change 1002950 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.17] Use EditEntity for ItemMergeInteractor

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

Change 1002951 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.17] Use EditEntity for MergeLexemesInteractor

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

Change 1002952 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.18] Use EditEntity for ItemMergeInteractor

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

Change 1002953 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.18] Use EditEntity for MergeLexemesInteractor

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

Mentioned in SAL (#wikimedia-operations) [2024-02-13T17:39:09Z] <logmsgbot> lucaswerkmeister-wmde@deploy2002 Started scap: Backport for [[gerrit:1002950|Use EditEntity for ItemMergeInteractor (T356149 T356764)]], [[gerrit:1002951|Use EditEntity for MergeLexemesInteractor (T356149 T356764)]], [[gerrit:1002952|Use EditEntity for ItemMergeInteractor (T356149 T356764)]], [[gerrit:1002953|Use EditEntity for MergeLexemesInteractor (T356149 T356764)]]

Mentioned in SAL (#wikimedia-operations) [2024-02-13T17:40:40Z] <logmsgbot> lucaswerkmeister-wmde@deploy2002 lucaswerkmeister-wmde: Backport for [[gerrit:1002950|Use EditEntity for ItemMergeInteractor (T356149 T356764)]], [[gerrit:1002951|Use EditEntity for MergeLexemesInteractor (T356149 T356764)]], [[gerrit:1002952|Use EditEntity for ItemMergeInteractor (T356149 T356764)]], [[gerrit:1002953|Use EditEntity for MergeLexemesInteractor (T356149 T356764)]] synced to the testservers (https://wik

Mentioned in SAL (#wikimedia-operations) [2024-02-13T17:49:21Z] <logmsgbot> lucaswerkmeister-wmde@deploy2002 Finished scap: Backport for [[gerrit:1002950|Use EditEntity for ItemMergeInteractor (T356149 T356764)]], [[gerrit:1002951|Use EditEntity for MergeLexemesInteractor (T356149 T356764)]], [[gerrit:1002952|Use EditEntity for ItemMergeInteractor (T356149 T356764)]], [[gerrit:1002953|Use EditEntity for MergeLexemesInteractor (T356149 T356764)]] (duration: 10m 11s)

Change 1002959 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

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

Change 1002960 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_41] Use EditEntity for ItemMergeInteractor

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

Change 1002959 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

Reason:

will cherry pick again once I8bfb95f45e (REL1_41, change 1002961) is merged

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

Change 1002959 restored by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

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

Change 1002960 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_41] Use EditEntity for ItemMergeInteractor

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

Change 1002959 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

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

Change 1002962 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_40] Use EditEntity for ItemMergeInteractor

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

Change 1002962 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@REL1_40] Use EditEntity for ItemMergeInteractor

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

Change 1003477 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_40] Use EditEntity for MergeLexemesInteractor

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

Change 1003478 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_39] Use EditEntity for ItemMergeInteractor

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

Change 1003477 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_40] Use EditEntity for MergeLexemesInteractor

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

Change 1003480 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_39] Use EditEntity for MergeLexemesInteractor

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

Change 1003478 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@REL1_39] Use EditEntity for ItemMergeInteractor

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

Change 1003480 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_39] Use EditEntity for MergeLexemesInteractor

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

Change 1003797 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Dependency injection for temp users on merge

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

Change 1003798 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Make EditEntity service return new context

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

Change 1003799 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] WIP: Create temp users on redirect/merge

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

Change 1003800 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] WIP: updated createRedirect() call

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

Change 1005052 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Add strict types to touched files

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

Change 1005053 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Use TempUserTestTrait in tests

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

Change 1005056 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] Add createaccount permission in TempUserTestTrait

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

Change 1005056 merged by jenkins-bot:

[mediawiki/core@master] Add createaccount permission in TempUserTestTrait

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

Change 1003797 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Dependency injection for temp users on merge

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

Change 1003798 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Make EditEntity service return new context

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

Change 1003799 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Create temp users on redirect/merge

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

Change 1005052 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Add strict types to touched files

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

Change 1005053 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use TempUserTestTrait in tests

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

Okay, I think this should be done and ready for verification…

Arian_Bozorg claimed this task.

Looks good to me! Thanks so much :)