diff --git a/gateway/platforms/webhook.py b/gateway/platforms/webhook.py index 9995ac3870..e3a736a451 100644 --- a/gateway/platforms/webhook.py +++ b/gateway/platforms/webhook.py @@ -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) diff --git a/tests/gateway/test_webhook_signature_rate_limit.py b/tests/gateway/test_webhook_signature_rate_limit.py new file mode 100644 index 0000000000..54d733f01b --- /dev/null +++ b/tests/gateway/test_webhook_signature_rate_limit.py @@ -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