From 77c9bdd694d43e2fdd4ac895b9ecad8beb2f84c5 Mon Sep 17 00:00:00 2001 From: Marian Date: Sat, 1 Aug 2026 08:38:07 +0000 Subject: [PATCH] fix(oidc): reject an unrecognized boolean instead of guessing _env_bool returned the default for anything outside {true,1,yes}, so BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED=on silently read as OFF and BAMBUDDY_OIDC_ENABLED=on silently disabled the provider -- the exact opposite of what .env.example claimed. Unrecognized values now raise EnvOIDCConfigError, caught in _apply_env_oidc_provider the same way a bad DEFAULT_GROUP or a ValidationError already is: logged and left running, never released on a typo. Also promotes _env_bool to env_bool now that it has a call site in auth.py, and corrects the boolean-parsing sentence in .env.example. --- .env.example | 3 +- backend/app/api/routes/auth.py | 4 +- backend/app/core/oidc_env.py | 37 +++++++++++--- .../tests/integration/test_oidc_env_apply.py | 49 ++++++++++++++++++ backend/tests/unit/test_oidc_env_reader.py | 51 +++++++++++++++++-- 5 files changed, 128 insertions(+), 16 deletions(-) diff --git a/.env.example b/.env.example index 12e4e3d1c..911f8ce96 100644 --- a/.env.example +++ b/.env.example @@ -100,7 +100,8 @@ LOG_TO_FILE=true # BAMBUDDY_OIDC_AUTOLOGIN=false # BAMBUDDY_OIDC_DEFAULT_GROUP= # -# Booleans accept true/1/yes; anything else keeps the default. +# Booleans accept true/1/yes or false/0/no (case-insensitive). Blank or unset +# uses the default; any other value is rejected and the provider is skipped. # # DEFAULT_GROUP is the group new users land in when AUTO_CREATE_USERS is on; # without it they get Viewers. It matches a group NAME exactly (case-sensitive) diff --git a/backend/app/api/routes/auth.py b/backend/app/api/routes/auth.py index 9e4e0fa43..8f279d948 100644 --- a/backend/app/api/routes/auth.py +++ b/backend/app/api/routes/auth.py @@ -35,7 +35,7 @@ from backend.app.core.auth import ( security, ) from backend.app.core.database import async_session, get_db -from backend.app.core.oidc_env import _env_bool +from backend.app.core.oidc_env import env_bool from backend.app.core.permissions import ALL_PERMISSIONS from backend.app.models.auth_ephemeral import AuthEphemeralToken, AuthRateLimitEvent, EventType, TokenType from backend.app.models.group import Group @@ -123,7 +123,7 @@ def _local_login_env_bypass() -> bool: an install whose SSO provider is unreachable. Accepted truthy values: ``true``, ``1``, ``yes`` (case-insensitive). """ - return _env_bool("BAMBUDDY_LOCAL_LOGIN", False) + return env_bool("BAMBUDDY_LOCAL_LOGIN", False) def _get_client_ip(request: Request) -> str: diff --git a/backend/app/core/oidc_env.py b/backend/app/core/oidc_env.py index 62269cb22..d5900a6e7 100644 --- a/backend/app/core/oidc_env.py +++ b/backend/app/core/oidc_env.py @@ -30,11 +30,25 @@ _REQUIRED = ( ) _TRUTHY = {"true", "1", "yes"} +_FALSY = {"false", "0", "no"} -def _env_bool(key: str, default: bool) -> bool: +class EnvOIDCConfigError(Exception): + """A BAMBUDDY_OIDC_* value the reader cannot interpret. Only ever carries a + boolean variable's name and value -- booleans are not secret, so the message + is safe to log in full (unlike client_secret, which never reaches here).""" + + +def env_bool(key: str, default: bool) -> bool: value = os.environ.get(key) - return default if value is None else value.strip().lower() in _TRUTHY + if value is None or value.strip() == "": + return default # absent or blank == unset -> default, per the module's promise + norm = value.strip().lower() + if norm in _TRUTHY: + return True + if norm in _FALSY: + return False + raise EnvOIDCConfigError(f"{key}={value!r} is not a recognized boolean (use true/1/yes or false/0/no)") def read_env_oidc_config() -> dict | None: @@ -52,13 +66,13 @@ def read_env_oidc_config() -> dict | None: "client_id": os.environ["BAMBUDDY_OIDC_CLIENT_ID"], "client_secret": os.environ["BAMBUDDY_OIDC_CLIENT_SECRET"], "scopes": (os.environ.get("BAMBUDDY_OIDC_SCOPES") or "").strip() or "openid email profile", - "is_enabled": _env_bool("BAMBUDDY_OIDC_ENABLED", True), - "auto_create_users": _env_bool("BAMBUDDY_OIDC_AUTO_CREATE_USERS", False), - "auto_link_existing_accounts": _env_bool("BAMBUDDY_OIDC_AUTO_LINK_EXISTING", False), + "is_enabled": env_bool("BAMBUDDY_OIDC_ENABLED", True), + "auto_create_users": env_bool("BAMBUDDY_OIDC_AUTO_CREATE_USERS", False), + "auto_link_existing_accounts": env_bool("BAMBUDDY_OIDC_AUTO_LINK_EXISTING", False), "email_claim": (os.environ.get("BAMBUDDY_OIDC_EMAIL_CLAIM") or "").strip() or "email", - "require_email_verified": _env_bool("BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED", True), + "require_email_verified": env_bool("BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED", True), "icon_url": (os.environ.get("BAMBUDDY_OIDC_ICON_URL") or "").strip() or None, - "is_autologin": _env_bool("BAMBUDDY_OIDC_AUTOLOGIN", False), + "is_autologin": env_bool("BAMBUDDY_OIDC_AUTOLOGIN", False), # A name, not an id: ids are assigned per install, so the same compose # file would point at a different group on every deployment. Resolved # against the database in apply_env_oidc_provider -- the reader has no @@ -116,7 +130,14 @@ async def _apply_env_oidc_provider(db: AsyncSession) -> None: from backend.app.models.oidc_provider import OIDCProvider from backend.app.schemas.auth import OIDCProviderCreate - config = read_env_oidc_config() + try: + config = read_env_oidc_config() + except EnvOIDCConfigError as exc: + # Same disposition as a ValidationError or an unmatched DEFAULT_GROUP: + # log clearly and leave any running provider as it was. Safe to log the + # full message -- EnvOIDCConfigError only ever carries a boolean var. + logger.error("BAMBUDDY_OIDC_* config rejected, provider not applied: %s", exc) + return if config is None: # Nothing to look up by name any more, so the previously managed rows are diff --git a/backend/tests/integration/test_oidc_env_apply.py b/backend/tests/integration/test_oidc_env_apply.py index 76c224e85..df5445f89 100644 --- a/backend/tests/integration/test_oidc_env_apply.py +++ b/backend/tests/integration/test_oidc_env_apply.py @@ -173,6 +173,55 @@ async def test_a_rejected_config_never_logs_the_client_secret(db_session, monkey assert "S3CR3T" not in caplog.text # not even a fragment of the value +# --- an unrecognized boolean is rejected, not guessed -------------------------- +# `_env_bool` used to return the default for anything outside {true,1,yes}, so +# BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED=on silently read as OFF and +# BAMBUDDY_OIDC_ENABLED=on silently disabled the provider. Strict parsing +# refuses the config instead -- through the same clean path a bad +# DEFAULT_GROUP or a ValidationError already uses, so a typo never releases a +# provider that was running fine. + + +@pytest.mark.asyncio +async def test_an_unrecognized_require_email_verified_leaves_a_running_provider_intact(db_session, monkeypatch, caplog): + _configure(monkeypatch) + await apply_env_oidc_provider(db_session) + original = await _env_provider(db_session) + original_id, original_enabled = original.id, original.is_enabled + + monkeypatch.setenv("BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED", "on") + with caplog.at_level(logging.ERROR): + await apply_env_oidc_provider(db_session) + + provider = await _env_provider(db_session) + assert provider is not None, "a typo must not release the provider" + assert provider.id == original_id + assert provider.is_enabled == original_enabled + assert provider.is_env_managed is True + assert "rejected" in caplog.text + assert "BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED" in caplog.text + + +@pytest.mark.asyncio +async def test_an_unrecognized_enabled_leaves_a_running_provider_intact(db_session, monkeypatch, caplog): + _configure(monkeypatch) + await apply_env_oidc_provider(db_session) + original = await _env_provider(db_session) + original_id, original_enabled = original.id, original.is_enabled + + monkeypatch.setenv("BAMBUDDY_OIDC_ENABLED", "on") + with caplog.at_level(logging.ERROR): + await apply_env_oidc_provider(db_session) + + provider = await _env_provider(db_session) + assert provider is not None, "a typo must not release the provider" + assert provider.id == original_id + assert provider.is_enabled == original_enabled + assert provider.is_env_managed is True + assert "rejected" in caplog.text + assert "BAMBUDDY_OIDC_ENABLED" in caplog.text + + @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 diff --git a/backend/tests/unit/test_oidc_env_reader.py b/backend/tests/unit/test_oidc_env_reader.py index 80cc5cb17..a9342f722 100644 --- a/backend/tests/unit/test_oidc_env_reader.py +++ b/backend/tests/unit/test_oidc_env_reader.py @@ -10,7 +10,7 @@ from __future__ import annotations import pytest -from backend.app.core.oidc_env import read_env_oidc_config +from backend.app.core.oidc_env import EnvOIDCConfigError, env_bool, read_env_oidc_config REQUIRED = { "BAMBUDDY_OIDC_NAME": "Keycloak", @@ -93,21 +93,62 @@ def test_booleans_accept_the_project_truthy_spellings(monkeypatch, raw): assert read_env_oidc_config()["auto_create_users"] is True -@pytest.mark.parametrize("raw", ["false", "0", "no", "", "off", "nonsense"]) -def test_anything_else_is_false(monkeypatch, raw): - """Only the three documented spellings enable a flag; an unrecognised value - must not silently turn on auto-create-users.""" +@pytest.mark.parametrize("raw", ["false", "FALSE", "False", "0", "no", "NO"]) +def test_falsy_values_are_false(monkeypatch, raw): _set_required(monkeypatch) monkeypatch.setenv("BAMBUDDY_OIDC_AUTO_CREATE_USERS", raw) assert read_env_oidc_config()["auto_create_users"] is False +@pytest.mark.parametrize("raw", ["on", "enabled", "y", "nonsense"]) +def test_an_unrecognized_boolean_is_rejected(monkeypatch, raw): + """Only the documented spellings are accepted; an unrecognised value must + not silently turn a flag on or off -- it must refuse the whole config + instead of guessing (M-R4 strict boolean parsing).""" + _set_required(monkeypatch) + monkeypatch.setenv("BAMBUDDY_OIDC_AUTO_CREATE_USERS", raw) + with pytest.raises(EnvOIDCConfigError, match="BAMBUDDY_OIDC_AUTO_CREATE_USERS"): + read_env_oidc_config() + + def test_a_boolean_default_of_true_can_be_turned_off(monkeypatch): _set_required(monkeypatch) monkeypatch.setenv("BAMBUDDY_OIDC_REQUIRE_EMAIL_VERIFIED", "false") assert read_env_oidc_config()["require_email_verified"] is False +# --- env_bool, tested directly ------------------------------------------------ +# The reader-level tests above pin the contract through read_env_oidc_config; +# these exercise the helper itself so its default/blank/reject behavior is +# proven independently of any particular BAMBUDDY_OIDC_* field. + + +@pytest.mark.parametrize("raw", ["false", "FALSE", "0", "no", "NO"]) +def test_env_bool_falsy_values_are_false(monkeypatch, raw): + monkeypatch.setenv("SOME_FLAG", raw) + assert env_bool("SOME_FLAG", True) is False + + +@pytest.mark.parametrize("default", [True, False]) +def test_env_bool_absent_is_the_given_default(monkeypatch, default): + monkeypatch.delenv("SOME_FLAG", raising=False) + assert env_bool("SOME_FLAG", default) is default + + +@pytest.mark.parametrize("raw", ["", " "]) +@pytest.mark.parametrize("default", [True, False]) +def test_env_bool_blank_is_the_given_default(monkeypatch, raw, default): + monkeypatch.setenv("SOME_FLAG", raw) + assert env_bool("SOME_FLAG", default) is default + + +@pytest.mark.parametrize("raw", ["on", "enabled", "y", "nonsense"]) +def test_env_bool_rejects_an_unrecognized_value(monkeypatch, raw): + monkeypatch.setenv("SOME_FLAG", raw) + with pytest.raises(EnvOIDCConfigError, match="SOME_FLAG"): + env_bool("SOME_FLAG", True) + + def test_optional_strings_override_their_defaults(monkeypatch): _set_required(monkeypatch) monkeypatch.setenv("BAMBUDDY_OIDC_SCOPES", "openid profile groups")