diff --git a/api/auth.py b/api/auth.py index 2b429d26..fa10e6fb 100644 --- a/api/auth.py +++ b/api/auth.py @@ -493,7 +493,8 @@ def is_oidc_auth_enabled() -> bool: def get_oidc_startup_warning() -> str | None: - """Return a startup warning when OIDC auth is only partially configured.""" + """Return a startup warning when OIDC auth is only partially configured, + or when allow_values uses whitespace that is no longer a separator.""" try: cfg = get_config() raw = cfg.get("webui_oidc") if isinstance(cfg, dict) else {} @@ -515,24 +516,40 @@ def get_oidc_startup_warning() -> str | None: if not any((issuer, client_id, allow_claim, allow_values)): return None - if issuer and client_id and allow_claim and allow_values: - return None - missing = [] - if not issuer: - missing.append("issuer") - if not client_id: - missing.append("client_id") - if not allow_claim: - missing.append("allow_claim") - if not allow_values: - missing.append("allow_values") + warnings = [] - joined = ", ".join(missing) - return ( - "Native OIDC login is only partially configured; missing " - f"{joined}. The WebUI will not enable OIDC auth until all four fields are set." - ) + if not (issuer and client_id and allow_claim and allow_values): + missing = [] + if not issuer: + missing.append("issuer") + if not client_id: + missing.append("client_id") + if not allow_claim: + missing.append("allow_claim") + if not allow_values: + missing.append("allow_values") + joined = ", ".join(missing) + warnings.append( + "Native OIDC login is only partially configured; missing " + f"{joined}. The WebUI will not enable OIDC auth until all four fields are set." + ) + + # Detect whitespace-only allow_values scalar that may contain multiple intended values. + # Runs unconditionally so the warning reaches startup even when other auth methods + # short-circuit is_auth_enabled() before the OIDC branch is evaluated. + raw_allow_env = os.getenv("HERMES_WEBUI_OIDC_ALLOW_VALUES") + raw_allow = raw_allow_env if raw_allow_env is not None else raw.get("allow_values") + if raw_allow is not None and not isinstance(raw_allow, (list, tuple, set)): + try: + from api.auth_oidc import _normalize_allow_values, _ALLOW_VALUES_WHITESPACE_WARNING + normalized = _normalize_allow_values(raw_allow) + if any(any(ch.isspace() for ch in v) for v in normalized): + warnings.append(_ALLOW_VALUES_WHITESPACE_WARNING) + except Exception: + logger.debug("Failed to check OIDC allow_values for whitespace", exc_info=True) + + return "\n".join(warnings) if warnings else None def is_auth_enabled() -> bool: diff --git a/api/auth_oidc.py b/api/auth_oidc.py index 5ca44f85..8adeba77 100644 --- a/api/auth_oidc.py +++ b/api/auth_oidc.py @@ -40,6 +40,14 @@ _jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {} _warned_allow_values: set[str] = set() +_ALLOW_VALUES_WHITESPACE_WARNING = ( + "HERMES_WEBUI_OIDC_ALLOW_VALUES has one or more entries with internal whitespace; " + "whitespace is not a value separator, so a value like " + '"alice@example.com bob@example.com" is treated as a single entry. ' + 'Use a comma-delimited scalar (e.g. "value1,value2") or a YAML array. ' + "If this is one intentional multi-word group, it is already correct and no action is needed." +) + class _NoRedirect(urllib.request.HTTPRedirectHandler): def redirect_request(self, *args, **kwargs): @@ -173,19 +181,12 @@ def _resolve_oidc_config() -> dict[str, Any]: if ( raw_allow is not None and not isinstance(raw_allow, (list, tuple, set)) - and "," not in str(raw_allow) - and "\n" not in str(raw_allow) - and any(ch.isspace() for ch in str(raw_allow).strip()) + and any(any(ch.isspace() for ch in v) for v in allow_values) ): key = str(raw_allow) if key not in _warned_allow_values: _warned_allow_values.add(key) - logger.warning( - "HERMES_WEBUI_OIDC_ALLOW_VALUES contains spaces but no commas or newlines; " - "whitespace is no longer a value separator. " - "Use a comma-delimited scalar (e.g. \"value1,value2\") or a YAML array. " - "If this value is one multi-word group name, it is already correct and needs no action." - ) + logger.warning(_ALLOW_VALUES_WHITESPACE_WARNING) return { "issuer": str(pick("issuer", "HERMES_WEBUI_OIDC_ISSUER") or "").strip(), "client_id": str(pick("client_id", "HERMES_WEBUI_OIDC_CLIENT_ID") or "").strip(), diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index d597f3f1..74bd91c1 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -3,12 +3,9 @@ Scope: _resolve_oidc_config warning detection and _enforce_allowlist decisions. No whitespace splitting under any heuristic; the warning is diagnostic only. """ -import importlib.util import logging -import os import sys from pathlib import Path -from typing import Any from unittest.mock import patch sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) @@ -18,8 +15,6 @@ import pytest import api.auth_oidc as auth_oidc from api.auth_oidc import _enforce_allowlist, _normalize_allow_values, OIDCAuthError -_BASE_WORKTREE = r"D:\Repos\.claude\pr-sweep\base-worktrees\webui-6645-4085-before-320789ae" - @pytest.fixture(autouse=True) def _reset_warned_cache(): @@ -28,22 +23,6 @@ def _reset_warned_cache(): auth_oidc._warned_allow_values.clear() -@pytest.fixture(scope="module") -def base_normalize_allow_values(): - """Load _normalize_allow_values from the base worktree for differential comparison.""" - base_path = os.path.join(_BASE_WORKTREE, "api", "auth_oidc.py") - spec = importlib.util.spec_from_file_location("auth_oidc_base", base_path) - mod = importlib.util.module_from_spec(spec) - # Inject dependencies that are already loaded; _normalize_allow_values is pure. - sys.path.insert(0, _BASE_WORKTREE) - try: - spec.loader.exec_module(mod) - finally: - if _BASE_WORKTREE in sys.path: - sys.path.remove(_BASE_WORKTREE) - return mod._normalize_allow_values - - def _resolve(monkeypatch, *, env=None, cfg_list=None): """Call _resolve_oidc_config with controlled allow_values input. @@ -63,7 +42,7 @@ def _resolve(monkeypatch, *, env=None, cfg_list=None): # --------------------------------------------------------------------------- -# reproduction — base-fails / head-passes +# reproduction # --------------------------------------------------------------------------- def test_legacy_scalar_warns(monkeypatch, caplog): @@ -71,19 +50,17 @@ def test_legacy_scalar_warns(monkeypatch, caplog): with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): cfg = _resolve(monkeypatch, env="alice@example.com bob@example.com") - # head: warning names the setting assert any( "HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records ), "expected warning naming HERMES_WEBUI_OIDC_ALLOW_VALUES" - # head: warning names both accepted forms warning_text = " ".join(r.message for r in caplog.records) assert "comma" in warning_text.lower(), "warning must mention comma-delimited form" assert "yaml array" in warning_text.lower() or "yaml" in warning_text.lower(), ( "warning must mention YAML array form" ) - # head: allowlist unchanged — one combined value, claim still denied + # allowlist unchanged — one combined value, claim still denied assert cfg["allow_values"] == ["alice@example.com bob@example.com"] with pytest.raises(OIDCAuthError): _enforce_allowlist( @@ -94,11 +71,11 @@ def test_legacy_scalar_warns(monkeypatch, caplog): # --------------------------------------------------------------------------- -# authorization unchanged — differential +# authorization unchanged # --------------------------------------------------------------------------- def test_enforce_decisions_unchanged(monkeypatch): - """_enforce_allowlist allow/deny decisions are identical to origin/master behavior.""" + """_enforce_allowlist allow/deny decisions are correct for every standard input shape.""" # no allow_claim → always passes _enforce_allowlist({"email": "anyone"}, allow_claim="", allow_values=[]) @@ -277,7 +254,7 @@ def test_canonical_no_warning(monkeypatch, caplog): # --------------------------------------------------------------------------- -# mode/state matrix — one case per Fault Scope matrix row +# mode/state matrix # --------------------------------------------------------------------------- def test_allow_values_matrix(monkeypatch, caplog): @@ -342,7 +319,7 @@ def test_warning_not_per_request(monkeypatch, caplog): # --------------------------------------------------------------------------- -# P1: Unicode whitespace — tab and NBSP separated scalars warn +# Unicode whitespace # --------------------------------------------------------------------------- def test_tab_separated_scalar_warns(monkeypatch, caplog): @@ -365,7 +342,7 @@ def test_tab_separated_scalar_warns(monkeypatch, caplog): def test_nbsp_separated_scalar_warns(monkeypatch, caplog): """Non-breaking-space-separated scalar emits a warning.""" - nbsp_val = "alice@example.com\u00a0bob@example.com" + nbsp_val = "alice@example.com bob@example.com" with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): cfg = _resolve(monkeypatch, env=nbsp_val) @@ -393,41 +370,54 @@ def test_leading_trailing_whitespace_only_no_warn(monkeypatch, caplog): # --------------------------------------------------------------------------- -# P2: unlisted-shape differential — head matches base for every input type +# mixed comma-plus-space scalar # --------------------------------------------------------------------------- -def test_normalize_allow_values_unlisted_shapes_match_base(base_normalize_allow_values): - """_normalize_allow_values output is identical to origin/master for unlisted shapes. +def test_mixed_comma_space_scalar_warns(monkeypatch, caplog): + """Mixed comma-plus-space scalar warns; the element with inner whitespace silently denies.""" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + cfg = _resolve(monkeypatch, env="alice@x.com bob@x.com, carol@x.com") - Shapes under test: None, bool, int, tuple, set, bare-CR scalar. - Uses the base worktree's implementation as the reference rather than asserting - constants, so the claim "for any input" is honest. - """ - base = base_normalize_allow_values - head = _normalize_allow_values + assert cfg["allow_values"] == ["alice@x.com bob@x.com", "carol@x.com"] + warning_text = " ".join(r.message for r in caplog.records) + assert "HERMES_WEBUI_OIDC_ALLOW_VALUES" in warning_text, ( + "mixed comma-plus-space scalar must emit a warning naming HERMES_WEBUI_OIDC_ALLOW_VALUES" + ) + # The message must describe what was detected, not say the value contains no commas. + assert "internal whitespace" in warning_text, ( + "warning must describe internal whitespace in entries, not claim the value has no commas" + ) + # carol authenticates; alice and bob are denied (their combined address has inner whitespace) + _enforce_allowlist( + {"email": "carol@x.com"}, + allow_claim="email", + allow_values=cfg["allow_values"], + ) + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"email": "alice@x.com"}, + allow_claim="email", + allow_values=cfg["allow_values"], + ) - shapes: list[Any] = [ - None, - True, - False, - 42, - 3.14, - ("alice@example.com", "bob@example.com"), - {"alice@example.com", "bob@example.com"}, - "alice\rbob", # bare CR, no newline — stays combined - "", - " ", + +# --------------------------------------------------------------------------- +# unlisted input shapes +# --------------------------------------------------------------------------- + +def test_normalize_allow_values_unlisted_shapes(): + """_normalize_allow_values returns the expected value for every non-standard input shape.""" + assert _normalize_allow_values(None) == [] + assert _normalize_allow_values(True) == ["True"] + assert _normalize_allow_values(False) == ["False"] + assert _normalize_allow_values(42) == ["42"] + assert _normalize_allow_values(3.14) == ["3.14"] + assert _normalize_allow_values(("alice@example.com", "bob@example.com")) == [ + "alice@example.com", "bob@example.com" ] - - for raw in shapes: - head_result = head(raw) - base_result = base(raw) - # Sets have undefined order; compare as sets of strings. - if isinstance(raw, set): - assert set(head_result) == set(base_result), ( - f"set input {raw!r}: head={head_result} base={base_result}" - ) - else: - assert head_result == base_result, ( - f"input {raw!r}: head={head_result!r} base={base_result!r}" - ) + assert set(_normalize_allow_values({"alice@example.com", "bob@example.com"})) == { + "alice@example.com", "bob@example.com" + } + assert _normalize_allow_values("alice\rbob") == ["alice\rbob"] # bare CR stays combined + assert _normalize_allow_values("") == [] + assert _normalize_allow_values(" ") == []