Repository navigation
feat: seal MCP event subscriptions into an envelope - #132
Open
ChiragAgg5k wants to merge 2 commits into
Open
ChiragAgg5k wants to merge 2 commits into
ChiragAgg5k wants to merge 2 commits into
Conversation
🔵 Tier A · Mergeable after minor fixes
This change adds an MCP Events catalog and protocol support, plus helpers for deterministic subscription IDs, sealed subscription envelopes, key rotation, and Appwrite webhook signatures. It also adds event documentation, configuration guidance, and an end-to-end test harness for hosted protocol discovery and catalog listing.
Note Not approving while a bug finding is open: Restrict subscription IDs to the generated hex suffix
Fix with agent prompt### Issue 1
src/mcp_server_appwrite/events/envelope.py:390-392
**Add direct tests for envelope opening and tampering**
The envelope tests only seal subscriptions to measure their size; none call `Keyring.open`, and the added E2E tests do not exercise ingress. A regression in decryption or the ID/project associated-data binding could therefore pass the suite, so please add direct round-trip and rejection tests for this security boundary.
### Issue 2
src/mcp_server_appwrite/events/envelope.py:155-159
**Restrict subscription IDs to the generated hex suffix**
This accepts any 32-character alphanumeric suffix, such as `sub_zzzz...`, even though `subscription_id()` always emits 32 lowercase hexadecimal characters and the docstring says this checks IDs the module could produce. A caller using this helper to reject foreign IDs will mistakenly accept IDs outside that format.
```suggestion
return re.fullmatch(r"sub_[0-9a-f]{32}", value) is not None
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 7
⏳ Still open from earlier reviews · 1
Reviewed |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
Comment on lines
+155
to
+159
| return ( | ||
| len(value) == SUBSCRIPTION_ID_MAX | ||
| and value.startswith(SUBSCRIPTION_PREFIX) | ||
| and SUBSCRIPTION_ID_PATTERN.fullmatch(value) is not None | ||
| ) |
There was a problem hiding this comment.
Restrict subscription IDs to the generated hex suffix
This accepts any 32-character alphanumeric suffix, such as sub_zzzz..., even though subscription_id() always emits 32 lowercase hexadecimal characters and the docstring says this checks IDs the module could produce. A caller using this helper to reject foreign IDs will mistakenly accept IDs outside that format.
Suggested change
| return ( | |
| len(value) == SUBSCRIPTION_ID_MAX | |
| and value.startswith(SUBSCRIPTION_PREFIX) | |
| and SUBSCRIPTION_ID_PATTERN.fullmatch(value) is not None | |
| ) | |
| return re.fullmatch(r"sub_[0-9a-f]{32}", value) is not None |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcp_server_appwrite/events/envelope.py
Line: 155-159
Comment:
**Restrict subscription IDs to the generated hex suffix**
This accepts any 32-character alphanumeric suffix, such as `sub_zzzz...`, even though `subscription_id()` always emits 32 lowercase hexadecimal characters and the docstring says this checks IDs the module could produce. A caller using this helper to reject foreign IDs will mistakenly accept IDs outside that format.
```suggestion
return re.fullmatch(r"sub_[0-9a-f]{32}", value) is not None
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · bug · Reply if this doesn't apply.
Each MCP Events subscription is stored only in its Appwrite webhook. The envelope seals the delivery state into authPassword with AES-256-GCM, bound to the subscription id and project, and the webhook signing key is derived so the ingress can verify deliveries without any storage.
Round trips, tampering, binding, rotation and expiry are covered through the ingress e2e flows in the layer above. What stays is the PHP-computed Appwrite signature vector, subscription id determinism and charset until subscribe has an e2e flow, and the size budget for appwrite/appwrite#14293.
ChiragAgg5k
force-pushed
the
feat/events-envelope
branch
from
October 9, 2026 13:53
aef5109 to
a995f8f
Compare
Comment on lines
+390
to
+392
| plaintext = AESGCM(key.encryption).decrypt( | ||
| nonce, sealed, _associated(key.id, id, project) | ||
| ) |
There was a problem hiding this comment.
Add direct tests for envelope opening and tampering
The envelope tests only seal subscriptions to measure their size; none call Keyring.open, and the added E2E tests do not exercise ingress. A regression in decryption or the ID/project associated-data binding could therefore pass the suite, so please add direct round-trip and rejection tests for this security boundary.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcp_server_appwrite/events/envelope.py
Line: 390-392
Comment:
**Add direct tests for envelope opening and tampering**
The envelope tests only seal subscriptions to measure their size; none call `Keyring.open`, and the added E2E tests do not exercise ingress. A regression in decryption or the ID/project associated-data binding could therefore pass the suite, so please add direct round-trip and rejection tests for this security boundary.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · testing · Reply if this doesn't apply.
This was referenced Oct 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Summary
Part 3 of 4 for MCP Events (#127). Adds
mcp_server_appwrite.events.envelope, the sealed subscription envelope that lets the hosted server stay stateless: each subscription is one Appwrite project webhook, and everything the ingress needs to deliver an event is sealed into that webhook'sauthPassword, which Appwrite sends back as HTTP Basic auth on every delivery.This is a self-contained library. Nothing is wired into
server.pyorhttp_app.py; the subscribe and ingress PRs consume it.What's in it:
Subscriptionfrozen dataclass:id,project,name,arguments(sorted, read-only mapping),callback,secrets(currentwhsec_first, previous during rotation),expires(epoch ms),principal(digest, never raw tokens).Principal(issuer, subject, client).digest: base64url of the first 128 bits of SHA-256 over canonical JSON of OAuthiss,sub,client_id(22 chars).subscription_id(principal, callback, name, arguments):sub_+ 32 hex chars of SHA-256 over canonical JSON. Exactly 36 chars, valid for Appwrite'sCustomId/Keyvalidator ([A-Za-z0-9._-], no leading special char). Argument order does not change it.Keyring(fromMCP_EVENTS_SEALING_KEYS),seal/open, typedEnvelopeErrorwith anEnvelopeFailurereason, andSubscription.expired()kept separate from opening so the ingress can tell "expired, 2xx drop" from "invalid".secret(Keyring.signing_key) andappwrite_signature/verify_appwrite_signatureforX-Appwrite-Webhook-Signature.cryptographyis now a direct dependency (it was already locked at 49.0.0 throughpyjwt[crypto]).Format
mcp-events/seal/v1).nname,aarguments,ccallback,ssecrets,eexpiry (epoch ms),pprincipal digest.canonical_json(["v1", key id, subscription id, project id]), so an envelope copied onto another webhook or project, or relabelled with another key id, fails authentication. On open, the contents must also hash back to the subscription id (binding_mismatchotherwise).[A-Za-z0-9._-], safe as a Basic auth password. Appwrite only sends Basic auth whenauthUsernameis non-empty too, so the subscribe PR must set a fixed, non-secret username.Example (throwaway key;
tablesdb.row.created, 79-char callback URL, one 32-byte secret):Size budget
Measured with 36-char Appwrite ids, a 22-char principal digest, a 13-digit expiry and key id
k1. Secrets arewhsec_+ base64 of the stated byte length.tablesdb.row.created, 79-char URL, one 32-byte secret)users.user.created, 100-char URL, one 32-byte secrettablesdb.row.created, 150-char URL, one 32-byte secrettablesdb.row.created, 150-char URL, two 32-byte secrets (rotation)functions.deployment.completed+status, 300-char URL, two 64-byte secretstablesdb.row.created, 300-char URL, two 64-byte secretsENVELOPE_BUDGET(sealraisesEnvelopeTooLargeabove it)authPasswordtoday: reliable limit (encrypted column)authPasswordtoday: API validatorText(256)Even the smallest realistic envelope (~430) is over both current limits, so a compact binary format would not help: the callback URL and the secrets dominate and are incompressible. What appwrite/appwrite#14293 must allow:
authPasswordof at leastENVELOPE_BUDGET= 2048 characters, stored intact. That means the column change already proposed there (httpPass→VAR_TEXT65535) and raising theauthPasswordparam validator on create and update fromText(256)to at leastText(2048), which the issue doesn't propose yet.SizeBudgetTestsfails if the worst case grows past the budget; subscribe should mapEnvelopeTooLargeto-32602(callback URL too long).Key management
MCP_EVENTS_SEALING_KEYS=<id>:<base64 32 bytes>[,<id>:<base64 32 bytes>...], read from the environment like the other hosted config (Keyring.from_env()), documented in.env.exampleanddocs/development.md. In production it comes from Parameter Store.openuses the key id named in the envelope (no trial decryption).[A-Za-z0-9_-]+and must be unique. A missing variable raisesKeyringErrorwith a generation hint (openssl rand -base64 32).secretishex(HMAC-SHA256(HKDF(sealing key, "mcp-events/appwrite-signature/v1"), subscription id)): 64 chars, within Appwrite'sText(256, 8). Separate HKDF subkeys mean the AES key never doubles as an HMAC key. The ingress derives the secret from the key id in the delivery's envelope, since every webhook write carries an envelope and secret made from the same key.base64(HMAC-SHA1(url . body, secret))(src/Appwrite/Platform/Workers/Webhooks.php), whereurlis the webhook's configured URL. The ingress must rebuild the canonical ingress URL, not trust the URL the request arrived on.Tests
Envelope behavior is covered end to end by the ingress flows in #134 (
tests/e2e/test_events_ingress.py), which run against the real server:Keyring.sealand the ingress'sopen.401 envelope. An envelope for another project, or one copied from another webhook, also gets401 envelope.k1delivers; restarted withk2,k1, the old envelope still delivers and ak2one does too; restarted withk2only, thek1envelope gets200 retired_keyand nothing is delivered.200 dropped expiredand nothing is delivered.MCP_EVENTS_SEALING_KEYSis missing, and short, all-zero, repeated, non-base64, id-less and duplicate-id keys stop startup withKeyringError.Unit tests kept (
tests/unit/test_events_envelope.py, 11 tests), and why they are not e2e:CustomIdrules, and foreign ids are refused. Also covered: the principal digest is stable and 22 chars, and the derived webhooksecretfits Appwrite'sText(256, 8). Nothing derives an id from a request untilevents/subscribe(PR 5). Replace these with its e2e flow.ENVELOPE_BUDGET, and oversize envelopes raiseEnvelopeTooLarge. These are deliberately extreme inputs, and they are the contract Encrypted and length-limited attributes return 500 instead of 400 (webhooks authPassword/authUsername/url, variables value) appwrite#14293 has to meet.Everything else (canonical JSON, round trips, tampering per region, binding, rotation, keyring parsing, expiry) was removed in favor of the e2e flows above.
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: 279 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