phase: 84_docs_push_error_sanitization

**Phase 84 — final verification pass: all green, no defects found** - Verified implementation: `app/core/errors.py` (verbatim lift of sync masker), `app/api/sync.py` alias import, docs-push 502 `detail=sanitize_error(str(exc))`, all five `llm.py` error sites sanitized; new/extended test pins in place - Tests: `uv run pytest` → **1714 passed, 0 failed**; targeted pins (new unit ×2 + integration ×1, existing 502 pin) → 13 passed; sync/git-sources regression → 67 passed - Coverage: `uv run pytest --cov=app --cov-report=term-missing` → **99%** (`app/core/errors.py` 100%, `app/rag/llm.py` 100%) — >90% met - E2E isolation: `uv run pytest tests/e2e/test_smoke.py -v --no-cov` → **3 passed** - Lint/types: `uv run ruff check .` → clean; `uv run pyright` → **0 errors** - Criteria: 502 masks `*****@`/never token + row untouched ✅; LLM base-URL masked, credential-free strings byte-identical ✅; `_CREDS_RE` only in `app/core/errors.py` (working-tree grep) ✅; full gate green ✅; `git diff --stat` limited to the 4 app files + 2 modified test files + 3 phase task files (untracked: new module, new unit test, complete/ dir, reports, audit plan) ✅ - Commit/phase move left to the harness per instructions (task files already in `complete/`) - No deviations; nothing to fix - Next pending phase: **85_mobile_menu_gate_overlap**
2026-09-08 01:11:45 -04:00
parent fa189dede7
commit f4150421bb
23 changed files with 732 additions and 29 deletions
@@ -26,8 +26,10 @@ triggers — commit + ``git push`` the draft's file to the
 :func:`app.core.docs_push.push_document`; success records
 ``status`` / ``branch`` / ``commit_sha`` on the row and returns
 ``DocDraftPushed``; 409 while unconfigured, 422 on a path that no
-longer passes the guard-rails, 502 on git failure with git's stderr
-in the detail — the row untouched).
+longer passes the guard-rails, 502 on git failure with the
+SANITIZED git stderr in the detail (credential ``user:pass@``
+userinfo masked by :func:`app.core.errors.sanitize_error` — phase
+84, SEC-08) — the row untouched).

 Every ``path`` (create, update **and** push) passes the shared
 :func:`validate_draft_path` guard, so no draft can ever be created,
@@ -45,6 +47,7 @@ from sqlalchemy.orm import Session
 from app.config import Settings, get_settings
 from app.core.auth import require_admin
 from app.core.docs_push import DocsPushError, push_document
+from app.core.errors import sanitize_error
 from app.db import get_db
 from app.models import DocDraft
 from app.schemas import DocDraft as DocDraftOut
@@ -251,11 +254,16 @@ def push_doc_draft(
    3. the stored ``path`` re-runs :func:`validate_draft_path` → 422
       on the first violated rule (a row must not be pushable into a
       bad path, whatever wrote it);
-    4. :class:`DocsPushError` → 502 with ``detail=str(exc)`` — git's
-       stderr, the ``GitSyncError`` → ``detail`` mapping from
-       :mod:`app.api.git_sources`. Only a SUCCESS mutates the row:
-       the failed push leaves ``status`` / ``branch`` / ``commit_sha``
-       exactly as found (no partial commit).
+    4. :class:`DocsPushError` → 502 with the SANITIZED git stderr in
+       the detail (phase 84, SEC-08): ``str(exc)`` runs through
+       :func:`app.core.errors.sanitize_error`, which masks any
+       ``user:pass@`` userinfo git's stderr may echo (a
+       ``BOR_DOCS_REPO`` URL with embedded credentials) — the failing
+       repo and git's reason stay readable, and the
+       ``GitSyncError`` → ``detail`` mapping from
+       :mod:`app.api.git_sources` is otherwise kept. Only a SUCCESS
+       mutates the row: the failed push leaves ``status`` / ``branch``
+       / ``commit_sha`` exactly as found (no partial commit).

    On success the row becomes ``status = "pushed"`` with the landed
    ``branch`` and ``commit_sha`` (must equal ``git rev-parse
@@ -283,7 +291,11 @@ def push_doc_draft(
            commit_message=f"docs: {row.title}",
        )
    except DocsPushError as exc:
-        raise HTTPException(status_code=502, detail=str(exc)) from None
+        # Phase 84 (SEC-08): git's stderr may echo the remote URL with
+        # embedded credentials — the RESPONSE is where the secret would
+        # leak, so it is sanitized here (the DocsPushError itself keeps
+        # carrying the full stderr for logs/inspection).
+        raise HTTPException(status_code=502, detail=sanitize_error(str(exc))) from None
    row.status = "pushed"
    row.branch = branch
    row.commit_sha = sha
@@ -57,12 +57,15 @@ carries the phase-64 per-file progress — ``current_file`` (the
 ``files_done`` / ``files_total`` — null/0/0 before the import starts
 (clone/pull reports no file yet) and in terminal states, which clear
 ``current_file`` but keep the run's final counts.
+
+The ``failed`` state's ``error`` string is masked by the shared
+sanitizer — the ``user:pass@`` masker now lives in :mod:`app.core.errors`
+(imported here under the private name ``_sanitize_error``).
 """
 from __future__ import annotations

 import asyncio
 import logging
-import re
 from dataclasses import dataclass, field
 from datetime import UTC, datetime
 from pathlib import Path
@@ -72,6 +75,7 @@ from fastapi import APIRouter, Depends, HTTPException

 from app.config import get_settings
 from app.core.auth import require_admin
+from app.core.errors import sanitize_error as _sanitize_error
 from app.db import SessionLocal
 from app.rag.git_sources import effective_sources
 from app.rag.importer import ImportSummary, import_sources
@@ -89,20 +93,6 @@ router = APIRouter(
    dependencies=[Depends(require_admin)],  # phase 16 pattern: admin-only surface
 )

-#: ``user:pass@`` inside any error text (git stderr, endpoint URLs) —
-#: masked so a sync failure can never leak credentials into the UI.
-_CREDS_RE = re.compile(r"[A-Za-z0-9._~%*-]+:[A-Za-z0-9._~%*-]+@")
-
-
-def _sanitize_error(message: str) -> str:
-    """Mask credentials embedded in an error string (no secrets in the UI).
-
-    Git's stderr is otherwise surfaced verbatim (phase locked decisions) —
-    it names the failing repo and git's reason, which is what the admin
-    needs to fix things.
-    """
-    return _CREDS_RE.sub("*****@", message)
-

@dataclass
 class SyncStatus:
@@ -0,0 +1,57 @@
+"""Shared error-string sanitizer (phase 84 — audit SEC-08 / SEC-13).
+
+``sanitize_error`` masks ``user:pass@`` userinfo in an error string so
+no error surface ever ships an embedded credential into the UI, a
+response body, or an SSE frame. It is the verbatim lift of the phase-32
+sync masker (same regex, same ``*****@`` replacement — byte-identical
+behavior); ``app/api/sync.py`` keeps the private name ``_sanitize_error``
+as an alias import, and the docs-push 502 detail (SEC-08) and the LLM
+error messages (SEC-13) run through the same function.
+
+Audit basis (``.agents/remediation_plan.md``, security audit 2026-09-07):
+
+* SEC-08 (Medium) — the docs-push 502 surfaced git's stderr verbatim;
+  ``BOR_DOCS_REPO`` is documented as "any remote (URL or local path)",
+  so an ``https://user:token@host/...`` URL is a normal config shape,
+  and a failed push (revoked token, network) echoes the remote URL in
+  git's stderr straight into the browser and the logs. The sync path
+  already solved exactly this problem — the docs-push and LLM surfaces
+  simply never got the treatment.
+* SEC-13 (Low) — the LLM error f-strings interpolated
+  ``settings.llm_base_url`` raw; a base URL configured with embedded
+  credentials would echo into SSE ``error`` frames and logs. Same class
+  of leak, same fix.
+
+Contract (narrow userinfo-regex only — byte-identical for
+credential-free text):
+
+* Only the git/HTTP ``user:pass@`` userinfo shape is rewritten — a run
+  of userinfo characters, the separating colon, a second run, and the
+  ``@``. Ordinary text survives character for character: plain hosts,
+  ``https://host/...`` URLs without userinfo, prose with a colon +
+  space (``fatal: ...``), emails in prose (no colon userinfo run
+  before the ``@``). The existing sync / git-sources / LLM error-string
+  tests are the tripwire proving the move is behavior-identical.
+* The replacement shape is ``*****@`` — the existing sync copy, so
+  every already-pinned masked error stays byte-identical.
+* Idempotent for the masked forms the surfaces produce (sanitizing an
+  already-sanitized git/URL failure string is a no-op).
+"""
+from __future__ import annotations
+
+import re
+
+#: ``user:pass@`` inside any error text (git stderr, endpoint URLs) —
+#: the narrow userinfo run (git/HTTP convention) that carries
+#: credentials; everything else is left untouched.
+_CREDS_RE = re.compile(r"[A-Za-z0-9._~%*-]+:[A-Za-z0-9._~%*-]+@")
+
+
+def sanitize_error(message: str) -> str:
+    """Mask credentials embedded in an error string (no secrets in the UI).
+
+    Git's stderr is otherwise surfaced verbatim (the admin needs the
+    failing repo and git's reason to fix things) — only the ``user:pass@``
+    userinfo shape is rewritten, to ``*****@``.
+    """
+    return _CREDS_RE.sub("*****@", message)
@@ -29,6 +29,7 @@ from openai import AsyncOpenAI, AsyncStream
 from openai.types.chat import ChatCompletionChunk, ChatCompletionMessageParam

 from app.config import Settings, get_settings
+from app.core.errors import sanitize_error

 if TYPE_CHECKING:
    # Phase 71: the filter type is only needed for typing (the module
@@ -285,7 +286,11 @@ class LLMClient:
                raise
            except Exception as e:  # noqa: BLE001 — wrap transport-level failures
                raise EmbeddingError(
-                    f"embeddings request to {self.settings.llm_base_url} failed: {e}"
+                    # Phase 84 (SEC-13): a base URL configured with
+                    # embedded ``user:pass@`` credentials must not reach
+                    # the error string — sanitized at construction.
+                    f"embeddings request to {sanitize_error(self.settings.llm_base_url)} "
+                    f"failed: {e}"
                ) from e
            self.embed_batches += 1
            self._check_dims(vecs)
@@ -324,18 +329,20 @@ class LLMClient:
            raise
        except Exception as e:  # noqa: BLE001 — wrap transport-level failures
            raise LLMError(
-                f"chat completion from {self.settings.llm_base_url} failed: {e}"
+                # Phase 84 (SEC-13): sanitize the base URL (see embed).
+                f"chat completion from {sanitize_error(self.settings.llm_base_url)} "
+                f"failed: {e}"
            ) from e
        if not resp.choices:
            raise LLMError(
-                f"chat completion from {self.settings.llm_base_url} "
+                f"chat completion from {sanitize_error(self.settings.llm_base_url)} "
                "returned no choices"
            )
        content = resp.choices[0].message.content
        if content is None or not content.strip():
            raise LLMError(
-                f"chat completion from {self.settings.llm_base_url} returned "
-                "empty content — refusing to store a silent summary"
+                f"chat completion from {sanitize_error(self.settings.llm_base_url)} "
+                "returned empty content — refusing to store a silent summary"
            )
        return content.strip()

@@ -483,7 +490,10 @@ class LLMClient:
        except LLMError:
            raise
        except Exception as e:  # noqa: BLE001 — wrap transport-level failures
-            raise LLMError(f"chat stream from {self.settings.llm_base_url} failed: {e}") from e
+            # Phase 84 (SEC-13): sanitize the base URL (see embed).
+            raise LLMError(
+                f"chat stream from {sanitize_error(self.settings.llm_base_url)} failed: {e}"
+            ) from e
        finally:
            # Phase 48: deterministic teardown — whenever ``create()``
            # succeeded, close the endpoint's stream on every subsequent