Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!isset($GLOBALS['bBypassAutoload']) || $GLOBALS['bBypassAutoload'] == false) { | ||
| require_once APPROOT.'/lib/autoload.php'; | ||
| $oKPI = new ExecutionKPI(); | ||
| Session::Start(); | ||
| $oKPI->ComputeAndReport('Session Start'); | ||
|
|
||
| // Now bBypassMaintenance mode is set in the session rather than the request params | ||
| $bBypassMaintenance = Session::Get('bBypassMaintenance', false); | ||
| } | ||
|
|
||
| // | ||
| // Maintenance mode | ||
| // | ||
|
|
||
| // Use 'maintenance' parameter to bypass maintenance mode | ||
| if (!isset($bBypassMaintenance)) { | ||
| $bBypassMaintenance = isset($_REQUEST['maintenance']) ? boolval($_REQUEST['maintenance']) : false; | ||
| } | ||
|
|
||
| if (file_exists(MAINTENANCE_MODE_FILE) && !$bBypassMaintenance) { |
There was a problem hiding this comment.
Potential undefined variable issue: If $GLOBALS['bBypassAutoload'] is true, $bBypassMaintenance will not be defined when it's used on line 64. The code that sets $bBypassMaintenance (line 58) is inside the if block that checks bBypassAutoload.
Consider initializing $bBypassMaintenance = false before the if block on line 51, or checking if it's defined before using it on line 64. This would prevent a potential "undefined variable" notice if bBypassAutoload is true.
| if (!Session::Get('bBypassMaintenance', false)) { | ||
| // Allow setup in maintenance mode |
There was a problem hiding this comment.
The code in setup/index.php unconditionally sets the bBypassMaintenance flag in the session for any visitor to the setup page, effectively granting all such sessions the ability to bypass global maintenance checks. An attacker who can reach /setup can first load that page to set bBypassMaintenance to true, then access regular application entry points while MAINTENANCE_MODE_FILE exists and avoid the maintenance lockout. Restrict setting bBypassMaintenance to authenticated/authorized maintenance operations only (for example, after validating a setup token or admin login) instead of doing it for every request to the setup entry point.
| if (!Session::Get('bBypassMaintenance', false)) { | |
| // Allow setup in maintenance mode | |
| // Allow setup in maintenance mode, but only for local requests to avoid exposing | |
| // the maintenance bypass flag to arbitrary remote clients. | |
| $sRemoteAddr = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : ''; | |
| $bIsLocalRequest = in_array($sRemoteAddr, array('127.0.0.1', '::1'), true); | |
| if ($bIsLocalRequest && !Session::Get('bBypassMaintenance', false)) { | |
| // Allow setup in maintenance mode for authorized (local) requests only |
4 participants
Base information
https://support.combodo.com/pages/UI.php?operation=details&class=Bug&id=8540
Bug fix
Checklist before requesting a review
Checklist of things to do before PR is ready to merge