Skip to content

feat: sign and deliver MCP events safely - #133

Open
ChiragAgg5k wants to merge 2 commits into
feat/events-envelopefrom
feat/events-delivery
Open

ChiragAgg5k wants to merge 2 commits into
feat/events-envelopefrom
feat/events-delivery

Conversation

@ChiragAgg5k

@ChiragAgg5k ChiragAgg5k commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Stack

feat/events ← #131 ← #132 ← #133 ← #134 ← #135 (events/subscribe / events/unsubscribe)

Merges into feat/events; feat/events → main lands 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.py or http_app.py yet.

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 and Host, 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.
  • Declares httpcore explicitly since egress.py imports it (already locked as an httpx dependency; no new packages).

Design notes

SSRF. egress.Backend is an httpcore.AsyncNetworkBackend plugged into the connection pool of an httpx.AsyncHTTPTransport subclass. httpcore calls connect_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 with server_hostname taken from the request origin, so SNI and certificate validation use the hostname, and httpx builds Host from 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() combines ipaddress.is_global and 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::/23 incl. Teredo, 2001:db8::/32, 2002::/16 6to4, 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 and trust_env are off, because a proxy would resolve the name itself. Responses are read raw with Accept-Encoding: identity, so a compressed reply can't inflate past the cap. The only loosening is the explicit allow_loopback=True constructor flag (loopback plus http), which only tests use. Nothing reads it from the environment.

Verification. A signed {"type":"verification","challenge":<32 random bytes, urlsafe>} with webhook-id: msg_verification_<random>. The endpoint must return a 2xx JSON object whose challenge matches, compared with hmac.compare_digest. Failures raise CallbackError(reason): 4xx is http_4xx, 5xx is http_5xx, 3xx or a missing or wrong echo is challenge_failed, a timeout is timeout, an ssl.SSLError anywhere in the cause chain is tls_error, and a forbidden destination or other connection failure is connection_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). RetryPolicy defaults 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, and webhook-id stays the event id. Any non-2xx except 410/413 is retried, as are timeouts and connection errors (the sketch says retry on non-2xx). 410/413 and a forbidden destination are final (rejected). Exhausted retries are abandoned with the last error category. Retries run in-process via an injectable sleep, 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 real Dispatcher and Egress deliver to a local HTTPS receiver that checks every request with the official standardwebhooks library:

  • Signing: every delivery verifies with standardwebhooks. Body is exactly {eventId, name, timestamp, data, cursor: null}, webhook-id equals eventId, X-MCP-Subscription-Id and Content-Type are set. During a secret rotation the header carries two signatures; each secret verifies and an unrelated one does not.
  • Retries with a short injected policy (0.3 s, 0.3 s, 0.6 s; 1 s timeout): 500, then a timeout, then 302 (the Location is never requested), then 200. That gives four attempts with the same webhook-id, re-signed with non-decreasing and finally newer timestamps. 410 and 413 get exactly one attempt each (rejected / http_4xx), and 503 is abandoned after 4 attempts (http_5xx).
  • TLS: a receiver whose certificate is not trusted gets no request, the handshake fails, and the delivery is abandoned as tls_error.
  • Address pinning: the egress dials 127.0.0.1, while the receiver sees SNI localhost and Host: localhost:<port>. The certificate has only a DNS SAN, so it validated against the hostname.
  • The default (production) egress refuses a loopback callback (rejected / connection_refused), and the receiver gets nothing.
  • A burst of 8 events to one host peaks at 4 in-flight requests.
  • An event over 256 KiB is not sent (too_large).

Unit tests kept, and why they are not e2e:

  • tests/unit/test_events_egress.py:
    • The blocklist table (44 addresses incl. IPv6, IPv4-mapped, NAT64, 6to4 and Teredo), the allow table and the loopback flag's scope. A test machine cannot route to most of these addresses.
    • Mixed public/private DNS answers, DNS rebinding between check and connect, and the connect-target spy. These need a resolver that answers differently on demand.
    • URL shapes (scheme, credentials, missing host) that subscribe (PR 5) will reject before any delivery.
    • The 16 KiB response body cap. Deliveries discard response bodies, so the cap is invisible on the wire.
  • 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. Only events/subscribe (PR 5) calls them, and nothing in this stack does yet. Replace them with its e2e flow.
    • The production retry schedule: 30 s / 2 min / 8 min with ±20% jitter, 4 attempts, worst case under 15 minutes. Its real-time waits are too long to run end to end.

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: pass
  • uv run --group dev black --check src tests: pass
  • uv run --group dev pyright: 0 errors
  • uv run python -m unittest discover -s tests/unit: 304 tests OK
  • uv run --group e2e python -m unittest discover -s tests/e2e: 2 tests OK (about 4 s)
  • docker build -t appwrite-mcp:e2e .: builds

@hansi-codes

hansi-codes Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

🟡 Tier B · Needs changes before merging

Two previously reported findings remain open: the loopback flag permits HTTP to public destinations, and semaphore wait is outside the request deadline.

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.

Verdict New comments Fixed Still open
💬 Commented 0 0 2

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
File Change
.env.example Documents the optional MCP Events sealing-key setting.
.github/workflows/ci.yml Adds an end-to-end test job.
AGENTS.md Updates local test and contribution guidance for E2E coverage.
README.md Links to the MCP Events documentation.
docs/development.md Documents sealing keys and the E2E test suite.
docs/events.md Adds the MCP Events feature, catalog, error, and testing guide.
docs/flags.md Documents enabling the feature flag and checking discovery and the catalog.
pyproject.toml Adds cryptography and httpcore runtime dependencies and the E2E test group.
uv.lock Locks the added runtime and E2E dependencies.
src/mcp_server_appwrite/events/__init__.py Introduces the events package.
src/mcp_server_appwrite/events/catalog.py Defines event schemas, argument validation, and Appwrite event patterns.
src/mcp_server_appwrite/events/delivery.py Implements webhook signing, callback verification, and bounded event retries.
src/mcp_server_appwrite/events/egress.py Adds DNS-checked outbound HTTP with response limits and per-host concurrency control.
src/mcp_server_appwrite/events/envelope.py Adds sealed subscription envelopes, key rotation, and Appwrite signature helpers.
src/mcp_server_appwrite/events/errors.py Defines MCP Events JSON-RPC errors and error data.
src/mcp_server_appwrite/events/protocol.py Adds feature-gated events discovery and the events/list handler.
src/mcp_server_appwrite/flags.py Registers the events flag and its enabled-value parser.
src/mcp_server_appwrite/server.py Registers event protocol support when enabled for HTTP transport.
tests/e2e/support.py Adds a hosted-server harness and raw HTTP MCP client for E2E tests.
tests/e2e/test_events_protocol.py Exercises feature-flagged discovery, catalog schemas, and disabled behavior.
tests/unit/test_events_catalog.py Tests catalog argument validation and unknown-event errors.
tests/unit/test_events_delivery.py Tests secret validation, callback verification, signing, and retry behavior.
tests/unit/test_events_egress.py Tests destination policy, DNS checks, and bounded response bodies.
tests/unit/test_events_envelope.py Tests subscription sealing, keyring behavior, and Appwrite signature helpers.
tests/unit/test_events_protocol.py Checks events remain disabled on stdio.
⏳ Still open from earlier reviews · 2

Reviewed 8cc957c · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Tier A · See the inline comments. Summary

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}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +346 to +347
async with self._limiter.slot(parsed.host):
with anyio.fail_after(self._timeout):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant