Add custom header and text settings for access-denial display#16
Add custom header and text settings for access-denial display#16yaronkoren wants to merge 7 commits intomywikis:mainfrom
Conversation
|
@jeffw16 - what do you think? |
|
@yaronkoren I think this is a good compromise, but I'd still like to see the use 418 code be kept for backwards compatibility. It can be deprecated for now and removed in a later version. |
|
Alright, done. |
There was a problem hiding this comment.
Pull request overview
This PR updates the extension’s “fast deny” behavior for protected special pages by moving from a hard-coded 418 teapot response to configurable denial header/text values, with a compatibility fallback to the existing 418 option.
Changes:
- Add new configuration keys for a custom denial header and denial text.
- Update the SpecialPage hook to use the custom denial response (or derive it from the legacy 418 setting).
- Replace the
denyAccessWith418()helper with a newdenyAccessCustom()helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
includes/Hooks.php |
Reads new config values and uses a new fast-deny helper to short-circuit expensive special-page execution. |
extension.json |
Adds CrawlerProtectionDenialHeader / CrawlerProtectionDenialText config defaults alongside the legacy CrawlerProtectionUse418. |
| protected function denyAccessCustom( $customDenialHeader, $customDenialText ) { | ||
| header( htmlspecialchars( $customDenialHeader ) ); | ||
| die( htmlspecialchars( $customDenialText ) ); |
There was a problem hiding this comment.
denyAccessCustom() uses header( htmlspecialchars( ... ) ). htmlspecialchars() is for HTML output, not HTTP headers; it will corrupt valid status/header lines (e.g. turning quotes/ampersands into entities) and still doesn’t prevent response-splitting via CR/LF. Use MediaWiki’s response API (WebResponse/status handling) and validate/sanitize the header value specifically for HTTP headers (e.g. reject/strip \r/\n and restrict to a status line or allowlisted header names).
| if ( $customDenialHeader !== null && $customDenialText !== null ) { | ||
| $this->denyAccessCustom( $customDenialHeader, $customDenialText ); | ||
| } |
There was a problem hiding this comment.
The fast-deny path was renamed from denyAccessWith418() to denyAccessCustom(), but the PHPUnit suite still references/mocks denyAccessWith418 (e.g. HooksTest covers and expects that method). This will break tests/CI; update the unit tests (and any stubs) to use the new method and add coverage for the new config combinations.
| if ( $customDenialHeader !== null && $customDenialText !== null ) { | ||
| $this->denyAccessCustom( $customDenialHeader, $customDenialText ); | ||
| } |
There was a problem hiding this comment.
The custom-denial branch only triggers when both CrawlerProtectionDenialHeader and CrawlerProtectionDenialText are non-null. If an admin sets only one of the two, the code silently falls back to the expensive denyAccess() path (and the use418 fallback won’t fill the missing value). Consider supporting partial configuration by providing defaults (e.g. default status header and/or default text) or treating either value being set as enabling the fast-deny path.
| "CrawlerProtectionUse418": { | ||
| "value": false | ||
| }, | ||
| "CrawlerProtectionDenialHeader": { | ||
| "value": null | ||
| }, | ||
| "CrawlerProtectionDenialText": { | ||
| "value": null |
There was a problem hiding this comment.
PR title/description says the "use 418" option is being replaced, but CrawlerProtectionUse418 remains in extension.json and is still read/used as a fallback in Hooks. If this option is intentionally kept for backwards compatibility, it should be documented as deprecated (and docs/tests updated accordingly); otherwise remove it to match the PR intent.
yaronkoren
changed the title
This allows for much greater freedom in choosing the error code, the language of the message, and even the visual look.