fix(plugins): replace dependency marker files with a real satisfaction check (#390)

* fix(plugins): replace dependency marker files with a real satisfaction 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

* fix(security): close path-injection gap in dependency-satisfaction checks

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.

* fix(security): replace basename-only sanitiser with a trusted-enumeration 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).

* fix(security): redact URL credentials from pip subprocess output before 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.

* fix(security): stop interpolating req_file/pip-output into log calls

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).

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

test/test_permission_utils.py

@@ -0,0 +1,83 @@
+"""
+Tests for src.common.permission_utils's URL-credential redaction.
+
+Covers the fix for a CodeQL clear-text-logging-of-secrets alert:
+install_requirements_file() must never let a private index URL's embedded
+user:pass@ credentials reach logs or its returned CompletedProcess, since
+pip can echo that URL back verbatim in its own stderr/stdout on failure.
+"""
+
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+from src.common.permission_utils import _redact_url_credentials, install_requirements_file
+
+
+class TestRedactUrlCredentials:
+    def test_redacts_embedded_basic_auth(self):
+        text = "Could not fetch URL https://alice:s3cr3t@pypi.example.com/simple/: 403"
+        redacted = _redact_url_credentials(text)
+        assert "s3cr3t" not in redacted
+        assert "alice" not in redacted
+        assert "https://***:***@pypi.example.com/simple/" in redacted
+
+    def test_leaves_credential_free_text_unchanged(self):
+        text = "ERROR: Could not find a version that satisfies the requirement foo==1.0"
+        assert _redact_url_credentials(text) == text
+
+    def test_handles_none_and_empty(self):
+        assert _redact_url_credentials(None) == ""
+        assert _redact_url_credentials("") == ""
+
+    def test_does_not_touch_denied_check_phrases(self):
+        """The fixed phrases install_requirements_file greps for must survive
+        redaction untouched -- they don't overlap with URL syntax, but this
+        pins that assumption so a regex change can't silently break it."""
+        text = "sudo: a password is required"
+        assert _redact_url_credentials(text) == text
+
+
+class TestInstallRequirementsFileRedaction:
+    @patch('src.common.permission_utils.subprocess.run')
+    def test_wrapper_path_redacts_stderr_and_stdout(self, mock_run, tmp_path):
+        """safe_pip_install.sh exists in this repo, so install_requirements_file
+        takes the sudo-wrapper branch; a failing result must come back
+        with any embedded index-URL credentials already redacted."""
+        req_file = tmp_path / "requirements.txt"
+        req_file.write_text("requests\n")
+
+        mock_run.return_value = MagicMock(
+            returncode=1,
+            stdout="Looking in indexes: https://bob:hunter2@pypi.internal/simple\n",
+            stderr="ERROR https://bob:hunter2@pypi.internal/simple/foo: 401",
+        )
+
+        result = install_requirements_file(req_file, timeout=5)
+
+        assert "hunter2" not in result.stdout
+        assert "hunter2" not in result.stderr
+        assert "https://***:***@pypi.internal" in result.stdout
+        assert "https://***:***@pypi.internal" in result.stderr
+
+    @patch('src.common.permission_utils.subprocess.run')
+    @patch('src.common.permission_utils.Path.exists', return_value=False)
+    def test_no_wrapper_fallback_path_redacts_stderr_and_stdout(self, mock_exists, mock_run, tmp_path):
+        """No safe_pip_install.sh wrapper -> falls straight to the
+        sys.executable pip fallback (the second subprocess.run call site);
+        its result must come back redacted too, independent of the wrapper
+        branch's own redaction above."""
+        req_file = tmp_path / "requirements.txt"
+        req_file.write_text("requests\n")
+
+        mock_run.return_value = MagicMock(
+            returncode=1,
+            stdout="Looking in indexes: https://carol:swordfish@pypi.internal/simple\n",
+            stderr="ERROR https://carol:swordfish@pypi.internal/simple/foo: 401",
+        )
+
+        result = install_requirements_file(req_file, timeout=5)
+
+        assert "swordfish" not in result.stdout
+        assert "swordfish" not in result.stderr
+        assert "https://***:***@pypi.internal" in result.stdout
+        assert "https://***:***@pypi.internal" in result.stderr

test/test_plugin_loader.py

@@ -193,7 +193,7 @@ class TestPluginLoader:
         
         mock_subprocess.return_value = MagicMock(returncode=0)
         
-        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin")
+        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir)
         
         assert result is True
         mock_subprocess.assert_called_once()
@@ -204,7 +204,7 @@ class TestPluginLoader:
         plugin_dir = tmp_plugins_dir / "test_plugin"
         plugin_dir.mkdir()
         
-        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin")
+        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir)
         
         assert result is True
         mock_subprocess.assert_not_called()
@@ -216,20 +216,23 @@ class TestPluginLoader:
         plugin_dir.mkdir()
         requirements_file = plugin_dir / "requirements.txt"
         requirements_file.write_text("package1==1.0.0\n")
-        
+
         mock_subprocess.return_value = MagicMock(returncode=1)
 
-        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin")
+        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir)
 
         assert result is False
 
+    @patch('src.plugin_system.plugin_loader.requirements_are_satisfied', return_value=False)
     @patch('subprocess.run')
     def test_install_dependencies_retries_with_ignore_installed_on_apt_conflict(
-        self, mock_subprocess, plugin_loader, tmp_plugins_dir
+        self, mock_subprocess, mock_satisfied, plugin_loader, tmp_plugins_dir
     ):
         """An apt-managed package with no pip RECORD file triggers a retry with
         --ignore-installed rather than silently assuming the old version satisfies
-        the requirement."""
+        the requirement. requirements_are_satisfied() is mocked False here because
+        this scenario is exactly the case where the installed (apt) version does
+        NOT satisfy the pin — that's why pip attempts a reinstall in the first place."""
         plugin_dir = tmp_plugins_dir / "test_plugin"
         plugin_dir.mkdir()
         requirements_file = plugin_dir / "requirements.txt"
@@ -242,16 +245,17 @@ class TestPluginLoader:
         retry_attempt = MagicMock(returncode=0, stderr="")
         mock_subprocess.side_effect = [first_attempt, retry_attempt]
 
-        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin")
+        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir)
 
         assert result is True
         assert mock_subprocess.call_count == 2
         retry_cmd = mock_subprocess.call_args_list[1][0][0]
         assert "--ignore-installed" in retry_cmd
 
+    @patch('src.plugin_system.plugin_loader.requirements_are_satisfied', return_value=False)
     @patch('subprocess.run')
     def test_install_dependencies_apt_conflict_retry_also_fails(
-        self, mock_subprocess, plugin_loader, tmp_plugins_dir
+        self, mock_subprocess, mock_satisfied, plugin_loader, tmp_plugins_dir
     ):
         """Still tolerates the failure (returns True) if the --ignore-installed
         retry itself fails, matching the prior soft-fallback behavior."""
@@ -267,14 +271,15 @@ class TestPluginLoader:
         retry_attempt = MagicMock(returncode=1, stderr="some other pip error")
         mock_subprocess.side_effect = [first_attempt, retry_attempt]
 
-        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin")
+        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir)
 
         assert result is True
         assert mock_subprocess.call_count == 2
 
+    @patch('src.plugin_system.plugin_loader.requirements_are_satisfied', return_value=False)
     @patch('subprocess.run')
     def test_install_dependencies_apt_conflict_retry_times_out(
-        self, mock_subprocess, plugin_loader, tmp_plugins_dir
+        self, mock_subprocess, mock_satisfied, plugin_loader, tmp_plugins_dir
     ):
         """A retry timeout must be tolerated the same way as a retry failure
         (return True), not propagate to the outer TimeoutExpired handler and
@@ -293,7 +298,47 @@ class TestPluginLoader:
             subprocess.TimeoutExpired(cmd="pip", timeout=300),
         ]
 
-        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin")
+        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir)
 
         assert result is True
         assert mock_subprocess.call_count == 2
+
+    @patch('subprocess.run')
+    def test_install_dependencies_already_satisfied_skips_pip(self, mock_subprocess, plugin_loader, tmp_plugins_dir):
+        """A requirement already satisfied in the current environment shouldn't invoke pip."""
+        plugin_dir = tmp_plugins_dir / "test_plugin"
+        plugin_dir.mkdir()
+        requirements_file = plugin_dir / "requirements.txt"
+        requirements_file.write_text("pytest>=1.0\n")
+
+        result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir)
+
+        assert result is True
+        mock_subprocess.assert_not_called()
+
+    def test_install_dependencies_requires_plugins_dir(self, plugin_loader, tmp_plugins_dir):
+        """plugins_dir is a required argument, not an optional trust-me flag --
+        calling without it must fail loudly (TypeError) rather than silently
+        falling back to trusting plugin_dir unchecked."""
+        plugin_dir = tmp_plugins_dir / "test_plugin"
+        plugin_dir.mkdir()
+
+        with pytest.raises(TypeError):
+            plugin_loader.install_dependencies(plugin_dir, "test_plugin")
+
+    @patch('subprocess.run')
+    def test_install_dependencies_rejects_path_outside_plugins_dir(
+        self, mock_subprocess, plugin_loader, tmp_path, tmp_plugins_dir
+    ):
+        """A plugin_dir that doesn't actually live inside plugins_dir (e.g. a
+        manifest-derived id crafted to traverse elsewhere) must be rejected
+        rather than read from -- this is the path-injection containment
+        check CodeQL flagged as missing."""
+        outside_dir = tmp_path / "outside"
+        outside_dir.mkdir()
+        (outside_dir / "requirements.txt").write_text("requests>=2.0\n")
+
+        result = plugin_loader.install_dependencies(outside_dir, "evil_plugin", plugins_dir=tmp_plugins_dir)
+
+        assert result is False
+        mock_subprocess.assert_not_called()

test/test_plugin_system.py

@@ -133,8 +133,9 @@ class TestPluginLoader:
     
     def test_dependency_check(self):
         """Test dependency checking logic."""
-        # This would test _check_dependencies_installed and _install_plugin_dependencies
-        # which requires mocking subprocess calls and file operations
+        # Covered by test_plugin_loader.py's install_dependencies tests,
+        # which exercise requirements_has_real_deps/requirements_are_satisfied
+        # and the pip subprocess fallback.
 
 
 class TestPluginExecutor: