Fix/spec webhook verification example - #116
Merged
Merged
Conversation
…n resource
The subscription resource was addressed five different ways across the
repository, and two of those paths were load-bearing for conformance:
spec §5.1 POST /eep/subscribe
spec §10 POST /subscribe
spec §10 POST /subscriptions/:id/resume
middleware GET|DELETE /eep/subscribe/:id
compliance-cli GET /eep/subscriptions
compliance-cli POST /eep/subscriptions/:id/test
`POST /eep/subscriptions/:id/test` is how the Core-tier probe triggers a
delivery in order to verify Standard Webhooks headers and the HMAC
signature. It was specified nowhere and implemented nowhere — not in the
middleware, not in either reference implementation. Worse, it failed
silently: `fetch` rejects only on a transport error, so a 404 resolved
normally, the runner slept 5s, received nothing, and reported "Webhook
delivery received: FAIL". Implementers saw a delivery failure and went
hunting in their own dispatcher for a bug that was never theirs.
`GET /eep/subscriptions` (the Standard-tier rate-limit probe) had the
same shape: asserted against a 404.
The paths chosen here are not new. `docs/guides/how-to-subscribe.md`
has documented `/eep/subscriptions/{id}` with `pause`, `resume`, `test`
and `DELETE` since v0.1, and `delivery_guarantees.md` references the
same collection. The guide was right; the spec and the middleware were
the outliers. Creation stays on `POST /eep/subscribe` because that is
what the manifest advertises as `layers.layer2_webhook` and what the
`rel="subscribe"` Link header points at.
Changes:
- Spec: new §5.1.1 making the subscription resource normative — the
member operations, their status codes and scopes, the rule that
`delivery_secret` is never re-exposed, 404-not-403 for another
subscriber's id, and the semantics of a test delivery.
- Spec: §10 lifecycle now uses the canonical paths and defines what a
"failed delivery" is (a fully exhausted §5.4 retry schedule); §14.2's
conformance line points at §5.1.1 instead of claiming an unwritten
lifecycle.
- Middleware: serve list / pause / resume / test alongside the existing
status and unsubscribe handlers, all under `/eep/subscriptions`, with
the pre-§5.1.1 `/eep/subscribe/:id` paths kept as deprecated aliases.
- Middleware: `WebhookDispatcher` routes `com.eep.subscription.test` to
the single subscription in `data.subscription_id` rather than fanning
out by `event_types` — a test delivery must reach a subscriber whose
patterns would never match it, and must reach nobody else.
- compliance-cli: the trigger now reports its own outcome, naming the
missing endpoint on a 404, and downstream signature probes SKIP rather
than FAIL when no delivery could be triggered.
Refs: EEP audit 2026-08 findings A4, A9
Signed-off-by: Ugur Cekmez <ucekmez@gmail.com>
fix(spec,middleware,compliance-cli): define one canonical subscription resource
There was a problem hiding this comment.
🟡 Changes recommended
The newly added subscription-management endpoints are not scoped to an authenticated caller and the test-delivery trigger lacks the spec-required rate limiting, creating concrete security/abuse risks and a mismatch with §5.1.1.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements the normative subscription-management API introduced in SPECIFICATION.md §5.1.1, including a synthetic “test delivery” event path that enables conformance tooling to verify Standard Webhooks signing without waiting for organic traffic.
Changes:
- Add
/eep/subscriptions/*member endpoints (list/pause/resume/test) to@eep-dev/middlewareand keep deprecated/eep/subscribe/:idaliases for 0.1.x. - Ensure
WebhookDispatcherroutescom.eep.subscription.testto exactly one addressed subscription (bypassingevent_typesmatching) and add focused tests. - Update
@eep-dev/compliance-climessaging/flow around the §5.1.1 test trigger, and make §5.1.1 normative in the spec text.
File summaries
| File | Description |
|---|---|
| packages/@eep-dev/middleware/src/index.ts | Re-export test-delivery event type constant. |
| packages/@eep-dev/middleware/src/dispatcher/webhook-dispatcher.ts | Route synthetic test deliveries to a single subscription ID (no fan-out). |
| packages/@eep-dev/middleware/src/dispatcher/webhook-dispatcher.test.ts | Add coverage for §5.1.1 test delivery routing + signing behavior. |
| packages/@eep-dev/middleware/src/core/request-handler.ts | Define TEST_DELIVERY_EVENT_TYPE constant for shared use. |
| packages/@eep-dev/middleware/src/core/eep-server.ts | Add /eep/subscriptions endpoints, plus deprecated aliases under /eep/subscribe/:id. |
| packages/@eep-dev/middleware/src/core/eep-server.test.ts | Expand route + handler test coverage for new subscription endpoints. |
| packages/@eep-dev/compliance-cli/src/index.ts | Improve §5.1.1 trigger handling/reporting and downstream probe skipping logic. |
| docs/current/SPECIFICATION.md | Make §5.1.1 endpoints normative; clarify lifecycle diagram text and requirements. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+396
to
+401
| getSubscriptionListHandler(): RequestHandler { | ||
| return async () => { | ||
| const subscriptions = await this.dbAdapter.listSubscriptions(); | ||
| const safe = subscriptions.map(({ delivery_secret: _secret, ...rest }) => rest); | ||
| return { status: 200, body: { subscriptions: safe, count: safe.length } }; | ||
| }; |
Comment on lines
+488
to
+512
| getSubscriptionTestHandler(): RequestHandler { | ||
| return async (request) => { | ||
| const subscriptionId = request.params?.subscriptionId; | ||
| if (!subscriptionId) { | ||
| return { | ||
| status: 400, | ||
| body: { error: "invalid_request", message: "subscription_id is required" } | ||
| }; | ||
| } | ||
| const subscription = await this.dbAdapter.getSubscription(subscriptionId); | ||
| if (!subscription) { | ||
| return { | ||
| status: 404, | ||
| body: { error: "not_found", message: `subscription ${subscriptionId} does not exist` } | ||
| }; | ||
| } | ||
| if (subscription.status !== "active") { | ||
| return { | ||
| status: 409, | ||
| body: { | ||
| error: "conflict", | ||
| message: `subscription ${subscriptionId} is ${subscription.status}; test deliveries require an active subscription` | ||
| } | ||
| }; | ||
| } |
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
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.
Summary
Scope
Checklist
npm test/pytestin affected packages).Notes for reviewers