Address review findings on 1.7.0: denial headers, API status code, hook entry points

.github/scripts/check-i18n-qqq.sh

@@ -1,29 +1,71 @@
 #!/usr/bin/env bash
-# Check that every message key in i18n/en.json has a corresponding
-# documentation entry in i18n/qqq.json.  Exits with code 1 when any
-# keys are missing so that CI can enforce the MediaWiki "MUST" requirement.
+# Check that i18n/en.json and i18n/qqq.json describe the same set of message
+# keys: every message must be documented, and qqq must not document messages
+# that no longer exist.  Exits with code 1 when the two sets differ so that CI
+# can enforce the MediaWiki "MUST" requirement.
 
 set -euo pipefail
 
 EXTENSION_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
 
-python3 - "$EXTENSION_ROOT/i18n/en.json" "$EXTENSION_ROOT/i18n/qqq.json" << 'PYTHON'
-import json, sys
+php -- "$EXTENSION_ROOT/i18n/en.json" "$EXTENSION_ROOT/i18n/qqq.json" << 'PHP'
+<?php
+/**
+ * @param string $path
+ * @return string[]
+ */
+function crawlerProtectionMessageKeys( string $path ): array {
+	$contents = file_get_contents( $path );
+	if ( $contents === false ) {
+		fwrite( STDERR, "ERROR: Unable to read $path\n" );
+		exit( 1 );
+	}
 
-en_path, qqq_path = sys.argv[1], sys.argv[2]
+	$data = json_decode( $contents, true );
+	if ( !is_array( $data ) ) {
+		fwrite( STDERR, "ERROR: $path is not valid JSON\n" );
+		exit( 1 );
+	}
 
-with open(en_path, encoding='utf-8') as f:
-    en_keys = {k for k in json.load(f) if k != '@metadata'}
+	$keys = array_keys( $data );
+	return array_values( array_filter(
+		$keys,
+		static function ( $key ) {
+			return $key !== '@metadata';
+		}
+	) );
+}
 
-with open(qqq_path, encoding='utf-8') as f:
-    qqq_keys = {k for k in json.load(f) if k != '@metadata'}
+[ , $enPath, $qqqPath ] = $argv;
 
-missing = en_keys - qqq_keys
-if missing:
-    print("ERROR: Keys present in en.json but missing from qqq.json:")
-    for key in sorted(missing):
-        print(f"  - {key}")
-    sys.exit(1)
+$enKeys = crawlerProtectionMessageKeys( $enPath );
+$qqqKeys = crawlerProtectionMessageKeys( $qqqPath );
 
-print(f"OK: All {len(en_keys)} message key(s) from en.json are documented in qqq.json.")
-PYTHON
+$status = 0;
+
+$missing = array_diff( $enKeys, $qqqKeys );
+if ( $missing ) {
+	sort( $missing );
+	echo "ERROR: Keys present in en.json but missing from qqq.json:\n";
+	foreach ( $missing as $key ) {
+		echo "  - $key\n";
+	}
+	$status = 1;
+}
+
+$orphaned = array_diff( $qqqKeys, $enKeys );
+if ( $orphaned ) {
+	sort( $orphaned );
+	echo "ERROR: Keys documented in qqq.json but absent from en.json:\n";
+	foreach ( $orphaned as $key ) {
+		echo "  - $key\n";
+	}
+	$status = 1;
+}
+
+if ( $status === 0 ) {
+	printf( "OK: All %d message key(s) from en.json are documented in qqq.json.\n", count( $enKeys ) );
+}
+
+exit( $status );
+PHP

README.md

@@ -81,9 +81,11 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
   single IPv4/IPv6 addresses (`'1.2.3.4'`, `'2001:db8::1'`), CIDR notation
   (`'1.2.3.0/24'`, `'2001:db8::/32'`), and explicit ranges
   (`'1.2.3.1 - 1.2.3.10'`). The client IP is resolved via `WebRequest::getIP()`,
