Security audit, code quality fixes, and UI improvements#35
Merged
mstrhakr merged 19 commits intomstrhakr:mainfrom Feb 13, 2026
Merged
Security audit, code quality fixes, and UI improvements#35mstrhakr merged 19 commits intomstrhakr:mainfrom
mstrhakr merged 19 commits intomstrhakr:mainfrom
Conversation
…lidation, improve shell escaping - 1a: Add basename() sanitization to all POST['script'] usages via getPostScript() helper to prevent path traversal attacks in exec.php - 1b: Add CSRF token validation to exec.php, compose_util.php, and inject csrf_token into all frontend pages (main, settings, dashboard) - 1c: Remove unnecessary urldecode() from all POST parameters in exec.php and compose_util_functions.php — PHP already decodes form data - 1d: Improve shell escaping in execComposeCommandInTTY() — use pkill -f with escapeshellarg() instead of pgrep|awk pipeline, escapeshellarg the socket path
- 2a: Fix icon.php calling undefined getComposeRoot() — use
locate_compose_root('compose.manager') which is defined in defines.php
- 2b: Move update status file reads outside per-container loops in
checkStackUpdates and getStackContainers — split into two-pass approach
(clear cached SHAs once, then check updates) to eliminate redundant
file reads/writes inside loops
- 2c: Remove duplicate trailing code block in event/started (lines 241-253)
that caused a bash syntax error
- 3e: Add COMPOSE_UPDATE_STATUS_FILE and UNRAID_UPDATE_STATUS_FILE constants to defines.php and replace 8 hardcoded string literals throughout exec.php
…iables - 4a: Remove eval from docker rmi (use array expansion instead), docker compose ls, and docker ps commands in compose.sh where eval was not needed. Eval remains where @Q-quoted variables require it. - 4b: Add error handling for the logs command in compose.sh - 4c: Quote COMPOSE_ROOT variable in stopping_docker and event/started glob patterns to handle paths with spaces. Quote find arguments in compose.sh and fix unquoted envFile test.
- 5b: Rewrite editName() and editDesc() to use DOM construction (jQuery .val(), .attr(), .on()) instead of string concatenation with .html() — prevents attribute breakout XSS - 5c: Fix applyName()/cancelName() to use .text() instead of .html() for user-supplied values. Fix applyDesc() to escape HTML before inserting with .html() (using existing escapeHtml() helper). Fix cancelDesc() to use .text(). Escape stackName in tooltipster content injection.
…njection
- deleteStack: apply basename() to stackName to prevent path traversal (1d)
- addStack: validate stackPath is under /mnt/ or /boot/config/ (1c)
- setEnvPath: validate env file path is under allowed root (1b)
- saveBackupSettings: whitelist allowed keys, validate numeric/enum fields,
sanitize values to prevent INI injection (1a, 6b)
- restoreBackup: apply basename() to archive parameter (1g)
- resolveArchivePath: always use basename() instead of allowing absolute paths (1g)
- uploadBackup: explicitly append CSRF token to FormData (1e)
- setEnvPath: replace exec('rm') with @Unlink() (3e partial)
…ron script
- checkAllStacksUpdates: load status file once, batch-clear local SHAs,
save once — eliminates per-container loadJSON/saveJSON calls (2a)
- saveProfiles: fix double-echo when deleting empty profiles, replace
exec('rm') with @Unlink() (2b, 3e)
- updateAutostart: replace exec('rm') with @Unlink() (3e)
- backup_cron.sh: parse all JSON fields in single php -r call instead
of 5 separate invocations (2d)
…ink()
- addStack: replace 7-line inline sanitization with sanitizeFolderName() call (3a)
- defines.php: add PENDING_RECHECK_FILE constant (3c)
- exec.php: use PENDING_RECHECK_FILE constant in markStackForRecheck,
getPendingRecheckStacks, and clearStackRecheck cases (3c)
- dashboard_stacks.php: use COMPOSE_UPDATE_STATUS_FILE constant instead of
hardcoded path (3d)
- updateAutostart: replace exec('rm') with @Unlink() (3e)
- compose.sh: refactor from string-based \ + eval to proper Bash arrays for env_args, file_args, profile_args, cmd_args. Eliminates eval from down, stop, logs, and run_with_retry (2c, 4c) - compose.sh: run_with_retry now accepts command as positional args and uses printf '%%q' for script -c instead of eval (4c) - compose.sh: update fallback image extraction to use file_args array - stopping_docker: use arrays and find -print0 for safe path handling with spaces in file paths (4b) - patch.sh: increase mktemp random suffix from 4 to 8 chars (4d)
…lename - deleteStack(): escape project and stackName with escapeHtml() before injecting into SweetAlert html:true dialog (5a) - applyDesc(): use .text() with CSS white-space:pre-line instead of .html() with <br> replacement to avoid latent XSS vector (5b) - All editorFileName updates: replace .html(response.fileName) with .text() across editComposeFile, editEnv, editComposeFileByProject, editEnvByProject, editOverrideByProject (5c) - compose_list.php: apply htmlspecialchars() to description before converting newlines to <br> tags (5e)
- backup_functions.php updateBackupCron(): replace PID-based temp file naming with PHP tempnam() for unpredictable filenames (6a) - Use escapeshellarg() when passing temp file path to crontab command
- event/started: Refactor start_stack() from string-based command construction (bash -c) to array-based execution, eliminating shell injection risk from stack names, env paths, and profile names - exec.php: Sanitize changeName input to only allow safe characters, preventing shell metacharacters from being stored and later used in bash scripts - compose_list.php: HTML-escape profilesJson in data-profiles attribute to prevent attribute breakout from profile names containing quotes - dashboard: Add escapeHtml()/escapeAttr() helpers, escape all dynamic container data (names, images, SHAs, icons, WebUI URLs) in HTML output - dashboard: Replace inline onclick handlers with data-* attributes and event delegation for both container icons and stack icons, eliminating attribute injection vectors
…leanup - Fix CSRF token: replace $.ajaxSetup with $.ajaxPrefilter in 3 JS files (compose_manager_main.php, settings.page, dashboard.page) to reliably inject csrf_token into POST requests regardless of data format - Add read-only action whitelists in exec.php and compose_util.php to skip CSRF validation on safe GET-like actions - Extract buildComposeArgs() helper in exec_functions.php to deduplicate compose file/env/project resolution across getStackContainers, checkStackUpdates, and checkAllStacksUpdates - Guard getPostScript() with function_exists for test compatibility - Remove eval from backup_cron.sh, use individual command substitutions - Remove dead code ( redefinition) from dashboard_stacks.php - Add shift to compose.sh wildcard case to prevent infinite loop - Fix ExecActionsTest: remove incorrect urlencode() from values, skip setEnvPath test on Windows (requires Linux path validation)
Same root cause as ExecActionsTest fix - tests set \ directly but wrapped values in urlencode(), causing path/profile corruption. All 233 tests now pass with 0 failures.
The CSRF token validation was added by our audit but is unnecessary: - Unraid plugins run behind Unraid's own authentication on a local network - The plugin-level CSRF layer was redundant and actively breaking all POST requests (Save Settings, Backup Now, compose actions, etc.) - Removed server-side validation from exec.php and compose_util.php - Removed client-side token injection (ajaxPrefilter) from all 3 JS files - Removed manual FormData csrf_token append from backup upload
- Source header: split CSS rule so only td gets #606060 color, th inherits the theme default (was applying gray to both) - Flash fix: cached expands just slideDown existing DOM content instead of re-fetching via AJAX (which caused visible spinner flash + double slideDown). Uncached expands defer slideDown until after render completes.
realpath() returns false when the target directory doesn't exist, causing the path validation to reject the env path. Create the directory before the test so it resolves on GitHub Actions.
GitHub Actions runners don't have write permission to /mnt/, so mkdir fails silently and realpath() returns false. Skip gracefully instead of failing.
- Add compose_root to the setEnvPath path validation allowlist (alongside /mnt/ and /boot/config/) so env files stored next to stacks are accepted - Test now uses a temp dir under compose_root instead of /mnt/, making it work on all platforms (Linux CI, Windows, macOS) without skipping
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.
Security Fixes
evalusage with safe array-based argument handling; applyescapeshellarg()consistentlyurldecode()calls that could bypass input validationCode Quality
defines.phpsanitizeStackName,sanitizeDescription,isValidWebUIUrl)buildComposeArgs()helper to eliminate repeated argument-building logiccheckAllStacksUpdates(single JSON write instead of per-container)Shell Script Hardening
tempnam()+escapeshellarg()for crontab temp filesevalfromcompose.sh,backup_cron.sh, event scriptsUI Fixes
#606060color; header inherits theme defaultTest Fixes
ComposeUtilTest: remove incorrecturlencode()from POST test dataExecActionsTestexpectations to match refactored code