Merge pull request #112 from christianlouis/codex/secret-redaction-hardening

fix: redact secret diagnostics
2026-05-22 22:03:07 +02:00
parent 4d1ed329ec eeff24fa5c
commit fbbfecf634
10 changed files with 125 additions and 15 deletions
@@ -17,6 +17,7 @@ from pydantic import BaseModel
 from sqlalchemy.orm import Session

 from app.core.database import get_db
+from app.core.redaction import redact_sensitive_text, sanitize_for_log
 from app.core.security import require_admin_auth
 from app.models.mail_source import MailSource
 from app.models.mail_source_import import MailSourceImport
@@ -134,7 +135,12 @@ class MailSourceImportResponse(BaseModel):

 def _sanitize_for_log(value: object) -> str:
    """Remove CR/LF characters from a value to prevent log injection attacks."""
-    return str(value).replace("\r", "").replace("\n", " ")
+    return sanitize_for_log(value)
+
+
+def _redact_sensitive_text(value: object) -> str:
+    """Remove log-injection characters and redact secret-like diagnostic text."""
+    return redact_sensitive_text(value)


 def _get_source_or_404(source_id: int, db: Session) -> MailSource:
@@ -404,7 +410,7 @@ async def fetch_mail_source(
        logger.warning(
            "Manual fetch warning for source id=%d: %s",
            int(source_id),
-            _sanitize_for_log(err),
+            _redact_sensitive_text(err),
        )

    return _fetch_response(source, results)
@@ -500,7 +506,7 @@ async def test_stored_mail_source(
            logger.error(
                "Gmail API test failed for source id=%d: %s",
                int(source_id),
-                _sanitize_for_log(exc),
+                _redact_sensitive_text(exc),
            )
            return {
                "success": False,
@@ -672,7 +678,7 @@ async def gmail_oauth_callback(
        logger.error(
            "Gmail token exchange error for source id=%d: %s",
            int(source_id),
-            _sanitize_for_log(exc),
+            _redact_sensitive_text(exc),
        )
        html = (
            "<html><body><p>Token exchange failed. "
@@ -746,9 +752,14 @@ async def gmail_oauth_callback_post(
            redirect_uri=payload.redirect_uri,
        )
    except ValueError as exc:
+        logger.error(
+            "Gmail token exchange error for source id=%d: %s",
+            int(source_id),
+            _redact_sensitive_text(exc),
+        )
        raise HTTPException(
            status_code=status.HTTP_400_BAD_REQUEST,
-            detail=str(exc),
+            detail="Token exchange failed. Please check the Gmail connection settings and try again.",
        ) from exc

    access_token = token_data.get("access_token")
@@ -843,7 +854,7 @@ async def gmail_fetch_reports(
        logger.warning(
            "Gmail fetch warning for source id=%d: %s",
            int(source_id),
-            _sanitize_for_log(err),
+            _redact_sensitive_text(err),
        )

    return {
@@ -0,0 +1,26 @@
+"""
+Utilities for making diagnostic text safe to store or log.
+"""
+
+import re
+
+SENSITIVE_VALUE = "**redacted**"
+
+_SENSITIVE_KEY_PATTERN = re.compile(
+    r"(?i)([\"']?\b(?:access_token|api_key|apikey|authorization|bearer|client_secret|"
+    r"gmail_client_secret|id_token|passwd|password|refresh_token|secret|token)\b[\"']?"
+    r"\s*[:=]\s*[\"']?)([^\"'\s,;&}]+)([\"']?)"
+)
+_BEARER_TOKEN_PATTERN = re.compile(r"(?i)\b(bearer)\s+([A-Za-z0-9._~+/=-]{8,})")
+
+
+def sanitize_for_log(value: object) -> str:
+    """Remove CR/LF characters from a value to prevent log injection attacks."""
+    return str(value).replace("\r", "").replace("\n", " ")
+
+
+def redact_sensitive_text(value: object) -> str:
+    """Sanitize text and redact common secret-bearing key/value fragments."""
+    text = sanitize_for_log(value)
+    text = _SENSITIVE_KEY_PATTERN.sub(r"\1" + SENSITIVE_VALUE + r"\3", text)
+    return _BEARER_TOKEN_PATTERN.sub(r"\1 " + SENSITIVE_VALUE, text)
@@ -193,7 +193,7 @@ async def require_admin_auth(

    # 2. Static admin API key
    if api_key and verify_api_key(api_key):
-        return {"auth_type": "api_key", "api_key": api_key}
+        return {"auth_type": "api_key"}

    # 3. Bearer JWT (app-issued; also covers Bearer tokens set by older clients)
    if bearer:
@@ -4,6 +4,7 @@ from typing import Any, Dict, Iterable, Optional

 from sqlalchemy.orm import Session

+from app.core.redaction import redact_sensitive_text
 from app.models.mail_source import MailSource
 from app.models.mail_source_import import MailSourceImport

@@ -24,7 +25,7 @@ DETAIL_FIELDS = {

 def _sanitize_error(value: object) -> str:
    """Return a compact, log-safe error string for storage and UI display."""
-    text = str(value).replace("\r", "").replace("\n", " ").strip()
+    text = redact_sensitive_text(value).strip()
    if len(text) > MAX_ERROR_LENGTH:
        return text[: MAX_ERROR_LENGTH - 3] + "..."
    return text
@@ -84,7 +84,7 @@ def authed_client(test_app: FastAPI, db_session):  # pylint: disable=redefined-o
    """

    async def mock_admin_auth():
-        return {"auth_type": "api_key", "api_key": "test-key"}
+        return {"auth_type": "api_key"}

    def override_get_db():
        try:
@@ -4,6 +4,7 @@ Tests for MailSource model and mail-sources API endpoints.

 import asyncio
 import json
+import logging
 from datetime import datetime
 from unittest.mock import MagicMock, patch
 from urllib.parse import parse_qs, urlparse
@@ -138,6 +139,34 @@ class TestMailSourceImportModel:
        assert len(details[0]["filename"]) == 300
        assert "ignored" not in details[0]

+    def test_record_import_attempt_redacts_sensitive_values(self, db_session: Session):
+        source = MailSource(name="Secret Sanitizer", method="GMAIL_API")
+        db_session.add(source)
+        db_session.commit()
+        db_session.refresh(source)
+
+        attempt = record_import_attempt(
+            db_session,
+            source,
+            {
+                "success": False,
+                "errors": ["provider returned access_token=ya29.raw-token"],
+                "details": [
+                    {
+                        "status": "failed",
+                        "error": 'oauth failed: {"client_secret":"GOCSPX-raw-secret"}',
+                    },
+                ],
+            },
+            started_at=datetime.utcnow(),
+            trigger="manual",
+        )
+
+        assert "ya29.raw-token" not in attempt.errors
+        assert "GOCSPX-raw-secret" not in attempt.details
+        assert "**redacted**" in attempt.errors
+        assert "**redacted**" in attempt.details
+

 class TestImportHistoryDecoding:
    """Unit tests for import-history JSON decoding helpers."""
@@ -1058,6 +1087,25 @@ class TestSanitizeForLog:

        assert _sanitize_for_log("example.com") == "example.com"

+    def test_redacts_sensitive_key_values(self):
+        from app.api.api_v1.endpoints.mail_sources import _redact_sensitive_text
+
+        text = _redact_sensitive_text(
+            'Token exchange failed: access_token=ya29.secret client_secret="GOCSPX-secret"'
+        )
+
+        assert "ya29.secret" not in text
+        assert "GOCSPX-secret" not in text
+        assert text.count("**redacted**") == 2
+
+    def test_redacts_bearer_tokens(self):
+        from app.api.api_v1.endpoints.mail_sources import _redact_sensitive_text
+
+        text = _redact_sensitive_text("Authorization failed for Bearer ya29.long-secret-token")
+
+        assert "ya29.long-secret-token" not in text
+        assert "Bearer **redacted**" in text
+

 # ---------------------------------------------------------------------------
 # Source-to-response helper (password masking)
@@ -1164,7 +1212,9 @@ class TestGmailCallbackGet:
        resp = authed_client.get(f"/api/v1/mail-sources/{source_id}/gmail/callback?code=xyz")
        assert resp.status_code == 404

-    def test_callback_token_exchange_error_returns_html_400(self, authed_client: TestClient):
+    def test_callback_token_exchange_error_returns_html_400(
+        self, authed_client: TestClient, caplog
+    ):
        """Token exchange raises ValueError – return a user-facing HTML error page."""
        create_resp = authed_client.post(
            "/api/v1/mail-sources",
@@ -1172,14 +1222,20 @@ class TestGmailCallbackGet:
        )
        source_id = create_resp.json()["id"]

+        caplog.set_level(logging.ERROR, logger="app.api.api_v1.endpoints.mail_sources")
        with patch(
            "app.api.api_v1.endpoints.mail_sources.GmailClient.exchange_code_for_tokens",
-            side_effect=ValueError("bad token"),
+            side_effect=ValueError(
+                "bad token access_token=ya29.raw-token client_secret=GOCSPX-raw-secret"
+            ),
        ):
            resp = authed_client.get(f"/api/v1/mail-sources/{source_id}/gmail/callback?code=abc")

        assert resp.status_code == 400
        assert "token exchange failed" in resp.text.lower() or "failed" in resp.text.lower()
+        assert "ya29.raw-token" not in caplog.text
+        assert "GOCSPX-raw-secret" not in caplog.text
+        assert "**redacted**" in caplog.text

    def test_callback_no_access_token_returns_html_400(self, authed_client: TestClient):
        """Exchange succeeds but Google returns no access token – HTML 400."""
@@ -1270,16 +1326,21 @@ class TestGmailCallbackPost:
        )
        assert resp.status_code == 400

-    def test_post_callback_token_exchange_error_returns_400(self, authed_client: TestClient):
+    def test_post_callback_token_exchange_error_returns_400(
+        self, authed_client: TestClient, caplog
+    ):
        create_resp = authed_client.post(
            "/api/v1/mail-sources",
            json={"name": "Post TokenErr", "method": "GMAIL_API"},
        )
        source_id = create_resp.json()["id"]

+        caplog.set_level(logging.ERROR, logger="app.api.api_v1.endpoints.mail_sources")
        with patch(
            "app.api.api_v1.endpoints.mail_sources.GmailClient.exchange_code_for_tokens",
-            side_effect=ValueError("bad exchange"),
+            side_effect=ValueError(
+                "bad exchange refresh_token=1//raw-refresh client_secret=GOCSPX-raw-secret"
+            ),
        ):
            resp = authed_client.post(
                f"/api/v1/mail-sources/{source_id}/gmail/callback",
@@ -1287,6 +1348,14 @@ class TestGmailCallbackPost:
            )

        assert resp.status_code == 400
+        assert resp.json()["detail"] == (
+            "Token exchange failed. Please check the Gmail connection settings and try again."
+        )
+        assert "1//raw-refresh" not in resp.text
+        assert "GOCSPX-raw-secret" not in resp.text
+        assert "1//raw-refresh" not in caplog.text
+        assert "GOCSPX-raw-secret" not in caplog.text
+        assert "**redacted**" in caplog.text

    def test_post_callback_no_access_token_returns_400(self, authed_client: TestClient):
        create_resp = authed_client.post(
@@ -104,6 +104,7 @@ class TestRequireAdminAuth:
                request=self._make_request(), api_key=key, bearer=None
            )
            assert result["auth_type"] == "api_key"
+            assert "api_key" not in result
        finally:
            from app.core.security import _api_keys

@@ -69,13 +69,13 @@ Quality bar:
 Objective: make self-hosted deployments safer.

 Priority tasks:
- Keep raw secrets out of diagnostics, logs, and UI responses.
 - Add startup validation for production settings.
 - Add backup and restore documentation.
 - Add a release checklist covering migrations, tests, and smoke checks.

 Delivered:
 - Documented a 1Password secret-injection deployment flow for local, Docker Compose, and systemd deployments.
+- Redacted secret-like values from mail-source diagnostics, stored import history, OAuth error logs, and validated admin auth contexts.

 ## Later Milestones

@@ -96,9 +96,10 @@ Goal: make production deployments safer and easier to operate.

 Delivered:
 - 1Password-based secret injection flow for local, Docker Compose, and systemd deployments.
+- Raw mailbox/OAuth secrets are redacted from mail-source diagnostics, import history, and OAuth error logs.
+- Admin authentication contexts no longer carry raw API keys after validation.

 Planned:
- Avoid exposing raw mailbox/OAuth secrets in logs, UI responses, and diagnostics.
 - Add startup checks for production-critical configuration.
 - Add backup/restore guidance for database deployments.
 - Add release checklist covering migrations, tests, and smoke checks.
@@ -156,6 +156,7 @@ This file tracks the specific implementation tasks for each milestone of the DMA

 ## Future Milestones
 - [x] Production secret handling guide using 1Password injection
+- [x] Redact mailbox/OAuth secrets from diagnostics, logs, import history, and auth contexts
 - [ ] Apprise notifications and alert rules
 - [ ] DNS health guidance and Cloudflare read-only inspection
 - [ ] Guided setup and operator health pages