fix(plugins): replace dependency marker files with a real satisfaction check - #390
Merged
Merged
Conversation
…n check The .dependencies_installed hash-marker system only tracked "was this exact requirements.txt hashed before" — not whether the packages it names are actually present. That made it fragile (a wiped venv, a manually removed package, or a lost/corrupted marker forces a needless full pip reinstall or, worse, a false skip) and produced dead weight for the ~10 plugins whose requirements.txt is comment-only (they still paid a pip subprocess on first boot before a marker existed). Replace it with requirements_are_satisfied() in plugin_loader.py, which checks each real requirement line against importlib.metadata directly, so install_dependencies() only shells out to pip when something is actually missing or version-mismatched. Drops the marker file entirely: removed all marker read/write sites in plugin_loader.py and store_manager.py, the now-pointless marker-cleanup step in the git-update path, the unused legacy marker implementation in plugin_manager.py, and the already-stale clear_dependency_markers.sh script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughPlugin dependency installation now parses requirements and checks installed versions before invoking pip. Marker-file tracking and duplicate installers were removed, store updates adjust stashing behavior, and tests cover satisfied dependencies and retry paths. ChangesPlugin dependency installation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -6 |
| Duplication | -2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugin_system/store_manager.py (1)
1900-1923: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated preflight-check pattern with
plugin_loader.install_dependencies.The
requirements_has_real_deps/requirements_are_satisfiedguard-and-log sequence here is nearly identical to the one insrc/plugin_system/plugin_loader.py'sinstall_dependencies(lines 261-273). Consider extracting a small shared helper (e.g.should_skip_pip_install(requirements_file, label, logger)) that both call sites can reuse, since the two pip-invocation strategies (subprocess vsinstall_requirements_file) can remain separate.♻️ Possible shared helper
def should_skip_pip_install(requirements_file: str, label: str, logger) -> bool: if not requirements_has_real_deps(requirements_file): logger.debug("requirements.txt for %s has no real dependencies, skipping pip", label) return True if requirements_are_satisfied(requirements_file): logger.debug("Dependencies for %s already satisfied, skipping pip", label) return True return False🤖 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 `@src/plugin_system/store_manager.py` around lines 1900 - 1923, Extract the duplicated requirements preflight checks from _install_dependencies and plugin_loader.install_dependencies into a shared should_skip_pip_install helper, reusing requirements_has_real_deps and requirements_are_satisfied with consistent debug logging. Update both callers to invoke the helper while preserving their existing, separate pip installation strategies and the missing-file handling in _install_dependencies.
🤖 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 `@src/plugin_system/plugin_loader.py`:
- Around line 26-93: Address CodeQL’s uncontrolled-path warnings in
requirements_has_real_deps and requirements_are_satisfied by making each
function explicitly validate or sanitize requirements_file before calling
open(). Reuse the module’s established realpath/basename containment approach,
or change the functions to accept a prevalidated Path type and enforce the
allowed directory boundary internally; update their callers accordingly while
preserving existing error behavior.
---
Nitpick comments:
In `@src/plugin_system/store_manager.py`:
- Around line 1900-1923: Extract the duplicated requirements preflight checks
from _install_dependencies and plugin_loader.install_dependencies into a shared
should_skip_pip_install helper, reusing requirements_has_real_deps and
requirements_are_satisfied with consistent debug logging. Update both callers to
invoke the helper while preserving their existing, separate pip installation
strategies and the missing-file handling in _install_dependencies.
🪄 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: ada0a19e-e8d2-4b39-9438-6e21851f72ac
📒 Files selected for processing (7)
requirements.txtscripts/clear_dependency_markers.shsrc/plugin_system/plugin_loader.pysrc/plugin_system/plugin_manager.pysrc/plugin_system/store_manager.pytest/test_plugin_loader.pytest/test_plugin_system.py
💤 Files with no reviewable changes (2)
- src/plugin_system/plugin_manager.py
- scripts/clear_dependency_markers.sh
…ecks CodeQL flagged 2 new high-severity "uncontrolled data used in path expression" alerts at the open() calls inside this PR's new requirements_has_real_deps()/requirements_are_satisfied() -- both are reachable from paths that were never run through the basename+trusted-base sanitiser this codebase already uses elsewhere: - PluginLoader.install_dependencies() only applied that sanitiser when its optional plugins_dir argument was actually passed; the "no plugins_dir" branch trusted plugin_dir_real directly. Made plugins_dir required (not Optional) so that branch can't exist, and added an explicit guard in load_plugin() so install_deps=True without a plugins_dir fails loudly instead of silently. Production's only real caller (PluginManager) always passes plugins_dir already; the harness/dev-server/render-plugin callers all use install_deps=False and are unaffected. - StoreManager._install_dependencies() never sanitised plugin_path at all, and its call sites ultimately derive that path from a plugin's own manifest.json "id" field (install_plugin_from_url) -- a malicious plugin could otherwise point requirements_file outside plugins_dir. Applied the same os.path.basename()-based containment pattern PluginLoader already uses (and that CodeQL recognises as a real sanitiser). Added test_install_dependencies_requires_plugins_dir and test_install_dependencies_rejects_path_outside_plugins_dir to lock in the actual security property, not just quiet the scanner. Verified: all 20 tests in test_plugin_loader.py pass, plus the PR's existing test plan (test_plugin_system.py, test_store_manager_caches.py: 53 passed) and the full CI plugin-safety suite (test_harness.py, test_visual_rendering.py, test_plugin_matrix.py: 52 passed, 2 pre-existing skips) all still pass.
…tion check The previous commit's os.path.basename() + os.path.join() pattern (which a pre-existing code comment claimed CodeQL recognises as a sanitiser) did not actually clear the alert -- the next CodeQL run still flagged the same 2 sink lines, plus a new one at the os.path.join() call itself. Taking a substring of tainted data apparently isn't treated as a barrier by this query, whatever the comment assumed. Replaced it with find_trusted_subdir(): enumerate the trusted plugins_dir via os.scandir() and only use a name that scandir itself produced, matched by equality against the caller's requested name. The path is then built from that enumerated entry, not from the caller's string -- a value sourced from iterating a trusted, non-tainted directory carries no taint regardless of what it happens to equal, which is a stronger and more conventional allowlist-style barrier than string-stripping. Applied identically in both PluginLoader.install_dependencies() and StoreManager._install_dependencies(), sharing one implementation. Re-verified: all 65 tests across test_plugin_loader.py (20, including the 2 new security regression tests), test_store_manager_caches.py (35), test_plugin_system.py (10) pass, plus the full CI plugin-safety suite (test_harness.py/test_visual_rendering.py/test_plugin_matrix.py: 52 passed, 2 pre-existing skips).
…re logging CodeQL flagged 3 clear-text-logging-of-secrets alerts in install_requirements_file() (src/common/permission_utils.py:353,360,371). Pre-existing on main, unrelated to this PR's own diff, but now visible since the path-injection alerts that previously took priority in the annotation list are fixed. The underlying risk is real: pip can echo a private index URL's embedded basic-auth credentials (from a requirements.txt --index-url line or PIP_INDEX_URL) back verbatim in its own stderr/stdout on failure, and this function both logs that output directly and returns it to callers -- store_manager.py's _install_dependencies() logs result.stderr from this same function too. Added _redact_url_credentials(), applied immediately after each of the two subprocess.run() calls (mutating result.stderr/stdout in place) rather than patching each log call site individually. This closes the leak at the source: every downstream use -- the three flagged log lines, the "note" string embedded in the returned stdout, and store_manager.py's own logging of the returned result -- gets the redacted text for free. Verified the fixed-phrase "denied" check (`"a password is required" in result.stderr`) is unaffected, since URL syntax and those phrases don't overlap -- covered explicitly by test_does_not_touch_denied_check_phrases. Added test/test_permission_utils.py (6 tests) covering the redaction helper directly and both subprocess.run() call sites (the sudo-wrapper branch, which this repo's scripts/fix_perms/safe_pip_install.sh makes live, and the no-wrapper fallback branch). All pass.
The previous commit's redaction (mutating result.stderr/stdout right after each subprocess.run()) didn't clear CodeQL's clear-text-logging alerts -- same lesson as the path-injection fix earlier in this PR: a static analyzer can't tell "this value was already sanitised two lines up" from "this is still the raw tainted value" just by looking at a single log call in isolation, so it conservatively keeps flagging it regardless of what the redaction function actually does. Removed all dynamic interpolation (req_file, result.stderr) from the 3 flagged logger.warning() calls entirely, replacing them with fixed messages plus (for the one that had it) result.returncode, which is a plain int with no possible taint. The full redacted detail is still available where it actually matters -- in the returned CompletedProcess.stderr/stdout and the "note" text -- just not duplicated into a log line a scanner has to reason about in isolation. Re-verified: all 6 test_permission_utils.py tests still pass (they assert on the returned result, not log call arguments), plus the full test_plugin_loader.py/test_store_manager_caches.py/test_plugin_system.py suite (71 passed, 1 pre-existing deselect, 4 subtests).
ChuckBuilds
added a commit
that referenced
this pull request
Jul 11, 2026
CodeQL flagged 2 high-severity path-injection alerts and 7 medium stack-trace-exposure alerts introduced by this PR. Path injection (PluginLoader.find_plugin_directory, Strategy 2): replaced the resolve()+relative_to() containment check with find_trusted_subdir() (already used elsewhere in this file since #390) -- a name enumerated directly from plugins_dir via scandir() carries no taint regardless of what the caller's plugin_id string was, which CodeQL's path-injection query recognises; a post-hoc containment check on a path built from the tainted string apparently isn't, matching what #390 found for a similar pattern. Stack trace exposure: render_service.py's update()/display() exception handlers embedded the raw exception message directly into the response returned to the preview endpoint's caller. Showing *something* here is the point of the endpoint (it's the plugin author's own code failing, not a server secret), but the message was otherwise unbounded and could echo back the server's own directory layout (e.g. a FileNotFoundError embeds the full path it tried). Added _safe_exc_message(): collapses absolute paths to their basename and caps length, with the full exception (traceback included) now logged server-side via logger.warning(..., exc_info=True) before the sanitized version reaches the client. Applied the same helper in dev_server.py's two matching load-failure handlers; web_interface/blueprints/api_v3.py's HTML preview fragment (the other 2 flagged locations) consumes the same result['errors']/['warnings'] fields, so fixing the shared source in render_service.py covers it too -- confirmed it's the only other consumer. Verified: existing test/web_interface/test_plugin_preview.py assertions only check for an empty errors list on success, not exact failure-message content, so no test changes needed. Full test/test_plugin_loader.py + test/test_plugin_system.py: 31 passed, 1 pre-existing failure (test_circuit_breaker, unrelated).
ChuckBuilds
added a commit
that referenced
this pull request
Jul 11, 2026
…eQL sinks Round 2. The previous commit's _safe_exc_message() (regex-redact absolute paths, cap length) didn't clear the stack-trace-exposure alerts -- same lesson as PR #390's permission_utils.py fix: CodeQL doesn't trust a custom transformation function as a sanitiser, regardless of what it actually does. Removed the helper entirely; render_service.py/dev_server.py's update()/display()/load-failure handlers now surface only the exception's class name (a fixed, bounded string, never derived from the exception's own content) to the client, with the full exception (traceback included) still logged server-side via logger.warning(..., exc_info=True). Fixing the path-injection alert in find_plugin_directory also had a side effect: CodeQL's interprocedural analysis now traces further downstream through that changed function, surfacing 5 new clear-text-logging-of- secrets alerts in code that mostly predates this PR (scripts/render_plugin.py has zero other changes in this diff) -- logging plugin_id/file paths that originate from CLI args or discovered plugin directories. Not real secrets, but CodeQL's model doesn't distinguish "externally-influenced string" from "credential" here. Removed the path/plugin_id values from the 5 flagged log lines (check_plugin.py, render_plugin.py, plugin_loader.py x3), keeping a plugin_id-only or fully generic message where one was already available elsewhere in the same log line's context. Verified: full test/test_plugin_loader.py + test/test_plugin_system.py + the adaptive-layout/element-style/preview suites, no test asserts on the specific log/error message text that changed.
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
.dependencies_installedhash-marker system only tracked "was this exactrequirements.txthashed before," not whether the packages it names are actually installed — fragile against wiped venvs, manually removed packages, or lost/corrupted markers, and dead weight for the ~10 plugins whoserequirements.txtis comment-only.requirements_are_satisfied()inplugin_loader.py, which checks each real requirement line againstimportlib.metadatadirectly, soinstall_dependencies()only shells out to pip when something is actually missing or version-mismatched.plugin_loader.py/store_manager.py, the now-pointless marker-cleanup step in the git-update path, the unused legacy marker implementation inplugin_manager.py, and the already-stalescripts/clear_dependency_markers.sh.packagingas an explicit top-level dependency (was only a transitive dep ofpytestbefore).Test plan
./venv/bin/python3 -m pytest test/test_plugin_loader.py test/test_store_manager_caches.py -q— 53 passed./venv/bin/python3 -m pytest test/test_plugin_system.py -q --deselect test/test_plugin_system.py::TestPluginHealth::test_circuit_breaker— 10 passed, 1 deselected (pre-existing unrelated failure — stale mock signature in a cache_manager test, confirmed failing in isolation on this branch's base too)pytest) skips the pip subprocess entirelyrequirements_are_satisfied()against real pluginrequirements.txtfiles (vacuous and real-deps cases) and edge cases (unmet packages, impossible version specs,-rpip-option lines)🤖 Generated with Claude Code
https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Summary by CodeRabbit
New Features
Bug Fixes