소스 검색

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.
Marian 1 개월 전
부모
커밋
77c9bdd694
5개의 변경된 파일128개의 추가작업 그리고 16개의 파일을 삭제
  1. 2 1
      .env.example
  2. 2 2
      backend/app/api/routes/auth.py
  3. 29 8
      backend/app/core/oidc_env.py
  4. 49 0
      backend/tests/integration/test_oidc_env_apply.py
  5. 46 5
      backend/tests/unit/test_oidc_env_reader.py

+ 2 - 1
.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)

+ 2 - 2
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:

+ 29 - 8
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

+ 49 - 0
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

+ 46 - 5
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")