Two Codex adversarial-security-gate findings on the batch: 1. #7331 CORE (backward-compat break): making '|' an unconditional trusted-groups separator would re-split an existing deployment whose group NAME legitimately contains a literal '|' (e.g. 'unprivileged|admins' read as one group), silently changing its profile binding. Pipe splitting is now OPT-IN via HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR; comma/newline stay the default. Tests: literal-pipe preserved by default, pipe splits only with the flag set. 2. #6772 follow-up (my own hardening introduced it): rebuilding the origin from parsed parts dropped the brackets around an IPv6 host, so 'http://[::1]:8787' became 'http://::1:8787' and never matched the browser's bracketed clientDataJSON.origin -> passkey login over IPv6 would fail. Re-bracket IPv6 literals. Added rp_context regression tests (Origin-first, port preserved, IPv6 bracketed, malformed->Host fallback, userinfo stripped).
This commit is contained in:
1 parent
2e62c4f896
commit
e7841a5988
4 files changed
+136
-17
No files matched your search
+19
-9
@@ -105,6 +105,12 @@ _TRUSTED_AUTH_HEADER_ENV = 'HERMES_WEBUI_TRUSTED_AUTH_HEADER'
|
||||
_TRUSTED_GROUPS_HEADER_ENV = 'HERMES_WEBUI_TRUSTED_GROUPS_HEADER'
|
||||
_TRUSTED_GROUP_PROFILE_MAP_ENV = 'HERMES_WEBUI_GROUP_PROFILE_MAP'
|
||||
_TRUSTED_AUTH_LOGOUT_URL_ENV = 'HERMES_WEBUI_TRUSTED_AUTH_LOGOUT_URL'
|
||||
# Opt-in: also treat '|' as a group separator in the trusted-groups header.
|
||||
# Off by default so an existing deployment whose group NAME legitimately
|
||||
# contains a literal '|' is never silently re-split into two groups (which
|
||||
# could change its profile binding). Set to 1/true/yes/on for identity
|
||||
# providers (some Authentik outpost configs) that emit "admins|developpeur".
|
||||
_TRUSTED_GROUPS_PIPE_SEPARATOR_ENV = 'HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR'
|
||||
_TRUSTED_AUTH_WARNINGS_EMITTED: set[str] = set()
|
||||
|
||||
|
||||
@@ -722,15 +728,19 @@ def _trusted_groups_header_value(handler) -> list[str]:
|
||||
if not raw:
|
||||
return []
|
||||
values = []
|
||||
# Authentik's outpost typically joins multiple group names with a comma,
|
||||
# but this deployment's proxy provider/property mapping emits a
|
||||
# pipe-separated list instead (e.g. "admins|developpeur"). A bare comma
|
||||
# split silently treats a multi-group pipe-joined value as one unmatched
|
||||
# group name, dropping the session to the unbound "default" profile even
|
||||
# though the identity is a legitimate member of a mapped group. Accept
|
||||
# comma, pipe, and newline as equivalent separators so either format
|
||||
# works regardless of which the outpost happens to send.
|
||||
normalized = str(raw).replace('\n', ',').replace('|', ',')
|
||||
# Authentik's outpost typically joins multiple group names with a comma or
|
||||
# newline; parse those as separators by default. Some proxy provider /
|
||||
# property-mapping configs instead emit a pipe-separated list (e.g.
|
||||
# "admins|developpeur"), which a comma-only split would treat as one
|
||||
# unmatched group name — silently dropping the session to the unbound
|
||||
# "default" profile despite a legitimate mapped membership. Pipe splitting
|
||||
# is therefore available but OPT-IN (HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR),
|
||||
# because a group NAME can legitimately contain a literal '|' and must not be
|
||||
# re-split by default — doing so unconditionally could change an existing
|
||||
# deployment's profile binding.
|
||||
normalized = str(raw).replace('\n', ',')
|
||||
if str(os.getenv(_TRUSTED_GROUPS_PIPE_SEPARATOR_ENV, '')).strip().lower() in ('1', 'true', 'yes', 'on'):
|
||||
normalized = normalized.replace('|', ',')
|
||||
for part in normalized.split(','):
|
||||
part = part.strip()
|
||||
if part:
|
||||
|
||||
+5
-1
@@ -208,7 +208,11 @@ def rp_context(handler) -> tuple[str, str]:
|
||||
from urllib.parse import urlparse
|
||||
parsed = urlparse(browser_origin.strip())
|
||||
if parsed.scheme in ("http", "https") and parsed.hostname:
|
||||
netloc = parsed.hostname if parsed.port is None else f"{parsed.hostname}:{parsed.port}"
|
||||
# Re-bracket IPv6 literals: parsed.hostname strips the [] that the
|
||||
# browser's clientDataJSON.origin carries, so an unbracketed
|
||||
# "http://::1:8787" would never match the stored origin.
|
||||
host_part = f"[{parsed.hostname}]" if ":" in parsed.hostname else parsed.hostname
|
||||
netloc = host_part if parsed.port is None else f"{host_part}:{parsed.port}"
|
||||
return parsed.hostname, f"{parsed.scheme}://{netloc}"
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
@@ -395,3 +395,73 @@ def test_passwordless_settings_and_last_passkey_guard_are_wired():
|
||||
assert "id=\"btnGoPasswordless\"" in index
|
||||
assert "async function goPasswordless" in panels
|
||||
assert "prompt(" not in panels
|
||||
|
||||
|
||||
# ── rp_context: RPID/origin derivation (Origin-first, IPv6-safe) — #6772 ──────
|
||||
|
||||
class _HeaderHandler:
|
||||
"""Minimal stand-in for the HTTP handler rp_context() reads headers from."""
|
||||
|
||||
def __init__(self, headers):
|
||||
# case-insensitive get, like http.client.HTTPMessage
|
||||
self._h = {k.lower(): v for k, v in headers.items()}
|
||||
|
||||
class _H:
|
||||
def __init__(self, h):
|
||||
self._h = h
|
||||
|
||||
def get(self, name, default=""):
|
||||
return self._h.get(name.lower(), default)
|
||||
|
||||
@property
|
||||
def headers(self):
|
||||
return _HeaderHandler._H(self._h)
|
||||
|
||||
|
||||
def test_rp_context_prefers_wellformed_origin_over_host():
|
||||
from api import passkeys
|
||||
|
||||
h = _HeaderHandler({"Origin": "https://app.example.com", "Host": "backend.internal:9000"})
|
||||
rp_id, origin = passkeys.rp_context(h)
|
||||
assert rp_id == "app.example.com"
|
||||
assert origin == "https://app.example.com"
|
||||
|
||||
|
||||
def test_rp_context_preserves_origin_port():
|
||||
from api import passkeys
|
||||
|
||||
h = _HeaderHandler({"Origin": "http://localhost:8787", "Host": "localhost:8787"})
|
||||
rp_id, origin = passkeys.rp_context(h)
|
||||
assert rp_id == "localhost"
|
||||
assert origin == "http://localhost:8787"
|
||||
|
||||
|
||||
def test_rp_context_ipv6_origin_is_bracketed_to_match_clientdata():
|
||||
"""A browser's clientDataJSON.origin brackets an IPv6 host ('http://[::1]:8787').
|
||||
rp_context must store the same bracketed form, or _client_data()'s origin
|
||||
comparison (and thus passkey login over IPv6) would always fail."""
|
||||
from api import passkeys
|
||||
|
||||
h = _HeaderHandler({"Origin": "http://[::1]:8787", "Host": "[::1]:8787"})
|
||||
rp_id, origin = passkeys.rp_context(h)
|
||||
assert rp_id == "::1"
|
||||
assert origin == "http://[::1]:8787"
|
||||
|
||||
|
||||
def test_rp_context_falls_back_to_host_on_malformed_origin():
|
||||
from api import passkeys
|
||||
|
||||
for bad in ("javascript:alert(1)", "file:///etc/passwd", "not-a-url", ""):
|
||||
h = _HeaderHandler({"Origin": bad, "Host": "real.example.com"})
|
||||
rp_id, _origin = passkeys.rp_context(h)
|
||||
assert rp_id == "real.example.com", f"expected Host fallback for Origin={bad!r}"
|
||||
|
||||
|
||||
def test_rp_context_strips_origin_userinfo():
|
||||
"""A userinfo-bearing Origin must not be stored verbatim; the hostname wins."""
|
||||
from api import passkeys
|
||||
|
||||
h = _HeaderHandler({"Origin": "https://user:pw@evil.example.com", "Host": "real.example.com"})
|
||||
rp_id, origin = passkeys.rp_context(h)
|
||||
assert rp_id == "evil.example.com"
|
||||
assert "@" not in origin and origin == "https://evil.example.com"
|
||||
@@ -219,18 +219,12 @@ def test_group_map_prefers_mapping_order_over_header_order(monkeypatch):
|
||||
("admins", ["admins"]),
|
||||
# Comma-separated (the format this parser originally supported).
|
||||
("admins,developpeur", ["admins", "developpeur"]),
|
||||
# Pipe-separated (this deployment's Authentik outpost format).
|
||||
("admins|developpeur", ["admins", "developpeur"]),
|
||||
# Newline-separated (already supported before this fix).
|
||||
("admins\ndeveloppeur", ["admins", "developpeur"]),
|
||||
# Mixed separators in the same value.
|
||||
("admins,developpeur|it\nops", ["admins", "developpeur", "it", "ops"]),
|
||||
# Repeated/adjacent separators collapse instead of producing
|
||||
# Repeated/adjacent comma separators collapse instead of producing
|
||||
# empty group names.
|
||||
("admins||developpeur", ["admins", "developpeur"]),
|
||||
("admins,,developpeur", ["admins", "developpeur"]),
|
||||
# Surrounding and interior whitespace is trimmed per group.
|
||||
(" admins | developpeur ", ["admins", "developpeur"]),
|
||||
("admins, developpeur", ["admins", "developpeur"]),
|
||||
],
|
||||
)
|
||||
@@ -238,6 +232,45 @@ def test_trusted_groups_header_value_accepts_separator_variants(
|
||||
monkeypatch, raw_header, expected
|
||||
):
|
||||
_trusted_env(monkeypatch, groups_header="Remote-Groups")
|
||||
monkeypatch.delenv("HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR", raising=False)
|
||||
handler = _Handler(headers={"Remote-User": "alice", "Remote-Groups": raw_header})
|
||||
|
||||
assert auth._trusted_groups_header_value(handler) == expected
|
||||
|
||||
|
||||
def test_trusted_groups_pipe_is_literal_by_default(monkeypatch):
|
||||
"""Without the opt-in, a '|' is part of the group NAME, not a separator.
|
||||
|
||||
This is the backward-compatibility guarantee: an existing deployment whose
|
||||
group name legitimately contains a '|' must not be silently re-split into
|
||||
two groups (which could change its profile binding)."""
|
||||
_trusted_env(monkeypatch, groups_header="Remote-Groups")
|
||||
monkeypatch.delenv("HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR", raising=False)
|
||||
handler = _Handler(
|
||||
headers={"Remote-User": "alice", "Remote-Groups": "admins|developpeur"}
|
||||
)
|
||||
|
||||
assert auth._trusted_groups_header_value(handler) == ["admins|developpeur"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"raw_header, expected",
|
||||
[
|
||||
# Pipe-separated (this deployment's Authentik outpost format).
|
||||
("admins|developpeur", ["admins", "developpeur"]),
|
||||
# Mixed separators in the same value.
|
||||
("admins,developpeur|it\nops", ["admins", "developpeur", "it", "ops"]),
|
||||
# Repeated/adjacent pipe separators collapse instead of producing
|
||||
# empty group names.
|
||||
("admins||developpeur", ["admins", "developpeur"]),
|
||||
# Surrounding and interior whitespace is trimmed per group.
|
||||
(" admins | developpeur ", ["admins", "developpeur"]),
|
||||
],
|
||||
)
|
||||
def test_trusted_groups_pipe_separator_opt_in(monkeypatch, raw_header, expected):
|
||||
"""With HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR set, '|' also splits."""
|
||||
_trusted_env(monkeypatch, groups_header="Remote-Groups")
|
||||
monkeypatch.setenv("HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR", "1")
|
||||
handler = _Handler(headers={"Remote-User": "alice", "Remote-Groups": raw_header})
|
||||
|
||||
assert auth._trusted_groups_header_value(handler) == expected
|
||||
@@ -250,6 +283,7 @@ def test_group_map_accepts_pipe_separated_header_value(monkeypatch):
|
||||
# of a mapped group: a lone-membership identity works fine (no
|
||||
# separator to parse), but a second group membership starts producing a
|
||||
# pipe-joined header value that a comma-only split can't see through.
|
||||
# Pipe splitting is opt-in, so this exercises the env flag explicitly.
|
||||
#
|
||||
# NOTE: this only covers the concrete-mapped-group case. The
|
||||
# wildcard-mapped-group case (a "*" mapping value dominating a concrete
|
||||
@@ -261,6 +295,7 @@ def test_group_map_accepts_pipe_separated_header_value(monkeypatch):
|
||||
groups_header="Remote-Groups",
|
||||
group_map={"hermes_devops": "devops"},
|
||||
)
|
||||
monkeypatch.setenv("HERMES_WEBUI_TRUSTED_GROUPS_PIPE_SEPARATOR", "1")
|
||||
handler = _Handler(
|
||||
headers={"Remote-User": "alice", "Remote-Groups": "hermes_devops|other_group"}
|
||||
)
|
||||
|
||||
Reference in new issue
Block a user