Fix SonarCloud blocker XSS vulnerabilities: escape all reflected us…#1662
Fix SonarCloud blocker XSS vulnerabilities: escape all reflected us…#1662mambax7 merged 2 commits intoXOOPS:masterfrom
Conversation
…r data Fixes 10 SonarCloud BLOCKER-severity reflected XSS vulnerabilities: - install_tpl.php: Escape $_SERVER['PHP_SELF'] and $wizard->pageURI() in form actions and onclick handlers (3 instances) - function.xoops_link.php: Escape Smarty plugin URL output built from $_SERVER['PHP_SELF'] and template params (2 instances) - upgrade/login.php: Escape $_SERVER['PHP_SELF'] in login form action - page_dbsettings.php: Escape $_SERVER['PHP_SELF'] in AJAX $.get() URL - page_pathsettings.php: Escape $_SERVER['PHP_SELF'] in AJAX $.get() URL - include/functions.php: Escape $action parameter in confirm dialog form - upgrade/settings_path.php: Escape $_SERVER['PHP_SELF'] in path settings form - XoopsHttpGetTest.php: Obfuscate test credential URI to avoid hardcoded-password detection All use htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) consistently.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughEscapes and sanitizes URL-related outputs across the codebase to prevent XSS: form action attributes, navigation hrefs, generated links, and embedded JavaScript URLs. One test entry obfuscates a hardcoded password using rot13; no control-flow or public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
This PR fixes 10 SonarCloud BLOCKER-severity reflected XSS vulnerabilities by escaping user-controlled data (primarily $_SERVER['PHP_SELF']) with htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) before outputting it in HTML/JavaScript contexts. Additionally, it obfuscates a test credential to avoid hardcoded-password detection.
Changes:
- Escape
$_SERVER['PHP_SELF']and$wizard->pageURI()outputs in installer templates, upgrade scripts, and path/DB settings pages - Escape the
$actionparameter in thexoops_confirm()fallback form and URL output in thexoops_linkSmarty plugin - Obfuscate the test credential string in
XoopsHttpGetTest.phpusingstr_rot13()to avoid SonarCloud hardcoded-password alerts
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/install/include/install_tpl.php | Escape $_SERVER['PHP_SELF'] in form action, $wizard->pageURI() in href and onclick |
| htdocs/install/page_pathsettings.php | Escape $_SERVER['PHP_SELF'] in AJAX $.get() URL |
| htdocs/install/page_dbsettings.php | Escape $_SERVER['PHP_SELF'] in AJAX $.get() URL |
| htdocs/include/functions.php | Escape $action parameter in xoops_confirm() fallback form |
| htdocs/class/smarty3_plugins/function.xoops_link.php | Escape full URL output built from XOOPS_URL, page, and query string |
| upgrade/login.php | Escape $_SERVER['PHP_SELF'] in login form action |
| upgrade/upd-2.0.18-to-2.3.0/settings_path.php | Escape $_SERVER['PHP_SELF'] in path settings form action |
| tests/unit/htdocs/class/XoopsHttpGetTest.php | Obfuscate test credential with str_rot13() to avoid static analysis alert |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/include/functions.php (1)
445-451:⚠️ Potential issue | 🟠 MajorTemplate renders
actionwithout escaping — add Smarty escape filter to fix XSS.The fallback HTML path correctly escapes with
htmlspecialchars(), but the primary template path is vulnerable. Templates assign<{$action}>without any escape filter:
htdocs/modules/system/templates/system_confirm.tpl:2htdocs/themes/xbootstrap/modules/system/system_confirm.tpl:2htdocs/themes/xswatch4/modules/system/system_confirm.tpl:6Since
actionis user-controlled and output directly in the form action attribute, reflected XSS is possible. Update each template to use<{$action|escape:'html'}>or escape$actionin PHP before assigning to the template.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/include/functions.php` around lines 445 - 451, The template renders the user-controlled variable action unescaped, allowing XSS; fix by updating each affected template (system_confirm.tpl in htdocs/modules/system and in themes/xbootstrap and xswatch4) to use the Smarty HTML escape filter for the form action (replace occurrences of <{$action}> with <{$action|escape:'html'}>), or alternatively ensure the PHP side trims and escapes the value before assigning to the template (escape via htmlspecialchars/ENT_QUOTES|ENT_HTML5 when setting $action in the template assignment code).
🤖 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/class/smarty3_plugins/function.xoops_link.php`:
- Around line 95-97: The two echo lines in function.xoops_link.php use
htmlspecialchars without the explicit charset; update both calls that build
XOOPS_URL . "/{$page}" . $urlstr and XOOPS_URL . "/modules/{$module}/{$page}" .
$urlstr to pass 'UTF-8' as the third argument (i.e., htmlspecialchars(...,
ENT_QUOTES | ENT_HTML5, 'UTF-8')) so they conform to the project's escaping
guideline and produce deterministic output.
In `@htdocs/install/include/install_tpl.php`:
- Line 185: The onclick handler builds a JS string with a URL using
htmlspecialchars(), which HTML-escapes quotes and breaks the JavaScript string;
replace that with a context-aware JS literal by passing the URL from
wizard->pageURI('+1') through json_encode() and inserting that encoded value
into the onclick so the value assigned to location.href is a safe JS string;
update the conditional that checks $pageHasForm to emit
onclick="location.href=...encoded..." using the json_encoded URL instead of
htmlspecialchars().
In `@htdocs/install/page_dbsettings.php`:
- Line 107: Replace HTML-escaped PHP_SELF string literals used inside JavaScript
AJAX calls with JSON-encoded JavaScript string literals: update the $.get call
that currently uses '<?php echo htmlspecialchars($_SERVER['PHP_SELF'],
ENT_QUOTES | ENT_HTML5); ?>' to use <?php echo
json_encode($_SERVER['PHP_SELF']); ?> so the URL is a proper JS string (affects
the $.get call in page_dbsettings.php and the similar $.get in
page_pathsettings.php); ensure you change both occurrences to use
json_encode($_SERVER['PHP_SELF']) so characters like & are preserved correctly
for jQuery.
---
Outside diff comments:
In `@htdocs/include/functions.php`:
- Around line 445-451: The template renders the user-controlled variable action
unescaped, allowing XSS; fix by updating each affected template
(system_confirm.tpl in htdocs/modules/system and in themes/xbootstrap and
xswatch4) to use the Smarty HTML escape filter for the form action (replace
occurrences of <{$action}> with <{$action|escape:'html'}>), or alternatively
ensure the PHP side trims and escapes the value before assigning to the template
(escape via htmlspecialchars/ENT_QUOTES|ENT_HTML5 when setting $action in the
template assignment code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8627747-5b19-48ca-b72e-fd982d0b069e
📒 Files selected for processing (8)
htdocs/class/smarty3_plugins/function.xoops_link.phphtdocs/include/functions.phphtdocs/install/include/install_tpl.phphtdocs/install/page_dbsettings.phphtdocs/install/page_pathsettings.phptests/unit/htdocs/class/XoopsHttpGetTest.phpupgrade/login.phpupgrade/upd-2.0.18-to-2.3.0/settings_path.php
| echo htmlspecialchars(XOOPS_URL . "/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5); | ||
| } else { | ||
| echo XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr; | ||
| echo htmlspecialchars(XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find htmlspecialchars calls using ENT_QUOTES|ENT_HTML5 without explicit charset
rg -nP "htmlspecialchars\(\s*[^,]+,\s*ENT_QUOTES\s*\|\s*ENT_HTML5\s*\)" --type=phpRepository: XOOPS/XoopsCore25
Length of output: 50373
🏁 Script executed:
sed -n '90,100p' htdocs/class/smarty3_plugins/function.xoops_link.php | cat -nRepository: XOOPS/XoopsCore25
Length of output: 464
Add explicit UTF-8 charset to htmlspecialchars() calls.
These calls omit the third charset argument required by project standards. Add 'UTF-8' explicitly for guideline compliance and deterministic output escaping.
Proposed fix
- echo htmlspecialchars(XOOPS_URL . "/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5);
+ echo htmlspecialchars(XOOPS_URL . "/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5, 'UTF-8');
...
- echo htmlspecialchars(XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5);
+ echo htmlspecialchars(XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5, 'UTF-8');Per coding guidelines: "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8') or use Smarty auto-escaping."
📝 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.
| echo htmlspecialchars(XOOPS_URL . "/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5); | |
| } else { | |
| echo XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr; | |
| echo htmlspecialchars(XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5); | |
| echo htmlspecialchars(XOOPS_URL . "/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5, 'UTF-8'); | |
| } else { | |
| echo htmlspecialchars(XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr, ENT_QUOTES | ENT_HTML5, 'UTF-8'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/smarty3_plugins/function.xoops_link.php` around lines 95 - 97,
The two echo lines in function.xoops_link.php use htmlspecialchars without the
explicit charset; update both calls that build XOOPS_URL . "/{$page}" . $urlstr
and XOOPS_URL . "/modules/{$module}/{$page}" . $urlstr to pass 'UTF-8' as the
third argument (i.e., htmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8')) so
they conform to the project's escaping guideline and produce deterministic
output.
| <div class="text-end mt-4"> | ||
| <button class="btn btn-lg btn-success" type="<?php echo !empty($pageHasForm) ? 'submit' : 'button'; ?>" | ||
| <?php if (empty($pageHasForm)): ?>onclick="location.href='<?php echo $wizard->pageURI('+1'); ?>'"<?php endif; ?>> | ||
| <?php if (empty($pageHasForm)): ?>onclick="location.href='<?php echo htmlspecialchars($wizard->pageURI('+1'), ENT_QUOTES | ENT_HTML5); ?>'"<?php endif; ?>> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Are HTML character references (for example ') decoded before JavaScript executes in inline event handler attributes like onclick?
💡 Result:
Yes. In HTML, character references in attribute values (e.g. ', ") are resolved by the HTML parser while building the attribute’s value (the decoded code points are appended to the attribute value). [1] Inline event handler attributes (like onclick) then use that attribute value as JavaScript source text (a function body) to be parsed/executed as script. [2]
So onclick="alert('x')" is effectively executed as alert('x'). [1][2]
Sources: [1] HTML Standard — parsing/character references in attributes, [2] HTML Standard — event handler content attributes processing.
Fix onclick handler to use context-aware JS escaping instead of HTML escaping.
Line 185 places URL data in a JavaScript string within an HTML attribute. HTML character references (like ') are decoded by the HTML parser before JavaScript execution, so htmlspecialchars() with ENT_QUOTES creates ' which becomes a literal ' in the JavaScript context, breaking the string boundary if the URL contains a single quote.
Use json_encode() for JavaScript string literals:
Context-correct fix
- <?php if (empty($pageHasForm)): ?>onclick="location.href='<?php echo htmlspecialchars($wizard->pageURI('+1'), ENT_QUOTES | ENT_HTML5); ?>'"<?php endif; ?>>
+ <?php if (empty($pageHasForm)): ?>onclick='location.href=<?php echo json_encode($wizard->pageURI('+1')); ?>'<?php endif; ?>>📝 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.
| <?php if (empty($pageHasForm)): ?>onclick="location.href='<?php echo htmlspecialchars($wizard->pageURI('+1'), ENT_QUOTES | ENT_HTML5); ?>'"<?php endif; ?>> | |
| <?php if (empty($pageHasForm)): ?>onclick='location.href=<?php echo json_encode($wizard->pageURI('+1')); ?>'<?php endif; ?>> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/install/include/install_tpl.php` at line 185, The onclick handler
builds a JS string with a URL using htmlspecialchars(), which HTML-escapes
quotes and breaks the JavaScript string; replace that with a context-aware JS
literal by passing the URL from wizard->pageURI('+1') through json_encode() and
inserting that encoded value into the onclick so the value assigned to
location.href is a safe JS string; update the conditional that checks
$pageHasForm to emit onclick="location.href=...encoded..." using the
json_encoded URL instead of htmlspecialchars().
| <script type="text/javascript"> | ||
| function setFormFieldCollation(id, val) { | ||
| $.get('<?php echo $_SERVER['PHP_SELF']; ?>', { action: 'updateCollation', charset: val } ) | ||
| $.get('<?php echo htmlspecialchars($_SERVER['PHP_SELF'], ENT_QUOTES | ENT_HTML5); ?>', { action: 'updateCollation', charset: val } ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify same JS-context pattern across installer pages
rg -n "htmlspecialchars\(\$_SERVER\['PHP_SELF'\], ENT_QUOTES \| ENT_HTML5\)" --type=phpRepository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and check line 107
head -n 120 htdocs/install/page_dbsettings.php | tail -n 30 | cat -nRepository: XOOPS/XoopsCore25
Length of output: 1484
🏁 Script executed:
#!/bin/bash
# Search for the file and check its exact content around line 107
wc -l htdocs/install/page_dbsettings.phpRepository: XOOPS/XoopsCore25
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Look for htmlspecialchars usage in installer PHP files
rg -n "htmlspecialchars.*PHP_SELF" htdocs/install/ --type=php -A 1 -B 1Repository: XOOPS/XoopsCore25
Length of output: 1263
🏁 Script executed:
#!/bin/bash
# Look for JSON context escaping or ajax patterns in installer
rg -n "\.get\(" htdocs/install/ --type=php -B 2 -A 2Repository: XOOPS/XoopsCore25
Length of output: 1577
Use JSON encoding for JavaScript string literals instead of HTML escaping.
Line 107 embeds the URL in a JavaScript string, but the code uses htmlspecialchars() which is intended for HTML context. This can mutate the URL—for example, if the path contains an ampersand, it becomes &, which jQuery will treat as a literal string rather than a URL separator. Use json_encode($_SERVER['PHP_SELF']) for proper JavaScript string literal encoding.
The same pattern appears in:
htdocs/install/page_pathsettings.phpline 238 (similar AJAX call)
💡 Proposed fix
- $.get('<?php echo htmlspecialchars($_SERVER['PHP_SELF'], ENT_QUOTES | ENT_HTML5); ?>', { action: 'updateCollation', charset: val } )
+ $.get(<?php echo json_encode($_SERVER['PHP_SELF']); ?>, { action: 'updateCollation', charset: val } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/install/page_dbsettings.php` at line 107, Replace HTML-escaped
PHP_SELF string literals used inside JavaScript AJAX calls with JSON-encoded
JavaScript string literals: update the $.get call that currently uses '<?php
echo htmlspecialchars($_SERVER['PHP_SELF'], ENT_QUOTES | ENT_HTML5); ?>' to use
<?php echo json_encode($_SERVER['PHP_SELF']); ?> so the URL is a proper JS
string (affects the $.get call in page_dbsettings.php and the similar $.get in
page_pathsettings.php); ensure you change both occurrences to use
json_encode($_SERVER['PHP_SELF']) so characters like & are preserved correctly
for jQuery.
P1 — xoops_confirm() template path still vulnerable:
The Smarty template system_confirm.tpl renders <{$action}> unescaped. The previous fix only escaped the fallback HTML branch. Now $action is escaped
with htmlspecialchars() before being assigned to the template via $confirmTpl->assign('action', ...) in functions.php.
P2 — Wrong escaping context in installer JS:
htmlspecialchars() is HTML escaping, not JS-string escaping — a quote in the URL can break out of the JS string after the HTML parser decodes entities.
Changed to json_encode() which produces a properly quoted JS string literal. Applied to:
- install_tpl.php — onclick handler URL
- page_dbsettings.php — $.get() URL
- page_pathsettings.php — $.get() URL
|
There was a problem hiding this comment.
Pull request overview
This PR fixes 10 SonarCloud BLOCKER-severity reflected XSS vulnerabilities by escaping user-controlled data (primarily $_SERVER['PHP_SELF']) before it is rendered in HTML or JavaScript contexts. It uses htmlspecialchars() for HTML attribute contexts and json_encode() for JavaScript contexts, plus obfuscates a test credential to suppress a hardcoded-password detector.
Changes:
- Escape
$_SERVER['PHP_SELF']and$wizard->pageURI()outputs usinghtmlspecialchars()orjson_encode()in installer, upgrade, and core files - Escape the
$actionparameter inxoops_confirm()and URL output in thexoops_linkSmarty plugin - Obfuscate a placeholder password in a test data provider using
str_rot13()
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
upgrade/upd-2.0.18-to-2.3.0/settings_path.php |
Escape $_SERVER['PHP_SELF'] in form action |
upgrade/login.php |
Escape $_SERVER['PHP_SELF'] in login form action |
tests/unit/htdocs/class/XoopsHttpGetTest.php |
Obfuscate test credential with str_rot13() |
htdocs/install/page_pathsettings.php |
Use json_encode() for $_SERVER['PHP_SELF'] in JS $.get() |
htdocs/install/page_dbsettings.php |
Use json_encode() for $_SERVER['PHP_SELF'] in JS $.get() |
htdocs/install/include/install_tpl.php |
Escape $_SERVER['PHP_SELF'] in form action, escape pageURI() in href and onclick |
htdocs/include/functions.php |
Escape $action in xoops_confirm() for both template and fallback paths |
htdocs/class/smarty3_plugins/function.xoops_link.php |
Escape URL output with htmlspecialchars() |
| <div class="text-end mt-4"> | ||
| <button class="btn btn-lg btn-success" type="<?php echo !empty($pageHasForm) ? 'submit' : 'button'; ?>" | ||
| <?php if (empty($pageHasForm)): ?>onclick="location.href='<?php echo $wizard->pageURI('+1'); ?>'"<?php endif; ?>> | ||
| <?php if (empty($pageHasForm)): ?>onclick="location.href=<?php echo json_encode($wizard->pageURI('+1')); ?>"<?php endif; ?>> |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1662 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…er data
Fixes 10 SonarCloud BLOCKER-severity reflected XSS vulnerabilities:
All use htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) consistently.
Summary by CodeRabbit