From 1116b43fbdd656f63776d7b1b18d63e5f1f30337 Mon Sep 17 00:00:00 2001 From: Marian Date: Tue, 28 Jul 2026 18:49:58 +0000 Subject: [PATCH] fix(oidc): never log the client_secret when env config is rejected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apply_env_oidc_provider logged the raw Pydantic exception on rejection. client_secret has max_length=512, so a longer value raises string_too_long and str(exc) embeds input_value=..., leaking BAMBUDDY_OIDC_CLIENT_SECRET into the logs (maziggy review, PR #2625). Split the catch: ValidationError logs errors(include_input=False), which strips submitted values; any other exception logs only its class name, never str(exc). Rejection stays survivable — a bad config is still skipped and the app still boots. Adds two regression tests: an over-long secret is rejected without the value reaching the log, and a non-ValidationError is survived without leaking its message. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/app/core/oidc_env.py | 14 +++++- .../tests/integration/test_oidc_env_apply.py | 44 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/backend/app/core/oidc_env.py b/backend/app/core/oidc_env.py index 39f9d6078..c8e769dd1 100644 --- a/backend/app/core/oidc_env.py +++ b/backend/app/core/oidc_env.py @@ -12,6 +12,7 @@ from __future__ import annotations import logging import os +from pydantic import ValidationError from sqlalchemy import select, update from sqlalchemy.ext.asyncio import AsyncSession @@ -122,8 +123,19 @@ async def apply_env_oidc_provider(db: AsyncSession) -> None: # The same schema the API uses, so env config cannot reach a state the # UI would have refused (notably the SEC-1 auto-link check). validated = OIDCProviderCreate(**config) + except ValidationError as exc: + # errors(include_input=False) strips the submitted values -- str(exc) + # embeds input_value=... and would leak BAMBUDDY_OIDC_CLIENT_SECRET. + logger.error( + "BAMBUDDY_OIDC_* config rejected, provider not applied: %s", + exc.errors(include_input=False), + ) + return except Exception as exc: # noqa: BLE001 -- any rejection must be survivable - logger.error("BAMBUDDY_OIDC_* config rejected, provider not applied: %s", exc) + # Log only the exception class, never str(exc): an unexpected error here + # could carry a configured value in its message. Structural guarantee, + # not one contingent on which exceptions the schema validators raise. + logger.error("BAMBUDDY_OIDC_* config could not be applied: %s", type(exc).__name__) return if existing is None: diff --git a/backend/tests/integration/test_oidc_env_apply.py b/backend/tests/integration/test_oidc_env_apply.py index ac222431d..8f97842fd 100644 --- a/backend/tests/integration/test_oidc_env_apply.py +++ b/backend/tests/integration/test_oidc_env_apply.py @@ -7,6 +7,8 @@ recreating the provider would silently unlink every account bound to it. from __future__ import annotations +import logging + import pytest from sqlalchemy import select @@ -152,6 +154,48 @@ async def test_an_unsafe_auto_link_config_is_skipped_not_raised(db_session, monk assert await _env_provider(db_session) is None +@pytest.mark.asyncio +async def test_a_rejected_config_never_logs_the_client_secret(db_session, monkeypatch, caplog): + """client_secret has max_length=512, so an over-long value raises + string_too_long. The rejection must be logged without the value: str(exc) + embeds input_value=..., which would leak the secret (no-secrets-in-logs).""" + secret = "S3CR3T" * 100 # > 512 chars -> ValidationError on client_secret + _configure(monkeypatch, BAMBUDDY_OIDC_CLIENT_SECRET=secret) + + with caplog.at_level(logging.ERROR): + await apply_env_oidc_provider(db_session) + + assert await _env_provider(db_session) is None # rejected, not booted-through + assert "rejected" in caplog.text # the rejection was actually logged + assert secret not in caplog.text + assert "S3CR3T" not in caplog.text # not even a fragment of the value + + +@pytest.mark.asyncio +async def test_a_non_validation_error_is_survivable_and_leaks_nothing(db_session, monkeypatch, caplog): + """The generic except branch handles anything that isn't a ValidationError + (e.g. a library call raising mid-construction). It must not stop boot and, + since such a message could carry a configured value, must log only the + exception class -- never str(exc).""" + # oidc_env imports OIDCProviderCreate inside the function (to avoid an + # import cycle), so patch it at its source module, not on oidc_env. + import backend.app.schemas.auth as auth_schemas + + def _raise(**_kwargs): + raise RuntimeError("boom leaked-secret") + + monkeypatch.setattr(auth_schemas, "OIDCProviderCreate", _raise) + _configure(monkeypatch, BAMBUDDY_OIDC_CLIENT_SECRET="leaked-secret") + + with caplog.at_level(logging.ERROR): + await apply_env_oidc_provider(db_session) # must not raise + + assert await _env_provider(db_session) is None + assert "could not be applied" in caplog.text + assert "RuntimeError" in caplog.text # class is logged... + assert "leaked-secret" not in caplog.text # ...but nothing from the message + + @pytest.mark.asyncio async def test_applying_twice_without_changes_is_a_no_op(db_session, monkeypatch): """Every boot re-applies; the second run must not create a second row."""