diff --git a/src/common/permission_utils.py b/src/common/permission_utils.py index ecf217c2..adf5a564 100644 --- a/src/common/permission_utils.py +++ b/src/common/permission_utils.py @@ -8,6 +8,7 @@ files that need to be accessible by both root service and web user. import os import logging +import re import shutil as _shutil import subprocess import sys @@ -16,6 +17,25 @@ from typing import Optional logger = logging.getLogger(__name__) +# Matches the credentials portion of a "scheme://user:pass@host" URL, so pip's +# own error output can be logged/displayed without echoing back a private +# index URL's embedded basic-auth secret verbatim (e.g. from a +# requirements.txt --index-url line or the PIP_INDEX_URL env var). +_URL_CREDENTIALS_RE = re.compile(r'://[^/\s@:]+:[^/\s@]+@') + + +def _redact_url_credentials(text: Optional[str]) -> str: + """Replace embedded user:pass@ URL credentials in text with a placeholder. + + Safe to call on any subprocess output destined for logs: it only ever + shortens/replaces the credential substring, never changes the presence + or absence of the specific fixed phrases callers check for + (e.g. "a password is required"), so it can't affect control flow. + """ + if not text: + return text or "" + return _URL_CREDENTIALS_RE.sub('://***:***@', text) + # System directories that should never have their permissions modified # These directories have special system-level permissions that must be preserved PROTECTED_SYSTEM_DIRECTORIES = { # nosec B108 - these are checked to PREVENT permission changes, not to use as temp paths @@ -338,6 +358,13 @@ def install_requirements_file(req_file: Path, timeout: int = 300) -> subprocess. ["sudo", "-n", bash_path, str(wrapper), str(req_file)], capture_output=True, text=True, timeout=timeout, cwd=str(project_root) ) + # Redact immediately: pip can echo a private index URL's embedded + # basic-auth credentials back in its own error/progress output + # (e.g. from a requirements.txt --index-url line). Doesn't affect + # the fixed-phrase "denied" check below -- those phrases never + # overlap with URL syntax. + result.stderr = _redact_url_credentials(result.stderr) + result.stdout = _redact_url_credentials(result.stdout) if result.returncode == 0: return result # Distinguish "sudo rejected this exact command line" (worth @@ -386,6 +413,7 @@ def install_requirements_file(req_file: Path, timeout: int = 300) -> subprocess. [sys.executable, "-m", "pip", "install", "--break-system-packages", "--ignore-installed", "-r", str(req_file)], capture_output=True, text=True, timeout=timeout, cwd=str(project_root) ) - result.stdout = note + (result.stdout or "") + result.stderr = _redact_url_credentials(result.stderr) + result.stdout = note + _redact_url_credentials(result.stdout) return result diff --git a/test/test_permission_utils.py b/test/test_permission_utils.py new file mode 100644 index 00000000..c09ad389 --- /dev/null +++ b/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