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

README.md

@@ -15,6 +15,17 @@ intensive.
   `curl -s "[YOURWIKI]api.php?action=query&meta=siteinfo&siprop=specialpagealiases&format=json" | jq -r '.query.specialpagealiases[].aliases[]' | sort`
   Of course certain Specials MUST be allowed like Special:Login so do not block
   everything.
+* `$wgCrawlerProtectedActions` - array of MediaWiki action names to block for
+  anonymous users (default: `[ 'history' ]`). Removing `'history'` from this
+  list disables protection of the history-listing page but does not affect
+  whether individual revisions and diffs are protected (see
+  `$wgCrawlerProtectionProtectRevisions` below).
+* `$wgCrawlerProtectionProtectRevisions` - when `true` (default), anonymous
+  access to individual revisions and diffs (requests using `type=revision`,
+  `diff=`, or `oldid=` query parameters) is denied. Set to `false` to allow
+  anonymous access to those URLs. This setting is independent of
+  `$wgCrawlerProtectedActions`, so you can protect revisions/diffs without
+  protecting the history listing page, or vice versa.
 * `$wgCrawlerProtectionUse418` - drop denied requests in a quick way via
   `die();` with
   [418 I'm a teapot](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/418)

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,11 @@
             "value": "403 Forbidden. You must be logged in to view this page."
         },
         "CrawlerProtectionAllowedIPs": {
-            "value": []
+            "value": [],
+            "merge_strategy": "provide_default"
+        },
+        "CrawlerProtectionProtectRevisions": {
+            "value": true
         }
     },
     "ServiceWiringFiles": [

includes/CrawlerProtectionService.php

@@ -44,6 +44,7 @@ class CrawlerProtectionService {
 		'CrawlerProtectedActions',
 		'CrawlerProtectedSpecialPages',
 		'CrawlerProtectionAllowedIPs',
+		'CrawlerProtectionProtectRevisions',
 	];
 
 	/** @var ServiceOptions */
@@ -90,11 +91,20 @@ public function checkPerformAction(
 		$diffId = (int)$request->getVal( 'diff' );
 		$oldId = (int)$request->getVal( 'oldid' );
 
+		// $wgCrawlerProtectionProtectRevisions independently controls whether
+		// type=revision, diff and oldid requests are blocked. This allows
+		// operators to disable history-listing protection (by removing 'history'
+		// from $wgCrawlerProtectedActions) while still blocking direct access
+		// to individual revisions and diffs, or vice versa.
+		$revisionsProtected = $this->options->get( 'CrawlerProtectionProtectRevisions' );
+
 		if (
-			$type === 'revision'
-			|| $this->isProtectedAction( $action )
-			|| $diffId > 0
-			|| $oldId > 0
+			$this->isProtectedAction( $action )
+			|| ( $revisionsProtected && (
+				$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

@@ -44,20 +44,23 @@ public static function setUpBeforeClass(): void {
 	 * @param array $protectedActions
 	 * @param string|array $allowedIPs
 	 * @param ResponseFactory|\PHPUnit\Framework\MockObject\MockObject|null $responseFactory
+	 * @param bool $protectRevisions
 	 * @return CrawlerProtectionService
 	 */
 	private function buildService(
 		array $protectedPages = [ 'recentchangeslinked', 'whatlinkshere', 'mobilediff' ],
 		array $protectedActions = [ 'history' ],
 		$allowedIPs = [],
-		$responseFactory = null
+		$responseFactory = null,
+		bool $protectRevisions = true
 	): CrawlerProtectionService {
 		$options = new ServiceOptions(
 			CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
 			[
 				'CrawlerProtectedActions' => $protectedActions,
 				'CrawlerProtectedSpecialPages' => $protectedPages,
-				'CrawlerProtectionAllowedIPs' => $allowedIPs
+				'CrawlerProtectionAllowedIPs' => $allowedIPs,
+				'CrawlerProtectionProtectRevisions' => $protectRevisions,
 			]
 		);
 
@@ -204,6 +207,36 @@ public function testCheckPerformActionBlocksConfiguredAction() {
 		$this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
 	}
 
+	/**
+	 * When CrawlerProtectionProtectRevisions is false and 'history' is not in
+	 * CrawlerProtectedActions, the revision-shaped parameters (type=revision,
+	 * diff, oldid) should be allowed. The action=history allowed-case is
+	 * covered separately by testCheckPerformActionAllowsActionNotInConfig().
+	 *
+	 * @covers ::checkPerformAction
+	 * @dataProvider provideRevisionOnlyRequestParams
+	 *
+	 * @param array $getValMap
+	 * @param string $msg
+	 */
+	public function testCheckPerformActionAllowsRevisionsWhenNotConfigured(
+		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, false );
+		$this->assertTrue( $service->checkPerformAction( $output, $user, $request ), $msg );
+	}
+
 	/**
 	 * @covers ::checkPerformAction
 	 */
@@ -227,6 +260,135 @@ public function testCheckPerformActionAllowsActionNotInConfig() {
 		$this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
 	}
 
+	/**
+	 * When CrawlerProtectionProtectRevisions is true and 'history' is NOT in
+	 * CrawlerProtectedActions, revision/diff requests should still be blocked.
+	 * This allows operators to protect individual revisions without protecting
+	 * the history listing page.
+	 *
+	 * @covers ::checkPerformAction
+	 * @dataProvider provideRevisionOnlyRequestParams
+	 *
+	 * @param array $getValMap
+	 * @param string $msg
+	 */
+	public function testCheckPerformActionBlocksRevisionsWhenProtectRevisionsTrueHistoryUnconfigured(
+		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->once() )->method( 'denyAccess' )->with( $output );
+
+		// history not in protected actions, but protectRevisions = true
+		$service = $this->buildService( [], [], [], $responseFactory, true );
+		$this->assertFalse( $service->checkPerformAction( $output, $user, $request ), $msg );
+	}
+
+	/**
+	 * Data provider for revision/diff request parameters (excludes action=history
+	 * since that is controlled independently by CrawlerProtectedActions).
+	 *
+	 * @return array
+	 */
+	public function provideRevisionOnlyRequestParams(): array {
+		return [
+			'type=revision' => [
+				[
+					[ 'type', null, 'revision' ],
+					[ 'action', null, null ],
+					[ 'diff', null, null ],
+					[ 'oldid', null, null ],
+				],
+				'type=revision should be blocked when CrawlerProtectionProtectRevisions is true',
+			],
+			'diff=42' => [
+				[
+					[ 'type', null, null ],
+					[ 'action', null, null ],
+					[ 'diff', null, '42' ],
+					[ 'oldid', null, null ],
+				],
+				'diff=42 should be blocked when CrawlerProtectionProtectRevisions is true',
+			],
+			'oldid=99' => [
+				[
+					[ 'type', null, null ],
+					[ 'action', null, null ],
+					[ 'diff', null, null ],
+					[ 'oldid', null, '99' ],
+				],
+				'oldid=99 should be blocked when CrawlerProtectionProtectRevisions is true',
+			],
+		];
+	}
+
+	/**
+	 * When CrawlerProtectionProtectRevisions is false, revision/diff requests
+	 * should be allowed even when 'history' is in CrawlerProtectedActions.
+	 *
+	 * @covers ::checkPerformAction
+	 * @dataProvider provideRevisionOnlyRequestParams
+	 *
+	 * @param array $getValMap
+	 * @param string $msg
+	 */
+	public function testCheckPerformActionAllowsRevisionsWhenProtectRevisionsFalse(
+		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' );
+
+		// history is protected, but protectRevisions = false
+		$service = $this->buildService( [], [ 'history' ], [], $responseFactory, false );
+		$this->assertTrue(
+			$service->checkPerformAction( $output, $user, $request ),
+			$msg
+		);
+	}
+
+	/**
+	 * action=history is controlled solely by CrawlerProtectedActions. It must
+	 * remain blocked even when CrawlerProtectionProtectRevisions is false.
+	 *
+	 * @covers ::checkPerformAction
+	 */
+	public function testCheckPerformActionBlocksHistoryListingEvenWhenProtectRevisionsFalse() {
+		$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( [
+			[ 'type', null, null ],
+			[ 'action', null, 'history' ],
+			[ 'diff', null, null ],
+			[ 'oldid', null, null ],
+		] );
+
+		$responseFactory = $this->createMock( ResponseFactory::class );
+		$responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+		// history is protected, but protectRevisions = false
+		$service = $this->buildService( [], [ 'history' ], [], $responseFactory, false );
+		$this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+	}
+
 	// ---------------------------------------------------------------
 	// isProtectedAction tests
 	// ---------------------------------------------------------------