Refactor: Protect GUI from fatal PHP errors#2421
Conversation
WalkthroughLayouts and PageBuilder now prepare generated eval payloads ($evalContent/$evalFile) and either eval them directly when an Eval flag is truthy or delegate execution to a new buffered, guarded helper ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Browser
participant Layout as DefaultPageLayout (PHP)
participant EvalH as evalContent.php
participant Log as Logger
UI->>Layout: Request page/button render
Note right of Layout: prepare $evalContent and $evalFile
alt Eval flag is true
Layout->>Layout: eval($evalContent)
else Eval flag is false
Layout->>EvalH: include evalContent.php (buffered, guarded eval)
rect rgb(240,248,255)
Note right of EvalH: start output buffer\ninstall error handler\nexecute eval
end
alt eval succeeds
EvalH-->>Layout: output flushed, $evalSuccess = true
else eval fails
EvalH->>Log: log error with $evalFile context
EvalH-->>Layout: $evalSuccess = false (console.error emitted)
end
end
Layout-->>UI: Rendered content (based on eval outcome)
sequenceDiagram
autonumber
participant PB as PageBuilder::page_enabled
participant EvalH as evalContent.php
participant Log as Logger
PB->>PB: build $evalContent (Cond), set $evalFile, $evalNoBanner=true
PB->>EvalH: include evalContent.php (guarded eval)
alt eval succeeds
EvalH-->>PB: $evalSuccess = true (enabled remains)
else eval fails
EvalH->>Log: log failure with $evalFile
EvalH-->>PB: $evalSuccess = false
end
PB-->>PB: return ($enabled && $evalSuccess)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php (1)
46-76: Correct evalContent include path across this file.Both includes on Line 51 and Line 75 target
$docroot/webGui/.../evalContent.php, but the helper actually ships underplugins/dynamix/include/.... The include therefore fails and any tab content withoutEval="true"never runs, breaking the tabs. Update both spots (and any others) to reference the real location.- include "$docroot/webGui/include/DefaultPageLayout/evalContent.php"; + include "$docroot/plugins/dynamix/include/DefaultPageLayout/evalContent.php";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emhttp/plugins/dynamix.docker.manager/AddContainer.page(1 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout.php(2 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php(2 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php(1 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-30T00:50:13.286Z
Learnt from: zackspear
PR: unraid/webgui#2212
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php:37-47
Timestamp: 2025-05-30T00:50:13.286Z
Learning: The .page template system in unraid/webgui is legacy implementation. There is no current appetite to change the eval() usage in .page template files like MainContentTabbed.php, even when security concerns are raised.
Applied to files:
emhttp/plugins/dynamix/include/DefaultPageLayout.php
🧬 Code graph analysis (3)
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php (1)
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContent.php (1)
generateContent(124-138)
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php (1)
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContent.php (1)
generateContent(124-138)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (2)
emhttp/plugins/dynamix/include/PageBuilder.php (2)
annotate(140-142)includePageStylesheets(131-138)emhttp/plugins/dynamix/include/Translations.php (1)
parse_text(63-66)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/PageBuilder.php(1 hunks)
🔇 Additional comments (1)
emhttp/plugins/dynamix/include/PageBuilder.php (1)
47-47: LGTM: Necessary global declaration.Adding
$docrootto the global declarations is required for the include path on line 54.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php (2)
8-31: Consider using a finally block for reliable buffer cleanup.While the current code cleans the output buffer in both success and error paths, using a
finallyblock would guarantee cleanup even if unexpected errors occur outside the explicit catch scenarios.Consider this refactor:
$evalSuccess = false; ob_start(); try { set_error_handler(function($severity, $message, $file, $line) use ($evalFile) { // Convert catchable errors to exceptions if ($severity & (E_USER_ERROR | E_RECOVERABLE_ERROR)) { throw new ErrorException($message, 0, $severity, $file, $line); } else { error_log("PHP Warning/notice in $evalFile"); } // Let other error types be handled normally return false; }); eval($evalContent); - restore_error_handler(); $evalSuccess = true; - ob_end_flush(); } catch (Throwable $e) { - restore_error_handler(); error_log("Error evaluating content in $evalFile: ".$e->getMessage()."\nStack trace:\n".$e->getTraceAsString()); ob_clean(); echo "<script>console.error('".htmlspecialchars("Error evaluating content in $evalFile: ".$e->getMessage())."');</script>"; - ob_end_flush(); +} finally { + restore_error_handler(); + if (ob_get_level() > 0) { + ob_end_flush(); + } }
1-7: Add validation for required variables.The file expects
$evalContentand$evalFileto be provided by the caller but doesn't validate their presence. Adding validation would make debugging easier if these variables are missing.Add validation at the beginning:
<?php // This evaluates the contents of PHP code. Has to be "included" because the code is evaluated in the context of the calling page. // $evalContent is the PHP code to evaluate. // $evalFile is the file that the code is being evaluated in // If an error occurs, a banner warning (disappearing in 10 seconds) is added to the page. // The PHP error logged will also include the path of the .page file for easier debugging +if (!isset($evalContent) || !isset($evalFile)) { + error_log("evalContent.php: Missing required variables (\$evalContent or \$evalFile)"); + return; +} + $evalSuccess = false;
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php (2)
17-26: Error handler checks uncatchable error types.The severity check on line 19 includes
E_ERROR,E_CORE_ERROR, andE_COMPILE_ERROR, which cannot be caught by custom error handlers in PHP—they terminate script execution before the handler runs. OnlyE_RECOVERABLE_ERRORandE_USER_ERRORfrom that list are actually catchable.Apply this diff to check only catchable error types:
set_error_handler(function($severity, $message, $file, $line) use ($evalFile) { - // Only convert errors (not warnings, notices, etc.) to exceptions - if ($severity & (E_ERROR | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR)) { + // Convert catchable errors to exceptions + if ($severity & (E_USER_ERROR | E_RECOVERABLE_ERROR)) { throw new ErrorException($message, 0, $severity, $file, $line); } else { error_log("PHP Warning/notice in $evalFile"); } - // Let warnings and notices be handled normally + // Let other error types be handled normally return false; });
33-33: Fix the typo in the error log message.There's an extra closing parenthesis before the colon in the error log message.
Apply this diff:
- error_log("Error evaluating content in $evalFile): ".$e->getMessage()."\nStack trace:\n".$e->getTraceAsString()); + error_log("Error evaluating content in $evalFile: ".$e->getMessage()."\nStack trace:\n".$e->getTraceAsString());
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php (1)
22-22: Log the actual warning/notice message for debugging.The error log currently only indicates a warning/notice occurred in the file but doesn't include the actual message, file location, or line number, making debugging difficult.
Apply this diff to include the full error details:
- error_log("PHP Warning/notice in $evalFile"); + error_log("PHP Warning/notice in $evalFile: $message at $file:$line");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php(1 hunks)
🔇 Additional comments (1)
emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php (1)
8-13: LGTM!The initialization and validation logic is correct. Setting
$evalSuccessto false by default and providing a fallback for$evalFileensures proper tracking and error reporting.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php (3)
5-6: Update the comment to match actual behavior.The comment states "a banner warning (disappearing in 10 seconds) is added to the page," but the current implementation (line 43) only outputs a
console.error()message—no banner UI is generated.Apply this diff to correct the comment:
-// If an error occurs, a banner warning (disappearing in 10 seconds) is added to the page. +// If an error occurs, an error message is logged to the browser console. // The PHP error logged will also include the path of the .page file for easier debugging
17-33: Error handler checks uncatchable error types.Line 23 includes
E_ERROR,E_CORE_ERROR, andE_COMPILE_ERRORin the fatal errors bitmask, but these error types cannot be caught by custom error handlers in PHP—they terminate script execution before the handler runs. OnlyE_RECOVERABLE_ERRORandE_USER_ERRORfrom this list are actually catchable.Apply this diff to check only catchable error types:
- $fatalErrors = E_ERROR | E_PARSE | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR; + // Only catchable error types: E_RECOVERABLE_ERROR and E_USER_ERROR + $fatalErrors = E_USER_ERROR | E_RECOVERABLE_ERROR;
43-43: Usejson_encodefor safer JavaScript output.Using
htmlspecialchars()for JavaScript string escaping is insufficient. If$evalFileor the error message contains single quotes, backslashes, or line breaks, it will break the JavaScript string literal and potentially introduce XSS vulnerabilities.Apply this diff to use
json_encode()for proper escaping:- echo "<script>console.error('".htmlspecialchars("Error evaluating content in $evalFile: ".$e->getMessage())."');</script>"; + $errorMessage = "Error evaluating content in $evalFile: ".$e->getMessage(); + echo "<script>console.error(".json_encode($errorMessage).");</script>";emhttp/plugins/dynamix/include/DefaultPageLayout.php (1)
136-146: Fix evalContent include path; avoid direct eval bypassing safety
- The include points to webGui/include/... which likely doesn’t exist; evalContent.php lives under plugins/dynamix/include/... per this PR. This will fatally fail and break button actions.
- Using eval($evalContent) when Eval=true bypasses the new guard and can still take down the page on fatals. Prefer routing Eval=true through the guarded helper (or unify on the helper).
Apply the path fix:
- include "$docroot/webGui/include/DefaultPageLayout/evalContent.php"; + include "$docroot/plugins/dynamix/include/DefaultPageLayout/evalContent.php";Option A (recommended): flip logic so Eval=true uses the guard:
- if ( filter_var($button['Eval']??false, FILTER_VALIDATE_BOOLEAN) ) { - eval($evalContent); - } else { - include "$docroot/plugins/dynamix/include/DefaultPageLayout/evalContent.php"; - } + if ( filter_var($button['Eval']??false, FILTER_VALIDATE_BOOLEAN) ) { + include "$docroot/plugins/dynamix/include/DefaultPageLayout/evalContent.php"; + } else { + eval($evalContent); + }Option B (safest): always go through the helper and drop the direct eval:
- if ( filter_var($button['Eval']??false, FILTER_VALIDATE_BOOLEAN) ) { - eval($evalContent); - } else { - include "$docroot/plugins/dynamix/include/DefaultPageLayout/evalContent.php"; - } + include "$docroot/plugins/dynamix/include/DefaultPageLayout/evalContent.php";Run these checks to catch other occurrences and ensure consistency:
#!/bin/bash # Wrong include path occurrences rg -nP "include\s+[\"']\$docroot/webGui/include/DefaultPageLayout/evalContent\.php[\"']" -C2 # Correct include path occurrences rg -nP "include\s+[\"']\$docroot/plugins/dynamix/include/DefaultPageLayout/evalContent\.php[\"']" -C2 # Direct evals of $evalContent rg -nP --type=php -C2 '\beval\s*\(\s*\$evalContent\s*\)'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
emhttp/plugins/dynamix.docker.manager/AddContainer.page(1 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout.php(3 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php(2 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php(1 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/evalContent.php(1 hunks)emhttp/plugins/dynamix/include/PageBuilder.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-04T05:22:33.141Z
Learnt from: Squidly271
PR: unraid/webgui#2421
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php:23-0
Timestamp: 2025-10-04T05:22:33.141Z
Learning: In the Unraid webgui repository, `emhttp/webGui` is a symlink that points to `plugins/dynamix`. Therefore, paths using `$docroot/webGui/...` correctly resolve to `$docroot/plugins/dynamix/...` at runtime.
Applied to files:
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php
📚 Learning: 2025-05-30T00:50:13.286Z
Learnt from: zackspear
PR: unraid/webgui#2212
File: emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php:37-47
Timestamp: 2025-05-30T00:50:13.286Z
Learning: The .page template system in unraid/webgui is legacy implementation. There is no current appetite to change the eval() usage in .page template files like MainContentTabbed.php, even when security concerns are raised.
Applied to files:
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php
🔇 Additional comments (6)
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabless.php (1)
22-30: LGTM! Guarded evaluation implemented correctly.The conditional evaluation logic properly handles both paths:
- When
Evalis true: direct evaluation for pages that opt out of safety- When
Evalis false/absent: delegates toevalContent.phpfor error-protected evaluationThe implementation correctly uses
filter_var()for boolean validation and maintains the necessary context variables. Based on learnings, thewebGuisymlink path resolves correctly at runtime.emhttp/plugins/dynamix/include/PageBuilder.php (1)
45-55: LGTM! Safe condition evaluation integrated correctly.The refactoring properly delegates condition evaluation to
evalContent.php:
$docrootis correctly added to the global scope for the include path$evalContentconstructs the condition check as an assignment statement$evalSuccessis properly initialized and updated by the included file (shares function scope)- Return logic requires both
$enabled(condition result) and$evalSuccess(execution safety)This ensures page conditions are evaluated with error protection, preventing fatal errors from disabling the entire interface.
emhttp/plugins/dynamix/include/DefaultPageLayout/MainContentTabbed.php (1)
46-52: LGTM! Guarded evaluation implemented correctly in both locations.The conditional evaluation logic is properly implemented in two places:
- Lines 46-52: For pages without titles that have content (e.g., parent pages like Share.page)
- Lines 69-77: For tab panel content rendering
Both implementations correctly:
- Construct
$evalContentand$evalFilefor context- Use
filter_var()withFILTER_VALIDATE_BOOLEANfor proper flag parsing- Route to direct
eval()whenEval=true, or delegate toevalContent.phpfor error-protected evaluation whenEval=false/absentBased on learnings, this change respects the legacy
.pagetemplate system while adding a safety layer for error-prone content.Also applies to: 69-77
emhttp/plugins/dynamix/include/DefaultPageLayout.php (3)
149-152: Confirm evaluation flow for $pages (styles only here)This loop now only annotates and includes stylesheets; no eval. Please confirm the guarded evaluation for $pages content is now handled in HeadInlineJS/MainContent to avoid regressions.
81-81: No functional changeWhitespace-only change; no action needed.
169-169: LGTMClosing tag adjustment is fine.
Simple protection -> assigns default in case of an error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/PageBuilder.php (1)
21-25: Good safety improvement with error handling.The try-catch block appropriately protects against fatal errors from
eval(). CatchingThrowableensures bothErrorandExceptiontypes are handled, and returning the default value is a reasonable fallback for a getter function.Optional: Consider adding error logging for debugging.
While silently returning the default is reasonable, logging the error (e.g., via
my_logger()) could aid in debugging malformed key expressions during development.Apply this diff to add optional error logging:
try { eval("\$var=$key;"); } catch (Throwable $e) { + my_logger("Failed to evaluate key '$key': {$e->getMessage()}"); return $default; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/PageBuilder.php(2 hunks)
🔇 Additional comments (1)
emhttp/plugins/dynamix/include/PageBuilder.php (1)
51-58: No breaking changes detected forpage_enabled.The only caller (
if (page_enabled($page))) correctly handles the boolean return value.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor