Make history-related blocking respect $wgCrawlerProtectedActions and add $wgCrawlerProtectionProtectRevisions

includes/CrawlerProtectionService.php

@@ -90,11 +90,20 @@ public function checkPerformAction(
 		$diffId = (int)$request->getVal( 'diff' );
 		$oldId = (int)$request->getVal( 'oldid' );
 
+		// The type=revision, diff and oldid parameters are all ways of
+		// viewing page history. They are only blocked when 'history' is
+		// in the configured list of protected actions, so that operators
+		// can fully disable history blocking by removing 'history' from
+		// $wgCrawlerProtectedActions.
+		$historyProtected = $this->isProtectedAction( 'history' );
+
 		if (
-			$type === 'revision'
-			|| $this->isProtectedAction( $action )
-			|| $diffId > 0
-			|| $oldId > 0
+			$this->isProtectedAction( $action )
+			|| ( $historyProtected && (
+				$type === 'revision'
+				|| $diffId > 0
+				|| $oldId > 0
+			) )
 		) {
 			$this->responseFactory->denyAccess( $output );
 			return false;

tests/phpunit/namespaced-stubs.php

@@ -100,9 +100,13 @@ public function getContext() {
 
 namespace MediaWiki\User {
 	class User {
-		public function isRegistered() {
+		public function isRegistered(): bool {
 			return false;
 		}
+
+		public function getName(): string {
+			return '';
+		}
 	}
 }
 

tests/phpunit/unit/CrawlerProtectionServiceTest.php

@@ -204,6 +204,83 @@ public function testCheckPerformActionBlocksConfiguredAction() {
 		$this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
 	}
 
+	/**
+	 * When 'history' is removed from CrawlerProtectedActions, the
+	 * history-related parameters (type=revision, diff, oldid) should
+	 * also be allowed, so that operators can fully disable history
+	 * blocking via configuration (see issue: history can't be unblocked).
+	 *
+	 * @covers ::checkPerformAction
+	 * @dataProvider provideHistoryRelatedRequestParams
+	 *
+	 * @param array $getValMap
+	 * @param string $msg
+	 */
+	public function testCheckPerformActionAllowsHistoryRelatedWhenNotConfigured(
+		array $getValMap, string $msg
+	) {
+		$output = $this->createMock( self::$outputPageClassName );
+		$user = $this->createMock( self::$userClassName );
+		$user->method( 'isRegistered' )->willReturn( false );
+		$user->method( 'getName' )->willReturn( '127.0.0.1' );
+
+		$request = $this->createMock( self::$webRequestClassName );
+		$request->method( 'getVal' )->willReturnMap( $getValMap );
+
+		$responseFactory = $this->createMock( ResponseFactory::class );
+		$responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+		$service = $this->buildService( [], [], [], $responseFactory );
+		$this->assertTrue( $service->checkPerformAction( $output, $user, $request ), $msg );
+	}
+
+	/**
+	 * Data provider for history-related parameters that should be
+	 * allowed when 'history' is not in CrawlerProtectedActions.
+	 *
+	 * @return array
+	 */
+	public function provideHistoryRelatedRequestParams(): array {
+		return [
+			'action=history' => [
+				[
+					[ 'type', null, null ],
+					[ 'action', null, 'history' ],
+					[ 'diff', null, null ],
+					[ 'oldid', null, null ],
+				],
+				'action=history should be allowed when history not protected',
+			],
+			'type=revision' => [
+				[
+					[ 'type', null, 'revision' ],
+					[ 'action', null, null ],
+					[ 'diff', null, null ],
+					[ 'oldid', null, null ],
+				],
+				'type=revision should be allowed when history not protected',
+			],
+			'diff=42' => [
+				[
+					[ 'type', null, null ],
+					[ 'action', null, null ],
+					[ 'diff', null, '42' ],
+					[ 'oldid', null, null ],
+				],
+				'diff=42 should be allowed when history not protected',
+			],
+			'oldid=99' => [
+				[
+					[ 'type', null, null ],
+					[ 'action', null, null ],
+					[ 'diff', null, null ],
+					[ 'oldid', null, '99' ],
+				],
+				'oldid=99 should be allowed when history not protected',
+			],
+		];
+	}
+
 	/**
 	 * @covers ::checkPerformAction
 	 */