Browse Source

fix(oidc): strip the required BAMBUDDY_OIDC_* values and register the local-login bypass

A Kubernetes Secret written as a block scalar carries a trailing newline, and
the schema bounds the four required variables by max_length only, so an
unstripped issuer_url was stored and enabled and then raised httpx.InvalidURL
on the first click of the SSO button -- the authorize-time failure the
all-or-nothing rule exists to prevent. Whitespace-only values got through the
same way, contradicting the reader's own "an empty required var counts as
unset". The optional variables have always treated blank as unset; the
required ones now do too.

Also registers BAMBUDDY_LOCAL_LOGIN (#1589) in the typo guard, which logged
"possible typo" for it on every boot while listing every BAMBUDDY_OIDC_*
variable as legitimate.
maziggy 1 month ago
parent
commit
aef4f3a3e9

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 4 - 0
backend/app/core/config.py

@@ -135,6 +135,10 @@ _INTENTIONAL_UNSETTINGS = {
     "LOG_DIR",  # config.py (above)
     "LOG_DIR",  # config.py (above)
     "LOG_LEVEL",  # main.py logging setup
     "LOG_LEVEL",  # main.py logging setup
     "BUG_REPORT_RELAY_URL",  # config.py (above)
     "BUG_REPORT_RELAY_URL",  # config.py (above)
+    # #1589 — api/routes/auth.py reads this on the login path. Unregistered it
+    # logged "possible typo" at every boot, telling an operator who is locked
+    # out and following the documented recovery that the variable is not real.
+    "BAMBUDDY_LOCAL_LOGIN",
     # #2593 — core/oidc_env.py reads these directly; they are not Settings
     # #2593 — core/oidc_env.py reads these directly; they are not Settings
     # fields because they map to an OIDCProvider row, not to app config.
     # fields because they map to an OIDCProvider row, not to app config.
     "BAMBUDDY_OIDC_NAME",
     "BAMBUDDY_OIDC_NAME",

+ 14 - 6
backend/app/core/oidc_env.py

@@ -65,16 +65,24 @@ def read_env_oidc_config() -> dict | None:
     """The provider's fields from the environment, or None if it isn't configured.
     """The provider's fields from the environment, or None if it isn't configured.
 
 
     An empty required var counts as unset -- `BAMBUDDY_OIDC_CLIENT_SECRET=` in
     An empty required var counts as unset -- `BAMBUDDY_OIDC_CLIENT_SECRET=` in
-    a compose file is a forgotten value, not an intentional empty secret.
+    a compose file is a forgotten value, not an intentional empty secret. Blank
+    means blank *after* stripping, and the surviving value is stripped too: a
+    Kubernetes Secret written as a block scalar (``stringData: secret: |``) or
+    created from a file carries a trailing newline that nothing downstream
+    rejects -- max_length is the only bound the schema puts on these four. An
+    issuer_url with a trailing newline is stored and enabled, and then fails
+    with httpx.InvalidURL on the first click of the SSO button, which is the
+    authorize-time failure the all-or-nothing rule above exists to prevent.
     """
     """
-    if not all(os.environ.get(key) for key in _REQUIRED):
+    required = {key: (os.environ.get(key) or "").strip() for key in _REQUIRED}
+    if not all(required.values()):
         return None
         return None
 
 
     return {
     return {
-        "name": os.environ["BAMBUDDY_OIDC_NAME"],
-        "issuer_url": os.environ["BAMBUDDY_OIDC_ISSUER_URL"],
-        "client_id": os.environ["BAMBUDDY_OIDC_CLIENT_ID"],
-        "client_secret": os.environ["BAMBUDDY_OIDC_CLIENT_SECRET"],
+        "name": required["BAMBUDDY_OIDC_NAME"],
+        "issuer_url": required["BAMBUDDY_OIDC_ISSUER_URL"],
+        "client_id": required["BAMBUDDY_OIDC_CLIENT_ID"],
+        "client_secret": required["BAMBUDDY_OIDC_CLIENT_SECRET"],
         "scopes": (os.environ.get("BAMBUDDY_OIDC_SCOPES") or "").strip() or "openid email profile",
         "scopes": (os.environ.get("BAMBUDDY_OIDC_SCOPES") or "").strip() or "openid email profile",
         "is_enabled": env_bool("BAMBUDDY_OIDC_ENABLED", True),
         "is_enabled": env_bool("BAMBUDDY_OIDC_ENABLED", True),
         "auto_create_users": env_bool("BAMBUDDY_OIDC_AUTO_CREATE_USERS", False),
         "auto_create_users": env_bool("BAMBUDDY_OIDC_AUTO_CREATE_USERS", False),

+ 9 - 0
backend/tests/integration/test_local_login_gate.py

@@ -116,6 +116,15 @@ class TestLocalLoginGate:
         # Bypass stays off (same 401 as no env var), and crucially not a 500.
         # Bypass stays off (same 401 as no env var), and crucially not a 500.
         assert response.status_code == 401, response.text
         assert response.status_code == 401, response.text
 
 
+    def test_the_bypass_var_is_registered_in_the_typo_guard(self):
+        """config.py logs "possible typo" for any unregistered BAMBUDDY_* var.
+        Unregistered, this one tells an operator who is locked out and following
+        the documented recovery that the variable they just set is not real --
+        while the same line lists every BAMBUDDY_OIDC_* var as legitimate."""
+        from backend.app.core.config import _INTENTIONAL_UNSETTINGS
+
+        assert "BAMBUDDY_LOCAL_LOGIN" in _INTENTIONAL_UNSETTINGS
+
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
     async def test_forgot_password_rejected_when_local_disabled(
     async def test_forgot_password_rejected_when_local_disabled(

+ 26 - 3
backend/tests/unit/test_oidc_env_reader.py

@@ -56,14 +56,37 @@ def test_returns_none_when_any_single_required_var_is_missing(monkeypatch, missi
     assert read_env_oidc_config() is None
     assert read_env_oidc_config() is None
 
 
 
 
-def test_an_empty_required_var_counts_as_unset(monkeypatch):
+@pytest.mark.parametrize("raw", ["", "   ", "\n", " \t\n "])
+@pytest.mark.parametrize("key", sorted(REQUIRED))
+def test_an_empty_required_var_counts_as_unset(monkeypatch, key, raw):
     """`BAMBUDDY_OIDC_CLIENT_SECRET=` in a compose file is a forgotten value,
     """`BAMBUDDY_OIDC_CLIENT_SECRET=` in a compose file is a forgotten value,
-    not an intentional empty secret."""
+    not an intentional empty secret -- and neither is one holding only
+    whitespace, which the optional vars have always treated as unset."""
     _set_required(monkeypatch)
     _set_required(monkeypatch)
-    monkeypatch.setenv("BAMBUDDY_OIDC_CLIENT_SECRET", "")
+    monkeypatch.setenv(key, raw)
     assert read_env_oidc_config() is None
     assert read_env_oidc_config() is None
 
 
 
 
+@pytest.mark.parametrize("key", sorted(REQUIRED))
+def test_a_required_var_is_stripped(monkeypatch, key):
+    """A Kubernetes Secret written as a block scalar carries a trailing
+    newline, and the schema bounds these four by max_length only -- so an
+    unstripped issuer_url reaches the database, enables the SSO button and
+    then raises httpx.InvalidURL on the first click, long after startup could
+    have refused it."""
+    _set_required(monkeypatch)
+    monkeypatch.setenv(key, f"  {REQUIRED[key]}\n")
+
+    cfg = read_env_oidc_config()
+    field = {
+        "BAMBUDDY_OIDC_NAME": "name",
+        "BAMBUDDY_OIDC_ISSUER_URL": "issuer_url",
+        "BAMBUDDY_OIDC_CLIENT_ID": "client_id",
+        "BAMBUDDY_OIDC_CLIENT_SECRET": "client_secret",
+    }[key]
+    assert cfg[field] == REQUIRED[key]
+
+
 def test_reads_the_required_vars(monkeypatch):
 def test_reads_the_required_vars(monkeypatch):
     _set_required(monkeypatch)
     _set_required(monkeypatch)
     cfg = read_env_oidc_config()
     cfg = read_env_oidc_config()

File diff suppressed because it is too large
+ 0 - 0
static/assets/index-CCCWDEkl.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
 
     <!-- Splash screens for iOS -->
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-CbDmTKuP.js"></script>
+    <script type="module" crossorigin src="/assets/index-CCCWDEkl.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-C_6BSgrK.css">
     <link rel="stylesheet" crossorigin href="/assets/index-C_6BSgrK.css">
   </head>
   </head>
   <body>
   <body>

Some files were not shown because too many files changed in this diff