Pin all Request calls to explicit GET/POST hash, fix elvis on zero-valid fields#1650
Conversation
Replace ~30 hashless Request::getXxx('key', default) calls with
explicit 'GET' or 'POST' hash arguments across system admin pages
(groups, comments, preferences, smilies, maintenance, userrank),
profile admin (category), and profile search.
Key changes:
- All op parameters use dual-source hasVar pattern for GET/POST
- Pagination params (start, limit) pinned to 'GET'
- Form field values pinned to 'POST'
- Display/filter params pinned to 'GET'
- Replace elvis (?:) dual-source patterns in profile/search.php
with hasVar ternary to avoid zero-valid field bugs (_larger,
_smaller, _match, start, limit, fieldValues)
- Fix category.php delete case: id=0 with ?: would skip POST
Add PHPUnit source-scanning test that verifies no hashless
Request::getXxx() calls remain and no elvis operator is used
for dual-source Request patterns in all hardened files.
WalkthroughThese changes systematically refactor input source handling across multiple admin modules to enforce explicit HTTP method preferences—favoring POST for action parameters and GET for pagination. Additionally, a new test suite validates that all Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
Hardens request-variable handling by making Xmf\Request::getXxx() call sites explicit about the source hash (GET vs POST), and removes ?: elvis patterns where 0/empty-string values must be preserved.
Changes:
- Pin
Request::getXxx()calls to explicit'GET'/'POST'in multiple system admin controllers and profile modules. - Replace elvis-based dual-source request reads in
profile/search.phpwithhasVar()-based selection to preserve zero-valid values. - Add a unit test to enforce “no hashless Request calls” and “no elvis dual-source Request calls” across the hardened file set.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/hardening/RequestHashPinningTest.php | Adds static-analysis tests to detect hashless Request::getXxx() calls and elvis dual-source patterns. |
| htdocs/modules/system/admin/userrank/main.php | Pins op, paging, and form fields to explicit GET/POST sources. |
| htdocs/modules/system/admin/smilies/main.php | Pins op, paging, and form fields to explicit GET/POST sources. |
| htdocs/modules/system/admin/preferences/main.php | Pins op and confcat_id to explicit sources. |
| htdocs/modules/system/admin/maintenance/main.php | Pins op and maintenance form inputs to explicit POST. |
| htdocs/modules/system/admin/groups/main.php | Pins op to explicit GET/POST source selection. |
| htdocs/modules/system/admin/comments/main.php | Pins op, com_id, and pagination inputs to explicit sources. |
| htdocs/modules/profile/search.php | Replaces elvis dual-source reads with hasVar() selection for multiple search inputs. |
| htdocs/modules/profile/admin/category.php | Replaces elvis dual-source id selection with hasVar() ternary. |
| $criteria->setOrder($order); | ||
|
|
||
| $limit = Request::getInt('limit', 0, 'GET') ?: Request::getInt('limit', $limit_default, 'POST'); | ||
| $limit = Request::hasVar('limit', 'GET') ? Request::getInt('limit', $limit_default, 'GET') : Request::getInt('limit', $limit_default, 'POST'); |
There was a problem hiding this comment.
$limit can now be 0 (or negative) when provided via GET because the code checks hasVar() rather than using an elvis/default fallback. In ProfileProfileHandler::search() a criteria limit of 0 means “no LIMIT”, so this can trigger an unbounded user query (potentially very expensive). Clamp/validate limit to a sane range (e.g., >=1 and <= max) before calling setLimit(), or treat 0 as “use default”.
| $limit = Request::hasVar('limit', 'GET') ? Request::getInt('limit', $limit_default, 'GET') : Request::getInt('limit', $limit_default, 'POST'); | |
| $limit = Request::hasVar('limit', 'GET') ? Request::getInt('limit', $limit_default, 'GET') : Request::getInt('limit', $limit_default, 'POST'); | |
| if ($limit < 1) { | |
| $limit = $limit_default; | |
| } |
| self::markTestSkipped("File not found: {$filePath}"); | ||
| } | ||
|
|
||
| $source = file_get_contents($filePath); |
There was a problem hiding this comment.
file_get_contents($filePath) can return false even when file_exists() is true (permissions, transient FS errors). With strict_types=1, passing false into explode() will raise a TypeError and turn this into a fatal error instead of a clean test failure. Add an assertNotFalse($source) (and optionally assertNotEmpty) after reading the file, similar to how other source-analysis tests handle this.
| $source = file_get_contents($filePath); | |
| $source = file_get_contents($filePath); | |
| self::assertNotFalse( | |
| $source, | |
| sprintf('Unable to read source file: %s', basename($filePath)) | |
| ); | |
| self::assertNotEmpty( | |
| $source, | |
| sprintf('Source file is empty: %s', basename($filePath)) | |
| ); |
| $source = file_get_contents($filePath); | ||
| $lines = explode("\n", $source); |
There was a problem hiding this comment.
Same as above: file_get_contents($filePath) isn’t checked for false before being used, which can cause a TypeError and abort the test run. Add an assertNotFalse($source) (and optionally assertNotEmpty) after reading the file so failures report cleanly.
Fixes B-1 showstopper on PR XOOPS#1650. The smilies_update_display and userrank_update_special toggle handlers read IDs from GET only, but system_setStatus() in admin.js sends them via $.post(). Changed both to Request::getInt(..., 0, 'POST') so the AJAX on/off toggles work correctly.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/profile/search.php (1)
302-340:⚠️ Potential issue | 🟡 MinorZero-valued range bounds are still silently dropped.
The guards at Line 311, Line 321, Line 331, and Line 337 still treat
0as “not provided”, so the newhasVar()reads at Line 302 and Line 303 do not actually make_larger=0/_smaller=0searchable. Use presence/validation checks instead of numeric truthiness here.🧩 One way to preserve
0without accepting invalid input- $intLarger = Request::hasVar($fieldname . '_larger', 'GET') ? Request::getInt($fieldname . '_larger', 0, 'GET') : Request::getInt($fieldname . '_larger', 0, 'POST'); - $intSmaller = Request::hasVar($fieldname . '_smaller', 'GET') ? Request::getInt($fieldname . '_smaller', 0, 'GET') : Request::getInt($fieldname . '_smaller', 0, 'POST'); - if ($intLarger !== 0) { + if ($largerVal !== '' && filter_var($largerVal, FILTER_VALIDATE_INT) !== false) { + $intLarger = (int) $largerVal; $search_url[] = $fieldname . '_larger=' . $intLarger; $searchvars[] = $fieldname; $criteria->add(new Criteria($fieldname, $intLarger, '>=')); } - if ($intSmaller !== 0) { + if ($smallerVal !== '' && filter_var($smallerVal, FILTER_VALIDATE_INT) !== false) { + $intSmaller = (int) $smallerVal; $search_url[] = $fieldname . '_smaller=' . $intSmaller; $searchvars[] = $fieldname; $criteria->add(new Criteria($fieldname, $intSmaller, '<=')); }Apply the same presence-based check in the
date/datetimebranch so epoch0is not discarded there either.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/search.php` around lines 302 - 340, The code currently drops zero bounds by testing numeric truthiness; update the date/datetime and default branches to use presence and explicit validation instead of >0/!==0 checks: for $largerVal/$smallerVal call strtotime and treat the result as valid when !== false (or cast to int and allow 0) and use Request::hasVar($fieldname.'_larger'...) / Request::hasVar($fieldname.'_smaller'...) to decide whether to add a criteria; for the integer branch use Request::hasVar to detect presence and validate with is_numeric (or Request::getInt but gated by hasVar) before adding $intLarger/$intSmaller to $search_url, $searchvars and $criteria so a provided 0 is preserved while still rejecting non-numeric input.
🤖 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/modules/profile/search.php`:
- Around line 439-442: The code currently only enforces a lower bound for $limit
but allows arbitrarily large values to reach $criteria->setLimit(), so add an
upper clamp after reading the value: compute $limit using Request::getInt as
before, then if $limit < 1 set to $limit_default and if $limit > <CAP> set to
<CAP> (use an existing configuration constant or a module-level MAX_LIMIT
instead of a literal); update any relevant documentation/comments and ensure
$criteria->setLimit($limit) receives the clamped value.
In `@tests/unit/htdocs/hardening/RequestHashPinningTest.php`:
- Around line 121-122: The regex stored in $elvisPattern uses [^)]* which fails
on nested parentheses in argument lists (so Request::getInt(... max(0, $x) ...)
will not match); replace the naive [^)]* with a more robust pattern that allows
nested parentheses or a balanced group (e.g., use a non-greedy match with
optional nested-parentheses handling or a recursive pattern if PCRE is enabled)
so the pattern still targets the Request::getString|getInt|... call followed by
?: Request::; update the $elvisPattern variable accordingly to ensure it matches
calls with complex arguments.
- Around line 54-106: The noHashlessRequestCalls method is over-complex; extract
the inner match-handling logic into a private helper to reduce nesting and
cognitive complexity. Create a helper (e.g., handleRequestCallMatch or
collectHashlessRequestCall) that accepts the original line, the match full text
and offset, the line number and a reference to $violations; move the parenPos
calculation, call to extractBalancedArgs, argCount via countArguments, and the
violation push (when argCount <= 2) into that helper. Replace the inner loop
body in noHashlessRequestCalls with a call to this helper for each match so the
main test reads as a simple loop over matches and delegates parsing/violation
logic to the new method; continue using CALL_START_PATTERN, extractBalancedArgs
and countArguments as-is.
- Around line 7-9: The tests use PHPUnit 10 attributes which break compatibility
with PHPUnit 9.6; update the two test methods noHashlessRequestCalls and
noElvisOperatorOnDualSourceRequestCalls to use docblock annotations (`@test` and
`@dataProvider`) instead of #[Test] and #[DataProvider], remove the now-unused use
statements for PHPUnit\Framework\Attributes\Test and
PHPUnit\Framework\Attributes\DataProvider, and ensure the data provider names
referenced in the docblocks match the existing provider methods.
---
Outside diff comments:
In `@htdocs/modules/profile/search.php`:
- Around line 302-340: The code currently drops zero bounds by testing numeric
truthiness; update the date/datetime and default branches to use presence and
explicit validation instead of >0/!==0 checks: for $largerVal/$smallerVal call
strtotime and treat the result as valid when !== false (or cast to int and allow
0) and use Request::hasVar($fieldname.'_larger'...) /
Request::hasVar($fieldname.'_smaller'...) to decide whether to add a criteria;
for the integer branch use Request::hasVar to detect presence and validate with
is_numeric (or Request::getInt but gated by hasVar) before adding
$intLarger/$intSmaller to $search_url, $searchvars and $criteria so a provided 0
is preserved while still rejecting non-numeric input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63cf4713-23d5-4396-b77a-583b77afa746
📒 Files selected for processing (9)
htdocs/modules/profile/admin/category.phphtdocs/modules/profile/search.phphtdocs/modules/system/admin/comments/main.phphtdocs/modules/system/admin/groups/main.phphtdocs/modules/system/admin/maintenance/main.phphtdocs/modules/system/admin/preferences/main.phphtdocs/modules/system/admin/smilies/main.phphtdocs/modules/system/admin/userrank/main.phptests/unit/htdocs/hardening/RequestHashPinningTest.php
| $limit = Request::hasVar('limit', 'GET') ? Request::getInt('limit', $limit_default, 'GET') : Request::getInt('limit', $limit_default, 'POST'); | ||
| if ($limit < 1) { | ||
| $limit = $limit_default; | ||
| } |
There was a problem hiding this comment.
Clamp limit on the upper end too.
This guards against < 1, but ?limit=1000000 still flows straight into $criteria->setLimit(). On a public search endpoint, that can turn a cheap paged query into a very large read and render.
📏 Suggested cap
$limit = Request::hasVar('limit', 'GET') ? Request::getInt('limit', $limit_default, 'GET') : Request::getInt('limit', $limit_default, 'POST');
if ($limit < 1) {
$limit = $limit_default;
+ } elseif ($limit > 100) {
+ $limit = 100;
}If the cap is configurable elsewhere, use that constant/config value instead of a literal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $limit = Request::hasVar('limit', 'GET') ? Request::getInt('limit', $limit_default, 'GET') : Request::getInt('limit', $limit_default, 'POST'); | |
| if ($limit < 1) { | |
| $limit = $limit_default; | |
| } | |
| $limit = Request::hasVar('limit', 'GET') ? Request::getInt('limit', $limit_default, 'GET') : Request::getInt('limit', $limit_default, 'POST'); | |
| if ($limit < 1) { | |
| $limit = $limit_default; | |
| } elseif ($limit > 100) { | |
| $limit = 100; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/search.php` around lines 439 - 442, The code currently
only enforces a lower bound for $limit but allows arbitrarily large values to
reach $criteria->setLimit(), so add an upper clamp after reading the value:
compute $limit using Request::getInt as before, then if $limit < 1 set to
$limit_default and if $limit > <CAP> set to <CAP> (use an existing configuration
constant or a module-level MAX_LIMIT instead of a literal); update any relevant
documentation/comments and ensure $criteria->setLimit($limit) receives the
clamped value.
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHPUnit 9.6 attribute support for Test and DataProvider
💡 Result:
PHPUnit 9.6 does not support PHP 8 attributes such as #[Test] or #[DataProvider] for marking test methods or wiring data providers. In 9.6 you must use:
- test method naming convention
test*or the docblock annotation@testto mark a method as a test. [1] - the docblock annotation
@dataProviderto attach a data provider to a test method. [2]
Attribute-based equivalents like PHPUnit\Framework\Attributes\DataProvider (and other PHPUnit\Framework\Attributes\*) are documented for PHPUnit 10+. [3]
Sources
[1] PHPUnit 9.6 manual – “Writing Tests for PHPUnit” (@test annotation) (docs.phpunit.de)
[2] PHPUnit 9.6 manual – “Data Providers” (@dataProvider annotation) (docs.phpunit.de)
[3] PHPUnit 10 manual – “Attributes” (includes DataProvider(...) attribute) (docs.phpunit.de)
Citations:
- 1: https://docs.phpunit.de/en/9.6/writing-tests-for-phpunit.html
- 2: https://docs.phpunit.de/en/9.6/writing-tests-for-phpunit.html
- 3: https://docs.phpunit.de/en/10.5/attributes.html
PHPUnit attributes require version 10+; tests must work across 9.6, 10.5, and 11.x.
PHP 8 attributes (#[Test], #[DataProvider]) are not recognized by PHPUnit 9.6. Add docblock annotations to maintain compatibility:
Required fix
- #[Test]
- #[DataProvider('hardenedFilesProvider')]
+ /**
+ * `@test`
+ * `@dataProvider` hardenedFilesProvider
+ */
public function noHashlessRequestCalls(string $filePath): voidApply to both noHashlessRequestCalls and noElvisOperatorOnDualSourceRequestCalls. Remove the unused Attributes\Test and Attributes\DataProvider imports once docblock annotations are in place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/hardening/RequestHashPinningTest.php` around lines 7 - 9,
The tests use PHPUnit 10 attributes which break compatibility with PHPUnit 9.6;
update the two test methods noHashlessRequestCalls and
noElvisOperatorOnDualSourceRequestCalls to use docblock annotations (`@test` and
`@dataProvider`) instead of #[Test] and #[DataProvider], remove the now-unused use
statements for PHPUnit\Framework\Attributes\Test and
PHPUnit\Framework\Attributes\DataProvider, and ensure the data provider names
referenced in the docblocks match the existing provider methods.
| #[Test] | ||
| #[DataProvider('hardenedFilesProvider')] | ||
| public function noHashlessRequestCalls(string $filePath): void | ||
| { | ||
| if (!file_exists($filePath)) { | ||
| self::markTestSkipped("File not found: {$filePath}"); | ||
| } | ||
|
|
||
| $source = file_get_contents($filePath); | ||
| self::assertNotFalse($source, "Failed to read file: {$filePath}"); | ||
| $lines = explode("\n", $source); | ||
| $violations = []; | ||
|
|
||
| foreach ($lines as $lineNum => $line) { | ||
| // Skip comments | ||
| $trimmed = ltrim($line); | ||
| if (str_starts_with($trimmed, '//') || str_starts_with($trimmed, '*') || str_starts_with($trimmed, '/*')) { | ||
| continue; | ||
| } | ||
|
|
||
| // Find all Request::getXxx( occurrences and extract balanced args | ||
| if (preg_match_all(self::CALL_START_PATTERN, $line, $matches, PREG_OFFSET_CAPTURE)) { | ||
| foreach ($matches[0] as $matchInfo) { | ||
| $fullMatch = $matchInfo[0]; | ||
| $offset = $matchInfo[1]; | ||
| // Find the opening paren position | ||
| $parenPos = $offset + strlen($fullMatch) - 1; | ||
| // Extract balanced argument string | ||
| $inner = self::extractBalancedArgs($line, $parenPos); | ||
| if ($inner === null) { | ||
| continue; | ||
| } | ||
| $argCount = self::countArguments($inner); | ||
|
|
||
| // Calls with <= 2 args are missing the hash (3rd arg) | ||
| if ($argCount <= 2) { | ||
| $callText = substr($line, $offset, strlen($fullMatch) + strlen($inner) + 1); | ||
| $violations[] = sprintf(' Line %d: %s', $lineNum + 1, trim($callText)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| self::assertEmpty( | ||
| $violations, | ||
| sprintf( | ||
| "Found %d hashless Request::getXxx() call(s) in %s:\n%s", | ||
| count($violations), | ||
| basename($filePath), | ||
| implode("\n", $violations), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing complexity in noHashlessRequestCalls.
SonarCloud flags cognitive complexity of 18 (threshold: 15). While this is test code and the parsing logic is inherently branchy, you could extract the inner loop body into a helper method for clarity.
♻️ Optional refactor to reduce nesting
+ /**
+ * Check a single line for hashless Request calls.
+ * `@return` array<string> Violations found on this line
+ */
+ private static function checkLineForHashlessCalls(string $line, int $lineNum): array
+ {
+ $violations = [];
+ if (preg_match_all(self::CALL_START_PATTERN, $line, $matches, PREG_OFFSET_CAPTURE)) {
+ foreach ($matches[0] as $matchInfo) {
+ $fullMatch = $matchInfo[0];
+ $offset = $matchInfo[1];
+ $parenPos = $offset + strlen($fullMatch) - 1;
+ $inner = self::extractBalancedArgs($line, $parenPos);
+ if ($inner === null) {
+ continue;
+ }
+ $argCount = self::countArguments($inner);
+ if ($argCount <= 2) {
+ $callText = substr($line, $offset, strlen($fullMatch) + strlen($inner) + 1);
+ $violations[] = sprintf(' Line %d: %s', $lineNum + 1, trim($callText));
+ }
+ }
+ }
+ return $violations;
+ }🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 56-56: Refactor this function to reduce its Cognitive Complexity from 18 to the 15 allowed.
🪛 PHPMD (2.15.0)
[warning] 56-106: The method noHashlessRequestCalls() has a Cyclomatic Complexity of 10. The configured cyclomatic complexity threshold is 10. (undefined)
(CyclomaticComplexity)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/hardening/RequestHashPinningTest.php` around lines 54 -
106, The noHashlessRequestCalls method is over-complex; extract the inner
match-handling logic into a private helper to reduce nesting and cognitive
complexity. Create a helper (e.g., handleRequestCallMatch or
collectHashlessRequestCall) that accepts the original line, the match full text
and offset, the line number and a reference to $violations; move the parenPos
calculation, call to extractBalancedArgs, argCount via countArguments, and the
violation push (when argCount <= 2) into that helper. Replace the inner loop
body in noHashlessRequestCalls with a call to this helper for each match so the
main test reads as a simple loop over matches and delegates parsing/violation
logic to the new method; continue using CALL_START_PATTERN, extractBalancedArgs
and countArguments as-is.
| // Pattern: Request::getXxx(...) ?: Request::getXxx(...) | ||
| $elvisPattern = '/Request::(?:getString|getInt|getCmd|getWord|getFloat|getArray|getVar|getBool|getEmail|getUrl|getText)\s*\([^)]*\)\s*\?:\s*Request::/'; |
There was a problem hiding this comment.
Regex [^)]* may produce false negatives for nested parentheses.
The elvis pattern uses [^)]* which won't match calls containing nested parentheses (e.g., Request::getInt('id', max(0, $x), 'GET')). This could miss violations in edge cases.
For the current codebase this is likely fine, but worth documenting or hardening if you expect more complex argument expressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/hardening/RequestHashPinningTest.php` around lines 121 -
122, The regex stored in $elvisPattern uses [^)]* which fails on nested
parentheses in argument lists (so Request::getInt(... max(0, $x) ...) will not
match); replace the naive [^)]* with a more robust pattern that allows nested
parentheses or a balanced group (e.g., use a non-greedy match with optional
nested-parentheses handling or a recursive pattern if PCRE is enabled) so the
pattern still targets the Request::getString|getInt|... call followed by ?:
Request::; update the $elvisPattern variable accordingly to ensure it matches
calls with complex arguments.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1650 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Request::getXxx()calls to explicit'GET'or'POST'across 8 files, using dual-sourcehasVarpattern foropparameters that can come from either method?:elvis operators on zero-valid fields inprofile/search.phpwithhasVarternary to prevent0being treated as falsyFiles modified
system/admin/groups/main.php,comments/main.php,preferences/main.php,smilies/main.php,maintenance/main.php,userrank/main.phpprofile/admin/category.php,profile/search.phpTest plan
Summary by CodeRabbit
Bug Fixes
Tests