fix: harden Apps page against errors from corrupt caches and malformed data - #128
Open
Squidly271 wants to merge 5 commits into
Open
fix: harden Apps page against errors from corrupt caches and malformed data#128Squidly271 wants to merge 5 commits into
Squidly271 wants to merge 5 commits into
Conversation
ci(release): establish beta promotion gate
…d data Add defensive guards across the Apps backend so corrupt cache files, malformed application-feed/template data, and unusual settings values degrade gracefully instead of throwing PHP errors: - Pinned apps: cast cache reads to array, guard the menu-state and toggle paths, and honor the Hide Deprecated setting. - Template info cache ($GLOBALS['templates']) and Docker info (getAllInfo): always return arrays, with guarded $info lookups. - Translation wrapper tr(): coerce non-string input so it can't fatal in dynamix _(). - XML generation: reset Array2XML static state on failure and catch conversion errors in makeXML. - Guard a couple of never-populated template fields (TemplateURL, Version). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Apps page now normalizes corrupted data, catches XML-generation failures, handles missing template metadata, and filters deprecated pinned apps when configured. The unreleased changelog documents these fixes. ChangesApps page resilience and pinned-app filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@source/community.applications/usr/local/emhttp/plugins/community.applications/include/helpers.php`:
- Around line 1181-1186: Update callers of makeXML, especially the usages in
exec.php, to explicitly detect a false return value before writing XML to disk.
Handle the failure gracefully by skipping the write or returning an appropriate
error, while preserving normal writes for valid XML results.
- Around line 2052-2055: Replace scalar-to-array casts for JSON cache values
with explicit array validation so invalid data produces an empty array. In
helpers.php lines 2052-2055, update the containers return path; also update the
readJsonFile calls in helpers.php lines 66 and 83, exec.php lines 1971 and 1988,
and skins/Narrow/skin.php line 1165 to use an is_array check with an empty-array
fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 448ef3fd-e7c6-4aa6-97c3-b6792768c803
📒 Files selected for processing (7)
plugins/CHANGES.mdsource/community.applications/usr/local/emhttp/plugins/community.applications/include/exec.phpsource/community.applications/usr/local/emhttp/plugins/community.applications/include/helpers.phpsource/community.applications/usr/local/emhttp/plugins/community.applications/include/pinned_apps_helpers.phpsource/community.applications/usr/local/emhttp/plugins/community.applications/include/xml_libs.phpsource/community.applications/usr/local/emhttp/plugins/community.applications/skins/Narrow/skin.phpsource/community.applications/usr/local/emhttp/plugins/community.applications/skins/Narrow/skin_helpers.php
Squidly271
added a commit
that referenced
this pull request
Jul 20, 2026
- Use is_array(...) ? ... : [] instead of (array) casts so a corrupt cache that decodes to a scalar yields an empty array, not a one-element array wrapping the junk value (getGlobals, getFullGlobals, getAllInfo, pinApp, areAppsPinned, my_display_apps). - Guard makeXML callers (createXML, convert_docker): on a false return, send an error response instead of writing an empty XML file to disk. - Declare makeXML @return string|false to match the failure path (PHPStan). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Use is_array(...) ? ... : [] instead of (array) casts so a corrupt cache that decodes to a scalar yields an empty array, not a one-element array wrapping the junk value (getGlobals, getFullGlobals, getAllInfo, pinApp, areAppsPinned, my_display_apps). - Guard makeXML callers (createXML, convert_docker): on a false return, send an error response instead of writing an empty XML file to disk. - Declare makeXML @return string|false to match the failure path (PHPStan). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move caPluginUpdateCheck out of page init into the setupActionCentre callback (guarded to run once). Kicking a CA plugin update check during init could leave enableActionCentre blocking on an update that the same page load just started. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Squidly271
force-pushed
the
fix/php-error-hardening
branch
from
July 20, 2026 04:50
1df109d to
4ccdd1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/community.applications/usr/local/emhttp/plugins/community.applications/include/helpers.php (1)
1158-1159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden the
makeXML()failure contract.
makeXML()catches anyThrowable, but the PHPDoc saysfalseis returned only for invalid XML names. Document this as a general XML-conversion failure, or narrow the catch to the specific expected exception.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/community.applications/usr/local/emhttp/plugins/community.applications/include/helpers.php` around lines 1158 - 1159, Update the makeXML() PHPDoc return description to state that false may be returned for any XML-conversion failure, matching its catch of any Throwable; do not limit the documented failure to invalid XML tag or attribute names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@source/community.applications/usr/local/emhttp/plugins/community.applications/include/helpers.php`:
- Around line 2053-2057: Normalize the result of the cache read immediately
after loading CA_PATHS['info'], before the refresh decision checks such as
!$containers or empty($containers). In the surrounding Docker cache refresh
function, convert any non-array value to [] at that point so corrupt scalar data
triggers the existing refresh path; then remove the redundant final is_array
guard while preserving the typed array return.
---
Nitpick comments:
In
`@source/community.applications/usr/local/emhttp/plugins/community.applications/include/helpers.php`:
- Around line 1158-1159: Update the makeXML() PHPDoc return description to state
that false may be returned for any XML-conversion failure, matching its catch of
any Throwable; do not limit the documented failure to invalid XML tag or
attribute names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bda91ddf-d6b0-40d4-bbd5-66631fdfc254
📒 Files selected for processing (4)
source/community.applications/usr/local/emhttp/plugins/community.applications/Apps.pagesource/community.applications/usr/local/emhttp/plugins/community.applications/include/exec.phpsource/community.applications/usr/local/emhttp/plugins/community.applications/include/helpers.phpsource/community.applications/usr/local/emhttp/plugins/community.applications/skins/Narrow/skin.php
🚧 Files skipped from review as they are similar to previous changes (1)
- source/community.applications/usr/local/emhttp/plugins/community.applications/include/exec.php
Addresses CodeRabbit: the is_array guard was only at getAllInfo's return, so a truthy scalar from a corrupt cache made both !$containers and empty() false, skipping the refresh and returning [] indefinitely. Normalize non-arrays to [] right after the read so the existing refresh path rebuilds from Docker. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defensive hardening across the Apps backend so corrupt cache files, malformed application-feed/template data, and unusual settings values degrade gracefully instead of throwing PHP errors (warnings or fatals) under PHP 8.
Changes
(array)(pinApp,areAppsPinned,my_display_apps), andfindPinnedTemplatenow honors the Hide Deprecated setting alongside the existing blacklist/incompatible skips.$GLOBALS['templates']is coerced to an array at all four assignment sites, so a corrupttemplatescache can't reachcount()/foreachas a scalar.getAllInfo()always returns an array;$infolookups in the card/sidebar action builders are guarded (??and$ind !== false) so a stale-vs-live mismatch can't misread or warn.tr()coerces non-string input to a safe string so it can't fatal inside dynamix_()'strim().Array2XML::createXMLresets its staticDomDocumentstate even when conversion throws (fixing a state leak into the next call), andmakeXML()catches conversion exceptions instead of propagating a 500.TemplateURL,Version) that were read without??.Notes
.plgversion,CHANGESblock, andca.md5intentionally left untouched; a user-facingFixed:bullet is added toplugins/CHANGES.md.🤖 Generated with Claude Code
Summary by CodeRabbit