mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-12 08:51:53 +00:00
fix(dashboard-auth): warn when public_url override is silently rejected (#43214)
A non-empty HERMES_DASHBOARD_PUBLIC_URL / dashboard.public_url value that fails URL validation (overwhelmingly: a missing http(s):// scheme, e.g. "hermes.domain.com") was silently discarded by resolve_public_url(), falling back to reconstructing the OAuth redirect_uri from request headers. Behind a reverse proxy that doesn't forward X-Forwarded-Proto reliably, that yields an http:// callback even though the operator explicitly set the public URL — with no signal as to why (#42780). Emit a deduplicated operator-facing WARNING (once per distinct value, since resolve_public_url runs per request) naming the offending value and the required scheme. Turns a silent footgun into a self-diagnosing one; behaviour is otherwise unchanged. Tests assert the warning fires for a scheme-less value, is deduplicated across repeated calls, and stays silent for a valid value — all three fail without the fix.
This commit is contained in:
parent
b96bd4808d
commit
7df3aa34b1
2 changed files with 130 additions and 2 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue