mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-02 09:18:06 +00:00
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).
This commit is contained in:
@@ -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 "
|
||||
|
||||
Reference in New Issue
Block a user