Fix issue #2515: Clean up temporary terminal files #2517
Conversation
- Use fixed filename 'file.manager.terminal.sh' instead of timestamp-based names - Remove temporary script after terminal is opened (not dependent on script success) - Use user's shell from posix_getpwuid() instead of hardcoded bash - Add sed_escape() function similar to preg_quote() with optional delimiter - Prevents accumulation of /var/tmp/File_Manager_*.run.sh files
WalkthroughReplaces dynamic terminal naming with a static UI identifier and moves terminal startup to per-session script/profile files; adds a sed-escaping helper, determines the target shell via posix_getpwuid(0), and makes startup scripts self-delete and exec the user shell with --norc -i. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant PHP as OpenTerminal.php
participant FS as Filesystem
participant ttyd
participant Shell
Browser->>PHP: Request "Open terminal here"
PHP->>FS: write /tmp/file.manager.terminal.<id>.profile
PHP->>FS: write /var/tmp/file.manager.terminal.<id>.sh
PHP->>ttyd: spawn ttyd (name: file.manager.terminal) running the startup script
ttyd->>FS: execute /var/tmp/file.manager.terminal.<id>.sh
/var/tmp/...sh->>FS: source profile, remove profile, unlink self
/var/tmp/...sh->>Shell: exec <user_shell> --norc -i
Shell->>Browser: interactive session via ttyd
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
🔧 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
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix/include/OpenTerminal.php:
- Around line 43-51: The sed_escape() function currently only escapes BRE
pattern metacharacters but not the replacement-context special character '&',
which leads to unintended substitution when used in the sed replacement; update
sed_escape (or create sed_replacement_escape) to also escape '&' for replacement
context (e.g., add '&' to the preg_replace pattern or perform an additional
str_replace to replace '&' with '\&') so paths like "Tom & Jerry" are safe, and
optionally rename the function and update its comment to indicate it escapes for
sed replacements.
- Around line 77-78: The fixed file paths for the terminal script/profile
(variables like $exec = "/var/tmp/file.manager.terminal.sh" and the profile file
"/tmp/file.manager.terminal.profile") create a race where concurrent terminals
can overwrite each other; change these to include a unique suffix (e.g.,
getmypid() or a random token/uniqid()) when you create the filenames and use the
same generated name for all reads/writes in OpenTerminal.php so each session has
its own script/profile; ensure the code that writes the script, writes the
profile, and invokes/cleans up the script/profile all reference the generated
filename variable.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/OpenTerminal.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
📚 Learning: 2025-10-26T12:07:25.907Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2429
File: emhttp/plugins/dynamix/nchan/file_manager:248-249
Timestamp: 2025-10-26T12:07:25.907Z
Learning: Unraid defaults to bash as the shell and does not have dash available. Process substitution syntax `>(...)` used with PHP's `exec()` function (which calls `/bin/sh`) is safe on Unraid because `/bin/sh` is bash.
Applied to files:
emhttp/plugins/dynamix/include/OpenTerminal.php
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/OpenTerminal.php
🔇 Additional comments (3)
emhttp/plugins/dynamix/Browse.page (1)
1391-1396: LGTM!The change to use a static identifier
'file.manager.terminal'aligns with the consolidated terminal script approach inOpenTerminal.php. This properly addresses the PR objective of preventing accumulation of temporary files.emhttp/plugins/dynamix/include/OpenTerminal.php (2)
82-83: Good improvement: Dynamic shell detection.Using
posix_getpwuid(0)['shell']instead of hardcoding/bin/bashis a solid improvement that respects the system configuration and aligns with the standard terminal path (line 105).
95-101: LGTM: Self-deletion mechanism is correctly implemented.The grouped command
{ rm -f '$exec'; exec $user_shell --norc -i; }ensures cleanup happens before exec replaces the process. Since bash loads the script into memory before execution, this pattern is safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix/include/OpenTerminal.php:
- Around line 43-51: Remove the unused function bre_escape from
OpenTerminal.php: locate the function definition named bre_escape (the block
starting with function bre_escape($s, $delimiter = null)) and delete it along
with its doc/comments, ensuring no other code references bre_escape remain; run
a quick project-wide search to confirm it's not used before committing.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/OpenTerminal.php
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui codebase, Squidly271 prefers file locking to be handled centrally within the file_put_contents_atomic function rather than in individual functions that use it. This maintains cleaner architecture by centralizing the locking mechanism.
Applied to files:
emhttp/plugins/dynamix/include/OpenTerminal.php
📚 Learning: 2025-06-03T21:27:15.912Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2230
File: emhttp/plugins/dynamix/include/Templates.php:63-74
Timestamp: 2025-06-03T21:27:15.912Z
Learning: In the Unraid WebGUI codebase (emhttp/plugins/dynamix/include/Templates.php), there are known duplicate ID issues in checkbox templates across multiple template instances that the maintainers are aware of but have chosen not to address due to the effort required for legacy code improvements.
Applied to files:
emhttp/plugins/dynamix/include/OpenTerminal.php
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/include/OpenTerminal.php
📚 Learning: 2025-10-26T12:07:25.907Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2429
File: emhttp/plugins/dynamix/nchan/file_manager:248-249
Timestamp: 2025-10-26T12:07:25.907Z
Learning: Unraid defaults to bash as the shell and does not have dash available. Process substitution syntax `>(...)` used with PHP's `exec()` function (which calls `/bin/sh`) is safe on Unraid because `/bin/sh` is bash.
Applied to files:
emhttp/plugins/dynamix/include/OpenTerminal.php
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/OpenTerminal.php
🔇 Additional comments (3)
emhttp/plugins/dynamix/include/OpenTerminal.php (3)
52-55: LGTM!The
sed_escapefunction correctly escapes both backslashes and ampersands in the proper order for sed replacement strings.
81-87: Good approach using fixed filename and dynamic shell detection.Using a fixed path eliminates temp file accumulation, and
posix_getpwuid(0)['shell']is more robust than hardcoding/bin/bash.Minor consideration: the fixed filename could cause issues if multiple File Manager terminals are opened simultaneously (second invocation would overwrite the script). Given Unraid's single-user nature, this is likely acceptable, but worth noting.
106-111: LGTM!Good consistency using
posix_getpwuid(0)['shell']in both the path-specific terminal and the standard login shell branches.
1 participant
Fixes #2515
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.