mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(dashboard-auth): harden the public native-authorize surface
Two tightenings on /auth/native/authorize (a public pre-auth route): - Per-IP pending cap (8): the broker store is capacity-bounded fail-closed at 256 entries with a 600s TTL, so one unauthenticated spammer could fill it and deny native sign-in gateway-wide for the pending window. Pending entries now record the requester IP and each address is capped well above any legitimate concurrent-login count; other addresses keep signing in. - Loopback redirect_uri accepts IP literals only (127.0.0.1 / ::1): 'localhost' can be re-pointed via the hosts file or a hostile resolver (RFC 8252 \u00a78.3 says to use loopback IP literals); the desktop always sends 127.0.0.1, so nothing legitimate used the name. 3 new tests: per-IP cap enforced, cap frees on TTL expiry, localhost redirect rejected at the route.
This commit is contained in:
parent
edebe45482
commit
4d23b2238e
3 changed files with 89 additions and 4 deletions
|
|
@ -84,6 +84,13 @@ _CODE_TTL_SECONDS = 120 # 2 minutes — generous for a slow local hop.
|
|||
# concurrent-login count for a single desktop user.
|
||||
_MAX_ENTRIES = 256
|
||||
|
||||
# Per-IP cap on concurrent PENDING authorizations. /auth/native/authorize is a
|
||||
# public (pre-auth) route, so without this a single unauthenticated spammer
|
||||
# could fill the global store (600s TTL each) and lock out legitimate native
|
||||
# logins for the pending window. A real desktop runs at most a couple of
|
||||
# concurrent sign-ins from one address; 8 is generous.
|
||||
_MAX_PENDING_PER_IP = 8
|
||||
|
||||
_lock = threading.Lock()
|
||||
|
||||
|
||||
|
|
@ -98,6 +105,7 @@ class _Pending:
|
|||
code_challenge: str # the DESKTOP's S256 challenge (cc_d), base64url no-pad
|
||||
redirect_uri: str # the desktop's loopback redirect (127.0.0.1:<port>/...)
|
||||
client_state: str # the desktop's own ``state`` (echoed back on redirect)
|
||||
client_ip: str # requester IP at authorize time (per-IP pending cap)
|
||||
expires_at: int
|
||||
|
||||
|
||||
|
|
@ -157,6 +165,7 @@ def register_pending(
|
|||
code_challenge: str,
|
||||
redirect_uri: str,
|
||||
client_state: str,
|
||||
client_ip: str = "",
|
||||
now: Optional[int] = None,
|
||||
) -> str:
|
||||
"""Stash a pending native authorization; return an opaque ``broker_state``.
|
||||
|
|
@ -165,12 +174,17 @@ def register_pending(
|
|||
S256 challenge (``cc_d``) — we never see the verifier until redemption.
|
||||
``redirect_uri`` is the desktop's loopback callback and ``client_state`` is
|
||||
the desktop's own CSRF ``state`` (echoed verbatim on the final redirect).
|
||||
``client_ip`` is the requester's address, used only for the per-IP pending
|
||||
cap below.
|
||||
|
||||
The returned ``broker_state`` is what the gateway threads through its OWN
|
||||
upstream PKCE round trip (inside the ``hermes_session_pkce`` cookie), so the
|
||||
callback can find this entry again via :func:`complete_pending`.
|
||||
|
||||
Raises ``NativeFlowError`` if the store is at capacity (fail closed).
|
||||
Raises ``NativeFlowError`` if the store is at capacity or the caller's IP
|
||||
already holds ``_MAX_PENDING_PER_IP`` live pending entries (fail closed —
|
||||
this is a public pre-auth route, so one spammer must not be able to fill
|
||||
the global store and deny sign-in to everyone else).
|
||||
"""
|
||||
now = int(time.time()) if now is None else now
|
||||
broker_state = secrets.token_urlsafe(32)
|
||||
|
|
@ -178,10 +192,18 @@ def register_pending(
|
|||
_gc_locked(now)
|
||||
if not _capacity_ok_locked():
|
||||
raise NativeFlowError("native-flow authorization store at capacity")
|
||||
if client_ip and (
|
||||
sum(1 for v in _pending.values() if v.client_ip == client_ip)
|
||||
>= _MAX_PENDING_PER_IP
|
||||
):
|
||||
raise NativeFlowError(
|
||||
"too many pending native authorizations from this address"
|
||||
)
|
||||
_pending[broker_state] = _Pending(
|
||||
code_challenge=code_challenge,
|
||||
redirect_uri=redirect_uri,
|
||||
client_state=client_state,
|
||||
client_ip=client_ip,
|
||||
expires_at=now + _PENDING_TTL_SECONDS,
|
||||
)
|
||||
return broker_state
|
||||
|
|
|
|||
|
|
@ -255,7 +255,10 @@ def _validate_loopback_redirect_uri(raw: str) -> str:
|
|||
|
||||
RFC 8252 §7.3 restricts native-app redirects to the loopback interface.
|
||||
We accept only ``http://127.0.0.1[:port]/...`` and ``http://[::1][:port]/...``
|
||||
(and the literal ``localhost`` host, which some OS browsers normalise).
|
||||
— literal loopback IPs. ``localhost`` is deliberately NOT accepted
|
||||
(RFC 8252 §8.3: the name can resolve to a non-loopback address via the
|
||||
hosts file or a hostile resolver, so clients "SHOULD use loopback IP
|
||||
literals"; the desktop always sends ``127.0.0.1``).
|
||||
A non-loopback host would let an attacker who can reach ``/auth/native/
|
||||
authorize`` (a public route) turn the gateway's authenticated callback
|
||||
into an open redirect that leaks a live authorization code to an
|
||||
|
|
@ -272,10 +275,13 @@ def _validate_loopback_redirect_uri(raw: str) -> str:
|
|||
detail="native redirect_uri must be http:// on the loopback interface",
|
||||
)
|
||||
host = (parsed.hostname or "").lower()
|
||||
if host not in ("127.0.0.1", "::1", "localhost"):
|
||||
if host not in ("127.0.0.1", "::1"):
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail="native redirect_uri host must be loopback (127.0.0.1 / ::1)",
|
||||
detail=(
|
||||
"native redirect_uri host must be a loopback IP literal "
|
||||
"(127.0.0.1 / ::1)"
|
||||
),
|
||||
)
|
||||
return raw
|
||||
|
||||
|
|
@ -338,6 +344,7 @@ async def auth_native_authorize(
|
|||
code_challenge=code_challenge,
|
||||
redirect_uri=redirect_uri,
|
||||
client_state=state,
|
||||
client_ip=_client_ip(request),
|
||||
)
|
||||
except native_flow.NativeFlowError as e:
|
||||
raise HTTPException(status_code=503, detail=str(e))
|
||||
|
|
|
|||
|
|
@ -190,6 +190,44 @@ def test_broker_capacity_fails_closed():
|
|||
)
|
||||
|
||||
|
||||
def test_broker_per_ip_pending_cap():
|
||||
"""One address cannot hog the pending store (public pre-auth route)."""
|
||||
_verifier, challenge = _make_pkce()
|
||||
for _ in range(native_flow._MAX_PENDING_PER_IP):
|
||||
native_flow.register_pending(
|
||||
code_challenge=challenge, redirect_uri="http://127.0.0.1:1/cb",
|
||||
client_state="s", client_ip="203.0.113.7",
|
||||
)
|
||||
# The capped IP is refused...
|
||||
with pytest.raises(native_flow.NativeFlowError):
|
||||
native_flow.register_pending(
|
||||
code_challenge=challenge, redirect_uri="http://127.0.0.1:1/cb",
|
||||
client_state="s", client_ip="203.0.113.7",
|
||||
)
|
||||
# ...while a different address still signs in fine.
|
||||
assert native_flow.register_pending(
|
||||
code_challenge=challenge, redirect_uri="http://127.0.0.1:1/cb",
|
||||
client_state="s", client_ip="198.51.100.9",
|
||||
)
|
||||
|
||||
|
||||
def test_broker_per_ip_cap_frees_on_expiry():
|
||||
"""Expired pending entries stop counting against the per-IP cap."""
|
||||
_verifier, challenge = _make_pkce()
|
||||
now = int(time.time())
|
||||
for _ in range(native_flow._MAX_PENDING_PER_IP):
|
||||
native_flow.register_pending(
|
||||
code_challenge=challenge, redirect_uri="http://127.0.0.1:1/cb",
|
||||
client_state="s", client_ip="203.0.113.7", now=now,
|
||||
)
|
||||
# Past the pending TTL the old entries are GC'd and the IP can retry.
|
||||
assert native_flow.register_pending(
|
||||
code_challenge=challenge, redirect_uri="http://127.0.0.1:1/cb",
|
||||
client_state="s", client_ip="203.0.113.7",
|
||||
now=now + native_flow._PENDING_TTL_SECONDS + 1,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Route-level E2E against StubAuthProvider
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -314,6 +352,24 @@ def test_native_authorize_rejects_non_loopback_redirect(gated_client):
|
|||
assert "loopback" in r.json()["detail"].lower()
|
||||
|
||||
|
||||
def test_native_authorize_rejects_localhost_name(gated_client):
|
||||
"""RFC 8252 §8.3 — loopback IP literals only; `localhost` can be
|
||||
re-pointed via the hosts file / a hostile resolver."""
|
||||
_verifier, challenge = _make_pkce()
|
||||
r = gated_client.get(
|
||||
"/auth/native/authorize",
|
||||
params={
|
||||
"provider": "stub",
|
||||
"code_challenge": challenge,
|
||||
"code_challenge_method": "S256",
|
||||
"redirect_uri": "http://localhost:53999/cb",
|
||||
"state": "s",
|
||||
},
|
||||
)
|
||||
assert r.status_code == 400
|
||||
assert "loopback" in r.json()["detail"].lower()
|
||||
|
||||
|
||||
def test_native_authorize_requires_s256(gated_client):
|
||||
_verifier, challenge = _make_pkce()
|
||||
r = gated_client.get(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue