Repository navigation
feat: serve events/subscribe and events/unsubscribe - #135
Open
ChiragAgg5k wants to merge 3 commits into
Open
ChiragAgg5k wants to merge 3 commits into
ChiragAgg5k wants to merge 3 commits into
Conversation
Each subscription is one managed Appwrite webhook in the subscriber's project, written with their own OAuth token, so the hosted server stays stateless. Subscribe validates, authorizes with a resource read, cleans up the caller's expired webhooks, verifies the callback and upserts the webhook; unsubscribe deletes it by the principal-bound id.
An Appwrite REST emulator joins the e2e harness, and the webhooks worker now fires the webhooks the server actually stored, closing the loop from subscribe to signed delivery to unsubscribe. The unit tests that only existed because nothing called the catalog, whsec_ validation, the handshake or subscription ids over HTTP are gone.
Covers the TTL policy, the webhook label used for cleanup, Free-plan limits, required OAuth scopes and secret rotation, and keeps the events flag framed as a testing flag.
🔵 Tier A · Mergeable after minor fixes
Adds HTTP-only 2 of 20 changed files were too large to include in full.
Note Not approving while a bug finding is open: Map a missing project on unsubscribe to Forbidden
Fix with agent prompt### Issue 1
src/mcp_server_appwrite/events/subscriptions.py:564-565
**Map a missing project on unsubscribe to Forbidden**
When `events/unsubscribe` targets a deleted or nonexistent project, `webhooks.get()` returns `404 project_not_found` and `_remove` reaches this fallback, producing `-32603` instead of the documented `-32012`. Mapping this Appwrite error type to Forbidden would keep a caller-side project error from being reported as an internal failure.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 21
Reviewed |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
Comment on lines
+564
to
+565
| return EventsError.internal( | ||
| f"Appwrite could not save the subscription (HTTP {code or 'error'})" |
There was a problem hiding this comment.
Map a missing project on unsubscribe to Forbidden
When events/unsubscribe targets a deleted or nonexistent project, webhooks.get() returns 404 project_not_found and _remove reaches this fallback, producing -32603 instead of the documented -32012. Mapping this Appwrite error type to Forbidden would keep a caller-side project error from being reported as an internal failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcp_server_appwrite/events/subscriptions.py
Line: 564-565
Comment:
**Map a missing project on unsubscribe to Forbidden**
When `events/unsubscribe` targets a deleted or nonexistent project, `webhooks.get()` returns `404 project_not_found` and `_remove` reaches this fallback, producing `-32603` instead of the documented `-32012`. Mapping this Appwrite error type to Forbidden would keep a caller-side project error from being reported as an internal failure.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · error-handling · 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 ← this PRMerges 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
The top layer of MCP Events (#127):
events/subscribeandevents/unsubscribe, registered byprotocol.register(server, …)only with theMCP_EVENTSflag on and the HTTP transport. The server stays stateless: each subscription is one managed Appwrite webhook in the subscriber's project, written with the subscriber's own OAuth token, and the sealed envelope lives only in itsauthPassword.events/subscriptions.py:Subscriptions.subscribe/unsubscribe, the TTLGrant, and the mapping from Appwrite errors to events codes.events/webhooks.py: what a managed webhook looks like (Labelname, fixedauthUsername, derived secret), how it is recognized, and the SDK calls (appwrite_consoleWebhooksservice).events/catalog.py: each event declares its authorization read (Access: path, scope, how errors name the resource).events/protocol.py: registers both methods, withtools/call-style telemetry and Sentry reporting.events/ingress.py: exposes its keyring, egress and dispatcher so subscribe shares them.server.py/http_app.py:build_mcp_server(..., ingress=), so subscribe writes webhooks pointing at the same ingress the app mounts.telemetry.py:mcp.events.subscriptions(operation, outcome, event, reason).Subscribe flow
catalog.lookup(name)(-32011,kind: "event"),event.validate(arguments)(-32602),delivery.modemust bewebhook(-32014, absent means webhook), callback must behttpswithout credentials and passEgress.check(private, loopback, metadata →-32602),delivery.decode_secret(-32602),ttlMs/maxAgeMsshape (-32602).Principal(iss, sub, client_id).digest. A token withoutsubis-32012; no token never reaches JSON-RPC (HTTP 401).EnvelopeTooLarge→-32602. Sealing happens before anything leaves the server.resolve_client(project_id):GET /functions/{id},/sites/{id},/tablesdb/{db}/tables/{table},/storage/buckets/{id}, orGET /userswithlimit(1)(least privilege that provesusers.read).project:webhooks.*scope fails before the callback gets a POST.$id= subscription id,url=Ingress.url(id),tls: true,enabled: true,events=event.patterns(arguments),authUsername=mcp-events,authPassword= envelope,secret=keyring.signing_key(id)), or on refreshPUTthenPATCH /secret. A409race falls through to the refresh path.Result:
{id, refreshBefore, cursor: null, truncated: false}plus the SDK'sresultType: "complete".events/unsubscribevalidates name and arguments, recomputes the id from the principal anddelivery.url, and deletes the webhook only if it is managed. Missing → still{}. The id includes the principal, so a caller can only reach their own subscriptions (tested with a second user).Decisions
Verification is not cached; the handshake runs on every subscribe and refresh. There is no server-side store, and
authPasswordis write-only, so the server cannot tell whether a refresh carries the same callback and secret as the live webhook. Keeping a "verified" fact in the new envelope would not help either: it would have to be trusted from the request being verified. One signed POST per refresh (hourly by default, at most every 5 minutes) is cheap, keeps the spec's MUST trivially true, and re-proves the callback wants deliveries under the secret it is about to get. The spec's per-(principal, url)cache is a SHOULD-level optimization for servers with memory.Expired cleanup reads expiry from the webhook name. The name is a label:
MCP events | <event> | until <UTC expiry> | owner <first 8 chars of the principal digest>. It is the only place the expiry can be read back without opening the envelope.$updatedAt+ max TTL was the alternative, but it would keep an expired 1 h subscription holding a Free-plan slot for up to 24 h. Cleanup deletes only webhooks whose id is a subscription id and whose name parses as a label, whose owner tag is the caller's, and whose expiry has passed. A tag collision between users could at worst delete an expired webhook whose deliveries the ingress already drops. The separator is ASCII on purpose: Appwrite echoes the name in theX-Appwrite-Webhook-Nameheader of every delivery (the first draft used·, which the e2e caught as an invalid header).NotFound vs Forbidden. Appwrite authenticates the caller against the project before it looks the resource up, so a
404for the resource itself (function_not_found,table_not_found,database_not_found, …) means the caller is in the project and the resource is missing: NotFound withdata.kind = "resource".401/403(not a member, missing scope) is Forbidden, naming the scope to grant when Appwrite saysgeneral_unauthorized_scope.404 project_not_foundis also Forbidden, so project ids cannot be probed.Secret rotation replaces; it cannot dual-sign. The previous secret exists only in the old envelope, which Appwrite never returns, and keeping it anywhere else would put a secret outside the envelope, which #127 rules out. Deliveries already in flight were opened from the old envelope and keep signing with the old secret through their retries, which covers the spec's "in-flight deliveries verify under either" intent. New deliveries sign with the new secret only. Documented in
docs/events.md.TTL. Default 1 h, floor 5 min, cap 24 h; absent or
null→ 1 h (neverrefreshBefore: null). The envelope expires at the end of the grant, andrefreshBeforeis a tenth of the grant earlier (6 min for 1 h, 30 s at the floor), sorefreshBefore≤ granted expiry ≤ suggestion (except at the floor, which the spec allows).No partial state. Create is one call. On refresh, if the
PATCH /secretfails after thePUTsucceeded, the webhook is deleted rather than left with an envelope and signing key that may disagree (Appwrite's currentGET/PUTno longer return the secret, so it is always re-set rather than compared).Plan limit
max. Cloud refuses with403 additional_resource_not_allowedwhentotal >= planMax, sodata.maxis the number of webhooks in the project after cleanup.Error mapping
dataproject_id), non-https / credentialed / private / unresolvable callback, badwhsec_, oversized envelope, badttlMs/maxAgeMs-32602-32011{"kind": "event"}-32011{"kind": "resource"}project:webhooks.read/.write; nonexistent project; subscription id taken by a renamed webhook-32012-32013{"limit": "webhooks", "max": <n>}delivery.modenotwebhook-32014-32015{"reason": challenge_failed | timeout | connection_refused | tls_error | http_4xx | http_5xx}5xxor anything unexpected-32603Only
-32603goes to Sentry (tags:mcp.method,transport, client tags,event.name,appwrite.project_id). Every outcome is counted inmcp.messages.received,mcp.jsonrpc.errorsandmcp.events.subscriptions.Tests
Harness (
tests/e2e/support.py).Cloudis a small Appwrite REST emulator the server reaches throughAPPWRITE_ENDPOINT: the console region lookupresolve_clientmakes, the five authorization reads, and/v1/webhooks(list with limit/offset, create, get, update, delete,PATCH /secret). It checks projects (404 project_not_found), membership (401 user_unauthorized) and scopes (401 general_unauthorized_scope) like Appwrite, keepsauthPasswordwrite-only and returns the secret only from create and the secret update (current Appwrite), enforces the Free plan limit (403 additional_resource_not_allowed),CustomId, the 128-char name and a 2048-charauthPassword(what #14293 must allow), and takes injected failures.Appwrite.fire(cloud, project, event, body)delivers from the webhooks the server stored, with their real URL, Basic auth (authUsername+ envelope) and HMAC secret, matching events the way Appwrite's worker does. The token stub now returns the claims the real verifier produces (iss,sub,client_id,project_id) for a few users.Flows (
tests/e2e/test_events_subscribe.py, 8 tests, ~10 s):Appwrite.fire→ ingress →standardwebhooks-verified delivery. Refresh three times: same id, one webhook,refreshBeforefor the floor (5 min), the cap (24 h) andnull(1 h), with arguments reordered on one refresh; every refresh handshakes with a fresh challenge. Rotation: handshake and next delivery verify with the new secret only. Another user's unsubscribe for the same URL touches nothing. Unsubscribe → webhook gone → Appwrite fires nothing → no delivery; unsubscribing again is{}. Subscription counters: created +1, refreshed +4, removed +1, absent +2.-32011 kind: resource. Non-member, missingfunctions.read(message namesproject:functions.read), nonexistent project, and a token withoutsub→-32012. No bearer → HTTP 401. None of these POST to the callback or store a webhook.-32602): every ID argument of every event with*,a*,a.b,fn1.executions, empty, leading-/_, a space and 37 chars; an invalid deploymentstatus; unknown or missing arguments; http, ftp, file, host-less, unparseable, credentialed, private and metadata callbacks; missing, non-string, empty, unprefixed, uppercase-prefixed, spaced, non-base64, badly padded, 23-byte and 65-byte secrets; an oversized envelope; badttlMs/maxAgeMs. Unknown event →-32011 kind: event;poll/push→-32014. None of them reaches Appwrite or the callback.-32015):challenge_failed(endpoint cannot verify; and a 307 that is not followed),http_4xx,http_5xx,timeout,tls_error(untrusted certificate),connection_refused. No webhook stored, no raw response in the message.-32012naming both scopes, before any handshake (unsubscribe too). Appwrite503on the authorization read, the listing and the create →-32603with nothing stored.500onPATCH /secretduring a refresh →-32603and the webhook is deleted. Counters:mcp.jsonrpc.errors{-32603}+4.-32013{limit: "webhooks", max: 2}with an upgrade-to-Pro / unsubscribe message; unsubscribing frees the slot.sub_webhook renamed by a person, another user's expired subscription and our own live one all survive cleanup byte for byte; a renamed webhook holding the subscription's own id makes subscribe fail with-32012and unsubscribe leave it alone.-32602, with no Appwrite call and no POST.tests/e2e/test_events_protocol.py: subscribe/unsubscribe are no longer expected to be-32601with the flag on; with it off, all three methods are.tests/e2e/test_events_ingress.py: docstring now explains why those flows still hand-build webhooks (expired, retired-key, foreign-event, two-secret and tampered states subscribe never produces).Unit tests deleted because these flows now cover them:
tests/unit/test_events_catalog.py(whole file): argument validation for every ID argument, and NotFound for unknown events.tests/unit/test_events_delivery.py:SecretTest(allwhsec_cases) andVerificationTest(echo, fresh challenges, mismatch, 4xx/5xx, redirect not followed, timeout, connection refused, forbidden destination).tests/unit/test_events_envelope.py:SubscriptionIdTests(determinism, argument order, principal binding, Appwrite custom-id charset, signing key fitsText(256, 8)).tests/unit/test_events_egress.py:ValidateUrlTest(callback URL shapes).No remaining unit test covers the
EventsErrorconstructors: every code anddatashape is asserted on the wire instead.Unit tests kept, and why e2e cannot reach them:
test_events_envelope.py: the PHP-computed Appwrite signature vector (external truth; the harness signs with the same Python formula) and the envelope size budget (deliberately extreme inputs, the contract for #14293).test_events_egress.py: the address blocklist (most ranges are unroutable from a test machine), mixed DNS answers and rebinding (need a resolver that changes its answer), the connect-target spy, and the 16 KiB response cap (invisible on the wire).test_events_delivery.py: the production retry schedule (30 s / 2 min / 8 min, too long to wait for).test_events_protocol.py: stdio never serves events (stdio needs a live project to boot).Verification
Run locally on Python 3.12.8 with uv 0.11.22 lockfile (the CI versions):
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: 281 tests OKuv run --group e2e python -m unittest discover -s tests/e2e: 23 tests OK (about 30 s), run twicedocker build -t appwrite-mcp:subscribe .: buildsNot yet
A live end-to-end run against Cloud staging (real OAuth tokens, real Appwrite webhooks and plan limits) follows appwrite/appwrite#14293, which must let
authPasswordhold up to 2048 characters (column andText(256)validator). Until then the emulator above plays Appwrite.