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

.github/workflows/ci.yml

@@ -51,7 +51,9 @@ jobs:
       - name: Setup Composer
         # Don't block insecure packages because MediaWiki might be using outdated dependencies, which shouldn't block the extension's pipelines from running
         run: |
-          echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}},"config":{"audit":{"abandoned":"report","block-insecure":false}}}' > composer.local.json
+          echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}}}' > composer.local.json
+          # composer-merge-plugin does not merge the "config" section from composer.local.json, so set the advisory policy on the root composer.json directly
+          composer config --no-plugins policy.advisories.block false
           composer update
           composer update
       - name: Lint
@@ -96,7 +98,9 @@ jobs:
       - name: Setup Composer
         # Don't block insecure packages because MediaWiki might be using outdated dependencies, which shouldn't block the extension's pipelines from running
         run: |
-          echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}},"config":{"audit":{"abandoned":"report","block-insecure":false}}}' > composer.local.json
+          echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}}}' > composer.local.json
+          # composer-merge-plugin does not merge the "config" section from composer.local.json, so set the advisory policy on the root composer.json directly
+          composer config --no-plugins policy.advisories.block false
           composer update
           composer update
       - name: Phan
@@ -137,7 +141,9 @@ jobs:
       - name: Setup Composer
         # Don't block insecure packages because MediaWiki might be using outdated dependencies, which shouldn't block the extension's pipelines from running
         run: |
-          echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}},"config":{"audit":{"abandoned":"report","block-insecure":false}}}' > composer.local.json
+          echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}}}' > composer.local.json
+          # composer-merge-plugin does not merge the "config" section from composer.local.json, so set the advisory policy on the root composer.json directly
+          composer config --no-plugins policy.advisories.block false
           composer update
           composer update
       - name: Install MediaWiki

extension.json

@@ -27,14 +27,16 @@
         "CrawlerProtectedActions": {
             "value": [
                 "history"
-            ]
+            ],
+            "merge_strategy": "provide_default"
         },
         "CrawlerProtectedSpecialPages": {
             "value": [
                 "mobilediff",
                 "recentchangeslinked",
                 "whatlinkshere"
-            ]
+            ],
+            "merge_strategy": "provide_default"
         },
         "CrawlerProtectionRawDenial": {
             "value": false
@@ -49,7 +51,8 @@
             "value": "403 Forbidden. You must be logged in to view this page."
         },
         "CrawlerProtectionAllowedIPs": {
-            "value": []
+            "value": [],
+            "merge_strategy": "provide_default"
         }
     },
     "ServiceWiringFiles": [

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
 	 */