From 6dff1e964471ab3de6e0d72b998b7cb69830f3f3 Mon Sep 17 00:00:00 2001 From: Marian Date: Fri, 31 Jul 2026 13:10:14 +0000 Subject: [PATCH] fix(oidc): make apply_env_oidc_provider never raise on DB errors The db.execute/db.commit calls in the upsert and release paths sat outside the try/except that only wrapped OIDCProviderCreate, so a commit failure at startup (connection blip, WAL lock) propagated out of the lifespan and took the instance down -- the exact outcome this module exists to avoid. The body now runs inside a private _apply_env_oidc_provider(), with the public entry point catching, logging and rolling back on any exception. --- backend/app/core/oidc_env.py | 16 ++++++++++++-- .../tests/integration/test_oidc_env_apply.py | 22 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/backend/app/core/oidc_env.py b/backend/app/core/oidc_env.py index 07cfc54ce..16a3f3d71 100644 --- a/backend/app/core/oidc_env.py +++ b/backend/app/core/oidc_env.py @@ -90,9 +90,21 @@ _APPLIED_FIELDS = ( async def apply_env_oidc_provider(db: AsyncSession) -> None: """Upsert the env-managed provider, or release it when the config is gone. - Never raises: this runs during startup, and a typo in one variable must not - stop the app from booting. A rejected config is logged and skipped. + Never raises: this runs during startup, and a typo in one variable -- or a + DB error on commit -- must not stop the app from booting. A rejected + config is logged and skipped. """ + try: + await _apply_env_oidc_provider(db) + except Exception as exc: # noqa: BLE001 -- startup must survive any failure here + # Never str(exc): a DB error message can echo a configured value. Class only. + logger.error("BAMBUDDY_OIDC_* could not be applied: %s", type(exc).__name__) + # A commit may have half-applied; roll back so the shared session is + # left clean for the rest of startup. + await db.rollback() + + +async def _apply_env_oidc_provider(db: AsyncSession) -> None: # Imported here rather than at module scope: app.core is imported by the # models themselves, so a top-level import would be a cycle. from backend.app.models.group import Group diff --git a/backend/tests/integration/test_oidc_env_apply.py b/backend/tests/integration/test_oidc_env_apply.py index 1d9066fd8..965ee1c78 100644 --- a/backend/tests/integration/test_oidc_env_apply.py +++ b/backend/tests/integration/test_oidc_env_apply.py @@ -198,6 +198,28 @@ async def test_a_non_validation_error_is_survivable_and_leaks_nothing(db_session assert "leaked-secret" not in caplog.text # ...but nothing from the message +@pytest.mark.asyncio +async def test_a_commit_failure_is_survivable_and_leaks_nothing(db_session, monkeypatch, caplog): + """The upsert's db.execute/db.commit calls sit outside the inner + ValidationError guard -- a Postgres blip or a SQLite WAL lock at startup + must not propagate out of the lifespan either. Only the exception class + may be logged, never str(exc), since a DB error message can echo a + configured value.""" + + async def _raise_on_commit(): + raise RuntimeError("database is locked") + + monkeypatch.setattr(db_session, "commit", _raise_on_commit) + _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 "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."""