From 3a674d7e94e20c02c1563029adac7119780a2cb5 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Wed, 29 Jul 2026 14:58:08 -0700
Subject: [PATCH 01/16] i18n: add qqq.json, descriptionmsg, localised
raw-denial responses, and CI qqq completeness check (#55)
* Initial plan
* Add i18n improvements: qqq.json, descriptionmsg, localised raw-denial, CI check
* tests: add unit tests for i18n fallback in denyAccess and denyAccessWith418
* tests: refactor i18n fallback tests to AAA format with explicit assertions
* fix: use ->plain() instead of ->text() to resolve SecurityCheck-XSS phan errors
* fix: suppress SecurityCheck-XSS for trusted message bodies in raw denial
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
---
.github/scripts/check-i18n-qqq.sh | 29 +++++++
.github/workflows/ci.yml | 9 ++
extension.json | 6 +-
i18n/en.json | 5 +-
i18n/qqq.json | 10 +++
includes/ResponseFactory.php | 18 +++-
tests/phpunit/stubs.php | 18 ++++
tests/phpunit/unit/ResponseFactoryTest.php | 97 ++++++++++++++++++++++
8 files changed, 185 insertions(+), 7 deletions(-)
create mode 100755 .github/scripts/check-i18n-qqq.sh
create mode 100644 i18n/qqq.json
diff --git a/.github/scripts/check-i18n-qqq.sh b/.github/scripts/check-i18n-qqq.sh
new file mode 100755
index 0000000..ec14ba8
--- /dev/null
+++ b/.github/scripts/check-i18n-qqq.sh
@@ -0,0 +1,29 @@
+#!/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.
+
+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
+
+en_path, qqq_path = sys.argv[1], sys.argv[2]
+
+with open(en_path, encoding='utf-8') as f:
+ en_keys = {k for k in json.load(f) if k != '@metadata'}
+
+with open(qqq_path, encoding='utf-8') as f:
+ qqq_keys = {k for k in json.load(f) if k != '@metadata'}
+
+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)
+
+print(f"OK: All {len(en_keys)} message key(s) from en.json are documented in qqq.json.")
+PYTHON
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 033a352..f7267c5 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -49,6 +49,15 @@ jobs:
HEAD_REVISION: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}
run: .github/scripts/check-version-bump.sh "$BASE_REVISION" "$HEAD_REVISION"
+ i18n:
+ name: i18n Check
+ runs-on: ubuntu-latest
+ steps:
+ - name: Setup Extension
+ uses: actions/checkout@v4
+ - name: Check qqq.json completeness
+ run: .github/scripts/check-i18n-qqq.sh
+
style:
name: Code Style
runs-on: ${{ matrix.os }}
diff --git a/extension.json b/extension.json
index 0493207..2968838 100644
--- a/extension.json
+++ b/extension.json
@@ -1,9 +1,9 @@
{
"name": "CrawlerProtection",
"author": "[https://mywikis.com MyWikis LLC]",
- "version": "1.6.0",
+ "version": "1.6.1",
"url": "https://www.mediawiki.org/wiki/Extension:CrawlerProtection",
- "description": "Suite of protective measures to protect wikis from crawlers.",
+ "descriptionmsg": "crawlerprotection-desc",
"type": "hook",
"requires": {
"MediaWiki": ">= 1.39.4"
@@ -54,7 +54,7 @@
"value": "HTTP/1.0 403 Forbidden"
},
"CrawlerProtectionRawDenialText": {
- "value": "403 Forbidden. You must be logged in to view this page."
+ "value": ""
},
"CrawlerProtectionAllowedIPs": {
"value": [],
diff --git a/i18n/en.json b/i18n/en.json
index 6ca93dc..c797290 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -2,6 +2,9 @@
"@metadata": {
"authors": [ "MyWikis LLC" ]
},
+ "crawlerprotection-desc": "Suite of protective measures to protect wikis from crawlers.",
"crawlerprotection-accessdenied-title": "Access denied",
- "crawlerprotection-accessdenied-text": "You must be logged in to perform this action or view this special page."
+ "crawlerprotection-accessdenied-text": "You must be logged in to perform this action or view this special page.",
+ "crawlerprotection-rawdenial-text": "403 Forbidden. You must be logged in to view this page.",
+ "crawlerprotection-rawdenial-teapot": "I'm a teapot"
}
diff --git a/i18n/qqq.json b/i18n/qqq.json
new file mode 100644
index 0000000..23fc954
--- /dev/null
+++ b/i18n/qqq.json
@@ -0,0 +1,10 @@
+{
+ "@metadata": {
+ "authors": []
+ },
+ "crawlerprotection-desc": "Description of the [[mw:Extension:CrawlerProtection|CrawlerProtection]] extension, shown on [[Special:Version]]. {{desc}}",
+ "crawlerprotection-accessdenied-title": "Title of the access-denied page shown to anonymous users who attempt to access a protected action or special page. Displayed as the page heading.",
+ "crawlerprotection-accessdenied-text": "Body text of the access-denied page shown to anonymous users who attempt to access a protected action or special page. May contain wikitext.",
+ "crawlerprotection-rawdenial-text": "Body of the raw HTTP 403 response sent when $wgCrawlerProtectionRawDenial is enabled and $wgCrawlerProtectionUse418 is false. Rendered as plain text without HTML processing. This message is used only when the operator has not set a custom value for $wgCrawlerProtectionRawDenialText.",
+ "crawlerprotection-rawdenial-teapot": "Body of the raw HTTP 418 (I'm a Teapot) response sent when $wgCrawlerProtectionUse418 is enabled. Rendered as plain text without HTML processing."
+}
diff --git a/includes/ResponseFactory.php b/includes/ResponseFactory.php
index 9da0abb..9e9986f 100644
--- a/includes/ResponseFactory.php
+++ b/includes/ResponseFactory.php
@@ -39,7 +39,6 @@
class ResponseFactory {
private const TEAPOT_HEADER = 'HTTP/1.0 418 I\'m a teapot';
- private const TEAPOT_BODY = 'I\'m a teapot';
/** @var string[] List of constructor options this class accepts */
public const CONSTRUCTOR_OPTIONS = [
@@ -73,15 +72,23 @@ public function __construct( ServiceOptions $options ) {
*
* @param OutputPage $output Used only for the "pretty" strategy
* @return void
+ * @suppress SecurityCheck-XSS The raw body comes from wiki configuration
+ * or from an interface message, both of which are trusted sources.
*/
public function denyAccess( $output ): void {
if ( $this->options->get( 'CrawlerProtectionRawDenial' ) ) {
if ( $this->options->get( 'CrawlerProtectionUse418' ) ) {
$this->denyAccessWith418();
} else {
+ $rawText = $this->options->get( 'CrawlerProtectionRawDenialText' );
+ if ( $rawText === '' ) {
+ $rawText = wfMessage( 'crawlerprotection-rawdenial-text' )
+ ->inContentLanguage()
+ ->text();
+ }
$this->denyAccessRaw(
$this->options->get( 'CrawlerProtectionRawDenialHeader' ),
- $this->options->get( 'CrawlerProtectionRawDenialText' )
+ $rawText
);
}
} else {
@@ -94,9 +101,14 @@ public function denyAccess( $output ): void {
*
* @return void
* @suppress PhanPluginNeverReturnMethod
+ * @suppress SecurityCheck-XSS The body comes from an interface message,
+ * which is a trusted source.
*/
protected function denyAccessWith418(): void {
- $this->denyAccessRaw( self::TEAPOT_HEADER, self::TEAPOT_BODY );
+ $this->denyAccessRaw(
+ self::TEAPOT_HEADER,
+ wfMessage( 'crawlerprotection-rawdenial-teapot' )->inContentLanguage()->text()
+ );
}
/**
diff --git a/tests/phpunit/stubs.php b/tests/phpunit/stubs.php
index a8d11ed..94a92d5 100644
--- a/tests/phpunit/stubs.php
+++ b/tests/phpunit/stubs.php
@@ -19,6 +19,15 @@
*/
function wfMessage( $key ) {
return new class() {
+ /**
+ * Return self for chained calls
+ *
+ * @return static
+ */
+ public function inContentLanguage() {
+ return $this;
+ }
+
/**
* Return plain text version of message
*
@@ -27,6 +36,15 @@ function wfMessage( $key ) {
public function plain() {
return 'Mock message';
}
+
+ /**
+ * Return text version of message
+ *
+ * @return string
+ */
+ public function text() {
+ return 'Mock message';
+ }
};
}
}
diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php
index 1068fb0..14c810f 100644
--- a/tests/phpunit/unit/ResponseFactoryTest.php
+++ b/tests/phpunit/unit/ResponseFactoryTest.php
@@ -163,6 +163,103 @@ public function testDenyAccessFallsThroughToPretty() {
$factory->denyAccess( $output );
}
+ /**
+ * When CrawlerProtectionRawDenialText is empty, denyAccess must fall back
+ * to the i18n message for the raw-denial body instead of passing an empty
+ * string to denyAccessRaw.
+ *
+ * @covers ::denyAccess
+ */
+ public function testDenyAccessRawUsesI18nWhenOverrideIsEmpty() {
+ if ( defined( 'MEDIAWIKI' ) ) {
+ $this->markTestSkipped(
+ 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
+ );
+ }
+
+ // Arrange
+ $capturedHeader = null;
+ $capturedBody = null;
+
+ $factory = $this->getMockBuilder( ResponseFactory::class )
+ ->setConstructorArgs( [
+ new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [
+ 'CrawlerProtectionUse418' => false,
+ 'CrawlerProtectionRawDenial' => true,
+ 'CrawlerProtectionRawDenialHeader' => 'HTTP/1.0 403 Forbidden',
+ 'CrawlerProtectionRawDenialText' => '',
+ ] )
+ ] )
+ ->onlyMethods( [ 'denyAccessRaw' ] )
+ ->getMock();
+
+ $factory->method( 'denyAccessRaw' )
+ ->willReturnCallback(
+ static function ( string $header, string $message ) use ( &$capturedHeader, &$capturedBody ) {
+ $capturedHeader = $header;
+ $capturedBody = $message;
+ }
+ );
+
+ $output = $this->createMock( self::$outputPageClassName );
+
+ // Act
+ $factory->denyAccess( $output );
+
+ // Assert
+ $this->assertSame( 'HTTP/1.0 403 Forbidden', $capturedHeader );
+ $this->assertSame( 'Mock message', $capturedBody );
+ }
+
+ /**
+ * When both Use418 and RawDenial are enabled, denyAccess should produce a
+ * 418 response whose body comes from the i18n teapot message rather than a
+ * hardcoded string.
+ *
+ * @covers ::denyAccess
+ * @covers ::denyAccessWith418
+ */
+ public function testDenyAccessWith418UsesI18nMessage() {
+ if ( defined( 'MEDIAWIKI' ) ) {
+ $this->markTestSkipped(
+ 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
+ );
+ }
+
+ // Arrange
+ $capturedHeader = null;
+ $capturedBody = null;
+
+ $factory = $this->getMockBuilder( ResponseFactory::class )
+ ->setConstructorArgs( [
+ new ServiceOptions( ResponseFactory::CONSTRUCTOR_OPTIONS, [
+ 'CrawlerProtectionUse418' => true,
+ 'CrawlerProtectionRawDenial' => true,
+ 'CrawlerProtectionRawDenialHeader' => '',
+ 'CrawlerProtectionRawDenialText' => '',
+ ] )
+ ] )
+ ->onlyMethods( [ 'denyAccessRaw' ] )
+ ->getMock();
+
+ $factory->method( 'denyAccessRaw' )
+ ->willReturnCallback(
+ static function ( string $header, string $message ) use ( &$capturedHeader, &$capturedBody ) {
+ $capturedHeader = $header;
+ $capturedBody = $message;
+ }
+ );
+
+ $output = $this->createMock( self::$outputPageClassName );
+
+ // Act
+ $factory->denyAccess( $output );
+
+ // Assert
+ $this->assertSame( 'HTTP/1.0 418 I\'m a teapot', $capturedHeader );
+ $this->assertSame( 'Mock message', $capturedBody );
+ }
+
/**
* @covers ::__construct
*/
From 38ab42a19100d5a22a15f50cb8be4adfbec7b411 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Wed, 29 Jul 2026 21:22:20 -0700
Subject: [PATCH 02/16] Protect api.php and rest.php entry points from crawlers
(#56)
* Initial plan
* feat: protect api.php (ApiCheckCanExecute) and rest.php (RestCheckCanExecute) entry points
- Add CrawlerProtectedApiModules config (default []) for Action API module protection
- Add CrawlerProtectedRestPaths config (default []) for REST API path protection via fnmatch glob patterns
- Register ApiCheckCanExecute and RestCheckCanExecute hooks in extension.json
- Restructure Hooks.php with namespace-block compat stubs so the class implements both new hook interfaces on all supported MW versions; REST protection is silently skipped on MW < 1.42 where RestCheckCanExecute does not fire
- Add checkApiModule, isProtectedApiModule, checkRestPath, isProtectedRestPath to CrawlerProtectionService
- Add stub interfaces for ApiCheckCanExecuteHook, RestCheckCanExecuteHook, HttpException in namespaced-stubs.php
- Add unit tests for all new service methods and hook handlers
- Update README with entry-point coverage table and new config documentation
Closes #48
* Operationalize Copilot guidance with repo-specific rules, correct scope, and agent setup workflow (#61)
* Initial plan
* Overhaul Copilot instructions with repo-specific CI architecture and testing guidance
* Clarify trimmed guidance and Copilot setup workflow intent
* Restore comprehensive MediaWiki instruction guidance
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* fix: use FNM_PATHNAME for REST path globs, fix docblock, bump version
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* fix: drop core hook interface stubs and use LocalizedHttpException for REST denial
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* ci: pin actions to SHAs, extend matrix to MW 1.44/1.45, add Dependabot, enforce minus-x (#59)
* Initial plan
* ci: hygiene improvements - pin actions, extend matrix, add dependabot
* ci: skip minus-x on REL1_39 where MediaWiki does not ship it
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* Bump actions/checkout from 4.1.7 to 7.0.1 (#65)
Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.7 to 7.0.1.
- [Release notes](https://github.com/actions/checkout/releases)
- [Commits](https://github.com/actions/checkout/compare/v4.1.7...v7.0.1)
---
updated-dependencies:
- dependency-name: actions/checkout
dependency-version: 7.0.1
dependency-type: direct:production
update-type: version-update:semver-major
...
Signed-off-by: dependabot[bot]
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Merge main; cover query sub-modules and correct REST hook MW version
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* test: extract REST handler stub helper with property docs (MW 1.45 phpcs)
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---------
Signed-off-by: dependabot[bot]
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Jeffrey Wang
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
---
.github/copilot-instructions.md | 84 +++++
.github/dependabot.yml | 20 ++
...lerprotection-architecture.instructions.md | 43 +++
...lerprotection-release-i18n.instructions.md | 29 ++
.../crawlerprotection-testing.instructions.md | 18 +
...wiki-extensions-clean-code.instructions.md | 2 +-
.../mediawiki-extensions.instructions.md | 2 +-
.github/workflows/ci.yml | 35 +-
.github/workflows/copilot-setup-steps.yml | 38 ++
Makefile | 2 +-
README.md | 35 ++
TESTING.md | 12 +-
build | 1 +
composer.json | 5 +
extension.json | 12 +-
includes/CrawlerProtectionService.php | 110 ++++++
includes/Hooks.php | 97 ++++++
tests/phpunit/namespaced-stubs.php | 52 +++
.../unit/CrawlerProtectionServiceTest.php | 329 +++++++++++++++++-
tests/phpunit/unit/HooksTest.php | 250 +++++++++++++
20 files changed, 1154 insertions(+), 22 deletions(-)
create mode 100644 .github/dependabot.yml
create mode 100644 .github/instructions/crawlerprotection-architecture.instructions.md
create mode 100644 .github/instructions/crawlerprotection-release-i18n.instructions.md
create mode 100644 .github/instructions/crawlerprotection-testing.instructions.md
create mode 100644 .github/workflows/copilot-setup-steps.yml
create mode 160000 build
diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md
index e69de29..b4bfcfe 100644
--- a/.github/copilot-instructions.md
+++ b/.github/copilot-instructions.md
@@ -0,0 +1,84 @@
+# CrawlerProtection Copilot instructions
+
+This repository is a MediaWiki extension that blocks high-cost crawler traffic for anonymous users.
+
+## Extension scope
+
+- Main logic: `includes/CrawlerProtectionService.php`
+- Response rendering: `includes/ResponseFactory.php`
+- Hook entry points only:
+ - `MediaWikiPerformAction`
+ - `SpecialPageBeforeExecute`
+
+Keep hook handlers thin; put behavior in services.
+
+## Compatibility and syntax floor
+
+- Extension requirement: MediaWiki `>= 1.39.4`
+- CI matrix currently tests:
+ - REL1_39 on PHP 7.4, 8.1, and 8.2 (phpunit job)
+ - REL1_43 on PHP 8.2, 8.3, 8.4
+- Treat **PHP 7.4 syntax as mandatory** for production code and tests.
+
+Do **not** introduce PHP 8-only syntax in extension code:
+
+- constructor property promotion
+- `match`
+- union/intersection types
+- enums
+- `readonly`
+- nullsafe operator `?->`
+- named arguments
+- first-class callable syntax (`foo(...)`)
+
+## Hard CI invariants
+
+- Non-hidden-path content changes must include a version bump in `extension.json`.
+- Changes in `i18n/` must include at least a patch bump.
+- CI validates: parallel-lint, PHPCS, Phan (`--minimum-target-php-version=7.4`), PHPUnit.
+
+See `.github/scripts/check-version-bump.sh` and `.github/workflows/ci.yml`.
+
+## Repository layout
+
+- `includes/` extension classes and wiring
+- `i18n/` localization JSON
+- `tests/phpunit/unit/` unit tests
+- `tests/phpunit/stubs.php` and `tests/phpunit/namespaced-stubs.php` test stubs
+
+## Validation commands
+
+Primary local workflow uses docker-compose-ci:
+
+- `make ci`
+- `MW_VERSION=1.39 PHP_VERSION=8.1 make ci`
+- `MW_VERSION=1.43 PHP_VERSION=8.3 make ci`
+
+Inside container (`make bash`):
+
+- `composer phpcs`
+- `composer phpcbf`
+- `composer phpunit`
+
+For setup details, see `TESTING.md`, `.github/CI-SETUP.md`, and `.github/DOCKER-CI-QUICKREF.md`.
+
+## Instruction precedence
+
+1. `.github/copilot-instructions.md` (repo-specific source of truth)
+2. `.github/instructions/crawlerprotection-*.instructions.md`
+3. `.github/instructions/php.instructions.md`
+4. `.github/instructions/mediawiki-extensions*.instructions.md`
+5. `.github/instructions/mediawiki.instructions.md`
+
+If guidance conflicts, prefer the highest entry above and then CI/tooling output.
+
+## Definition of done
+
+Before finalizing a change:
+
+- extension behavior and tests updated minimally for the task
+- `extension.json` version bump done when required by CI gate
+- PHP 7.4 syntax compatibility preserved
+- PHPCS/Phan/PHPUnit checks run for affected areas
+- i18n changes include both `en.json` and `qqq.json`
+- README updated when configuration or behavior changes
diff --git a/.github/dependabot.yml b/.github/dependabot.yml
new file mode 100644
index 0000000..c2e82c0
--- /dev/null
+++ b/.github/dependabot.yml
@@ -0,0 +1,20 @@
+version: 2
+
+updates:
+ - package-ecosystem: "composer"
+ directory: "/"
+ schedule:
+ interval: "weekly"
+ open-pull-requests-limit: 5
+
+ - package-ecosystem: "github-actions"
+ directory: "/"
+ schedule:
+ interval: "weekly"
+ open-pull-requests-limit: 5
+
+ - package-ecosystem: "gitsubmodule"
+ directory: "/"
+ schedule:
+ interval: "weekly"
+ open-pull-requests-limit: 5
diff --git a/.github/instructions/crawlerprotection-architecture.instructions.md b/.github/instructions/crawlerprotection-architecture.instructions.md
new file mode 100644
index 0000000..269aa91
--- /dev/null
+++ b/.github/instructions/crawlerprotection-architecture.instructions.md
@@ -0,0 +1,43 @@
+---
+applyTo: "**/*.php"
+---
+
+# CrawlerProtection architecture and security rules
+
+## Architecture invariants
+
+- This extension protects anonymous traffic at these hook points only:
+ - `MediaWikiPerformAction` (index.php page views and actions)
+ - `SpecialPageBeforeExecute` (index.php special pages)
+ - `ApiCheckCanExecute` (api.php modules)
+ - `RestCheckCanExecute` (rest.php paths, MediaWiki 1.44+)
+- `includes/Hooks.php` must remain a thin adapter layer delegating to services.
+- Business logic belongs in `CrawlerProtectionService` and response rendering in `ResponseFactory`.
+- Instantiate services only in `includes/ServiceWiring.php`.
+- Service names must use the existing namespace pattern:
+ - `CrawlerProtection.ResponseFactory`
+ - `CrawlerProtection.CrawlerProtectionService`
+- New service config must use `CONSTRUCTOR_OPTIONS` + injected `ServiceOptions`.
+- New config keys must follow `CrawlerProtection*` / `CrawlerProtected*` naming and be added to
+ `extension.json` with `merge_strategy: "provide_default"` for arrays.
+
+## Multi-version compatibility pattern
+
+- Maintain the `class_alias` compatibility blocks in `includes/Hooks.php` global scope.
+- Keep aliases before `use` statements so static analysis can resolve names.
+- If adding newly namespaced core classes with older fallback equivalents, extend this pattern.
+
+## Security-focused rules for this extension
+
+- Never trust arbitrary forwarded IP headers directly; rely on MediaWiki request/user abstractions.
+- Treat allowlist values as CIDR/IP ranges and validate via MediaWiki/Wikimedia helpers (current pattern: `IPUtils::isInRanges`).
+- For denial responses, avoid reflecting unescaped user-controlled data in raw output.
+- Changes that affect denial responses must preserve safe cache behavior and avoid cache-poisoning vectors
+ (status codes, cache headers, and vary semantics should remain deliberate and explicit).
+
+## Logging policy
+
+- Current extension logic is intentionally minimal and may not emit operational logs.
+- If logging is introduced for diagnostics/security events, inject `Psr\Log\LoggerInterface`
+ from `ServiceWiring.php` using channel name `CrawlerProtection`.
+- Do not call `LoggerFactory::getInstance()` outside wiring.
diff --git a/.github/instructions/crawlerprotection-release-i18n.instructions.md b/.github/instructions/crawlerprotection-release-i18n.instructions.md
new file mode 100644
index 0000000..571881f
--- /dev/null
+++ b/.github/instructions/crawlerprotection-release-i18n.instructions.md
@@ -0,0 +1,29 @@
+---
+applyTo: "**/*"
+---
+
+# CrawlerProtection release, i18n, and JSON formatting rules
+
+## Release and CI gates
+
+- This repository enforces a version-bump gate via `.github/scripts/check-version-bump.sh`.
+- Any non-hidden-path content change must bump `extension.json` `version`.
+- Any `i18n/` change must include at least a patch version bump.
+- Admin/maintain collaborators may override the gate in CI; do not rely on override for routine work.
+- Prefer semantic intent:
+ - patch: fixes/documentation clarifications
+ - minor: new config keys/behavior additions
+
+## i18n checklist (when adding/changing user-facing text)
+
+- Update `i18n/en.json` and `i18n/qqq.json` together.
+- Use `crawlerprotection-` prefixed, hyphenated message keys.
+- Keep punctuation/colon inside the message text, not hardcoded in PHP.
+- Avoid dynamically concatenated message keys when finite full keys are known.
+- Run the banana checker when available in your MediaWiki test environment.
+
+## JSON formatting conventions
+
+- Keep JSON files (`extension.json`, `composer.json`, `i18n/*.json`) with 4-space indentation.
+- Preserve key order style already used by surrounding file unless a functional reason requires reordering.
+- Do not reformat unrelated JSON sections in behavior-focused changes.
diff --git a/.github/instructions/crawlerprotection-testing.instructions.md b/.github/instructions/crawlerprotection-testing.instructions.md
new file mode 100644
index 0000000..927fa8e
--- /dev/null
+++ b/.github/instructions/crawlerprotection-testing.instructions.md
@@ -0,0 +1,18 @@
+---
+applyTo: "tests/phpunit/**/*.php"
+---
+
+# CrawlerProtection testing rules
+
+- Unit tests in this repo use `PHPUnit\Framework\TestCase` with local stubs.
+- Keep using `tests/phpunit/stubs.php` and `tests/phpunit/namespaced-stubs.php` for MW shims in unit tests.
+- When production code consumes a new MediaWiki class/function that is unavailable in isolated unit tests,
+ update stubs in the same change.
+- Add regression tests for each bug fix or behavior change.
+- Prefer data providers over duplicated case-by-case tests.
+- Add `@covers` / `@coversDefaultClass` for changed production classes where practical.
+- Prefer adding structure/registration checks (for example, extension.json/service wiring smoke coverage)
+ whenever changing hooks, wiring, or constructor options.
+- For wiring/registration changes, add or update smoke-test coverage strategy notes in the PR,
+ and prefer introducing integration tests under `tests/phpunit/integration/` when CI support is added.
+- Ensure tests remain compatible with supported MediaWiki/PHP versions (including PHP 7.4 syntax limits).
diff --git a/.github/instructions/mediawiki-extensions-clean-code.instructions.md b/.github/instructions/mediawiki-extensions-clean-code.instructions.md
index 507c892..bae19c6 100644
--- a/.github/instructions/mediawiki-extensions-clean-code.instructions.md
+++ b/.github/instructions/mediawiki-extensions-clean-code.instructions.md
@@ -1,5 +1,5 @@
---
-applyTo: "*"
+applyTo: "**/*"
---
# MediaWiki Extension Clean Code Best Practices
diff --git a/.github/instructions/mediawiki-extensions.instructions.md b/.github/instructions/mediawiki-extensions.instructions.md
index ffd5cab..2200094 100644
--- a/.github/instructions/mediawiki-extensions.instructions.md
+++ b/.github/instructions/mediawiki-extensions.instructions.md
@@ -1,5 +1,5 @@
---
-applyTo: "*"
+applyTo: "**/*"
---
# MediaWiki Extension Best Practices
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index f7267c5..b094723 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -7,7 +7,6 @@ on:
push:
branches:
- main
- - specialPageList # Add your development branch
pull_request:
env:
@@ -20,7 +19,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Setup Extension
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
fetch-depth: 0
- name: Check override permission
@@ -66,7 +65,7 @@ jobs:
matrix:
os: [ ubuntu-latest ]
php: [ '8.2', '8.3', '8.4' ]
- mediawiki: [ REL1_43 ]
+ mediawiki: [ REL1_43, REL1_44, REL1_45 ]
include:
- os: ubuntu-latest
php: '7.4'
@@ -76,19 +75,19 @@ jobs:
mediawiki: REL1_39
steps:
- name: Setup PHP
- uses: shivammathur/setup-php@v2
+ uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.37.2
with:
php-version: ${{ matrix.php }}
extensions: mbstring, intl
coverage: none
tools: composer
- name: Setup MediaWiki
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
repository: wikimedia/mediawiki
ref: ${{ matrix.mediawiki }}
- name: Setup Extension
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
path: extensions/${{ env.EXTNAME }}
- name: Setup Composer
@@ -103,6 +102,12 @@ jobs:
run: ./vendor/bin/parallel-lint --exclude node_modules --exclude vendor extensions/${{ env.EXTNAME }}
- name: PHP Code Sniffer
run: cd extensions/${{ env.EXTNAME }} && ../../vendor/bin/phpcs -sp
+ - name: Minus-x
+ # MediaWiki only ships mediawiki/minus-x in its development dependencies from 1.43 onwards,
+ # and composer-merge-plugin does not install the extension's own development dependencies.
+ # The check is version independent, so running it on the newer branches is enough.
+ if: matrix.mediawiki != 'REL1_39'
+ run: cd extensions/${{ env.EXTNAME }} && ../../vendor/bin/minus-x check .
security:
name: Static Analysis
@@ -111,9 +116,9 @@ jobs:
fail-fast: false
matrix:
os: [ ubuntu-latest ]
- # 1.43 phan is broken on php 8.4
+ # phan is broken on php 8.4 for some MW versions
php: [ '8.2', '8.3' ]
- mediawiki: [ REL1_43 ]
+ mediawiki: [ REL1_43, REL1_44, REL1_45 ]
include:
- os: ubuntu-latest
php: '7.4'
@@ -123,19 +128,19 @@ jobs:
mediawiki: REL1_39
steps:
- name: Setup PHP
- uses: shivammathur/setup-php@v2
+ uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.37.2
with:
php-version: ${{ matrix.php }}
extensions: mbstring, intl, ast
coverage: none
tools: composer
- name: Setup MediaWiki
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
repository: wikimedia/mediawiki
ref: ${{ matrix.mediawiki }}
- name: Setup Extension
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
path: extensions/${{ env.EXTNAME }}
- name: Setup Composer
@@ -156,7 +161,7 @@ jobs:
matrix:
os: [ ubuntu-latest ]
php: [ '8.2', '8.3', '8.4' ]
- mediawiki: [ REL1_43 ]
+ mediawiki: [ REL1_43, REL1_44, REL1_45 ]
include:
- os: ubuntu-latest
php: '7.4'
@@ -166,19 +171,19 @@ jobs:
mediawiki: REL1_39
steps:
- name: Setup PHP
- uses: shivammathur/setup-php@v2
+ uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.37.2
with:
php-version: ${{ matrix.php }}
extensions: mbstring, intl, ast
coverage: none
tools: composer
- name: Setup MediaWiki
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
repository: wikimedia/mediawiki
ref: ${{ matrix.mediawiki }}
- name: Setup Extension
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
path: extensions/${{ env.EXTNAME }}
- name: Setup Composer
diff --git a/.github/workflows/copilot-setup-steps.yml b/.github/workflows/copilot-setup-steps.yml
new file mode 100644
index 0000000..1a5ea96
--- /dev/null
+++ b/.github/workflows/copilot-setup-steps.yml
@@ -0,0 +1,38 @@
+name: Copilot Setup Steps
+
+# This workflow exists to pre-provision the same baseline dependencies that Copilot cloud agents
+# need when running repository tasks (PHP/composer + build submodule + extension deps).
+# It is not a replacement for CI and does not run full lint/test jobs.
+on:
+ workflow_dispatch:
+ push:
+ paths:
+ - .github/workflows/copilot-setup-steps.yml
+ pull_request:
+ paths:
+ - .github/workflows/copilot-setup-steps.yml
+
+jobs:
+ copilot-setup-steps:
+ runs-on: ubuntu-latest
+ timeout-minutes: 30
+ permissions:
+ contents: read
+
+ steps:
+ - name: Checkout repository
+ uses: actions/checkout@v7.0.1
+
+ - name: Set up PHP
+ uses: shivammathur/setup-php@v2
+ with:
+ php-version: "8.2"
+ extensions: mbstring, intl, ast
+ coverage: none
+ tools: composer
+
+ - name: Initialize build submodule
+ run: git submodule update --init --recursive
+
+ - name: Install extension dependencies
+ run: composer install --no-interaction --prefer-dist
diff --git a/Makefile b/Makefile
index 315dde4..450f2cd 100644
--- a/Makefile
+++ b/Makefile
@@ -3,7 +3,7 @@ export
# setup for docker-compose-ci build directory
ifeq (,$(wildcard ./build/))
- $(shell git submodule update --init --remote)
+ $(shell git submodule update --init)
endif
EXTENSION=CrawlerProtection
diff --git a/README.md b/README.md
index a5ff8bd..9041b18 100644
--- a/README.md
+++ b/README.md
@@ -4,6 +4,22 @@ Protect wikis against crawler bots. CrawlerProtection denies **anonymous** user
access to certain MediaWiki action URLs and SpecialPages which are resource
intensive.
+# Entry-point coverage
+
+| Entry point | Protected |
+|---|---|
+| `index.php` (page views, action=history, diffs, etc.) | ✅ Always |
+| `index.php` (Special pages) | ✅ Always |
+| `api.php` (Action API modules) | ✅ Configurable via `$wgCrawlerProtectedApiModules` |
+| `rest.php` (REST API paths) | ✅ Configurable via `$wgCrawlerProtectedRestPaths` (MW 1.44+) |
+
+Both `$wgCrawlerProtectedApiModules` and `$wgCrawlerProtectedRestPaths` default to
+an empty list so that upgrades do not silently break existing anonymous API
+consumers. Operators must opt in to API/REST protection by adding entries.
+
+The same bypass semantics apply on every entry point: registered users and IP
+addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
+
# Configuration
* `$wgCrawlerProtectedSpecialPages` - array of special pages to protect
@@ -37,6 +53,25 @@ intensive.
reaches the application. MediaWiki always emits a `title` alongside these
parameters, so their presence without one indicates a crawler-generated
request. Set to `[]` to disable this check.
+* `$wgCrawlerProtectedApiModules` - array of Action API module names to block
+ for anonymous users (default: `[]`). Matches both top-level action names
+ (e.g. `'compare'`, `'parse'`) and the `prop`, `list`, `meta` and `generator`
+ sub-modules of `action=query` (e.g. `'revisions'`, `'recentchanges'`,
+ `'backlinks'`). Matching is case-insensitive. Example that
+ protects the most common crawler-attractive modules:
+ ```php
+ $wgCrawlerProtectedApiModules = [ 'compare', 'parse', 'revisions', 'recentchanges', 'backlinks' ];
+ ```
+* `$wgCrawlerProtectedRestPaths` - array of REST API path glob patterns to
+ block for anonymous users (default: `[]`). Each pattern is tested with
+ `fnmatch()` with the `FNM_PATHNAME` flag, so `*` matches any single path
+ component (it never spans a `/`) and `**` is not supported. Example that
+ protects history and compare endpoints:
+ ```php
+ $wgCrawlerProtectedRestPaths = [ '/page/*/history', '/revision/*/compare/*' ];
+ ```
+ REST protection requires MediaWiki 1.44 or later; the setting is silently
+ ignored on older versions.
* `$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/TESTING.md b/TESTING.md
index 9818459..ad2e46f 100644
--- a/TESTING.md
+++ b/TESTING.md
@@ -60,8 +60,16 @@ composer phpunit # Run unit tests
## Update Docker CI
+To update the `build` submodule to the latest upstream commit, run:
+
```bash
-git submodule update --init --remote
+cd build && git fetch origin && git checkout origin/main && cd ..
+git add build
+git commit -m "build: update docker-compose-ci submodule"
```
-See `.github/CI-SETUP.md` and `.github/DOCKER-CI-QUICKREF.md` for more details.
+Then open a PR with the updated submodule pointer.
+
+> **Note:** The `build` submodule is pinned to a specific commit for reproducibility.
+> The Makefile will check out that exact commit when you run `make`.
+> Do **not** use `--remote` when updating, as that would bypass the pin.
diff --git a/build b/build
new file mode 160000
index 0000000..8c13885
--- /dev/null
+++ b/build
@@ -0,0 +1 @@
+Subproject commit 8c13885f0c746d7f1a27b8f913dd517fe395162e
diff --git a/composer.json b/composer.json
index 5501a9f..ac30fb5 100644
--- a/composer.json
+++ b/composer.json
@@ -1,4 +1,7 @@
{
+ "require": {
+ "php": ">=7.4"
+ },
"require-dev": {
"phpunit/phpunit": "^9.0",
"mediawiki/mediawiki-codesniffer": "43.0.0",
@@ -23,10 +26,12 @@
"scripts": {
"test": [
"@phpcs",
+ "@minus-x",
"@phpunit"
],
"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/"
},
"config": {
diff --git a/extension.json b/extension.json
index 2968838..4736935 100644
--- a/extension.json
+++ b/extension.json
@@ -1,7 +1,7 @@
{
"name": "CrawlerProtection",
"author": "[https://mywikis.com MyWikis LLC]",
- "version": "1.6.1",
+ "version": "1.7.0",
"url": "https://www.mediawiki.org/wiki/Extension:CrawlerProtection",
"descriptionmsg": "crawlerprotection-desc",
"type": "hook",
@@ -20,7 +20,9 @@
}
},
"Hooks": {
+ "ApiCheckCanExecute": "main",
"MediaWikiPerformAction": "main",
+ "RestCheckCanExecute": "main",
"SpecialPageBeforeExecute": "main"
},
"config": {
@@ -30,6 +32,10 @@
],
"merge_strategy": "provide_default"
},
+ "CrawlerProtectedApiModules": {
+ "value": [],
+ "merge_strategy": "provide_default"
+ },
"CrawlerProtectedSpecialPages": {
"value": [
"mobilediff",
@@ -44,6 +50,10 @@
],
"merge_strategy": "provide_default"
},
+ "CrawlerProtectedRestPaths": {
+ "value": [],
+ "merge_strategy": "provide_default"
+ },
"CrawlerProtectionRawDenial": {
"value": false
},
diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php
index 6609bcb..753fa09 100644
--- a/includes/CrawlerProtectionService.php
+++ b/includes/CrawlerProtectionService.php
@@ -46,7 +46,9 @@ class CrawlerProtectionService {
/** @var string[] List of constructor options this class accepts */
public const CONSTRUCTOR_OPTIONS = [
'CrawlerProtectedActions',
+ 'CrawlerProtectedApiModules',
'CrawlerProtectedQueryParams',
+ 'CrawlerProtectedRestPaths',
'CrawlerProtectedSpecialPages',
'CrawlerProtectionAllowedIPs',
'CrawlerProtectionProtectRevisions',
@@ -216,6 +218,114 @@ public function checkSpecialPage(
return true;
}
+ /**
+ * Check whether an Action API module call should be blocked.
+ *
+ * Returns false (= deny) when the module is in the configured
+ * protected-modules list and the caller is anonymous. Returns
+ * true otherwise.
+ *
+ * @param string $moduleName The canonical module name (e.g. "revisions", "compare")
+ * @param User $user
+ * @return bool
+ */
+ public function checkApiModule( string $moduleName, $user ): bool {
+ return $this->checkApiModules( [ $moduleName ], $user );
+ }
+
+ /**
+ * Check whether an Action API request involving the given modules
+ * should be blocked.
+ *
+ * Returns false (= deny) when any of the modules is in the configured
+ * protected-modules list and the caller is anonymous. Returns true
+ * otherwise.
+ *
+ * @param string[] $moduleNames Module names involved in the request, i.e.
+ * the requested action plus, for action=query, its sub-modules
+ * @param User $user
+ * @return bool
+ */
+ public function checkApiModules( array $moduleNames, $user ): bool {
+ if ( $this->cliMode ) {
+ return true;
+ }
+
+ if ( $user->isRegistered() || $this->isIPAllowed( $user->getName() ) ) {
+ return true;
+ }
+
+ foreach ( $moduleNames as $moduleName ) {
+ if ( $this->isProtectedApiModule( $moduleName ) ) {
+ return false;
+ }
+ }
+
+ return true;
+ }
+
+ /**
+ * Determine whether the given API module name is in the
+ * configured list of protected modules.
+ *
+ * The comparison is case-insensitive.
+ *
+ * @param string $moduleName
+ * @return bool
+ */
+ public function isProtectedApiModule( string $moduleName ): bool {
+ $protected = array_map(
+ 'strtolower',
+ $this->options->get( 'CrawlerProtectedApiModules' ) ?? []
+ );
+ return in_array( strtolower( $moduleName ), $protected, true );
+ }
+
+ /**
+ * Check whether a REST API request should be blocked.
+ *
+ * Returns false (= deny) when the path matches a configured
+ * protected pattern and the caller is anonymous. Returns true
+ * otherwise.
+ *
+ * @param string $path The request path (e.g. "/page/Main_Page/history")
+ * @param User $user
+ * @return bool
+ */
+ public function checkRestPath( string $path, $user ): bool {
+ if ( $this->cliMode ) {
+ return true;
+ }
+
+ if ( $user->isRegistered() || $this->isIPAllowed( $user->getName() ) ) {
+ return true;
+ }
+
+ return !$this->isProtectedRestPath( $path );
+ }
+
+ /**
+ * Determine whether the given REST path matches any configured
+ * protected-path pattern.
+ *
+ * Each pattern is tested as a glob (fnmatch) with the FNM_PATHNAME
+ * flag, so a "*" wildcard matches a single path component and never
+ * spans a "/" separator. See $wgCrawlerProtectedRestPaths in the
+ * README for example patterns.
+ *
+ * @param string $path
+ * @return bool
+ */
+ public function isProtectedRestPath( string $path ): bool {
+ $patterns = $this->options->get( 'CrawlerProtectedRestPaths' ) ?? [];
+ foreach ( $patterns as $pattern ) {
+ if ( fnmatch( $pattern, $path, FNM_PATHNAME ) ) {
+ return true;
+ }
+ }
+ return false;
+ }
+
/**
* Determine whether the given special page name is in the
* configured list of protected special pages.
diff --git a/includes/Hooks.php b/includes/Hooks.php
index fa8329f..c6bb64e 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -49,10 +49,13 @@ class_alias( '\Article', '\MediaWiki\Page\Article' );
use MediaWiki\Output\OutputPage;
use MediaWiki\Page\Article;
use MediaWiki\Request\WebRequest;
+use MediaWiki\Rest\HttpException;
+use MediaWiki\Rest\LocalizedHttpException;
use MediaWiki\SpecialPage\Hook\SpecialPageBeforeExecuteHook;
use MediaWiki\SpecialPage\SpecialPage;
use MediaWiki\Title\Title;
use MediaWiki\User\User;
+use Wikimedia\Message\MessageValue;
/**
* Hook handler for the CrawlerProtection extension.
@@ -113,4 +116,98 @@ public function onSpecialPageBeforeExecute( $special, $subPage ) {
$special->getContext()->getUser()
);
}
+
+ /**
+ * Block protected Action API modules for anonymous users.
+ *
+ * Handler for the ApiCheckCanExecute hook, which fires for the requested
+ * action module before it is executed. Returns false and sets an error
+ * message key to deny the request when the module is in the configured
+ * protected list and the caller is anonymous.
+ *
+ * This class deliberately does not implement ApiCheckCanExecuteHook:
+ * hook handlers are dispatched by method name, and not implementing the
+ * interface keeps the handler usable across supported MediaWiki
+ * versions without redeclaring core interfaces.
+ *
+ * @param mixed $module ApiBase instance
+ * @param mixed $user User object
+ * @param string|mixed &$message Error message key set on denial
+ * @return bool|void False to deny execution
+ */
+ public function onApiCheckCanExecute( $module, $user, &$message ) {
+ if ( !$this->crawlerProtectionService->checkApiModules(
+ $this->getApiModuleNames( $module ),
+ $user
+ ) ) {
+ $message = 'crawlerprotection-accessdenied-text';
+ return false;
+ }
+ }
+
+ /**
+ * Collect the API module names involved in a request.
+ *
+ * The ApiCheckCanExecute hook only fires for the requested action, so for
+ * action=query the requested sub-modules are read from the request
+ * parameters in order to make them protectable by name as well.
+ *
+ * @param mixed $module ApiBase instance
+ * @return string[]
+ */
+ private function getApiModuleNames( $module ): array {
+ $names = [ $module->getModuleName() ];
+
+ if ( $names[0] !== 'query' ) {
+ return $names;
+ }
+
+ $request = $module->getMain()->getRequest();
+ foreach ( [ 'prop', 'list', 'meta', 'generator' ] as $param ) {
+ $value = $request->getVal( $param );
+ if ( $value === null || $value === '' ) {
+ continue;
+ }
+ // MediaWiki uses "\x1f" as the separator when a multi-value
+ // parameter starts with that character, and "|" otherwise.
+ $separator = substr( $value, 0, 1 ) === "\x1f" ? "\x1f" : '|';
+ foreach ( explode( $separator, $value ) as $subModule ) {
+ $subModule = trim( $subModule );
+ if ( $subModule !== '' ) {
+ $names[] = $subModule;
+ }
+ }
+ }
+
+ return $names;
+ }
+
+ /**
+ * Block protected REST API paths for anonymous users.
+ *
+ * Handler for the RestCheckCanExecute hook, available on MW 1.44+. On
+ * older versions the hook never fires and this method is never called,
+ * so the REST API is unprotected there.
+ *
+ * As with onApiCheckCanExecute, RestCheckCanExecuteHook is deliberately
+ * not implemented so the class loads on MediaWiki versions that do not
+ * ship that interface.
+ *
+ * @param mixed $module REST Module instance
+ * @param mixed $handler REST Handler instance
+ * @param string $path The request path (e.g. "/page/Main_Page/history")
+ * @param mixed $request PSR-7 request
+ * @param HttpException|null &$error Set to an HttpException to deny the request
+ * @return bool|void False to deny execution
+ */
+ public function onRestCheckCanExecute( $module, $handler, string $path, $request, &$error ) {
+ $user = $handler->getAuthority()->getUser();
+ if ( !$this->crawlerProtectionService->checkRestPath( $path, $user ) ) {
+ $error = new LocalizedHttpException(
+ MessageValue::new( 'crawlerprotection-accessdenied-text' ),
+ 403
+ );
+ return false;
+ }
+ }
}
diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php
index b173c54..2546f53 100644
--- a/tests/phpunit/namespaced-stubs.php
+++ b/tests/phpunit/namespaced-stubs.php
@@ -16,6 +16,58 @@ public function onSpecialPageBeforeExecute( $special, $subPage );
}
}
+// API/REST stubs
+namespace MediaWiki\Rest {
+ class HttpException extends \RuntimeException {
+ /**
+ * @param string $message
+ * @param int $code HTTP status code
+ */
+ public function __construct( string $message = '', int $code = 500 ) {
+ parent::__construct( $message, $code );
+ }
+ }
+
+ class LocalizedHttpException extends HttpException {
+ /**
+ * @param \Wikimedia\Message\MessageValue $messageValue
+ * @param int $code HTTP status code
+ */
+ public function __construct( $messageValue, int $code = 500 ) {
+ parent::__construct( $messageValue->getKey(), $code );
+ }
+ }
+}
+
+namespace Wikimedia\Message {
+ class MessageValue {
+ /** @var string */
+ private string $key;
+
+ /**
+ * @param string $key
+ */
+ public function __construct( string $key ) {
+ $this->key = $key;
+ }
+
+ /**
+ * @param string $key
+ * @return self
+ */
+ public static function new( string $key ): self {
+ return new self( $key );
+ }
+
+ /**
+ * @return string
+ */
+ public function getKey(): string {
+ return $this->key;
+ }
+ }
+}
+
// ServiceOptions stub
namespace MediaWiki\Config {
class ServiceOptions {
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index 0247d8d..f9728af 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -47,6 +47,8 @@ public static function setUpBeforeClass(): void {
* @param bool $protectRevisions
* @param array $protectedQueryParams
* @param bool $cliMode
+ * @param array $protectedApiModules
+ * @param array $protectedRestPaths
* @return CrawlerProtectionService
*/
private function buildService(
@@ -56,13 +58,17 @@ private function buildService(
$responseFactory = null,
bool $protectRevisions = true,
array $protectedQueryParams = [ 'target' ],
- bool $cliMode = false
+ bool $cliMode = false,
+ array $protectedApiModules = [],
+ array $protectedRestPaths = []
): CrawlerProtectionService {
$options = new ServiceOptions(
CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
[
'CrawlerProtectedActions' => $protectedActions,
+ 'CrawlerProtectedApiModules' => $protectedApiModules,
'CrawlerProtectedQueryParams' => $protectedQueryParams,
+ 'CrawlerProtectedRestPaths' => $protectedRestPaths,
'CrawlerProtectedSpecialPages' => $protectedPages,
'CrawlerProtectionAllowedIPs' => $allowedIPs,
'CrawlerProtectionProtectRevisions' => $protectRevisions,
@@ -946,4 +952,325 @@ public function provideAllowedIPs(): array {
'String instead of array' => [ '1.2.3.4', '1.2.3.4' ],
];
}
+
+ // ---------------------------------------------------------------
+ // isProtectedApiModule tests
+ // ---------------------------------------------------------------
+
+ /**
+ * @covers ::isProtectedApiModule
+ */
+ public function testIsProtectedApiModuleReturnsTrueForConfiguredModule() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions', 'compare' ]
+ );
+ $this->assertTrue( $service->isProtectedApiModule( 'revisions' ) );
+ $this->assertTrue( $service->isProtectedApiModule( 'compare' ) );
+ }
+
+ /**
+ * @covers ::isProtectedApiModule
+ */
+ public function testIsProtectedApiModuleReturnsFalseForUnconfiguredModule() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertFalse( $service->isProtectedApiModule( 'query' ) );
+ $this->assertFalse( $service->isProtectedApiModule( 'parse' ) );
+ }
+
+ /**
+ * @covers ::isProtectedApiModule
+ */
+ public function testIsProtectedApiModuleReturnsFalseWhenListEmpty() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, []
+ );
+ $this->assertFalse( $service->isProtectedApiModule( 'revisions' ) );
+ }
+
+ /**
+ * @covers ::isProtectedApiModule
+ */
+ public function testIsProtectedApiModuleIsCaseInsensitive() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'Revisions' ]
+ );
+ $this->assertTrue( $service->isProtectedApiModule( 'revisions' ) );
+ $this->assertTrue( $service->isProtectedApiModule( 'REVISIONS' ) );
+ $this->assertTrue( $service->isProtectedApiModule( 'Revisions' ) );
+ }
+
+ // ---------------------------------------------------------------
+ // checkApiModule tests
+ // ---------------------------------------------------------------
+
+ /**
+ * @covers ::checkApiModule
+ */
+ public function testCheckApiModuleAllowsRegisteredUser() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( true );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertTrue( $service->checkApiModule( 'revisions', $user ) );
+ }
+
+ /**
+ * @covers ::checkApiModule
+ */
+ public function testCheckApiModuleBlocksAnonymousForProtectedModule() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertFalse( $service->checkApiModule( 'revisions', $user ) );
+ }
+
+ /**
+ * @covers ::checkApiModule
+ */
+ public function testCheckApiModuleAllowsAnonymousForUnprotectedModule() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertTrue( $service->checkApiModule( 'query', $user ) );
+ }
+
+ /**
+ * @covers ::checkApiModule
+ */
+ public function testCheckApiModuleAllowsWhenListEmpty() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, []
+ );
+ $this->assertTrue( $service->checkApiModule( 'revisions', $user ) );
+ }
+
+ /**
+ * @covers ::checkApiModule
+ */
+ public function testCheckApiModuleAllowsOnCommandLine() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], true, [ 'revisions' ]
+ );
+ $this->assertTrue( $service->checkApiModule( 'revisions', $user ) );
+ }
+
+ /**
+ * @covers ::checkApiModules
+ */
+ public function testCheckApiModulesBlocksWhenAnySubModuleIsProtected() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertFalse(
+ $service->checkApiModules( [ 'query', 'links', 'revisions' ], $user )
+ );
+ }
+
+ /**
+ * @covers ::checkApiModules
+ */
+ public function testCheckApiModulesAllowsWhenNoModuleIsProtected() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertTrue(
+ $service->checkApiModules( [ 'query', 'links', 'categories' ], $user )
+ );
+ }
+
+ /**
+ * @covers ::checkApiModules
+ */
+ public function testCheckApiModulesAllowsRegisteredUser() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( true );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertTrue( $service->checkApiModules( [ 'query', 'revisions' ], $user ) );
+ }
+
+ /**
+ * @covers ::checkApiModule
+ */
+ public function testCheckApiModuleAllowsAllowedIP() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [ '1.2.3.4' ], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertTrue( $service->checkApiModule( 'revisions', $user ) );
+ }
+
+ // ---------------------------------------------------------------
+ // isProtectedRestPath tests
+ // ---------------------------------------------------------------
+
+ /**
+ * @covers ::isProtectedRestPath
+ */
+ public function testIsProtectedRestPathReturnsFalseWhenListEmpty() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], []
+ );
+ $this->assertFalse( $service->isProtectedRestPath( '/page/Main_Page/history' ) );
+ }
+
+ /**
+ * @covers ::isProtectedRestPath
+ */
+ public function testIsProtectedRestPathMatchesExactPath() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/Main_Page/history' ]
+ );
+ $this->assertTrue( $service->isProtectedRestPath( '/page/Main_Page/history' ) );
+ }
+
+ /**
+ * @covers ::isProtectedRestPath
+ */
+ public function testIsProtectedRestPathMatchesGlobPattern() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertTrue( $service->isProtectedRestPath( '/page/Main_Page/history' ) );
+ $this->assertTrue( $service->isProtectedRestPath( '/page/Talk:Foo/history' ) );
+ }
+
+ /**
+ * @covers ::isProtectedRestPath
+ */
+ public function testIsProtectedRestPathDoesNotMatchUnrelatedPath() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertFalse( $service->isProtectedRestPath( '/page/Main_Page' ) );
+ $this->assertFalse( $service->isProtectedRestPath( '/search' ) );
+ }
+
+ /**
+ * @covers ::isProtectedRestPath
+ */
+ public function testIsProtectedRestPathWildcardDoesNotSpanSlash() {
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertFalse( $service->isProtectedRestPath( '/page/Foo/Bar/history' ) );
+ }
+
+ // ---------------------------------------------------------------
+ // checkRestPath tests
+ // ---------------------------------------------------------------
+
+ /**
+ * @covers ::checkRestPath
+ */
+ public function testCheckRestPathAllowsRegisteredUser() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( true );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user ) );
+ }
+
+ /**
+ * @covers ::checkRestPath
+ */
+ public function testCheckRestPathBlocksAnonymousForProtectedPath() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertFalse( $service->checkRestPath( '/page/Main_Page/history', $user ) );
+ }
+
+ /**
+ * @covers ::checkRestPath
+ */
+ public function testCheckRestPathAllowsAnonymousForUnprotectedPath() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertTrue( $service->checkRestPath( '/search', $user ) );
+ }
+
+ /**
+ * @covers ::checkRestPath
+ */
+ public function testCheckRestPathAllowsWhenListEmpty() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], []
+ );
+ $this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user ) );
+ }
+
+ /**
+ * @covers ::checkRestPath
+ */
+ public function testCheckRestPathAllowsOnCommandLine() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], true, [], [ '/page/*/history' ]
+ );
+ $this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user ) );
+ }
+
+ /**
+ * @covers ::checkRestPath
+ */
+ public function testCheckRestPathAllowsAllowedIP() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->method( 'getName' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [ '1.2.3.4' ], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user ) );
+ }
}
diff --git a/tests/phpunit/unit/HooksTest.php b/tests/phpunit/unit/HooksTest.php
index aead599..612a030 100644
--- a/tests/phpunit/unit/HooksTest.php
+++ b/tests/phpunit/unit/HooksTest.php
@@ -199,4 +199,254 @@ public function getOutput() {
}
};
}
+
+ /**
+ * Build a stub API module exposing a module name and request parameters.
+ *
+ * @param string $moduleName
+ * @param array $params
+ * @return \stdClass Stub API module
+ */
+ private function makeApiModule( string $moduleName, array $params = [] ) {
+ $request = new class( $params ) {
+ /** @var array */
+ private $params;
+
+ public function __construct( array $params ) {
+ $this->params = $params;
+ }
+
+ public function getVal( $name ) {
+ return $this->params[$name] ?? null;
+ }
+ };
+
+ $main = new class( $request ) {
+ /** @var \stdClass */
+ private $request;
+
+ public function __construct( $request ) {
+ $this->request = $request;
+ }
+
+ public function getRequest() {
+ return $this->request;
+ }
+ };
+
+ return new class( $moduleName, $main ) {
+ /** @var string */
+ private $moduleName;
+ /** @var \stdClass */
+ private $main;
+
+ public function __construct( string $moduleName, $main ) {
+ $this->moduleName = $moduleName;
+ $this->main = $main;
+ }
+
+ public function getModuleName(): string {
+ return $this->moduleName;
+ }
+
+ public function getMain() {
+ return $this->main;
+ }
+ };
+ }
+
+ /**
+ * @covers ::onApiCheckCanExecute
+ */
+ public function testOnApiCheckCanExecuteDelegatesToService() {
+ $user = $this->createMock( self::$userClassName );
+ $module = $this->makeApiModule( 'compare' );
+
+ $service = $this->createMock( CrawlerProtectionService::class );
+ $service->expects( $this->once() )
+ ->method( 'checkApiModules' )
+ ->with( [ 'compare' ], $user )
+ ->willReturn( false );
+
+ $hooks = new Hooks( $service );
+ $message = null;
+ $result = $hooks->onApiCheckCanExecute( $module, $user, $message );
+
+ $this->assertFalse( $result );
+ $this->assertNotNull( $message );
+ }
+
+ /**
+ * @covers ::onApiCheckCanExecute
+ */
+ public function testOnApiCheckCanExecutePassesThroughWhenAllowed() {
+ $user = $this->createMock( self::$userClassName );
+ $module = $this->makeApiModule( 'query' );
+
+ $service = $this->createMock( CrawlerProtectionService::class );
+ $service->expects( $this->once() )
+ ->method( 'checkApiModules' )
+ ->with( [ 'query' ], $user )
+ ->willReturn( true );
+
+ $hooks = new Hooks( $service );
+ $message = null;
+ $result = $hooks->onApiCheckCanExecute( $module, $user, $message );
+
+ // Returns null (no explicit false) when allowed
+ $this->assertNotFalse( $result );
+ $this->assertNull( $message );
+ }
+
+ /**
+ * @covers ::onApiCheckCanExecute
+ */
+ public function testOnApiCheckCanExecuteCollectsQuerySubModules() {
+ $user = $this->createMock( self::$userClassName );
+ $module = $this->makeApiModule( 'query', [
+ 'prop' => 'revisions|links',
+ 'list' => 'recentchanges',
+ 'meta' => '',
+ 'generator' => 'allpages',
+ ] );
+
+ $service = $this->createMock( CrawlerProtectionService::class );
+ $service->expects( $this->once() )
+ ->method( 'checkApiModules' )
+ ->with(
+ [ 'query', 'revisions', 'links', 'recentchanges', 'allpages' ],
+ $user
+ )
+ ->willReturn( true );
+
+ $hooks = new Hooks( $service );
+ $message = null;
+ $hooks->onApiCheckCanExecute( $module, $user, $message );
+ }
+
+ /**
+ * @covers ::onApiCheckCanExecute
+ */
+ public function testOnApiCheckCanExecuteHandlesUnitSeparatorMultiValues() {
+ $user = $this->createMock( self::$userClassName );
+ $module = $this->makeApiModule( 'query', [
+ 'prop' => "\x1frevisions\x1flinks",
+ ] );
+
+ $service = $this->createMock( CrawlerProtectionService::class );
+ $service->expects( $this->once() )
+ ->method( 'checkApiModules' )
+ ->with( [ 'query', 'revisions', 'links' ], $user )
+ ->willReturn( true );
+
+ $hooks = new Hooks( $service );
+ $message = null;
+ $hooks->onApiCheckCanExecute( $module, $user, $message );
+ }
+
+ /**
+ * @covers ::onApiCheckCanExecute
+ */
+ public function testOnApiCheckCanExecuteIgnoresSubModulesForOtherActions() {
+ $user = $this->createMock( self::$userClassName );
+ $module = $this->makeApiModule( 'parse', [ 'prop' => 'links|templates' ] );
+
+ $service = $this->createMock( CrawlerProtectionService::class );
+ $service->expects( $this->once() )
+ ->method( 'checkApiModules' )
+ ->with( [ 'parse' ], $user )
+ ->willReturn( true );
+
+ $hooks = new Hooks( $service );
+ $message = null;
+ $hooks->onApiCheckCanExecute( $module, $user, $message );
+ }
+
+ /**
+ * Build a stub REST handler exposing an authority for the given user.
+ *
+ * @param \PHPUnit\Framework\MockObject\MockObject $user
+ * @return \stdClass Stub REST handler
+ */
+ private function makeRestHandler( $user ) {
+ $authority = new class( $user ) {
+ /** @var \PHPUnit\Framework\MockObject\MockObject */
+ private $user;
+
+ /**
+ * @param \PHPUnit\Framework\MockObject\MockObject $user
+ */
+ public function __construct( $user ) {
+ $this->user = $user;
+ }
+
+ /**
+ * @return \PHPUnit\Framework\MockObject\MockObject
+ */
+ public function getUser() {
+ return $this->user;
+ }
+ };
+
+ return new class( $authority ) {
+ /** @var \stdClass */
+ private $authority;
+
+ /**
+ * @param \stdClass $authority
+ */
+ public function __construct( $authority ) {
+ $this->authority = $authority;
+ }
+
+ /**
+ * @return \stdClass
+ */
+ public function getAuthority() {
+ return $this->authority;
+ }
+ };
+ }
+
+ /**
+ * @covers ::onRestCheckCanExecute
+ */
+ public function testOnRestCheckCanExecuteDelegatesToService() {
+ $user = $this->createMock( self::$userClassName );
+ $handler = $this->makeRestHandler( $user );
+
+ $service = $this->createMock( CrawlerProtectionService::class );
+ $service->expects( $this->once() )
+ ->method( 'checkRestPath' )
+ ->with( '/page/Main_Page/history', $user )
+ ->willReturn( false );
+
+ $hooks = new Hooks( $service );
+ $error = null;
+ $result = $hooks->onRestCheckCanExecute( null, $handler, '/page/Main_Page/history', null, $error );
+
+ $this->assertFalse( $result );
+ $this->assertNotNull( $error );
+ }
+
+ /**
+ * @covers ::onRestCheckCanExecute
+ */
+ public function testOnRestCheckCanExecutePassesThroughWhenAllowed() {
+ $user = $this->createMock( self::$userClassName );
+ $handler = $this->makeRestHandler( $user );
+
+ $service = $this->createMock( CrawlerProtectionService::class );
+ $service->expects( $this->once() )
+ ->method( 'checkRestPath' )
+ ->with( '/search', $user )
+ ->willReturn( true );
+
+ $hooks = new Hooks( $service );
+ $error = null;
+ $result = $hooks->onRestCheckCanExecute( null, $handler, '/search', null, $error );
+
+ $this->assertNotFalse( $result );
+ $this->assertNull( $error );
+ }
}
From 1fda40958028524208c3539705998fae950ebb39 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Wed, 29 Jul 2026 21:44:52 -0700
Subject: [PATCH 03/16] Fix IP allowlist to use WebRequest::getIP(); add
temp-account policy; normalize array configs (#57)
* Initial plan
* Fix IP allowlist to use WebRequest::getIP(), add TreatTempUsersAsAnon config, normalize array configs
- CrawlerProtectionService: derive client IP from $request->getIP() (not
$user->getName()) in both checkPerformAction() and checkSpecialPage()
- Add $request parameter to checkSpecialPage(); update Hooks.php to pass
$special->getContext()->getRequest()
- Add CrawlerProtectionTreatTempUsersAsAnon config (default false) and
isUserAllowed() helper; temp-account users (isRegistered+isTemp) are
treated as anonymous when the flag is true
- Pre-normalise all array-valued configs at construction time: coerce
scalars to single-element arrays, drop non-string entries, log warnings
(prevents fatal on misconfigured scalar values)
- Validate CrawlerProtectionAllowedIPs entries at construction time and
log a warning for invalid IP/range strings
- Inject LoggerInterface via constructor and ServiceWiring.php
- Add CrawlerProtectionTreatTempUsersAsAnon to extension.json
- Update tests: fix IP tests to use request->getIP(), add $request param
to checkSpecialPage() calls, add tests for temp accounts, IP-from-
request, and scalar misconfiguration
- Add getIP() to WebRequest stub for unit tests
- Document new config options in README.md
* Operationalize Copilot guidance with repo-specific rules, correct scope, and agent setup workflow (#61)
* Initial plan
* Overhaul Copilot instructions with repo-specific CI architecture and testing guidance
* Clarify trimmed guidance and Copilot setup workflow intent
* Restore comprehensive MediaWiki instruction guidance
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* Validate IP allowlist entries with IPUtils only; make temp-user mocks version-safe
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* ci: pin actions to SHAs, extend matrix to MW 1.44/1.45, add Dependabot, enforce minus-x (#59)
* Initial plan
* ci: hygiene improvements - pin actions, extend matrix, add dependabot
* ci: skip minus-x on REL1_39 where MediaWiki does not ship it
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* tests: give the Hooks mock context a getRequest() so special-page tests pass
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
Co-authored-by: Jeffrey Wang
---
.github/workflows/ci.yml | 2 +-
README.md | 14 +
extension.json | 3 +
includes/CrawlerProtectionService.php | 164 +++++++++-
includes/Hooks.php | 3 +-
includes/ServiceWiring.php | 4 +-
tests/phpunit/namespaced-stubs.php | 4 +
.../unit/CrawlerProtectionServiceTest.php | 302 +++++++++++++++++-
tests/phpunit/unit/HooksTest.php | 27 +-
9 files changed, 485 insertions(+), 38 deletions(-)
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index b094723..73a5565 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -53,7 +53,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Setup Extension
- uses: actions/checkout@v4
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- name: Check qqq.json completeness
run: .github/scripts/check-i18n-qqq.sh
diff --git a/README.md b/README.md
index 9041b18..12ff83b 100644
--- a/README.md
+++ b/README.md
@@ -76,4 +76,18 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
`die();` with
[418 I'm a teapot](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/418)
code (default: `false`)
+* `$wgCrawlerProtectionAllowedIPs` - array of IP addresses or ranges that are
+ always allowed through, even for anonymous requests (default: `[]`). Supports
+ 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.
+* `$wgCrawlerProtectionTreatTempUsersAsAnon` - when `true`, users with
+ [temporary accounts](https://www.mediawiki.org/wiki/Help:Temporary_accounts)
+ (`$wgAutoCreateTempUser`, available since MediaWiki 1.42) are treated as
+ anonymous and subject to protection like any other non-logged-in visitor.
+ When `false` (default), temporary-account users are treated as registered
+ users and bypass all protection checks. Set to `true` if you do not want
+ crawlers that receive a temporary account to bypass protection.
diff --git a/extension.json b/extension.json
index 4736935..9c81940 100644
--- a/extension.json
+++ b/extension.json
@@ -72,6 +72,9 @@
},
"CrawlerProtectionProtectRevisions": {
"value": true
+ },
+ "CrawlerProtectionTreatTempUsersAsAnon": {
+ "value": false
}
},
"ServiceWiringFiles": [
diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php
index 753fa09..6cbcdfa 100644
--- a/includes/CrawlerProtectionService.php
+++ b/includes/CrawlerProtectionService.php
@@ -29,6 +29,7 @@
use MediaWiki\Output\OutputPage;
use MediaWiki\Request\WebRequest;
use MediaWiki\User\User;
+use Psr\Log\LoggerInterface;
use Wikimedia\IPUtils;
/**
@@ -52,6 +53,7 @@ class CrawlerProtectionService {
'CrawlerProtectedSpecialPages',
'CrawlerProtectionAllowedIPs',
'CrawlerProtectionProtectRevisions',
+ 'CrawlerProtectionTreatTempUsersAsAnon',
];
/** @var ServiceOptions */
@@ -63,20 +65,56 @@ class CrawlerProtectionService {
/** @var bool */
private bool $cliMode;
+ /** @var LoggerInterface */
+ private LoggerInterface $logger;
+
+ /** @var string[] Normalised CrawlerProtectedActions */
+ private array $normalizedProtectedActions;
+
+ /** @var string[] Normalised CrawlerProtectedApiModules */
+ private array $normalizedProtectedApiModules;
+
+ /** @var string[] Normalised CrawlerProtectedRestPaths */
+ private array $normalizedProtectedRestPaths;
+
+ /** @var string[] Normalised CrawlerProtectedQueryParams */
+ private array $normalizedProtectedQueryParams;
+
+ /** @var string[] Normalised CrawlerProtectedSpecialPages */
+ private array $normalizedProtectedSpecialPages;
+
+ /** @var string[] Normalised and validated CrawlerProtectionAllowedIPs */
+ private array $normalizedAllowedIPs;
+
/**
* @param ServiceOptions $options
* @param ResponseFactory $responseFactory
* @param bool $cliMode
+ * @param LoggerInterface $logger
*/
public function __construct(
ServiceOptions $options,
ResponseFactory $responseFactory,
- bool $cliMode
+ bool $cliMode,
+ LoggerInterface $logger
) {
$options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
$this->options = $options;
$this->responseFactory = $responseFactory;
$this->cliMode = $cliMode;
+ $this->logger = $logger;
+
+ // Pre-normalise all array-valued configs at construction time so that a
+ // misconfigured scalar never fatals on array_map / foreach, and so that
+ // any warning is logged exactly once per request rather than per call.
+ $this->normalizedProtectedActions = $this->normalizeArrayConfig( 'CrawlerProtectedActions' );
+ $this->normalizedProtectedApiModules = $this->normalizeArrayConfig( 'CrawlerProtectedApiModules' );
+ $this->normalizedProtectedRestPaths = $this->normalizeArrayConfig( 'CrawlerProtectedRestPaths' );
+ $this->normalizedProtectedQueryParams = $this->normalizeArrayConfig( 'CrawlerProtectedQueryParams' );
+ $this->normalizedProtectedSpecialPages = $this->normalizeArrayConfig( 'CrawlerProtectedSpecialPages' );
+ $this->normalizedAllowedIPs = $this->normalizeAndValidateIPs(
+ $this->normalizeArrayConfig( 'CrawlerProtectionAllowedIPs' )
+ );
}
/**
@@ -99,7 +137,13 @@ public function checkPerformAction(
return true;
}
- if ( $user->isRegistered() || $this->isIPAllowed( $user->getName() ) ) {
+ if ( $this->isUserAllowed( $user ) ) {
+ return true;
+ }
+
+ // Use the canonical client IP from WebRequest (correctly applies
+ // trusted-proxy / X-Forwarded-For handling) rather than the username.
+ if ( $this->normalizedAllowedIPs !== [] && $this->isIPAllowed( $request->getIP() ) ) {
return true;
}
@@ -154,7 +198,7 @@ public function hasProtectedQueryParam( $request ): bool {
return false;
}
- foreach ( $this->options->get( 'CrawlerProtectedQueryParams' ) as $param ) {
+ foreach ( $this->normalizedProtectedQueryParams as $param ) {
if ( $request->getVal( $param ) !== null ) {
return true;
}
@@ -180,7 +224,7 @@ public function isProtectedAction( ?string $action ): bool {
$protectedActions = array_map(
'strtolower',
- $this->options->get( 'CrawlerProtectedActions' )
+ $this->normalizedProtectedActions
);
return in_array( strtolower( $action ), $protectedActions, true );
@@ -195,18 +239,26 @@ public function isProtectedAction( ?string $action ): bool {
* @param string $specialPageName The canonical special page name
* @param OutputPage $output
* @param User $user
+ * @param WebRequest $request
* @return bool
*/
public function checkSpecialPage(
string $specialPageName,
$output,
- $user
+ $user,
+ $request
): bool {
if ( $this->cliMode ) {
return true;
}
- if ( $user->isRegistered() || $this->isIPAllowed( $user->getName() ) ) {
+ if ( $this->isUserAllowed( $user ) ) {
+ return true;
+ }
+
+ // Use the canonical client IP from WebRequest (correctly applies
+ // trusted-proxy / X-Forwarded-For handling) rather than the username.
+ if ( $this->normalizedAllowedIPs !== [] && $this->isIPAllowed( $request->getIP() ) ) {
return true;
}
@@ -251,7 +303,7 @@ public function checkApiModules( array $moduleNames, $user ): bool {
return true;
}
- if ( $user->isRegistered() || $this->isIPAllowed( $user->getName() ) ) {
+ if ( $this->isUserAllowed( $user ) || $this->isIPAllowed( $user->getName() ) ) {
return true;
}
@@ -276,7 +328,7 @@ public function checkApiModules( array $moduleNames, $user ): bool {
public function isProtectedApiModule( string $moduleName ): bool {
$protected = array_map(
'strtolower',
- $this->options->get( 'CrawlerProtectedApiModules' ) ?? []
+ $this->normalizedProtectedApiModules
);
return in_array( strtolower( $moduleName ), $protected, true );
}
@@ -297,7 +349,7 @@ public function checkRestPath( string $path, $user ): bool {
return true;
}
- if ( $user->isRegistered() || $this->isIPAllowed( $user->getName() ) ) {
+ if ( $this->isUserAllowed( $user ) || $this->isIPAllowed( $user->getName() ) ) {
return true;
}
@@ -317,7 +369,7 @@ public function checkRestPath( string $path, $user ): bool {
* @return bool
*/
public function isProtectedRestPath( string $path ): bool {
- $patterns = $this->options->get( 'CrawlerProtectedRestPaths' ) ?? [];
+ $patterns = $this->normalizedProtectedRestPaths;
foreach ( $patterns as $pattern ) {
if ( fnmatch( $pattern, $path, FNM_PATHNAME ) ) {
return true;
@@ -341,8 +393,6 @@ public function isProtectedRestPath( string $path ): bool {
* @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(
@@ -354,7 +404,7 @@ static function ( string $p ): string {
}
return $lower;
},
- $protectedSpecialPages
+ $this->normalizedProtectedSpecialPages
);
$name = strtolower( $specialPageName );
@@ -369,12 +419,92 @@ static function ( string $p ): string {
* @return bool
*/
private function isIPAllowed( string $ip ): bool {
- $allowedIPs = $this->options->get( 'CrawlerProtectionAllowedIPs' );
+ return IPUtils::isInRanges( $ip, $this->normalizedAllowedIPs );
+ }
- if ( !is_array( $allowedIPs ) ) {
- $allowedIPs = [ $allowedIPs ];
+ /**
+ * Determine whether the given user should be allowed through the protection
+ * without an IP check.
+ *
+ * Registered users are allowed unless CrawlerProtectionTreatTempUsersAsAnon
+ * is true and the user holds a temporary account. Temporary accounts were
+ * introduced in MediaWiki 1.42 (User::isTemp()); on earlier versions the
+ * method does not exist and the guard below is a no-op.
+ *
+ * @param User $user
+ * @return bool
+ */
+ private function isUserAllowed( $user ): bool {
+ if ( !$user->isRegistered() ) {
+ return false;
+ }
+
+ // When CrawlerProtectionTreatTempUsersAsAnon is true, a temporary-account
+ // user is treated as anonymous so the rest of the protection logic applies.
+ if ( $this->options->get( 'CrawlerProtectionTreatTempUsersAsAnon' )
+ && method_exists( $user, 'isTemp' )
+ && $user->isTemp()
+ ) {
+ return false;
+ }
+
+ return true;
+ }
+
+ /**
+ * Coerce a config value to a string array, logging a warning if the raw
+ * value is not already an array or contains non-string elements.
+ *
+ * @param string $configKey
+ * @return string[]
+ */
+ private function normalizeArrayConfig( string $configKey ): array {
+ $value = $this->options->get( $configKey );
+
+ if ( !is_array( $value ) ) {
+ $this->logger->warning(
+ 'CrawlerProtection: Config {configKey} should be an array; got a scalar. ' .
+ 'Treating it as a single-element array.',
+ [ 'configKey' => $configKey ]
+ );
+ $value = [ $value ];
+ }
+
+ $filtered = array_values( array_filter( $value, 'is_string' ) );
+
+ if ( count( $filtered ) !== count( $value ) ) {
+ $this->logger->warning(
+ 'CrawlerProtection: Config {configKey} contains non-string entries; ' .
+ 'they have been ignored.',
+ [ 'configKey' => $configKey ]
+ );
}
- return IPUtils::isInRanges( $ip, $allowedIPs );
+ return $filtered;
+ }
+
+ /**
+ * Validate IP allowlist entries and log a warning for any entry that does
+ * not look like a valid IP address, CIDR range, or explicit range.
+ *
+ * Invalid entries are kept in the returned array because
+ * IPUtils::isInRanges() handles them gracefully (returns false), so they
+ * do not cause errors; the warning simply makes typos discoverable.
+ *
+ * @param string[] $ips
+ * @return string[]
+ */
+ private function normalizeAndValidateIPs( array $ips ): array {
+ foreach ( $ips as $ip ) {
+ // Accept single IPs, CIDR ranges, and explicit "a - b" ranges.
+ if ( !IPUtils::isIPAddress( $ip ) && !IPUtils::isValidRange( $ip ) ) {
+ $this->logger->warning(
+ 'CrawlerProtection: CrawlerProtectionAllowedIPs entry "{ip}" does not look like ' .
+ 'a valid IP address, CIDR range, or explicit IP range.',
+ [ 'ip' => $ip ]
+ );
+ }
+ }
+ return $ips;
}
}
diff --git a/includes/Hooks.php b/includes/Hooks.php
index c6bb64e..54c25e5 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -113,7 +113,8 @@ public function onSpecialPageBeforeExecute( $special, $subPage ) {
return $this->crawlerProtectionService->checkSpecialPage(
$special->getName(),
$special->getContext()->getOutput(),
- $special->getContext()->getUser()
+ $special->getContext()->getUser(),
+ $special->getContext()->getRequest()
);
}
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index a5e1d51..58acf32 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -26,6 +26,7 @@
use MediaWiki\Config\ServiceOptions;
use MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService;
use MediaWiki\Extension\CrawlerProtection\ResponseFactory;
+use MediaWiki\Logger\LoggerFactory;
use MediaWiki\MediaWikiServices;
return [
@@ -46,7 +47,8 @@ static function ( MediaWikiServices $services ): CrawlerProtectionService {
$services->getMainConfig()
),
$services->get( 'CrawlerProtection.ResponseFactory' ),
- defined( 'MW_ENTRY_POINT' ) && MW_ENTRY_POINT === 'cli'
+ defined( 'MW_ENTRY_POINT' ) && MW_ENTRY_POINT === 'cli',
+ LoggerFactory::getInstance( 'CrawlerProtection' )
);
},
];
diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php
index 2546f53..e556cf7 100644
--- a/tests/phpunit/namespaced-stubs.php
+++ b/tests/phpunit/namespaced-stubs.php
@@ -167,6 +167,10 @@ class WebRequest {
public function getVal( $name, $default = null ) {
return $default;
}
+
+ public function getIP(): string {
+ return '127.0.0.1';
+ }
}
}
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index f9728af..e9002a7 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -6,6 +6,7 @@
use MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService;
use MediaWiki\Extension\CrawlerProtection\ResponseFactory;
use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
/**
* @coversDefaultClass \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService
@@ -49,6 +50,7 @@ public static function setUpBeforeClass(): void {
* @param bool $cliMode
* @param array $protectedApiModules
* @param array $protectedRestPaths
+ * @param bool $treatTempUsersAsAnon
* @return CrawlerProtectionService
*/
private function buildService(
@@ -60,7 +62,8 @@ private function buildService(
array $protectedQueryParams = [ 'target' ],
bool $cliMode = false,
array $protectedApiModules = [],
- array $protectedRestPaths = []
+ array $protectedRestPaths = [],
+ bool $treatTempUsersAsAnon = false
): CrawlerProtectionService {
$options = new ServiceOptions(
CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
@@ -72,12 +75,39 @@ private function buildService(
'CrawlerProtectedSpecialPages' => $protectedPages,
'CrawlerProtectionAllowedIPs' => $allowedIPs,
'CrawlerProtectionProtectRevisions' => $protectRevisions,
+ 'CrawlerProtectionTreatTempUsersAsAnon' => $treatTempUsersAsAnon,
]
);
$responseFactory ??= $this->createMock( ResponseFactory::class );
- return new CrawlerProtectionService( $options, $responseFactory, $cliMode );
+ return new CrawlerProtectionService( $options, $responseFactory, $cliMode, new NullLogger() );
+ }
+
+ /**
+ * Build a registered user mock whose isTemp() returns the given value.
+ *
+ * User::isTemp() only exists in MediaWiki 1.42 and later, so the method is
+ * added to the mock when the underlying class does not define it.
+ *
+ * @param bool $isTemp
+ * @return \PHPUnit\Framework\MockObject\MockObject
+ */
+ private function newTempAwareUserMock( bool $isTemp ) {
+ $builder = $this->getMockBuilder( self::$userClassName )
+ ->disableOriginalConstructor();
+
+ if ( method_exists( self::$userClassName, 'isTemp' ) ) {
+ $builder->onlyMethods( [ 'isRegistered', 'isTemp' ] );
+ } else {
+ $builder->onlyMethods( [ 'isRegistered' ] )->addMethods( [ 'isTemp' ] );
+ }
+
+ $user = $builder->getMock();
+ $user->method( 'isRegistered' )->willReturn( true );
+ $user->method( 'isTemp' )->willReturn( $isTemp );
+
+ return $user;
}
// ---------------------------------------------------------------
@@ -415,7 +445,6 @@ public function testCheckPerformActionAllowsRevisionsWhenNotConfigured(
$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 );
@@ -468,7 +497,6 @@ public function testCheckPerformActionBlocksRevisionsWhenProtectRevisionsTrueHis
$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 );
@@ -535,7 +563,6 @@ public function testCheckPerformActionAllowsRevisionsWhenProtectRevisionsFalse(
$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 );
@@ -561,7 +588,6 @@ public function testCheckPerformActionBlocksHistoryListingEvenWhenProtectRevisio
$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( [
@@ -731,6 +757,8 @@ public function testCheckSpecialPageBlocksAnonymous( string $specialPageName ) {
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
$responseFactory = $this->createMock( ResponseFactory::class );
$responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
@@ -740,7 +768,7 @@ public function testCheckSpecialPageBlocksAnonymous( string $specialPageName ) {
[],
$responseFactory
);
- $this->assertFalse( $service->checkSpecialPage( $specialPageName, $output, $user ) );
+ $this->assertFalse( $service->checkSpecialPage( $specialPageName, $output, $user, $request ) );
}
/**
@@ -754,6 +782,8 @@ public function testCheckSpecialPageAllowsRegistered( string $specialPageName )
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( true );
+ $request = $this->createMock( self::$webRequestClassName );
+
$responseFactory = $this->createMock( ResponseFactory::class );
$responseFactory->expects( $this->never() )->method( 'denyAccess' );
@@ -763,7 +793,7 @@ public function testCheckSpecialPageAllowsRegistered( string $specialPageName )
[],
$responseFactory
);
- $this->assertTrue( $service->checkSpecialPage( $specialPageName, $output, $user ) );
+ $this->assertTrue( $service->checkSpecialPage( $specialPageName, $output, $user, $request ) );
}
/**
@@ -774,6 +804,8 @@ public function testCheckSpecialPageAllowsUnprotected() {
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
$responseFactory = $this->createMock( ResponseFactory::class );
$responseFactory->expects( $this->never() )->method( 'denyAccess' );
@@ -783,7 +815,7 @@ public function testCheckSpecialPageAllowsUnprotected() {
[],
$responseFactory
);
- $this->assertTrue( $service->checkSpecialPage( 'Search', $output, $user ) );
+ $this->assertTrue( $service->checkSpecialPage( 'Search', $output, $user, $request ) );
}
/**
@@ -794,6 +826,8 @@ public function testCheckSpecialPageAllowsProtectedPageOnCommandLine() {
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
$responseFactory = $this->createMock( ResponseFactory::class );
$responseFactory->expects( $this->never() )->method( 'denyAccess' );
@@ -806,7 +840,7 @@ public function testCheckSpecialPageAllowsProtectedPageOnCommandLine() {
[ 'target' ],
true
);
- $this->assertTrue( $service->checkSpecialPage( 'WhatLinksHere', $output, $user ) );
+ $this->assertTrue( $service->checkSpecialPage( 'WhatLinksHere', $output, $user, $request ) );
}
// ---------------------------------------------------------------
@@ -887,9 +921,9 @@ public function testCheckPerformActionAllowsAllowedIPs( $allowedIPs, string $ip
$output = $this->createMock( self::$outputPageClassName );
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
- $user->method( 'getName' )->willReturn( $ip );
$request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( $ip );
$request->method( 'getVal' )->willReturnMap( [
[ 'type', null, 'revision' ],
] );
@@ -912,9 +946,9 @@ public function testCheckPerformActionBlocksNotAllowedIPs( array $allowedIPs, st
$output = $this->createMock( self::$outputPageClassName );
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
- $user->method( 'getName' )->willReturn( $ip );
$request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( $ip );
$request->method( 'getVal' )->willReturnMap( [
[ 'type', null, 'revision' ],
] );
@@ -953,6 +987,250 @@ public function provideAllowedIPs(): array {
];
}
+ // ---------------------------------------------------------------
+ // IP read from WebRequest::getIP() tests
+ // ---------------------------------------------------------------
+
+ /**
+ * Verify that the IP used for allowlist matching comes from $request->getIP()
+ * and not from the (now irrelevant) username.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testCheckPerformActionUsesRequestIPNotUsername() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ // Username does NOT match the allowlist; request IP DOES.
+ // getName() must never be called for IP resolution.
+ $user->expects( $this->never() )->method( 'getName' );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( '1.2.3.4' );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'type', null, 'revision' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $service = $this->buildService( [], [ 'history' ], [ '1.2.3.4' ], $responseFactory );
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * Verify that the IP used for allowlist matching in checkSpecialPage comes
+ * from $request->getIP().
+ *
+ * @covers ::checkSpecialPage
+ */
+ public function testCheckSpecialPageUsesRequestIPNotUsername() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $user->expects( $this->never() )->method( 'getName' );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( '1.2.3.4' );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $service = $this->buildService(
+ [ 'WhatLinksHere' ], [], [ '1.2.3.4' ], $responseFactory
+ );
+ $this->assertTrue( $service->checkSpecialPage( 'WhatLinksHere', $output, $user, $request ) );
+ }
+
+ // ---------------------------------------------------------------
+ // Temporary-account user tests
+ // ---------------------------------------------------------------
+
+ /**
+ * With CrawlerProtectionTreatTempUsersAsAnon = false (default), a
+ * temporary-account user (isRegistered() = true, isTemp() = true) should
+ * be allowed through just like any other registered user.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testCheckPerformActionAllowsTempUserWhenFlagIsFalse() {
+ $output = $this->createMock( self::$outputPageClassName );
+
+ // Simulate a temporary-account user: registered but isTemp() = true.
+ $user = $this->newTempAwareUserMock( true );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'type', null, 'revision' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ // treatTempUsersAsAnon = false (default)
+ $service = $this->buildService( [], [ 'history' ], [], $responseFactory );
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * With CrawlerProtectionTreatTempUsersAsAnon = true, a user whose
+ * isRegistered() returns true but isTemp() returns true should be treated
+ * as anonymous and therefore blocked on a protected action.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testCheckPerformActionBlocksTempUserWhenFlagIsTrue() {
+ $output = $this->createMock( self::$outputPageClassName );
+
+ // Simulate a temporary-account user: registered but isTemp() = true.
+ $user = $this->newTempAwareUserMock( true );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'type', null, 'revision' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ // treatTempUsersAsAnon = true
+ $service = $this->buildService(
+ [], [ 'history' ], [], $responseFactory, true, [ 'target' ], false, [], [], true
+ );
+ $this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * With CrawlerProtectionTreatTempUsersAsAnon = true, a fully registered
+ * (non-temp) user should still be allowed through.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testCheckPerformActionAllowsRegisteredNonTempWhenFlagIsTrue() {
+ $output = $this->createMock( self::$outputPageClassName );
+
+ $user = $this->newTempAwareUserMock( false );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'type', null, 'revision' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ // treatTempUsersAsAnon = true, but user is not a temp account
+ $service = $this->buildService(
+ [], [ 'history' ], [], $responseFactory, true, [ 'target' ], false, [], [], true
+ );
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ // ---------------------------------------------------------------
+ // Scalar misconfiguration tests
+ // ---------------------------------------------------------------
+
+ /**
+ * When CrawlerProtectedActions is set to a scalar string instead of an
+ * array, the service should still function (no fatal) and treat the scalar
+ * as a single-element list.
+ *
+ * @covers ::isProtectedAction
+ */
+ public function testIsProtectedActionToleratesScalarConfig() {
+ $options = new ServiceOptions(
+ CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
+ [
+ 'CrawlerProtectedActions' => 'history',
+ 'CrawlerProtectedApiModules' => [],
+ 'CrawlerProtectedQueryParams' => [ 'target' ],
+ 'CrawlerProtectedRestPaths' => [],
+ 'CrawlerProtectedSpecialPages' => [],
+ 'CrawlerProtectionAllowedIPs' => [],
+ 'CrawlerProtectionProtectRevisions' => true,
+ 'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ ]
+ );
+ $service = new CrawlerProtectionService(
+ $options,
+ $this->createMock( ResponseFactory::class ),
+ false,
+ new NullLogger()
+ );
+
+ $this->assertTrue( $service->isProtectedAction( 'history' ) );
+ $this->assertFalse( $service->isProtectedAction( 'edit' ) );
+ }
+
+ /**
+ * When CrawlerProtectedQueryParams is set to a scalar string instead of an
+ * array, the service should still function and treat the scalar as a
+ * single-element list.
+ *
+ * @covers ::hasProtectedQueryParam
+ */
+ public function testHasProtectedQueryParamToleratesScalarConfig() {
+ $options = new ServiceOptions(
+ CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
+ [
+ 'CrawlerProtectedActions' => [],
+ 'CrawlerProtectedApiModules' => [],
+ 'CrawlerProtectedQueryParams' => 'target',
+ 'CrawlerProtectedRestPaths' => [],
+ 'CrawlerProtectedSpecialPages' => [],
+ 'CrawlerProtectionAllowedIPs' => [],
+ 'CrawlerProtectionProtectRevisions' => true,
+ 'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ ]
+ );
+ $service = new CrawlerProtectionService(
+ $options,
+ $this->createMock( ResponseFactory::class ),
+ false,
+ new NullLogger()
+ );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'title', null, null ],
+ [ 'target', null, 'Project:Foo' ],
+ ] );
+
+ $this->assertTrue( $service->hasProtectedQueryParam( $request ) );
+ }
+
+ /**
+ * When CrawlerProtectedSpecialPages is set to a scalar string instead of an
+ * array, the service should still function and treat the scalar as a
+ * single-element list.
+ *
+ * @covers ::isProtectedSpecialPage
+ */
+ public function testIsProtectedSpecialPageToleratesScalarConfig() {
+ $options = new ServiceOptions(
+ CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
+ [
+ 'CrawlerProtectedActions' => [],
+ 'CrawlerProtectedApiModules' => [],
+ 'CrawlerProtectedQueryParams' => [],
+ 'CrawlerProtectedRestPaths' => [],
+ 'CrawlerProtectedSpecialPages' => 'WhatLinksHere',
+ 'CrawlerProtectionAllowedIPs' => [],
+ 'CrawlerProtectionProtectRevisions' => true,
+ 'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ ]
+ );
+ $service = new CrawlerProtectionService(
+ $options,
+ $this->createMock( ResponseFactory::class ),
+ false,
+ new NullLogger()
+ );
+
+ $this->assertTrue( $service->isProtectedSpecialPage( 'WhatLinksHere' ) );
+ $this->assertFalse( $service->isProtectedSpecialPage( 'Search' ) );
+ }
+
// ---------------------------------------------------------------
// isProtectedApiModule tests
// ---------------------------------------------------------------
diff --git a/tests/phpunit/unit/HooksTest.php b/tests/phpunit/unit/HooksTest.php
index 612a030..7e636ac 100644
--- a/tests/phpunit/unit/HooksTest.php
+++ b/tests/phpunit/unit/HooksTest.php
@@ -118,8 +118,9 @@ public function testOnMediaWikiPerformActionPassesThroughTrue() {
public function testOnSpecialPageBeforeExecuteDelegatesToService() {
$output = $this->createMock( self::$outputPageClassName );
$user = $this->createMock( self::$userClassName );
+ $request = $this->createMock( self::$webRequestClassName );
- $context = $this->createMockContext( $user, $output );
+ $context = $this->createMockContext( $user, $output, $request );
$special = $this->createMock( self::$specialPageClassName );
$special->method( 'getName' )->willReturn( 'WhatLinksHere' );
@@ -128,7 +129,7 @@ public function testOnSpecialPageBeforeExecuteDelegatesToService() {
$service = $this->createMock( CrawlerProtectionService::class );
$service->expects( $this->once() )
->method( 'checkSpecialPage' )
- ->with( 'WhatLinksHere', $output, $user )
+ ->with( 'WhatLinksHere', $output, $user, $request )
->willReturn( false );
$hooks = new Hooks( $service );
@@ -143,8 +144,9 @@ public function testOnSpecialPageBeforeExecuteDelegatesToService() {
public function testOnSpecialPageBeforeExecutePassesThroughTrue() {
$output = $this->createMock( self::$outputPageClassName );
$user = $this->createMock( self::$userClassName );
+ $request = $this->createMock( self::$webRequestClassName );
- $context = $this->createMockContext( $user, $output );
+ $context = $this->createMockContext( $user, $output, $request );
$special = $this->createMock( self::$specialPageClassName );
$special->method( 'getName' )->willReturn( 'Search' );
@@ -153,6 +155,7 @@ public function testOnSpecialPageBeforeExecutePassesThroughTrue() {
$service = $this->createMock( CrawlerProtectionService::class );
$service->expects( $this->once() )
->method( 'checkSpecialPage' )
+ ->with( 'Search', $output, $user, $request )
->willReturn( true );
$hooks = new Hooks( $service );
@@ -166,22 +169,27 @@ public function testOnSpecialPageBeforeExecutePassesThroughTrue() {
*
* @param \PHPUnit\Framework\MockObject\MockObject $user Mock user object
* @param \PHPUnit\Framework\MockObject\MockObject $output Mock output object
+ * @param \PHPUnit\Framework\MockObject\MockObject $request Mock request object
* @return \stdClass Mock context
*/
- private function createMockContext( $user, $output ) {
- return new class( $user, $output ) {
+ private function createMockContext( $user, $output, $request ) {
+ return new class( $user, $output, $request ) {
/** @var \PHPUnit\Framework\MockObject\MockObject */
private $user;
/** @var \PHPUnit\Framework\MockObject\MockObject */
private $output;
+ /** @var \PHPUnit\Framework\MockObject\MockObject */
+ private $request;
/**
* @param \PHPUnit\Framework\MockObject\MockObject $user
* @param \PHPUnit\Framework\MockObject\MockObject $output
+ * @param \PHPUnit\Framework\MockObject\MockObject $request
*/
- public function __construct( $user, $output ) {
+ public function __construct( $user, $output, $request ) {
$this->user = $user;
$this->output = $output;
+ $this->request = $request;
}
/**
@@ -197,6 +205,13 @@ public function getUser() {
public function getOutput() {
return $this->output;
}
+
+ /**
+ * @return \PHPUnit\Framework\MockObject\MockObject
+ */
+ public function getRequest() {
+ return $this->request;
+ }
};
}
From 0ac023fc6f1e0852902ffa9704083673d90b6f64 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Wed, 29 Jul 2026 21:46:32 -0700
Subject: [PATCH 04/16] test: add integration tests and fix denyAccessPretty
branch testability (#58)
* Initial plan
* test: add integration tests and fix denyAccessPretty testability
- Replace version_compare(MW_VERSION,...) with method_exists() in
ResponseFactory::denyAccessPretty() so both title-setter branches
(setPageTitle for MW<1.41, setPageTitleMsg for MW>=1.41) are reachable
in tests without redefining the MW_VERSION constant.
- Add tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
(MediaWikiIntegrationTestCase) covering:
* Service-container wiring: both services resolve from the real container
* Hook registration: MediaWikiPerformAction and SpecialPageBeforeExecute
are registered
* Real OutputPage state: denyAccessPretty() sets HTTP 403 on a genuine
OutputPage (CI matrix runs REL1_39 + REL1_43, covering both
method_exists() branches)
* End-to-end behaviour: anonymous users are blocked on protected actions
and special pages; registered users are not
- Add unit tests for both branches of the new method_exists() logic in
ResponseFactoryTest: modern path (setPageTitleMsg called, setPageTitle
never) and legacy path (anonymous stub without setPageTitleMsg triggers
setPageTitle).
- composer.json: restrict standalone `composer phpunit` to unit/ only so
integration tests (which need a real MediaWiki install) are not run in
isolation; register integration test namespace in autoload-dev.
- ci.yml: convert phpunit matrix to explicit include entries and enable
pcov coverage on the REL1_43/PHP 8.2 job, passing --coverage-text to
phpunit when coverage != none.
Closes #50
* Operationalize Copilot guidance with repo-specific rules, correct scope, and agent setup workflow (#61)
* Initial plan
* Overhaul Copilot instructions with repo-specific CI architecture and testing guidance
* Clarify trimmed guidance and Copilot setup workflow intent
* Restore comprehensive MediaWiki instruction guidance
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* ci: pin actions to SHAs, extend matrix to MW 1.44/1.45, add Dependabot, enforce minus-x (#59)
* Initial plan
* ci: hygiene improvements - pin actions, extend matrix, add dependabot
* ci: skip minus-x on REL1_39 where MediaWiki does not ship it
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* Bump actions/checkout from 4.1.7 to 7.0.1 (#65)
Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.7 to 7.0.1.
- [Release notes](https://github.com/actions/checkout/releases)
- [Commits](https://github.com/actions/checkout/compare/v4.1.7...v7.0.1)
---
updated-dependencies:
- dependency-name: actions/checkout
dependency-version: 7.0.1
dependency-type: direct:production
update-type: version-update:semver-major
...
Signed-off-by: dependabot[bot]
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* test: make integration tests version-agnostic for Title/FauxRequest/User
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---------
Signed-off-by: dependabot[bot]
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jeffrey Wang
---
.github/workflows/ci.yml | 15 +-
composer.json | 3 +-
includes/ResponseFactory.php | 12 +-
.../CrawlerProtectionIntegrationTest.php | 356 ++++++++++++++++++
tests/phpunit/unit/ResponseFactoryTest.php | 67 ++++
5 files changed, 446 insertions(+), 7 deletions(-)
create mode 100644 tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 73a5565..962584a 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -163,19 +163,27 @@ jobs:
php: [ '8.2', '8.3', '8.4' ]
mediawiki: [ REL1_43, REL1_44, REL1_45 ]
include:
+ # REL1_43/PHP 8.2 also collects code coverage.
+ - os: ubuntu-latest
+ php: '8.2'
+ mediawiki: REL1_43
+ coverage: pcov
+ # REL1_39 jobs
- os: ubuntu-latest
php: '7.4'
mediawiki: REL1_39
+ coverage: none
- os: ubuntu-latest
php: '8.2'
mediawiki: REL1_39
+ coverage: none
steps:
- name: Setup PHP
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.37.2
with:
php-version: ${{ matrix.php }}
extensions: mbstring, intl, ast
- coverage: none
+ coverage: ${{ matrix.coverage || 'none' }}
tools: composer
- name: Setup MediaWiki
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
@@ -197,4 +205,7 @@ jobs:
- name: Install MediaWiki
run: php maintenance/install.php --dbtype=sqlite --with-extensions --pass=UnitTestingAdminPassword519 UnitTesting WikiAdmin
- name: Phpunit
- run: ./vendor/bin/phpunit -- extensions/${{ env.EXTNAME }}/tests/phpunit
+ run: |
+ FLAGS=""
+ [ "${{ matrix.coverage }}" = "pcov" ] && FLAGS="--coverage-text"
+ ./vendor/bin/phpunit $FLAGS -- extensions/${{ env.EXTNAME }}/tests/phpunit
diff --git a/composer.json b/composer.json
index ac30fb5..b404c79 100644
--- a/composer.json
+++ b/composer.json
@@ -16,7 +16,8 @@
},
"autoload-dev": {
"psr-4": {
- "MediaWiki\\Extension\\CrawlerProtection\\Tests\\": "tests/phpunit/unit/"
+ "MediaWiki\\Extension\\CrawlerProtection\\Tests\\": "tests/phpunit/unit/",
+ "MediaWiki\\Extension\\CrawlerProtection\\Tests\\Integration\\": "tests/phpunit/integration/"
},
"files": [
"tests/phpunit/stubs.php",
diff --git a/includes/ResponseFactory.php b/includes/ResponseFactory.php
index 9e9986f..8308f6d 100644
--- a/includes/ResponseFactory.php
+++ b/includes/ResponseFactory.php
@@ -136,11 +136,15 @@ protected function denyAccessPretty( $output ): void {
wfMessage( 'crawlerprotection-accessdenied-text' )->plain()
);
- if ( version_compare( MW_VERSION, '1.41', '<' ) ) {
- $output->setPageTitle( wfMessage( 'crawlerprotection-accessdenied-title' ) );
- } else {
+ $msg = wfMessage( 'crawlerprotection-accessdenied-title' );
+ // setPageTitleMsg() was added in MediaWiki 1.41; fall back to
+ // setPageTitle() for earlier versions. Using method_exists() keeps
+ // both branches reachable in tests regardless of MW_VERSION.
+ if ( method_exists( $output, 'setPageTitleMsg' ) ) {
// @phan-suppress-next-line PhanUndeclaredMethod Exists in 1.41+
- $output->setPageTitleMsg( wfMessage( 'crawlerprotection-accessdenied-title' ) );
+ $output->setPageTitleMsg( $msg );
+ } else {
+ $output->setPageTitle( $msg );
}
}
}
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
new file mode 100644
index 0000000..07b2e5e
--- /dev/null
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -0,0 +1,356 @@
+= 1.41) branches through method_exists().
+ * - End-to-end service behaviour: confirms that checkPerformAction() and
+ * checkSpecialPage() block anonymous users as configured.
+ *
+ * @group CrawlerProtection
+ * @group Database
+ * @coversDefaultClass \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService
+ */
+class CrawlerProtectionIntegrationTest extends MediaWikiIntegrationTestCase {
+
+ // ---------------------------------------------------------------
+ // Helpers
+ // ---------------------------------------------------------------
+
+ /**
+ * Override every CrawlerProtection config key so that tests are not
+ * sensitive to wiki-local defaults.
+ *
+ * @param array $overrides Values that differ from the all-defaults baseline.
+ */
+ private function overrideCrawlerProtectionConfig( array $overrides = [] ): void {
+ $defaults = [
+ 'CrawlerProtectedActions' => [ 'history' ],
+ 'CrawlerProtectedSpecialPages' => [ 'whatlinkshere', 'recentchangeslinked' ],
+ 'CrawlerProtectedQueryParams' => [ 'target' ],
+ 'CrawlerProtectionAllowedIPs' => [],
+ 'CrawlerProtectionProtectRevisions' => true,
+ 'CrawlerProtectionRawDenial' => false,
+ 'CrawlerProtectionUse418' => false,
+ 'CrawlerProtectionRawDenialHeader' => 'HTTP/1.0 403 Forbidden',
+ 'CrawlerProtectionRawDenialText' => '403 Forbidden',
+ ];
+
+ $this->overrideConfigValues( array_merge( $defaults, $overrides ) );
+ }
+
+ /**
+ * Return a fresh OutputPage attached to a minimal RequestContext.
+ *
+ * A Title is required so that OutputPage::addWikiTextAsInterface() can
+ * invoke the parser without an "Invalid title" error. TitleFactory is
+ * used instead of the Title class so that the test works both on releases
+ * that still provide the unnamespaced Title alias and on those that do not.
+ *
+ * @return \MediaWiki\Output\OutputPage|\OutputPage
+ */
+ private function makeOutputPage() {
+ $context = new \RequestContext();
+ $context->setTitle(
+ $this->getServiceContainer()->getTitleFactory()->makeTitle( NS_MAIN, 'Test' )
+ );
+ return $context->getOutput();
+ }
+
+ /**
+ * Build a FauxRequest carrying the given query parameters.
+ *
+ * FauxRequest lives in the MediaWiki\Request namespace on newer releases
+ * and in the global namespace on older ones, so the class is resolved at
+ * run time rather than referenced directly.
+ *
+ * @param array $params Query parameters
+ * @return \MediaWiki\Request\FauxRequest|\FauxRequest
+ */
+ private function makeRequest( array $params ) {
+ $class = class_exists( \MediaWiki\Request\FauxRequest::class )
+ ? \MediaWiki\Request\FauxRequest::class
+ : 'FauxRequest';
+ return new $class( $params );
+ }
+
+ /**
+ * Return a real anonymous user object for the given IP address.
+ *
+ * A real user is used rather than a mock because the User class has moved
+ * between namespaces across supported releases, and mocking it by name
+ * fails on releases where the chosen name is only an alias.
+ *
+ * @param string $ip
+ * @return \MediaWiki\User\User|\User
+ */
+ private function makeAnonUser( string $ip = '1.2.3.4' ) {
+ return $this->getServiceContainer()->getUserFactory()->newAnonymous( $ip );
+ }
+
+ /**
+ * Build a CrawlerProtectionService wired against the real container but
+ * with cliMode forced to false.
+ *
+ * PHPUnit runs PHP as a CLI process, so ServiceWiring.php detects
+ * MW_ENTRY_POINT === 'cli' and constructs the container service with
+ * cliMode = true, which bypasses all protection. Tests that exercise
+ * the blocking path must use this helper instead of pulling the service
+ * directly from the container.
+ *
+ * @return CrawlerProtectionService
+ */
+ private function makeWebModeService(): CrawlerProtectionService {
+ return new CrawlerProtectionService(
+ new ServiceOptions(
+ CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
+ $this->getServiceContainer()->getMainConfig()
+ ),
+ $this->getServiceContainer()->get( 'CrawlerProtection.ResponseFactory' ),
+ // false = web-request mode — not CLI
+ false
+ );
+ }
+
+ // ---------------------------------------------------------------
+ // Service-container wiring
+ // ---------------------------------------------------------------
+
+ /**
+ * Verify that CrawlerProtection.CrawlerProtectionService resolves from
+ * the real service container and is of the correct type.
+ *
+ * This catches typos in extension.json service names and constructor
+ * mis-wiring in ServiceWiring.php.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::__construct
+ */
+ public function testCrawlerProtectionServiceResolvesFromContainer(): void {
+ $service = $this->getServiceContainer()->get( 'CrawlerProtection.CrawlerProtectionService' );
+ $this->assertInstanceOf( CrawlerProtectionService::class, $service );
+ }
+
+ /**
+ * Verify that CrawlerProtection.ResponseFactory resolves from the real
+ * service container and is of the correct type.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\ResponseFactory::__construct
+ */
+ public function testResponseFactoryResolvesFromContainer(): void {
+ $factory = $this->getServiceContainer()->get( 'CrawlerProtection.ResponseFactory' );
+ $this->assertInstanceOf( ResponseFactory::class, $factory );
+ }
+
+ // ---------------------------------------------------------------
+ // Hook registration
+ // ---------------------------------------------------------------
+
+ /**
+ * Verify that the MediaWikiPerformAction hook handler is registered.
+ *
+ * This catches mistakes in the "Hooks" section of extension.json.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::__construct
+ */
+ public function testMediaWikiPerformActionHookIsRegistered(): void {
+ $this->assertTrue(
+ $this->getServiceContainer()->getHookContainer()->isRegistered( 'MediaWikiPerformAction' ),
+ 'MediaWikiPerformAction hook must be registered by the extension'
+ );
+ }
+
+ /**
+ * Verify that the SpecialPageBeforeExecute hook handler is registered.
+ *
+ * This catches mistakes in the "Hooks" section of extension.json.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::__construct
+ */
+ public function testSpecialPageBeforeExecuteHookIsRegistered(): void {
+ $this->assertTrue(
+ $this->getServiceContainer()->getHookContainer()->isRegistered( 'SpecialPageBeforeExecute' ),
+ 'SpecialPageBeforeExecute hook must be registered by the extension'
+ );
+ }
+
+ // ---------------------------------------------------------------
+ // ResponseFactory::denyAccessPretty() with real OutputPage
+ // ---------------------------------------------------------------
+
+ /**
+ * Confirm that the "pretty" denial path sets HTTP 403 on a real OutputPage.
+ *
+ * This exercises the method_exists()-based branch in denyAccessPretty():
+ * on MW < 1.41 the method uses setPageTitle(); on MW >= 1.41 it uses
+ * setPageTitleMsg(). Both paths must end with a 403 status code, and the
+ * CI matrix (REL1_39 and REL1_43+) naturally covers both branches.
+ *
+ * OutputPage::getStatusCode() was added in MW 1.45. On earlier versions
+ * the test verifies the call does not throw; the status-code assertion is
+ * skipped when the getter is absent.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\ResponseFactory::denyAccessPretty
+ * @covers \MediaWiki\Extension\CrawlerProtection\ResponseFactory::denyAccess
+ */
+ public function testDenyAccessPrettySetsStatusCode403OnRealOutputPage(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectionRawDenial' => false,
+ 'CrawlerProtectionUse418' => false,
+ ] );
+
+ /** @var ResponseFactory $factory */
+ $factory = $this->getServiceContainer()->get( 'CrawlerProtection.ResponseFactory' );
+ $output = $this->makeOutputPage();
+
+ $factory->denyAccess( $output );
+
+ if ( method_exists( $output, 'getStatusCode' ) ) {
+ $this->assertSame( 403, $output->getStatusCode() );
+ } else {
+ // On MW < 1.45, getStatusCode() does not exist; verify only that
+ // denyAccess() completed without throwing.
+ $this->addToAssertionCount( 1 );
+ }
+ }
+
+ // ---------------------------------------------------------------
+ // CrawlerProtectionService::checkPerformAction()
+ // ---------------------------------------------------------------
+
+ /**
+ * An anonymous user requesting a protected action must be blocked (return
+ * false) and the OutputPage must receive HTTP 403.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkPerformAction
+ */
+ public function testAnonymousUserWithProtectedActionIsBlocked(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedActions' => [ 'history' ],
+ 'CrawlerProtectionProtectRevisions' => false,
+ ] );
+
+ $service = $this->makeWebModeService();
+
+ $user = $this->makeAnonUser();
+
+ $request = $this->makeRequest( [ 'action' => 'history' ] );
+ $output = $this->makeOutputPage();
+
+ $result = $service->checkPerformAction( $output, $user, $request );
+
+ $this->assertFalse( $result, 'checkPerformAction must return false to abort the request' );
+ if ( method_exists( $output, 'getStatusCode' ) ) {
+ $this->assertSame( 403, $output->getStatusCode() );
+ }
+ }
+
+ /**
+ * A registered (logged-in) user requesting a protected action must not
+ * be blocked — the service must return true.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkPerformAction
+ */
+ public function testRegisteredUserWithProtectedActionIsNotBlocked(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedActions' => [ 'history' ],
+ ] );
+
+ $service = $this->makeWebModeService();
+
+ // getMutableTestUser() returns a real registered user object.
+ $user = $this->getMutableTestUser()->getUser();
+ $request = $this->makeRequest( [ 'action' => 'history' ] );
+ $output = $this->makeOutputPage();
+
+ $result = $service->checkPerformAction( $output, $user, $request );
+
+ $this->assertTrue( $result, 'checkPerformAction must return true for registered users' );
+ }
+
+ // ---------------------------------------------------------------
+ // CrawlerProtectionService::checkSpecialPage()
+ // ---------------------------------------------------------------
+
+ /**
+ * An anonymous user visiting a protected special page must be blocked.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkSpecialPage
+ */
+ public function testAnonymousUserOnProtectedSpecialPageIsBlocked(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedSpecialPages' => [ 'WhatLinksHere' ],
+ ] );
+
+ $service = $this->makeWebModeService();
+
+ $user = $this->makeAnonUser();
+
+ $output = $this->makeOutputPage();
+
+ $result = $service->checkSpecialPage( 'WhatLinksHere', $output, $user );
+
+ $this->assertFalse( $result, 'checkSpecialPage must return false to abort the request' );
+ if ( method_exists( $output, 'getStatusCode' ) ) {
+ $this->assertSame( 403, $output->getStatusCode() );
+ }
+ }
+
+ /**
+ * A registered user visiting a protected special page must not be blocked.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkSpecialPage
+ */
+ public function testRegisteredUserOnProtectedSpecialPageIsNotBlocked(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedSpecialPages' => [ 'WhatLinksHere' ],
+ ] );
+
+ $service = $this->makeWebModeService();
+
+ $user = $this->getMutableTestUser()->getUser();
+ $output = $this->makeOutputPage();
+
+ $result = $service->checkSpecialPage( 'WhatLinksHere', $output, $user );
+
+ $this->assertTrue( $result, 'checkSpecialPage must return true for registered users' );
+ }
+}
diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php
index 14c810f..af6569f 100644
--- a/tests/phpunit/unit/ResponseFactoryTest.php
+++ b/tests/phpunit/unit/ResponseFactoryTest.php
@@ -267,4 +267,71 @@ public function testConstructorAcceptsValidOptions() {
$factory = $this->buildFactory();
$this->assertInstanceOf( ResponseFactory::class, $factory );
}
+
+ /**
+ * When OutputPage exposes setPageTitleMsg() (MW 1.41+), denyAccessPretty()
+ * must call that method rather than the legacy setPageTitle().
+ *
+ * @covers ::denyAccess
+ * @covers ::denyAccessPretty
+ */
+ public function testDenyAccessPrettyCallsSetPageTitleMsgOnModernOutputPage() {
+ if ( defined( 'MEDIAWIKI' ) ) {
+ $this->markTestSkipped(
+ 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
+ );
+ }
+
+ $output = $this->createMock( self::$outputPageClassName );
+ $output->expects( $this->once() )->method( 'setPageTitleMsg' );
+ $output->expects( $this->never() )->method( 'setPageTitle' );
+
+ $this->buildFactory()->denyAccess( $output );
+ }
+
+ /**
+ * When OutputPage lacks setPageTitleMsg() (MW < 1.41), denyAccessPretty()
+ * must fall back to the legacy setPageTitle() method.
+ *
+ * Uses an anonymous stub that deliberately omits setPageTitleMsg() so that
+ * method_exists() returns false, exercising the backwards-compat branch.
+ *
+ * @covers ::denyAccess
+ * @covers ::denyAccessPretty
+ */
+ public function testDenyAccessPrettyFallsBackToSetPageTitleOnLegacyOutputPage() {
+ if ( defined( 'MEDIAWIKI' ) ) {
+ $this->markTestSkipped(
+ 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
+ );
+ }
+
+ $setPageTitleCallCount = 0;
+
+ // Anonymous class without setPageTitleMsg() simulates MW < 1.41 OutputPage.
+ $output = new class ( $setPageTitleCallCount ) {
+ /** @var int */
+ private int $count;
+
+ public function __construct( int &$count ) {
+ $this->count = &$count;
+ }
+
+ public function setStatusCode( int $code ): void {
+ }
+
+ public function addWikiTextAsInterface( string $text ): void {
+ }
+
+ public function setPageTitle( $title ): void {
+ $this->count++;
+ }
+
+ // Intentionally no setPageTitleMsg() to trigger the legacy branch.
+ };
+
+ $this->buildFactory()->denyAccess( $output );
+
+ $this->assertSame( 1, $setPageTitleCallCount );
+ }
}
From 37fc858e4dc38426958431a574ad1195ec257be7 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Wed, 29 Jul 2026 22:12:37 -0700
Subject: [PATCH 05/16] Add CrawlerProtectionShouldDeny hook for bespoke access
policy (#62)
* Initial plan
* Add CrawlerProtectionShouldDeny hook
* Merge candidate/1.7.0 and fix unit tests to not construct HookContainer
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* tests: assert the hook still runs for allowlisted IPs
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Jeffrey Wang
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
README.md | 40 +++
includes/CrawlerProtectionService.php | 96 ++++---
.../Hook/CrawlerProtectionShouldDenyHook.php | 70 +++++
includes/HookRunner.php | 62 +++++
includes/ServiceWiring.php | 2 +
.../CrawlerProtectionIntegrationTest.php | 14 +-
.../unit/CrawlerProtectionServiceTest.php | 257 +++++++++++++++++-
7 files changed, 499 insertions(+), 42 deletions(-)
create mode 100644 includes/Hook/CrawlerProtectionShouldDenyHook.php
create mode 100644 includes/HookRunner.php
diff --git a/README.md b/README.md
index 12ff83b..70771f1 100644
--- a/README.md
+++ b/README.md
@@ -91,3 +91,43 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
users and bypass all protection checks. Set to `true` if you do not want
crawlers that receive a temporary account to bypass protection.
+# Hooks
+
+## CrawlerProtectionShouldDeny
+
+Runs after CrawlerProtection has decided whether to deny a request, but before
+the denial is carried out. Handlers can implement bespoke policy (cookie
+checks, proof-of-work, CAPTCHA integration, fingerprint heuristics, crawler
+allowlists, ...) without patching this extension.
+
+Parameters:
+
+* `User $user` - the user making the request.
+* `WebRequest $request` - the current request.
+* `string|null $specialPageName` - canonical name of the special page being
+ executed, or `null` if the request is not a special page view.
+* `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`.
+
+Example, allowing anonymous access when a request carries a secret header:
+
+```php
+$wgHooks['CrawlerProtectionShouldDeny'][] = static function (
+ $user, $request, $specialPageName, &$shouldDeny
+) {
+ if ( $shouldDeny && $request->getHeader( 'X-My-Crawler-Token' ) === $secret ) {
+ $shouldDeny = false;
+ }
+};
+```
+
diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php
index 6cbcdfa..79d521e 100644
--- a/includes/CrawlerProtectionService.php
+++ b/includes/CrawlerProtectionService.php
@@ -26,6 +26,7 @@
namespace MediaWiki\Extension\CrawlerProtection;
use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Extension\CrawlerProtection\Hook\CrawlerProtectionShouldDenyHook;
use MediaWiki\Output\OutputPage;
use MediaWiki\Request\WebRequest;
use MediaWiki\User\User;
@@ -62,6 +63,9 @@ class CrawlerProtectionService {
/** @var ResponseFactory */
private ResponseFactory $responseFactory;
+ /** @var CrawlerProtectionShouldDenyHook */
+ private CrawlerProtectionShouldDenyHook $hookRunner;
+
/** @var bool */
private bool $cliMode;
@@ -89,18 +93,21 @@ class CrawlerProtectionService {
/**
* @param ServiceOptions $options
* @param ResponseFactory $responseFactory
+ * @param CrawlerProtectionShouldDenyHook $hookRunner
* @param bool $cliMode
* @param LoggerInterface $logger
*/
public function __construct(
ServiceOptions $options,
ResponseFactory $responseFactory,
+ CrawlerProtectionShouldDenyHook $hookRunner,
bool $cliMode,
LoggerInterface $logger
) {
$options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
$this->options = $options;
$this->responseFactory = $responseFactory;
+ $this->hookRunner = $hookRunner;
$this->cliMode = $cliMode;
$this->logger = $logger;
@@ -137,37 +144,38 @@ public function checkPerformAction(
return true;
}
- if ( $this->isUserAllowed( $user ) ) {
- return true;
+ $shouldDeny = false;
+
+ if ( !$this->isUserAllowed( $user ) && !$this->isRequestIPAllowed( $request ) ) {
+ $type = $request->getVal( 'type' );
+ $action = $request->getVal( 'action' );
+ $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' );
+
+ $shouldDeny = $this->isProtectedAction( $action )
+ || $this->hasProtectedQueryParam( $request )
+ || ( $revisionsProtected && (
+ $type === 'revision'
+ || $diffId > 0
+ || $oldId > 0
+ ) );
}
- // Use the canonical client IP from WebRequest (correctly applies
- // trusted-proxy / X-Forwarded-For handling) rather than the username.
- if ( $this->normalizedAllowedIPs !== [] && $this->isIPAllowed( $request->getIP() ) ) {
- return true;
- }
+ $this->hookRunner->onCrawlerProtectionShouldDeny(
+ $user,
+ $request,
+ null,
+ $shouldDeny
+ );
- $type = $request->getVal( 'type' );
- $action = $request->getVal( 'action' );
- $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 (
- $this->isProtectedAction( $action )
- || $this->hasProtectedQueryParam( $request )
- || ( $revisionsProtected && (
- $type === 'revision'
- || $diffId > 0
- || $oldId > 0
- ) )
- ) {
+ if ( $shouldDeny ) {
$this->responseFactory->denyAccess( $output );
return false;
}
@@ -252,17 +260,18 @@ public function checkSpecialPage(
return true;
}
- if ( $this->isUserAllowed( $user ) ) {
- return true;
- }
+ $shouldDeny = !$this->isUserAllowed( $user )
+ && !$this->isRequestIPAllowed( $request )
+ && $this->isProtectedSpecialPage( $specialPageName );
- // Use the canonical client IP from WebRequest (correctly applies
- // trusted-proxy / X-Forwarded-For handling) rather than the username.
- if ( $this->normalizedAllowedIPs !== [] && $this->isIPAllowed( $request->getIP() ) ) {
- return true;
- }
+ $this->hookRunner->onCrawlerProtectionShouldDeny(
+ $user,
+ $request,
+ $specialPageName,
+ $shouldDeny
+ );
- if ( $this->isProtectedSpecialPage( $specialPageName ) ) {
+ if ( $shouldDeny ) {
$this->responseFactory->denyAccess( $output );
return false;
}
@@ -412,6 +421,19 @@ static function ( string $p ): string {
return in_array( $name, $normalizedProtectedPages, true );
}
+ /**
+ * Checks whether the request originates from an allowed IP range.
+ *
+ * The canonical client IP is taken from WebRequest (which correctly applies
+ * trusted-proxy / X-Forwarded-For handling) rather than the username.
+ *
+ * @param WebRequest $request
+ * @return bool
+ */
+ private function isRequestIPAllowed( $request ): bool {
+ return $this->normalizedAllowedIPs !== [] && $this->isIPAllowed( $request->getIP() );
+ }
+
/**
* Checks whether the given IP is in an allowed IP range.
*
diff --git a/includes/Hook/CrawlerProtectionShouldDenyHook.php b/includes/Hook/CrawlerProtectionShouldDenyHook.php
new file mode 100644
index 0000000..272c92c
--- /dev/null
+++ b/includes/Hook/CrawlerProtectionShouldDenyHook.php
@@ -0,0 +1,70 @@
+hookContainer = $hookContainer;
+ }
+
+ /**
+ * @inheritDoc
+ */
+ public function onCrawlerProtectionShouldDeny(
+ $user,
+ $request,
+ ?string $specialPageName,
+ bool &$shouldDeny
+ ) {
+ return $this->hookContainer->run(
+ 'CrawlerProtectionShouldDeny',
+ [ $user, $request, $specialPageName, &$shouldDeny ]
+ );
+ }
+}
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 58acf32..c813829 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -25,6 +25,7 @@
use MediaWiki\Config\ServiceOptions;
use MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService;
+use MediaWiki\Extension\CrawlerProtection\HookRunner;
use MediaWiki\Extension\CrawlerProtection\ResponseFactory;
use MediaWiki\Logger\LoggerFactory;
use MediaWiki\MediaWikiServices;
@@ -47,6 +48,7 @@ static function ( MediaWikiServices $services ): CrawlerProtectionService {
$services->getMainConfig()
),
$services->get( 'CrawlerProtection.ResponseFactory' ),
+ new HookRunner( $services->getHookContainer() ),
defined( 'MW_ENTRY_POINT' ) && MW_ENTRY_POINT === 'cli',
LoggerFactory::getInstance( 'CrawlerProtection' )
);
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
index 07b2e5e..c54c9cd 100644
--- a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -27,7 +27,9 @@
use MediaWiki\Config\ServiceOptions;
use MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService;
+use MediaWiki\Extension\CrawlerProtection\HookRunner;
use MediaWiki\Extension\CrawlerProtection\ResponseFactory;
+use MediaWiki\Logger\LoggerFactory;
use MediaWikiIntegrationTestCase;
/**
@@ -146,8 +148,10 @@ private function makeWebModeService(): CrawlerProtectionService {
$this->getServiceContainer()->getMainConfig()
),
$this->getServiceContainer()->get( 'CrawlerProtection.ResponseFactory' ),
+ new HookRunner( $this->getServiceContainer()->getHookContainer() ),
// false = web-request mode — not CLI
- false
+ false,
+ LoggerFactory::getInstance( 'CrawlerProtection' )
);
}
@@ -326,7 +330,9 @@ public function testAnonymousUserOnProtectedSpecialPageIsBlocked(): void {
$output = $this->makeOutputPage();
- $result = $service->checkSpecialPage( 'WhatLinksHere', $output, $user );
+ $request = $this->makeRequest( [] );
+
+ $result = $service->checkSpecialPage( 'WhatLinksHere', $output, $user, $request );
$this->assertFalse( $result, 'checkSpecialPage must return false to abort the request' );
if ( method_exists( $output, 'getStatusCode' ) ) {
@@ -349,7 +355,9 @@ public function testRegisteredUserOnProtectedSpecialPageIsNotBlocked(): void {
$user = $this->getMutableTestUser()->getUser();
$output = $this->makeOutputPage();
- $result = $service->checkSpecialPage( 'WhatLinksHere', $output, $user );
+ $request = $this->makeRequest( [] );
+
+ $result = $service->checkSpecialPage( 'WhatLinksHere', $output, $user, $request );
$this->assertTrue( $result, 'checkSpecialPage must return true for registered users' );
}
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index e9002a7..a4cfe79 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -4,6 +4,7 @@
use MediaWiki\Config\ServiceOptions;
use MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService;
+use MediaWiki\Extension\CrawlerProtection\Hook\CrawlerProtectionShouldDenyHook;
use MediaWiki\Extension\CrawlerProtection\ResponseFactory;
use PHPUnit\Framework\TestCase;
use Psr\Log\NullLogger;
@@ -51,6 +52,7 @@ public static function setUpBeforeClass(): void {
* @param array $protectedApiModules
* @param array $protectedRestPaths
* @param bool $treatTempUsersAsAnon
+ * @param callable[] $shouldDenyHandlers Handlers for CrawlerProtectionShouldDeny
* @return CrawlerProtectionService
*/
private function buildService(
@@ -63,7 +65,8 @@ private function buildService(
bool $cliMode = false,
array $protectedApiModules = [],
array $protectedRestPaths = [],
- bool $treatTempUsersAsAnon = false
+ bool $treatTempUsersAsAnon = false,
+ array $shouldDenyHandlers = []
): CrawlerProtectionService {
$options = new ServiceOptions(
CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
@@ -81,7 +84,15 @@ private function buildService(
$responseFactory ??= $this->createMock( ResponseFactory::class );
- return new CrawlerProtectionService( $options, $responseFactory, $cliMode, new NullLogger() );
+ $hookRunner = new HookRunnerFake( $shouldDenyHandlers );
+
+ return new CrawlerProtectionService(
+ $options,
+ $responseFactory,
+ $hookRunner,
+ $cliMode,
+ new NullLogger()
+ );
}
/**
@@ -1154,6 +1165,7 @@ public function testIsProtectedActionToleratesScalarConfig() {
$service = new CrawlerProtectionService(
$options,
$this->createMock( ResponseFactory::class ),
+ new HookRunnerFake(),
false,
new NullLogger()
);
@@ -1186,6 +1198,7 @@ public function testHasProtectedQueryParamToleratesScalarConfig() {
$service = new CrawlerProtectionService(
$options,
$this->createMock( ResponseFactory::class ),
+ new HookRunnerFake(),
false,
new NullLogger()
);
@@ -1223,6 +1236,7 @@ public function testIsProtectedSpecialPageToleratesScalarConfig() {
$service = new CrawlerProtectionService(
$options,
$this->createMock( ResponseFactory::class ),
+ new HookRunnerFake(),
false,
new NullLogger()
);
@@ -1551,4 +1565,243 @@ public function testCheckRestPathAllowsAllowedIP() {
);
$this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user ) );
}
+
+ // ---------------------------------------------------------------
+ // CrawlerProtectionShouldDeny hook tests
+ // ---------------------------------------------------------------
+
+ /**
+ * @covers ::checkPerformAction
+ */
+ public function testHookCanAllowOtherwiseDeniedPerformAction() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'action', null, 'history' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $seen = [];
+ $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $specialPageName, $shouldDeny ];
+ $shouldDeny = false;
+ };
+
+ $service = $this->buildService(
+ [], [ 'history' ], [], $responseFactory, true, [ 'target' ], false, [], [], false, [ $handler ]
+ );
+
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ $this->assertSame( [ $user, $request, null, true ], $seen );
+ }
+
+ /**
+ * @covers ::checkPerformAction
+ */
+ public function testHookCanDenyOtherwiseAllowedPerformAction() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( true );
+
+ $request = $this->createMock( self::$webRequestClassName );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) {
+ $shouldDeny = true;
+ };
+
+ $service = $this->buildService(
+ [], [ 'history' ], [], $responseFactory, true, [ 'target' ], false, [], [], false, [ $handler ]
+ );
+
+ $this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * The allowlisted-IP short circuit must not skip the hook, so handlers can
+ * still deny a request coming from an allowed IP.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testHookRunsForAllowlistedIP() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( '1.2.3.4' );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'action', null, 'history' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ $seen = [];
+ $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $specialPageName, $shouldDeny ];
+ $shouldDeny = true;
+ };
+
+ $service = $this->buildService(
+ [], [ 'history' ], [ '1.2.3.4' ], $responseFactory, true, [ 'target' ], false, [], [], false,
+ [ $handler ]
+ );
+
+ $this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+ $this->assertSame( [ $user, $request, null, false ], $seen );
+ }
+
+ /**
+ * @covers ::checkPerformAction
+ */
+ public function testHookIsNotRunInCliMode() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $request = $this->createMock( self::$webRequestClassName );
+
+ $called = false;
+ $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$called ) {
+ $called = true;
+ $shouldDeny = true;
+ };
+
+ $service = $this->buildService(
+ [], [ 'history' ], [], null, true, [ 'target' ], true, [], [], false, [ $handler ]
+ );
+
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ $this->assertFalse( $called );
+ }
+
+ /**
+ * @covers ::checkPerformAction
+ */
+ public function testHookAbortReturnValueKeepsDecision() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'action', null, 'history' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $secondCalled = false;
+ $first = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) {
+ $shouldDeny = false;
+ return false;
+ };
+ $second = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$secondCalled ) {
+ $secondCalled = true;
+ $shouldDeny = true;
+ };
+
+ $service = $this->buildService(
+ [], [ 'history' ], [], $responseFactory, true, [ 'target' ], false, [], [], false, [ $first, $second ]
+ );
+
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ $this->assertFalse( $secondCalled );
+ }
+
+ /**
+ * @covers ::checkSpecialPage
+ */
+ public function testHookCanAllowOtherwiseDeniedSpecialPage() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $seen = [];
+ $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $specialPageName, $shouldDeny ];
+ $shouldDeny = false;
+ };
+
+ $service = $this->buildService(
+ [ 'whatlinkshere' ], [], [], $responseFactory, true, [ 'target' ], false, [], [], false, [ $handler ]
+ );
+
+ $this->assertTrue(
+ $service->checkSpecialPage( 'WhatLinksHere', $output, $user, $request )
+ );
+ $this->assertSame( [ $user, $request, 'WhatLinksHere', true ], $seen );
+ }
+
+ /**
+ * @covers ::checkSpecialPage
+ */
+ public function testHookCanDenyOtherwiseAllowedSpecialPage() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) {
+ $shouldDeny = true;
+ };
+
+ $service = $this->buildService(
+ [ 'whatlinkshere' ], [], [], $responseFactory, true, [ 'target' ], false, [], [], false, [ $handler ]
+ );
+
+ $this->assertFalse( $service->checkSpecialPage( 'Search', $output, $user, $request ) );
+ }
+}
+
+// phpcs:disable Generic.Files.OneObjectStructurePerFile.MultipleFound
+// phpcs:disable MediaWiki.Files.ClassMatchesFilename.NotMatch
+/**
+ * Test double for HookRunner that dispatches to plain closures.
+ *
+ * The real HookRunner needs a HookContainer, which cannot be constructed
+ * standalone, so tests use this fake to exercise the hook without one.
+ */
+class HookRunnerFake implements CrawlerProtectionShouldDenyHook {
+
+ /** @var callable[] */
+ private array $handlers;
+
+ /**
+ * @param callable[] $handlers
+ */
+ public function __construct( array $handlers = [] ) {
+ $this->handlers = $handlers;
+ }
+
+ /**
+ * @inheritDoc
+ */
+ public function onCrawlerProtectionShouldDeny(
+ $user,
+ $request,
+ ?string $specialPageName,
+ bool &$shouldDeny
+ ) {
+ foreach ( $this->handlers as $handler ) {
+ if ( $handler( $user, $request, $specialPageName, $shouldDeny ) === false ) {
+ return false;
+ }
+ }
+
+ return true;
+ }
}
From 6f5647daa729d4c4b345b114525532a7d9084d8e Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Wed, 29 Jul 2026 22:33:03 -0700
Subject: [PATCH 06/16] Mark denial responses as noindex,nofollow (#63)
* Initial plan
* Send X-Robots-Tag on denials and robot policy on pretty denial page
* Use WebResponse::header() for X-Robots-Tag on the pretty denial path
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* Scope X-Robots-Tag to the pretty denial path only
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* Resolve merge conflicts with candidate/1.7.0 and bump to 1.7.1
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
* Downgrade version from 1.7.1 to 1.7.0
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Jeffrey Wang
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
README.md | 4 ++
includes/ResponseFactory.php | 5 ++
tests/phpunit/namespaced-stubs.php | 27 ++++++++
tests/phpunit/unit/ResponseFactoryTest.php | 72 +++++++++++++++++++++-
4 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/README.md b/README.md
index 70771f1..6fb1d12 100644
--- a/README.md
+++ b/README.md
@@ -91,6 +91,10 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
users and bypass all protection checks. Set to `true` if you do not want
crawlers that receive a temporary account to bypass protection.
+The pretty denial page carries an `X-Robots-Tag: noindex,nofollow` header and
+the same robot policy as a `` tag, so that well-behaved crawlers stop
+re-requesting denied URLs.
+
# Hooks
## CrawlerProtectionShouldDeny
diff --git a/includes/ResponseFactory.php b/includes/ResponseFactory.php
index 8308f6d..94453d8 100644
--- a/includes/ResponseFactory.php
+++ b/includes/ResponseFactory.php
@@ -40,6 +40,9 @@ class ResponseFactory {
private const TEAPOT_HEADER = 'HTTP/1.0 418 I\'m a teapot';
+ /** Robot directive sent on the pretty denial page */
+ private const ROBOT_POLICY = 'noindex,nofollow';
+
/** @var string[] List of constructor options this class accepts */
public const CONSTRUCTOR_OPTIONS = [
'CrawlerProtectionUse418',
@@ -132,6 +135,8 @@ protected function denyAccessRaw( string $header, string $message ): void {
*/
protected function denyAccessPretty( $output ): void {
$output->setStatusCode( 403 );
+ $output->setRobotPolicy( self::ROBOT_POLICY );
+ $output->getRequest()->response()->header( 'X-Robots-Tag: ' . self::ROBOT_POLICY );
$output->addWikiTextAsInterface(
wfMessage( 'crawlerprotection-accessdenied-text' )->plain()
);
diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php
index e556cf7..b910bc6 100644
--- a/tests/phpunit/namespaced-stubs.php
+++ b/tests/phpunit/namespaced-stubs.php
@@ -135,6 +135,16 @@ public function setPageTitle( $title ) {
public function setPageTitleMsg( $msg ) {
}
+
+ public function setRobotPolicy( $policy ) {
+ }
+
+ /**
+ * @return \MediaWiki\Request\WebRequest
+ */
+ public function getRequest() {
+ return new \MediaWiki\Request\WebRequest();
+ }
}
}
@@ -171,6 +181,23 @@ public function getVal( $name, $default = null ) {
public function getIP(): string {
return '127.0.0.1';
}
+
+ /**
+ * @return WebResponse
+ */
+ public function response() {
+ return new WebResponse();
+ }
+ }
+
+ class WebResponse {
+ /**
+ * @param string $string
+ * @param bool $replace
+ * @param null|int $http_response_code
+ */
+ public function header( $string, $replace = true, $http_response_code = null ) {
+ }
}
}
diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php
index af6569f..18e44cc 100644
--- a/tests/phpunit/unit/ResponseFactoryTest.php
+++ b/tests/phpunit/unit/ResponseFactoryTest.php
@@ -4,6 +4,7 @@
use MediaWiki\Config\ServiceOptions;
use MediaWiki\Extension\CrawlerProtection\ResponseFactory;
+use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
/**
@@ -13,12 +14,41 @@ class ResponseFactoryTest extends TestCase {
/** @var string */
private static string $outputPageClassName;
+ /** @var string */
+ private static string $webRequestClassName;
+
+ /** @var string */
+ private static string $webResponseClassName;
+
public static function setUpBeforeClass(): void {
parent::setUpBeforeClass();
self::$outputPageClassName = class_exists( '\MediaWiki\Output\OutputPage' )
? '\MediaWiki\Output\OutputPage'
: '\OutputPage';
+ self::$webRequestClassName = class_exists( '\MediaWiki\Request\WebRequest' )
+ ? '\MediaWiki\Request\WebRequest'
+ : '\WebRequest';
+ self::$webResponseClassName = class_exists( '\MediaWiki\Request\WebResponse' )
+ ? '\MediaWiki\Request\WebResponse'
+ : '\WebResponse';
+ }
+
+ /**
+ * Build an OutputPage mock whose getRequest()->response() is a mock too.
+ *
+ * @param MockObject|null &$response Receives the WebResponse mock
+ * @return MockObject OutputPage mock
+ */
+ private function buildOutputMock( &$response = null ) {
+ $response = $this->createMock( self::$webResponseClassName );
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'response' )->willReturn( $response );
+
+ $output = $this->createMock( self::$outputPageClassName );
+ $output->method( 'getRequest' )->willReturn( $request );
+
+ return $output;
}
/**
@@ -53,10 +83,13 @@ public function testDenyAccessPrettySetStatusCode() {
);
}
- $output = $this->createMock( self::$outputPageClassName );
+ $output = $this->buildOutputMock();
$output->expects( $this->once() )
->method( 'setStatusCode' )
->with( 403 );
+ $output->expects( $this->once() )
+ ->method( 'setRobotPolicy' )
+ ->with( 'noindex,nofollow' );
$output->expects( $this->once() )
->method( 'addWikiTextAsInterface' );
@@ -64,6 +97,27 @@ public function testDenyAccessPrettySetStatusCode() {
$factory->denyAccess( $output );
}
+ /**
+ * The pretty denial must ask crawlers not to index or follow the URL.
+ *
+ * @covers ::denyAccessPretty
+ */
+ public function testPrettyDenialSendsRobotsHeader() {
+ if ( defined( 'MEDIAWIKI' ) ) {
+ $this->markTestSkipped(
+ 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
+ );
+ }
+
+ $output = $this->buildOutputMock( $response );
+ $response->expects( $this->once() )
+ ->method( 'header' )
+ ->with( 'X-Robots-Tag: noindex,nofollow' );
+
+ $factory = $this->buildFactory();
+ $factory->denyAccess( $output );
+ }
+
/**
* @covers ::denyAccess
*/
@@ -282,7 +336,7 @@ public function testDenyAccessPrettyCallsSetPageTitleMsgOnModernOutputPage() {
);
}
- $output = $this->createMock( self::$outputPageClassName );
+ $output = $this->buildOutputMock();
$output->expects( $this->once() )->method( 'setPageTitleMsg' );
$output->expects( $this->never() )->method( 'setPageTitle' );
@@ -327,6 +381,20 @@ public function setPageTitle( $title ): void {
$this->count++;
}
+ public function setRobotPolicy( $policy ): void {
+ }
+
+ public function getRequest() {
+ return new class {
+ public function response() {
+ return new class {
+ public function header( $string, $replace = true, $code = null ) {
+ }
+ };
+ }
+ };
+ }
+
// Intentionally no setPageTitleMsg() to trigger the legacy branch.
};
From 9988307c916e81c1694f28f296fbb73a6186f66e Mon Sep 17 00:00:00 2001
From: Jeffrey Wang
Date: Wed, 29 Jul 2026 22:53:00 -0700
Subject: [PATCH 07/16] Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---
tests/phpunit/unit/ResponseFactoryTest.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php
index 18e44cc..fffe614 100644
--- a/tests/phpunit/unit/ResponseFactoryTest.php
+++ b/tests/phpunit/unit/ResponseFactoryTest.php
@@ -262,7 +262,7 @@ static function ( string $header, string $message ) use ( &$capturedHeader, &$ca
// Assert
$this->assertSame( 'HTTP/1.0 403 Forbidden', $capturedHeader );
- $this->assertSame( 'Mock message', $capturedBody );
+ $this->assertSame( wfMessage( 'crawlerprotection-rawdenial-text' )->inContentLanguage()->text(), $capturedBody );
}
/**
From ae265992be1d47f48d0f80e92b1419a6a601cf3a Mon Sep 17 00:00:00 2001
From: Jeffrey Wang
Date: Wed, 29 Jul 2026 22:53:09 -0700
Subject: [PATCH 08/16] Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---
.../CrawlerProtectionIntegrationTest.php | 21 +++++++++++--------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
index c54c9cd..bac061f 100644
--- a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -66,15 +66,18 @@ class CrawlerProtectionIntegrationTest extends MediaWikiIntegrationTestCase {
*/
private function overrideCrawlerProtectionConfig( array $overrides = [] ): void {
$defaults = [
- 'CrawlerProtectedActions' => [ 'history' ],
- 'CrawlerProtectedSpecialPages' => [ 'whatlinkshere', 'recentchangeslinked' ],
- 'CrawlerProtectedQueryParams' => [ 'target' ],
- 'CrawlerProtectionAllowedIPs' => [],
- 'CrawlerProtectionProtectRevisions' => true,
- 'CrawlerProtectionRawDenial' => false,
- 'CrawlerProtectionUse418' => false,
- 'CrawlerProtectionRawDenialHeader' => 'HTTP/1.0 403 Forbidden',
- 'CrawlerProtectionRawDenialText' => '403 Forbidden',
+ 'CrawlerProtectedActions' => [ 'history' ],
+ 'CrawlerProtectedApiModules' => [],
+ 'CrawlerProtectedRestPaths' => [],
+ 'CrawlerProtectedSpecialPages' => [ 'whatlinkshere', 'recentchangeslinked' ],
+ 'CrawlerProtectedQueryParams' => [ 'target' ],
+ 'CrawlerProtectionAllowedIPs' => [],
+ 'CrawlerProtectionProtectRevisions' => true,
+ 'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ 'CrawlerProtectionRawDenial' => false,
+ 'CrawlerProtectionUse418' => false,
+ 'CrawlerProtectionRawDenialHeader' => 'HTTP/1.0 403 Forbidden',
+ 'CrawlerProtectionRawDenialText' => '',
];
$this->overrideConfigValues( array_merge( $defaults, $overrides ) );
From ae308033760a271d46511750e255383da30d60b4 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 30 Jul 2026 06:01:07 +0000
Subject: [PATCH 09/16] Use canonical request IP for API/REST allowlist; run
denial tests under MediaWiki
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
README.md | 3 +-
includes/CrawlerProtectionService.php | 29 +++++++----
includes/Hooks.php | 15 +++++-
tests/phpunit/namespaced-stubs.php | 18 +++++++
.../unit/CrawlerProtectionServiceTest.php | 48 ++++++++++++++++++-
tests/phpunit/unit/HooksTest.php | 4 +-
tests/phpunit/unit/ResponseFactoryTest.php | 46 ++++--------------
7 files changed, 112 insertions(+), 51 deletions(-)
diff --git a/README.md b/README.md
index 6fb1d12..d71220b 100644
--- a/README.md
+++ b/README.md
@@ -82,7 +82,8 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
(`'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.
+ with the rest of MediaWiki. 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
diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php
index 79d521e..0866c74 100644
--- a/includes/CrawlerProtectionService.php
+++ b/includes/CrawlerProtectionService.php
@@ -288,10 +288,12 @@ public function checkSpecialPage(
*
* @param string $moduleName The canonical module name (e.g. "revisions", "compare")
* @param User $user
+ * @param WebRequest|null $request Request the check applies to, used to
+ * resolve the canonical client IP for the allowlist
* @return bool
*/
- public function checkApiModule( string $moduleName, $user ): bool {
- return $this->checkApiModules( [ $moduleName ], $user );
+ public function checkApiModule( string $moduleName, $user, $request = null ): bool {
+ return $this->checkApiModules( [ $moduleName ], $user, $request );
}
/**
@@ -305,14 +307,16 @@ public function checkApiModule( string $moduleName, $user ): bool {
* @param string[] $moduleNames Module names involved in the request, i.e.
* the requested action plus, for action=query, its sub-modules
* @param User $user
+ * @param WebRequest|null $request Request the check applies to, used to
+ * resolve the canonical client IP for the allowlist
* @return bool
*/
- public function checkApiModules( array $moduleNames, $user ): bool {
+ public function checkApiModules( array $moduleNames, $user, $request = null ): bool {
if ( $this->cliMode ) {
return true;
}
- if ( $this->isUserAllowed( $user ) || $this->isIPAllowed( $user->getName() ) ) {
+ if ( $this->isUserAllowed( $user ) || $this->isRequestIPAllowed( $request ) ) {
return true;
}
@@ -351,14 +355,16 @@ public function isProtectedApiModule( string $moduleName ): bool {
*
* @param string $path The request path (e.g. "/page/Main_Page/history")
* @param User $user
+ * @param WebRequest|null $request Request the check applies to, used to
+ * resolve the canonical client IP for the allowlist
* @return bool
*/
- public function checkRestPath( string $path, $user ): bool {
+ public function checkRestPath( string $path, $user, $request = null ): bool {
if ( $this->cliMode ) {
return true;
}
- if ( $this->isUserAllowed( $user ) || $this->isIPAllowed( $user->getName() ) ) {
+ if ( $this->isUserAllowed( $user ) || $this->isRequestIPAllowed( $request ) ) {
return true;
}
@@ -427,11 +433,18 @@ static function ( string $p ): string {
* The canonical client IP is taken from WebRequest (which correctly applies
* trusted-proxy / X-Forwarded-For handling) rather than the username.
*
- * @param WebRequest $request
+ * A null request (for example when an entry point cannot supply one) is
+ * never allowed through, so the regular protection logic applies.
+ *
+ * @param WebRequest|null $request
* @return bool
*/
private function isRequestIPAllowed( $request ): bool {
- return $this->normalizedAllowedIPs !== [] && $this->isIPAllowed( $request->getIP() );
+ if ( $request === null || $this->normalizedAllowedIPs === [] ) {
+ return false;
+ }
+
+ return $this->isIPAllowed( $request->getIP() );
}
/**
diff --git a/includes/Hooks.php b/includes/Hooks.php
index 54c25e5..bbbe26a 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -38,6 +38,7 @@ class_alias( '\WebRequest', '\MediaWiki\Request\WebRequest' );
if ( version_compare( MW_VERSION, '1.42', '<' ) ) {
class_alias( '\MediaWiki', '\MediaWiki\Actions\ActionEntryPoint' );
+ class_alias( '\RequestContext', '\MediaWiki\Context\RequestContext' );
}
if ( version_compare( MW_VERSION, '1.44', '<' ) ) {
@@ -45,6 +46,7 @@ class_alias( '\Article', '\MediaWiki\Page\Article' );
}
use MediaWiki\Actions\ActionEntryPoint;
+use MediaWiki\Context\RequestContext;
use MediaWiki\Hook\MediaWikiPerformActionHook;
use MediaWiki\Output\OutputPage;
use MediaWiki\Page\Article;
@@ -139,7 +141,8 @@ public function onSpecialPageBeforeExecute( $special, $subPage ) {
public function onApiCheckCanExecute( $module, $user, &$message ) {
if ( !$this->crawlerProtectionService->checkApiModules(
$this->getApiModuleNames( $module ),
- $user
+ $user,
+ $module->getMain()->getRequest()
) ) {
$message = 'crawlerprotection-accessdenied-text';
return false;
@@ -203,7 +206,15 @@ private function getApiModuleNames( $module ): array {
*/
public function onRestCheckCanExecute( $module, $handler, string $path, $request, &$error ) {
$user = $handler->getAuthority()->getUser();
- if ( !$this->crawlerProtectionService->checkRestPath( $path, $user ) ) {
+ // The REST RequestInterface exposes no canonical client IP, so the
+ // WebRequest of the current context is used instead: it applies the
+ // wiki's trusted-proxy / X-Forwarded-For handling, matching the
+ // behaviour of the index.php and api.php entry points.
+ if ( !$this->crawlerProtectionService->checkRestPath(
+ $path,
+ $user,
+ RequestContext::getMain()->getRequest()
+ ) ) {
$error = new LocalizedHttpException(
MessageValue::new( 'crawlerprotection-accessdenied-text' ),
403
diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php
index b910bc6..8cb8580 100644
--- a/tests/phpunit/namespaced-stubs.php
+++ b/tests/phpunit/namespaced-stubs.php
@@ -201,6 +201,24 @@ public function header( $string, $replace = true, $http_response_code = null ) {
}
}
+namespace MediaWiki\Context {
+ class RequestContext {
+ /**
+ * @return self
+ */
+ public static function getMain(): self {
+ return new self();
+ }
+
+ /**
+ * @return \MediaWiki\Request\WebRequest
+ */
+ public function getRequest() {
+ return new \MediaWiki\Request\WebRequest();
+ }
+ }
+}
+
namespace MediaWiki\Title {
class Title {
}
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index a4cfe79..3a7fcc9 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -1414,6 +1414,28 @@ public function testCheckApiModulesAllowsRegisteredUser() {
* @covers ::checkApiModule
*/
public function testCheckApiModuleAllowsAllowedIP() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ // The username must never be used to resolve the client IP.
+ $user->expects( $this->never() )->method( 'getName' );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [ '1.2.3.4' ], null, true, [], false, [ 'revisions' ]
+ );
+ $this->assertTrue( $service->checkApiModule( 'revisions', $user, $request ) );
+ }
+
+ /**
+ * Without a request the allowlist cannot be evaluated, so a protected
+ * module stays protected rather than being allowed by a username that
+ * happens to look like an allowlisted IP.
+ *
+ * @covers ::checkApiModule
+ */
+ public function testCheckApiModuleDeniesWhenNoRequestGiven() {
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
$user->method( 'getName' )->willReturn( '1.2.3.4' );
@@ -1421,7 +1443,7 @@ public function testCheckApiModuleAllowsAllowedIP() {
$service = $this->buildService(
[], [], [ '1.2.3.4' ], null, true, [], false, [ 'revisions' ]
);
- $this->assertTrue( $service->checkApiModule( 'revisions', $user ) );
+ $this->assertFalse( $service->checkApiModule( 'revisions', $user ) );
}
// ---------------------------------------------------------------
@@ -1556,6 +1578,28 @@ public function testCheckRestPathAllowsOnCommandLine() {
* @covers ::checkRestPath
*/
public function testCheckRestPathAllowsAllowedIP() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ // The username must never be used to resolve the client IP.
+ $user->expects( $this->never() )->method( 'getName' );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [ '1.2.3.4' ], null, true, [], false, [], [ '/page/*/history' ]
+ );
+ $this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user, $request ) );
+ }
+
+ /**
+ * Without a request the allowlist cannot be evaluated, so a protected
+ * path stays protected rather than being allowed by a username that
+ * happens to look like an allowlisted IP.
+ *
+ * @covers ::checkRestPath
+ */
+ public function testCheckRestPathDeniesWhenNoRequestGiven() {
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
$user->method( 'getName' )->willReturn( '1.2.3.4' );
@@ -1563,7 +1607,7 @@ public function testCheckRestPathAllowsAllowedIP() {
$service = $this->buildService(
[], [], [ '1.2.3.4' ], null, true, [], false, [], [ '/page/*/history' ]
);
- $this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user ) );
+ $this->assertFalse( $service->checkRestPath( '/page/Main_Page/history', $user ) );
}
// ---------------------------------------------------------------
diff --git a/tests/phpunit/unit/HooksTest.php b/tests/phpunit/unit/HooksTest.php
index 7e636ac..88768aa 100644
--- a/tests/phpunit/unit/HooksTest.php
+++ b/tests/phpunit/unit/HooksTest.php
@@ -280,7 +280,7 @@ public function testOnApiCheckCanExecuteDelegatesToService() {
$service = $this->createMock( CrawlerProtectionService::class );
$service->expects( $this->once() )
->method( 'checkApiModules' )
- ->with( [ 'compare' ], $user )
+ ->with( [ 'compare' ], $user, $module->getMain()->getRequest() )
->willReturn( false );
$hooks = new Hooks( $service );
@@ -433,7 +433,7 @@ public function testOnRestCheckCanExecuteDelegatesToService() {
$service = $this->createMock( CrawlerProtectionService::class );
$service->expects( $this->once() )
->method( 'checkRestPath' )
- ->with( '/page/Main_Page/history', $user )
+ ->with( '/page/Main_Page/history', $user, $this->isInstanceOf( self::$webRequestClassName ) )
->willReturn( false );
$hooks = new Hooks( $service );
diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php
index fffe614..8d3ddc6 100644
--- a/tests/phpunit/unit/ResponseFactoryTest.php
+++ b/tests/phpunit/unit/ResponseFactoryTest.php
@@ -77,12 +77,6 @@ private function buildFactory( array $overrides = [] ): ResponseFactory {
* @covers ::denyAccessPretty
*/
public function testDenyAccessPrettySetStatusCode() {
- if ( defined( 'MEDIAWIKI' ) ) {
- $this->markTestSkipped(
- 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
- );
- }
-
$output = $this->buildOutputMock();
$output->expects( $this->once() )
->method( 'setStatusCode' )
@@ -103,12 +97,6 @@ public function testDenyAccessPrettySetStatusCode() {
* @covers ::denyAccessPretty
*/
public function testPrettyDenialSendsRobotsHeader() {
- if ( defined( 'MEDIAWIKI' ) ) {
- $this->markTestSkipped(
- 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
- );
- }
-
$output = $this->buildOutputMock( $response );
$response->expects( $this->once() )
->method( 'header' )
@@ -225,12 +213,6 @@ public function testDenyAccessFallsThroughToPretty() {
* @covers ::denyAccess
*/
public function testDenyAccessRawUsesI18nWhenOverrideIsEmpty() {
- if ( defined( 'MEDIAWIKI' ) ) {
- $this->markTestSkipped(
- 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
- );
- }
-
// Arrange
$capturedHeader = null;
$capturedBody = null;
@@ -262,7 +244,10 @@ static function ( string $header, string $message ) use ( &$capturedHeader, &$ca
// Assert
$this->assertSame( 'HTTP/1.0 403 Forbidden', $capturedHeader );
- $this->assertSame( wfMessage( 'crawlerprotection-rawdenial-text' )->inContentLanguage()->text(), $capturedBody );
+ $this->assertSame(
+ wfMessage( 'crawlerprotection-rawdenial-text' )->inContentLanguage()->text(),
+ $capturedBody
+ );
}
/**
@@ -274,12 +259,6 @@ static function ( string $header, string $message ) use ( &$capturedHeader, &$ca
* @covers ::denyAccessWith418
*/
public function testDenyAccessWith418UsesI18nMessage() {
- if ( defined( 'MEDIAWIKI' ) ) {
- $this->markTestSkipped(
- 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
- );
- }
-
// Arrange
$capturedHeader = null;
$capturedBody = null;
@@ -311,7 +290,10 @@ static function ( string $header, string $message ) use ( &$capturedHeader, &$ca
// Assert
$this->assertSame( 'HTTP/1.0 418 I\'m a teapot', $capturedHeader );
- $this->assertSame( 'Mock message', $capturedBody );
+ $this->assertSame(
+ wfMessage( 'crawlerprotection-rawdenial-teapot' )->inContentLanguage()->text(),
+ $capturedBody
+ );
}
/**
@@ -330,10 +312,8 @@ public function testConstructorAcceptsValidOptions() {
* @covers ::denyAccessPretty
*/
public function testDenyAccessPrettyCallsSetPageTitleMsgOnModernOutputPage() {
- if ( defined( 'MEDIAWIKI' ) ) {
- $this->markTestSkipped(
- 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
- );
+ if ( !method_exists( self::$outputPageClassName, 'setPageTitleMsg' ) ) {
+ $this->markTestSkipped( 'OutputPage::setPageTitleMsg() requires MediaWiki 1.41+' );
}
$output = $this->buildOutputMock();
@@ -354,12 +334,6 @@ public function testDenyAccessPrettyCallsSetPageTitleMsgOnModernOutputPage() {
* @covers ::denyAccessPretty
*/
public function testDenyAccessPrettyFallsBackToSetPageTitleOnLegacyOutputPage() {
- if ( defined( 'MEDIAWIKI' ) ) {
- $this->markTestSkipped(
- 'Skipped in MediaWiki integration environment: wfMessage() requires service container'
- );
- }
-
$setPageTitleCallCount = 0;
// Anonymous class without setPageTitleMsg() simulates MW < 1.41 OutputPage.
From 65b0fe72cb15448971c7139fd44ea79943450102 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 30 Jul 2026 06:33:19 +0000
Subject: [PATCH 10/16] Add opt-in X-Forwarded-For allowlist matching for wikis
behind a reverse proxy
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
README.md | 44 ++++
extension.json | 3 +
includes/CrawlerProtectionService.php | 58 ++++-
.../CrawlerProtectionIntegrationTest.php | 93 ++++++++
tests/phpunit/namespaced-stubs.php | 9 +
.../unit/CrawlerProtectionServiceTest.php | 215 +++++++++++++++++-
6 files changed, 420 insertions(+), 2 deletions(-)
diff --git a/README.md b/README.md
index d71220b..5043b03 100644
--- a/README.md
+++ b/README.md
@@ -91,11 +91,55 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
When `false` (default), temporary-account users are treated as registered
users and bypass all protection checks. Set to `true` if you do not want
crawlers that receive a temporary account to bypass protection.
+* `$wgCrawlerProtectionTrustXForwardedFor` - when `true`, the IP allowlist also
+ matches against the address reported in the `X-Forwarded-For` header
+ (default: `false`). See
+ [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 `` tag, so that well-behaved crawlers stop
re-requesting denied URLs.
+## Wikis behind a reverse proxy
+
+When the wiki sits behind a reverse proxy such as HAProxy, nginx or Varnish,
+every request reaches PHP from the proxy's address. `WebRequest::getIP()` only
+follows `X-Forwarded-For` for proxies MediaWiki has been told to trust, so
+until the proxy is declared, `$wgCrawlerProtectionAllowedIPs` (and MediaWiki's
+own blocking, rate limiting and `CheckUser` data) sees the proxy address
+instead of the visitor's.
+
+The correct fix is a one-line MediaWiki setting rather than an Apache or
+extension change - declare the proxy in `LocalSettings.php`:
+
+```php
+$wgCdnServersNoPurge = [ '10.0.0.1' ]; // HAProxy address or CIDR range
+```
+
+Do this whenever you can: it fixes the client IP wiki-wide, for every feature,
+not just for this extension. (Add `$wgUsePrivateIPs = true;` as well if the
+visitors you want to allowlist use private addresses.)
+
+For wikis that cannot change that setting, this extension offers a narrower
+opt-in fallback that applies to the allowlist only:
+
+```php
+$wgCrawlerProtectionTrustXForwardedFor = true;
+```
+
+With it enabled, when the connecting address does not match
+`$wgCrawlerProtectionAllowedIPs` the *last* entry of `X-Forwarded-For` is
+checked as well. Only the last entry is used, because a reverse proxy appends
+the address it observed to the end of the chain; any earlier entries may have
+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.
+
# Hooks
## CrawlerProtectionShouldDeny
diff --git a/extension.json b/extension.json
index 9c81940..705599c 100644
--- a/extension.json
+++ b/extension.json
@@ -75,6 +75,9 @@
},
"CrawlerProtectionTreatTempUsersAsAnon": {
"value": false
+ },
+ "CrawlerProtectionTrustXForwardedFor": {
+ "value": false
}
},
"ServiceWiringFiles": [
diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php
index 0866c74..b460a60 100644
--- a/includes/CrawlerProtectionService.php
+++ b/includes/CrawlerProtectionService.php
@@ -55,6 +55,7 @@ class CrawlerProtectionService {
'CrawlerProtectionAllowedIPs',
'CrawlerProtectionProtectRevisions',
'CrawlerProtectionTreatTempUsersAsAnon',
+ 'CrawlerProtectionTrustXForwardedFor',
];
/** @var ServiceOptions */
@@ -433,6 +434,10 @@ static function ( string $p ): string {
* The canonical client IP is taken from WebRequest (which correctly applies
* trusted-proxy / X-Forwarded-For handling) rather than the username.
*
+ * When CrawlerProtectionTrustXForwardedFor is enabled, the address reported
+ * by the reverse proxy in X-Forwarded-For is consulted as well, for wikis
+ * whose proxy is not registered in $wgCdnServersNoPurge.
+ *
* A null request (for example when an entry point cannot supply one) is
* never allowed through, so the regular protection logic applies.
*
@@ -444,7 +449,58 @@ private function isRequestIPAllowed( $request ): bool {
return false;
}
- return $this->isIPAllowed( $request->getIP() );
+ if ( $this->isIPAllowed( $request->getIP() ) ) {
+ return true;
+ }
+
+ $forwardedIP = $this->getForwardedIP( $request );
+
+ return $forwardedIP !== null && $this->isIPAllowed( $forwardedIP );
+ }
+
+ /**
+ * Resolve the client address reported by the immediate reverse proxy in the
+ * X-Forwarded-For header, or null when it is unavailable or not trusted.
+ *
+ * MediaWiki only follows X-Forwarded-For for proxies that are registered as
+ * trusted (see $wgCdnServers / $wgCdnServersNoPurge), so behind an
+ * unregistered proxy such as HAProxy WebRequest::getIP() returns the proxy's
+ * own address. Registering the proxy remains the correct fix; this opt-in
+ * fallback covers wikis that cannot do so.
+ *
+ * Only the *last* entry of the header is used. A reverse proxy appends the
+ * address it observed to the end of the chain, so earlier entries may have
+ * been supplied by the client and must never be trusted. This still assumes
+ * that every request reaches the wiki through exactly one such proxy, which
+ * is why the behaviour is disabled by default.
+ *
+ * @param WebRequest $request
+ * @return string|null Sanitized IP address, or null if none can be trusted
+ */
+ private function getForwardedIP( $request ): ?string {
+ if ( !$this->options->get( 'CrawlerProtectionTrustXForwardedFor' ) ) {
+ return null;
+ }
+
+ $forwardedFor = $request->getHeader( 'X-Forwarded-For' );
+ if ( !is_string( $forwardedFor ) || trim( $forwardedFor ) === '' ) {
+ return null;
+ }
+
+ $chain = explode( ',', $forwardedFor );
+ $candidate = trim( (string)end( $chain ) );
+ if ( $candidate === '' ) {
+ return null;
+ }
+
+ // canonicalize() rejects anything that is not a single IP address,
+ // returning null (older releases returned false).
+ $canonical = IPUtils::canonicalize( $candidate );
+ if ( !is_string( $canonical ) || $canonical === '' ) {
+ return null;
+ }
+
+ return IPUtils::sanitizeIP( $canonical );
}
/**
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
index bac061f..14e2243 100644
--- a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -74,6 +74,7 @@ private function overrideCrawlerProtectionConfig( array $overrides = [] ): void
'CrawlerProtectionAllowedIPs' => [],
'CrawlerProtectionProtectRevisions' => true,
'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ 'CrawlerProtectionTrustXForwardedFor' => false,
'CrawlerProtectionRawDenial' => false,
'CrawlerProtectionUse418' => false,
'CrawlerProtectionRawDenialHeader' => 'HTTP/1.0 403 Forbidden',
@@ -313,6 +314,98 @@ public function testRegisteredUserWithProtectedActionIsNotBlocked(): void {
$this->assertTrue( $result, 'checkPerformAction must return true for registered users' );
}
+ // ---------------------------------------------------------------
+ // X-Forwarded-For allowlist
+ // ---------------------------------------------------------------
+
+ /**
+ * Build a request that reaches the wiki through a reverse proxy: the
+ * connecting address is the proxy, and the client address is reported in
+ * the X-Forwarded-For header.
+ *
+ * A real WebRequest is used so that the header lookup exercises
+ * MediaWiki's own getHeader() implementation.
+ *
+ * @param array $params Query parameters
+ * @param string $forwardedFor X-Forwarded-For header value
+ * @param string $proxyIP Connecting (proxy) address
+ * @return \MediaWiki\Request\FauxRequest|\FauxRequest
+ */
+ private function makeProxiedRequest(
+ array $params,
+ string $forwardedFor,
+ string $proxyIP = '10.0.0.1'
+ ) {
+ $request = $this->makeRequest( $params );
+ $request->setIP( $proxyIP );
+ $request->setHeaders( [ 'X-Forwarded-For' => $forwardedFor ] );
+
+ return $request;
+ }
+
+ /**
+ * Behind a reverse proxy that is not registered in $wgCdnServersNoPurge,
+ * WebRequest::getIP() reports the proxy address, so an allowlisted client
+ * is still blocked while CrawlerProtectionTrustXForwardedFor is off.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkPerformAction
+ */
+ public function testForwardedIPIsIgnoredWhenTrustDisabled(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedActions' => [ 'history' ],
+ 'CrawlerProtectionAllowedIPs' => [ '1.2.3.4' ],
+ ] );
+
+ $service = $this->makeWebModeService();
+ $request = $this->makeProxiedRequest( [ 'action' => 'history' ], '1.2.3.4' );
+
+ $this->assertFalse(
+ $service->checkPerformAction( $this->makeOutputPage(), $this->makeAnonUser(), $request )
+ );
+ }
+
+ /**
+ * With CrawlerProtectionTrustXForwardedFor enabled, the address reported
+ * by the proxy is matched against the allowlist.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkPerformAction
+ */
+ public function testForwardedIPIsAllowedWhenTrustEnabled(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedActions' => [ 'history' ],
+ 'CrawlerProtectionAllowedIPs' => [ '1.2.3.4' ],
+ 'CrawlerProtectionTrustXForwardedFor' => true,
+ ] );
+
+ $service = $this->makeWebModeService();
+ $request = $this->makeProxiedRequest( [ 'action' => 'history' ], '1.2.3.4' );
+
+ $this->assertTrue(
+ $service->checkPerformAction( $this->makeOutputPage(), $this->makeAnonUser(), $request )
+ );
+ }
+
+ /**
+ * Entries a client prepends to the header must never be trusted: only the
+ * address appended by the proxy (the last entry) is matched.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkPerformAction
+ */
+ public function testSpoofedForwardedChainIsRejected(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedActions' => [ 'history' ],
+ 'CrawlerProtectionAllowedIPs' => [ '1.2.3.4' ],
+ 'CrawlerProtectionTrustXForwardedFor' => true,
+ ] );
+
+ $service = $this->makeWebModeService();
+ $request = $this->makeProxiedRequest( [ 'action' => 'history' ], '1.2.3.4, 203.0.113.9' );
+
+ $this->assertFalse(
+ $service->checkPerformAction( $this->makeOutputPage(), $this->makeAnonUser(), $request )
+ );
+ }
+
// ---------------------------------------------------------------
// CrawlerProtectionService::checkSpecialPage()
// ---------------------------------------------------------------
diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php
index 8cb8580..622f8bf 100644
--- a/tests/phpunit/namespaced-stubs.php
+++ b/tests/phpunit/namespaced-stubs.php
@@ -182,6 +182,15 @@ public function getIP(): string {
return '127.0.0.1';
}
+ /**
+ * @param string $name
+ * @param int $flags
+ * @return string|false
+ */
+ public function getHeader( $name, $flags = 0 ) {
+ return false;
+ }
+
/**
* @return WebResponse
*/
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index 3a7fcc9..ff37257 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -53,6 +53,7 @@ public static function setUpBeforeClass(): void {
* @param array $protectedRestPaths
* @param bool $treatTempUsersAsAnon
* @param callable[] $shouldDenyHandlers Handlers for CrawlerProtectionShouldDeny
+ * @param bool $trustXForwardedFor
* @return CrawlerProtectionService
*/
private function buildService(
@@ -66,7 +67,8 @@ private function buildService(
array $protectedApiModules = [],
array $protectedRestPaths = [],
bool $treatTempUsersAsAnon = false,
- array $shouldDenyHandlers = []
+ array $shouldDenyHandlers = [],
+ bool $trustXForwardedFor = false
): CrawlerProtectionService {
$options = new ServiceOptions(
CrawlerProtectionService::CONSTRUCTOR_OPTIONS,
@@ -79,6 +81,7 @@ private function buildService(
'CrawlerProtectionAllowedIPs' => $allowedIPs,
'CrawlerProtectionProtectRevisions' => $protectRevisions,
'CrawlerProtectionTreatTempUsersAsAnon' => $treatTempUsersAsAnon,
+ 'CrawlerProtectionTrustXForwardedFor' => $trustXForwardedFor,
]
);
@@ -1053,6 +1056,216 @@ public function testCheckSpecialPageUsesRequestIPNotUsername() {
$this->assertTrue( $service->checkSpecialPage( 'WhatLinksHere', $output, $user, $request ) );
}
+ // ---------------------------------------------------------------
+ // X-Forwarded-For allowlist tests
+ // ---------------------------------------------------------------
+
+ /**
+ * Build an anonymous request whose canonical IP is the reverse proxy and
+ * whose X-Forwarded-For header carries the given value.
+ *
+ * @param string|false $forwardedFor Header value, or false when absent
+ * @param string $proxyIP Address WebRequest::getIP() resolves to
+ * @return \PHPUnit\Framework\MockObject\MockObject WebRequest mock
+ */
+ private function makeProxiedRequest( $forwardedFor, string $proxyIP = '10.0.0.1' ) {
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( $proxyIP );
+ $request->method( 'getHeader' )->willReturn( $forwardedFor );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'type', null, 'revision' ],
+ ] );
+
+ return $request;
+ }
+
+ /**
+ * Behind a reverse proxy that MediaWiki does not know about, the canonical
+ * IP is the proxy's own address, so an allowlisted client is blocked unless
+ * CrawlerProtectionTrustXForwardedFor is enabled.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testForwardedIPIsIgnoredWhenTrustDisabled() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->makeProxiedRequest( '1.2.3.4' );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ $service = $this->buildService( [], [ 'history' ], [ '1.2.3.4' ], $responseFactory );
+ $this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * @covers ::checkPerformAction
+ */
+ public function testForwardedIPIsAllowedWhenTrustEnabled() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->makeProxiedRequest( '1.2.3.4' );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $service = $this->buildService(
+ [], [ 'history' ], [ '1.2.3.4' ], $responseFactory, true, [ 'target' ], false, [], [], false, [], true
+ );
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * CIDR and explicit ranges must work for forwarded addresses too.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testForwardedIPMatchesAllowedRange() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->makeProxiedRequest( '2001:0db8:85a3::7344' );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $service = $this->buildService(
+ [], [ 'history' ], [ '2001:0db8:85a3::/96' ], $responseFactory,
+ true, [ 'target' ], false, [], [], false, [], true
+ );
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * Only the address appended by the reverse proxy (the last entry) counts.
+ * A client that prepends an allowlisted address to spoof the header must
+ * still be blocked.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testForwardedIPIgnoresClientSuppliedChainEntries() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ // The client claimed to be 1.2.3.4; the proxy appended the address it saw.
+ $request = $this->makeProxiedRequest( '1.2.3.4, 203.0.113.9' );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ $service = $this->buildService(
+ [], [ 'history' ], [ '1.2.3.4' ], $responseFactory, true, [ 'target' ], false, [], [], false, [], true
+ );
+ $this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * @covers ::checkPerformAction
+ * @dataProvider provideUnusableForwardedForHeaders
+ *
+ * @param string|false $forwardedFor
+ */
+ public function testUnusableForwardedForHeaderDenies( $forwardedFor ) {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->makeProxiedRequest( $forwardedFor );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ $service = $this->buildService(
+ [], [ 'history' ], [ '1.2.3.4' ], $responseFactory, true, [ 'target' ], false, [], [], false, [], true
+ );
+ $this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ public function provideUnusableForwardedForHeaders(): array {
+ return [
+ 'header absent' => [ false ],
+ 'empty header' => [ '' ],
+ 'whitespace only' => [ ' ' ],
+ 'not an IP address' => [ 'unknown' ],
+ 'range instead of address' => [ '1.2.3.0/24' ],
+ 'trailing separator' => [ '1.2.3.4,' ],
+ ];
+ }
+
+ /**
+ * The forwarded address is only consulted when the canonical IP does not
+ * already match, and never allows a request the allowlist does not cover.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testForwardedIPDoesNotOverrideAllowedCanonicalIP() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ // Canonical IP is allowlisted, forwarded address is not.
+ $request = $this->makeProxiedRequest( '203.0.113.9', '1.2.3.4' );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'denyAccess' );
+
+ $service = $this->buildService(
+ [], [ 'history' ], [ '1.2.3.4' ], $responseFactory, true, [ 'target' ], false, [], [], false, [], true
+ );
+ $this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * An empty allowlist short-circuits before the header is read, so enabling
+ * the toggle alone can never let a request through.
+ *
+ * @covers ::checkPerformAction
+ */
+ public function testForwardedIPIsNotConsultedWithEmptyAllowlist() {
+ $output = $this->createMock( self::$outputPageClassName );
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'getIP' )->willReturn( '10.0.0.1' );
+ $request->expects( $this->never() )->method( 'getHeader' );
+ $request->method( 'getVal' )->willReturnMap( [
+ [ 'type', null, 'revision' ],
+ ] );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
+
+ $service = $this->buildService(
+ [], [ 'history' ], [], $responseFactory, true, [ 'target' ], false, [], [], false, [], true
+ );
+ $this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
+ }
+
+ /**
+ * The REST entry point resolves the allowlist through the same request, so
+ * the forwarded address applies there as well.
+ *
+ * @covers ::checkRestPath
+ */
+ public function testForwardedIPAppliesToRestPaths() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $request = $this->makeProxiedRequest( '1.2.3.4' );
+
+ $service = $this->buildService(
+ [], [], [ '1.2.3.4' ], null, true, [], false, [], [ '/page/*/history' ], false, [], true
+ );
+ $this->assertTrue( $service->checkRestPath( '/page/Main_Page/history', $user, $request ) );
+ }
+
// ---------------------------------------------------------------
// Temporary-account user tests
// ---------------------------------------------------------------
From fbe4022e2b5326322a2928559fe940a167a0ffec Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 30 Jul 2026 06:43:41 +0000
Subject: [PATCH 11/16] Add missing CrawlerProtectionTrustXForwardedFor key to
scalar-config unit tests
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
tests/phpunit/unit/CrawlerProtectionServiceTest.php | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index ff37257..f366679 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -1373,6 +1373,7 @@ public function testIsProtectedActionToleratesScalarConfig() {
'CrawlerProtectionAllowedIPs' => [],
'CrawlerProtectionProtectRevisions' => true,
'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ 'CrawlerProtectionTrustXForwardedFor' => false,
]
);
$service = new CrawlerProtectionService(
@@ -1406,6 +1407,7 @@ public function testHasProtectedQueryParamToleratesScalarConfig() {
'CrawlerProtectionAllowedIPs' => [],
'CrawlerProtectionProtectRevisions' => true,
'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ 'CrawlerProtectionTrustXForwardedFor' => false,
]
);
$service = new CrawlerProtectionService(
@@ -1444,6 +1446,7 @@ public function testIsProtectedSpecialPageToleratesScalarConfig() {
'CrawlerProtectionAllowedIPs' => [],
'CrawlerProtectionProtectRevisions' => true,
'CrawlerProtectionTreatTempUsersAsAnon' => false,
+ 'CrawlerProtectionTrustXForwardedFor' => false,
]
);
$service = new CrawlerProtectionService(
From 78ad668d75bcf129624f624d52a1ea59c53b0761 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Thu, 30 Jul 2026 14:00:54 -0700
Subject: [PATCH 12/16] Address cicalese review: denial headers, API 403, hook
entry point, docs and tests (#66)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
.github/scripts/check-i18n-qqq.sh | 78 ++++++--
README.md | 49 +++--
TESTING.md | 3 +-
composer.json | 3 +-
includes/CrawlerProtectionService.php | 79 +++++++-
.../Hook/CrawlerProtectionShouldDenyHook.php | 27 ++-
includes/HookRunner.php | 3 +-
includes/ResponseFactory.php | 34 +++-
.../CrawlerProtectionIntegrationTest.php | 148 ++++++++++++++
tests/phpunit/namespaced-stubs.php | 6 +
.../unit/CrawlerProtectionServiceTest.php | 188 ++++++++++++++++--
tests/phpunit/unit/ResponseFactoryTest.php | 48 +++++
12 files changed, 593 insertions(+), 73 deletions(-)
diff --git a/.github/scripts/check-i18n-qqq.sh b/.github/scripts/check-i18n-qqq.sh
index ec14ba8..be95be4 100755
--- a/.github/scripts/check-i18n-qqq.sh
+++ b/.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'
+` 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 `` 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;
}
};
diff --git a/TESTING.md b/TESTING.md
index ad2e46f..292f911 100644
--- a/TESTING.md
+++ b/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
diff --git a/composer.json b/composer.json
index b404c79..1d8061e 100644
--- a/composer.json
+++ b/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": {
diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php
index b460a60..07636e7 100644
--- a/includes/CrawlerProtectionService.php
+++ b/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.',
diff --git a/includes/Hook/CrawlerProtectionShouldDenyHook.php b/includes/Hook/CrawlerProtectionShouldDenyHook.php
index 272c92c..8e63399 100644
--- a/includes/Hook/CrawlerProtectionShouldDenyHook.php
+++ b/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
);
diff --git a/includes/HookRunner.php b/includes/HookRunner.php
index e5220bc..5345df2 100644
--- a/includes/HookRunner.php
+++ b/includes/HookRunner.php
@@ -51,12 +51,13 @@ public function __construct( HookContainer $hookContainer ) {
public function onCrawlerProtectionShouldDeny(
$user,
$request,
+ string $entryPoint,
?string $specialPageName,
bool &$shouldDeny
) {
return $this->hookContainer->run(
'CrawlerProtectionShouldDeny',
- [ $user, $request, $specialPageName, &$shouldDeny ]
+ [ $user, $request, $entryPoint, $specialPageName, &$shouldDeny ]
);
}
}
diff --git a/includes/ResponseFactory.php b/includes/ResponseFactory.php
index 94453d8..d50715b 100644
--- a/includes/ResponseFactory.php
+++ b/includes/ResponseFactory.php
@@ -40,7 +40,7 @@ class ResponseFactory {
private const TEAPOT_HEADER = 'HTTP/1.0 418 I\'m a teapot';
- /** Robot directive sent on the pretty denial page */
+ /** Robot directive sent on every denial response */
private const ROBOT_POLICY = 'noindex,nofollow';
/** @var string[] List of constructor options this class accepts */
@@ -117,6 +117,10 @@ protected function denyAccessWith418(): void {
/**
* Output a raw HTTP response and halt.
*
+ * The robot directive is sent here as well as on the pretty denial page,
+ * so that all three denial strategies tell well-behaved crawlers not to
+ * re-request the URL.
+ *
* @param string $header
* @param string $message
* @return void
@@ -124,9 +128,37 @@ protected function denyAccessWith418(): void {
*/
protected function denyAccessRaw( string $header, string $message ): void {
header( $header );
+ header( 'X-Robots-Tag: ' . self::ROBOT_POLICY );
die( $message );
}
+ /**
+ * Mark a denial that is not rendered through OutputPage.
+ *
+ * Used for the Action API and REST entry points, which produce their own
+ * error body. Core's ApiCheckCanExecute denial path calls
+ * ApiBase::dieWithError() without an HTTP code, which would otherwise
+ * return "200 OK" with an error body and nothing telling the crawler to
+ * stop, so the status code is set here.
+ *
+ * @param mixed $response WebResponse to write headers to, or null when the
+ * entry point cannot supply one. WebResponse moved between namespaces
+ * across supported releases, hence the loose type.
+ * @param int|null $statusCode HTTP status to set, or null to leave it alone
+ * @return void
+ */
+ public function markDenied( $response, ?int $statusCode = null ): void {
+ if ( $response === null ) {
+ return;
+ }
+
+ if ( $statusCode !== null ) {
+ $response->statusHeader( $statusCode );
+ }
+
+ $response->header( 'X-Robots-Tag: ' . self::ROBOT_POLICY );
+ }
+
/**
* Output a pretty 403 Access Denied page using i18n messages.
*
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
index 14e2243..69ed5c2 100644
--- a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -220,6 +220,88 @@ public function testSpecialPageBeforeExecuteHookIsRegistered(): void {
);
}
+ /**
+ * Verify that the ApiCheckCanExecute hook handler is registered.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onApiCheckCanExecute
+ */
+ public function testApiCheckCanExecuteHookIsRegistered(): void {
+ $this->assertTrue(
+ $this->getServiceContainer()->getHookContainer()->isRegistered( 'ApiCheckCanExecute' ),
+ 'ApiCheckCanExecute hook must be registered by the extension'
+ );
+ }
+
+ /**
+ * Verify that the RestCheckCanExecute hook handler is registered.
+ *
+ * The hook itself only fires on MediaWiki 1.44 and later, but the handler
+ * is registered on every supported version.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onRestCheckCanExecute
+ */
+ public function testRestCheckCanExecuteHookIsRegistered(): void {
+ $this->assertTrue(
+ $this->getServiceContainer()->getHookContainer()->isRegistered( 'RestCheckCanExecute' ),
+ 'RestCheckCanExecute hook must be registered by the extension'
+ );
+ }
+
+ /**
+ * Hooks deliberately does not implement core's ApiCheckCanExecuteHook and
+ * RestCheckCanExecuteHook interfaces, so PHP cannot catch signature drift
+ * against core. Compare the handler signatures with the core interfaces
+ * wherever those interfaces exist.
+ *
+ * @dataProvider provideCoreHookInterfaces
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onApiCheckCanExecute
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onRestCheckCanExecute
+ * @param string $interface Core hook interface name
+ * @param string $method Handler method name
+ */
+ public function testHandlerSignatureMatchesCoreInterface( string $interface, string $method ): void {
+ if ( !interface_exists( $interface ) ) {
+ $this->markTestSkipped( "$interface is not available on this MediaWiki version" );
+ }
+
+ $expected = new \ReflectionMethod( $interface, $method );
+ $actual = new \ReflectionMethod( \MediaWiki\Extension\CrawlerProtection\Hooks::class, $method );
+
+ $this->assertSame(
+ $expected->getNumberOfParameters(),
+ $actual->getNumberOfParameters(),
+ "$method must take the same number of parameters as $interface"
+ );
+
+ foreach ( $expected->getParameters() as $index => $parameter ) {
+ $this->assertSame(
+ $parameter->isPassedByReference(),
+ $actual->getParameters()[$index]->isPassedByReference(),
+ "Parameter #$index of $method must match the by-reference mode of $interface"
+ );
+ }
+ }
+
+ /**
+ * @return array[]
+ */
+ public static function provideCoreHookInterfaces(): array {
+ return [
+ 'ApiCheckCanExecute' => [
+ 'MediaWiki\\Api\\Hook\\ApiCheckCanExecuteHook',
+ 'onApiCheckCanExecute',
+ ],
+ 'ApiCheckCanExecute (legacy namespace)' => [
+ 'MediaWiki\\Hook\\ApiCheckCanExecuteHook',
+ 'onApiCheckCanExecute',
+ ],
+ 'RestCheckCanExecute' => [
+ 'MediaWiki\\Rest\\Hook\\RestCheckCanExecuteHook',
+ 'onRestCheckCanExecute',
+ ],
+ ];
+ }
+
// ---------------------------------------------------------------
// ResponseFactory::denyAccessPretty() with real OutputPage
// ---------------------------------------------------------------
@@ -457,4 +539,70 @@ public function testRegisteredUserOnProtectedSpecialPageIsNotBlocked(): void {
$this->assertTrue( $result, 'checkSpecialPage must return true for registered users' );
}
+
+ // ---------------------------------------------------------------
+ // Action API and REST entry points
+ // ---------------------------------------------------------------
+
+ /**
+ * An anonymous Action API request naming a protected module must be
+ * denied.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkApiModules
+ */
+ public function testAnonymousApiRequestForProtectedModuleIsBlocked(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedApiModules' => [ 'revisions' ],
+ ] );
+
+ $service = $this->makeWebModeService();
+
+ $this->assertFalse(
+ $service->checkApiModules(
+ [ 'query', 'revisions' ],
+ $this->makeAnonUser(),
+ $this->makeRequest( [] )
+ )
+ );
+ }
+
+ /**
+ * A registered user must not be blocked on the Action API.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkApiModules
+ */
+ public function testRegisteredApiRequestForProtectedModuleIsNotBlocked(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedApiModules' => [ 'revisions' ],
+ ] );
+
+ $service = $this->makeWebModeService();
+
+ $this->assertTrue(
+ $service->checkApiModules(
+ [ 'query', 'revisions' ],
+ $this->getMutableTestUser()->getUser(),
+ $this->makeRequest( [] )
+ )
+ );
+ }
+
+ /**
+ * An anonymous REST request matching a protected path pattern must be
+ * denied, and a non-matching path must not be.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\CrawlerProtectionService::checkRestPath
+ */
+ public function testAnonymousRestRequestForProtectedPathIsBlocked(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedRestPaths' => [ '/page/*/history' ],
+ ] );
+
+ $service = $this->makeWebModeService();
+ $user = $this->makeAnonUser();
+ $request = $this->makeRequest( [] );
+
+ $this->assertFalse( $service->checkRestPath( '/page/Main_Page/history', $user, $request ) );
+ $this->assertTrue( $service->checkRestPath( '/page/Main_Page/bare', $user, $request ) );
+ }
}
diff --git a/tests/phpunit/namespaced-stubs.php b/tests/phpunit/namespaced-stubs.php
index 622f8bf..072ad5c 100644
--- a/tests/phpunit/namespaced-stubs.php
+++ b/tests/phpunit/namespaced-stubs.php
@@ -207,6 +207,12 @@ class WebResponse {
*/
public function header( $string, $replace = true, $http_response_code = null ) {
}
+
+ /**
+ * @param int $code
+ */
+ public function statusHeader( $code ) {
+ }
}
}
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index f366679..b267226 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -22,6 +22,9 @@ class CrawlerProtectionServiceTest extends TestCase {
/** @var string */
private static string $webRequestClassName;
+ /** @var string */
+ private static string $webResponseClassName;
+
public static function setUpBeforeClass(): void {
parent::setUpBeforeClass();
@@ -36,6 +39,10 @@ public static function setUpBeforeClass(): void {
self::$webRequestClassName = class_exists( '\MediaWiki\Request\WebRequest' )
? '\MediaWiki\Request\WebRequest'
: '\WebRequest';
+
+ self::$webResponseClassName = class_exists( '\MediaWiki\Request\WebResponse' )
+ ? '\MediaWiki\Request\WebResponse'
+ : '\WebResponse';
}
/**
@@ -1847,8 +1854,8 @@ public function testHookCanAllowOtherwiseDeniedPerformAction() {
$responseFactory->expects( $this->never() )->method( 'denyAccess' );
$seen = [];
- $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$seen ) {
- $seen = [ $user2, $request2, $specialPageName, $shouldDeny ];
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $entryPoint, $specialPageName, $shouldDeny ];
$shouldDeny = false;
};
@@ -1857,7 +1864,7 @@ public function testHookCanAllowOtherwiseDeniedPerformAction() {
);
$this->assertTrue( $service->checkPerformAction( $output, $user, $request ) );
- $this->assertSame( [ $user, $request, null, true ], $seen );
+ $this->assertSame( [ $user, $request, 'index', null, true ], $seen );
}
/**
@@ -1873,7 +1880,7 @@ public function testHookCanDenyOtherwiseAllowedPerformAction() {
$responseFactory = $this->createMock( ResponseFactory::class );
$responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
- $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) {
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) {
$shouldDeny = true;
};
@@ -1905,8 +1912,8 @@ public function testHookRunsForAllowlistedIP() {
$responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
$seen = [];
- $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$seen ) {
- $seen = [ $user2, $request2, $specialPageName, $shouldDeny ];
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $entryPoint, $specialPageName, $shouldDeny ];
$shouldDeny = true;
};
@@ -1916,7 +1923,7 @@ public function testHookRunsForAllowlistedIP() {
);
$this->assertFalse( $service->checkPerformAction( $output, $user, $request ) );
- $this->assertSame( [ $user, $request, null, false ], $seen );
+ $this->assertSame( [ $user, $request, 'index', null, false ], $seen );
}
/**
@@ -1928,7 +1935,7 @@ public function testHookIsNotRunInCliMode() {
$request = $this->createMock( self::$webRequestClassName );
$called = false;
- $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$called ) {
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) use ( &$called ) {
$called = true;
$shouldDeny = true;
};
@@ -1958,11 +1965,13 @@ public function testHookAbortReturnValueKeepsDecision() {
$responseFactory->expects( $this->never() )->method( 'denyAccess' );
$secondCalled = false;
- $first = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) {
+ $first = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) {
$shouldDeny = false;
return false;
};
- $second = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$secondCalled ) {
+ $second = static function (
+ $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny
+ ) use ( &$secondCalled ) {
$secondCalled = true;
$shouldDeny = true;
};
@@ -1988,8 +1997,8 @@ public function testHookCanAllowOtherwiseDeniedSpecialPage() {
$responseFactory->expects( $this->never() )->method( 'denyAccess' );
$seen = [];
- $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) use ( &$seen ) {
- $seen = [ $user2, $request2, $specialPageName, $shouldDeny ];
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $entryPoint, $specialPageName, $shouldDeny ];
$shouldDeny = false;
};
@@ -2000,7 +2009,155 @@ public function testHookCanAllowOtherwiseDeniedSpecialPage() {
$this->assertTrue(
$service->checkSpecialPage( 'WhatLinksHere', $output, $user, $request )
);
- $this->assertSame( [ $user, $request, 'WhatLinksHere', true ], $seen );
+ $this->assertSame( [ $user, $request, 'index', 'WhatLinksHere', true ], $seen );
+ }
+
+ /**
+ * The hook must run for Action API requests as well, with the "api"
+ * entry point and no special page name.
+ *
+ * @covers ::checkApiModules
+ */
+ public function testHookRunsForApiRequests() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
+ $seen = [];
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $entryPoint, $specialPageName, $shouldDeny ];
+ $shouldDeny = false;
+ };
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [ 'revisions' ], [], false, [ $handler ]
+ );
+
+ $this->assertTrue( $service->checkApiModules( [ 'query', 'revisions' ], $user, $request ) );
+ $this->assertSame( [ $user, $request, 'api', null, true ], $seen );
+ }
+
+ /**
+ * The hook must run for REST requests as well, with the "rest" entry
+ * point and no special page name.
+ *
+ * @covers ::checkRestPath
+ */
+ public function testHookRunsForRestRequests() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
+ $seen = [];
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) use ( &$seen ) {
+ $seen = [ $user2, $request2, $entryPoint, $specialPageName, $shouldDeny ];
+ $shouldDeny = true;
+ };
+
+ $service = $this->buildService(
+ [], [], [], null, true, [], false, [], [ '/page/*/history' ], false, [ $handler ]
+ );
+
+ $this->assertFalse( $service->checkRestPath( '/search', $user, $request ) );
+ $this->assertSame( [ $user, $request, 'rest', null, false ], $seen );
+ }
+
+ // ---------------------------------------------------------------
+ // API and REST denial headers
+ // ---------------------------------------------------------------
+
+ /**
+ * A denied Action API request must be marked with HTTP 403, because core
+ * answers an ApiCheckCanExecute veto with "200 OK" otherwise.
+ *
+ * @covers ::checkApiModules
+ */
+ public function testDeniedApiRequestIsMarkedWith403() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $response = $this->createMock( self::$webResponseClassName );
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'response' )->willReturn( $response );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )
+ ->method( 'markDenied' )
+ ->with( $response, 403 );
+
+ $service = $this->buildService(
+ [], [], [], $responseFactory, true, [], false, [ 'revisions' ]
+ );
+
+ $this->assertFalse( $service->checkApiModules( [ 'query', 'revisions' ], $user, $request ) );
+ }
+
+ /**
+ * An allowed Action API request must not touch the response headers.
+ *
+ * @covers ::checkApiModules
+ */
+ public function testAllowedApiRequestIsNotMarked() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+ $request = $this->createMock( self::$webRequestClassName );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->never() )->method( 'markDenied' );
+
+ $service = $this->buildService(
+ [], [], [], $responseFactory, true, [], false, [ 'revisions' ]
+ );
+
+ $this->assertTrue( $service->checkApiModules( [ 'query', 'links' ], $user, $request ) );
+ }
+
+ /**
+ * A denied REST request gets its status from the LocalizedHttpException
+ * raised by the hook handler, so only the robot directive is added.
+ *
+ * @covers ::checkRestPath
+ */
+ public function testDeniedRestRequestIsMarkedWithoutStatusCode() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $response = $this->createMock( self::$webResponseClassName );
+ $request = $this->createMock( self::$webRequestClassName );
+ $request->method( 'response' )->willReturn( $response );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )
+ ->method( 'markDenied' )
+ ->with( $response );
+
+ $service = $this->buildService(
+ [], [], [], $responseFactory, true, [], false, [], [ '/page/*/history' ]
+ );
+
+ $this->assertFalse( $service->checkRestPath( '/page/Main_Page/history', $user, $request ) );
+ }
+
+ /**
+ * Without a request there is no response to mark, but the denial itself
+ * must still take effect.
+ *
+ * @covers ::checkApiModules
+ */
+ public function testDeniedApiRequestWithoutRequestMarksNothing() {
+ $user = $this->createMock( self::$userClassName );
+ $user->method( 'isRegistered' )->willReturn( false );
+
+ $responseFactory = $this->createMock( ResponseFactory::class );
+ $responseFactory->expects( $this->once() )
+ ->method( 'markDenied' )
+ ->with( null, 403 );
+
+ $service = $this->buildService(
+ [], [], [], $responseFactory, true, [], false, [ 'revisions' ]
+ );
+
+ $this->assertFalse( $service->checkApiModules( [ 'revisions' ], $user ) );
}
/**
@@ -2015,7 +2172,7 @@ public function testHookCanDenyOtherwiseAllowedSpecialPage() {
$responseFactory = $this->createMock( ResponseFactory::class );
$responseFactory->expects( $this->once() )->method( 'denyAccess' )->with( $output );
- $handler = static function ( $user2, $request2, $specialPageName, &$shouldDeny ) {
+ $handler = static function ( $user2, $request2, $entryPoint, $specialPageName, &$shouldDeny ) {
$shouldDeny = true;
};
@@ -2053,11 +2210,12 @@ public function __construct( array $handlers = [] ) {
public function onCrawlerProtectionShouldDeny(
$user,
$request,
+ string $entryPoint,
?string $specialPageName,
bool &$shouldDeny
) {
foreach ( $this->handlers as $handler ) {
- if ( $handler( $user, $request, $specialPageName, $shouldDeny ) === false ) {
+ if ( $handler( $user, $request, $entryPoint, $specialPageName, $shouldDeny ) === false ) {
return false;
}
}
diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php
index 8d3ddc6..4500356 100644
--- a/tests/phpunit/unit/ResponseFactoryTest.php
+++ b/tests/phpunit/unit/ResponseFactoryTest.php
@@ -244,6 +244,10 @@ static function ( string $header, string $message ) use ( &$capturedHeader, &$ca
// Assert
$this->assertSame( 'HTTP/1.0 403 Forbidden', $capturedHeader );
+ // The empty config value must not reach denyAccessRaw(); under the
+ // stub runner every message resolves to the same text, so the
+ // non-empty assertion is what pins the fallback there.
+ $this->assertNotSame( '', $capturedBody );
$this->assertSame(
wfMessage( 'crawlerprotection-rawdenial-text' )->inContentLanguage()->text(),
$capturedBody
@@ -290,12 +294,56 @@ static function ( string $header, string $message ) use ( &$capturedHeader, &$ca
// Assert
$this->assertSame( 'HTTP/1.0 418 I\'m a teapot', $capturedHeader );
+ $this->assertNotSame( '', $capturedBody );
$this->assertSame(
wfMessage( 'crawlerprotection-rawdenial-teapot' )->inContentLanguage()->text(),
$capturedBody
);
}
+ /**
+ * Denials that are not rendered through OutputPage (Action API and REST)
+ * must be marked noindex,nofollow, and the API path must also carry an
+ * explicit HTTP status.
+ *
+ * @covers ::markDenied
+ */
+ public function testMarkDeniedSendsStatusAndRobotsHeader() {
+ $response = $this->createMock( self::$webResponseClassName );
+ $response->expects( $this->once() )->method( 'statusHeader' )->with( 403 );
+ $response->expects( $this->once() )
+ ->method( 'header' )
+ ->with( 'X-Robots-Tag: noindex,nofollow' );
+
+ $this->buildFactory()->markDenied( $response, 403 );
+ }
+
+ /**
+ * Without a status code only the robot directive is sent, for callers
+ * whose status is already set elsewhere (REST).
+ *
+ * @covers ::markDenied
+ */
+ public function testMarkDeniedWithoutStatusCodeOnlySendsRobotsHeader() {
+ $response = $this->createMock( self::$webResponseClassName );
+ $response->expects( $this->never() )->method( 'statusHeader' );
+ $response->expects( $this->once() )
+ ->method( 'header' )
+ ->with( 'X-Robots-Tag: noindex,nofollow' );
+
+ $this->buildFactory()->markDenied( $response );
+ }
+
+ /**
+ * A null response (entry point without one) must be tolerated.
+ *
+ * @covers ::markDenied
+ */
+ public function testMarkDeniedToleratesNullResponse() {
+ $this->buildFactory()->markDenied( null, 403 );
+ $this->addToAssertionCount( 1 );
+ }
+
/**
* @covers ::__construct
*/
From 380e1ebb576bce19aedd548cb4149afcfb7f063a Mon Sep 17 00:00:00 2001
From: Jeffrey Wang
Date: Thu, 30 Jul 2026 15:03:34 -0700
Subject: [PATCH 13/16] Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---
tests/phpunit/unit/ResponseFactoryTest.php | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/phpunit/unit/ResponseFactoryTest.php b/tests/phpunit/unit/ResponseFactoryTest.php
index 4500356..362676d 100644
--- a/tests/phpunit/unit/ResponseFactoryTest.php
+++ b/tests/phpunit/unit/ResponseFactoryTest.php
@@ -387,10 +387,10 @@ public function testDenyAccessPrettyFallsBackToSetPageTitleOnLegacyOutputPage()
// Anonymous class without setPageTitleMsg() simulates MW < 1.41 OutputPage.
$output = new class ( $setPageTitleCallCount ) {
/** @var int */
- private int $count;
+ private $count;
- public function __construct( int &$count ) {
- $this->count = &$count;
+ public function __construct( &$count ) {
+ $this->count =& $count;
}
public function setStatusCode( int $code ): void {
From 238efeee527821868e775f54dcab0ab31aa3e387 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 30 Jul 2026 22:13:06 +0000
Subject: [PATCH 14/16] Address cicalese follow-up: signature test names, real
ApiMain coverage, REST path docs
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
...lerprotection-architecture.instructions.md | 6 +
README.md | 13 +-
includes/CrawlerProtectionService.php | 5 +
.../CrawlerProtectionIntegrationTest.php | 147 +++++++++++++++++-
.../unit/CrawlerProtectionServiceTest.php | 22 ++-
tests/phpunit/unit/HooksTest.php | 4 +-
6 files changed, 181 insertions(+), 16 deletions(-)
diff --git a/.github/instructions/crawlerprotection-architecture.instructions.md b/.github/instructions/crawlerprotection-architecture.instructions.md
index 269aa91..fa83f7a 100644
--- a/.github/instructions/crawlerprotection-architecture.instructions.md
+++ b/.github/instructions/crawlerprotection-architecture.instructions.md
@@ -11,6 +11,12 @@ applyTo: "**/*.php"
- `SpecialPageBeforeExecute` (index.php special pages)
- `ApiCheckCanExecute` (api.php modules)
- `RestCheckCanExecute` (rest.php paths, MediaWiki 1.44+)
+- The extension also fires its own `CrawlerProtectionShouldDeny` hook
+ (`includes/Hook/CrawlerProtectionShouldDenyHook.php`, dispatched via
+ `includes/HookRunner.php`) from every entry point above. Handlers receive the
+ entry point and may flip the decision either way, so they can deny requests
+ that the built-in checks would allow - including requests from registered
+ users - or allow requests the built-in checks would deny.
- `includes/Hooks.php` must remain a thin adapter layer delegating to services.
- Business logic belongs in `CrawlerProtectionService` and response rendering in `ResponseFactory`.
- Instantiate services only in `includes/ServiceWiring.php`.
diff --git a/README.md b/README.md
index 17877f0..03d7e3e 100644
--- a/README.md
+++ b/README.md
@@ -65,8 +65,17 @@ addresses in `$wgCrawlerProtectionAllowedIPs` are always permitted.
* `$wgCrawlerProtectedRestPaths` - array of REST API path glob patterns to
block for anonymous users (default: `[]`). Each pattern is tested with
`fnmatch()` with the `FNM_PATHNAME` flag, so `*` matches any single path
- component (it never spans a `/`) and `**` is not supported. Example that
- protects history and compare endpoints:
+ component (it never spans a `/`) and `**` is not supported.
+
+ **Patterns are module-relative: leave out the module prefix.** MediaWiki's
+ REST router strips the `rest.php` root *and* the module prefix before the
+ path reaches the extension, so `GET /w/rest.php/v1/page/Main_Page/history`
+ is matched as `/page/Main_Page/history`. Writing `/v1/page/*/history` - the
+ form that appears in an access log - therefore never matches, and no warning
+ is emitted. The upside is that one pattern covers every module version:
+ `/page/*/history` protects `/v1`, `/coredev/v0` and any future prefix alike.
+
+ Example that protects history and compare endpoints:
```php
$wgCrawlerProtectedRestPaths = [ '/page/*/history', '/revision/*/compare/*' ];
```
diff --git a/includes/CrawlerProtectionService.php b/includes/CrawlerProtectionService.php
index 07636e7..0faf6ca 100644
--- a/includes/CrawlerProtectionService.php
+++ b/includes/CrawlerProtectionService.php
@@ -420,6 +420,11 @@ public function checkRestPath( string $path, $user, $request = null ): bool {
* spans a "/" separator. See $wgCrawlerProtectedRestPaths in the
* README for example patterns.
*
+ * The path passed by the RestCheckCanExecute hook is module-relative:
+ * core's Router strips the rest.php root and the module prefix, so
+ * "/w/rest.php/v1/page/Main_Page/history" arrives as
+ * "/page/Main_Page/history" and patterns must omit the prefix.
+ *
* @param string $path
* @return bool
*/
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
index 69ed5c2..e09180f 100644
--- a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -279,6 +279,14 @@ public function testHandlerSignatureMatchesCoreInterface( string $interface, str
$actual->getParameters()[$index]->isPassedByReference(),
"Parameter #$index of $method must match the by-reference mode of $interface"
);
+ // The handlers are deliberately untyped, so a reordering of
+ // same-arity parameters in core would otherwise go unnoticed:
+ // the parameter names are what pins the order.
+ $this->assertSame(
+ $parameter->getName(),
+ $actual->getParameters()[$index]->getName(),
+ "Parameter #$index of $method must have the same name as in $interface"
+ );
}
}
@@ -291,10 +299,6 @@ public static function provideCoreHookInterfaces(): array {
'MediaWiki\\Api\\Hook\\ApiCheckCanExecuteHook',
'onApiCheckCanExecute',
],
- 'ApiCheckCanExecute (legacy namespace)' => [
- 'MediaWiki\\Hook\\ApiCheckCanExecuteHook',
- 'onApiCheckCanExecute',
- ],
'RestCheckCanExecute' => [
'MediaWiki\\Rest\\Hook\\RestCheckCanExecuteHook',
'onRestCheckCanExecute',
@@ -605,4 +609,139 @@ public function testAnonymousRestRequestForProtectedPathIsBlocked(): void {
$this->assertFalse( $service->checkRestPath( '/page/Main_Page/history', $user, $request ) );
$this->assertTrue( $service->checkRestPath( '/page/Main_Page/bare', $user, $request ) );
}
+
+ // ---------------------------------------------------------------
+ // Hooks::onApiCheckCanExecute through a real ApiMain
+ // ---------------------------------------------------------------
+
+ /**
+ * Build a real ApiMain around a FauxRequest carrying the given parameters.
+ *
+ * A FauxRequest puts ApiMain into internal mode, where execute() calls
+ * executeAction() directly: checkExecutePermissions() - and therefore the
+ * ApiCheckCanExecute hook - still runs, but an ApiUsageException
+ * propagates to the caller instead of being formatted into a response
+ * body. ApiMain moved into the MediaWiki\Api namespace after MW 1.39,
+ * which keeps a class alias under the old global name on every supported
+ * release, so the unqualified name is used here.
+ *
+ * @param array $params Request parameters
+ * @param \MediaWiki\User\User|\User $user User making the request
+ * @return \ApiMain
+ */
+ private function makeApiMain( array $params, $user ) {
+ $context = new \RequestContext();
+ $context->setRequest( $this->makeRequest( $params ) );
+ $context->setUser( $user );
+ $context->setTitle(
+ $this->getServiceContainer()->getTitleFactory()->makeTitle( NS_MAIN, 'Test' )
+ );
+
+ return new \ApiMain( $context );
+ }
+
+ /**
+ * Install a web-mode CrawlerProtectionService in the container so that the
+ * hook handler built by ObjectFactory uses it instead of the CLI-mode
+ * service that ServiceWiring.php produces under PHPUnit.
+ */
+ private function useWebModeServiceInContainer(): void {
+ $this->setService(
+ 'CrawlerProtection.CrawlerProtectionService',
+ $this->makeWebModeService()
+ );
+ }
+
+ /**
+ * The hook handler must deny an anonymous action=query request naming a
+ * protected sub-module, and the denial must carry HTTP 403 rather than
+ * core's default "200 OK with an error body".
+ *
+ * This exercises the adapter itself - sub-module parsing, the $message
+ * contract with core, and the status code - which the service-level tests
+ * above cannot reach.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onApiCheckCanExecute
+ */
+ public function testApiCheckCanExecuteDeniesAnonymousProtectedSubModule(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedApiModules' => [ 'revisions' ],
+ ] );
+ $this->useWebModeServiceInContainer();
+
+ $api = $this->makeApiMain(
+ [ 'action' => 'query', 'prop' => 'revisions', 'titles' => 'Main Page' ],
+ $this->makeAnonUser()
+ );
+
+ try {
+ $api->execute();
+ $this->fail( 'An anonymous request for a protected sub-module must be denied' );
+ } catch ( \ApiUsageException $e ) {
+ $this->assertTrue(
+ $e->getStatusValue()->hasMessage( 'crawlerprotection-accessdenied-text' ),
+ 'The denial must use the extension error message'
+ );
+ }
+
+ $response = $api->getRequest()->response();
+ $this->assertSame( 403, $response->getStatusCode() );
+ $this->assertSame(
+ 'noindex,nofollow',
+ $response->getHeader( 'X-Robots-Tag' ),
+ 'A denied API request must tell crawlers not to re-request the URL'
+ );
+ }
+
+ /**
+ * The hook handler must let an anonymous request through when no
+ * configured module is involved.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onApiCheckCanExecute
+ */
+ public function testApiCheckCanExecuteAllowsUnprotectedModule(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedApiModules' => [ 'revisions' ],
+ ] );
+ $this->useWebModeServiceInContainer();
+
+ $api = $this->makeApiMain(
+ [ 'action' => 'query', 'meta' => 'siteinfo' ],
+ $this->makeAnonUser()
+ );
+
+ $api->execute();
+
+ $this->assertArrayHasKey(
+ 'query',
+ $api->getResult()->getResultData( [], [ 'Strip' => 'all' ] ),
+ 'An unprotected module must execute normally'
+ );
+ }
+
+ /**
+ * A registered user must not be denied by the hook handler even when the
+ * module is protected.
+ *
+ * @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onApiCheckCanExecute
+ */
+ public function testApiCheckCanExecuteAllowsRegisteredUserOnProtectedModule(): void {
+ $this->overrideCrawlerProtectionConfig( [
+ 'CrawlerProtectedApiModules' => [ 'siteinfo' ],
+ ] );
+ $this->useWebModeServiceInContainer();
+
+ $api = $this->makeApiMain(
+ [ 'action' => 'query', 'meta' => 'siteinfo' ],
+ $this->getMutableTestUser()->getUser()
+ );
+
+ $api->execute();
+
+ $this->assertArrayHasKey(
+ 'query',
+ $api->getResult()->getResultData( [], [ 'Strip' => 'all' ] ),
+ 'A registered user must not be denied'
+ );
+ }
}
diff --git a/tests/phpunit/unit/CrawlerProtectionServiceTest.php b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
index b267226..6c74af5 100644
--- a/tests/phpunit/unit/CrawlerProtectionServiceTest.php
+++ b/tests/phpunit/unit/CrawlerProtectionServiceTest.php
@@ -1013,8 +1013,15 @@ public function provideAllowedIPs(): array {
// ---------------------------------------------------------------
/**
- * Verify that the IP used for allowlist matching comes from $request->getIP()
- * and not from the (now irrelevant) username.
+ * Verify that the IP used for allowlist matching is read from
+ * $request->getIP() rather than from User::getName().
+ *
+ * For a real anonymous user the two agree — User::getName() returns
+ * IPUtils::sanitizeIP( $this->getRequest()->getIP() ) — so this is a
+ * mechanism check, not a regression test for a past behavioural bug:
+ * it pins the request as the single source of the client address, which
+ * is what makes the same resolution reusable on api.php and rest.php,
+ * where no User-derived address is available.
*
* @covers ::checkPerformAction
*/
@@ -1022,8 +1029,7 @@ public function testCheckPerformActionUsesRequestIPNotUsername() {
$output = $this->createMock( self::$outputPageClassName );
$user = $this->createMock( self::$userClassName );
$user->method( 'isRegistered' )->willReturn( false );
- // Username does NOT match the allowlist; request IP DOES.
- // getName() must never be called for IP resolution.
+ // getName() must never be consulted for IP resolution.
$user->expects( $this->never() )->method( 'getName' );
$request = $this->createMock( self::$webRequestClassName );
@@ -1653,8 +1659,8 @@ public function testCheckApiModuleAllowsAllowedIP() {
/**
* Without a request the allowlist cannot be evaluated, so a protected
- * module stays protected rather than being allowed by a username that
- * happens to look like an allowlisted IP.
+ * module stays protected: the extension never falls back to any other
+ * source for the client address.
*
* @covers ::checkApiModule
*/
@@ -1817,8 +1823,8 @@ public function testCheckRestPathAllowsAllowedIP() {
/**
* Without a request the allowlist cannot be evaluated, so a protected
- * path stays protected rather than being allowed by a username that
- * happens to look like an allowlisted IP.
+ * path stays protected: the extension never falls back to any other
+ * source for the client address.
*
* @covers ::checkRestPath
*/
diff --git a/tests/phpunit/unit/HooksTest.php b/tests/phpunit/unit/HooksTest.php
index 88768aa..501e3b3 100644
--- a/tests/phpunit/unit/HooksTest.php
+++ b/tests/phpunit/unit/HooksTest.php
@@ -301,7 +301,7 @@ public function testOnApiCheckCanExecutePassesThroughWhenAllowed() {
$service = $this->createMock( CrawlerProtectionService::class );
$service->expects( $this->once() )
->method( 'checkApiModules' )
- ->with( [ 'query' ], $user )
+ ->with( [ 'query' ], $user, $module->getMain()->getRequest() )
->willReturn( true );
$hooks = new Hooks( $service );
@@ -454,7 +454,7 @@ public function testOnRestCheckCanExecutePassesThroughWhenAllowed() {
$service = $this->createMock( CrawlerProtectionService::class );
$service->expects( $this->once() )
->method( 'checkRestPath' )
- ->with( '/search', $user )
+ ->with( '/search', $user, $this->isInstanceOf( self::$webRequestClassName ) )
->willReturn( true );
$hooks = new Hooks( $service );
From 809e7f40816f971c3b21e935039a355823658de7 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Thu, 30 Jul 2026 22:17:34 +0000
Subject: [PATCH 15/16] Make ApiMain integration assertions version-agnostic
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
.../CrawlerProtectionIntegrationTest.php | 22 ++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
index e09180f..b308160 100644
--- a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -674,20 +674,36 @@ public function testApiCheckCanExecuteDeniesAnonymousProtectedSubModule(): void
$this->makeAnonUser()
);
+ $denied = false;
try {
$api->execute();
- $this->fail( 'An anonymous request for a protected sub-module must be denied' );
- } catch ( \ApiUsageException $e ) {
+ } catch ( \Exception $e ) {
+ // ApiUsageException moved into the MediaWiki\Api namespace after
+ // MW 1.39, so it is recognised by the method it exposes rather
+ // than by class name.
$this->assertTrue(
+ method_exists( $e, 'getStatusValue' ),
+ 'Expected an ApiUsageException, got ' . get_class( $e ) . ': ' . $e->getMessage()
+ );
+ $denied = true;
+ $this->assertTrue(
+ // @phan-suppress-next-line PhanUndeclaredMethod ApiUsageException only
$e->getStatusValue()->hasMessage( 'crawlerprotection-accessdenied-text' ),
'The denial must use the extension error message'
);
}
+ $this->assertTrue( $denied, 'An anonymous request for a protected sub-module must be denied' );
+
$response = $api->getRequest()->response();
- $this->assertSame( 403, $response->getStatusCode() );
+ $this->assertSame(
+ 403,
+ // @phan-suppress-next-line PhanUndeclaredMethod FauxResponse only
+ $response->getStatusCode()
+ );
$this->assertSame(
'noindex,nofollow',
+ // @phan-suppress-next-line PhanUndeclaredMethod FauxResponse only
$response->getHeader( 'X-Robots-Tag' ),
'A denied API request must tell crawlers not to re-request the URL'
);
From 8bd6c1b299420ed7d9725db4864ef0d8cd631167 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Fri, 31 Jul 2026 00:13:25 +0000
Subject: [PATCH 16/16] Soften ApiMain integration test status assertion
comments
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
---
.../CrawlerProtectionIntegrationTest.php | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
index b308160..b10270c 100644
--- a/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
+++ b/tests/phpunit/integration/CrawlerProtectionIntegrationTest.php
@@ -654,12 +654,14 @@ private function useWebModeServiceInContainer(): void {
/**
* The hook handler must deny an anonymous action=query request naming a
- * protected sub-module, and the denial must carry HTTP 403 rather than
- * core's default "200 OK with an error body".
+ * protected sub-module, and must itself set HTTP 403 on the response
+ * rather than leaving core's default "200 OK with an error body".
*
- * This exercises the adapter itself - sub-module parsing, the $message
- * contract with core, and the status code - which the service-level tests
- * above cannot reach.
+ * This exercises the adapter itself - sub-module parsing and the $message
+ * contract with core - which the service-level tests above cannot reach.
+ * The status and header assertions pin what the extension sets; whether
+ * core preserves them while printing the error response is out of scope
+ * here, since internal mode does not reach the printer.
*
* @covers \MediaWiki\Extension\CrawlerProtection\Hooks::onApiCheckCanExecute
*/
@@ -699,13 +701,14 @@ public function testApiCheckCanExecuteDeniesAnonymousProtectedSubModule(): void
$this->assertSame(
403,
// @phan-suppress-next-line PhanUndeclaredMethod FauxResponse only
- $response->getStatusCode()
+ $response->getStatusCode(),
+ 'The extension must set HTTP 403 on a denied API request'
);
$this->assertSame(
'noindex,nofollow',
// @phan-suppress-next-line PhanUndeclaredMethod FauxResponse only
$response->getHeader( 'X-Robots-Tag' ),
- 'A denied API request must tell crawlers not to re-request the URL'
+ 'The extension must tell crawlers not to re-request a denied URL'
);
}