Page MenuHomePhabricator

Wikidata is editable for blocked users
Closed, ResolvedPublic

Description

The following undesired situations are possible:

  1. A blocked anonymous user (IP address) can perform all kind of edits on Wikidata: https://www.wikidata.org/wiki/Special:Contributions/84.81.160.164
  2. A blocked registered or anonymous user can edit Wikibase entities: https://www.wikidata.org/wiki/Special:Contributions/Abian, https://www.wikidata.org/wiki/Special:Contributions/Crazykidd

Most of the identified accounts started to edit on November 30. See T210962.

Open questions:

  • Is this behavior Wikidata.org specific? Is it in the Wikibase extension?
  • Is T210830 the same?

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Lydia_Pintscher triaged this task as Unbreak Now! priority.Dec 2 2018, 6:43 PM
Sjoerddebruin lowered the priority of this task from Unbreak Now! to Needs Triage.Dec 2 2018, 6:43 PM
Sjoerddebruin triaged this task as Unbreak Now! priority.
Sjoerddebruin updated the task description. (Show Details)
Sjoerddebruin updated the task description. (Show Details)Dec 2 2018, 6:46 PM
abian added a subtask: Restricted Task.Dec 2 2018, 7:57 PM
Tarrow added a comment.Dec 2 2018, 8:19 PM

Anyone, got any clues as to when the problem started?

abian added a comment.Dec 2 2018, 8:22 PM

Anyone, got any clues as to when the problem started?

Not for now. T210962 should let us estimate it.

I authored T210830 on Nov 30 at around noon, based on an edit very early that day. That task is very likely about the same problem.

abian updated the task description. (Show Details)Dec 2 2018, 8:44 PM
Tarrow added a comment.Dec 2 2018, 8:53 PM

I looked though the commits deployed in the last two trains and nothing jumped out at me.

Someone could double check the editentity refactoring I guess but I was the reviewer of it and didn't notice anything out of place.

After this is done we should remove the comment on extensions/Wikibase/repo/includes/EditEntity/MediawikiEditEntity.php#476 this confused me for a moment

I just tested on test.wikidata.org for both creating an item and editing and editing existing terms were successfully stopped for a blocked account.

edits to statement using so wbsetclaim and wbremoveclaims however went through

Might be related to T182983.

Might be related to T182983.

I tested but nope.

It looks like reverting https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/471993/ from core fixes the issue

abian updated the task description. (Show Details)Dec 2 2018, 11:38 PM
abian updated the task description. (Show Details)
Addshore added a comment.EditedDec 2 2018, 11:43 PM

It looks like this stems from Title::checkUserBlock and then Action::requiresUnblock.
ViewEntityAction::requiresUnblock returns false and has done since 2012.
EditEntityView extends this, but doesnt override the methods, so has the same behaviour as a view...
This method has a confusing name...
I'm not yet sure why this didn't work while testing terms, perhaps it uses a different action? hmpf..

Might be related to T182983.

I tested but nope.
It looks like reverting https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/471993/ from core fixes the issue

Since this is security sensitive, I'd rather revert the core patch since we there's a decent chance other extensions are affected, and this is a significant breaking change.

Since this is security sensitive, I'd rather revert the core patch since we there's a decent chance other extensions are affected, and this is a significant breaking change.

I just checked through all of the other actions and everything else looks okay.
The issue in Wikibase was due to the edit action extending the view action.

I'll let you do the core revert now though and I'll head to bed and remove my .6 patch for Wikibase.
We will get the patch on master merged in EU morning time tomorrow.

It looks like this stems from Title::checkUserBlock and then Action::requiresUnblock.
ViewEntityAction::requiresUnblock returns false and has done since 2012.
EditEntityView extends this, but doesnt override the methods, so has the same behaviour as a view...
This method has a confusing name...
I'm not yet sure why this didn't work while testing terms, perhaps it uses a different action? hmpf..

It sounds like EditEntityView:: requiresUnblock() should return true since the user is required to be unblocked in order to preform the action. Does changing it to true resolve the issue?

I've reverted https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/471993/ on wmf.6, and confirmed on test.wikidata.org that it fixes the issue. I'd appreciate it if someone could verify on wikidata.org itself before we make this task public.

@TBolliger FYI T208862 is being reverted, so plan accordingly. :)

jrbs added a subscriber: jrbs.Dec 3 2018, 6:49 AM

I've reverted https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/471993/ on wmf.6, and confirmed on test.wikidata.org that it fixes the issue. I'd appreciate it if someone could verify on wikidata.org itself before we make this task public.

Confirmed it is no longer an issue on wikidata / testwikidata

I guess we can make this public now.
Though we should also merge the change in Wikibase master to avoid this being deployed unpatched with this weeks train

dmaza added a comment.Dec 3 2018, 2:29 PM

@Addshore @Legoktm are we ready to move forward with the patch on wikibase? What do we need to get https://gerrit.wikimedia.org/r/c/mediawiki/core/+/471993 (or a version of it) back on wmf.6?

