fix(plugins): prevent root-owned files from blocking plugin updates

[ChuckBuilds]

Summary

• The root ledmatrix.service creates __pycache__ and data cache files owned by root inside plugin directories
• The web service (non-root) cannot delete these when updating or uninstalling plugins, causing "Permission denied" failures
• This was the root cause of the Olympics plugin update failure on devpi

Approach: Defense in Depth (3 Layers)

Layer 1: Prevention

• Added Environment=PYTHONDONTWRITEBYTECODE=1 to ledmatrix.service systemd template
• Added sys.dont_write_bytecode = True in run.py as belt-and-suspenders

Layer 2: Sudoers fallback

• Added scoped rm -rf sudoers rules for plugin-repos/* and plugins/* directories in configure_web_sudo.sh

Layer 3: Code-level sudo fallback

• Added sudo_remove_directory() utility in permission_utils.py using sudo -n (non-interactive)
• Rewrote _safe_remove_directory() with 3-stage approach: normal rmtree → chmod fix → sudo fallback
• Replaced 12 bare shutil.rmtree() calls with _safe_remove_directory() throughout store_manager.py

Test plan

• Deploy to devpi, reinstall systemd service (sudo bash scripts/install/install_service.sh)
• Update sudoers by running configure_web_sudo.sh on devpi
• Restart both services and verify no new root-owned __pycache__ dirs are created
• Test plugin update/uninstall/reinstall via web UI
• Manually create a root-owned test file in a plugin dir and verify sudo fallback works

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added “Clear operation history” action and richer version/branch metadata for plugin operations.
  • Installer now exposes an option to remove plugin directories during update/uninstall and installs a vetted safe-removal helper.

• Bug Fixes

  • Safer plugin removal with permission-aware fallbacks and vetted helper support.
  • Prevents Python bytecode file creation to avoid permission/ownership conflicts.

• UI Updates

  • Cleaner operation history display with normalized statuses and refined filters.

[coderabbitai]

Warning

Rate limit exceeded

@ChuckBuilds has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 11 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

Prevents Python bytecode generation; centralizes safe plugin-directory removal with a three-stage fallback (normal → chmod-retry → sudo helper) and adds a vetted safe removal script plus sudoers exposure; adds permission-utils sudo removal helper; adds OperationHistory.clear_history and records version/branch/commit metadata for plugin install/update operations.

Changes

Cohort / File(s)	Summary
Bytecode Prevention run.py, systemd/ledmatrix.service	Disable Python bytecode generation via sys.dont_write_bytecode = True and Environment=PYTHONDONTWRITEBYTECODE=1.
Sudoers & Helper Script scripts/install/configure_web_sudo.sh, scripts/fix_perms/safe_plugin_rm.sh	Add safe_plugin_rm.sh, expose its path in generated sudoers, use command -v lookups, add sudoers entry permitting vetted removal of plugin dirs, and harden script ownership/permissions.
Permission Utilities src/common/permission_utils.py	Add sudo_remove_directory(path: Path, allowed_bases: Optional[list]=None) -> bool to validate paths and invoke the vetted sudo helper with timeout and robust error handling.
Plugin Store Safety src/plugin_system/store_manager.py	Add _safe_remove_directory(self, path: Path) -> bool and replace direct shutil.rmtree calls with the three-stage removal flow (rmtree → chmod & retry → sudo helper fallback).
Operation History src/plugin_system/operation_history.py	Add OperationHistory.clear_history() to clear in-memory history and persist the cleared state.
Web API & UI web_interface/blueprints/api_v3.py, web_interface/templates/v3/partials/operation_history.html	Add _get_plugin_version(), record version/branch/commit in operation history for install/update flows, add clear_operation_history endpoint, normalize statuses client-side, and implement functional Clear History UI action.

Sequence Diagram(s)

sequenceDiagram
actor User
participant API as Web API
participant StoreMgr as PluginStoreManager
participant PermUtils as permission_utils
participant FS as Filesystem
User->>API: Request install/update plugin
API->>StoreMgr: install_plugin()/update_plugin()
StoreMgr->>StoreMgr: _safe_remove_directory(target)
Note over StoreMgr,FS: Stage 1 — Normal removal
StoreMgr->>FS: shutil.rmtree(target)
alt Removed
FS-->>StoreMgr: Success
else Permission error
Note over StoreMgr,FS: Stage 2 — Repair perms & retry
StoreMgr->>FS: chmod -R (repair) and retry rmtree
alt Removed
FS-->>StoreMgr: Success
else Still denied
Note over StoreMgr,PermUtils: Stage 3 — Use sudo helper
StoreMgr->>PermUtils: sudo_remove_directory(target)
PermUtils->>FS: sudo safe_plugin_rm.sh -> rm -rf target
FS-->>PermUtils: Success/Failure
PermUtils-->>StoreMgr: True/False
end
end
StoreMgr-->>API: Removal result
API->>API: Record operation_history (version/branch/commit)
API-->>User: Operation result

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Fix/checkbox save and dynamic duration #182: Replaces direct shutil.rmtree with _safe_remove_directory across install/update/uninstall flows — closely matches removal centralization here.
• Fix/plugin permission errors #181: Adds _safe_remove_directory and centralized safer plugin-directory removal with permission-fix and sudo fallback — strong overlap.
• fix(web): unify operation history tracking for monorepo plugins #240: Adds OperationHistory.clear_history() and API/UI changes for clearing history — overlaps the history-clearing feature here.

Poem

🐇 I hopped through folders, soft and spry,
No bytecode nests where root might lie.
Three steps I take to tidy the den,
History cleared — we start again. 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Merge Conflict Detection	⚠️ Warning	❌ Merge conflicts detected (7 files): ⚔️ run.py (content) ⚔️ scripts/install/configure_web_sudo.sh (content) ⚔️ scripts/setup_plugin_repos.py (content) ⚔️ src/common/permission_utils.py (content) ⚔️ src/plugin_system/store_manager.py (content) ⚔️ systemd/ledmatrix.service (content) ⚔️ web_interface/static/v3/plugins_manager.js (content) These conflicts must be resolved before merging into main.	Resolve conflicts locally and push changes to this branch.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: preventing root-owned files from blocking plugin updates through multiple defensive strategies (bytecode disabling, sudoers rules, and sudo-based fallback removal).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/plugin-permission-ownership

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@scripts/install/configure_web_sudo.sh`:
- Around line 73-78: The sudoers lines granting $WEB_USER NOPASSWD access to
"$RM_PATH -rf $PROJECT_ROOT/plugin-repos/*" and similar are vulnerable to path
traversal via wildcards; replace the wildcard approach by either (A) removing
the wildcard and enumerating exact allowed targets passed from the application,
or (B) create a vetted helper such as safe_plugin_rm.sh that is granted sudo
instead of rm/find; the helper must realpath-resolve the provided target(s) and
verify they are inside $PROJECT_ROOT/plugin-repos or $PROJECT_ROOT/plugins
before calling rm -rf, and update the sudoers entries to allow $WEB_USER to run
only that helper (referenced symbols: $WEB_USER, $RM_PATH, $FIND_PATH,
$PROJECT_ROOT, safe_plugin_rm.sh).
In `@src/common/permission_utils.py`:
- Around line 203-238: The sudo_remove_directory function currently runs a
privileged 'sudo rm -rf' on any Path; add a defensive allow-list check before
invoking subprocess to ensure the resolved target is inside expected base
directories (e.g., the shared constants or the same bases used by
configure_web_sudo.sh such as "plugins" and "plugin-repos") or accept an
explicit allowed_bases parameter; implement validation by resolving the
candidate path (follow symlinks) and confirming it is a descendant of at least
one allowed base (use Path.resolve() and Path.is_relative_to() or an equivalent
check), and if the check fails log an error and return False without calling
subprocess.run; keep the rest of sudo_remove_directory behavior unchanged
(timeouts, logging, error handling).
In `@src/plugin_system/store_manager.py`:
- Around line 838-841: The code currently calls shutil.move(str(plugin_path),
str(correct_path)) after attempting to remove an existing directory without
checking _safe_remove_directory(correct_path)'s return value; update the logic
in the block handling correct_path and manifest_plugin_id so that you check the
boolean result of self._safe_remove_directory(correct_path) and if it returns
False, log an error/warning and return False (or otherwise abort) before calling
shutil.move to avoid proceeding on failed cleanup or nesting directories; ensure
you reference correct_path, manifest_plugin_id, _safe_remove_directory,
plugin_path and shutil.move when applying the change.

🧹 Nitpick comments (6)

scripts/install/configure_web_sudo.sh (1)

33-34: which is not POSIX-portable; prefer command -v.

On some minimal systems (e.g., certain Raspberry Pi images), which may not be installed or may behave differently. command -v is a shell built-in and universally available.

Suggested diff

-RM_PATH=$(which rm)-FIND_PATH=$(which find)+RM_PATH=$(command -v rm)+FIND_PATH=$(command -v find)

(Same applies to the existing which calls on lines 27–32, but those are outside the scope of this change.)

web_interface/templates/v3/partials/operation_history.html (1)

318-331: Clear history implementation is solid.

The DELETE call, success/error handling, and local state cleanup are well-structured. The confirmation dialog prevents accidental purges.

One minor point: consider resetting currentPage = 1 before calling applyFilters() on line 325 to avoid showing an empty page if the user was on a later page when clearing.

Suggested tweak

 if (data.status === 'success') {
allHistory = [];
+ currentPage = 1;
applyFilters();

web_interface/blueprints/api_v3.py (2)

57-72: Add a fail-fast guard for missing plugin_id or manager

If this helper is called before the blueprint is fully initialized or with an empty plugin_id, it can raise AttributeError or read from the plugins root. A quick guard keeps behavior deterministic and avoids noisy warnings.

Suggested guard

 def _get_plugin_version(plugin_id: str) -> str:
"""Read the installed version from a plugin's manifest.json.
@@
"""
+ if not plugin_id or not getattr(api_v3, "plugin_store_manager", None):+ return ''
manifest_path = Path(api_v3.plugin_store_manager.plugins_dir) / plugin_id / "manifest.json"

As per coding guidelines "Validate inputs and handle errors early (Fail Fast principle)".

---

2162-2165: Avoid mislabeling toggle failures when enabled isn't resolved

If the exception happens before data is populated, the fallback always logs “disable.” Consider preferring enabled when available, else default to “toggle.”

Optional tweak

- if api_v3.operation_history:- toggle_type = "enable" if ('data' in locals() and data.get('enabled')) else "disable"+ if api_v3.operation_history:+ toggle_type = "toggle"+ if 'enabled' in locals():+ toggle_type = "enable" if enabled else "disable"+ elif 'data' in locals() and 'enabled' in data:+ toggle_type = "enable" if bool(data.get('enabled')) else "disable"

src/plugin_system/store_manager.py (2)

1567-1625: Well-structured three-stage removal — a few minor improvements.

The defense-in-depth approach (normal → chmod → sudo) is sound and well-documented. A few observations:

1. dirs loop variable unused (Line 1598): Rename to _dirs to signal intent.
2. import stat inside method (Line 1597): Move to the module-level imports for clarity.
3. Stage 2 chmod 0o777 won't help for root-owned files (Lines 1601, 1606): You can't chmod files you don't own, so this stage only works for same-user files as documented. Consider using stat.S_IRWXU (0o700) instead — it's sufficient for deletion by the owning user and avoids the S103 lint warning about overly permissive masks.

♻️ Suggested refinements

+import stat # at module-level imports
def _safe_remove_directory(self, path: Path) -> bool:
...
# Stage 2: Try chmod + retry (works when we own the files)
try:
- import stat- for root, dirs, files in os.walk(path):+ for root, _dirs, files in os.walk(path):
root_path = Path(root)
try:
- os.chmod(root_path, stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)+ os.chmod(root_path, stat.S_IRWXU)
except (OSError, PermissionError):
pass
for file in files:
try:
- os.chmod(root_path / file, stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)+ os.chmod(root_path / file, stat.S_IRWXU)
except (OSError, PermissionError):
pass

---

1053-1056: Remaining shutil.rmtree(temp_dir) in finally block could mask exceptions.

This temp directory cleanup wasn't converted to _safe_remove_directory. Since temp directories are user-owned, permission issues are unlikely, but a bare shutil.rmtree without ignore_errors=True could raise and mask the original exception from the try block. Consider adding ignore_errors=True for consistency with similar cleanup at Lines 1377 and 1432.

♻️ Suggested fix

 finally:
# Cleanup temp directory if it still exists
if temp_dir and temp_dir.exists():
- shutil.rmtree(temp_dir)+ shutil.rmtree(temp_dir, ignore_errors=True)

[coderabbitai]

Actionable comments posted: 4

🤖 Fix all issues with AI agents

In `@scripts/fix_perms/safe_plugin_rm.sh`:
- Around line 25-28: The ALLOWED_BASES initialization in safe_plugin_rm.sh uses
realpath which fails if directories don't exist; change the calls inside
ALLOWED_BASES to use realpath --canonicalize-missing on
"$PROJECT_ROOT/plugin-repos" and "$PROJECT_ROOT/plugins" (preserving existing
quoting) so missing directories won't abort the script under set -e; update the
ALLOWED_BASES assignment that references realpath to use the
--canonicalize-missing flag for both entries.
In `@src/common/permission_utils.py`:
- Around line 263-269: The subprocess.run invocation hardcodes '/bin/bash',
which can mismatch the sudoers BASH_PATH; import shutil at top and resolve the
real bash path (e.g., shutil.which('bash') with a safe fallback) and use that
resolved path in the subprocess.run call that currently passes ['/bin/bash',
str(helper_script), str(path)] (refer to the subprocess.run usage and the
helper_script/path variables) so the invoked shell path exactly matches the
sudoers entry.
In `@src/plugin_system/store_manager.py`:
- Around line 1366-1369: The call to self._safe_remove_directory(target_path)
must be checked before proceeding to shutil.move to avoid moving into a
partially-removed target; modify the block that currently calls
_safe_remove_directory then shutil.move so that after target_path.exists() you
call removed = self._safe_remove_directory(target_path) and if removed is falsy
(or an exception occurs) abort/raise a clear error (including target_path and
source_plugin_dir) or return early instead of calling shutil.move; ensure
shutil.move is only executed when _safe_remove_directory reports success.
- Around line 1588-1596: The removal logic around shutil.rmtree currently only
catches PermissionError and treats other exceptions as fatal, which skips the
chmod retry stages; update the try/except in the method that calls shutil.rmtree
(store_manager.py, the function performing "Stage 1: Try normal removal") to
catch OSError instead of PermissionError for permission-related failures so
EPERM (errno 1) is handled the same as EACCES, and keep the existing broad
Exception handler for truly unexpected exceptions (or re-raise/log non-OSError
exceptions) so stages 2 and 3 still run on permission failures; reference the
shutil.rmtree call and the existing except blocks to locate and change the catch
to OSError.

🧹 Nitpick comments (3)

scripts/fix_perms/safe_plugin_rm.sh (1)

59-60: Add -- before the path argument to rm -rf.

While the path validation above makes it unlikely for the target to start with a dash, using -- is a defensive best practice for commands invoked via sudo.

Proposed fix

 # All checks passed — remove the target
-rm -rf "$RESOLVED_TARGET"+rm -rf -- "$RESOLVED_TARGET"

src/plugin_system/store_manager.py (1)

1617-1618: Consider hoisting the import to module level.

The lazy import of sudo_remove_directory inside _safe_remove_directory is evaluated on every sudo-fallback invocation. Since permission_utils is already part of the project (and imported elsewhere in this file at Lines 1263, 1361, 1423), moving it to the top-level imports would be cleaner.

Proposed change

At the top of the file (around line 19), add:

fromsrc.common.permission_utilsimportsudo_remove_directory

Then simplify Line 1618:

- from src.common.permission_utils import sudo_remove_directory- if sudo_remove_directory(path):+ if sudo_remove_directory(path):

scripts/install/configure_web_sudo.sh (1)

71-74: Sudoers entry correctly delegates to the vetted helper script.

The previous wildcard vulnerability (past review) is properly addressed by routing through safe_plugin_rm.sh, which performs its own realpath validation. The * wildcard in sudoers is now safe because the helper rejects invalid paths and requires exactly one argument.

One minor consideration: the script at SAFE_RM_PATH must be owned by root and not writable by the web user, otherwise privilege escalation is possible. You may want to add a chmod 755 / chown root:root step for the helper script when applying the sudoers config, or document this requirement.

#!/bin/bash# Check if there's any ownership/permission hardening for safe_plugin_rm.sh# in the install scripts or documentation
rg -n "safe_plugin_rm" -g '*.sh' -g '*.md' -g '*.txt'

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@scripts/install/configure_web_sudo.sh`:
- Around line 27-33: Validate the results of each command -v call in
configure_web_sudo.sh by checking PYTHON_PATH, SYSTEMCTL_PATH, REBOOT_PATH,
POWEROFF_PATH, BASH_PATH, JOURNALCTL_PATH and SAFE_RM_PATH for non-empty values
before using them to generate sudoers entries; if any are empty either (a) abort
with a clear error and non-zero exit (so the install fails fast) or (b) omit
only the related sudoers lines and log a warning, and ensure you run visudo -c
(or use sudo cp to /etc/sudoers.d/ with validation) only after validation
succeeds so malformed entries like "NOPASSWD: start ledmatrix.service" are
never written.

🧹 Nitpick comments (2)

src/common/permission_utils.py (2)

266-272: Passes unresolved path to subprocess after validating resolved — minor inconsistency.

Line 268 passes the original path to the helper script, while the validation on Lines 240-256 operates on the resolved path. The helper does its own realpath resolution so the end result is the same, but passing the already-resolved path would be more consistent and eliminate a (theoretical) TOCTOU window between the two resolutions.

Suggested change

 result = subprocess.run(
- ['sudo', '-n', bash_path, str(helper_script), str(path)],+ ['sudo', '-n', bash_path, str(helper_script), str(resolved)],
capture_output=True,
text=True,
timeout=30
)

---

273-275: path.exists() check after removal uses the original (possibly symlink) path.

If path was a symlink, rm -rf on the resolved target won't remove the dangling symlink itself, so path.exists() could return False (dangling symlink) or True depending on OS behavior. Using resolved (or path.resolve()) for the existence check would be more reliable.