mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-14 04:02:26 +00:00
fix(webhook): widen INSECURE_NO_AUTH loopback check + tests + docs
Follow-up to the previous commit: - Add _is_loopback_host() helper covering 127.0.0.1, localhost, ::1, ip6-localhost, ip6-loopback (case-insensitive). Empty/None host is treated as non-loopback since unset usually means public default bind. - Fix mixed-indent comment in the safety rail (comment now aligned with the if-block) and collapse the nested-if into one condition. - Add TestInsecureNoAuthSafetyRail covering rejection on 0.0.0.0, a LAN IP, and empty host; allowance on 127.0.0.1/localhost; plus unit-level parametrized coverage of _is_loopback_host for spellings we can't bind in the hermetic test env (::1, ip6-localhost, ip6-loopback). - Pin test_connect_starts_server + test_webhook_deliver_only defaults to 127.0.0.1 so they keep passing under the new rail. - Document the behavior in website/docs/user-guide/messaging/webhooks.md.
This commit is contained in:
parent
fb4f953569
commit
898b6d7d55
4 changed files with 116 additions and 11 deletions
|
|
@ -59,6 +59,29 @@ DEFAULT_PORT = 8644
|
||||||
_INSECURE_NO_AUTH = "INSECURE_NO_AUTH"
|
_INSECURE_NO_AUTH = "INSECURE_NO_AUTH"
|
||||||
_DYNAMIC_ROUTES_FILENAME = "webhook_subscriptions.json"
|
_DYNAMIC_ROUTES_FILENAME = "webhook_subscriptions.json"
|
||||||
|
|
||||||
|
# Hostnames/IP literals that only serve connections originating on the same
|
||||||
|
# machine. Anything else is treated as a public bind for safety-rail purposes.
|
||||||
|
_LOOPBACK_HOSTS = frozenset({
|
||||||
|
"127.0.0.1",
|
||||||
|
"localhost",
|
||||||
|
"::1",
|
||||||
|
"ip6-localhost",
|
||||||
|
"ip6-loopback",
|
||||||
|
})
|
||||||
|
|
||||||
|
|
||||||
|
def _is_loopback_host(host: str) -> bool:
|
||||||
|
"""True when `host` binds only to the local machine.
|
||||||
|
|
||||||
|
Covers IPv4 loopback, the standard `localhost` alias, IPv6 loopback in
|
||||||
|
both bracketed and bare form, and the common Debian-style aliases. Any
|
||||||
|
falsy value (empty string, None) is conservatively treated as non-loopback
|
||||||
|
because an unset host usually means the platform-default public bind.
|
||||||
|
"""
|
||||||
|
if not host:
|
||||||
|
return False
|
||||||
|
return host.strip().lower() in _LOOPBACK_HOSTS
|
||||||
|
|
||||||
|
|
||||||
def check_webhook_requirements() -> bool:
|
def check_webhook_requirements() -> bool:
|
||||||
"""Check if webhook adapter dependencies are available."""
|
"""Check if webhook adapter dependencies are available."""
|
||||||
|
|
@ -125,15 +148,18 @@ class WebhookAdapter(BasePlatformAdapter):
|
||||||
f"Set 'secret' on the route or globally. "
|
f"Set 'secret' on the route or globally. "
|
||||||
f"For testing without auth, set secret to '{_INSECURE_NO_AUTH}'."
|
f"For testing without auth, set secret to '{_INSECURE_NO_AUTH}'."
|
||||||
)
|
)
|
||||||
# Safety rail: Prevent INSECURE_NO_AUTH on non-localhost bindings
|
|
||||||
if secret == _INSECURE_NO_AUTH:
|
# Safety rail: refuse to start if INSECURE_NO_AUTH is combined with a
|
||||||
if self._host not in ("127.0.0.1", "localhost"):
|
# non-loopback bind. The escape hatch is for local testing only;
|
||||||
raise ValueError(
|
# serving an unauthenticated route on a public interface is a
|
||||||
f"[webhook] Route '{name}' uses INSECURE_NO_AUTH secret "
|
# deployment-grade footgun we'd rather crash early than ship.
|
||||||
f"but is bound to non-localhost host '{self._host}'. "
|
if secret == _INSECURE_NO_AUTH and not _is_loopback_host(self._host):
|
||||||
f"INSECURE_NO_AUTH is for local testing only. "
|
raise ValueError(
|
||||||
f"Refusing to start to prevent accidental exposure."
|
f"[webhook] Route '{name}' uses INSECURE_NO_AUTH secret "
|
||||||
)
|
f"but is bound to non-loopback host '{self._host}'. "
|
||||||
|
f"INSECURE_NO_AUTH is for local testing only. "
|
||||||
|
f"Refusing to start to prevent accidental exposure."
|
||||||
|
)
|
||||||
# deliver_only routes bypass the agent — the POST body becomes a
|
# deliver_only routes bypass the agent — the POST body becomes a
|
||||||
# direct push notification via the configured delivery target.
|
# direct push notification via the configured delivery target.
|
||||||
# Validate up-front so misconfiguration surfaces at startup rather
|
# Validate up-front so misconfiguration surfaces at startup rather
|
||||||
|
|
|
||||||
|
|
@ -352,7 +352,7 @@ class TestHTTPHandling:
|
||||||
async def test_connect_starts_server(self):
|
async def test_connect_starts_server(self):
|
||||||
"""connect() starts the HTTP listener and marks adapter as connected."""
|
"""connect() starts the HTTP listener and marks adapter as connected."""
|
||||||
routes = {"r1": {"secret": _INSECURE_NO_AUTH, "prompt": "x"}}
|
routes = {"r1": {"secret": _INSECURE_NO_AUTH, "prompt": "x"}}
|
||||||
adapter = _make_adapter(routes=routes, port=0)
|
adapter = _make_adapter(routes=routes, host="127.0.0.1", port=0)
|
||||||
# Use port 0 — the OS picks a free port, but aiohttp requires a real bind.
|
# Use port 0 — the OS picks a free port, but aiohttp requires a real bind.
|
||||||
# We just test that the method completes and marks connected.
|
# We just test that the method completes and marks connected.
|
||||||
# Need to mock TCPSite to avoid actual binding.
|
# Need to mock TCPSite to avoid actual binding.
|
||||||
|
|
@ -758,3 +758,80 @@ class TestDeliverCrossPlatformThreadId:
|
||||||
mock_target.send.assert_awaited_once_with(
|
mock_target.send.assert_awaited_once_with(
|
||||||
"12345", "hello", metadata=None
|
"12345", "hello", metadata=None
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestInsecureNoAuthSafetyRail:
|
||||||
|
"""connect() refuses to start when INSECURE_NO_AUTH is combined with a
|
||||||
|
non-loopback bind. Guards against accidentally exposing an unauthenticated
|
||||||
|
webhook endpoint on a public interface."""
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_connect_rejects_insecure_no_auth_on_public_bind(self):
|
||||||
|
"""INSECURE_NO_AUTH + 0.0.0.0 is refused before the server starts."""
|
||||||
|
routes = {"r1": {"secret": _INSECURE_NO_AUTH, "prompt": "x"}}
|
||||||
|
adapter = _make_adapter(routes=routes, host="0.0.0.0", port=0)
|
||||||
|
with pytest.raises(ValueError, match="INSECURE_NO_AUTH"):
|
||||||
|
await adapter.connect()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_connect_rejects_insecure_no_auth_on_lan_ip(self):
|
||||||
|
"""A LAN IP is treated as public."""
|
||||||
|
routes = {"r1": {"secret": _INSECURE_NO_AUTH, "prompt": "x"}}
|
||||||
|
adapter = _make_adapter(routes=routes, host="192.168.1.50", port=0)
|
||||||
|
with pytest.raises(ValueError, match="non-loopback"):
|
||||||
|
await adapter.connect()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_connect_rejects_insecure_no_auth_on_empty_host(self):
|
||||||
|
"""Empty host is conservatively treated as non-loopback."""
|
||||||
|
routes = {"r1": {"secret": _INSECURE_NO_AUTH, "prompt": "x"}}
|
||||||
|
adapter = _make_adapter(routes=routes, host="", port=0)
|
||||||
|
with pytest.raises(ValueError, match="INSECURE_NO_AUTH"):
|
||||||
|
await adapter.connect()
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"host",
|
||||||
|
["127.0.0.1", "localhost"],
|
||||||
|
)
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_connect_allows_insecure_no_auth_on_loopback(self, host):
|
||||||
|
"""Recognised loopback hosts are permitted with INSECURE_NO_AUTH."""
|
||||||
|
routes = {"r1": {"secret": _INSECURE_NO_AUTH, "prompt": "x"}}
|
||||||
|
adapter = _make_adapter(routes=routes, host=host, port=0)
|
||||||
|
try:
|
||||||
|
with patch.object(adapter, "_reload_dynamic_routes"):
|
||||||
|
result = await adapter.connect()
|
||||||
|
assert result is True
|
||||||
|
finally:
|
||||||
|
await adapter.disconnect()
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"host",
|
||||||
|
["127.0.0.1", "localhost", "Localhost", "::1", "ip6-localhost", "ip6-loopback"],
|
||||||
|
)
|
||||||
|
def test_is_loopback_host_accepts(self, host):
|
||||||
|
"""_is_loopback_host covers all documented loopback spellings."""
|
||||||
|
from gateway.platforms.webhook import _is_loopback_host
|
||||||
|
assert _is_loopback_host(host) is True
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"host",
|
||||||
|
["0.0.0.0", "192.168.1.5", "10.0.0.1", "example.com", "", None],
|
||||||
|
)
|
||||||
|
def test_is_loopback_host_rejects(self, host):
|
||||||
|
"""_is_loopback_host treats public/LAN/empty as non-loopback."""
|
||||||
|
from gateway.platforms.webhook import _is_loopback_host
|
||||||
|
assert _is_loopback_host(host) is False
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_connect_allows_real_secret_on_public_bind(self):
|
||||||
|
"""A real HMAC secret bound to 0.0.0.0 is the normal production case."""
|
||||||
|
routes = {"r1": {"secret": "real-secret-abc123", "prompt": "x"}}
|
||||||
|
adapter = _make_adapter(routes=routes, host="0.0.0.0", port=0)
|
||||||
|
try:
|
||||||
|
with patch.object(adapter, "_reload_dynamic_routes"):
|
||||||
|
result = await adapter.connect()
|
||||||
|
assert result is True
|
||||||
|
finally:
|
||||||
|
await adapter.disconnect()
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -33,7 +33,7 @@ from gateway.platforms.webhook import WebhookAdapter, _INSECURE_NO_AUTH
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
def _make_adapter(routes, **extra_kw) -> WebhookAdapter:
|
def _make_adapter(routes, **extra_kw) -> WebhookAdapter:
|
||||||
extra = {"host": "0.0.0.0", "port": 0, "routes": routes}
|
extra = {"host": "127.0.0.1", "port": 0, "routes": routes}
|
||||||
extra.update(extra_kw)
|
extra.update(extra_kw)
|
||||||
config = PlatformConfig(enabled=True, extra=extra)
|
config = PlatformConfig(enabled=True, extra=extra)
|
||||||
return WebhookAdapter(config)
|
return WebhookAdapter(config)
|
||||||
|
|
|
||||||
|
|
@ -395,6 +395,8 @@ If a secret is configured but no recognized signature header is present, the req
|
||||||
|
|
||||||
Every route must have a secret — either set directly on the route or inherited from the global `secret`. Routes without a secret cause the adapter to fail at startup with an error. For development/testing only, you can set the secret to `"INSECURE_NO_AUTH"` to skip validation entirely.
|
Every route must have a secret — either set directly on the route or inherited from the global `secret`. Routes without a secret cause the adapter to fail at startup with an error. For development/testing only, you can set the secret to `"INSECURE_NO_AUTH"` to skip validation entirely.
|
||||||
|
|
||||||
|
`INSECURE_NO_AUTH` is only accepted when the gateway is bound to a loopback host (`127.0.0.1`, `localhost`, `::1`). If it is combined with a non-loopback bind such as `0.0.0.0` or a LAN IP, the adapter refuses to start — this prevents accidentally exposing an unauthenticated endpoint on a public interface.
|
||||||
|
|
||||||
### Rate limiting
|
### Rate limiting
|
||||||
|
|
||||||
Each route is rate-limited to **30 requests per minute** by default (fixed-window). Configure this globally:
|
Each route is rate-limited to **30 requests per minute** by default (fixed-window). Configure this globally:
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue