Repository navigation
Add inbound webhook ingestion for connectors (Stripe first) - #1372
Conversation
Any connector can opt in (webhookEnabled/webhookProvider/webhookSecret) to receive inbound webhooks at POST /api/webhooks/:connectorId, independent of its own connector type. Only Stripe's signature scheme is verified for now, per the scope agreed in issue HelpCode-ai#988: a generic HMAC fallback and other providers (Meta/WhatsApp) are left for follow-up PRs so this one stays small and reviewable. - WebhookEvent model stores verified deliveries only; nothing lands there before the signature checks out. A periodic sweep plus a per-connector cap on insert bound storage, both env-configurable (WEBHOOK_EVENT_RETENTION_DAYS, WEBHOOK_EVENT_MAX_PER_CONNECTOR). - webhookSecret is encrypted with the same helpers as authConfig and never returned by any API or tool response. - The endpoint answers identically for an unknown connector id, a disabled hook, and a bad signature, so it is not an oracle for probing which connector ids exist. Body size is capped below the global 10MB limit, checked before the HMAC runs. main.ts's existing raw-body capture (previously only /api/support/webhook) now also covers /api/webhooks/, since Stripe's HMAC needs the exact bytes sent, not a re-serialized body. - Connectors that opt in get one builtin tool, <prefix>_recent_events, auto-created the same way the existing GraphQL/Email builtins are. It reads WebhookEvent through a new special case in DynamicMcpTools.executeWithEngine() (same pattern as the existing `static`/email/odata_* cases) rather than any engine, since it never makes an outbound call. Also added the matching read-only special case in tool-annotations.ts, which has its own independent method-name dispatch and would otherwise have left the tool's readOnlyHint unset. - Fixed 6 pre-existing spec files whose DynamicMcpTools construction used positional arguments that silently shifted out of alignment once a new required constructor parameter was added; one of them (code-mode.integration.spec.ts) would have changed what it actually tested, not just failed to type-check. Verified live against a real Postgres instance and the real HTTP API (login, connector creation, builtin tool auto-creation, annotations, and the webhook endpoint itself with actually-computed signatures), not only unit tests.
helpcodeai
left a comment
There was a problem hiding this comment.
Thanks, this is solid work and it follows what we agreed in #988. The signature check, the identical 401 answers, the encrypted secret and the organizationId filter in recentEvents all look right, and the migration is additive.
Three changes before we merge:
- Tool on update. The
<prefix>_recent_eventstool is only created increate(). If someone turnswebhookEnabledon later withPUT /connectors/:id, no tool appears, and if they turn it off the tool stays. Please create the tool when the flag goes from false to true on update, and remove it when it goes from true to false. A spec for both cases would be great. limitinput.params.limit as numberis passed as is. A string like"abc"givesNaN, andMath.min(Math.max(NaN, 1), 200)isNaN, so Prisma fails ontake: NaN. Please convert withNumber()and fall back to 20 when the result is not a finite number.- Size and personal data. One call can return 200 events of up to 256 KB each. That is up to 50 MB of raw Stripe payloads in the model context, and Stripe events carry customer names, e-mails and addresses. Please lower the maximum to 50 and say in
docs/operations/observability.md(or the connector docs) that payloads are stored for 7 days and returned to the model as they are.
On our side we still need to decide about plan limits on Cloud and a dashboard UI. Neither blocks this PR: the API-only version is fine as a first slice.
…ut, event cap Per @helpcodeai's review on PR HelpCode-ai#1372 (issue HelpCode-ai#988): - Create/remove the <prefix>_recent_events builtin tool when webhookEnabled flips on/off through PUT /connectors/:id, not only at connector creation. Matched by endpointMapping.method on removal, so a rename in the same request can't leave the old tool behind. - Coerce recentEvents()'s limit with Number() and fall back to 20 when it is not a finite number, instead of letting a non-numeric value (e.g. "abc") reach Prisma as take: NaN. - Lower the per-call event cap from 200 to 50 (a Stripe payload can be up to 256KB, carrying customer PII) and document in docs/operations/observability.md that stored payloads are returned to the model as received and kept for WEBHOOK_EVENT_RETENTION_DAYS. Added specs for both tool-lifecycle transitions and the limit coercion; regenerated llms-full.txt for the doc change.
|
@helpcodeai I've pushed an update addressing all three points from your review:
Let me know if anything's not quite right — happy to adjust. |
helpcodeai
left a comment
There was a problem hiding this comment.
Thanks, all three points are fixed: the tool follows webhookEnabled on update (and matching by endpointMapping.method is a good call), limit falls back to 20, and the cap is 50 with the retention documented. Merging now. Plan limits on Cloud and a dashboard UI are on our side, we will open follow-up issues for them.
|
@rangarajan19 main got a new spec ( |
Summary
@helpcodeai First slice of inbound webhook ingestion, as discussed and agreed on in #988: the
POST /api/webhooks/:connectorIdendpoint with Stripe signature verification, the<prefix>_recent_eventsbuiltin tool, theWebhookEventmodel, and retention. Scoped to Stripe only, per your note to leave the generic HMAC fallback and other providers for follow-ups.If anything here is off, or there's a better way to do a part of it, please just say so — happy to fix it right away.
Changes
WebhookEventPrisma model + 3 new nullable columns onConnector(webhookEnabled,webhookProvider,webhookSecret);webhookSecretis encrypted with the same helpers asauthConfigand never returned by any API or tool response (verified against every connector-serializing path, including the export/import util).POST /api/webhooks/:connectorId(webhook-ingest.controller.ts), outsideee/:Stripe-Signatureverification (HMAC-SHA256 overtimestamp.body, 300s replay tolerance), a body cap below the global 10MB limit checked before the HMAC runs,@Throttle()on the route, and an identical response for an unknown connector id, a disabled hook, and a bad signature.main.ts's existing raw-body capture (previously only/api/support/webhook) now also covers/api/webhooks/, since the HMAC needs the exact bytes sent, not a re-serialized body.<prefix>_recent_eventsbuiltin tool, auto-created the same way the existing GraphQL/Email builtins are. It readsWebhookEventthrough a new special case inDynamicMcpTools.executeWithEngine()(same pattern as the existingstatic/email/odata_*cases), since it's a read of our own storage, never an outbound call. Added the matching read-only special case intool-annotations.tstoo, which has its own independent method-name dispatch and would otherwise have left the tool'sreadOnlyHintunset.ConnectorAuthStatusService's) plus a per-connector cap enforced on insert. Both configurable viaWEBHOOK_EVENT_RETENTION_DAYS(default 7) andWEBHOOK_EVENT_MAX_PER_CONNECTOR(default 500), documented indocs/operations/observability.md.DynamicMcpToolsconstruction used positional arguments that silently shifted out of alignment once the new required constructor parameter was added; one of them (code-mode.integration.spec.ts) would have changed what it actually tested, not just failed to type-check.Type
Testing
Commands run from
packages/backendunless noted:npx tsc --noEmit -p tsconfig.json— 0 errors (confirmed against a clean baseline viagit stash -udiff; no new errors introduced).npm run lint— 0 errors; 11 pre-existing warnings in unrelated files, unchanged.npx jest src/connectors/webhook-signature.util.spec.ts— 9/9 (valid signature, wrong secret, tampered body, stale timestamp, malformed/missing header, non-numeric timestamp, length-mismatch guard, empty secret).npx jest src/connectors/webhook-events.service.spec.ts— 9/9 (record + dedupe on redelivery, cap enforcement across connectors, query scoping, retention sweep).npx jest src/connectors/webhook-ingest.controller.spec.ts— 10/10 (valid delivery, unknown connector, disabled hook, webhooks-not-enabled, wrong provider, bad signature, missing/oversized body, duplicate delivery, non-JSON body).npx jest src/mcp-server/dynamic-mcp-tools.webhook.spec.ts— 3/3, exercising the actual tool-dispatch wiring inDynamicMcpTools.executeTool(), not just the service in isolation.npx jest src/mcp-server/tool-annotations.spec.ts— 44/44, including the new read-only case forwebhook_recent_events.npx jest src/connectors src/mcp-server src/health src/common --maxWorkers=2— 159/163 suites, 3012/3029 tests passed (4 skipped suites, 17 skipped tests, all pre-existing and unrelated).node scripts/generate-llms-txt.mjs --check(repo root) — exit 0.Manual: ran the real backend against a real Postgres instance, through the actual HTTP API (not direct DB writes) — logged in, created a connector with
webhookEnabled/webhookProvider: stripe/webhookSecretviaPOST /api/connectorsand confirmedwebhookSecretis absent from the response, confirmed the builtin tool was auto-created, calledGET .../tools/:toolId/annotationsand confirmedeffective.readOnlyHint: true, and sent real Stripe-signed deliveries toPOST /api/webhooks/:connectorId— valid (200, stored correctly), bad signature (401), unknown connector (401, identical to bad signature), missing signature (401), wrong method (404), and an exact replay of the same event id (200, no error, confirmed via direct DB query that only one row exists). All dry-run rows were cleaned up afterward.Checklist
docs/operations/observability.md, regeneratedllms-full.txt)Related Issues
Relates to #988. No separate issue was filed for this slice: the scope (Stripe-only, this endpoint + this tool) was agreed on directly in the #988 discussion, where it was assigned to me.
Cloud vs self-hosted
Same code path on both; no branch anywhere in this PR checks
DEPLOYMENT_MODE. Ingestion, verification, storage, and retention all behave identically.Known follow-ups (left out of this PR on purpose)
webhookSecretcan't leak through that path. Happy to add it here or as a follow-up, whichever you'd prefer.