Page MenuHomePhabricator

AbuseFilter interferes with internal unauthenticated move actions
Open, Needs TriagePublic

Description

AbuseFilter uses the TitleMove hook, called from MovePage::moveUnsafe(), so it intercepts internal actions which call MovePage::move(), which is documented as moving "without taking user permissions into account". So AbuseFilter rules can break maintenance scripts, for example T394556#10964880.

I suggest adding a new hook to MovePage::moveIfAllowed() or authorizeMove() and then migrating AbuseFilter to use that hook instead.

Event Timeline

In a previous episode: T212082: Do not block page moves with AbuseFilter on LocalRenameJob page moves and a whole lot of linked tasks. My previous stance on this was that user permissions are not the only things AbuseFilter would look for. That's why, years ago, I made AF handle the TitleMove hook rather than whatever permission-related hook it was previously using. I later learned that the hook choice was intentional, but it still didn't feel fully right to me. What matter is not permissions per se, but rather the fact that the page move is initiated internally, and not as a consequence of user action. And obviously there are cases that are not entirely trivial to categorize (e.g., page moves in a global rename: someone definitely initiated something, but not the page move itself).

That was a while ago. I don't really have an opinion on this today. I do think it would be preferable to make this distinction of "internal" page move and reflect that in the hook if possible. If not, that's also fine.

Conceptually I think AbuseFilter should act at the top level of the hierarchy of user actions. So in a user rename action, AbuseFilter should decide whether to allow the user rename, not whether to allow the user page moves. Authorizing the user rename implicitly authorizes the subsidiary page moves.

Or to put it another way, we want an authorization layer which is fully separate from the storage backend. User actions should proceed by first obtaining authorization, and then by altering stored data. User rights are one kind of authorization, and AbuseFilter is another kind. AbuseFilter doesn't belong in the storage backend.

I don't agree with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/529917 . This needs to be a core concept, it's not reasonable for everything wanting to do moves to know about everything that might try to prevent those moves.

I think I agree with all of that. However, something that was true 6 years ago and is still true now is that the infrastructure for that doesn't exist. CentralAuth was the main use case that kept coming up over and over again, so I did something that worked for that scenario. I do agree that in principle, this should live in core, and a separate hook seems the simplest way to do that. My question was more about the definition/scope of said hook, and specifically the fact that it shouldn't be about user rights only.