Add configurable action protection via $wgCrawlerProtectedActions#14
Closed
Add configurable action protection via $wgCrawlerProtectedActions#14
Conversation
Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add configurable setting for anonymous action history
Add configurable action protection via $wgCrawlerProtectedActions
Feb 19, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new extension configuration option to make which action= requests are blocked for anonymous users configurable (previously hardcoded to action=history).
Changes:
- Introduces
CrawlerProtectedActionsconfig (default["history"]) to preserve existing behavior. - Updates the
MediaWikiPerformActionhook to consult config and block configured actions for anonymous users. - Documents the new option and adds/updates unit tests and test stubs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
extension.json |
Adds CrawlerProtectedActions config with default ["history"]. |
includes/Hooks.php |
Reads CrawlerProtectedActions from MainConfig and uses it to decide whether to deny anonymous requests. |
README.md |
Documents $wgCrawlerProtectedActions usage. |
tests/phpunit/unit/HooksTest.php |
Adds tests for action=history behavior. |
tests/phpunit/namespaced-stubs.php |
Extends the stub config to return a default CrawlerProtectedActions value. |
tests/phpunit/unit/HooksTest.php
Outdated
|
|
||
| $request = $this->createMock( self::$webRequestClassName ); | ||
| $request->method( 'getVal' )->willReturnMap( [ | ||
| [ 'action', null, 'history' ], |
There was a problem hiding this comment.
Same issue as above: the getVal() mock map includes a null default parameter, but the code under test calls getVal( 'action' ) with one argument. As written, this will likely return null and not test the intended behavior.
Suggested change
| [ 'action', null, 'history' ], | |
| [ 'action', 'history' ], |
Comment on lines
+167
to
+173
| // Skip this test in MediaWiki environment - it requires service container | ||
| if ( !property_exists( '\MediaWiki\MediaWikiServices', 'testUse418' ) ) { | ||
| $this->markTestSkipped( | ||
| 'Test requires stub MediaWikiServices. Skipped in MediaWiki unit test environment.' | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
These history-action tests are skipped whenever the real MediaWikiServices is present, but Hooks::onMediaWikiPerformAction() now always uses MediaWikiServices::getInstance()->getMainConfig(), and the other onMediaWikiPerformAction tests in this file are not skipped. This skip will unnecessarily drop coverage in a real MediaWiki test run; consider removing it (or only skipping when CrawlerProtectedActions cannot be read).
Suggested change
| // Skip this test in MediaWiki environment - it requires service container | |
| if ( !property_exists( '\MediaWiki\MediaWikiServices', 'testUse418' ) ) { | |
| $this->markTestSkipped( | |
| 'Test requires stub MediaWikiServices. Skipped in MediaWiki unit test environment.' | |
| ); | |
| } |
Comment on lines
+133
to
+135
| if ( $name === 'CrawlerProtectedActions' ) { | ||
| return [ 'history' ]; | ||
| } |
There was a problem hiding this comment.
namespaced-stubs.php hardcodes CrawlerProtectedActions to [ 'history' ], which makes it impossible to unit-test the new configurability (e.g., verifying that an empty array allows action=history, or that another action can be protected). Consider making this stub value controllable (e.g., via a static property) so tests can cover non-default configurations.
Author
There was a problem hiding this comment.
The feedback has already been addressed in commit 91c86bc. The changes include:
- Added
MediaWikiServices::$testProtectedActionsstatic property to make stub configuration controllable - Fixed mocks to use
willReturnCallbackfor proper parameter matching - Removed unnecessary test skips
- Added 3 new tests covering empty array and custom action configurations
- Updated README wording to clarify empty array behavior
All 26 tests pass ✅
README.md
Outdated
|
|
||
| * `$wgCrawlerProtectedActions` - array of actions to protect (default: `[ 'history' ]`). | ||
| Actions specified in this array will be denied for anonymous users. | ||
| Set to an empty array `[]` to allow all actions for anonymous users. |
There was a problem hiding this comment.
The README says setting $wgCrawlerProtectedActions = [] will "allow all actions for anonymous users", but anonymous users will still be blocked by the other checks in onMediaWikiPerformAction (type=revision, diff, oldid). Consider rewording to clarify that this only disables the action=-based restriction.
Suggested change
| Set to an empty array `[]` to allow all actions for anonymous users. | |
| Set to an empty array `[]` to disable `action=`-based restrictions for anonymous users (other checks | |
| such as `type=revision`, `diff`, or `oldid` may still block requests). |
Comment on lines
+73
to
+80
| $config = MediaWikiServices::getInstance()->getMainConfig(); | ||
| $protectedActions = $config->get( 'CrawlerProtectedActions' ); | ||
|
|
||
| if ( | ||
| !$user->isRegistered() | ||
| && ( | ||
| $type === 'revision' | ||
| || $action === 'history' | ||
| || in_array( $action, $protectedActions, true ) |
There was a problem hiding this comment.
CrawlerProtectedActions is now configurable, but there are no tests covering non-default configuration values (e.g. empty array allowing action=history, or protecting an additional action). Adding such cases would ensure the new config wiring is actually exercised and remains stable across MediaWiki versions.
Author
There was a problem hiding this comment.
Applied all feedback changes in commit 91c86bc:
- Fixed
getVal()mocks to usewillReturnCallbackfor proper single-argument matching - Removed unnecessary test skips from history action tests (they now work in both environments)
- Made stub config controllable via
MediaWikiServices::$testProtectedActionsstatic property - Added 3 new tests for non-default configurations:
testEmptyProtectedActionsAllowsHistory- verifies empty array allows historytestCustomProtectedActionBlocks- verifies custom actions are blockedtestCustomProtectedActionAllowsOtherActions- verifies non-configured actions are allowed
- Clarified README wording that empty array only disables action-based restrictions
All 26 tests pass and code follows MediaWiki standards.
tests/phpunit/unit/HooksTest.php
Outdated
|
|
||
| $request = $this->createMock( self::$webRequestClassName ); | ||
| $request->method( 'getVal' )->willReturnMap( [ | ||
| [ 'action', null, 'history' ], |
There was a problem hiding this comment.
The getVal() mocks use willReturnMap entries with two parameters ([ 'action', null, 'history' ]), but Hooks::onMediaWikiPerformAction() calls $request->getVal( 'action' ) with a single argument. That means the map entry won’t match and the mock will return null, so this test won’t actually exercise the history-action branch. Update the mock to match the one-arg call (or provide map entries for both 1-arg and 2-arg invocations).
Suggested change
| [ 'action', null, 'history' ], | |
| [ 'action', 'history' ], |
… docs Co-authored-by: jeffw16 <11380894+jeffw16@users.noreply.github.com>
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementation Plan for $wgCrawlerProtectedActions
$wgCrawlerProtectedActionsconfiguration variable toextension.jsonwith default value["history"]includes/Hooks.phpto use the new configuration instead of hardcoded$action === 'history'checktests/phpunit/namespaced-stubs.phpto support the new configurationREADME.mdto document the new configuration optionOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.