From ab11795aa431420c6bafbf8fde58179a75f1c249 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Fri, 31 Jul 2026 19:14:44 -0400 Subject: [PATCH 1/5] fix(auth): warn on ambiguous legacy OIDC allowlist scalars (#6603) --- api/auth_oidc.py | 23 ++- tests/test_auth_oidc.py | 302 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 322 insertions(+), 3 deletions(-) create mode 100644 tests/test_auth_oidc.py diff --git a/api/auth_oidc.py b/api/auth_oidc.py index 384bae2b..d0a0b9de 100644 --- a/api/auth_oidc.py +++ b/api/auth_oidc.py @@ -38,6 +38,8 @@ _discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} _jwks_lock = threading.Lock() _jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {} +_warned_allow_values: set[str] = set() + class _NoRedirect(urllib.request.HTTPRedirectHandler): def redirect_request(self, *args, **kwargs): @@ -166,9 +168,24 @@ def _resolve_oidc_config() -> dict[str, Any]: return env_value if env_value is not None else raw.get(name) scopes = _normalize_scopes(pick("scopes", "HERMES_WEBUI_OIDC_SCOPES")) - allow_values = _normalize_allow_values( - pick("allow_values", "HERMES_WEBUI_OIDC_ALLOW_VALUES") - ) + raw_allow = pick("allow_values", "HERMES_WEBUI_OIDC_ALLOW_VALUES") + allow_values = _normalize_allow_values(raw_allow) + 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 " " in str(raw_allow).strip() + ): + 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." + ) 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 new file mode 100644 index 00000000..8e16df94 --- /dev/null +++ b/tests/test_auth_oidc.py @@ -0,0 +1,302 @@ +"""Tests for #6603 — ambiguous legacy OIDC allowlist scalar warning. + +Scope: _resolve_oidc_config warning detection and _enforce_allowlist decisions. +No whitespace splitting under any heuristic; the warning is diagnostic only. +""" +import logging +import sys +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import pytest + +import api.auth_oidc as auth_oidc +from api.auth_oidc import _enforce_allowlist, _normalize_allow_values, OIDCAuthError + + +@pytest.fixture(autouse=True) +def _reset_warned_cache(): + auth_oidc._warned_allow_values.clear() + yield + auth_oidc._warned_allow_values.clear() + + +def _resolve(monkeypatch, *, env=None, cfg_list=None): + """Call _resolve_oidc_config with controlled allow_values input. + + Pass env= for a string env-var value, cfg_list= for a list from config. + Pass neither to simulate absent/None. + """ + monkeypatch.delenv("HERMES_WEBUI_OIDC_ALLOW_VALUES", raising=False) + if env is not None: + monkeypatch.setenv("HERMES_WEBUI_OIDC_ALLOW_VALUES", env) + webui_cfg = {} + elif cfg_list is not None: + webui_cfg = {"allow_values": cfg_list} + else: + webui_cfg = {} + with patch("api.auth_oidc.get_config", return_value={"webui_oidc": webui_cfg}): + return auth_oidc._resolve_oidc_config() + + +# --------------------------------------------------------------------------- +# reproduction — base-fails / head-passes +# --------------------------------------------------------------------------- + +def test_legacy_scalar_warns(monkeypatch, caplog): + """Exact scalar from #6603 issue emits a warning naming the setting and forms.""" + 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 + assert cfg["allow_values"] == ["alice@example.com bob@example.com"] + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"email": "alice@example.com"}, + allow_claim="email", + allow_values=cfg["allow_values"], + ) + + +# --------------------------------------------------------------------------- +# authorization unchanged — differential +# --------------------------------------------------------------------------- + +def test_enforce_decisions_unchanged(monkeypatch): + """_enforce_allowlist allow/deny decisions are identical to origin/master behavior.""" + # no allow_claim → always passes + _enforce_allowlist({"email": "anyone"}, allow_claim="", allow_values=[]) + + # matching value → allowed + _enforce_allowlist( + {"email": "alice@example.com"}, + allow_claim="email", + allow_values=["alice@example.com", "bob@example.com"], + ) + + # non-matching value → denied + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"email": "eve@example.com"}, + allow_claim="email", + allow_values=["alice@example.com", "bob@example.com"], + ) + + # absent claim → denied + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"sub": "1234"}, + allow_claim="email", + allow_values=["alice@example.com"], + ) + + # allow_claim set, allow_values empty → any non-empty claim value passes + _enforce_allowlist( + {"email": "anyone@example.com"}, + allow_claim="email", + allow_values=[], + ) + + # whitespace scalar resolves to one combined value; partial claim denied + combined_values = _normalize_allow_values("alice@example.com bob@example.com") + assert combined_values == ["alice@example.com bob@example.com"] + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"email": "alice@example.com"}, + allow_claim="email", + allow_values=combined_values, + ) + + +# --------------------------------------------------------------------------- +# multi-word group preserved — issue requirement 5a +# --------------------------------------------------------------------------- + +def test_multi_word_group(monkeypatch, caplog): + """'Hermes Users' stays one allow_values entry; a 'Hermes' fragment is denied.""" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + cfg = _resolve(monkeypatch, env="Hermes Users") + + # normalization preserves the full group name as one value + assert cfg["allow_values"] == ["Hermes Users"] + + # full group name → allowed + _enforce_allowlist( + {"groups": "Hermes Users"}, + allow_claim="groups", + allow_values=cfg["allow_values"], + ) + + # fragment → denied + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"groups": "Hermes"}, + allow_claim="groups", + allow_values=cfg["allow_values"], + ) + + # warning emitted (operator should confirm intent) + assert any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records) + + +# --------------------------------------------------------------------------- +# no fragment authorization — issue requirement 4 +# --------------------------------------------------------------------------- + +def test_no_fragment_authorized(monkeypatch): + """Team@Corp Admin@Corp as one value; Admin@Corp claim is denied.""" + combined = _normalize_allow_values("Team@Corp Admin@Corp") + assert combined == ["Team@Corp Admin@Corp"] + + # single-string claim + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"groups": "Admin@Corp"}, + allow_claim="groups", + allow_values=combined, + ) + + # array claim (simulates groups claim as list, matching _claim_values behavior) + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"groups": ["Admin@Corp"]}, + allow_claim="groups", + allow_values=combined, + ) + + # exact combined value is the only thing that passes + _enforce_allowlist( + {"groups": "Team@Corp Admin@Corp"}, + allow_claim="groups", + allow_values=combined, + ) + + +# --------------------------------------------------------------------------- +# canonical forms silent — negative space +# --------------------------------------------------------------------------- + +def test_canonical_no_warning(monkeypatch, caplog): + """Comma, newline, and list configurations emit no warning.""" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + # comma-delimited scalar + cfg = _resolve(monkeypatch, env="alice@example.com,bob@example.com") + assert cfg["allow_values"] == ["alice@example.com", "bob@example.com"] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "comma scalar must not emit a warning" + ) + + caplog.clear() + + # newline-delimited scalar + cfg = _resolve(monkeypatch, env="alice@example.com\nbob@example.com") + assert cfg["allow_values"] == ["alice@example.com", "bob@example.com"] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "newline scalar must not emit a warning" + ) + + caplog.clear() + + # YAML array (list from config) + cfg = _resolve(monkeypatch, cfg_list=["alice@example.com", "bob@example.com"]) + assert cfg["allow_values"] == ["alice@example.com", "bob@example.com"] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "list/array config must not emit a warning" + ) + + caplog.clear() + + # single token with no inner space + cfg = _resolve(monkeypatch, env="alice@example.com") + assert cfg["allow_values"] == ["alice@example.com"] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "single token without spaces must not emit a warning" + ) + + caplog.clear() + + # absent + cfg = _resolve(monkeypatch) + assert cfg["allow_values"] == [] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "absent value must not emit a warning" + ) + + +# --------------------------------------------------------------------------- +# mode/state matrix — one case per Fault Scope matrix row +# --------------------------------------------------------------------------- + +def test_allow_values_matrix(monkeypatch, caplog): + """Every configured shape resolves to its declared allowlist and warning state.""" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + # row 1: whitespace-only scalar, multiple intended values → one combined, warning + cfg = _resolve(monkeypatch, env="alice@example.com bob@example.com") + assert cfg["allow_values"] == ["alice@example.com bob@example.com"] + assert any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records) + caplog.clear() + auth_oidc._warned_allow_values.clear() + + # row 2: whitespace-only scalar, one intended multi-word group → one value, warning + cfg = _resolve(monkeypatch, env="Hermes Users") + assert cfg["allow_values"] == ["Hermes Users"] + assert any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records) + caplog.clear() + auth_oidc._warned_allow_values.clear() + + # row 3: comma scalar → split values, no warning + cfg = _resolve(monkeypatch, env="alice@example.com,bob@example.com") + assert cfg["allow_values"] == ["alice@example.com", "bob@example.com"] + assert not caplog.records + caplog.clear() + + # row 4: newline scalar → split values, no warning + cfg = _resolve(monkeypatch, env="alice@example.com\nbob@example.com") + assert cfg["allow_values"] == ["alice@example.com", "bob@example.com"] + assert not caplog.records + caplog.clear() + + # row 5: list/YAML array → each element, no warning + cfg = _resolve(monkeypatch, cfg_list=["alice@example.com", "bob@example.com"]) + assert cfg["allow_values"] == ["alice@example.com", "bob@example.com"] + assert not caplog.records + caplog.clear() + + # row 6: empty/absent → empty allowlist, no warning + cfg = _resolve(monkeypatch) + assert cfg["allow_values"] == [] + assert not caplog.records + + +# --------------------------------------------------------------------------- +# warning emitted once — not per request +# --------------------------------------------------------------------------- + +def test_warning_not_per_request(monkeypatch, caplog): + """Repeated config resolution with the same whitespace scalar emits the warning once.""" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + _resolve(monkeypatch, env="alice@example.com bob@example.com") + _resolve(monkeypatch, env="alice@example.com bob@example.com") + _resolve(monkeypatch, env="alice@example.com bob@example.com") + + matching = [ + r for r in caplog.records + if "HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message + ] + assert len(matching) == 1, ( + f"expected exactly 1 warning, got {len(matching)}" + ) From a4ea848f38d3b412bf925318b5841fb3382af35e Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Fri, 31 Jul 2026 19:51:57 -0400 Subject: [PATCH 2/5] fix(auth): detect all Unicode whitespace in ambiguous allowlist scalars (#6603) --- api/auth_oidc.py | 2 +- tests/test_auth_oidc.py | 135 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 3 deletions(-) diff --git a/api/auth_oidc.py b/api/auth_oidc.py index d0a0b9de..5ca44f85 100644 --- a/api/auth_oidc.py +++ b/api/auth_oidc.py @@ -175,7 +175,7 @@ def _resolve_oidc_config() -> dict[str, Any]: and not isinstance(raw_allow, (list, tuple, set)) and "," not in str(raw_allow) and "\n" not in str(raw_allow) - and " " in str(raw_allow).strip() + and any(ch.isspace() for ch in str(raw_allow).strip()) ): key = str(raw_allow) if key not in _warned_allow_values: diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index 8e16df94..d597f3f1 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -3,9 +3,12 @@ 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)) @@ -15,6 +18,8 @@ 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(): @@ -23,6 +28,22 @@ 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. @@ -220,11 +241,29 @@ def test_canonical_no_warning(monkeypatch, caplog): caplog.clear() - # single token with no inner space + # single token with no inner whitespace cfg = _resolve(monkeypatch, env="alice@example.com") assert cfg["allow_values"] == ["alice@example.com"] assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( - "single token without spaces must not emit a warning" + "single token without inner whitespace must not emit a warning" + ) + + caplog.clear() + + # single token with only leading/trailing whitespace (strip removes it, no inner ws) + cfg = _resolve(monkeypatch, env=" alice@example.com ") + assert cfg["allow_values"] == ["alice@example.com"] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "leading/trailing-only whitespace must not emit a warning" + ) + + caplog.clear() + + # leading/trailing tab only + cfg = _resolve(monkeypatch, env="\talice@example.com\t") + assert cfg["allow_values"] == ["alice@example.com"] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "leading/trailing-only tab must not emit a warning" ) caplog.clear() @@ -300,3 +339,95 @@ def test_warning_not_per_request(monkeypatch, caplog): assert len(matching) == 1, ( f"expected exactly 1 warning, got {len(matching)}" ) + + +# --------------------------------------------------------------------------- +# P1: Unicode whitespace — tab and NBSP separated scalars warn +# --------------------------------------------------------------------------- + +def test_tab_separated_scalar_warns(monkeypatch, caplog): + """Tab-separated scalar emits the same warning as a space-separated one.""" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + cfg = _resolve(monkeypatch, env="alice@example.com\tbob@example.com") + + assert cfg["allow_values"] == ["alice@example.com\tbob@example.com"] + assert any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "tab-separated scalar must emit a warning" + ) + # claim still denied + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"email": "alice@example.com"}, + allow_claim="email", + allow_values=cfg["allow_values"], + ) + + +def test_nbsp_separated_scalar_warns(monkeypatch, caplog): + """Non-breaking-space-separated scalar emits a warning.""" + nbsp_val = "alice@example.com\u00a0bob@example.com" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + cfg = _resolve(monkeypatch, env=nbsp_val) + + assert cfg["allow_values"] == [nbsp_val] + assert any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "NBSP-separated scalar must emit a warning" + ) + with pytest.raises(OIDCAuthError): + _enforce_allowlist( + {"email": "alice@example.com"}, + allow_claim="email", + allow_values=cfg["allow_values"], + ) + + +def test_leading_trailing_whitespace_only_no_warn(monkeypatch, caplog): + """Single token with only leading/trailing whitespace does not warn.""" + with caplog.at_level(logging.WARNING, logger="api.auth_oidc"): + cfg = _resolve(monkeypatch, env="\t alice@example.com \t") + + assert cfg["allow_values"] == ["alice@example.com"] + assert not any("HERMES_WEBUI_OIDC_ALLOW_VALUES" in r.message for r in caplog.records), ( + "leading/trailing-only whitespace must not emit a warning" + ) + + +# --------------------------------------------------------------------------- +# P2: unlisted-shape differential — head matches base for every input type +# --------------------------------------------------------------------------- + +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. + + 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 + + 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 + "", + " ", + ] + + 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}" + ) From 042804fd5ae6592b118f2473891a35c703dd2a98 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Fri, 31 Jul 2026 20:16:57 -0400 Subject: [PATCH 3/5] fix(auth): surface ambiguous OIDC allowlist scalars at startup (#6603) --- api/auth.py | 51 ++++++++++++------ api/auth_oidc.py | 19 +++---- tests/test_auth_oidc.py | 116 ++++++++++++++++++---------------------- 3 files changed, 97 insertions(+), 89 deletions(-) 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(" ") == [] From 28428d37530973ae4e9f477586b4723fb3241797 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Fri, 31 Jul 2026 20:27:32 -0400 Subject: [PATCH 4/5] fix(auth): name both config forms in the allowlist warning (#6603) --- api/auth_oidc.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/auth_oidc.py b/api/auth_oidc.py index 8adeba77..4168c338 100644 --- a/api/auth_oidc.py +++ b/api/auth_oidc.py @@ -41,8 +41,8 @@ _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 " + "webui_oidc.allow_values (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." From 737ba9044021298ebc22c2fc2abf857488663a00 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sat, 1 Aug 2026 05:53:07 -0400 Subject: [PATCH 5/5] fix(auth): align OIDC startup checks with runtime normalization (#6603) --- api/auth.py | 30 +++++++++++++------- tests/test_auth_oidc.py | 63 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/api/auth.py b/api/auth.py index fa10e6fb..fef5dc95 100644 --- a/api/auth.py +++ b/api/auth.py @@ -512,7 +512,18 @@ def get_oidc_startup_warning() -> str | None: issuer = bool(pick("issuer", "HERMES_WEBUI_OIDC_ISSUER")) client_id = bool(pick("client_id", "HERMES_WEBUI_OIDC_CLIENT_ID")) allow_claim = bool(pick("allow_claim", "HERMES_WEBUI_OIDC_ALLOW_CLAIM")) - allow_values = bool(pick("allow_values", "HERMES_WEBUI_OIDC_ALLOW_VALUES")) + 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") + normalized_allow_values = [] + allow_values_warning = None + try: + from api import auth_oidc + + normalized_allow_values = auth_oidc._normalize_allow_values(raw_allow) + allow_values_warning = auth_oidc._ALLOW_VALUES_WHITESPACE_WARNING + except Exception: + logger.debug("Failed to normalize OIDC allow_values", exc_info=True) + allow_values = bool(normalized_allow_values) if not any((issuer, client_id, allow_claim, allow_values)): return None @@ -538,16 +549,13 @@ def get_oidc_startup_warning() -> str | None: # 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) + if ( + allow_values_warning is not None + and raw_allow is not None + and not isinstance(raw_allow, (list, tuple, set)) + and any(any(ch.isspace() for ch in v) for v in normalized_allow_values) + ): + warnings.append(allow_values_warning) return "\n".join(warnings) if warnings else None diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index 74bd91c1..d36a0dd8 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -16,6 +16,17 @@ import api.auth_oidc as auth_oidc from api.auth_oidc import _enforce_allowlist, _normalize_allow_values, OIDCAuthError +@pytest.fixture(autouse=True) +def _clear_oidc_environment(monkeypatch): + for name in ( + "HERMES_WEBUI_OIDC_ISSUER", + "HERMES_WEBUI_OIDC_CLIENT_ID", + "HERMES_WEBUI_OIDC_ALLOW_CLAIM", + "HERMES_WEBUI_OIDC_ALLOW_VALUES", + ): + monkeypatch.delenv(name, raising=False) + + @pytest.fixture(autouse=True) def _reset_warned_cache(): auth_oidc._warned_allow_values.clear() @@ -23,6 +34,58 @@ def _reset_warned_cache(): auth_oidc._warned_allow_values.clear() +def _startup_warning(monkeypatch, allow_values): + import api.auth as auth + + monkeypatch.setattr( + auth, + "get_config", + lambda: { + "webui_oidc": { + "issuer": "https://issuer.example", + "client_id": "webui-client", + "allow_claim": "email", + "allow_values": allow_values, + } + }, + ) + return auth.get_oidc_startup_warning() + + +@pytest.mark.parametrize("allow_values", [[], [""]]) +def test_startup_warning_treats_empty_allowlist_shapes_as_missing(monkeypatch, allow_values): + warning = _startup_warning(monkeypatch, allow_values) + + assert warning is not None + assert "allow_values" in warning + assert auth_oidc._ALLOW_VALUES_WHITESPACE_WARNING not in warning + + +def test_startup_warning_accepts_multi_word_list(monkeypatch): + assert _startup_warning(monkeypatch, ["Hermes Users"]) is None + + +def test_startup_warning_accepts_comma_scalar(monkeypatch): + assert _startup_warning(monkeypatch, "alice@example.com,bob@example.com") is None + + +def test_startup_warning_reports_whitespace_scalar_migration(monkeypatch): + warning = _startup_warning(monkeypatch, "alice@example.com bob@example.com") + + assert warning is not None + assert "partially configured" not in warning + assert auth_oidc._ALLOW_VALUES_WHITESPACE_WARNING in warning + + +def test_startup_warning_uses_environment_allow_values_precedence(monkeypatch): + monkeypatch.setenv("HERMES_WEBUI_OIDC_ALLOW_VALUES", "") + + warning = _startup_warning(monkeypatch, ["alice@example.com"]) + + assert warning is not None + assert "allow_values" in warning + + def _resolve(monkeypatch, *, env=None, cfg_list=None): """Call _resolve_oidc_config with controlled allow_values input.