Page MenuHomePhabricator

ApiEditPage.php assumes that higher revision numbers have later timestamps
Closed, ResolvedPublic

Description

if ( $params['undo'] < $params['undoafter'] ) {
	list( $params['undo'], $params['undoafter'] ) =
		[ $params['undoafter'], $params['undo'] ];
}

This snippet helpfully checks if you reversed the order of the "undo" and "undoafter" parameters, and switches them if so. Unfortunately, it assumes that a higher revision ID indicates a later revision, which is not the case if old revisions are imported. In this case, it gets the undo backwards. Probably this special case could be removed entirely, assuming nobody's code depends on it, in which case if you reverse undo and undoafter you'll probably just get a merge conflict, right? Alternatively, we could add a check of the actual revision timestamps, but I didn't think about if that would be unduly inefficient.

Thanks to Anomie for pointing this out during code review of tests I was writing:

https://gerrit.wikimedia.org/r/c/420716/2/tests/phpunit/includes/api/ApiEditPageTest.php#632

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 21 2018, 2:12 PM
Anomie moved this task from Unsorted to Needs Code on the MediaWiki-API board.Mar 21 2018, 3:55 PM
Anomie added a project: good first task.
Anomie added a subscriber: Anomie.

I did some queries on the Analytics data since Feb 1. There were only 14 requests to action=edit with undo < undoafter in that time. All are by the same user, and I was able to track down the script in question and let the maintainer know about this task.

I think we're safe to fix the bug.

Krinkle moved this task from Backlog to API on the MediaWiki-Page-editing board.Apr 12 2018, 4:09 PM
DovAlp added a subscriber: DovAlp.EditedApr 27 2018, 11:38 PM

The code in question is wrapped in a another block

                if ( $params['undoafter'] > 0 ) {
	          if ( $params['undo'] < $params['undoafter'] ) {
		    list( $params['undo'], $params['undoafter'] ) =
		    [ $params['undoafter'], $params['undo'] ];
		  }
		  $undoafterRev = Revision::newFromId( $params['undoafter'] );
		}

It seems all that code does is create this problem. I'll submit a patch to get rid of it now.

Change 429533 had a related patch set uploaded (by DovAlp; owner: DovAlp):
[mediawiki/core@master] Fixes T190285

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

Simetrical, did the tests ever get changed. The code is edited but now the test suite is failing. The new tests should probably get merged in first.

Anomie added a comment.May 1 2018, 5:58 PM

Yes, the tests were merged. Your change will have to update the test to reflect the fixed behavior.

Note that includes/EditPage.php has the same special case and should presumably be fixed as well.

I can take a look at includes/EditPage.php later, but the original problem/tests have been fixed by my latest patch set. Let me know what you think.

(sorry for abandoning my patch for a while, life got in the way)

Change 429533 merged by jenkins-bot:
[mediawiki/core@master] ApiEditPage: Don't swap undo and undoafter parameters

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

Restricted Application added a project: Platform Engineering. · View Herald TranscriptSep 30 2020, 1:45 AM
Ammarpad closed this task as Resolved.Sep 30 2020, 1:57 AM
Ammarpad assigned this task to DovAlp.
Ammarpad removed a project: Platform Engineering.
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:39 PM