Platform Engineering - any interest in investigating this? Seems like a potential race condition?

MovePage::isValidMoveTarget() uses FOR UPDATE, but it's only called if Title::getArticleID() returns non-zero with no special flags. Then MovePage::moveToInternal() will delete the page if getArticleID(READ_LATEST) is non-zero. So it would seem that if the page is missing in the replica DB, isValidMove() will return true, and then moveToInternal() will unconditionally delete the page if it can be found in the master.

So it would seem that there's quite a wide exploitable window for unprivileged deletion of movable pages.

I was able to reproduce this with a lagged slave by inserting a sleep at the start of ChronologyProtector::shutdown(). Using two separate client IPs probably also would have worked. With a single client IP, the second request needs to execute after the first move completes, but before the first request commits the ChronologyProtector position.

This patch appears to be sufficient and should be deployed as a security update.

diff --git a/includes/MovePage.php b/includes/MovePage.php
index cdc994cab6..ffadf55586 100644
--- a/includes/MovePage.php
+++ b/includes/MovePage.php
@@ -231,7 +231,9 @@ class MovePage {
 
 		if ( $this->oldTitle->equals( $this->newTitle ) ) {
 			$status->fatal( 'selfmove' );
-		} elseif ( $this->newTitle->getArticleID() && !$this->isValidMoveTarget() ) {
+		} elseif ( $this->newTitle->getArticleID( Title::READ_LATEST /* T272386 */ )
+			&& !$this->isValidMoveTarget()
+		) {
 			// The move is allowed only if (1) the target doesn't exist, or (2) the target is a
 			// redirect to the source, and has no history (so we can undo bad moves right after
 			// they're done). If the target is a single revision redirect to a different page,

It seems fragile though, since READ_LATEST flags often get lost during refactoring, so I'm thinking about other redundant ways to ensure this bug doesn't recur.

In T272386#6786300, @tstarling wrote:

This patch appears to be sufficient and should be deployed as a security update.

Thanks for the patch! @Reedy or I can plan to deploy this on Monday, February 1st during the next security window.

It seems fragile though, since READ_LATEST flags often get lost during refactoring, so I'm thinking about other redundant ways to ensure this bug doesn't recur.

Sounds good. If these result in additional security patches, we can deploy those as well. Or if they're less sensitive and more code-hardening, I'd imagine they could just go through gerrit.

Change 678033 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@REL1_31] SECURITY: Non-admin deleted enwiki page in fast double move

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

Change 678039 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@REL1_35] SECURITY: Non-admin deleted enwiki page in fast double move

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

Change 678039 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: Non-admin deleted enwiki page in fast double move

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

Change 678033 merged by Reedy:

[mediawiki/core@REL1_31] SECURITY: Non-admin deleted enwiki page in fast double move

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

Change 678074 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@master] SECURITY: Non-admin deleted enwiki page in fast double move

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

Change 678074 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Non-admin deleted enwiki page in fast double move

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

Change 677964 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@REL1_36] SECURITY: Non-admin deleted enwiki page in fast double move

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

Change 677964 merged by jenkins-bot:

[mediawiki/core@REL1_36] SECURITY: Non-admin deleted enwiki page in fast double move

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