diff --git a/hermes_cli/dashboard_auth/prefix.py b/hermes_cli/dashboard_auth/prefix.py index 0c009502390..ae6d33214f5 100644 --- a/hermes_cli/dashboard_auth/prefix.py +++ b/hermes_cli/dashboard_auth/prefix.py @@ -31,6 +31,46 @@ _log = logging.getLogger(__name__) # rather than try to sanitise — the operator can fix their config. _REJECT_CHARS = frozenset(('"', "'", "<", ">", " ", "\n", "\r", "\t")) +# Remember which (source, value) pairs we've already warned about. +# ``resolve_public_url`` runs on every authenticated request, so an +# un-deduplicated warning would flood the logs once per request for a +# misconfigured deploy. Keyed on the raw value too, so changing the +# config and reloading surfaces a fresh warning. +_warned_malformed_public_urls: set = set() + + +def _warn_if_malformed(source: str, raw: str) -> None: + """Warn (once per distinct value) when a non-empty public-url value + was rejected by :func:`_normalise_public_url`. + + A non-empty value that normalises to ``""`` is almost always a + missing scheme (``hermes.example.com`` instead of + ``https://hermes.example.com``) — the single most common cause of + "I set HERMES_DASHBOARD_PUBLIC_URL but the OAuth callback is still + http://". Without this warning the value is silently discarded and + the dashboard falls back to reconstructing the redirect URI from + request headers, which behind a reverse proxy can yield the wrong + scheme. Surfacing it turns a silent footgun into a self-diagnosing + one. + """ + cleaned = raw.strip() if raw else "" + if not cleaned: + return # empty/unset is a legitimate "no override" — not malformed + key = (source, cleaned) + if key in _warned_malformed_public_urls: + return + _warned_malformed_public_urls.add(key) + _log.warning( + "%s is set to %r but was ignored because it is not a valid " + "absolute URL — it must include an http:// or https:// scheme " + "(e.g. https://%s). Falling back to reconstructing the OAuth " + "redirect URI from request headers, which may produce the wrong " + "scheme behind a reverse proxy.", + source, + cleaned, + cleaned.split("://")[-1] or "hermes.example.com", + ) + def normalise_prefix(raw: Optional[str]) -> str: """Normalise an X-Forwarded-Prefix header value. @@ -153,5 +193,9 @@ def resolve_public_url() -> str: env_clean = _normalise_public_url(env_raw) if env_clean: return env_clean - cfg_raw = _load_dashboard_section().get("public_url", "") - return _normalise_public_url(str(cfg_raw)) + _warn_if_malformed("HERMES_DASHBOARD_PUBLIC_URL env var", env_raw) + cfg_raw = str(_load_dashboard_section().get("public_url", "")) + cfg_clean = _normalise_public_url(cfg_raw) + if not cfg_clean: + _warn_if_malformed("dashboard.public_url in config.yaml", cfg_raw) + return cfg_clean diff --git a/tests/hermes_cli/test_dashboard_auth_prefix.py b/tests/hermes_cli/test_dashboard_auth_prefix.py index 74366c9c009..62f20be8e46 100644 --- a/tests/hermes_cli/test_dashboard_auth_prefix.py +++ b/tests/hermes_cli/test_dashboard_auth_prefix.py @@ -387,6 +387,90 @@ class TestPublicUrlOverride: redirect_uri = self._redirect_uri(gated_app_direct) assert redirect_uri == "https://from-config.example/auth/callback" + def test_scheme_less_public_url_env_warns_operator( + self, patch_config, monkeypatch, caplog + ): + """A non-empty env var that's missing its scheme (the #1 cause + of "I set HERMES_DASHBOARD_PUBLIC_URL but the callback is still + http://") must emit an operator-facing WARNING rather than being + silently discarded. Regression for #42780.""" + import logging + + from hermes_cli.dashboard_auth import prefix as prefix_mod + + # Reset the per-value dedup cache so the warning fires in-test + # regardless of test ordering. + prefix_mod._warned_malformed_public_urls.clear() + patch_config(None) + monkeypatch.setenv("HERMES_DASHBOARD_PUBLIC_URL", "hermes.domain.com") + + with caplog.at_level(logging.WARNING, logger=prefix_mod.__name__): + result = prefix_mod.resolve_public_url() + + assert result == "" # scheme-less value is still rejected + warnings = [ + r.getMessage() + for r in caplog.records + if r.levelno == logging.WARNING + ] + assert any( + "HERMES_DASHBOARD_PUBLIC_URL" in m + and "hermes.domain.com" in m + and "scheme" in m + for m in warnings + ), f"expected a scheme warning, got: {warnings!r}" + + def test_scheme_less_public_url_warning_is_deduplicated( + self, patch_config, monkeypatch, caplog + ): + """resolve_public_url runs per-request; the malformed-value + warning must fire at most once per distinct value so a + misconfigured deploy doesn't flood the logs.""" + import logging + + from hermes_cli.dashboard_auth import prefix as prefix_mod + + prefix_mod._warned_malformed_public_urls.clear() + patch_config(None) + monkeypatch.setenv("HERMES_DASHBOARD_PUBLIC_URL", "hermes.domain.com") + + with caplog.at_level(logging.WARNING, logger=prefix_mod.__name__): + for _ in range(5): + prefix_mod.resolve_public_url() + + scheme_warnings = [ + r + for r in caplog.records + if r.levelno == logging.WARNING + and "hermes.domain.com" in r.getMessage() + ] + assert len(scheme_warnings) == 1, ( + f"expected exactly one warning across 5 calls, " + f"got {len(scheme_warnings)}" + ) + + def test_valid_public_url_emits_no_warning( + self, patch_config, monkeypatch, caplog + ): + """A correctly-formed value must not produce a spurious warning.""" + import logging + + from hermes_cli.dashboard_auth import prefix as prefix_mod + + prefix_mod._warned_malformed_public_urls.clear() + patch_config(None) + monkeypatch.setenv( + "HERMES_DASHBOARD_PUBLIC_URL", "https://hermes.domain.com" + ) + + with caplog.at_level(logging.WARNING, logger=prefix_mod.__name__): + result = prefix_mod.resolve_public_url() + + assert result == "https://hermes.domain.com" + assert not [ + r for r in caplog.records if r.levelno == logging.WARNING + ] + # --------------------------------------------------------------------------- # Cookies: Path attribute + __Host- / __Secure- prefix rules