diff --git a/.github/instructions/mediawiki-extensions-clean-code.instructions.md b/.github/instructions/mediawiki-extensions-clean-code.instructions.md new file mode 100644 index 0000000..507c892 --- /dev/null +++ b/.github/instructions/mediawiki-extensions-clean-code.instructions.md @@ -0,0 +1,157 @@ +--- +applyTo: "*" +--- + +# MediaWiki Extension Clean Code Best Practices + +## Service container pattern + +Use the service container pattern (based on MediaWikiServices) to define how key objects are constructed and wired together. + +Absolutely no MediaWikiServices references should be in the code because everything should be injected. + +## Dependency injection + +Use this example from PluggableAuth to implement dependency injection. + +Objects should only be instantiated in `ServiceWiring.php`, not in any other file. + +Below is an `includes/ServiceWiring.php` file. + +```php +return [ + 'PluggableAuthFactory' => + static function ( MediaWikiServices $services ): PluggableAuthFactory { + return new PluggableAuthFactory( + new ServiceOptions( PluggableAuthFactory::CONSTRUCTOR_OPTIONS, $services->getMainConfig() ), + ExtensionRegistry::getInstance(), + $services->getAuthManager(), + LoggerFactory::getInstance( 'PluggableAuth' ), + $services->getObjectFactory() + ); + }, + 'PluggableAuthService' => + static function ( MediaWikiServices $services ): PluggableAuthService { + return new PluggableAuthService( + new ServiceOptions( PluggableAuthService::CONSTRUCTOR_OPTIONS, $services->getMainConfig() ), + ExtensionRegistry::getInstance(), + $services->getUserFactory(), + $services->get( 'PluggableAuthFactory' ), + $services->get( 'PluggableAuth.GroupProcessorRunner' ), + $services->getPermissionManager(), + $services->getAuthManager(), + LoggerFactory::getInstance( 'PluggableAuth' ), + $services->getUrlUtils() + ); + }, + 'PluggableAuth.GroupProcessorFactory' => + static function ( MediaWikiServices $services ): GroupProcessorFactory { + $factory = new GroupProcessorFactory( + ExtensionRegistry::getInstance()->getAttribute( 'PluggableAuthGroupSyncs' ), + $services->getObjectFactory() + ); + $factory->setLogger( LoggerFactory::getInstance( 'PluggableAuth' ) ); + return $factory; + }, + 'PluggableAuth.GroupProcessorRunner' => + static function ( MediaWikiServices $services ): GroupProcessorRunner { + $factory = new GroupProcessorRunner( + $services->getService( 'PluggableAuth.GroupProcessorFactory' ) + ); + $factory->setLogger( LoggerFactory::getInstance( 'PluggableAuth' ) ); + return $factory; + }, +]; +``` + +The ServiceWiring.php file returns an associative array where: + +- Keys are service names (e.g., 'PluggableAuthFactory', 'PluggableAuth.GroupProcessorRunner') +- Values are factory closures that receive the MediaWikiServices container and return a fully constructed service instance +- When any code in the extension (or elsewhere) calls $services->get('PluggableAuthFactory'), MediaWiki invokes the corresponding closure, injects all dependencies, and returns the object. Services are typically lazily instantiated and cached (singleton per request). + +## Loading LocalSettings.php config values into the extension + +All configs should be loaded with dependency injection. + +## Consuming services + +### 1. Declaratively via extension.json + +#### Hook handlers + +MediaWiki's ObjectFactory reads service names from `extension.json` and automatically injects them as constructor arguments. + +```json +"HookHandlers": { + "main": { + "class": "MediaWiki\\Extension\\PluggableAuth\\PluggableAuthHooks", + "services": [ + "PluggableAuthService", + "UrlUtils" + ] + } +}, +``` + +### 2. Internally between services + +Services reference each other via `$services->get(...)`. + +For example, in the MediaWiki PluggableAuth extension: + +- PluggableAuthService depends on PluggableAuthFactory and PluggableAuth.GroupProcessorRunner +- PluggableAuth.GroupProcessorRunner depends on PluggableAuth.GroupProcessorFactory + +## Testing services + +Dependency injection means code is easily testable. Below is an example of a test file for services. + +```php +class PluggableAuthServiceTest extends MediaWikiIntegrationTestCase { + + /** + * @param array $links + * @param array $expectedLinks + * @param array $options + * @param bool $shouldOverrideDefaultLogout + * @param string $msg + * @return void + * @throws \PHPUnit\Framework\MockObject\Exception + * @covers MediaWiki\Extension\PluggableAuth\PluggableAuthService::modifyLogoutLink + * @dataProvider provideTestModifyLogoutLinkData + */ + public function testModifyLogoutLink( $links, $expectedLinks, $options, $shouldOverrideDefaultLogout, $msg ) { + $serviceOptions = new ServiceOptions( + PluggableAuthService::CONSTRUCTOR_OPTIONS, + $options + ); + $extensionRegistry = $this->createMock( ExtensionRegistry::class ); + $userFactory = $this->createMock( UserFactory::class ); + $pluggableAuthPlugin = $this->createMock( PluggableAuthPlugin::class ); + $pluggableAuthPlugin->method( 'shouldOverrideDefaultLogout' )->willReturn( $shouldOverrideDefaultLogout ); + $pluggableAuthFactory = $this->createMock( PluggableAuthFactory::class ); + $pluggableAuthFactory->method( 'getInstance' )->willReturn( $pluggableAuthPlugin ); + $groupProcessorRunner = $this->createMock( GroupProcessorRunner::class ); + $permissionManager = $this->createMock( PermissionManager::class ); + $authManager = $this->createMock( AuthManager::class ); + $logger = $this->createMock( LoggerInterface::class ); + $service = new PluggableAuthService( + $serviceOptions, + $extensionRegistry, + $userFactory, + $pluggableAuthFactory, + $groupProcessorRunner, + $permissionManager, + $authManager, + $logger, + $this->getServiceContainer()->getUrlUtils() + ); + + $service->modifyLogoutLink( $links ); + + $this->assertEquals( $expectedLinks, $links, $msg ); + } + +} +``` diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d1505a5..274526a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,9 +25,6 @@ jobs: php: [ '8.2', '8.3', '8.4' ] mediawiki: [ REL1_43 ] include: - - os: ubuntu-latest - php: '7.4' - mediawiki: REL1_35 - os: ubuntu-latest php: '7.4' mediawiki: REL1_39 @@ -52,8 +49,9 @@ jobs: with: path: extensions/${{ env.EXTNAME }} - 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"]}}}' > composer.local.json + echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}},"config":{"audit":{"abandoned":"report","block-insecure":false}}}' > composer.local.json composer update composer update - name: Lint @@ -72,9 +70,6 @@ jobs: php: [ '8.2', '8.3' ] mediawiki: [ REL1_43 ] include: - - os: ubuntu-latest - php: '7.4' - mediawiki: REL1_35 - os: ubuntu-latest php: '7.4' mediawiki: REL1_39 @@ -99,8 +94,9 @@ jobs: with: path: extensions/${{ env.EXTNAME }} - 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"]}}}' > composer.local.json + echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}},"config":{"audit":{"abandoned":"report","block-insecure":false}}}' > composer.local.json composer update composer update - name: Phan @@ -115,9 +111,6 @@ jobs: php: [ '8.2', '8.3', '8.4' ] mediawiki: [ REL1_43 ] include: - - os: ubuntu-latest - php: '7.4' - mediawiki: REL1_35 - os: ubuntu-latest php: '7.4' mediawiki: REL1_39 @@ -142,8 +135,9 @@ jobs: with: path: extensions/${{ env.EXTNAME }} - 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"]}}}' > composer.local.json + echo '{"extra":{"merge-plugin":{"include":["extensions/*/composer.json","skins/*/composer.json"]}},"config":{"audit":{"abandoned":"report","block-insecure":false}}}' > composer.local.json composer update composer update - name: Install MediaWiki diff --git a/extension.json b/extension.json index b09e568..849dda5 100644 --- a/extension.json +++ b/extension.json @@ -1,11 +1,11 @@ { "name": "CrawlerProtection", "author": "MyWikis LLC", - "version": "1.1.0", + "version": "1.2.0", "description": "Suite of protective measures to protect wikis from crawlers.", "type": "hook", "requires": { - "MediaWiki": ">= 1.35" + "MediaWiki": ">= 1.39.4" }, "AutoloadNamespaces": { "MediaWiki\\Extension\\CrawlerProtection\\": "includes/" @@ -13,7 +13,9 @@ "HookHandlers": { "main": { "class": "MediaWiki\\Extension\\CrawlerProtection\\Hooks", - "services": [] + "services": [ + "CrawlerProtection.CrawlerProtectionService" + ] } }, "Hooks": { @@ -28,10 +30,22 @@ "whatlinkshere" ] }, + "CrawlerProtectionRawDenial": { + "value": false + }, "CrawlerProtectionUse418": { "value": false + }, + "CrawlerProtectionRawDenialHeader": { + "value": "HTTP/1.0 403 Forbidden" + }, + "CrawlerProtectionRawDenialText": { + "value": "403 Forbidden. You must be logged in to view this page." } }, + "ServiceWiringFiles": [ + "includes/ServiceWiring.php" + ], "license-name": "MIT", "Tests": { "phpunit": "tests/phpunit" diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php new file mode 100644 index 0000000..43feda7 --- /dev/null +++ b/includes/CrawlerProtectionService.php @@ -0,0 +1,166 @@ +assertRequiredOptions( self::CONSTRUCTOR_OPTIONS ); + $this->options = $options; + $this->responseFactory = $responseFactory; + } + + /** + * Check whether a regular page request should be blocked. + * + * Returns false (= abort further processing) when the request is + * blocked, true otherwise. + * + * @param OutputPage $output + * @param User $user + * @param WebRequest $request + * @return bool + */ + public function checkPerformAction( + $output, + $user, + $request + ): bool { + if ( $user->isRegistered() ) { + return true; + } + + $type = $request->getVal( 'type' ); + $action = $request->getVal( 'action' ); + $diffId = (int)$request->getVal( 'diff' ); + $oldId = (int)$request->getVal( 'oldid' ); + + if ( + $type === 'revision' + || $action === 'history' + || $diffId > 0 + || $oldId > 0 + ) { + $this->responseFactory->denyAccess( $output ); + return false; + } + + return true; + } + + /** + * Check whether a special page request should be blocked. + * + * Returns false (= abort further processing) when the request is + * blocked, true otherwise. + * + * @param string $specialPageName The canonical special page name + * @param OutputPage $output + * @param User $user + * @return bool + */ + public function checkSpecialPage( + string $specialPageName, + $output, + $user + ): bool { + if ( $user->isRegistered() ) { + return true; + } + + if ( $this->isProtectedSpecialPage( $specialPageName ) ) { + $this->responseFactory->denyAccess( $output ); + return false; + } + + return true; + } + + /** + * Determine whether the given special page name is in the + * configured list of protected special pages. + * + * Because this method is only called from the SpecialPageBeforeExecute + * hook, any "Foo:" prefix on a configured value is necessarily the + * "Special" namespace in English or its localized equivalent (e.g. + * "Spezial:" in German). We therefore simply strip everything up to + * and including the first colon rather than checking for a specific + * namespace name, which keeps the logic language-agnostic. + * + * @param string $specialPageName + * @return bool + */ + public function isProtectedSpecialPage( string $specialPageName ): bool { + $protectedSpecialPages = $this->options->get( 'CrawlerProtectedSpecialPages' ); + + // Normalize protected special pages: lowercase and strip any + // namespace prefix (everything up to and including the first ':'). + $normalizedProtectedPages = array_map( + static function ( string $p ): string { + $lower = strtolower( $p ); + $colonPos = strpos( $lower, ':' ); + if ( $colonPos !== false ) { + return substr( $lower, $colonPos + 1 ); + } + return $lower; + }, + $protectedSpecialPages + ); + + $name = strtolower( $specialPageName ); + + return in_array( $name, $normalizedProtectedPages, true ); + } +} diff --git a/includes/Hooks.php b/includes/Hooks.php index a2d9bba..fa8329f 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -1,13 +1,33 @@ crawlerProtectionService = $crawlerProtectionService; + } /** * Block sensitive page views for anonymous users via MediaWikiPerformAction. - * Handles: - * - ?type=revision - * - ?action=history - * - ?diff=1234 - * - ?oldid=1234 - * - * Special pages (e.g. Special:WhatLinksHere) are handled separately. * * @param OutputPage $output * @param Article $article @@ -65,92 +92,25 @@ public function onMediaWikiPerformAction( $request, $mediaWiki ) { - $type = $request->getVal( 'type' ); - $action = $request->getVal( 'action' ); - $diffId = (int)$request->getVal( 'diff' ); - $oldId = (int)$request->getVal( 'oldid' ); - - if ( - !$user->isRegistered() - && ( - $type === 'revision' - || $action === 'history' - || $diffId > 0 - || $oldId > 0 - ) - ) { - $this->denyAccess( $output ); - return false; - } - - return true; + return $this->crawlerProtectionService->checkPerformAction( + $output, + $user, + $request + ); } /** - * Block Special:RecentChangesLinked, Special:WhatLinksHere, and Special:MobileDiff for anonymous users. + * Block protected special pages for anonymous users. * * @param SpecialPage $special * @param string|null $subPage * @return bool False to abort execution */ public function onSpecialPageBeforeExecute( $special, $subPage ) { - $user = $special->getContext()->getUser(); - if ( $user->isRegistered() ) { - // logged-in users: allow - return true; - } - - $config = MediaWikiServices::getInstance()->getMainConfig(); - $protectedSpecialPages = $config->get( 'CrawlerProtectedSpecialPages' ); - $denyFast = $config->get( 'CrawlerProtectionUse418' ); - - // Normalize protected special pages: lowercase and strip 'Special:' prefix - $normalizedProtectedPages = array_map( - fn ( $p ) => ( $p = strtolower( $p ) ) && strpos( $p, strtolower( self::SPECIAL_PAGE_PREFIX ) ) === 0 - ? substr( $p, 8 ) - : $p, - $protectedSpecialPages + return $this->crawlerProtectionService->checkSpecialPage( + $special->getName(), + $special->getContext()->getOutput(), + $special->getContext()->getUser() ); - - $name = strtolower( $special->getName() ); - if ( in_array( $name, $normalizedProtectedPages, true ) ) { - $out = $special->getContext()->getOutput(); - if ( $denyFast ) { - $this->denyAccessWith418(); - } - $this->denyAccess( $out ); - return false; - } - - return true; - } - - /** - * Helper: output 418 Teapot and halt the processing immediately - * - * @return void - * @suppress PhanPluginNeverReturnMethod - */ - protected function denyAccessWith418() { - header( 'HTTP/1.0 I\'m a teapot' ); - die( 'I\'m a teapot' ); - } - - /** - * Helper: output 403 Access Denied page using i18n messages. - * - * @param OutputPage $output - * @return void - */ - protected function denyAccess( $output ): void { - $output->setStatusCode( 403 ); - $output->addWikiTextAsInterface( wfMessage( 'crawlerprotection-accessdenied-text' )->plain() ); - - if ( version_compare( MW_VERSION, '1.41', '<' ) ) { - $output->setPageTitle( wfMessage( 'crawlerprotection-accessdenied-title' ) ); - } else { - // @phan-suppress-next-line PhanUndeclaredMethod Exists in 1.41+ - $output->setPageTitleMsg( wfMessage( 'crawlerprotection-accessdenied-title' ) ); - } } } diff --git a/includes/ResponseFactory.php b/includes/ResponseFactory.php new file mode 100644 index 0000000..9da0abb --- /dev/null +++ b/includes/ResponseFactory.php @@ -0,0 +1,134 @@ +assertRequiredOptions( self::CONSTRUCTOR_OPTIONS ); + $this->options = $options; + } + + /** + * Deny access using the configured strategy. + * + * When CrawlerProtectionRawDenial is enabled, a raw HTTP response is + * sent. If CrawlerProtectionUse418 is *also* enabled the response + * uses the 418 "I'm a teapot" status; otherwise the configured + * header / body are used. + * + * When CrawlerProtectionRawDenial is disabled, a pretty 403 page is + * rendered through OutputPage regardless of the Use418 setting. + * + * @param OutputPage $output Used only for the "pretty" strategy + * @return void + */ + public function denyAccess( $output ): void { + if ( $this->options->get( 'CrawlerProtectionRawDenial' ) ) { + if ( $this->options->get( 'CrawlerProtectionUse418' ) ) { + $this->denyAccessWith418(); + } else { + $this->denyAccessRaw( + $this->options->get( 'CrawlerProtectionRawDenialHeader' ), + $this->options->get( 'CrawlerProtectionRawDenialText' ) + ); + } + } else { + $this->denyAccessPretty( $output ); + } + } + + /** + * Output a 418 "I'm a teapot" response and halt. + * + * @return void + * @suppress PhanPluginNeverReturnMethod + */ + protected function denyAccessWith418(): void { + $this->denyAccessRaw( self::TEAPOT_HEADER, self::TEAPOT_BODY ); + } + + /** + * Output a raw HTTP response and halt. + * + * @param string $header + * @param string $message + * @return void + * @suppress PhanPluginNeverReturnMethod + */ + protected function denyAccessRaw( string $header, string $message ): void { + header( $header ); + die( $message ); + } + + /** + * Output a pretty 403 Access Denied page using i18n messages. + * + * @param OutputPage $output + * @return void + */ + protected function denyAccessPretty( $output ): void { + $output->setStatusCode( 403 ); + $output->addWikiTextAsInterface( + wfMessage( 'crawlerprotection-accessdenied-text' )->plain() + ); + + if ( version_compare( MW_VERSION, '1.41', '<' ) ) { + $output->setPageTitle( wfMessage( 'crawlerprotection-accessdenied-title' ) ); + } else { + // @phan-suppress-next-line PhanUndeclaredMethod Exists in 1.41+ + $output->setPageTitleMsg( wfMessage( 'crawlerprotection-accessdenied-title' ) ); + } + } +} diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php new file mode 100644 index 0000000..bf53d6e --- /dev/null +++ b/includes/ServiceWiring.php @@ -0,0 +1,51 @@ + + static function ( MediaWikiServices $services ): ResponseFactory { + return new ResponseFactory( + new ServiceOptions( + ResponseFactory::CONSTRUCTOR_OPTIONS, + $services->getMainConfig() + ) + ); + }, + 'CrawlerProtection.CrawlerProtectionService' => + static function ( MediaWikiServices $services ): CrawlerProtectionService { + return new CrawlerProtectionService( + new ServiceOptions( + CrawlerProtectionService::CONSTRUCTOR_OPTIONS, + $services->getMainConfig() + ), + $services->get( 'CrawlerProtection.ResponseFactory' ) + ); + }, +]; diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php index 1dfdf57..eff675a 100644 --- a/tests/phpunit/namespaced-stubs.php +++ b/tests/phpunit/namespaced-stubs.php @@ -16,6 +16,59 @@ public function onSpecialPageBeforeExecute( $special, $subPage ); } } +// ServiceOptions stub +namespace MediaWiki\Config { + class ServiceOptions { + /** @var array */ + private array $options = []; + + /** + * @param string[] $keys + * @param mixed ...$sources + */ + public function __construct( array $keys, ...$sources ) { + foreach ( $sources as $source ) { + if ( $source instanceof Config ) { + foreach ( $keys as $key ) { + if ( !array_key_exists( $key, $this->options ) ) { + $this->options[$key] = $source->get( $key ); + } + } + } elseif ( is_array( $source ) ) { + foreach ( $keys as $key ) { + if ( !array_key_exists( $key, $this->options ) && array_key_exists( $key, $source ) ) { + $this->options[$key] = $source[$key]; + } + } + } + } + } + + /** + * @param string[] $expectedKeys + */ + public function assertRequiredOptions( array $expectedKeys ): void { + // no-op in tests + } + + /** + * @param string $key + * @return mixed + */ + public function get( string $key ) { + return $this->options[$key] ?? null; + } + } + + interface Config { + /** + * @param string $name + * @return mixed + */ + public function get( $name ); + } +} + // Core classes in their proper namespaces namespace MediaWiki\Output { class OutputPage { @@ -76,24 +129,11 @@ class ActionEntryPoint { } } -namespace MediaWiki\Config { - interface Config { - /** - * @param string $name - * @return mixed - */ - public function get( $name ); - } -} - namespace MediaWiki { class MediaWikiServices { /** @var MediaWikiServices|null */ private static $instance = null; - /** @var bool Control CrawlerProtectionUse418 config for testing */ - public static $testUse418 = false; - /** * @return MediaWikiServices */ @@ -130,19 +170,6 @@ public function getMainConfig() { * @return mixed */ public function get( $name ) { - if ( $name === 'CrawlerProtectedSpecialPages' ) { - return [ - 'RecentChangesLinked', - 'WhatLinksHere', - 'MobileDiff', - 'recentchangeslinked', - 'whatlinkshere', - 'mobilediff' - ]; - } - if ( $name === 'CrawlerProtectionUse418' ) { - return MediaWikiServices::$testUse418; - } return null; } }; diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php new file mode 100644 index 0000000..1e92f5e --- /dev/null +++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php @@ -0,0 +1,302 @@ + $protectedPages ] + ); + + $responseFactory ??= $this->createMock( ResponseFactory::class ); + + return new CrawlerProtectionService( $options, $responseFactory ); + } + + // --------------------------------------------------------------- + // checkPerformAction tests + // --------------------------------------------------------------- + + /** + * @covers ::checkPerformAction + */ + public function testCheckPerformActionAllowsRegisteredUser() { + $output = $this->createMock( self::$outputPageClassName ); + $user = $this->createMock( self::$userClassName ); + $user->method( 'isRegistered' )->willReturn( true ); + + $request = $this->createMock( self::$webRequestClassName ); + $request->method( 'getVal' )->willReturnMap( [ + [ 'type', null, 'revision' ], + ] ); + + $responseFactory = $this->createMock( ResponseFactory::class ); + $responseFactory->expects( $this->never() )->method( 'denyAccess' ); + + $service = $this->buildService( [], $responseFactory ); + $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) ); + } + + /** + * @covers ::checkPerformAction + * @dataProvider provideBlockedRequestParams + * + * @param array $getValMap + * @param string $msg + */ + public function testCheckPerformActionBlocksAnonymous( array $getValMap, string $msg ) { + $output = $this->createMock( self::$outputPageClassName ); + $user = $this->createMock( self::$userClassName ); + $user->method( 'isRegistered' )->willReturn( false ); + + $request = $this->createMock( self::$webRequestClassName ); + $request->method( 'getVal' )->willReturnMap( $getValMap ); + + $responseFactory = $this->createMock( ResponseFactory::class ); + $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output ); + + $service = $this->buildService( [], $responseFactory ); + $this->assertFalse( $service->checkPerformAction( $output, $user, $request ), $msg ); + } + + /** + * Data provider for request parameters that should trigger a block. + * + * @return array + */ + public function provideBlockedRequestParams(): array { + return [ + 'type=revision' => [ + [ + [ 'type', null, 'revision' ], + [ 'action', null, null ], + [ 'diff', null, null ], + [ 'oldid', null, null ], + ], + 'type=revision should be blocked', + ], + 'action=history' => [ + [ + [ 'type', null, null ], + [ 'action', null, 'history' ], + [ 'diff', null, null ], + [ 'oldid', null, null ], + ], + 'action=history should be blocked', + ], + 'diff=42' => [ + [ + [ 'type', null, null ], + [ 'action', null, null ], + [ 'diff', null, '42' ], + [ 'oldid', null, null ], + ], + 'diff=42 should be blocked', + ], + 'oldid=99' => [ + [ + [ 'type', null, null ], + [ 'action', null, null ], + [ 'diff', null, null ], + [ 'oldid', null, '99' ], + ], + 'oldid=99 should be blocked', + ], + ]; + } + + /** + * @covers ::checkPerformAction + */ + public function testCheckPerformActionAllowsNormalAnonymousView() { + $output = $this->createMock( self::$outputPageClassName ); + $user = $this->createMock( self::$userClassName ); + $user->method( 'isRegistered' )->willReturn( false ); + + $request = $this->createMock( self::$webRequestClassName ); + $request->method( 'getVal' )->willReturnMap( [ + [ 'type', null, null ], + [ 'action', null, 'view' ], + [ 'diff', null, null ], + [ 'oldid', null, null ], + ] ); + + $responseFactory = $this->createMock( ResponseFactory::class ); + $responseFactory->expects( $this->never() )->method( 'denyAccess' ); + + $service = $this->buildService( [], $responseFactory ); + $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) ); + } + + // --------------------------------------------------------------- + // checkSpecialPage tests + // --------------------------------------------------------------- + + /** + * @covers ::checkSpecialPage + * @dataProvider provideBlockedSpecialPages + * + * @param string $specialPageName + */ + public function testCheckSpecialPageBlocksAnonymous( string $specialPageName ) { + $output = $this->createMock( self::$outputPageClassName ); + $user = $this->createMock( self::$userClassName ); + $user->method( 'isRegistered' )->willReturn( false ); + + $responseFactory = $this->createMock( ResponseFactory::class ); + $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output ); + + $service = $this->buildService( + [ 'RecentChangesLinked', 'WhatLinksHere', 'MobileDiff' ], + $responseFactory + ); + $this->assertFalse( $service->checkSpecialPage( $specialPageName, $output, $user ) ); + } + + /** + * @covers ::checkSpecialPage + * @dataProvider provideBlockedSpecialPages + * + * @param string $specialPageName + */ + public function testCheckSpecialPageAllowsRegistered( string $specialPageName ) { + $output = $this->createMock( self::$outputPageClassName ); + $user = $this->createMock( self::$userClassName ); + $user->method( 'isRegistered' )->willReturn( true ); + + $responseFactory = $this->createMock( ResponseFactory::class ); + $responseFactory->expects( $this->never() )->method( 'denyAccess' ); + + $service = $this->buildService( + [ 'RecentChangesLinked', 'WhatLinksHere', 'MobileDiff' ], + $responseFactory + ); + $this->assertTrue( $service->checkSpecialPage( $specialPageName, $output, $user ) ); + } + + /** + * @covers ::checkSpecialPage + */ + public function testCheckSpecialPageAllowsUnprotected() { + $output = $this->createMock( self::$outputPageClassName ); + $user = $this->createMock( self::$userClassName ); + $user->method( 'isRegistered' )->willReturn( false ); + + $responseFactory = $this->createMock( ResponseFactory::class ); + $responseFactory->expects( $this->never() )->method( 'denyAccess' ); + + $service = $this->buildService( + [ 'RecentChangesLinked', 'WhatLinksHere', 'MobileDiff' ], + $responseFactory + ); + $this->assertTrue( $service->checkSpecialPage( 'Search', $output, $user ) ); + } + + // --------------------------------------------------------------- + // isProtectedSpecialPage tests + // --------------------------------------------------------------- + + /** + * @covers ::isProtectedSpecialPage + */ + public function testIsProtectedSpecialPageStripsPrefix() { + $service = $this->buildService( [ 'Special:WhatLinksHere' ] ); + $this->assertTrue( $service->isProtectedSpecialPage( 'WhatLinksHere' ) ); + } + + /** + * @covers ::isProtectedSpecialPage + */ + public function testIsProtectedSpecialPageIsCaseInsensitive() { + $service = $this->buildService( [ 'WhatLinksHere' ] ); + $this->assertTrue( $service->isProtectedSpecialPage( 'whatlinkshere' ) ); + $this->assertTrue( $service->isProtectedSpecialPage( 'WHATLINKSHERE' ) ); + $this->assertTrue( $service->isProtectedSpecialPage( 'WhAtLiNkShErE' ) ); + } + + /** + * @covers ::isProtectedSpecialPage + */ + public function testIsProtectedSpecialPageReturnsFalseForUnprotected() { + $service = $this->buildService( [ 'WhatLinksHere' ] ); + $this->assertFalse( $service->isProtectedSpecialPage( 'Search' ) ); + } + + /** + * @covers ::isProtectedSpecialPage + */ + public function testIsProtectedSpecialPageStripsLocalizedPrefix() { + $service = $this->buildService( [ 'Spezial:WhatLinksHere' ] ); + $this->assertTrue( $service->isProtectedSpecialPage( 'WhatLinksHere' ) ); + } + + /** + * @covers ::isProtectedSpecialPage + */ + public function testIsProtectedSpecialPageStripsAnyPrefix() { + $service = $this->buildService( [ 'Especial:WhatLinksHere' ] ); + $this->assertTrue( $service->isProtectedSpecialPage( 'WhatLinksHere' ) ); + } + + /** + * Data provider for blocked special pages. + * + * @return array + */ + public function provideBlockedSpecialPages(): array { + return [ + 'RecentChangesLinked' => [ 'RecentChangesLinked' ], + 'WhatLinksHere' => [ 'WhatLinksHere' ], + 'MobileDiff' => [ 'MobileDiff' ], + 'RecentChangesLinked lowercase' => [ 'recentchangeslinked' ], + 'WhatLinksHere lowercase' => [ 'whatlinkshere' ], + 'MobileDiff lowercase' => [ 'mobilediff' ], + 'MobileDiff mixed case' => [ 'MoBiLeDiFf' ], + ]; + } +} diff --git a/tests/phpunit/unit/HooksTest.php b/tests/phpunit/unit/HooksTest.php index 7345cb4..aead599 100644 --- a/tests/phpunit/unit/HooksTest.php +++ b/tests/phpunit/unit/HooksTest.php @@ -2,6 +2,7 @@ namespace MediaWiki\Extension\CrawlerProtection\Tests; +use MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService; use MediaWiki\Extension\CrawlerProtection\Hooks; use PHPUnit\Framework\TestCase; @@ -62,185 +63,86 @@ public static function setUpBeforeClass(): void { : '\WebRequest'; } - /** - * Reset test state after each test to prevent test pollution - * - * @return void - */ - protected function tearDown(): void { - parent::tearDown(); - // Reset the test config flag (only exists in stub environment) - if ( property_exists( '\MediaWiki\MediaWikiServices', 'testUse418' ) ) { - \MediaWiki\MediaWikiServices::$testUse418 = false; - } - // Only reset if the method exists (in our test stubs) - if ( method_exists( '\MediaWiki\MediaWikiServices', 'resetForTesting' ) ) { - \MediaWiki\MediaWikiServices::resetForTesting(); - } - } - /** * @covers ::onMediaWikiPerformAction */ - public function testRevisionTypeBlocksAnonymous() { + public function testOnMediaWikiPerformActionDelegatesToService() { $output = $this->createMock( self::$outputPageClassName ); - - $request = $this->createMock( self::$webRequestClassName ); - $request->method( 'getVal' )->willReturnMap( [ - [ 'type', null, 'revision' ], - ] ); - $user = $this->createMock( self::$userClassName ); - $user->method( 'isRegistered' )->willReturn( false ); - + $request = $this->createMock( self::$webRequestClassName ); $article = $this->createMock( self::$articleClassName ); $title = $this->createMock( self::$titleClassName ); $wiki = $this->createMock( self::$actionEntryPointClassName ); - $runner = $this->getMockBuilder( Hooks::class ) - ->onlyMethods( [ 'denyAccess' ] ) - ->getMock(); - $runner->expects( $this->once() )->method( 'denyAccess' ); + $service = $this->createMock( CrawlerProtectionService::class ); + $service->expects( $this->once() ) + ->method( 'checkPerformAction' ) + ->with( $output, $user, $request ) + ->willReturn( false ); + + $hooks = new Hooks( $service ); + $result = $hooks->onMediaWikiPerformAction( + $output, $article, $title, $user, $request, $wiki + ); - $result = $runner->onMediaWikiPerformAction( $output, $article, $title, $user, $request, $wiki ); $this->assertFalse( $result ); } /** * @covers ::onMediaWikiPerformAction */ - public function testRevisionTypeAllowsLoggedIn() { + public function testOnMediaWikiPerformActionPassesThroughTrue() { $output = $this->createMock( self::$outputPageClassName ); - - $request = $this->createMock( self::$webRequestClassName ); - $request->method( 'getVal' )->willReturnMap( [ - [ 'type', null, 'revision' ], - ] ); - $user = $this->createMock( self::$userClassName ); - $user->method( 'isRegistered' )->willReturn( true ); - - $article = $this->createMock( self::$articleClassName ); - $title = $this->createMock( self::$titleClassName ); - $wiki = $this->createMock( self::$actionEntryPointClassName ); - - $runner = $this->getMockBuilder( Hooks::class ) - ->onlyMethods( [ 'denyAccess' ] ) - ->getMock(); - $runner->expects( $this->never() )->method( 'denyAccess' ); - - $result = $runner->onMediaWikiPerformAction( $output, $article, $title, $user, $request, $wiki ); - $this->assertTrue( $result ); - } - - /** - * @covers ::onMediaWikiPerformAction - */ - public function testNonRevisionTypeAlwaysAllowed() { - $output = $this->createMock( self::$outputPageClassName ); - $request = $this->createMock( self::$webRequestClassName ); - $request->method( 'getVal' )->willReturnMap( [ - [ 'type', null, 'view' ], - ] ); - - $user = $this->createMock( self::$userClassName ); - $user->method( 'isRegistered' )->willReturn( false ); - $article = $this->createMock( self::$articleClassName ); $title = $this->createMock( self::$titleClassName ); $wiki = $this->createMock( self::$actionEntryPointClassName ); - $runner = $this->getMockBuilder( Hooks::class ) - ->onlyMethods( [ 'denyAccess' ] ) - ->getMock(); - $runner->expects( $this->never() )->method( 'denyAccess' ); + $service = $this->createMock( CrawlerProtectionService::class ); + $service->expects( $this->once() ) + ->method( 'checkPerformAction' ) + ->willReturn( true ); + + $hooks = new Hooks( $service ); + $result = $hooks->onMediaWikiPerformAction( + $output, $article, $title, $user, $request, $wiki + ); - $result = $runner->onMediaWikiPerformAction( $output, $article, $title, $user, $request, $wiki ); $this->assertTrue( $result ); } /** * @covers ::onSpecialPageBeforeExecute - * @dataProvider provideBlockedSpecialPages - * @param string $specialPageName */ - public function testSpecialPageBlocksAnonymous( $specialPageName ) { - // Skip this test in MediaWiki environment - it requires service container - if ( !property_exists( '\MediaWiki\MediaWikiServices', 'testUse418' ) ) { - $this->markTestSkipped( - 'Test requires stub MediaWikiServices. Skipped in MediaWiki unit test environment.' - ); - } - + public function testOnSpecialPageBeforeExecuteDelegatesToService() { $output = $this->createMock( self::$outputPageClassName ); - $user = $this->createMock( self::$userClassName ); - $user->method( 'isRegistered' )->willReturn( false ); $context = $this->createMockContext( $user, $output ); $special = $this->createMock( self::$specialPageClassName ); - $special->method( 'getName' )->willReturn( $specialPageName ); + $special->method( 'getName' )->willReturn( 'WhatLinksHere' ); $special->method( 'getContext' )->willReturn( $context ); - $runner = $this->getMockBuilder( Hooks::class ) - ->onlyMethods( [ 'denyAccess', 'denyAccessWith418' ] ) - ->getMock(); - $runner->expects( $this->once() )->method( 'denyAccess' )->with( $output ); - - $result = $runner->onSpecialPageBeforeExecute( $special, null ); - $this->assertFalse( $result ); - } - - /** - * @covers ::onSpecialPageBeforeExecute - * @dataProvider provideBlockedSpecialPages - * @param string $specialPageName - */ - public function testSpecialPageAllowsLoggedIn( $specialPageName ) { - // Skip this test in MediaWiki environment - it requires service container - if ( !property_exists( '\MediaWiki\MediaWikiServices', 'testUse418' ) ) { - $this->markTestSkipped( - 'Test requires stub MediaWikiServices. Skipped in MediaWiki unit test environment.' - ); - } - - $output = $this->createMock( self::$outputPageClassName ); - - $user = $this->createMock( self::$userClassName ); - $user->method( 'isRegistered' )->willReturn( true ); - - $context = $this->createMockContext( $user, $output ); - - $special = $this->createMock( self::$specialPageClassName ); - $special->method( 'getName' )->willReturn( $specialPageName ); - $special->method( 'getContext' )->willReturn( $context ); + $service = $this->createMock( CrawlerProtectionService::class ); + $service->expects( $this->once() ) + ->method( 'checkSpecialPage' ) + ->with( 'WhatLinksHere', $output, $user ) + ->willReturn( false ); - $runner = $this->getMockBuilder( Hooks::class ) - ->onlyMethods( [ 'denyAccess' ] ) - ->getMock(); - $runner->expects( $this->never() )->method( 'denyAccess' ); + $hooks = new Hooks( $service ); + $result = $hooks->onSpecialPageBeforeExecute( $special, null ); - $result = $runner->onSpecialPageBeforeExecute( $special, null ); - $this->assertTrue( $result ); + $this->assertFalse( $result ); } /** * @covers ::onSpecialPageBeforeExecute */ - public function testUnblockedSpecialPageAllowsAnonymous() { - // Skip this test in MediaWiki environment - it requires service container - if ( !property_exists( '\MediaWiki\MediaWikiServices', 'testUse418' ) ) { - $this->markTestSkipped( - 'Test requires stub MediaWikiServices. Skipped in MediaWiki unit test environment.' - ); - } - + public function testOnSpecialPageBeforeExecutePassesThroughTrue() { $output = $this->createMock( self::$outputPageClassName ); - $user = $this->createMock( self::$userClassName ); - $user->method( 'isRegistered' )->willReturn( false ); $context = $this->createMockContext( $user, $output ); @@ -248,12 +150,14 @@ public function testUnblockedSpecialPageAllowsAnonymous() { $special->method( 'getName' )->willReturn( 'Search' ); $special->method( 'getContext' )->willReturn( $context ); - $runner = $this->getMockBuilder( Hooks::class ) - ->onlyMethods( [ 'denyAccess' ] ) - ->getMock(); - $runner->expects( $this->never() )->method( 'denyAccess' ); + $service = $this->createMock( CrawlerProtectionService::class ); + $service->expects( $this->once() ) + ->method( 'checkSpecialPage' ) + ->willReturn( true ); + + $hooks = new Hooks( $service ); + $result = $hooks->onSpecialPageBeforeExecute( $special, null ); - $result = $runner->onSpecialPageBeforeExecute( $special, null ); $this->assertTrue( $result ); } @@ -265,7 +169,7 @@ public function testUnblockedSpecialPageAllowsAnonymous() { * @return \stdClass Mock context */ private function createMockContext( $user, $output ) { - $context = new class( $user, $output ) { + return new class( $user, $output ) { /** @var \PHPUnit\Framework\MockObject\MockObject */ private $user; /** @var \PHPUnit\Framework\MockObject\MockObject */ @@ -294,61 +198,5 @@ public function getOutput() { return $this->output; } }; - return $context; - } - - /** - * @covers ::onSpecialPageBeforeExecute - * @covers ::denyAccessWith418 - */ - public function testSpecialPageCallsDenyAccessWith418WhenConfigured() { - // This test only works with our test stubs, not in MediaWiki's PHPUnit environment - if ( !property_exists( '\MediaWiki\MediaWikiServices', 'testUse418' ) ) { - $this->markTestSkipped( - 'Test requires stub MediaWikiServices. Skipped in MediaWiki integration tests.' - ); - } - - // Enable 418 response in the test stub config - \MediaWiki\MediaWikiServices::$testUse418 = true; - - $output = $this->createMock( self::$outputPageClassName ); - - $user = $this->createMock( self::$userClassName ); - $user->method( 'isRegistered' )->willReturn( false ); - - $context = $this->createMockContext( $user, $output ); - - $special = $this->createMock( self::$specialPageClassName ); - $special->method( 'getName' )->willReturn( 'WhatLinksHere' ); - $special->method( 'getContext' )->willReturn( $context ); - - $runner = $this->getMockBuilder( Hooks::class ) - ->onlyMethods( [ 'denyAccessWith418' ] ) - ->getMock(); - // When denyFast is true, only denyAccessWith418 is called (it dies before denyAccess) - $runner->expects( $this->once() )->method( 'denyAccessWith418' ); - - $result = $runner->onSpecialPageBeforeExecute( $special, null ); - $this->assertFalse( $result ); - } - - /** - * Data provider for blocked special pages. - * - * @return array - */ - public function provideBlockedSpecialPages() { - return [ - 'RecentChangesLinked' => [ 'RecentChangesLinked' ], - 'WhatLinksHere' => [ 'WhatLinksHere' ], - 'MobileDiff' => [ 'MobileDiff' ], - // Test case sensitivity - 'RecentChangesLinked lowercase' => [ 'recentchangeslinked' ], - 'WhatLinksHere lowercase' => [ 'whatlinkshere' ], - 'MobileDiff lowercase' => [ 'mobilediff' ], - // Test mixed case - 'MobileDiff mixed case' => [ 'MoBiLeDiFf' ], - ]; } } diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php new file mode 100644 index 0000000..1068fb0 --- /dev/null +++ b/tests/phpunit/unit/ResponseFactoryTest.php @@ -0,0 +1,173 @@ + false, + 'CrawlerProtectionRawDenial' => false, + 'CrawlerProtectionRawDenialHeader' => 'HTTP/1.0 403 Forbidden', + 'CrawlerProtectionRawDenialText' => '403 Forbidden', + ]; + + $config = array_merge( $defaults, $overrides ); + + return new ResponseFactory( + new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, $config ) + ); + } + + /** + * @covers ::denyAccess + * @covers ::denyAccessPretty + */ + public function testDenyAccessPrettySetStatusCode() { + if ( defined( 'MEDIAWIKI' ) ) { + $this->markTestSkipped( + 'Skipped in MediaWiki integration environment: wfMessage() requires service container' + ); + } + + $output = $this->createMock( self::$outputPageClassName ); + $output->expects( $this->once() ) + ->method( 'setStatusCode' ) + ->with( 403 ); + $output->expects( $this->once() ) + ->method( 'addWikiTextAsInterface' ); + + $factory = $this->buildFactory(); + $factory->denyAccess( $output ); + } + + /** + * @covers ::denyAccess + */ + public function testDenyAccessChooses418WhenBothRawAndUse418() { + $factory = $this->getMockBuilder( ResponseFactory::class ) + ->setConstructorArgs( [ + new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [ + 'CrawlerProtectionUse418' => true, + 'CrawlerProtectionRawDenial' => true, + 'CrawlerProtectionRawDenialHeader' => '', + 'CrawlerProtectionRawDenialText' => '', + ] ) + ] ) + ->onlyMethods( [ 'denyAccessWith418' ] ) + ->getMock(); + + $factory->expects( $this->once() )->method( 'denyAccessWith418' ); + + $output = $this->createMock( self::$outputPageClassName ); + $factory->denyAccess( $output ); + } + + /** + * When RawDenial is false, Use418 should be a no-op and the factory + * must fall through to the pretty 403 page. + * + * @covers ::denyAccess + */ + public function testDenyAccessIgnoresUse418WhenRawDenialDisabled() { + $factory = $this->getMockBuilder( ResponseFactory::class ) + ->setConstructorArgs( [ + new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [ + 'CrawlerProtectionUse418' => true, + 'CrawlerProtectionRawDenial' => false, + 'CrawlerProtectionRawDenialHeader' => '', + 'CrawlerProtectionRawDenialText' => '', + ] ) + ] ) + ->onlyMethods( [ 'denyAccessPretty', 'denyAccessWith418' ] ) + ->getMock(); + + $factory->expects( $this->never() )->method( 'denyAccessWith418' ); + + $output = $this->createMock( self::$outputPageClassName ); + $factory->expects( $this->once() ) + ->method( 'denyAccessPretty' ) + ->with( $output ); + + $factory->denyAccess( $output ); + } + + /** + * @covers ::denyAccess + */ + public function testDenyAccessChoosesRawWhenConfigured() { + $factory = $this->getMockBuilder( ResponseFactory::class ) + ->setConstructorArgs( [ + new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [ + 'CrawlerProtectionUse418' => false, + 'CrawlerProtectionRawDenial' => true, + 'CrawlerProtectionRawDenialHeader' => 'HTTP/1.0 403 Forbidden', + 'CrawlerProtectionRawDenialText' => '403 Forbidden', + ] ) + ] ) + ->onlyMethods( [ 'denyAccessRaw' ] ) + ->getMock(); + + $factory->expects( $this->once() ) + ->method( 'denyAccessRaw' ) + ->with( 'HTTP/1.0 403 Forbidden', '403 Forbidden' ); + + $output = $this->createMock( self::$outputPageClassName ); + $factory->denyAccess( $output ); + } + + /** + * @covers ::denyAccess + */ + public function testDenyAccessFallsThroughToPretty() { + $factory = $this->getMockBuilder( ResponseFactory::class ) + ->setConstructorArgs( [ + new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [ + 'CrawlerProtectionUse418' => false, + 'CrawlerProtectionRawDenial' => false, + 'CrawlerProtectionRawDenialHeader' => '', + 'CrawlerProtectionRawDenialText' => '', + ] ) + ] ) + ->onlyMethods( [ 'denyAccessPretty' ] ) + ->getMock(); + + $output = $this->createMock( self::$outputPageClassName ); + $factory->expects( $this->once() ) + ->method( 'denyAccessPretty' ) + ->with( $output ); + + $factory->denyAccess( $output ); + } + + /** + * @covers ::__construct + */ + public function testConstructorAcceptsValidOptions() { + $factory = $this->buildFactory(); + $this->assertInstanceOf( ResponseFactory::class, $factory ); + } +}