fix(installer): fix broken Next button on non-form pages#1668
fix(installer): fix broken Next button on non-form pages#1668mambax7 merged 2 commits intoXOOPS:masterfrom
Conversation
json_encode() wraps the URL in double quotes, which clash with the HTML onclick attribute's double quotes, causing the browser to parse the attribute incorrectly. The Next button becomes non-functional, blocking the installer at page_start.php. Use single-quoted JS string inside the double-quoted HTML attribute instead.
WalkthroughAdjusted the install wizard "Next" button inline JavaScript so the computed Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
🚥 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
Fixes the XOOPS installer “Next” button on non-form pages by correcting how the target URL is embedded into the button’s onclick handler so the browser parses it correctly and navigation works (notably on page_start.php).
Changes:
- Update installer template to generate a valid
onclicknavigation expression for non-form wizard pages.
| <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 json_encode($wizard->pageURI('+1')); ?>"<?php endif; ?>> | ||
| <?php if (empty($pageHasForm)): ?>onclick="location.href='<?php echo $wizard->pageURI('+1'); ?>'"<?php endif; ?>> |
There was a problem hiding this comment.
The new onclick builds a JavaScript string using a raw $wizard->pageURI('+1') value without JS/HTML escaping. Since pageURI() incorporates $_SERVER['HTTP_HOST']/PHP_SELF (see installwizard::baseLocation()), a crafted Host header or unexpected characters (e.g. a single quote) could break out of the JS string and/or attribute and enable XSS. Prefer generating the URL as a proper JS string via json_encode(...) and avoid the quote-collision by switching the HTML attribute to single quotes (or otherwise escaping for both JS + HTML contexts).
| <?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 #1668 +/- ##
=============================================
+ Coverage 0 19.19% +19.19%
- Complexity 0 7448 +7448
=============================================
Files 0 620 +620
Lines 0 39085 +39085
=============================================
+ Hits 0 7501 +7501
- Misses 0 31584 +31584 ☔ 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: 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 `@htdocs/install/include/install_tpl.php`:
- Line 185: Escape the URL output in the onclick attribute by wrapping the
$wizard->pageURI('+1') call in htmlspecialchars to prevent XSS; replace the raw
echo with htmlspecialchars($wizard->pageURI('+1'), ENT_QUOTES | ENT_HTML5)
(keeping the single-quote attribute form) so the onclick uses a safely-escaped
URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 499029ef-a38d-4677-9bf6-fc086b3fdd2c
📒 Files selected for processing (1)
htdocs/install/include/install_tpl.php
Use json_encode() for proper JS escaping and single-quoted HTML attribute to avoid double-quote clash. Addresses review feedback from Copilot, CodeRabbit, and SonarCloud about unsanitized user-controllable data from $_SERVER in pageURI().
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
htdocs/install/include/install_tpl.php (1)
185-185:⚠️ Potential issue | 🟠 MajorLine 185:
onclickremains unsafe in HTML attribute context.
json_encode()alone is not sufficient here. Because the attribute is single-quoted, an apostrophe inpageURI()output (derived from$_SERVER['HTTP_HOST']) can still break the attribute and reopen XSS/malformed-HTML risk.🔧 Proposed fix
- <?php if (empty($pageHasForm)): ?>onclick='location.href=<?php echo json_encode($wizard->pageURI("+1")); ?>'<?php endif; ?>> + <?php if (empty($pageHasForm)): ?>onclick="location.href=<?php echo htmlspecialchars(json_encode($wizard->pageURI('+1'), JSON_HEX_APOS), ENT_QUOTES, 'UTF-8'); ?>"<?php endif; ?>>In PHP, does json_encode() escape single quotes by default, and how does JSON_HEX_APOS affect the output?As per coding guidelines: "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8') or use Smarty auto-escaping".
🤖 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 attribute uses json_encode($wizard->pageURI("+1")) which doesn't guarantee safety for single-quoted HTML attributes (an apostrophe in pageURI from $_SERVER['HTTP_HOST'] can break the attribute); update the output for the onclick in install_tpl.php to safely escape the URI — either run the pageURI result through htmlspecialchars(..., ENT_QUOTES, 'UTF-8') before emitting or call json_encode with JSON_HEX_APOS and still wrap with htmlspecialchars to be extra-safe; adjust the conditional that checks $pageHasForm and the onclick emission around $wizard->pageURI("+1") so the rendered attribute cannot be broken by unescaped apostrophes or other special chars.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@htdocs/install/include/install_tpl.php`:
- Line 185: The onclick attribute uses json_encode($wizard->pageURI("+1")) which
doesn't guarantee safety for single-quoted HTML attributes (an apostrophe in
pageURI from $_SERVER['HTTP_HOST'] can break the attribute); update the output
for the onclick in install_tpl.php to safely escape the URI — either run the
pageURI result through htmlspecialchars(..., ENT_QUOTES, 'UTF-8') before
emitting or call json_encode with JSON_HEX_APOS and still wrap with
htmlspecialchars to be extra-safe; adjust the conditional that checks
$pageHasForm and the onclick emission around $wizard->pageURI("+1") so the
rendered attribute cannot be broken by unescaped apostrophes or other special
chars.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71edd4c4-8063-4a7c-9b4b-90eaf4a81b39
📒 Files selected for processing (1)
htdocs/install/include/install_tpl.php
Summary
json_encode()on the Next button'sonclickwraps the URL in double quotes, which clash with the HTML attribute's double quotesonclick="location.href="http://..."as a truncated attribute, making the button non-functionalpage_start.php— users cannot proceed past step 2Test plan
page_start.php(and all other non-form pages)Summary by CodeRabbit