mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
fix(webhook): validate HMAC signature before rate limiting (#12544)
This commit is contained in:
Tranquil-Flow
Teknium
parent
35e7bf6b00
commit
6a228d52f7
2 changed files with 301 additions and 12 deletions
|
|
@ -313,24 +313,14 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
{"error": "Payload too large"}, status=413
|
||||
)
|
||||
|
||||
# ── Rate limiting ────────────────────────────────────────
|
||||
now = time.time()
|
||||
window = self._rate_counts.setdefault(route_name, [])
|
||||
window[:] = [t for t in window if now - t < 60]
|
||||
if len(window) >= self._rate_limit:
|
||||
return web.json_response(
|
||||
{"error": "Rate limit exceeded"}, status=429
|
||||
)
|
||||
window.append(now)
|
||||
|
||||
# Read body
|
||||
# Read body (must be done before any validation)
|
||||
try:
|
||||
raw_body = await request.read()
|
||||
except Exception as e:
|
||||
logger.error("[webhook] Failed to read body: %s", e)
|
||||
return web.json_response({"error": "Bad request"}, status=400)
|
||||
|
||||
# Validate HMAC signature (skip for INSECURE_NO_AUTH testing mode)
|
||||
# Validate HMAC signature FIRST (skip for INSECURE_NO_AUTH testing mode)
|
||||
secret = route_config.get("secret", self._global_secret)
|
||||
if secret and secret != _INSECURE_NO_AUTH:
|
||||
if not self._validate_signature(request, raw_body, secret):
|
||||
|
|
@ -341,6 +331,16 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
{"error": "Invalid signature"}, status=401
|
||||
)
|
||||
|
||||
# ── Rate limiting (after auth) ───────────────────────────
|
||||
now = time.time()
|
||||
window = self._rate_counts.setdefault(route_name, [])
|
||||
window[:] = [t for t in window if now - t < 60]
|
||||
if len(window) >= self._rate_limit:
|
||||
return web.json_response(
|
||||
{"error": "Rate limit exceeded"}, status=429
|
||||
)
|
||||
window.append(now)
|
||||
|
||||
# Parse payload
|
||||
try:
|
||||
payload = json.loads(raw_body)
|
||||
|
|
|
|||
289
tests/gateway/test_webhook_signature_rate_limit.py
Normal file
289
tests/gateway/test_webhook_signature_rate_limit.py
Normal file
|
|
@ -0,0 +1,289 @@
|
|||
"""Test that HMAC signature validation happens BEFORE rate limiting.
|
||||
|
||||
This verifies the fix for bug #12544: invalid signature requests must NOT
|
||||
consume rate-limit quota. Before the fix, rate limiting was applied before
|
||||
signature validation, so an attacker could exhaust a victim's rate limit
|
||||
with invalidly-signed requests and then make valid requests that get rejected
|
||||
with 429.
|
||||
|
||||
The correct order is:
|
||||
1. Read body
|
||||
2. Validate HMAC signature (reject 401 if invalid)
|
||||
3. Rate limit check (reject 429 if over limit)
|
||||
4. Process the webhook
|
||||
"""
|
||||
|
||||
import hashlib
|
||||
import hmac
|
||||
import json
|
||||
|
||||
import pytest
|
||||
from aiohttp import web
|
||||
from aiohttp.test_utils import TestClient, TestServer
|
||||
|
||||
from gateway.platforms.webhook import WebhookAdapter
|
||||
from gateway.config import PlatformConfig
|
||||
|
||||
|
||||
def _make_adapter(routes, rate_limit=5, **extra_kw) -> WebhookAdapter:
|
||||
"""Create a WebhookAdapter with the given routes."""
|
||||
extra = {
|
||||
"host": "0.0.0.0",
|
||||
"port": 0,
|
||||
"routes": routes,
|
||||
"rate_limit": rate_limit,
|
||||
}
|
||||
extra.update(extra_kw)
|
||||
config = PlatformConfig(enabled=True, extra=extra)
|
||||
return WebhookAdapter(config)
|
||||
|
||||
|
||||
def _create_app(adapter: WebhookAdapter) -> web.Application:
|
||||
"""Build the aiohttp Application from the adapter."""
|
||||
app = web.Application()
|
||||
app.router.add_get("/health", adapter._handle_health)
|
||||
app.router.add_post("/webhooks/{route_name}", adapter._handle_webhook)
|
||||
return app
|
||||
|
||||
|
||||
def _github_signature(body: bytes, secret: str) -> str:
|
||||
"""Compute X-Hub-Signature-256 for *body* using *secret*."""
|
||||
return "sha256=" + hmac.new(
|
||||
secret.encode(), body, hashlib.sha256
|
||||
).hexdigest()
|
||||
|
||||
|
||||
SIMPLE_PAYLOAD = {"event": "test", "data": "hello"}
|
||||
|
||||
|
||||
class TestSignatureBeforeRateLimit:
|
||||
"""Verify that invalid signatures do NOT consume rate limit quota."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_invalid_signature_does_not_consume_rate_limit(self):
|
||||
"""Send requests with invalid signatures up to the rate limit, then
|
||||
send a valid-signed request and verify it succeeds.
|
||||
|
||||
BEFORE FIX: Invalid signatures consume the rate limit bucket, so
|
||||
after 'rate_limit' bad requests the valid one would get 429.
|
||||
AFTER FIX: Invalid signatures are rejected with 401 first (before
|
||||
rate limiting), so the rate limit bucket is untouched. The valid
|
||||
request after many bad ones still succeeds.
|
||||
"""
|
||||
secret = "test-secret-key"
|
||||
route_name = "test-route"
|
||||
routes = {
|
||||
route_name: {
|
||||
"secret": secret,
|
||||
"events": ["push"],
|
||||
"prompt": "Event: {event}",
|
||||
"deliver": "log",
|
||||
}
|
||||
}
|
||||
rate_limit = 5
|
||||
adapter = _make_adapter(routes, rate_limit=rate_limit)
|
||||
|
||||
captured_events = []
|
||||
|
||||
async def _capture(event):
|
||||
captured_events.append(event)
|
||||
|
||||
adapter.handle_message = _capture
|
||||
app = _create_app(adapter)
|
||||
|
||||
body = json.dumps(SIMPLE_PAYLOAD).encode()
|
||||
|
||||
async with TestClient(TestServer(app)) as cli:
|
||||
# First exhaust the rate limit with invalid signatures
|
||||
for i in range(rate_limit):
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": "sha256=invalid", # bad sig
|
||||
"X-GitHub-Delivery": f"bad-{i}",
|
||||
},
|
||||
)
|
||||
# Each invalid signature should be rejected with 401
|
||||
assert resp.status == 401, (
|
||||
f"Expected 401 for invalid signature, got {resp.status}"
|
||||
)
|
||||
|
||||
# Now send a valid-signed request — it MUST succeed (202)
|
||||
# BEFORE FIX: This would return 429 because the 5 bad requests
|
||||
# consumed the rate limit bucket.
|
||||
# AFTER FIX: Bad requests don't touch rate limiting, so valid
|
||||
# request succeeds.
|
||||
valid_sig = _github_signature(body, secret)
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": valid_sig,
|
||||
"X-GitHub-Delivery": "good-001",
|
||||
},
|
||||
)
|
||||
assert resp.status == 202, (
|
||||
f"Expected 202 for valid request after invalid signatures, "
|
||||
f"got {resp.status}. Rate limit may have been consumed by "
|
||||
f"invalid requests (bug #12544 not fixed)."
|
||||
)
|
||||
|
||||
data = await resp.json()
|
||||
assert data["status"] == "accepted"
|
||||
|
||||
# The valid event should have been captured
|
||||
assert len(captured_events) == 1
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_valid_signature_still_rate_limited(self):
|
||||
"""Verify that VALID requests still respect rate limiting normally."""
|
||||
secret = "test-secret-key"
|
||||
route_name = "test-route"
|
||||
routes = {
|
||||
route_name: {
|
||||
"secret": secret,
|
||||
"events": ["push"],
|
||||
"prompt": "Event: {event}",
|
||||
"deliver": "log",
|
||||
}
|
||||
}
|
||||
rate_limit = 3
|
||||
adapter = _make_adapter(routes, rate_limit=rate_limit)
|
||||
|
||||
captured_events = []
|
||||
|
||||
async def _capture(event):
|
||||
captured_events.append(event)
|
||||
|
||||
adapter.handle_message = _capture
|
||||
app = _create_app(adapter)
|
||||
|
||||
body = json.dumps(SIMPLE_PAYLOAD).encode()
|
||||
|
||||
async with TestClient(TestServer(app)) as cli:
|
||||
# Send 'rate_limit' valid requests — all should succeed
|
||||
for i in range(rate_limit):
|
||||
valid_sig = _github_signature(body, secret)
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": valid_sig,
|
||||
"X-GitHub-Delivery": f"good-{i}",
|
||||
},
|
||||
)
|
||||
assert resp.status == 202
|
||||
|
||||
# The next valid request SHOULD be rate-limited
|
||||
valid_sig = _github_signature(body, secret)
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": valid_sig,
|
||||
"X-GitHub-Delivery": "good-over-limit",
|
||||
},
|
||||
)
|
||||
assert resp.status == 429, (
|
||||
f"Expected 429 when exceeding rate limit with valid requests, "
|
||||
f"got {resp.status}"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mixed_valid_and_invalid_signatures(self):
|
||||
"""Interleave invalid and valid requests. Only valid ones count
|
||||
against the rate limit."""
|
||||
secret = "test-secret-key"
|
||||
route_name = "test-route"
|
||||
routes = {
|
||||
route_name: {
|
||||
"secret": secret,
|
||||
"events": ["push"],
|
||||
"prompt": "Event: {event}",
|
||||
"deliver": "log",
|
||||
}
|
||||
}
|
||||
rate_limit = 3
|
||||
adapter = _make_adapter(routes, rate_limit=rate_limit)
|
||||
|
||||
captured_events = []
|
||||
|
||||
async def _capture(event):
|
||||
captured_events.append(event)
|
||||
|
||||
adapter.handle_message = _capture
|
||||
app = _create_app(adapter)
|
||||
|
||||
body = json.dumps(SIMPLE_PAYLOAD).encode()
|
||||
|
||||
async with TestClient(TestServer(app)) as cli:
|
||||
# Send 2 valid requests (should succeed)
|
||||
for i in range(2):
|
||||
valid_sig = _github_signature(body, secret)
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": valid_sig,
|
||||
"X-GitHub-Delivery": f"good-{i}",
|
||||
},
|
||||
)
|
||||
assert resp.status == 202
|
||||
|
||||
# Send 10 invalid requests (should all get 401, not consume quota)
|
||||
for i in range(10):
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": "sha256=invalid",
|
||||
"X-GitHub-Delivery": f"bad-{i}",
|
||||
},
|
||||
)
|
||||
assert resp.status == 401
|
||||
|
||||
# One more valid request should STILL succeed (only 2 consumed)
|
||||
valid_sig = _github_signature(body, secret)
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": valid_sig,
|
||||
"X-GitHub-Delivery": "good-3",
|
||||
},
|
||||
)
|
||||
assert resp.status == 202, (
|
||||
f"Expected 202 for 3rd valid request after many invalid ones, "
|
||||
f"got {resp.status}"
|
||||
)
|
||||
|
||||
# The 4th valid request should be rate-limited (2 + 2 = 4 = limit)
|
||||
valid_sig = _github_signature(body, secret)
|
||||
resp = await cli.post(
|
||||
f"/webhooks/{route_name}",
|
||||
data=body,
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"X-GitHub-Event": "push",
|
||||
"X-Hub-Signature-256": valid_sig,
|
||||
"X-GitHub-Delivery": "good-4",
|
||||
},
|
||||
)
|
||||
assert resp.status == 429
|
||||
|
||||
assert len(captured_events) == 3
|
||||
Loading…
Add table
Add a link
Reference in a new issue