From 22f0a96cbbe81b10ed58c24271ee73fd33468ce0 Mon Sep 17 00:00:00 2001
From: ChuckBuilds <charlesmynard@gmail.com>
Date: Fri, 10 Jul 2026 16:28:29 -0400
Subject: [PATCH] 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).
---
 src/common/permission_utils.py | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/common/permission_utils.py b/src/common/permission_utils.py
index adf5a564..3679c253 100644
--- a/src/common/permission_utils.py
+++ b/src/common/permission_utils.py
@@ -375,16 +375,24 @@ def install_requirements_file(req_file: Path, timeout: int = 300) -> subprocess.
                 for phrase in ("a password is required", "is not allowed to run", "no tty present")
             )
             if not denied:
+                # Deliberately don't interpolate req_file or the pip output here:
+                # this log line is scanner-visible, and a static analyzer can't
+                # tell "already redacted above" from "still raw" just by looking
+                # at this call in isolation. The full (redacted) text is still
+                # available to callers via the returned CompletedProcess.
                 logger.warning(
-                    "Root pip install failed (rc=%s) for %s: %s",
-                    result.returncode, req_file, result.stderr.strip()[:500],
+                    "Root pip install failed (rc=%s); see the returned "
+                    "CompletedProcess.stderr for details.",
+                    result.returncode,
                 )
                 return result
 
+        # Same reasoning as above: no req_file / pip-output interpolation in
+        # this log line, only in the returned note/CompletedProcess.
         logger.warning(
-            "Root pip install wrapper denied via sudo for %s; falling back to "
-            "user-level install: %s",
-            req_file, result.stderr.strip()[:500] if result else "no bash candidates found",
+            "Root pip install wrapper denied via sudo for all candidates; "
+            "falling back to user-level install. See the returned "
+            "CompletedProcess.stderr for details."
         )
         note = (
             f"[Root install unavailable ({(result.stderr.strip() if result else 'sudo denied') or 'sudo denied'}); "
@@ -394,8 +402,7 @@ def install_requirements_file(req_file: Path, timeout: int = 300) -> subprocess.
         )
     else:
         logger.warning(
-            "safe_pip_install.sh not found; falling back to user-level install for %s",
-            req_file,
+            "safe_pip_install.sh not found; falling back to user-level install."
         )
         note = (
             "[safe_pip_install.sh not found; installed for the current process's "