diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 274526a..6c26d23 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/README.md b/README.md index 27dbe91..703544e 100644 --- a/README.md +++ b/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) diff --git a/extension.json b/extension.json index d2486d0..4b41a6a 100644 --- a/extension.json +++ b/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": [ diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php index 3df4426..fb00df6 100644 --- a/includes/CrawlerProtectionService.php +++ b/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; diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php index eff675a..b173c54 100644 --- a/tests/phpunit/namespaced-stubs.php +++ b/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 ''; + } } } diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php index 9349363..d3e7acf 100644 --- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php +++ b/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 // ---------------------------------------------------------------