Anomie added a subscriber: Anomie.Dec 3 2018, 3:20 PM

@Addshore @Legoktm are we ready to move forward with the patch on wikibase? What do we need to get https://gerrit.wikimedia.org/r/c/mediawiki/core/+/471993 (or a version of it) back on wmf.6?

It looks like @Legoktm wants a review of classes that subclass Action, specifically looking for situations where a subclass overrides requiresUnblock() to return false to ensure that that class and any of its subclasses don't need it to return true after all.

At a quick grep, classes overriding requiresUnblock() to return false are:

  • (core) PurgeAction
  • (core) HistoryAction
  • (core) SpecialPageAction
  • (core) WatchAction
  • (core) RawAction
  • (core) InfoAction
  • (CirrusSearch) CirrusSearch\Dump
  • (WikibaseQualityConstraints) WikibaseQuality\ConstraintReport\Api\CheckConstraintsRdf
  • (Wikibase) Wikibase\ViewEntityAction

I don't have a convenient way to find subclasses of these classes; someone who uses an IDE might be able to help with that.

Also, in extensions not deployed on Wikimedia wikis,

  • (EducationProgram) EducationProgram\ViewAction
dmaza added a comment.Dec 3 2018, 3:44 PM

Is it safe to assume that we only need to check on those 4 extensions ( CirrusSearch, WikibaseQualityConstraints, Wikibase, EducationProgram ) ?
I still think that @Addshore's patch should be merged regardless.

@Addshore’s patch is already merged (on master – the wmf.6 backport is abandoned).

Anomie added a comment.Dec 3 2018, 4:01 PM

Is it safe to assume that we only need to check on those 4 extensions ( CirrusSearch, WikibaseQualityConstraints, Wikibase, EducationProgram ) ?

No, you also need to check any extension that subclasses any of the classes listed (directly or indirectly).

FWIW, I can tell you that CheckConstraintsRdf in WikibaseQualityConstraints is safe (it’s a read-only action), and I’m confident it has no subclasses (it would not make sense for any other extension to subclass it).

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 3 2018, 4:38 PM
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptDec 3 2018, 4:38 PM

Change 477307 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@master] Override requiresWrite in EditEntityAction

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

Addshore moved this task from incoming to in progress on the Wikidata board.Dec 3 2018, 5:37 PM

Change 477270 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Update comment in ViewEntityAction

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

Change 477307 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Override requiresWrite in EditEntityAction

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

abian closed subtask Restricted Task as Resolved.Dec 4 2018, 10:24 AM

Shouldn't the priority of this also be lowered, or do we still consider it "Unbreak Now"? It is been a few days too much to call it "now" IMO.

Lydia_Pintscher lowered the priority of this task from Unbreak Now! to High.Dec 4 2018, 4:43 PM

Just pointing to T211048, where we're auditing the affected classes - any feedback very welcome!

@Tchanders FYI, in another change we noticed that we’d also failed to override requiresWrite – could that require a similar investigation?

Anomie added a comment.Dec 5 2018, 6:15 PM

@Tchanders FYI, in another change we noticed that we’d also failed to override requiresWrite – could that require a similar investigation?

requiresWrite does not matter for this task.

Yes, but it’s a similar class of potential error, if I understand correctly, just not as critical.

TBolliger closed this task as Resolved.Dec 5 2018, 7:15 PM
TBolliger claimed this task.
TBolliger moved this task from Ready to Done on the Anti-Harassment (AHT Sprint 34) board.

Marking as resolved because the immediate problem has been fixed. Anti-Harassment is working through T211048: Audit classes that override Action::requiresUnblock method.

Thank you.

Liuxinyu970226 removed a subscriber: Liuxinyu970226.

Change 479874 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_29] Override requiresUnblock in EditEntityAction

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

Change 479875 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_30] Override requiresUnblock in EditEntityAction

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

Change 479876 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_31] Override requiresUnblock in EditEntityAction

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

Change 479877 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_32] Override requiresUnblock in EditEntityAction

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

Change 479874 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_29] Override requiresUnblock in EditEntityAction

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

Change 479875 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_30] Override requiresUnblock in EditEntityAction

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

Change 479876 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_31] Override requiresUnblock in EditEntityAction

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

Change 479877 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_32] Override requiresUnblock in EditEntityAction

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

Change 479878 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_29] Override requiresWrite in EditEntityAction

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

Change 479879 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_30] Override requiresWrite in EditEntityAction

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

Change 479880 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_31] Override requiresWrite in EditEntityAction

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

Change 479881 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@REL1_32] Override requiresWrite in EditEntityAction

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

Change 479878 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_29] Override requiresWrite in EditEntityAction

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

Change 479879 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_30] Override requiresWrite in EditEntityAction

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

Change 479880 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_31] Override requiresWrite in EditEntityAction

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

Change 479881 merged by Addshore:
[mediawiki/extensions/Wikibase@REL1_32] Override requiresWrite in EditEntityAction

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