Thanks for reporting this!

There doesn't seem to be any CSRF vulnerability here. It's just that AbuseFilter is logging on the permissions check (hook 'MovePageCheckPermissions') rather than the actual move, probably because there doesn't seem to be a hook in MovePage::move() that allows aborting the actual move.

@Anomie: When the URL includes both wpNewTitle= and wpOldTitle=, MovePageCheckPermissions is called twice, once when the form is displayed (so as to "save a click" according to the comments in SpecialMovepage.php), and again when the move is actually attempted. The second call only happens after the token is passed, and is not the problem. The first call happens without a token, and results in a publicly visible log entry associated with one's username, which is a problem when the link is followed from an external site.

In T208907#4728855, @Anomie wrote:

There doesn't seem to be any CSRF vulnerability here. It's just that AbuseFilter is logging on the permissions check (hook 'MovePageCheckPermissions') rather than the actual move, probably because there doesn't seem to be a hook in MovePage::move() that allows aborting the actual move.

I'd call it a low severity csrf. Write actions can still be done from a third party site - its just not a super important write action (relative to actually moving a page). In theory something like this could maybe be used to spam the log making it harder to read or maybe make an innocent person look malicious (admittedly that is a little of a stretch)

In T208907#4730603, @Bawolff wrote:

I'd call it a low severity csrf. Write actions can still be done from a third party site - its just not a super important write action (relative to actually moving a page). In theory something like this could maybe be used to spam the log making it harder to read or maybe make an innocent person look malicious (admittedly that is a little of a stretch)

It also allows the third party site to associate IPs with usernames, either from the timestamp, or by making the move target unique for each visitor.

In T208907#4730766, @suffusion_of_yellow wrote:

In T208907#4730603, @Bawolff wrote:

I'd call it a low severity csrf. Write actions can still be done from a third party site - its just not a super important write action (relative to actually moving a page). In theory something like this could maybe be used to spam the log making it harder to read or maybe make an innocent person look malicious (admittedly that is a little of a stretch)

It also allows the third party site to associate IPs with usernames, either from the timestamp, or by making the move target unique for each visitor.

Ah, I didn't realize that all users had the abusefilter-log right. You're correct, it could be used as a way to link IP addresses and usernames and exfiltrate that data.

Well, changing the hook used by AbuseFilter when checking move actions would be really great. Not only for this, but also because we're not really checking permissions, and end up showing a confusing error message (since it contains "you do not have permission..."), see T199622 and T65768. However, as Anomie pointed out, there doesn't seem to be a suitable hook other than this. Recently I found an almost suitable hook (can't recall which one), but it lacks some context.

In T208907#4731070, @Daimona wrote:

Well, changing the hook used by AbuseFilter when checking move actions would be really great. Not only for this, but also because we're not really checking permissions, and end up showing a confusing error message (since it contains "you do not have permission..."), see T199622 and T65768. However, as Anomie pointed out, there doesn't seem to be a suitable hook other than this. Recently I found an almost suitable hook (can't recall which one), but it lacks some context.

We can make new hooks :)

In T208907#4731401, @Bawolff wrote:

We can make new hooks :)

Yeah, and it'd be great :) Or also add more context to existing ones. I decided not to work on this to avoid messing up with core, but if you'd like to help me a bit I can go on and try :-)
I guess the main points to decide are where to call the hook and what parameters to pass.

EDIT: I think the hook I tried to use is TitleMove. Maybe we should add context there instead of adding a new hook?
EDIT2: Yes, that's the hook I saw, but now I recall it doesn't make it possible to prevent the move. I'm still wondering what's the best to do.
EDIT3 (the last one, I swear): We also have MovePageIsValidMove, which can prevent the upload. However, it's only meant to check "technical" details to determine whether the page can be moved, and not something like abuse filters. Anyway it doesn't pass a User object.

In T208907#4731420, @Daimona wrote:

EDIT: I think the hook I tried to use is TitleMove. Maybe we should add context there instead of adding a new hook?
EDIT2: Yes, that's the hook I saw, but now I recall it doesn't make it possible to prevent the move. I'm still wondering what's the best to do.

Yes, TitleMove is in the right place.

It might suffice to just pass in a Status object for the hook function to add an error to, something like

$status = Status::newGood();
Hooks::run( 'TitleMove', [ $this->oldTitle, $this->newTitle, $user, &$status ] );
if ( !$status->isOK() ) {
    // Move was aborted by the hook
    return $status;
}

EDIT3 (the last one, I swear): We also have MovePageIsValidMove, which can prevent the upload. However, it's only meant to check "technical" details to determine whether the page can be moved, and not something like abuse filters. Anyway it doesn't pass a User object.

MovePageIsValidMove looks like it would have the same problem as MovePageCheckPermissions does here.

@Anomie https://gerrit.wikimedia.org/r/#/c/mediawiki/core/ /472467/
This still needs some other patches to update the signature where the hook is used. Sent both patches, with a depends-on for the core patch... And added a $reason parameter as well, since we need it.

Testing the AbuseFilter with the new hook, it seems to work fine, but it still shows the message "you do not have permission...". I'll try to amend the core patch later to show a better message. I'll also send the AF patch to gerrit, with depends-on for the core patch, since no one will really understand the problem from this task by only seeing the patch.

Done everything. So we have:

• https://gerrit.wikimedia.org/r/#/c/472467/ main core patch
• https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Newsletter/ /472474/ update hook parameters
• https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ArticleRatings/ /472473/ update hook parameters
• https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/ /472668/ AF patch

The core patch causes:

PHP Warning:  Declaration of SpecialPageTranslationMovePage::showForm($par) should be compatible with MovePageForm::showForm($err, $isPermError = false) in /srv/mediawiki/tags/2018-11-19_09:36:50/extensions/Translate/tag/SpecialPageTranslationMovePage.php on line 16

I haven't looked into this task, but you might want to have a look that Translate doesn't replicate the same issue.

@Nikerabbit SpecialPageTranslationMovePage::showForm overrides SpecialMovePage::showForm, which has been changed here. However, the function in Extension:Translate isn't using the error parameter ($par) at all. So I guess you may add another boolean parameter to copy the signature. However, having a function which takes 2 parameters, and both of them are unused, isn't something good to have in the code.

I was wondering, do we want to backport everything to every supported branch, keep the task secret for a while, or what?

The warning, which I expected you to fix, has caused quite a bit log spam in translatewiki.net over the past weeks.

@Nikerabbit https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Translate/ /477225/ I forgot about it. Not elegant, but at least it works.

Oh sorry! I See what happened. I had created https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Translate/ /477217 after my last comment, but gerrit-bot did not have access to report it here.

@Nikerabbit Oh, I didn't notice that, and they're also pretty similar

Does this still need to be private?

In T208907#6135376, @suffusion_of_yellow wrote:

Does this still need to be private?

All patches seem to be merged (years ago), no PII on the task and it was never related to a security incident that WMF-Legal might want to protect, so yes, this can be made public.