RC-scoped 3.6.4 fixes for PHP 8, dblayertrap, and filter loading#28
Conversation
…3.6.4
Several low-blast-radius corrections for the XOOPS 2.7.0 RC lane.
Each change is small, independently revertable, and scoped to bug
and compatibility fixes — no new detection features, config
options, schema changes, or dependencies.
- PHP 8 compatibility fix in cookie cleanup path
- More defensive handling of an admin-configurable pattern setting
- Tightened response path for blocked queries
- Tightened filter-file loading to require explicit extensions
and resolved-path containment under the enabled-filters directory
- Refined request-metadata scanning to limit inputs to
server-process-intrinsic values
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughProtector module tightened file-loader validation with canonical realpath containment and stricter filename checks; altered SQL-injection termination to send HTTP 403 and generic body; adjusted PCRE delimiter and fixed constant quoting; limited server scanning to an allowlist and guarded admin regex usage; added PHPUnit regression tests covering all fixes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #28 +/- ##
============================================
- Coverage 19.23% 18.00% -1.24%
- Complexity 7584 7803 +219
============================================
Files 621 665 +44
Lines 40085 42827 +2742
============================================
Hits 7709 7709
- Misses 32376 35118 +2742 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/xoops_lib/modules/protector/class/protector.php`:
- Around line 144-149: The preg_match call in protector::_doubtful_requests
population used an invalid '?' delimiter causing preg_match to fail and silently
disable downstream checks; update the pattern used in preg_match (the call in
protector.php where preg_match is invoked) to use a valid delimiter (e.g., '#'
as in the diff) so that suspect values are correctly detected and stored in
$this->_doubtful_requests[(string)$key] thereby re-enabling check_sql_union(),
check_sql_isolatedcommentin(), and related replace_doubtful() logic to run on
real input.
In `@htdocs/xoops_lib/modules/protector/class/ProtectorFilter.php`:
- Around line 103-126: The code currently calls $baseReal =
realpath($this->filters_base) once but still iterates all readdir entries even
if $baseReal === false; hoist a guard immediately after computing $baseReal
(before the while loop that reads $dh) to handle the failure case (close $dh if
opened and return/exit the loader method or otherwise abort processing) so you
avoid repeating doomed checks inside the loop and fail fast; locate the realpath
call and the while (($file = readdir($dh)) ...) loop in ProtectorFilter.php and
add the early-return/cleanup when $baseReal === false.
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php`:
- Around line 43-55: The helper readBetween currently returns an empty string
when start or end delimiters are missing, making downstream assertions vacuously
true; modify readBetween(string $haystack, string $start, string $end) to fail
loudly by throwing an InvalidArgumentException (or returning null and the caller
asserting non-null) when either delimiter is not found or when the extracted
substring is empty, and update the tests that call readBetween (the allowlist
extraction sites) to expect a non-empty result before asserting
contains/not-contains so the allowlist checks are actually validated.
- Around line 266-339: The test risks leaking global state:
Protector::getInstance() is a singleton (Protector::getInstance) that can
capture superglobals at construction and the include_once that defines
protector_precommon_mixcase() pollutes the global namespace for all tests; fix
by isolating these effects: ensure tests that declare
protector_precommon_mixcase() run in a separate PHP process (add PHPUnit
`@runInSeparateProcess` to the test method or class) and reset the Protector
singleton after the test by nulling its cached instance via reflection (set the
private/static instance property to null) or by adding/using a test-only
Protector::resetInstance() helper, and keep the existing finally block to clear
_dblayertrap_doubtfuls and restore superglobals; reference
Protector::getInstance, dblayertrap_init, getDblayertrapDoubtfuls,
_dblayertrap_doubtfuls, and protector_precommon_mixcase.
- Around line 7-9: The test suite is silent under PHPUnit 9.6 because it uses
the PHPUnit 10+ attribute import (use PHPUnit\Framework\Attributes\Test) and
test methods that don't start with "test" (fix11_…, fix12_…, fix14_…, fix15_…,
fix91_…, smokeTest_…, plus the other listed methods), so update the suite to be
discovery-compatible: either add a /** `@test` */ docblock to each of those
methods or (preferable) rename each method to begin with the "test" prefix (e.g.
fix11_Xxx -> testFix11_Xxx) so PHPUnit 9.6 and 10+ both detect them; also remove
or keep the PHPUnit attribute import consistently (remove use
PHPUnit\Framework\Attributes\Test if you rely on docblocks/renames) and ensure
all 10 affected methods are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53800177-d34b-429c-aa09-063a7689c900
📒 Files selected for processing (4)
htdocs/xoops_lib/modules/protector/class/ProtectorFilter.phphtdocs/xoops_lib/modules/protector/class/ProtectorMysqlDatabase.class.phphtdocs/xoops_lib/modules/protector/class/protector.phptests/unit/htdocs/modules/protector/Protector364RcFixesTest.php
| use PHPUnit\Framework\Attributes\CoversNothing; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does PHPUnit 9.6 support PHP attributes like #[Test] from PHPUnit\Framework\Attributes\Test?
💡 Result:
No, PHPUnit 9.6 does not support PHP attributes like #[Test] from PHPUnit\Framework\Attributes\Test. PHP attributes support was introduced in PHPUnit 10.
Citations:
- 1: https://docs.phpunit.de/en/11.0/attributes.html
- 2: Support PHP 8 attributes for adding metadata to test classes and test methods as well as tested code units sebastianbergmann/phpunit#4502
- 3: https://docs.phpunit.de/en/9.6/
- 4: https://docs.phpunit.de/en/9.6/index.html
- 5: https://docs.phpunit.de/en/9.6/annotations.html
🏁 Script executed:
cat -n tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php | head -120Repository: XOOPS/XoopsCore27
Length of output: 5871
🏁 Script executed:
cat -n tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php | sed -n '61p;84p;106p;127p;153p;182p;250p;345p;372p;427p'Repository: XOOPS/XoopsCore27
Length of output: 250
🏁 Script executed:
cat -n tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php | sed -n '61,63p;84,86p;106,108p;127,129p;153,155p;182,184p;250,252p;345,347p;372,374p;427,429p'Repository: XOOPS/XoopsCore27
Length of output: 1218
Fix PHPUnit 9.6 incompatibility — all tests in this suite silently skip on 9.6.
Two combined issues violate the multi-version support requirement:
use PHPUnit\Framework\Attributes\Test;with#[Test]annotations are PHPUnit 10+ only. PHPUnit 9.6 ignores attributes entirely.- Method names (
fix11_…,fix12_…,fix14_…,fix15_…,fix91_…,smokeTest_…) do not start with thetestprefix. PHPUnit 9.6 auto-discovery requires eithertestprefix OR@testdocblock annotation.
Net effect: zero tests from this suite run under PHPUnit 9.6, allowing regressions to slip through while CI reports green. This also violates the naming guideline: "Name test methods test{MethodName} or test{MethodName}{Scenario}".
Fix: rename all test methods to use test prefix
Rename each method to start with test:
- #[Test]
- public function fix11_doubtfulRequestRegexUsesValidDelimiter(): void
+ #[Test]
+ public function testFix11DoubtfulRequestRegexUsesValidDelimiter(): voidApply to all 10 methods. Alternatively, add /** @test */ docblock to each method — PHPUnit 9.6 honors docblocks, 10+ honors attributes.
Affects lines 62, 85, 107, 128, 154, 183, 251, 346, 373, 428.
Per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methods test{MethodName} or test{MethodName}{Scenario}".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php` around lines
7 - 9, The test suite is silent under PHPUnit 9.6 because it uses the PHPUnit
10+ attribute import (use PHPUnit\Framework\Attributes\Test) and test methods
that don't start with "test" (fix11_…, fix12_…, fix14_…, fix15_…, fix91_…,
smokeTest_…, plus the other listed methods), so update the suite to be
discovery-compatible: either add a /** `@test` */ docblock to each of those
methods or (preferable) rename each method to begin with the "test" prefix (e.g.
fix11_Xxx -> testFix11_Xxx) so PHPUnit 9.6 and 10+ both detect them; also remove
or keep the PHPUnit attribute import consistently (remove use
PHPUnit\Framework\Attributes\Test if you rely on docblocks/renames) and ensure
all 10 affected methods are updated accordingly.
There was a problem hiding this comment.
Pull request overview
This PR delivers a set of small, RC-scoped stability/compatibility tightenings for the Protector module in XOOPS 2.7.0 RC, focusing on safer request handling, safer filter loading, and PHP 8 compatibility.
Changes:
- Fixes several Protector runtime paths (cookie purge, dblayertrap server scanning, DoS regex handling) to be more defensive under PHP 8+ and less prone to false positives.
- Tightens blocked-query response behavior to return HTTP 403 with a generic body.
- Hardens filter loading by requiring a
.phpsuffix and enforcingrealpath()containment within the enabled-filters directory, plus adds regression tests covering the tranche.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php |
Adds regression tests (source-level + a couple behavioral) to pin the RC fix tranche behavior. |
htdocs/xoops_lib/modules/protector/class/protector.php |
Adjusts doubtful-request scanning regex usage, fixes PHP 8 defined() usage, validates admin regex before applying, and restricts dblayertrap’s $_SERVER scan to an allowlist. |
htdocs/xoops_lib/modules/protector/class/ProtectorMysqlDatabase.class.php |
Changes SQL injection termination response to HTTP 403 + generic body. |
htdocs/xoops_lib/modules/protector/class/ProtectorFilter.php |
Hardens filter loading: require .php, use realpath(), and enforce base-directory containment. |
| // The broken "?...?" delimiter must not survive anywhere in the file. | ||
| $this->assertStringNotContainsString( | ||
| "preg_match('?[", | ||
| $source, | ||
| '"?" is not a valid PCRE delimiter — preg_match() returned false on every call' |
There was a problem hiding this comment.
This test’s failure message states that '"?" is not a valid PCRE delimiter' and that preg_match() returned false on every call. In PHP, '?' is an allowed regex delimiter; if the intent is to pin the delimiter change to '#', the wording should be updated so the test documents the actual regression/fix accurately (e.g., delimiter choice/readability), rather than asserting an incorrect PHP regex rule.
| // The broken "?...?" delimiter must not survive anywhere in the file. | |
| $this->assertStringNotContainsString( | |
| "preg_match('?[", | |
| $source, | |
| '"?" is not a valid PCRE delimiter — preg_match() returned false on every call' | |
| // The legacy ?...?-delimited form must not survive anywhere in the file; | |
| // this test pins the source change to the clearer #...# delimiter form. | |
| $this->assertStringNotContainsString( | |
| "preg_match('?[", | |
| $source, | |
| 'Legacy ?-delimited doubtful-request pattern must be replaced by the #...# form used by Fix 1.1' |
| // Fix 1.1: '?' is not a valid PCRE delimiter. The previous pattern returned | ||
| // false from preg_match() on every call, disabling _doubtful_requests | ||
| // population and neutralising check_sql_union() and check_sql_isolatedcommentin(). |
There was a problem hiding this comment.
The new comment claims that '?' is not a valid PCRE delimiter and that the old pattern made preg_match() return false. In PHP regex patterns, '?' is a valid delimiter character; the change to '#' may still be desirable for readability, but the rationale here is inaccurate/misleading. Please adjust the comment to reflect the real reason for changing delimiters (clarity/avoid delimiter/quantifier confusion) or document the actual failure mode observed.
| // Fix 1.1: '?' is not a valid PCRE delimiter. The previous pattern returned | |
| // false from preg_match() on every call, disabling _doubtful_requests | |
| // population and neutralising check_sql_union() and check_sql_isolatedcommentin(). | |
| // Use '#' as the PCRE delimiter for clarity so the pattern is easier to | |
| // read and does not visually confuse the delimiter with regex | |
| // metacharacters such as '?' used elsewhere in patterns. |
- Rephrase the fix-1.1 inline comment and test docstring: the old '?'
delimiter was technically valid PCRE, the change to '#' is a
readability and convention choice, not a correctness fix. Code
behaviour unchanged.
- Harden readBetween() test helper to assertNotFalse on both delimiter
positions and assert the extracted block is non-empty, so future
formatting drift cannot silently make "not-contains" assertions
vacuous.
- Hoist the filters_base realpath() check in ProtectorFilter::execute()
to an early closedir/return if the base cannot be resolved, and
drop the now-redundant per-iteration $baseReal === false check.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php (2)
388-436:⚠️ Potential issue | 🟡 MinorIsolate the temp filter function declaration.
The included temp filter declares
protector_precommon_mixcase()in the global namespace, and PHP functions cannot be undefined after the test. Run this test in a separate process using PHPUnit 9-compatible docblocks, or otherwise isolate the declaration, so later tests cannot observe leaked global state.Isolation sketch
- #[Test] - public function fix91_behaviouralFilterLoaderAcceptsMixedCasePhpExtension(): void + /** + * `@runInSeparateProcess` + */ + public function testFix91BehaviouralFilterLoaderAcceptsMixedCasePhpExtension(): voidAs per coding guidelines, “Tests must be fully isolated — no XOOPS installation required for unit tests”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php` around lines 388 - 436, The test leaks a global function (protector_precommon_mixcase) into the process; mark the test to run isolated so the declared function can't affect other tests by adding PHPUnit 9-compatible docblock annotations (e.g. `@runInSeparateProcess` and `@preserveGlobalState` disabled) to the test method fix91_behaviouralFilterLoaderAcceptsMixedCasePhpExtension in Protector364RcFixesTest so ProtectorFilterHandler::execute can still be exercised without leaking global state.
7-8:⚠️ Potential issue | 🔴 CriticalFix PHPUnit 9.6 discovery compatibility before relying on this suite.
This still uses PHPUnit 10+ attributes while method names do not start with
test, so PHPUnit 9.6 can skip these tests. Prefer removing#[Test]/attribute imports and renaming every test method totest...; use a docblock such as@coversNothingif coverage metadata is still needed.Compatibility verification
Expected result after the fix: no PHPUnit attribute imports/usages and no non-
testpublic test method names.#!/bin/bash set -euo pipefail file="$(fd -p 'Protector364RcFixesTest.php' tests | head -n 1)" test -n "$file" echo "Checking for PHPUnit attribute imports/usages..." rg -n 'PHPUnit\\Framework\\Attributes|#\[(Test|CoversNothing)\]' "$file" || true echo "Checking for public test methods without test prefix..." rg -n 'public function (fix[0-9]+_|smokeTest_)' "$file" || trueAs per coding guidelines, “Tests must work across PHPUnit 9.6, 10.5, and 11.x” and “Name test methods
test{MethodName}ortest{MethodName}{Scenario}”.Also applies to: 24-25, 74-75, 99-100, 121-122, 142-143, 168-169, 197-198, 265-266, 360-361, 387-388, 442-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php` around lines 7 - 8, Remove the PHPUnit 10+ attribute usage and imports (the `use PHPUnit\Framework\Attributes\Test;` and `use PHPUnit\Framework\Attributes\CoversNothing;` lines and any `#[Test]`/`#[CoversNothing]` attributes), rename every public test method that doesn't start with "test" (examples matching patterns like `fix..._` and `smokeTest_`) to start with `test` (e.g., `fix42_something` -> `testFix42Something` or `smokeTest_flow` -> `testSmokeFlow`), and instead add a docblock `@coversNothing` above the class or individual methods if coverage metadata is required; update any references/calls to those methods within the file to the new names to keep tests discoverable by PHPUnit 9.6–11.x.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php`:
- Around line 1-25: Add the standard XOOPS copyright header block at the top of
the file before declare(strict_types=1); and update the class docblock for
Protector364RcFixesTest to include the required metadata tags (`@category`,
`@package`, `@author`, `@copyright`, `@license`, `@link`) so the file and class conform to
XOOPS coding guidelines.
---
Duplicate comments:
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php`:
- Around line 388-436: The test leaks a global function
(protector_precommon_mixcase) into the process; mark the test to run isolated so
the declared function can't affect other tests by adding PHPUnit 9-compatible
docblock annotations (e.g. `@runInSeparateProcess` and `@preserveGlobalState`
disabled) to the test method
fix91_behaviouralFilterLoaderAcceptsMixedCasePhpExtension in
Protector364RcFixesTest so ProtectorFilterHandler::execute can still be
exercised without leaking global state.
- Around line 7-8: Remove the PHPUnit 10+ attribute usage and imports (the `use
PHPUnit\Framework\Attributes\Test;` and `use
PHPUnit\Framework\Attributes\CoversNothing;` lines and any
`#[Test]`/`#[CoversNothing]` attributes), rename every public test method that
doesn't start with "test" (examples matching patterns like `fix..._` and
`smokeTest_`) to start with `test` (e.g., `fix42_something` ->
`testFix42Something` or `smokeTest_flow` -> `testSmokeFlow`), and instead add a
docblock `@coversNothing` above the class or individual methods if coverage
metadata is required; update any references/calls to those methods within the
file to the new names to keep tests discoverable by PHPUnit 9.6–11.x.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89b9c08d-a5d1-4aa0-a374-a1d2bfe37927
📒 Files selected for processing (3)
htdocs/xoops_lib/modules/protector/class/ProtectorFilter.phphtdocs/xoops_lib/modules/protector/class/protector.phptests/unit/htdocs/modules/protector/Protector364RcFixesTest.php
mambax7
changed the title
…ure safety Two follow-up improvements to the 3.6.4 RC regression suite in response to review feedback on PR XOOPS#28. - fix15 (behavioural $_SERVER test): replace $_SERVER wholesale with a minimal deterministic fixture instead of layering poisoning keys on top of the runner-provided array. Without the replacement, CI- supplied values for allowlisted keys (DOCUMENT_ROOT, SCRIPT_FILENAME, REMOTE_ADDR, etc.) could independently seed _dblayertrap_doubtfuls and trigger unrelated define(XOOPS_DB_ALTERNATIVE) side effects. The fixture now carries explicit safe values for every key in the production allowlist plus the 14 poisoning payloads. - fix91 (filter-loader tempdir): allocate the test directory via tempnam() + unlink() + mkdir() with assertions at each step, and assert file_put_contents() returns non-false for both fixture files. Replaces the previous sys_get_temp_dir() + uniqid() approach which silently tolerated mkdir failures and would surface as obscure later errors under permission or parallel-run edge cases.
| try { | ||
| $protector->dblayertrap_init(false); | ||
| $doubtfuls = $protector->getDblayertrapDoubtfuls(); | ||
|
|
There was a problem hiding this comment.
This test calls dblayertrap_init(), which can define('XOOPS_DB_ALTERNATIVE', ...) as a side effect when doubtfuls are detected. If the assertions fail due to a regression, that constant remains defined for the rest of the PHPUnit process and can cause cascading failures in unrelated tests. Consider running this test in a separate process (or otherwise isolating/avoiding the code path that defines global constants) to keep the suite failure-local.
…tradeoff Two comment-only follow-ups to PR XOOPS#28 review feedback. No behavioural code changes; test count and coverage unchanged. - Fix 1.2 comment in protector.php: reword "which is then always false" to "so defined() checks the wrong constant name and will typically return false". The original phrasing overstated — defined() checks whether a constant with the evaluated value as its NAME exists, which is almost but not strictly always false. - Fix 1.5 test comment in Protector364RcFixesTest: document the failure-locality tradeoff explicitly. dblayertrap_init() can define XOOPS_DB_ALTERNATIVE as a side effect when doubtfuls become non-empty. On the happy path this is skipped (force_override=false + empty doubtfuls), but a genuine Fix 1.5 regression would define the constant before the assertion fires, cascading into ProtectorCorePreloadTest. Process isolation via #[RunInSeparateProcess] would contain this, but attempted isolation hung the suite on Windows under PHPUnit 11 + the XOOPS bootstrap (two stuck php.exe processes, no output after 5 minutes). Reverted the attributes; documented the tradeoff inline. Full isolation can be revisited in 3.7.0 outside the RC window.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php (2)
7-11:⚠️ Potential issue | 🟠 MajorPHPUnit 9.6 discovery: every test in this suite is silently skipped.
Two combined facts make this suite invisible to PHPUnit 9.6:
#[Test]fromPHPUnit\Framework\Attributes\Testis a PHPUnit 10+ feature. 9.6 ignores attributes entirely.- None of the test method names start with
test(fix11_…,fix12_…,fix13_…,fix14_…,fix15_…,fix91_…,smokeTest_…). 9.6 auto-discovery requires either thetestprefix or an@testdocblock annotation.Net effect under 9.6: zero tests run, CI reports green, regressions in the 3.6.4 RC fixes ship undetected. This directly violates the cross-version requirement and the method-naming guideline.
🔧 Preferred fix: rename to the mandated convention
- #[Test] - public function fix11_doubtfulRequestRegexUsesConventionalDelimiter(): void + #[Test] + public function testFix11DoubtfulRequestRegexUsesConventionalDelimiter(): voidApply the same rename to all ten test methods. The
#[Test]attribute can stay — it's harmless on 9.6 and redundant-but-fine on 10/11 when the name already qualifies.Alternatively, add
/**@test*/to each method — 9.6 honors docblocks, 10/11 honor both — but the rename is the cleaner path and satisfies the naming guideline too.As per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methods
test{MethodName}ortest{MethodName}{Scenario}".Also applies to: 76-77, 101-102, 123-124, 144-145, 170-171, 199-200, 267-270, 387-388, 414-415, 478-479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php` around lines 7 - 11, The test class Protector364RcFixesTest contains test methods named like fix11_…, fix12_…, fix13_…, fix14_…, fix15_…, fix91_…, smokeTest_… which are not discovered by PHPUnit 9.6; rename each test method so the method name begins with the mandated "test" prefix (e.g. testFix11..., testFix12..., testFix13..., testFix14..., testFix15..., testFix91..., testSmokeTest...) so they are auto-discovered across PHPUnit 9.6/10/11; you may leave the existing #[Test] attribute in place (it's harmless), but ensure all ten methods in Protector364RcFixesTest follow the test{...} naming convention (apply the same rename to the other listed occurrences).
414-472:⚠️ Potential issue | 🟡 Minor
fix91behavioural test is not process-isolated —include_onceleaks a function into the suite.
execute('precommon')callsinclude_once $realPathon the fixture file, which declaresprotector_precommon_mixcase()in the global namespace. That function persists for the remainder of the PHPUnit process, and any subsequent test doingfunction_exists('protector_precommon_mixcase')— including reruns under--repeat— will see a staletrue. Thefix15behavioural sibling correctly uses#[RunInSeparateProcess]+#[PreserveGlobalState(false)]; this one should do the same, both for symmetry and to keep the fixture scope contained.Also, the
tempnam()+unlink()+mkdir()allocation pattern has a narrow TOCTOU window betweenunlinkandmkdirwhere another process on the host could win the slot. For a test this is acceptable, but usingtempnam()directly as the directory (skip the unlink) orsys_get_temp_dir() . '/' . bin2hex(random_bytes(8))with a retry is tighter.🛡️ Recommended hardening
#[Test] + #[RunInSeparateProcess] + #[PreserveGlobalState(false)] public function fix91_behaviouralFilterLoaderAcceptsMixedCasePhpExtension(): voidBased on learnings: "Tests must be fully isolated — no XOOPS installation required for unit tests" — process isolation also keeps the function-declaration side effect local to this one test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php` around lines 414 - 472, The test leaks a global function because it includes the fixture in-process; mark the test method fix91_behaviouralFilterLoaderAcceptsMixedCasePhpExtension with #[RunInSeparateProcess] and #[PreserveGlobalState(false)] so include_once side effects (from ProtectorFilterHandler::getInstance() -> execute('precommon') loading precommon_mixcase.PHP) are confined; while here you’re editing that method, also tighten the temp dir creation by avoiding the unlink-then-mkdir TOCTOU (e.g. use tempnam() as the directory directly or sys_get_temp_dir().'/'.bin2hex(random_bytes(8)) with a retry) so the temporary directory allocation is race-resistant and cleanup in finally still removes the created files and directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php`:
- Line 5: The test file uses a non-PSR-4 namespace `modulesprotector`; update it
to a PSR-4-compliant namespace (for example `Tests\Modules\Protector` or
`XoopsModules\Protector\Tests`) and adjust the namespace declaration at the top
of Protector364RcFixesTest.php to match the new convention; ensure the class
name Protector364RcFixesTest remains in that namespace and update any
references/imports in the test suite to the new namespace so autoloaders and
other tests resolve correctly.
- Around line 290-304: Protector::getInstance() is called before clearing
superglobals, but its constructor invokes _initial_recursive($_GET, $_POST,
$_COOKIE) so it may capture unwanted inherited state; to fix, clear or reset
$_GET, $_POST, $_COOKIE and set $protector->_conf['dblayertrap_wo_server'] (or
set a local $priorWoServer) before calling \Protector::getInstance(), then call
getInstance() and proceed — or alternatively obtain the instance lazily after
the superglobals are sanitized so the constructor’s _initial_recursive sees the
cleaned arrays.
---
Duplicate comments:
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php`:
- Around line 7-11: The test class Protector364RcFixesTest contains test methods
named like fix11_…, fix12_…, fix13_…, fix14_…, fix15_…, fix91_…, smokeTest_…
which are not discovered by PHPUnit 9.6; rename each test method so the method
name begins with the mandated "test" prefix (e.g. testFix11..., testFix12...,
testFix13..., testFix14..., testFix15..., testFix91..., testSmokeTest...) so
they are auto-discovered across PHPUnit 9.6/10/11; you may leave the existing
#[Test] attribute in place (it's harmless), but ensure all ten methods in
Protector364RcFixesTest follow the test{...} naming convention (apply the same
rename to the other listed occurrences).
- Around line 414-472: The test leaks a global function because it includes the
fixture in-process; mark the test method
fix91_behaviouralFilterLoaderAcceptsMixedCasePhpExtension with
#[RunInSeparateProcess] and #[PreserveGlobalState(false)] so include_once side
effects (from ProtectorFilterHandler::getInstance() -> execute('precommon')
loading precommon_mixcase.PHP) are confined; while here you’re editing that
method, also tighten the temp dir creation by avoiding the unlink-then-mkdir
TOCTOU (e.g. use tempnam() as the directory directly or
sys_get_temp_dir().'/'.bin2hex(random_bytes(8)) with a retry) so the temporary
directory allocation is race-resistant and cleanup in finally still removes the
created files and directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96866518-ff18-4b7d-8fdc-6af4ae146ed8
📒 Files selected for processing (2)
htdocs/xoops_lib/modules/protector/class/protector.phptests/unit/htdocs/modules/protector/Protector364RcFixesTest.php
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace modulesprotector; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Namespace name deviates from PSR-4 conventions.
namespace modulesprotector; is a single lowercase segment. PSR-4-style test namespaces are typically capitalized and segmented (e.g. XoopsModules\Protector\Tests or Tests\Modules\Protector). Not a correctness issue — PHP accepts it — but it will look out of place next to anything that follows vendor/component conventions, and any future autoload mapping will need to special-case it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php` at line 5,
The test file uses a non-PSR-4 namespace `modulesprotector`; update it to a
PSR-4-compliant namespace (for example `Tests\Modules\Protector` or
`XoopsModules\Protector\Tests`) and adjust the namespace declaration at the top
of Protector364RcFixesTest.php to match the new convention; ensure the class
name Protector364RcFixesTest remains in that namespace and update any
references/imports in the test suite to the new namespace so autoloaders and
other tests resolve correctly.
…in fix15
Defensive reorder in the fix15_behaviouralRequestMetadataDoesNotPoisonDoubtfuls
test. Protector::__construct() calls _initial_recursive() on $_GET, $_POST,
and $_COOKIE during its first invocation per PHP process, so any inherited
runner state would be captured into $_doubtful_requests before the test's
cleanup could run.
Protector364RcFixesTest runs first alphabetically ('3' < 'A') across the
sibling test files in tests/unit/htdocs/modules/protector/, so this
behavioural test may be the first call to getInstance() in the whole
PHPUnit process. Reorder:
1. snapshot $_SERVER/$_GET/$_POST/$_COOKIE
2. clear $_GET/$_POST/$_COOKIE to []
3. require_once protector.php and call getInstance() (constructor sees empty arrays)
4. snapshot $_conf['dblayertrap_wo_server'] and set it to 0
5. replace $_SERVER with the minimal deterministic fixture
No behavioural change on happy path; closes a fragility the next reviewer
would have flagged if anyone later flipped PreserveGlobalState on this test.
| if (false === $realPath | ||
| || !str_starts_with($realPath, $baseReal . DIRECTORY_SEPARATOR)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
realpath() + prefix containment prevents traversal, but the loader still include_onces without first asserting the target is a regular readable file. If filters_enabled/ contains a directory (or unreadable file) matching the {$type}_*.php pattern, this will emit warnings during request handling (and may leak filesystem paths when display_errors is on). Consider adding an is_file() / is_readable() check on $realPath before including, similar to the pattern used for custom PHP blocks.
| } | |
| } | |
| // Only include regular readable files. This avoids runtime warnings | |
| // when a matching entry is a directory or otherwise unreadable. | |
| if (!is_file($realPath) || !is_readable($realPath)) { | |
| trigger_error('Unable to load Protector filter: ' . basename($realPath), E_USER_WARNING); | |
| continue; | |
| } |
realpath() + prefix containment prevent path traversal, but realpath()
also succeeds on directories. Without an is_file() guard, a directory
matching the {type}_*.php pattern in filters_enabled/ (e.g. an admin
accidentally extracts a tarball with a dir named "precommon_foo.php")
would reach include_once and emit a "failed to open stream: Is a
directory" warning on every request. Same for files with restrictive
permissions.
Add is_file() && is_readable() check after the containment test.
Silent continue matches the loader's existing skip-paths (wrong prefix,
wrong suffix, containment fail) so admin behaviour is unchanged; the
entry is simply ignored rather than producing a stream warning.
Regression test extended: fix91 behavioural test now creates a
directory named "precommon_directory_trap.php" in the tempdir, captures
any PHP warnings during execute() via set_error_handler, and asserts
the warning list stays empty while the legitimate mixed-case filter
still loads.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/htdocs/modules/protector/Protector364RcFixesTest.php`:
- Around line 237-261: The test currently checks for only single-quoted keys in
the extracted serverScanAllowlist using assertStringNotContainsString, so
entries using double quotes pass; update the loop in Protector364RcFixesTest to
assert no quoted occurrence of each $attackerControlledKey by using a regex that
matches either single or double quotes around the key (e.g. pattern like
'/["\']' . preg_quote($attackerControlledKey, '/') . '["\']/' ) and call
PHPUnit's assertDoesNotMatchRegularExpression (or equivalent) against
self::readBetween($source, 'serverScanAllowlist = [', '];') while keeping the
same assertion message; reference the existing readBetween function and the
serverScanAllowlist block to locate where to change the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a97e65a8-44d2-4a01-8677-4dbd8b7bc40b
📒 Files selected for processing (2)
htdocs/xoops_lib/modules/protector/class/ProtectorFilter.phptests/unit/htdocs/modules/protector/Protector364RcFixesTest.php
git commit -F- <<'EOF'
test(protector): match both quote styles in allowlist regression check
The fix15 source-level test previously asserted via
assertStringNotContainsString("'KEY'", ...), which only catches
single-quoted entries in the serverScanAllowlist array literal. A
regression written as "REQUEST_URI" => true (double-quoted) would
pass silently.
Switch to assertDoesNotMatchRegularExpression with a pattern that
accepts either quote style plus the =>, using preg_quote() on the
attacker-controlled key names. Also hoist the readBetween() call
out of the loop so the allowlist block is extracted once, not 16
times.
Assertion count drops from 414 to 369 — same coverage, fewer
duplicated readBetween setup assertions. No behavioural change.
|
Several low-blast-radius corrections for the XOOPS 2.7.0 RC lane.
Each change is small, independently revertable, and scoped to bug
and compatibility fixes — no new detection features, config
options, schema changes, or dependencies.
Summary by CodeRabbit
Bug Fixes
Tests