mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-18 03:54:17 +02:00
fix(oidc): never log the client_secret when env config is rejected
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
c163b3524a
commit
1116b43fbd
@@ -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:
|
||||
|
||||
@@ -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."""
|
||||
|
||||
Reference in New Issue
Block a user