Repository navigation
feat: sign and deliver MCP events safely - #133
ChiragAgg5k wants to merge 2 commits into
Conversation
🟡 Tier B · Needs changes before merging
Adds a feature-flagged MCP Events catalog and protocol support, plus standalone webhook signing, verification, retry, sealing, and SSRF-conscious egress components. It also adds event documentation, dependency and CI updates, and protocol and library tests. 1 of 24 changed files was too large to include in full.
Note Not approving while a bug finding is open: Limit the HTTP exception to loopback destinations Fix with agent prompt### Issue 1
src/mcp_server_appwrite/events/egress.py:136
**Limit the HTTP exception to loopback destinations**
With `allow_loopback=True`, this broadens the allowed schemes to HTTP for every host; the DNS policy still accepts globally routable answers. An embedding service that enables this test escape hatch can therefore send signed event data unencrypted to a public callback, so the HTTP allowance should also require a loopback destination.
### Issue 2
src/mcp_server_appwrite/events/egress.py:346-347
**Include semaphore wait time in the request deadline**
The per-host semaphore is acquired before `fail_after` starts, so requests queued behind slow calls have no deadline until a slot opens. Under a sufficiently long same-host queue, delivery can be delayed arbitrarily and exceed the documented per-request and retry time bounds; include the semaphore wait in the deadline.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 25
⏳ Still open from earlier reviews · 2
Reviewed |
| parsed = httpx.URL(url) | ||
| except (httpx.InvalidURL, TypeError) as error: | ||
| raise DestinationError("Callback URL is not a valid URL") from error | ||
| schemes = {SECURE_SCHEME, INSECURE_SCHEME} if allow_loopback else {SECURE_SCHEME} |
There was a problem hiding this comment.
Limit the HTTP exception to loopback destinations
With allow_loopback=True, this broadens the allowed schemes to HTTP for every host; the DNS policy still accepts globally routable answers. An embedding service that enables this test escape hatch can therefore send signed event data unencrypted to a public callback, so the HTTP allowance should also require a loopback destination.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcp_server_appwrite/events/egress.py
Line: 136
Comment:
**Limit the HTTP exception to loopback destinations**
With `allow_loopback=True`, this broadens the allowed schemes to HTTP for every host; the DNS policy still accepts globally routable answers. An embedding service that enables this test escape hatch can therefore send signed event data unencrypted to a public callback, so the HTTP allowance should also require a loopback destination.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · security · Reply if this doesn't apply.
| async with self._limiter.slot(parsed.host): | ||
| with anyio.fail_after(self._timeout): |
There was a problem hiding this comment.
Include semaphore wait time in the request deadline
The per-host semaphore is acquired before fail_after starts, so requests queued behind slow calls have no deadline until a slot opens. Under a sufficiently long same-host queue, delivery can be delayed arbitrarily and exceed the documented per-request and retry time bounds; include the semaphore wait in the deadline.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcp_server_appwrite/events/egress.py
Line: 346-347
Comment:
**Include semaphore wait time in the request deadline**
The per-host semaphore is acquired before `fail_after` starts, so requests queued behind slow calls have no deadline until a slot opens. Under a sufficiently long same-host queue, delivery can be delayed arbitrarily and exceed the documented per-request and retry time bounds; include the semaphore wait in the deadline.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · concurrency · Reply if this doesn't apply.
Callback URLs are client-chosen, so every webhook POST is an attacker-steerable request. Add an SSRF-safe egress path that pins the connection to a checked global address while keeping SNI and Host, and a Standard Webhooks dispatcher for verification handshakes and bounded, re-signed event delivery retries.
Signing, retries, final statuses, TLS and redirect handling, timeouts and per-host limits are covered through the ingress e2e flows (#134). What stays needs a controlled resolver or address table, runs only from subscribe (PR 5), or waits on the production retry schedule.
76310b5 to
8cc957c
Compare
Stack
feat/events← #131 ← #132 ← #133 ← #134 ← #135 (events/subscribe/events/unsubscribe)Merges into
feat/events;feat/events→mainlands as one feature after appwrite/appwrite#14293. Each PR's diff shows only its own layer, and every layer passes the full checklist on its own.Part of #127 (PR 2 of 4). A self-contained library; nothing is wired into
server.pyorhttp_app.pyyet.Summary
events/egress.py: the only outbound HTTP path for webhook callbacks (verification and delivery). https only, resolves DNS itself, refuses non-global addresses, connects to the checked IP while keeping SNI andHost, never follows redirects, one 10 s deadline per request, 16 KiB response cap, 4 concurrent requests per destination host.events/delivery.py: Standard Webhooks signing (webhook-id,webhook-timestamp,webhook-signature: v1,..., space-separated multi-signature for rotation),whsec_secret validation, the verification handshake, and single-event delivery with bounded retries.httpcoreexplicitly sinceegress.pyimports it (already locked as an httpx dependency; no new packages).Design notes
SSRF.
egress.Backendis anhttpcore.AsyncNetworkBackendplugged into the connection pool of anhttpx.AsyncHTTPTransportsubclass. httpcore callsconnect_tcp(hostname, port); the backend resolves the hostname, rejects the request if any answer is not permitted, and opens the socket to the checked IP string. httpcore then starts TLS withserver_hostnametaken from the request origin, so SNI and certificate validation use the hostname, and httpx buildsHostfrom the URL. The hostname is never resolved a second time, so rebinding between check and connect is impossible, and the check runs on every connection, not only at subscribe time.permitted()combinesipaddress.is_globaland the negative predicates with an explicit blocklist (0/8, 10/8, 100.64/10, 127/8, 169.254/16, 172.16/12, 192.0.0/24, documentation and benchmarking ranges, multicast, 240/4,::/96,100::/64,2001::/23incl. Teredo,2001:db8::/32,2002::/166to4,3fff::/20,fc00::/7,fe80::/10,fec0::/10,ff00::/8, local-use NAT64). IPv4-mapped and well-known NAT64 (64:ff9b::/96) addresses are judged by the embedded IPv4. Proxies andtrust_envare off, because a proxy would resolve the name itself. Responses are read raw withAccept-Encoding: identity, so a compressed reply can't inflate past the cap. The only loosening is the explicitallow_loopback=Trueconstructor flag (loopback plushttp), which only tests use. Nothing reads it from the environment.Verification. A signed
{"type":"verification","challenge":<32 random bytes, urlsafe>}withwebhook-id: msg_verification_<random>. The endpoint must return a 2xx JSON object whosechallengematches, compared withhmac.compare_digest. Failures raiseCallbackError(reason): 4xx ishttp_4xx, 5xx ishttp_5xx, 3xx or a missing or wrong echo ischallenge_failed, a timeout istimeout, anssl.SSLErroranywhere in the cause chain istls_error, and a forbidden destination or other connection failure isconnection_refused. Raw endpoint responses are never surfaced.Retry policy. One event per POST, body
{eventId, name, timestamp, data, cursor: null}serialized once as compact JSON, capped at 256 KiB before sending (PayloadTooLargeError).RetryPolicydefaults to 4 attempts with waits of 30 s, 2 min and 8 min, each jittered ±20%. The worst case is about 12.6 min of waiting plus 4 × 10 s timeouts, which is under 15 min. Every attempt is re-signed with a fresh timestamp, andwebhook-idstays the event id. Any non-2xx except410/413is retried, as are timeouts and connection errors (the sketch says retry on non-2xx).410/413and a forbidden destination are final (rejected). Exhausted retries areabandonedwith the last error category. Retries run in-process via an injectablesleep, and nothing is persisted.Tests
Delivery and egress are covered end to end by the ingress flows in #134 (
tests/e2e/test_events_ingress.py). There, the server's realDispatcherandEgressdeliver to a local HTTPS receiver that checks every request with the officialstandardwebhookslibrary:standardwebhooks. Body is exactly{eventId, name, timestamp, data, cursor: null},webhook-idequalseventId,X-MCP-Subscription-IdandContent-Typeare set. During a secret rotation the header carries two signatures; each secret verifies and an unrelated one does not.500, then a timeout, then302(theLocationis never requested), then200. That gives four attempts with the samewebhook-id, re-signed with non-decreasing and finally newer timestamps.410and413get exactly one attempt each (rejected/http_4xx), and503is abandoned after 4 attempts (http_5xx).tls_error.127.0.0.1, while the receiver sees SNIlocalhostandHost: localhost:<port>. The certificate has only a DNS SAN, so it validated against the hostname.rejected/connection_refused), and the receiver gets nothing.too_large).Unit tests kept, and why they are not e2e:
tests/unit/test_events_egress.py:tests/unit/test_events_delivery.py:whsec_validation and the verification handshake: success, single-use challenges, mismatch, malformed bodies, 4xx/5xx, a redirect not followed, timeout, connect error and a forbidden destination. Onlyevents/subscribe(PR 5) calls them, and nothing in this stack does yet. Replace them with its e2e flow.Removed: the Standard Webhooks vector test (superseded by verifying every e2e delivery with the official library), plus delivery body/headers, retry and re-signing, 410/413, transport errors, dual signing, the payload cap, timeouts, per-host concurrency and the local-TLS tests (SNI/Host, redirect, loopback without the flag, untrusted certificate). The e2e flows cover all of them.
Verification
Run locally on Python 3.12.8, lockfile written with uv 0.11.22 (the CI version):
uv run --group dev ruff check src tests: passuv run --group dev black --check src tests: passuv run --group dev pyright: 0 errorsuv run python -m unittest discover -s tests/unit: 304 tests OKuv run --group e2e python -m unittest discover -s tests/e2e: 2 tests OK (about 4 s)docker build -t appwrite-mcp:e2e .: builds