Fix/273 hardening - #179
Conversation
A GdImage frees itself when its last reference goes away (PHP 8.0+), so imagedestroy() has been a no-op for years and PHP 8.5 deprecates it. Delete the calls in image.php, the image CAPTCHA renderer and Protector's module_icon.php, unsetting where a variable or property held the image. The CAPTCHA renderer's branch-on-destroy-failure unwinds to a plain unset, and its required-GD-function check no longer lists the function. The remaining occurrences are inside the vendored TCPDF, which is upstream's to change.
With ONLY_LOCAL_IMAGES flipped to false, the raw request URL reached getimagesize() and file_get_contents(), honoring http://, phar:// and data:// -- an SSRF and phar-deserialization surface. The branch had also never worked for anyone: it truncated every rooted URL to a single character (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant), so no real-world behavior is lost. The constant stays and the branch now refuses the request, so a fork that flips it fails closed instead of fatal.
profile's user.php tore the session down on a bare GET, so any third-party page could end a visitor's session at will (forced-logout CSRF). A tokenless request now renders a POST confirmation carrying the token instead of acting immediately, which keeps every existing user.php?op=logout link working -- including the ones baked into cached theme and block templates -- at the cost of one extra click. Adds _US_SURETOLOGOUT to the core user language file and records it in lang_diff.txt.
…on headers The pm and profile preload events and profile register.php's activation redirect appended $_SERVER['QUERY_STRING'] verbatim to their Location targets -- eight sites. PHP already blocks CRLF in headers, so this was never header splitting, but an attacker-shaped query string reflected unchecked invites cache-poisoning and phishing parameter injection. The string is now appended only when it fits a conservative character allowlist (all core xoops_redirect producers urlencode their values, so legitimate flows pass unchanged) and is dropped otherwise.
Reviewer's GuideHardens image and redirect handling by removing deprecated GD cleanup APIs, disabling unsafe remote-image processing, filtering reflected query strings, and adding CSRF protection to logout while updating release metadata. Sequence diagram for CSRF-protected logoutsequenceDiagram
participant Browser
participant User as user.php
participant Security as xoopsSecurity
participant Session as sess_handler
Browser->>User: GET user.php?op=logout
User->>Security: check()
alt token missing or invalid
User->>Browser: xoops_confirm(['op' => 'logout'], 'user.php', _US_SURETOLOGOUT)
Browser-->>User: POST confirmation with token
User->>Security: check()
end
User->>Session: regenerate_id(true)
User-->>Browser: logout response
Sequence diagram for filtered redirect query stringssequenceDiagram
participant Request
participant Preload as PmCorePreload_or_ProfileCorePreload
participant Redirect as TargetPage
Request->>Preload: eventCore...Start(args)
Preload->>Preload: filteredQueryString()
alt query string matches allowlist and is at most 512 characters
Preload->>Redirect: header(location with filtered query string)
else absent or invalid query string
Preload->>Redirect: header(location without query string)
end
Flow diagram for fail-closed image validationflowchart TD
A["imageFilenameCheck(imageUrl)"] --> B{Local image path?}
B -- No --> C["exitInvalidRequest()"]
B -- Yes --> D["getimagesize(imagePath)"]
D --> E{Readable and within limits?}
E -- No --> C
E -- Yes --> F[Process local image]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe release updates GD image cleanup for PHP 8.5 compatibility, disables remote image processing, validates redirect query strings, and adds session-token protection with confirmation for logout requests. ChangesPHP compatibility and security updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change hardens image, redirect, and logout handling; no actionable merge-blocking risk remains at the current head beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant UserBrowser
participant ProfileLogout
participant SessionManager
UserBrowser->>ProfileLogout: Request logout without valid token
ProfileLogout-->>UserBrowser: Render POST confirmation form
UserBrowser->>ProfileLogout: Submit confirmation with token
ProfileLogout->>SessionManager: Validate session token
SessionManager-->>ProfileLogout: Return validation result
ProfileLogout-->>UserBrowser: Complete logout and redirect
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #179 +/- ##
============================================
+ Coverage 20.18% 20.24% +0.06%
- Complexity 8217 8233 +16
============================================
Files 673 674 +1
Lines 44242 44297 +55
============================================
+ Hits 8930 8970 +40
- Misses 35312 35327 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. This changes the logout trust boundary by requiring a valid session token and routing tokenless requests through a confirmation flow, so a mistaken policy or check affects every logout request immediately. Reverting restores the old flow, but sessions already invalidated or users already affected by an incorrect redirect or logout behavior cannot be fully restored by the revert.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThe PR hardens image and logout handling, rebuilds reflected redirect query strings safely, updates GD usage for current PHP versions, and prepares the XOOPS 2.7.3 release documentation.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the scope of the previous redirect thread. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| htdocs/include/file_safety.php | Adds the shared query-string rebuilding helper; the previously reported 1072-byte login redirect now survives the redirect hop. |
| htdocs/modules/profile/preloads/core.php | Uses the shared helper when forwarding profile routes, including the login path involved in the previous thread. |
| tests/unit/htdocs/include/RebuildQueryStringTest.php | Covers safe reconstruction, malformed input, the length policy, and round-trip preservation of the previously reported long redirect. |
| htdocs/image.php | Closes remote image handling and replaces deprecated explicit GD destruction with reference cleanup. |
| htdocs/modules/profile/user.php | Adds a confirmation step for logout requests that do not carry a valid session token. |
Reviews (4): Last reviewed commit: "fix(profile): give the logout confirmati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Hardens several request/redirect and image-handling paths in XOOPS 2.7.x to reduce exposure to unsafe inputs and align GD cleanup with newer PHP behavior.
Changes:
- Require CSRF token (or a token-backed confirmation POST) before completing logout.
- Stop reflecting raw
QUERY_STRINGinto multiple redirectLocationheaders by filtering it before appending. - Remove/close remote-image handling in
image.phpand replaceimagedestroy()calls with reference cleanup (unset) for PHP 8+ GD objects.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/xoops_lib/modules/protector/module_icon.php | Replace imagedestroy() with reference cleanup for generated module icons. |
| htdocs/modules/profile/user.php | Add token check + confirmation step to prevent tokenless forced-logout CSRF. |
| htdocs/modules/profile/register.php | Filter query string before redirecting to activation endpoint. |
| htdocs/modules/profile/preloads/core.php | Centralize filtered query string appending for profile-related preload redirects. |
| htdocs/modules/pm/preloads/core.php | Centralize filtered query string appending for PM preload redirects. |
| htdocs/language/english/user.php | Add new logout confirmation localization string. |
| htdocs/image.php | Close remote-image branch; tighten local-only assumptions; switch GD cleanup to unset. |
| htdocs/class/captcha/image/scripts/image.php | Replace imagedestroy() calls with reference cleanup for CAPTCHA images. |
| htdocs/class/captcha/image.php | Remove imagedestroy from required GD function checks. |
| docs/lang_diff.txt | Document the added language define for logout confirmation. |
| docs/changelog.270.txt | Document security hardening + GD cleanup changes for 2.7.3. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | ||
| $queryString = preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; | ||
| header('location: ./activate.php' . $queryString); |
There was a problem hiding this comment.
Fixed in b180fcb. The reflected string is now rebuilt with parse_str() + http_build_query(PHP_QUERY_RFC3986) instead of being gated by the character class, so every emitted byte is RFC 3986-safe or a valid escape by construction -- a malformed "%ZZ" or trailing "%" reaches the Location header as "%25ZZ" / "%25", and behavior no longer depends on how a cache or user agent normalizes invalid escapes.
| { | ||
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | ||
|
|
||
| return preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; |
There was a problem hiding this comment.
Fixed in b180fcb. The reflected string is now rebuilt with parse_str() + http_build_query(PHP_QUERY_RFC3986) instead of being gated by the character class, so every emitted byte is RFC 3986-safe or a valid escape by construction -- a malformed "%ZZ" or trailing "%" reaches the Location header as "%25ZZ" / "%25", and behavior no longer depends on how a cache or user agent normalizes invalid escapes.
| { | ||
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | ||
|
|
||
| return preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; |
There was a problem hiding this comment.
Fixed in b180fcb. The reflected string is now rebuilt with parse_str() + http_build_query(PHP_QUERY_RFC3986) instead of being gated by the character class, so every emitted byte is RFC 3986-safe or a valid escape by construction -- a malformed "%ZZ" or trailing "%" reaches the Location header as "%25ZZ" / "%25", and behavior no longer depends on how a cache or user agent normalizes invalid escapes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Follow-up to the XOOPS#179 review: the character allowlist reflected malformed percent-escapes verbatim ("%ZZ" and a trailing "%" match the class -- Copilot, three sites) and its 512-byte cap dropped a long urlencoded xoops_redirect entirely, so a login lost its destination (Greptile P1, reproduced with a 1072-byte publisher search URL). The filter now parses the query string with parse_str() and re-emits it with http_build_query(PHP_QUERY_RFC3986), capped at 2000 bytes for header-size sanity: every reflected byte is RFC 3986-safe or a valid escape by construction, hostile input becomes inert encoding instead of costing the visitor their query, and the redirect target parses the rebuilt string identically to the original, since parse_str() mirrors PHP's own request parsing (round-trip equivalence probed by execution).
| private static function filteredQueryString() | ||
| { | ||
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | ||
| if ('' === $queryString || strlen($queryString) > 2000) { | ||
| return ''; | ||
| } | ||
| parse_str($queryString, $params); | ||
| $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); | ||
|
|
||
| return ('' === $rebuilt) ? '' : ('?' . $rebuilt); |
There was a problem hiding this comment.
Fixed in 4524341. The rebuild now lives once as xoops_rebuildQueryString() in include/file_safety.php -- the side-effect-free helper home that already hosts the redirect-safety functions, guarded with function_exists() like its neighbors -- and both preloads plus register.php delegate to it. The contract is pinned by tests/unit/htdocs/include/RebuildQueryStringTest.php: malformed escapes re-emitted as valid ones, every emitted byte RFC 3986-safe or a valid escape, unusable input dropped to '', and the long urlencoded xoops_redirect surviving with round-trip parse equivalence at the target.
| private static function filteredQueryString() | ||
| { | ||
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | ||
| if ('' === $queryString || strlen($queryString) > 2000) { | ||
| return ''; | ||
| } | ||
| parse_str($queryString, $params); | ||
| $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); | ||
|
|
||
| return ('' === $rebuilt) ? '' : ('?' . $rebuilt); |
There was a problem hiding this comment.
Fixed in 4524341. The rebuild now lives once as xoops_rebuildQueryString() in include/file_safety.php -- the side-effect-free helper home that already hosts the redirect-safety functions, guarded with function_exists() like its neighbors -- and both preloads plus register.php delegate to it. The contract is pinned by tests/unit/htdocs/include/RebuildQueryStringTest.php: malformed escapes re-emitted as valid ones, every emitted byte RFC 3986-safe or a valid escape, unusable input dropped to '', and the long urlencoded xoops_redirect surviving with round-trip parse equivalence at the target.
… test Review follow-up on XOOPS#179: the parse_str()/http_build_query() rebuild was duplicated verbatim in the pm and profile preloads and inline in register.php. It now lives once as xoops_rebuildQueryString() in include/file_safety.php -- the side-effect-free helper home already hosting the redirect-safety functions, guarded with function_exists() like its neighbors -- and all three call sites delegate to it. A new RebuildQueryStringTest pins the contract: malformed escapes re-emitted as valid ones, hostile input reduced to inert encoding, every emitted byte RFC 3986-safe or a valid escape, unusable input dropped to '', and the >512-byte urlencoded xoops_redirect surviving with round-trip parse equivalence at the target.
Move this branch's four changelog entries out of the RC 1 block into a new "2.7.3 Final" section, each now attributed to XOOPS#179. Split lang_diff.txt the same way: the SCEditor strings stay under 2.7.3-RC1 and the new _US_SURETOLOGOUT confirmation string gets its own 2.7.3-Final section, so translators see exactly what changed since the RC packs. Rewrite docs/README.txt and docs/RELEASE_POST.md from their 2.7.2 versions for 2.7.3 Final: security hardening and PHP 8.6 readiness as the headline, SCEditor and the file-based debug configuration as features, the xoops.org-proven reliability fixes, upgrade guidance (schema changes -> run the upgrade wizard; no mainfile.php changes needed), and thanks to CHCCD for testing the release candidates and reporting XOOPS#161, XOOPS#162 and XOOPS#163.
The v2.7.3-RC1 tag points at 5a7ae41 (2026-08-11); everything merged after it belongs to Final. Verified mechanically by diffing the changelog against its tag-time content: the seven post-tag entries -- is_long(), the constructor bare-returns and their repository-wide test pin, the XOOPS#169 session save-handler contract, the tplsets hardening (XOOPS#176), PathGuard (XOOPS#178), and the PR template (XOOPS#175) -- move from the RC 1 block into the Final block, and the RC 1 block's emptied Build section is removed. The same diff exposed two gaps, both fixed: the Database section header (dropped by an earlier edit, leaving the XOOPS#154 Criteria entries under Security) is restored, and the post-tag fixes that never had entries get them under Final -- the search show-all repair and request validation and the browse.php Cache-Control fix (reported by CHCCD in XOOPS#161, XOOPS#162, XOOPS#163) and the XOBJ_DTYPE_UNICODE_* deprecation notice. RELEASE_POST.md gains the matching Deprecations note, and RELEASE_POST_BBCODE.txt is the same announcement in XOOPS BBCode, ready for the xoops.org news post.
| parse_str($queryString, $params); | ||
| $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); | ||
|
|
||
| return ('' === $rebuilt) ? '' : ('?' . $rebuilt); |
There was a problem hiding this comment.
Refuted by execution (PHP 8.2.33 and 8.4.21): parse_str() discards parameters with an empty name entirely, so "=" and "=x" parse to [] and xoops_rebuildQueryString() returns '' -- the "?=" output is not reachable. This exact case is pinned in RebuildQueryStringTest ('bare equals' in the droppedStrings provider), which passes. The near miss is "%3D", which parse_str() decodes to a parameter NAMED "=" with an empty value; that re-emits as "?%3D=", which is well-formed and harmless. No change made.
| include $GLOBALS['xoops']->path('header.php'); | ||
| include __DIR__ . '/header.php'; | ||
| xoops_confirm(['op' => 'logout'], 'user.php', _US_SURETOLOGOUT); | ||
| include __DIR__ . '/footer.php'; | ||
| exit(); |
Review follow-up on XOOPS#179: xoops_loadLanguage() falls back to English only when a language pack's file is missing entirely, so a pack that predates 2.7.3 Final has a user.php without _US_SURETOLOGOUT -- and an undefined constant is a fatal Error on PHP 8, taking down the logout confirmation on every non-English site until translations catch up. Guard with defined() || define() at the use site, per the repo's bare-conditional-constants pattern, so such sites get the English string instead of a fatal.
Why
What
Verification
Checklist
Notes for the items marked "see notes": CONTRIBUTING.md - Pull request checklist notes
Summary by Sourcery
Harden XOOPS 2.7.3 against security threats, improve production reliability, and prepare the core for PHP 8.6 while adding optional editor and debugging enhancements.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes