diff --git a/backend/app/api/api_v1/endpoints/mail_sources.py b/backend/app/api/api_v1/endpoints/mail_sources.py index 785b093..b9a0d98 100644 --- a/backend/app/api/api_v1/endpoints/mail_sources.py +++ b/backend/app/api/api_v1/endpoints/mail_sources.py @@ -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 = ( "

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 { diff --git a/backend/app/core/redaction.py b/backend/app/core/redaction.py new file mode 100644 index 0000000..6dfa410 --- /dev/null +++ b/backend/app/core/redaction.py @@ -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) diff --git a/backend/app/core/security.py b/backend/app/core/security.py index 582edd0..eab7d9a 100644 --- a/backend/app/core/security.py +++ b/backend/app/core/security.py @@ -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: diff --git a/backend/app/services/import_history.py b/backend/app/services/import_history.py index 12112a7..b8270fe 100644 --- a/backend/app/services/import_history.py +++ b/backend/app/services/import_history.py @@ -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 diff --git a/backend/app/tests/conftest.py b/backend/app/tests/conftest.py index d1c146f..491f55d 100644 --- a/backend/app/tests/conftest.py +++ b/backend/app/tests/conftest.py @@ -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: diff --git a/backend/app/tests/test_mail_sources.py b/backend/app/tests/test_mail_sources.py index 1c6d87c..b89ad74 100644 --- a/backend/app/tests/test_mail_sources.py +++ b/backend/app/tests/test_mail_sources.py @@ -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( diff --git a/backend/app/tests/test_security_extra.py b/backend/app/tests/test_security_extra.py index 4e98290..9aaba79 100644 --- a/backend/app/tests/test_security_extra.py +++ b/backend/app/tests/test_security_extra.py @@ -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 diff --git a/docs/development/roadmap.md b/docs/development/roadmap.md index 74e3a45..0eb2bc5 100644 --- a/docs/development/roadmap.md +++ b/docs/development/roadmap.md @@ -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 diff --git a/docs/milestones.md b/docs/milestones.md index aed12e5..ce52628 100644 --- a/docs/milestones.md +++ b/docs/milestones.md @@ -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. diff --git a/docs/todo.md b/docs/todo.md index 49ae262..101e720 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -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