-  which correctly handles trusted-proxy and `X-Forwarded-For` headers consistent
-  with the rest of MediaWiki. The same resolution is used for `index.php`,
-  `api.php` and `rest.php` requests.
+  which handles trusted proxies and `X-Forwarded-For` exactly as the rest of
+  MediaWiki does - which also means that an unregistered reverse proxy makes it
+  report the proxy address; see
+  [Wikis behind a reverse proxy](#wikis-behind-a-reverse-proxy) below. The same
+  resolution is used for `index.php`, `api.php` and `rest.php` requests.
 * `$wgCrawlerProtectionTreatTempUsersAsAnon` - when `true`, users with
   [temporary accounts](https://www.mediawiki.org/wiki/Help:Temporary_accounts)
   (`$wgAutoCreateTempUser`, available since MediaWiki 1.42) are treated as
@@ -97,9 +99,11 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
   [Wikis behind a reverse proxy](#wikis-behind-a-reverse-proxy) below; only
   enable this after reading that section.
 
-The pretty denial page carries an `X-Robots-Tag: noindex,nofollow` header and
-the same robot policy as a `<meta>` tag, so that well-behaved crawlers stop
-re-requesting denied URLs.
+Every denial carries an `X-Robots-Tag: noindex,nofollow` header - the pretty
+denial page, the raw denial and the 418 response alike - and the pretty page
+repeats the same robot policy as a `<meta>` tag, so that well-behaved crawlers
+stop re-requesting denied URLs. Denied Action API requests are answered with
+HTTP 403, as are denied REST API requests.
 
 ## Wikis behind a reverse proxy
 
@@ -136,9 +140,14 @@ been sent by the client. Nothing else in MediaWiki is affected, and the header
 can never cause a request to be denied - only allowlisted.
 
 **Only enable this if every request reaches the wiki through exactly one
-reverse proxy that sets or appends `X-Forwarded-For`.** If the web server is
-also reachable directly, or if there are several proxy hops, a client can
-forge the header and bypass protection by claiming an allowlisted address.
+reverse proxy that unconditionally sets or appends `X-Forwarded-For`.** If the
+web server is also reachable directly, or if there are several proxy hops, a
+client can forge the header and bypass protection by claiming an allowlisted
+address. The same applies when the proxy only fills the header in when it is
+absent - HAProxy's `option forwardfor if-none`, for example - because a
+client-supplied value then survives as the only entry in the chain. Configure
+the proxy to always overwrite the header (HAProxy: plain `option forwardfor`,
+nginx: `proxy_set_header X-Forwarded-For $remote_addr`).
 
 # Hooks
 
@@ -152,29 +161,31 @@ allowlists, ...) without patching this extension.
 Parameters:
 
 * `User $user` - the user making the request.
-* `WebRequest $request` - the current request.
+* `WebRequest|null $request` - the current request, or `null` when the entry
+  point cannot supply one.
+* `string $entryPoint` - the entry point the request arrived through: `'index'`
+  for `index.php`, `'api'` for `api.php` or `'rest'` for `rest.php`.
 * `string|null $specialPageName` - canonical name of the special page being
-  executed, or `null` if the request is not a special page view.
+  executed, or `null` if the request is not a special page view. Always `null`
+  for the `api` and `rest` entry points.
 * `bool &$shouldDeny` - whether the request will be denied. Set it to `true` to
   deny a request that would otherwise be allowed, or to `false` to allow a
   request that would otherwise be denied.
 
 Return `false` to stop other handlers from running; the value of `$shouldDeny`
 at that point is still honoured. The hook runs for every web request that
-reaches CrawlerProtection (but not on the command line), including requests by
-registered users and requests that touch no protected resource, so handlers
-must inspect `$shouldDeny` and the request themselves rather than assuming a
-denial is pending. It does not run for Action API or REST API requests, which
-are governed solely by `$wgCrawlerProtectedApiModules` and
-`$wgCrawlerProtectedRestPaths`.
+reaches CrawlerProtection at any of its entry points (but not on the command
+line), including requests by registered users and requests that touch no
+protected resource, so handlers must inspect `$shouldDeny` and the request
+themselves rather than assuming a denial is pending.
 
 Example, allowing anonymous access when a request carries a secret header:
 
 ```php
 $wgHooks['CrawlerProtectionShouldDeny'][] = static function (
-	$user, $request, $specialPageName, &$shouldDeny
+	$user, $request, $entryPoint, $specialPageName, &$shouldDeny
 ) {
-	if ( $shouldDeny && $request->getHeader( 'X-My-Crawler-Token' ) === $secret ) {
+	if ( $shouldDeny && $request && $request->getHeader( 'X-My-Crawler-Token' ) === $secret ) {
 		$shouldDeny = false;
 	}
 };

TESTING.md

@@ -55,7 +55,8 @@ Inside the container (`make bash`):
 composer test        # Run phpcs + phpunit
 composer phpcs       # Check code style
 composer phpcbf      # Fix code style
-composer phpunit     # Run unit tests
+composer phpunit     # Run unit tests (tests/phpunit/unit/)
+composer phpunit:integration  # Run integration tests (tests/phpunit/integration/)
 ```
 
 ## Update Docker CI

composer.json

@@ -33,7 +33,8 @@
         "phpcs": "vendor/bin/phpcs -sp --standard=.phpcs.xml",
         "phpcbf": "vendor/bin/phpcbf --standard=.phpcs.xml",
         "minus-x": "vendor/bin/minus-x check .",
-        "phpunit": "php ../../tests/phpunit/phpunit.php tests/phpunit/"
+        "phpunit": "php ../../tests/phpunit/phpunit.php tests/phpunit/unit/",
+        "phpunit:integration": "php ../../tests/phpunit/phpunit.php tests/phpunit/integration/"
     },
     "config": {
         "allow-plugins": {

includes/CrawlerProtectionService.php

@@ -172,6 +172,7 @@ public function checkPerformAction(
 		$this->hookRunner->onCrawlerProtectionShouldDeny(
 			$user,
 			$request,
+			CrawlerProtectionShouldDenyHook::ENTRY_POINT_INDEX,
 			null,
 			$shouldDeny
 		);
@@ -268,6 +269,7 @@ public function checkSpecialPage(
 		$this->hookRunner->onCrawlerProtectionShouldDeny(
 			$user,
 			$request,
+			CrawlerProtectionShouldDenyHook::ENTRY_POINT_INDEX,
 			$specialPageName,
 			$shouldDeny
 		);
@@ -317,16 +319,36 @@ public function checkApiModules( array $moduleNames, $user, $request = null ): b
 			return true;
 		}
 
-		if ( $this->isUserAllowed( $user ) || $this->isRequestIPAllowed( $request ) ) {
-			return true;
-		}
+		$shouldDeny = false;
 
-		foreach ( $moduleNames as $moduleName ) {
-			if ( $this->isProtectedApiModule( $moduleName ) ) {
-				return false;
+		if ( !$this->isUserAllowed( $user ) && !$this->isRequestIPAllowed( $request ) ) {
+			foreach ( $moduleNames as $moduleName ) {
+				if ( $this->isProtectedApiModule( $moduleName ) ) {
+					$shouldDeny = true;
+					break;
+				}
 			}
 		}
 
+		$this->hookRunner->onCrawlerProtectionShouldDeny(
+			$user,
+			$request,
+			CrawlerProtectionShouldDenyHook::ENTRY_POINT_API,
+			null,
+			$shouldDeny
+		);
+
+		if ( $shouldDeny ) {
+			// Core denies an ApiCheckCanExecute veto with dieWithError() and no
+			// HTTP code, which would answer "200 OK" with an error body, so the
+			// status and the robot directive are set here.
+			$this->responseFactory->markDenied(
+				$request !== null ? $request->response() : null,
+				403
+			);
+			return false;
+		}
+
 		return true;
 	}
 
@@ -365,11 +387,28 @@ public function checkRestPath( string $path, $user, $request = null ): bool {
 			return true;
 		}
 
-		if ( $this->isUserAllowed( $user ) || $this->isRequestIPAllowed( $request ) ) {
-			return true;
+		$shouldDeny = !$this->isUserAllowed( $user )
+			&& !$this->isRequestIPAllowed( $request )
+			&& $this->isProtectedRestPath( $path );
+
+		$this->hookRunner->onCrawlerProtectionShouldDeny(
+			$user,
+			$request,
+			CrawlerProtectionShouldDenyHook::ENTRY_POINT_REST,
+			null,
+			$shouldDeny
+		);
+
+		if ( $shouldDeny ) {
+			// The 403 status comes from the LocalizedHttpException raised by the
+			// hook handler; only the robot directive is added here.
+			$this->responseFactory->markDenied(
+				$request !== null ? $request->response() : null
+			);
+			return false;
 		}
 
-		return !$this->isProtectedRestPath( $path );
+		return true;
 	}
 
 	/**
@@ -386,6 +425,20 @@ public function checkRestPath( string $path, $user, $request = null ): bool {
 	 */
 	public function isProtectedRestPath( string $path ): bool {
 		$patterns = $this->normalizedProtectedRestPaths;
+		if ( $patterns === [] ) {
+			return false;
+		}
+
+		// fnmatch() is unavailable on a handful of exotic PHP builds; without it
+		// no pattern can be evaluated, so nothing is treated as protected.
+		if ( !function_exists( 'fnmatch' ) ) {
+			$this->logger->warning(
+				'CrawlerProtection: fnmatch() is unavailable, so ' .
+					'CrawlerProtectedRestPaths cannot be evaluated.'
+			);
+			return false;
+		}
+
 		foreach ( $patterns as $pattern ) {
 			if ( fnmatch( $pattern, $path, FNM_PATHNAME ) ) {
 				return true;
@@ -552,7 +605,9 @@ private function isUserAllowed( $user ): bool {
 	private function normalizeArrayConfig( string $configKey ): array {
 		$value = $this->options->get( $configKey );
 
-		if ( !is_array( $value ) ) {
+		$wasScalar = !is_array( $value );
+
+		if ( $wasScalar ) {
 			$this->logger->warning(
 				'CrawlerProtection: Config {configKey} should be an array; got a scalar. ' .
 					'Treating it as a single-element array.',
@@ -563,7 +618,9 @@ private function normalizeArrayConfig( string $configKey ): array {
 
 		$filtered = array_values( array_filter( $value, 'is_string' ) );
 
-		if ( count( $filtered ) !== count( $value ) ) {
+		// A non-string scalar has already been reported above; warning again
+		// about the same value would be noise.
+		if ( !$wasScalar && count( $filtered ) !== count( $value ) ) {
 			$this->logger->warning(
 				'CrawlerProtection: Config {configKey} contains non-string entries; ' .
 					'they have been ignored.',

includes/Hook/CrawlerProtectionShouldDenyHook.php

@@ -36,6 +36,15 @@
  */
 interface CrawlerProtectionShouldDenyHook {
 
+	/** Entry point value for index.php requests */
+	public const ENTRY_POINT_INDEX = 'index';
+
+	/** Entry point value for api.php (Action API) requests */
+	public const ENTRY_POINT_API = 'api';
+
+	/** Entry point value for rest.php (REST API) requests */
+	public const ENTRY_POINT_REST = 'rest';
+
 	/**
 	 * Called after CrawlerProtection has decided whether to deny a request,
 	 * but before the denial is carried out.
@@ -45,17 +54,22 @@ interface CrawlerProtectionShouldDenyHook {
 	 * otherwise be denied. Return false to stop other handlers from running;
 	 * the value of $shouldDeny at that point is still honoured.
 	 *
-	 * The hook runs for every web request that reaches CrawlerProtection,
-	 * including requests by registered users and requests that touch no
-	 * protected resource, so handlers must inspect $shouldDeny and the
-	 * request themselves rather than assuming a denial is pending.
+	 * The hook runs for every web request that reaches CrawlerProtection at
+	 * any of its entry points, including requests by registered users and
+	 * requests that touch no protected resource, so handlers must inspect
+	 * $shouldDeny and the request themselves rather than assuming a denial is
+	 * pending.
 	 *
 	 * @since 1.7.0
 	 *
 	 * @param \MediaWiki\User\User $user The user making the request
-	 * @param \MediaWiki\Request\WebRequest $request The current request
+	 * @param \MediaWiki\Request\WebRequest|null $request The current request,
+	 *   or null when the entry point cannot supply one
+	 * @param string $entryPoint Entry point the request arrived through: one of
+	 *   self::ENTRY_POINT_INDEX, self::ENTRY_POINT_API or self::ENTRY_POINT_REST
 	 * @param string|null $specialPageName Canonical name of the special page
-	 *   being executed, or null if the request is not a special page view
+	 *   being executed, or null if the request is not a special page view;
+	 *   always null for the api and rest entry points
 	 * @param bool &$shouldDeny Whether the request will be denied; modify to
 	 *   change the outcome
 	 * @return bool|void True or no return value to continue, false to stop
@@ -64,6 +78,7 @@ interface CrawlerProtectionShouldDenyHook {
 	public function onCrawlerProtectionShouldDeny(
 		$user,
 		$request,
+		string $entryPoint,
 		?string $specialPageName,
 		bool &$shouldDeny
 	);