From c2b0f6c6349915add4caa287ddba9f795899b327 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Sun, 15 Nov 2026 18:26:07 +0000 Subject: [PATCH 1/5] External services can now call a plugin at `POST /api/plugins//hooks/`. ProPR checks the request's signature or token before any plugin code runs, then runs the plugin's handler from a queue. Everything I ran passes: 19 server-side test files including the new ones, 58 UI component tests and 5 Playwright tests. Typecheck, the zero-warning lint and the OpenAPI check are clean too. I didn't run the full repository suite or a docs site build. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manifest. A webhook is now declared as `{ name, description, handler, verification, deliveryIdHeader?, respond? }`. `verification` is one of: - `hmac-sha256`: a header holding an optional prefix plus the HMAC of the raw body, in hex or base64. - `slack-v0`: Slack's signature, with the timestamp required to be within 5 minutes. - `token-header`: a header holding an optional prefix plus the secret, compared in constant time. Each scheme uses one of the plugin's signing-secret connections. This replaces the old placeholder `{ name, description, connection }`, so I updated the two tests that used it. What the route does with a request: - 404: an unknown or regenerated key, a disabled or uninstalled plugin, or a webhook name the manifest doesn't declare. Nothing is loaded. - 401: a missing or wrong signature, a stale Slack timestamp, or no signing secret set. It is counted per installation and logged with only the plugin, webhook and reason. - 409: a delivery id that was already received (a replay). - 413: a body over 256 KiB. - 429: the same rate limiter `/webhook` uses, applied before the body is read. - 202: verified and queued. The handler then runs as a delivery of kind `webhook`, shown in Activity, with the same retry schedule as event deliveries. Challenge responses. `respond` names another function in `handlers.ts`. It runs after verification and gets only the frozen request, never `ctx`, and it must return synchronously. Slack's `url_verification` echo works this way. Installation key. It is stored in a new `plugin_webhook_keys` table. Regenerate replaces it in one step, so the old URLs answer 404 at once, and it is recorded in the audit trail. Uninstalling deletes the key, so a reinstall gets a new URL. Two new admin routes list the URLs and regenerate the key; I regenerated the OpenAPI spec and client types. Plugins page. The Connect step now has a Webhook URLs section: one URL per webhook with Copy, the count of rejected requests, and Regenerate behind a confirmation. Preview screenshots (desktop, the confirmation, and phone) are attached separately. Behaviour you might not expect: - Request bodies are never written anywhere, so the queue lives only in the API process's memory. A webhook delivery still waiting when the API stops is marked failed on the next start, and a failed webhook delivery has no Retry button; the sending service has to send it again. - Webhook failures don't count towards the plugin's Degraded or Paused health. While event deliveries are paused, new webhook requests get 503. - A webhook's signing secret isn't required to enable the plugin. Until it is set, requests get 401. Acceptance criteria - AC1: met — `pluginWebhookReceiver.ts`, `pluginWebhookRoutes.ts`; `packages/core/test/pluginWebhooks.test.ts` ("a verified request runs its handler once; … run no plugin code") checks the handler runs once and no plugin module loads for a bad signature, stale timestamp or replay; `packages/api/test/pluginWebhookRoutes.test.ts` covers 413 for oversized bodies and 429 from the rate limiter. - AC2: met — `pluginWebhooks.test.ts` uses GitHub's documented vector and RFC 4231 test case 2 for `hmac-sha256` (hex and base64), Slack's documented request for `slack-v0`, and GitLab-style and bearer vectors for `token-header`, each with tampered cases. - AC3: met — `pluginWebhooks.test.ts` ("the synchronous challenge response …"): the challenge is echoed with no handler run, and a respond function that reaches for `ctx` gets one frozen argument, fails with 500, and changes nothing. The route test also checks the echo over HTTP. - AC4: met — `pluginWebhooks.test.ts` and `pluginWebhookRoutes.test.ts`: disabled, uninstalled and reinstalled plugins answer 404, and after Regenerate the old URL answers 404 while the new one works. - AC5: met — sentinel tests in `pluginWebhooks.test.ts` and `pluginWebhookRoutes.test.ts` dump every table and capture every log and console line. I also added a deliberate body log, and the test caught it. - AC6: met — `propr-ui/src/pages/PluginsPage/PluginWebhookUrls.tsx` in `PluginConnections.tsx`; `PluginWebhookUrls.test.tsx`, `propr-ui/e2e/plugin-webhooks.pw.ts`. - AC7: met — `docs/docs/features/plugins.md` (Inbound webhooks: the URL, verification schemes, delivery, challenge responses, routes) and `docs/docs/architecture/plugin-runtime.md` (Inbound webhooks: key, request path, schemes table, respond function, delivery runner, what is stored). --- docs/docs/architecture/plugin-runtime.md | 130 ++++++++- docs/docs/features/plugins.md | 167 ++++++++++- .../operations/configuration-reference.md | 2 +- docs/static/openapi/propr-api.yaml | 271 +++++++++++++++++- packages/api/openapi/directRoutes.ts | 2 + packages/api/openapi/registeredRoutes.ts | 2 +- packages/api/openapi/routeDocsPlugins.ts | 29 +- packages/api/openapi/schemasPlugins.ts | 25 +- packages/api/pluginRouteRegistry.ts | 7 +- packages/api/routes/pluginWebhookRoutes.ts | 71 +++++ packages/api/server.ts | 13 +- .../api/test/pluginActivityRoutes.test.ts | 6 +- packages/api/test/pluginRoutes.test.ts | 6 +- packages/api/test/pluginWebhookRoutes.test.ts | 183 ++++++++++++ packages/client/src/generated/apiTypes.ts | 48 +++- .../20261116000000_plugin_webhook_keys.js | 30 ++ packages/core/src/index.ts | 3 + packages/core/src/plugins/pluginStore.ts | 2 + .../core/src/plugins/pluginWebhookReceiver.ts | 125 ++++++++ .../core/src/plugins/pluginWebhookRunner.ts | 167 +++++++++++ packages/core/src/plugins/pluginWebhooks.ts | 213 ++++++++++++++ .../plugins/fixture-webhooks/handlers.ts | 45 +++ .../plugins/fixture-webhooks/manifest.json | 34 +++ .../core/test/fixtures/plugins/registry.json | 3 +- .../core/test/helpers/pluginHandlerFixture.ts | 3 +- .../core/test/pluginContributions.test.ts | 3 +- packages/core/test/pluginStore.test.ts | 5 +- packages/core/test/pluginWebhooks.test.ts | 257 +++++++++++++++++ packages/shared/src/pluginDeliveries.ts | 15 +- packages/shared/src/pluginInstallations.ts | 2 +- packages/shared/src/pluginSdk.ts | 33 ++- packages/shared/src/plugins.ts | 53 +++- packages/shared/test/plugins.test.ts | 22 +- propr-ui/e2e/plugin-webhooks.pw.ts | 86 ++++++ propr-ui/src/api/pluginsApi.ts | 7 + .../pages/PluginsPage/PluginActivity.test.tsx | 11 + .../src/pages/PluginsPage/PluginActivity.tsx | 14 +- .../pages/PluginsPage/PluginConnections.tsx | 4 +- .../PluginsPage/PluginWebhookUrls.test.tsx | 113 ++++++++ .../pages/PluginsPage/PluginWebhookUrls.tsx | 95 ++++++ .../pages/PluginsPage/usePluginWebhooks.ts | 33 +++ 41 files changed, 2274 insertions(+), 66 deletions(-) create mode 100644 packages/api/routes/pluginWebhookRoutes.ts create mode 100644 packages/api/test/pluginWebhookRoutes.test.ts create mode 100644 packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js create mode 100644 packages/core/src/plugins/pluginWebhookReceiver.ts create mode 100644 packages/core/src/plugins/pluginWebhookRunner.ts create mode 100644 packages/core/src/plugins/pluginWebhooks.ts create mode 100644 packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts create mode 100644 packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json create mode 100644 packages/core/test/pluginWebhooks.test.ts create mode 100644 propr-ui/e2e/plugin-webhooks.pw.ts create mode 100644 propr-ui/src/pages/PluginsPage/PluginWebhookUrls.test.tsx create mode 100644 propr-ui/src/pages/PluginsPage/PluginWebhookUrls.tsx create mode 100644 propr-ui/src/pages/PluginsPage/usePluginWebhooks.ts diff --git a/docs/docs/architecture/plugin-runtime.md b/docs/docs/architecture/plugin-runtime.md index e501539e5..4c9a5efa6 100644 --- a/docs/docs/architecture/plugin-runtime.md +++ b/docs/docs/architecture/plugin-runtime.md @@ -35,6 +35,10 @@ Ordinary task runs stay one-shot. | Event dispatcher | `packages/core/src/plugins/pluginDispatcher.ts`, started by the daemon (`src/daemon/pluginEventDispatch.ts`) | Reads the journal per installation and repository, runs subscribed handlers, retries, holds the lease. | | Delivery records | `packages/core/src/plugins/pluginDeliveries.ts`, tables `plugin_dispatch`, `plugin_delivery_cursors`, `plugin_deliveries` | Activity records, health, Retry, Resume, retention, the Degraded notification. | | Activity API | `packages/api/routes/pluginActivityRoutes.ts` | `GET …/:pluginId/activity`, `POST …/activity/:id/retry`, `POST …/:pluginId/resume`. | +| Webhook verification and keys | `packages/core/src/plugins/pluginWebhooks.ts`, table `plugin_webhook_keys` | The verification schemes, the installation key, its lookup and regeneration, the rejected-request count. | +| Webhook receiver | `packages/core/src/plugins/pluginWebhookReceiver.ts` | One inbound request: lookup, verification, the `respond` function, de-duplication, enqueue. | +| Webhook delivery runner | `packages/core/src/plugins/pluginWebhookRunner.ts`, started by the API (`packages/api/server.ts`) | Runs queued webhook deliveries in memory, with the event retry schedule, and records them in `plugin_deliveries`. | +| Webhook routes | `packages/api/routes/pluginWebhookRoutes.ts` | `POST /api/plugins/:installationKey/hooks/:webhookName` (public), `GET …/:pluginId/webhooks`, `POST …/:pluginId/webhooks/regenerate`. | ## Invocation @@ -65,8 +69,9 @@ ignores `ctx.signal` can keep its own timers alive, but it can no longer act through ProPR. The [event dispatcher](#event-dispatcher) calls the runner with -`PluginEventInput`. Later issues call it from the inbound webhook route -(`PluginWebhookInput`) and commands (`PluginCommandInput`). +`PluginEventInput`, and the [webhook delivery runner](#inbound-webhooks) calls +it with `PluginWebhookInput`. A later issue calls it from commands +(`PluginCommandInput`). `PluginInvokeInput` carries a JSON payload for direct calls and tests. ## Event dispatcher @@ -195,6 +200,127 @@ event is still in the journal and ahead of the repository's cursor (another handler of the event is retrying, or deliveries are paused), so reading the event again does not run a handler that already succeeded or failed for good. +## Inbound webhooks + +A plugin declares webhooks in `contributes.webhooks`. Each one is served at + +```text +POST /api/plugins//hooks/ +``` + +### Installation key + +`plugin_webhook_keys` has one row per installation. It holds the key (32 random +bytes, base64url, 43 characters), its SHA-256 (`key_hash`, unique, which +requests are looked up by), the `installed_at` of the installation it belongs +to, who created it and when, and the count, time and reason of the last +rejected request. The row is created the first time the Plugins page reads the +URLs. **Regenerate** replaces it in one transaction with an audit entry +(`regenerate_webhook_key`), so the old key matches nothing from then on. +Uninstalling deletes the row. A row from an earlier installation never matches, +because `installed_at` must equal the current installation's. + +A request resolves to a webhook only when all of these hold: the key's hash is +in the table, the installation exists, is enabled and is the one the key +belongs to, the plugin is in the registry, and its manifest declares the +webhook name. Anything else gets 404 before a secret is read or plugin code is +loaded. + +### Request path + +The route is registered in `packages/api/server.ts` next to `POST /webhook`, +before the JSON parser and the session middleware: + +1. The webhook rate limiter (`createWebhookRequestRateLimiter`, the one + `/webhook` uses), on top of the general `/api` limiter. +2. `express.raw` for any content type, up to `PLUGIN_WEBHOOK_LIMITS.bodyBytes` + (256 KiB). A larger body gets 413 and is neither echoed nor logged. +3. `PluginWebhookReceiver.receive`: + 1. Resolve the key and name, as described above (404). + 2. Open the webhook's `signing_secret` connection and run + `verifyPluginWebhook`. A failure gets 401 and is counted on the key row + (`recordPluginWebhookRejection`). It is logged as + `{ pluginId, webhook, reason }`, never with the body or a header. + 3. If the webhook has `respond`, call it (see below). A response is sent as + it is and the request ends there. + 4. Derive the delivery id: `whk--`, or a + random UUID in place of the value when the webhook declares no header or + the request does not send it. A malformed value gets 400. + 5. Refuse with 503 while the installation's event deliveries are paused. + 6. `PluginWebhookRunner.accept` inserts the `plugin_deliveries` row (kind + `webhook`, `event_type` `webhook.`, empty repository, `event_seq` 0). + The unique `(plugin_id, delivery_id)` index turns a replay into a 409. + When 1,000 deliveries are already held, the answer is 503. Otherwise it + answers 202. + +| Scheme | Computation | Notes | +| --- | --- | --- | +| `hmac-sha256` | `prefix + enc(HMAC-SHA256(secret, rawBody))`, `enc` hex (case-insensitive) or base64 | The digest is decoded and compared with `timingSafeEqual`. A repeated header is treated as missing. | +| `slack-v0` | `"v0=" + hex(HMAC-SHA256(secret, "v0:" + timestamp + ":" + rawBody))` | `X-Slack-Request-Timestamp` must be within `PLUGIN_WEBHOOK_LIMITS.timestampToleranceSeconds` (300) of now, in either direction. | +| `token-header` | `header == prefix + secret` | The SHA-256 digests of both sides are compared with `timingSafeEqual`, so neither the length nor the content leaks through timing. | + +The test vectors are GitHub's documented delivery (`It's a Secret to +Everybody`, `Hello, World!`), RFC 4231 test case 2 (`Jefe`), and Slack's +documented slash command request. They are in +`packages/core/test/pluginWebhooks.test.ts`. + +### The `respond` function + +`respond` names a function exported from `handlers.ts`, of type +`PluginWebhookResponder`. The receiver loads the handlers module and calls the +function with `Reflect.apply`, passing exactly one argument: the frozen request +`{ webhook, headers, body, receivedAt }`, whose headers are frozen too. No +`ctx` is built for it. It therefore cannot reach ProPR, outbound HTTP, state +or the log, and the tests check that a function that reaches for a second +argument fails. + +A returned thenable is refused, so the function must be synchronous. The +response is checked: a 2xx status, `text/plain` or `application/json`, and a +string body of at most `PLUGIN_WEBHOOK_LIMITS.responseBytes` (4 KiB). A refusal +or a throw gets 500. The error is logged by name only, because its message +could quote the request. + +### Webhook delivery runner + +Request bodies must never be stored, so webhook deliveries do not go through +the journal or Redis. `PluginWebhookRunner` holds each accepted delivery (the +request body, the filtered headers and the target installation) in the API +process's memory, keyed by its `plugin_deliveries` id. It then: + +1. Re-reads the installation before each attempt. A disabled, uninstalled or + reinstalled plugin fails the delivery (`Not delivered: the plugin was + disabled`). A record another writer already finished is dropped. Enabling + the plugin again runs `restartPluginDispatch`, which marks every unfinished + delivery failed, and that includes webhook deliveries. +2. Marks the record `running` only when it is still in the status it was read + in, runs the handler with `{ kind: 'webhook', …, deliveryId, attempt }`, and + records the outcome only while the record is still that attempt. +3. Retries on `PLUGIN_DELIVERY_RETRY_DELAYS_MS` (1 min, 5 min, 30 min, 2 h). + After the last delay the delivery is failed and its memory is released. + +The handler's headers are the request's minus the verification headers, +`authorization`, `proxy-authorization`, `cookie`, `host`, `connection`, +`content-length` and `transfer-encoding`. + +When the API starts, `recover()` marks webhook deliveries that an earlier +process left unfinished as failed: *ProPR stopped before the delivery finished, +and webhook requests are not kept*. `stop()` (on SIGTERM) lets attempts under +way finish and fails the deliveries still waiting. A failed webhook delivery +cannot be retried from Activity (`retryPluginDelivery` only accepts event +deliveries), because the request is gone. + +Webhook attempts do not update `plugin_dispatch` health. The dispatcher's +failure count, Degraded state and pause track event deliveries only. A paused +installation refuses new webhook requests with 503. + +**What is stored.** The key row (the key, its hash and the rejection count) and +the delivery record (the ids, the webhook and handler names, status, timing, +and the handler's redacted error and log lines). The body, the signature and +token headers, and the other request headers are never written to the database +or a log. The sentinel tests in `pluginWebhooks.test.ts` and +`packages/api/test/pluginWebhookRoutes.test.ts` dump every table and capture +every logger and console line to check this. + ## `ctx.propr`: the scoped client Each operation needs one permission: diff --git a/docs/docs/features/plugins.md b/docs/docs/features/plugins.md index c1b361812..8aa43faf6 100644 --- a/docs/docs/features/plugins.md +++ b/docs/docs/features/plugins.md @@ -15,8 +15,8 @@ integrity check as the [extension library's local package servers](./agent-exten :::note Work in progress This page describes the plugin **manifest**, the official plugin registry, the [Plugins page](#the-plugins-page), the installation API and the -[handler interface](#handlers) and [event subscriptions](#event-subscriptions). -Inbound webhooks and commands are added by later releases. +[handler interface](#handlers), [event subscriptions](#event-subscriptions) +and [inbound webhooks](#inbound-webhooks). Commands are added by a later release. ::: ## Where plugins live @@ -110,7 +110,7 @@ defined. | `automationTemplates` | `{ "id", "path" }`: a template folder in the automation template format. It must pass the same checks as the shipped templates. | | `commands` | `{ "name", "description", "permissions" }`. Accepted and validated; not used yet. | | `subscriptions` | `{ "on", "handler", "filter"? }`: deliver a repository event (`.`, such as `pull_request.merged`) to an exported handler. See [Event subscriptions](#event-subscriptions). Needs the `events.read` permission. | -| `webhooks` | `{ "name", "description", "connection" }`, where the connection is a `signing_secret`. Accepted and validated; not used yet. | +| `webhooks` | `{ "name", "description", "handler", "verification", "deliveryIdHeader"?, "respond"? }`: an inbound webhook URL whose verified requests run `handler`. `verification` is a built-in scheme with a `signing_secret` connection. See [Inbound webhooks](#inbound-webhooks). | Names are unique within each list, and command names are unique across all plugins. @@ -545,8 +545,9 @@ The input is one of the envelopes in `@propr/shared`: `{ kind: 'event', event }` `{ kind: 'command', command, args, repository, actor }` or `{ kind: 'invoke', payload }`. Events are delivered by [event subscriptions](#event-subscriptions), with a stable `deliveryId` and the -`attempt` number. Delivering webhooks and commands to handlers is added by later -releases. +`attempt` number. Webhook requests are delivered by +[inbound webhooks](#inbound-webhooks), also with a `deliveryId` and `attempt`. +Delivering commands to handlers is added by a later release. A call `ctx` refuses throws a `PluginSdkError` with a `code` such as `permission_denied`, `repository_not_allowed`, `host_not_allowed`, @@ -748,3 +749,159 @@ The same records are available to administrators through the API: Mutations need the same `X-ProPR-Plugins: 1` JSON contract as the other plugin routes, and Retry and Resume are recorded in the plugin's audit trail. + +## Inbound webhooks + +A plugin can receive requests from an external service: a chat mention, an +issue tracker update, an error alert. Each webhook in `contributes.webhooks` +gets its own URL. ProPR verifies every request with the webhook's scheme before +any plugin code runs, then runs the webhook's `handler` from the delivery queue. + +```json +"webhooks": [ + { + "name": "events", + "description": "Issue tracker updates", + "handler": "onTrackerEvent", + "verification": { "scheme": "hmac-sha256", "header": "X-Hub-Signature-256", "prefix": "sha256=", "connection": "tracker-signing" }, + "deliveryIdHeader": "X-GitHub-Delivery" + }, + { + "name": "slack", + "description": "Slack Events API", + "handler": "onSlackEvent", + "respond": "slackChallenge", + "verification": { "scheme": "slack-v0", "connection": "slack-signing" } + } +] +``` + +### The webhook URL + +```text +POST https:///api/plugins//hooks/ +``` + +The installation key is 43 random characters, created for each installation. +Copy the URLs from the plugin's **Connect** step on the Plugins page, under +**Webhook URLs**. Each one has a Copy button. When the instance has no public +base URL, only the path is shown. Prefix it with the address services use to +reach ProPR. + +Treat the URL as a secret, although it is not the only check. **Regenerate** +replaces the key in every URL at once: the old URLs answer 404 straight away, +so give each service its new URL afterwards. Regenerating is recorded in the +plugin's audit trail. Uninstalling deletes the key, and a reinstall gets a new +one. + +Requests for a disabled or uninstalled plugin, or for a webhook name the +manifest does not declare, get 404 and run nothing. Enabling the plugin again +brings back the same URLs. + +### Verification schemes + +Each scheme checks the request against the value of a `signing_secret` +connection, set in the Connect step. Header names are matched without regard to +letter case. + +| Scheme | Fields | What ProPR checks | +| --- | --- | --- | +| `hmac-sha256` | `header`, `prefix`?, `encoding`? (`hex`, the default, or `base64`), `connection` | `header` equals `prefix` followed by the HMAC-SHA256 of the raw body, keyed with the secret. GitHub's `X-Hub-Signature-256: sha256=…` is this scheme with `prefix: "sha256="`. | +| `slack-v0` | `connection` | Slack's [signed requests](https://api.slack.com/authentication/verifying-requests-from-slack): `X-Slack-Signature` equals `v0=` followed by the hex HMAC-SHA256 of `v0::`. The timestamp must be within 5 minutes of ProPR's clock, in either direction. | +| `token-header` | `header`, `prefix`?, `connection` | `header` equals `prefix` followed by the secret itself, as GitLab's `X-Gitlab-Token` does. Use `"header": "authorization", "prefix": "Bearer "` for a bearer token. | + +Signatures and tokens are compared in constant time. A request that fails +verification gets **401** and runs nothing. This also covers a missing or wrong +signature, a stale Slack timestamp, and a webhook whose signing secret is not +set. ProPR counts each one on the installation, and the Webhook URLs section +shows the count, the time of the last one and its reason. The rejection is +logged with the plugin, the webhook and the reason only. + +The tests use the published test vectors: GitHub's +([validating deliveries](https://docs.github.com/en/webhooks/using-webhooks/validating-webhook-deliveries)), +RFC 4231 test case 2 for plain HMAC-SHA256, and Slack's documented request. +They are in `packages/core/test/pluginWebhooks.test.ts`. + +### Delivery + +After a request is verified, ProPR answers **202** at once with +`{ "accepted": true, "deliveryId" }`. The handler then runs from the queue as a +delivery of kind `webhook`, with the same 30 second limit and the same retry +schedule as event deliveries: 1 minute, 5 minutes, 30 minutes and 2 hours, then +failed. The handler receives: + +```ts +{ kind: 'webhook', webhook, headers, body, receivedAt, deliveryId, attempt } +``` + +- `body` is the raw body as text, whatever its content type (JSON, + form-encoded or plain text). +- `headers` are the request's headers with lower-case names. ProPR removes the + verification headers, `Authorization`, `Cookie` and the hop-by-hop headers + first. +- `deliveryId` is `whk--` and is the same on every attempt. When + the webhook declares `deliveryIdHeader` and the service sends it, `` is + that value. Otherwise ProPR assigns one. + +Other limits and refusals: + +| Situation | Answer | +| --- | --- | +| Body larger than 256 KiB | **413**. Nothing runs. | +| A `deliveryIdHeader` value already received | **409** (a replay). Nothing runs. Delivery ids are remembered for as long as Activity records are kept (14 days). | +| A `deliveryIdHeader` value that is not 1 to 200 printable characters | **400**. | +| Event deliveries of the plugin are [paused](#health-degraded-and-resume) | **503** with `Retry-After`. | +| 1,000 webhook deliveries already waiting | **503** with `Retry-After`. | +| More requests than the webhook rate limit allows | **429**. This is the limiter `POST /webhook` uses: `PROPR_WEBHOOK_RATE_LIMIT_MAX` requests per `PROPR_WEBHOOK_RATE_LIMIT_WINDOW_MS`, 300 per minute by default. | + +Each webhook delivery appears in the plugin's [Activity](#activity) as +`webhook.`, with its handler, outcome, attempts, error and log lines. +ProPR never stores or logs request bodies, signature headers or tokens. The +request is held in the API process's memory only until its delivery is final. +Two things follow from that: + +- A failed webhook delivery has no **Retry** button. The service has to send it + again. +- A delivery still waiting when the API stops is marked failed with + *ProPR stopped before the delivery finished*. Most services redeliver on + their own. + +Webhook failures do not count towards the plugin's Degraded and paused +[health](#health-degraded-and-resume), which tracks event deliveries. + +### Challenge responses + +Some services check a new URL by sending a challenge that must be echoed in the +response, such as Slack's `url_verification`. `respond` names another exported +function of `handlers.ts`. It runs after verification and before the request is +queued, and it receives only the frozen request +`{ webhook, headers, body, receivedAt }`. It gets no `ctx`, so it cannot read +state, call ProPR or send HTTP requests. It must return synchronously: + +```ts +import { definePluginWebhookResponder } from '@propr/shared'; + +export const slackChallenge = definePluginWebhookResponder(request => { + const body = JSON.parse(request.body) as { type?: string; challenge?: string }; + return body.type === 'url_verification' && body.challenge + ? { contentType: 'text/plain', body: body.challenge } + : null; +}); +``` + +When it returns a response (a 2xx `status`, 200 by default, a `text/plain` or +`application/json` `contentType` and a `body` of at most 4 KiB), ProPR sends +that response and the handler does not run. When it returns `null`, the request +is queued as usual. If it throws, returns a promise or returns an invalid +response, ProPR answers **500** and runs nothing. + +### Webhook URL routes + +| Route | What it does | +| --- | --- | +| `GET /api/instance/plugins/v1/:pluginId/webhooks` | The webhook URLs of an installed plugin and its count of rejected requests (409 when not installed). | +| `POST /api/instance/plugins/v1/:pluginId/webhooks/regenerate` | Replace the installation key. The old URLs stop working at once. | +| `POST /api/plugins/:installationKey/hooks/:webhookName` | The public webhook endpoint described above. | + +The first two need a durable instance administrator, like the other plugin +routes, and Regenerate needs the `X-ProPR-Plugins: 1` JSON contract. diff --git a/docs/docs/operations/configuration-reference.md b/docs/docs/operations/configuration-reference.md index b20499e2e..33fd10740 100644 --- a/docs/docs/operations/configuration-reference.md +++ b/docs/docs/operations/configuration-reference.md @@ -54,7 +54,7 @@ The backend authenticates to GitHub in one of three modes — `demo`, `relay`, o | `PROPR_API_RATE_LIMIT_MAX` / `PROPR_API_RATE_LIMIT_WINDOW_MS` | `600` / `60000` | Per-client quota and window (milliseconds) for all `/api` requests. | Optional tuning. | | `PROPR_AUTH_RATE_LIMIT_MAX` / `PROPR_AUTH_RATE_LIMIT_WINDOW_MS` | `30` / `900000` | Additional, tighter per-client quota for OAuth initiation and callback endpoints. | Optional tuning. | | `PROPR_OPERATOR_RATE_LIMIT_MAX` / `PROPR_OPERATOR_RATE_LIMIT_WINDOW_MS` | `30` / `60000` | Per-client quota for authorized operator recovery endpoints (`/api/admin/followup-handoffs`), applied after the `instance.manage_runtime` guard. | Optional tuning. | -| `PROPR_WEBHOOK_RATE_LIMIT_MAX` / `PROPR_WEBHOOK_RATE_LIMIT_WINDOW_MS` | `300` / `60000` | Per-client quota for direct webhook requests, applied before body parsing and signature verification. | Optional tuning in direct-webhook mode. | +| `PROPR_WEBHOOK_RATE_LIMIT_MAX` / `PROPR_WEBHOOK_RATE_LIMIT_WINDOW_MS` | `300` / `60000` | Per-client quota for direct webhook requests and plugin webhook requests (`/api/plugins/…/hooks/…`), applied before body parsing and signature verification. | Optional tuning in direct-webhook mode or for plugins with webhooks. | | `PROPR_TRUSTED_PROXY_PEERS` | Unset; launcher-managed tunnel: reserved `self` mode | Comma-separated immediate proxy IPs, CIDRs, or `proxy-addr` names whose forwarded client IP and protocol are trusted. Unset ignores forwarding headers. The launcher injects `self` only for its managed sidecar sharing the API network namespace. Its broad `uniquelocal` name is accepted only when `API_PORT` is explicitly loopback-bound. | Reverse-proxy deployments; injected automatically for the managed tunnel. | | `PROPR_DESKTOP_TOKEN_TTL_DAYS` | Unset (no expiry) | Lifetime of newly paired desktop instance tokens, 1–3650 days. See [Desktop Pairing](./desktop-pairing.md). | Optional. | | `PROPR_DISCOVERY_RATE_LIMIT_MAX` / `PROPR_PAIRING_START_RATE_LIMIT_MAX` / `PROPR_PAIRING_POLL_RATE_LIMIT_MAX` | `60` per minute / `10` per 15 minutes / `180` per 15 minutes | Per-client quotas for desktop discovery, pairing start, and pairing polls. Matching `*_WINDOW_MS` variables set the windows. | Optional tuning. | diff --git a/docs/static/openapi/propr-api.yaml b/docs/static/openapi/propr-api.yaml index d927f1f56..5490b9a05 100644 --- a/docs/static/openapi/propr-api.yaml +++ b/docs/static/openapi/propr-api.yaml @@ -29,8 +29,8 @@ info: name: Apache-2.0 identifier: Apache-2.0 x-route-coverage: - total: 342 - documented: 104 + total: 345 + documented: 107 undocumented: 238 servers: - url: "{origin}" @@ -5593,6 +5593,90 @@ paths: x-propr-auth: member x-propr-permission: instance.manage_settings x-legacy-error: true + /api/instance/plugins/v1/{pluginId}/webhooks: + get: + operationId: getPluginWebhooks + summary: Read the installed plugin's inbound webhook URLs (one per declared webhook, with its verification scheme) and the count of requests refused because they failed verification. Refused (409) when the plugin is not installed. + description: |- + Durable instance administrator with instance.manage_settings required. Mutations require application/json and X-ProPR-Plugins: 1, and are recorded in the plugin audit trail. Connection secrets are never returned. + + Requires the `instance.manage_settings` instance permission. + tags: + - Configuration + security: + - sessionCookie: [] + - bearerAuth: [] + parameters: + - name: pluginId + in: path + required: true + description: Plugin id, for example `example-pack`. + schema: + type: string + responses: + "200": + description: The webhook URLs. + content: + application/json: + schema: + $ref: "#/components/schemas/PluginWebhooksResponse" + "401": + $ref: "#/components/responses/LegacyUnauthorized" + "403": + $ref: "#/components/responses/LegacyForbidden" + default: + $ref: "#/components/responses/LegacyUnexpectedError" + x-propr-auth: member + x-propr-permission: instance.manage_settings + x-legacy-error: true + /api/instance/plugins/v1/{pluginId}/webhooks/regenerate: + post: + operationId: regeneratePluginWebhookKey + summary: "Replace the plugin's installation key: every webhook URL changes and the old URLs answer 404 at once. Update the URL at each sending service afterwards." + description: |- + Durable instance administrator with instance.manage_settings required. Mutations require application/json and X-ProPR-Plugins: 1, and are recorded in the plugin audit trail. Connection secrets are never returned. + + Requires the `instance.manage_settings` instance permission. + tags: + - Configuration + security: + - sessionCookie: [] + - bearerAuth: [] + parameters: + - name: pluginId + in: path + required: true + description: Plugin id, for example `example-pack`. + schema: + type: string + - name: X-ProPR-Plugins + in: header + required: true + description: Must equal 1; browser calls require an authorized CORS preflight. + schema: + type: string + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/PluginEmptyRequest" + responses: + "200": + description: The new webhook URLs. + content: + application/json: + schema: + $ref: "#/components/schemas/PluginWebhooksResponse" + "401": + $ref: "#/components/responses/LegacyUnauthorized" + "403": + $ref: "#/components/responses/LegacyForbidden" + default: + $ref: "#/components/responses/LegacyUnexpectedError" + x-propr-auth: member + x-propr-permission: instance.manage_settings + x-legacy-error: true /api/instance/storage/v1/capabilities: get: operationId: getInstanceStorageCapabilities @@ -7566,6 +7650,93 @@ paths: x-propr-auth: member x-undocumented: true x-legacy-error: true + /api/plugins/{installationKey}/hooks/{webhookName}: + post: + operationId: receivePluginWebhook + summary: Receive an inbound webhook request for a plugin + description: Public. ProPR verifies the request with the webhook's scheme (`hmac-sha256`, `slack-v0` or `token-header`) and the plugin's signing secret before any plugin code runs, then queues the plugin's handler as a delivery of kind `webhook`. The body is read raw, up to 256 KiB; the webhook rate limiter applies. Bodies and signature headers are never stored or logged. + tags: + - Webhooks + security: [] + parameters: + - name: installationKey + in: path + required: true + description: The unguessable installation key from the Plugins page. + schema: + type: string + - name: webhookName + in: path + required: true + description: A webhook name from the plugin manifest. + schema: + type: string + requestBody: + required: true + description: "Any body, read raw: JSON, form-encoded or text, as the sending service signs it." + content: + application/json: + schema: + $ref: "#/components/schemas/JsonObject" + responses: + "200": + description: The webhook's respond function answered at once (a challenge echo); the handler does not run. + content: + text/plain: + schema: + type: string + "202": + description: "Verified and queued: `{ accepted: true, deliveryId }`." + content: + application/json: + schema: + $ref: "#/components/schemas/JsonObject" + "400": + description: The delivery id header is malformed. + content: + application/json: + schema: + $ref: "#/components/schemas/LegacyError" + "401": + description: "Verification failed: missing or wrong signature or token, a stale timestamp, or no signing secret set. Counted on the installation." + content: + application/json: + schema: + $ref: "#/components/schemas/LegacyError" + "404": + description: Unknown or regenerated installation key, a disabled or uninstalled plugin, or an undeclared webhook. + content: + application/json: + schema: + $ref: "#/components/schemas/LegacyError" + "409": + description: The delivery id was already received (a replay). + content: + application/json: + schema: + $ref: "#/components/schemas/LegacyError" + "413": + description: The body is larger than 256 KiB. + content: + application/json: + schema: + $ref: "#/components/schemas/LegacyError" + "429": + description: Rate limited. + content: + application/json: + schema: + $ref: "#/components/schemas/LegacyError" + "503": + description: Deliveries of the plugin are paused, or too many deliveries are waiting; retry after `Retry-After`. + content: + application/json: + schema: + $ref: "#/components/schemas/LegacyError" + default: + $ref: "#/components/responses/LegacyUnexpectedError" + x-propr-auth: public + x-legacy-error: true /api/preview-media/comments/{owner}/{repo}/{number}/{assetId}: get: operationId: getApiPreviewMediaCommentsOwnerRepoNumberAssetId @@ -13129,20 +13300,22 @@ components: description: The activity record id, used by the retry route. deliveryId: type: string - description: The stable delivery id the handler receives (`evt--`); the same on every attempt. + description: The stable delivery id the handler receives (`evt--`, or `whk--` for a webhook); the same on every attempt. kind: type: string enum: - event - gap - description: "`event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered." + - webhook + description: "`event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered. `webhook`: one verified inbound webhook request to its handler." repository: type: string + description: The event's repository; empty for a webhook delivery. eventSeq: type: integer - description: The journal `seq` of the event; for a gap, the first `seq` that may be missing. + description: The journal `seq` of the event; for a gap, the first `seq` that may be missing; 0 for a webhook delivery. eventType: - description: "`.`; null for a gap." + description: "`.`; `webhook.` for a webhook delivery; null for a gap." type: - string - "null" @@ -13400,9 +13573,17 @@ components: type: string description: type: string + scheme: + type: string + enum: + - hmac-sha256 + - slack-v0 + - token-header + description: How ProPR verifies the webhook's requests. required: - name - description + - scheme additionalProperties: false required: - mcpServers @@ -13751,6 +13932,7 @@ components: - handler_refused - retry_delivery - resume_deliveries + - regenerate_webhook_key actor: type: string at: @@ -14116,9 +14298,17 @@ components: type: string description: type: string + scheme: + type: string + enum: + - hmac-sha256 + - slack-v0 + - token-header + description: How ProPR verifies the webhook's requests. required: - name - description + - scheme additionalProperties: false required: - mcpServers @@ -14453,6 +14643,75 @@ components: - enableBlockers additionalProperties: false description: A plugin with its installation state on this instance. + PluginWebhooksResponse: + type: object + properties: + webhooks: + type: array + items: + type: object + properties: + name: + type: string + description: + type: string + scheme: + type: string + enum: + - hmac-sha256 + - slack-v0 + - token-header + path: + type: string + description: "`/api/plugins//hooks/`. The key is unguessable: treat the URL as a secret." + url: + description: The full URL; null when the instance has no public base URL. + type: + - string + - "null" + required: + - name + - description + - scheme + - path + - url + additionalProperties: false + description: One URL per webhook the plugin declares. + rejected: + type: object + properties: + count: + type: integer + lastAt: + type: + - number + - "null" + lastReason: + type: + - string + - "null" + required: + - count + - lastAt + - lastReason + additionalProperties: false + description: Requests refused because they failed verification. Their bodies and headers are never stored. + keyCreatedAt: + type: + - number + - "null" + keyCreatedBy: + description: The administrator who last regenerated the key, or `system`. + type: + - string + - "null" + required: + - webhooks + - rejected + - keyCreatedAt + - keyCreatedBy + additionalProperties: false + description: The inbound webhook URLs of an installed plugin. PullRequestReadiness: type: object properties: diff --git a/packages/api/openapi/directRoutes.ts b/packages/api/openapi/directRoutes.ts index 0160fe274..51f401de2 100644 --- a/packages/api/openapi/directRoutes.ts +++ b/packages/api/openapi/directRoutes.ts @@ -34,6 +34,8 @@ export const DIRECT_ROUTE_REGISTRATIONS: readonly RegisteredRoute[] = [ { method: 'get', path: '/api/instance/extensions/v1/oauth/client-metadata.json', auth: 'public', source: 'server.ts' }, { method: 'get', path: '/health', auth: 'public', source: 'server.ts' }, { method: 'post', path: '/webhook', auth: 'webhook', source: 'server.ts' }, + // Plugin webhooks: the unguessable installation key, then the webhook's own verification scheme. + { method: 'post', path: '/api/plugins/:installationKey/hooks/:webhookName', auth: 'public', source: 'server.ts' }, { method: 'get', path: '/api/compatibility', auth: 'public', source: 'server.ts' }, // Agent run containers authenticate with a request signed by SYSTEM_TASK_SECRET. { method: 'post', path: '/api/internal/agent-runs/:runId/mcp-grants', auth: 'public', source: 'server.ts' }, diff --git a/packages/api/openapi/registeredRoutes.ts b/packages/api/openapi/registeredRoutes.ts index 3d753ccce..d0a1acbea 100644 --- a/packages/api/openapi/registeredRoutes.ts +++ b/packages/api/openapi/registeredRoutes.ts @@ -43,7 +43,7 @@ export function listRegistryRouteEntries(): RouteEntry[] { return [ ...createOperationalRouteEntries(placeholderDeps()), ...createStorageRouteEntries(placeholderDeps()), - ...createPluginRouteEntries(placeholderDeps(), placeholderDeps()), + ...createPluginRouteEntries(placeholderDeps(), placeholderDeps(), placeholderDeps()), ...createMemberCatalogRouteEntries(placeholderDeps()), ...createManagementRouteEntries(placeholderDeps()), ]; diff --git a/packages/api/openapi/routeDocsPlugins.ts b/packages/api/openapi/routeDocsPlugins.ts index 37fdccdc3..77bb17cb7 100644 --- a/packages/api/openapi/routeDocsPlugins.ts +++ b/packages/api/openapi/routeDocsPlugins.ts @@ -2,8 +2,9 @@ import type { z } from 'zod'; import { PluginAcknowledgementRequest, PluginConnectionRequest, PluginDetailResponse, PluginEnabledRequest, PluginList, PluginRepositoriesRequest, PluginResponse, PluginSettingsRequest, PluginUninstallRequest, PluginEmptyRequest, PluginOAuthClientRequest, PluginOAuthCompleteRequest, PluginOAuthStartResponse, - PluginActivityResponse, PluginDeliveryResponse, PluginHealthResponse, + PluginActivityResponse, PluginDeliveryResponse, PluginHealthResponse, PluginWebhooksResponse, } from './schemasPlugins.js'; +import { JsonObject } from './schemas.js'; import type { RouteDoc } from './types.js'; /** Plugin installation routes (docs/features/plugins#installing-a-plugin). */ @@ -50,5 +51,31 @@ export const PLUGIN_ROUTE_DOCS: Record = { [`POST ${BASE}/:pluginId/resume`]: mutation('resumePluginDeliveries', 'Resume event deliveries paused after repeated failures: the failure count starts over, Degraded clears and waiting retries are due at once. Refused (409) when deliveries are not paused.', PluginEmptyRequest, { responses: { 200: { description: 'The new delivery health.', schema: PluginHealthResponse } }, }), + [`GET ${BASE}/:pluginId/webhooks`]: { + operationId: 'getPluginWebhooks', summary: 'Read the installed plugin\'s inbound webhook URLs (one per declared webhook, with its verification scheme) and the count of requests refused because they failed verification. Refused (409) when the plugin is not installed.', description, tags: ['Configuration'], + pathParams: pluginId, responses: { 200: { description: 'The webhook URLs.', schema: PluginWebhooksResponse } }, + }, + [`POST ${BASE}/:pluginId/webhooks/regenerate`]: mutation('regeneratePluginWebhookKey', 'Replace the plugin\'s installation key: every webhook URL changes and the old URLs answer 404 at once. Update the URL at each sending service afterwards.', PluginEmptyRequest, { + responses: { 200: { description: 'The new webhook URLs.', schema: PluginWebhooksResponse } }, + }), + 'POST /api/plugins/:installationKey/hooks/:webhookName': { + operationId: 'receivePluginWebhook', + summary: 'Receive an inbound webhook request for a plugin', + description: 'Public. ProPR verifies the request with the webhook\'s scheme (`hmac-sha256`, `slack-v0` or `token-header`) and the plugin\'s signing secret before any plugin code runs, then queues the plugin\'s handler as a delivery of kind `webhook`. The body is read raw, up to 256 KiB; the webhook rate limiter applies. Bodies and signature headers are never stored or logged.', + tags: ['Webhooks'], + pathParams: { installationKey: 'The unguessable installation key from the Plugins page.', webhookName: 'A webhook name from the plugin manifest.' }, + requestBody: { schema: JsonObject, description: 'Any body, read raw: JSON, form-encoded or text, as the sending service signs it.' }, + responses: { + 200: { description: 'The webhook\'s respond function answered at once (a challenge echo); the handler does not run.', contentType: 'text/plain' }, + 202: { description: 'Verified and queued: `{ accepted: true, deliveryId }`.', schema: JsonObject }, + 400: { description: 'The delivery id header is malformed.' }, + 401: { description: 'Verification failed: missing or wrong signature or token, a stale timestamp, or no signing secret set. Counted on the installation.' }, + 404: { description: 'Unknown or regenerated installation key, a disabled or uninstalled plugin, or an undeclared webhook.' }, + 409: { description: 'The delivery id was already received (a replay).' }, + 413: { description: 'The body is larger than 256 KiB.' }, + 429: { description: 'Rate limited.' }, + 503: { description: 'Deliveries of the plugin are paused, or too many deliveries are waiting; retry after `Retry-After`.' }, + }, + }, [`POST ${BASE}/:pluginId/connections/:connectionName/oauth/disconnect`]: mutation('disconnectPluginOAuth', 'Disconnect an `oauth` connection: its managed servers\' tokens are revoked where possible and deleted, and later runs leave the servers out.', PluginEmptyRequest, { pathParams: oauthParams }), }; diff --git a/packages/api/openapi/schemasPlugins.ts b/packages/api/openapi/schemasPlugins.ts index 0418d67d3..aa54e0f45 100644 --- a/packages/api/openapi/schemasPlugins.ts +++ b/packages/api/openapi/schemasPlugins.ts @@ -1,6 +1,6 @@ import { z } from 'zod'; import { - PLUGIN_AUDIT_ACTIONS, PLUGIN_CATEGORIES, PLUGIN_CONNECTION_TYPES, PLUGIN_DELIVERY_KINDS, PLUGIN_DELIVERY_STATUSES, PLUGIN_HEALTH_STATES, PLUGIN_PERMISSIONS, + PLUGIN_AUDIT_ACTIONS, PLUGIN_CATEGORIES, PLUGIN_CONNECTION_TYPES, PLUGIN_DELIVERY_KINDS, PLUGIN_DELIVERY_STATUSES, PLUGIN_HEALTH_STATES, PLUGIN_PERMISSIONS, PLUGIN_WEBHOOK_SCHEMES, } from '@propr/shared'; import { component } from './schemas.js'; @@ -17,7 +17,7 @@ const PluginManifestSummary = z.object({ mcpServers: z.array(z.object({}).loose()).describe('Contributed MCP servers without launch arguments or environment values.'), skills: z.array(z.string()), automationTemplates: z.array(z.string()), commands: z.array(z.object({ name: z.string(), description: z.string() })), subscriptions: z.array(z.string()), - webhooks: z.array(z.object({ name: z.string(), description: z.string() })), + webhooks: z.array(z.object({ name: z.string(), description: z.string(), scheme: z.enum(PLUGIN_WEBHOOK_SCHEMES).describe('How ProPR verifies the webhook\'s requests.') })), }), }).describe('The public projection of the plugin manifest.'); const acknowledgement = z.object({ actor: z.string(), at: z.number(), package: z.string(), version: z.string(), secretEnv: z.array(z.string()) }); @@ -89,11 +89,11 @@ export const PluginOAuthStartResponse = component('PluginOAuthStartResponse', z. const PluginLogLine = z.object({ level: z.enum(['debug', 'info', 'warn', 'error']), message: z.string(), fields: z.record(z.string(), z.unknown()).optional(), at: z.number() }); export const PluginDelivery = component('PluginDelivery', z.object({ id: z.number().int().describe('The activity record id, used by the retry route.'), - deliveryId: z.string().describe('The stable delivery id the handler receives (`evt--`); the same on every attempt.'), - kind: z.enum(PLUGIN_DELIVERY_KINDS).describe('`event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered.'), - repository: z.string(), - eventSeq: z.number().int().describe('The journal `seq` of the event; for a gap, the first `seq` that may be missing.'), - eventType: z.string().nullable().describe('`.`; null for a gap.'), + deliveryId: z.string().describe('The stable delivery id the handler receives (`evt--`, or `whk--` for a webhook); the same on every attempt.'), + kind: z.enum(PLUGIN_DELIVERY_KINDS).describe('`event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered. `webhook`: one verified inbound webhook request to its handler.'), + repository: z.string().describe('The event\'s repository; empty for a webhook delivery.'), + eventSeq: z.number().int().describe('The journal `seq` of the event; for a gap, the first `seq` that may be missing; 0 for a webhook delivery.'), + eventType: z.string().nullable().describe('`.`; `webhook.` for a webhook delivery; null for a gap.'), handler: z.string().nullable(), status: z.enum(PLUGIN_DELIVERY_STATUSES), attempt: z.number().int().describe('Attempts made so far.'), @@ -111,3 +111,14 @@ export const PluginHealth = component('PluginHealth', z.object({ export const PluginActivityResponse = component('PluginActivityResponse', z.object({ health: PluginHealth, deliveries: z.array(PluginDelivery).describe('The newest 100 delivery records, newest first.') })); export const PluginDeliveryResponse = component('PluginDeliveryResponse', z.object({ delivery: PluginDelivery })); export const PluginHealthResponse = component('PluginHealthResponse', z.object({ health: PluginHealth })); + +const PluginWebhookEndpoint = z.object({ + name: z.string(), description: z.string(), scheme: z.enum(PLUGIN_WEBHOOK_SCHEMES), + path: z.string().describe('`/api/plugins//hooks/`. The key is unguessable: treat the URL as a secret.'), + url: z.string().nullable().describe('The full URL; null when the instance has no public base URL.'), +}); +export const PluginWebhooksResponse = component('PluginWebhooksResponse', z.object({ + webhooks: z.array(PluginWebhookEndpoint).describe('One URL per webhook the plugin declares.'), + rejected: z.object({ count: z.number().int(), lastAt: z.number().nullable(), lastReason: z.string().nullable() }).describe('Requests refused because they failed verification. Their bodies and headers are never stored.'), + keyCreatedAt: z.number().nullable(), keyCreatedBy: z.string().nullable().describe('The administrator who last regenerated the key, or `system`.'), +}), 'The inbound webhook URLs of an installed plugin.'); diff --git a/packages/api/pluginRouteRegistry.ts b/packages/api/pluginRouteRegistry.ts index 04d0cdbb3..06109b9a9 100644 --- a/packages/api/pluginRouteRegistry.ts +++ b/packages/api/pluginRouteRegistry.ts @@ -1,11 +1,12 @@ import type { createPluginRoutes } from './routes/pluginRoutes.js'; import type { createPluginActivityRoutes } from './routes/pluginActivityRoutes.js'; +import type { createPluginWebhookAdminRoutes } from './routes/pluginWebhookRoutes.js'; import { requireManageSettings } from './permissionGuards.js'; import type { RouteEntry } from './routeRegistry.js'; import { PLUGIN_API_BASE } from '@propr/shared'; -/** Plugin installation and delivery activity routes; kept out of `routeRegistry.ts`, which is at its size limit. */ -export function createPluginRouteEntries(plugins: ReturnType, activity: ReturnType): RouteEntry[] { +/** Plugin installation, delivery activity and webhook URL routes; kept out of `routeRegistry.ts`, which is at its size limit. */ +export function createPluginRouteEntries(plugins: ReturnType, activity: ReturnType, webhooks: ReturnType): RouteEntry[] { const base = PLUGIN_API_BASE; return [ ['get', base, requireManageSettings, plugins.list], @@ -24,5 +25,7 @@ export function createPluginRouteEntries(plugins: ReturnType PluginWebhookReceiver | null, environment: NodeJS.ProcessEnv = process.env): RequestHandler[] { + const raw = express.raw({ type: () => true, limit: PLUGIN_WEBHOOK_LIMITS.bodyBytes }); + const readBody: RequestHandler = (req, res, next) => raw(req, res, error => { + if (!error) return next(); + const status = (error as { status?: number }).status; + // The body is not echoed or logged, whatever was wrong with it. + res.status(status === 413 ? 413 : 400).set('Cache-Control', 'no-store').json({ error: status === 413 ? `The request body is larger than ${PLUGIN_WEBHOOK_LIMITS.bodyBytes} bytes` : 'The request body could not be read' }); + }); + const send = (res: express.Response, reply: PluginWebhookReply) => { + res.status(reply.status).set({ 'Cache-Control': 'no-store', 'Content-Type': reply.contentType, 'X-Content-Type-Options': 'nosniff' }); + if (reply.retryAfterSeconds) res.set('Retry-After', String(reply.retryAfterSeconds)); + res.send(reply.body); + }; + const handle: RequestHandler = async (req, res) => { + const target = receiver(); + if (!target) return send(res, { status: 404, contentType: 'application/json', body: JSON.stringify({ error: 'Not found' }) }); + try { + const body = Buffer.isBuffer(req.body) ? req.body : Buffer.alloc(0); + send(res, await target.receive({ key: String(req.params.installationKey), name: String(req.params.webhookName), headers: req.headers, body })); + } catch (error) { + // Only the error's name: its message could quote the request. + console.error('[plugins] Webhook request failed:', error instanceof Error ? error.name : 'Error'); + if (!res.headersSent) send(res, { status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'Webhook processing failed' }) }); + } + }; + return [createWebhookRequestRateLimiter(environment), readBody, handle]; +} + +/** + * The installed plugin's webhook URLs, under `/api/instance/plugins/v1/:pluginId/webhooks`, + * and regenerating its installation key (every old URL stops working). The same + * administrator and preflight requirements as the installation routes apply. + */ +export function createPluginWebhookAdminRoutes(store = new PluginStore(db), database: Knex = db, environment: NodeJS.ProcessEnv = process.env, catalog: PluginCatalogSource = store.catalog ?? officialPluginCatalog) { + const handle = pluginRouteHandler(store); + const id = (req: Request) => { + const parsed = pluginIdSchema.safeParse(req.params.pluginId); + if (!parsed.success) throw new PluginError(404, 'Plugin not found'); + return parsed.data; + }; + const options = async (pluginId: string) => ({ manifest: (await catalog()).find(m => m.id === pluginId), origin: derivePublicOrigin(environment) ?? null }); + return { + list: handle(async req => { const pluginId = id(req); return pluginWebhookEndpoints(database, pluginId, await options(pluginId)); }), + regenerate: handle(async (req, actor) => { + const value = req.body as unknown; + if (!value || typeof value !== 'object' || Array.isArray(value) || Object.keys(value).length) throw new PluginError(400, 'Invalid plugin request'); + const pluginId = id(req); + return regeneratePluginWebhookKey(database, pluginId, actor, await options(pluginId)); + }), + }; +} diff --git a/packages/api/server.ts b/packages/api/server.ts index 3bf4c422f..fd7328dac 100644 --- a/packages/api/server.ts +++ b/packages/api/server.ts @@ -4,6 +4,9 @@ import { createStorageRouteEntries } from './instanceStorageRouteRegistry.js'; import { createPluginRouteEntries } from './pluginRouteRegistry.js'; import { createPluginRoutes } from './routes/pluginRoutes.js'; import { createPluginActivityRoutes } from './routes/pluginActivityRoutes.js'; +import { createPluginWebhookAdminRoutes, createPluginWebhookHandlers } from './routes/pluginWebhookRoutes.js'; +import { PluginWebhookRunner } from '../core/src/plugins/pluginWebhookRunner.js'; +import { PluginWebhookReceiver } from '../core/src/plugins/pluginWebhookReceiver.js'; import { protectStorageQueue } from '../core/src/storage/lifecycle.js'; import { createDefaultStorageService, createInstanceStorageRoutes } from './routes/instanceStorageRoutes.js'; import { startDashboardReadService, type DashboardReadService } from './services/dashboardReadService.js'; @@ -271,6 +274,9 @@ app.use(corsRejectionHandler); app.use('/api', createApiRequestRateLimiter()); setupWebhookRoute(); +// Plugin webhooks read their raw body: registered before the JSON parser and the session middleware. +let pluginWebhooks: { runner: PluginWebhookRunner; receiver: PluginWebhookReceiver } | undefined; +app.post('/api/plugins/:installationKey/hooks/:webhookName', ...createPluginWebhookHandlers(() => (demoMode ? null : pluginWebhooks?.receiver ?? null))); const defaultJsonParser = express.json({ limit: '1mb' }); app.use((req, res, next) => { @@ -516,7 +522,7 @@ export function setupRoutes(): void { const routes = [ ...operationalRoutes, ...createStorageRouteEntries(storageRoutes), - ...createPluginRouteEntries(createPluginRoutes(), createPluginActivityRoutes()), + ...createPluginRouteEntries(createPluginRoutes(), createPluginActivityRoutes(), createPluginWebhookAdminRoutes()), ...createMemberCatalogRouteEntries({ instanceCatalogRoutes }), ...createManagementRouteEntries({ adminRoutes, @@ -682,6 +688,10 @@ async function start(): Promise { await assertInstanceAdministratorConfigured(); await initRedis(); if (!demoMode) { + // Inbound plugin webhooks run in this process; their requests are held in memory only. + const runner = new PluginWebhookRunner({ database: db }); + await runner.recover().catch(error => console.warn('Could not close webhook deliveries left by an earlier process:', (error as Error).message)); + pluginWebhooks = { runner, receiver: new PluginWebhookReceiver({ database: db, runner }) }; await initializeNotificationBackground(); // Every API process drops its MCP cache on a published config event, so an // admin toggle applies across processes rather than waiting out the TTL. @@ -815,6 +825,7 @@ async function start(): Promise { connectMcpRegistration?.stop(); if (!demoMode) { shutdownTasks.push( + { name: 'plugin webhook deliveries', close: () => pluginWebhooks?.runner.stop() ?? Promise.resolve() }, { name: 'notification background service', close: () => notificationBackground?.close() ?? Promise.resolve() }, { name: 'visual-preview OAuth refresh scheduler', close: () => visualPreviewOAuthRefreshScheduler?.close() ?? Promise.resolve() }, { name: 'config reload subscriber', close: () => configReloadSubscription?.close() ?? Promise.resolve() }, diff --git a/packages/api/test/pluginActivityRoutes.test.ts b/packages/api/test/pluginActivityRoutes.test.ts index 12ec756d4..7babce00c 100644 --- a/packages/api/test/pluginActivityRoutes.test.ts +++ b/packages/api/test/pluginActivityRoutes.test.ts @@ -21,6 +21,7 @@ import { up as pluginStateMigration } from '../../core/src/db/migrations/2026111 import { up as journalMigration } from '../../core/src/db/migrations/20261112160000_create_repository_events.js'; import { up as taskDomainMigration } from '../../core/src/db/migrations/20261114120000_add_task_repository_event_domain.js'; import { up as deliveriesMigration } from '../../core/src/db/migrations/20261115000000_plugin_event_deliveries.js'; +import { up as webhookKeysMigration } from '../../core/src/db/migrations/20261116000000_plugin_webhook_keys.js'; /** The fixture plugin of the core dispatcher tests: its `flaky` handler fails while state key `fail` is above zero. */ const REGISTRY = path.join(path.dirname(new URL(import.meta.url).pathname), '..', '..', 'core', 'test', 'fixtures', 'plugins'); @@ -38,6 +39,7 @@ test('plugin activity API: deliveries and health are listed, a failed delivery i process.env.DB_FILENAME = path.join(root, 'fixture.sqlite'); const { createPluginRoutes } = await import('../routes/pluginRoutes.js'); const { createPluginActivityRoutes } = await import('../routes/pluginActivityRoutes.js'); + const { createPluginWebhookAdminRoutes } = await import('../routes/pluginWebhookRoutes.js'); const { createPluginRouteEntries } = await import('../pluginRouteRegistry.js'); const { registerRouteEntries } = await import('../routeRegistry.js'); const schemas = await import('../openapi/schemasPlugins.js'); @@ -46,7 +48,7 @@ test('plugin activity API: deliveries and health are listed, a failed delivery i const database = knex({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); await database.schema.createTable('instance_members', c => { c.string('github_user_id').primary(); c.string('role'); }); await database.schema.createTable('agent_definitions', c => { c.string('id').primary(); c.string('name'); }); - for (const migrate of [identityMigration, grantMigration, libraryMigration, oauthMigration, pluginMigration, pluginStateMigration, journalMigration, taskDomainMigration, deliveriesMigration]) await migrate(database); + for (const migrate of [identityMigration, grantMigration, libraryMigration, oauthMigration, pluginMigration, pluginStateMigration, journalMigration, taskDomainMigration, deliveriesMigration, webhookKeysMigration]) await migrate(database); await database('instance_members').insert([{ github_user_id: '1', role: 'admin' }, { github_user_id: '2', role: 'member' }]); const manifest = getOfficialPlugin(ID, { root: REGISTRY, includeDevelopment: true })!.manifest; const catalog = async () => [manifest]; @@ -61,7 +63,7 @@ test('plugin activity API: deliveries and health are listed, a failed delivery i req.authorization = { role: member ? 'member' : 'admin', permissions: member ? [] : ['instance.manage_settings'], source: 'local' }; next(); }); - const entries = createPluginRouteEntries(createPluginRoutes(store, {}), createPluginActivityRoutes(store, database, { PROPR_PLUGIN_PAUSE_AFTER_FAILURES: '3' })); + const entries = createPluginRouteEntries(createPluginRoutes(store, {}), createPluginActivityRoutes(store, database, { PROPR_PLUGIN_PAUSE_AFTER_FAILURES: '3' }), createPluginWebhookAdminRoutes(store, database, {})); registerRouteEntries(app, entries); const server = app.listen(0, '127.0.0.1'); await new Promise(r => server.once('listening', r)); t.after(async () => { await new Promise(r => server.close(() => r())); await database.destroy(); await (await import('@propr/core')).closeConnection(); await fs.rm(root, { recursive: true, force: true }); }); diff --git a/packages/api/test/pluginRoutes.test.ts b/packages/api/test/pluginRoutes.test.ts index 9beea2a1e..947cbcf20 100644 --- a/packages/api/test/pluginRoutes.test.ts +++ b/packages/api/test/pluginRoutes.test.ts @@ -16,6 +16,7 @@ import { up as pluginMigration } from '../../core/src/db/migrations/202611140000 import { up as pluginStateMigration } from '../../core/src/db/migrations/20261114180000_plugin_state.js'; import { up as journalMigration } from '../../core/src/db/migrations/20261112160000_create_repository_events.js'; import { up as deliveriesMigration } from '../../core/src/db/migrations/20261115000000_plugin_event_deliveries.js'; +import { up as webhookKeysMigration } from '../../core/src/db/migrations/20261116000000_plugin_webhook_keys.js'; import { up as grantMigration } from '../../core/src/db/migrations/20261013220000_extension_grant_network.js'; import { up as libraryMigration } from '../../core/src/db/migrations/20261113000000_extension_library.js'; import { up as oauthMigration } from '../../core/src/db/migrations/20261113120000_extension_oauth.js'; @@ -45,7 +46,7 @@ test('plugin REST API and MCP read tools: administrators only, audited, secret-f const database = knex({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); await database.schema.createTable('instance_members', c => { c.string('github_user_id').primary(); c.string('role'); }); await database.schema.createTable('agent_definitions', c => { c.string('id').primary(); c.string('name'); }); - await identityMigration(database); await grantMigration(database); await libraryMigration(database); await oauthMigration(database); await pluginMigration(database); await pluginStateMigration(database); await journalMigration(database); await deliveriesMigration(database); + await identityMigration(database); await grantMigration(database); await libraryMigration(database); await oauthMigration(database); await pluginMigration(database); await pluginStateMigration(database); await journalMigration(database); await deliveriesMigration(database); await webhookKeysMigration(database); // 3 holds the admin role in the session but is no longer a durable administrator. await database('instance_members').insert([{ github_user_id: '1', role: 'admin' }, { github_user_id: '2', role: 'member' }, { github_user_id: '3', role: 'member' }]); // The remote server at the synthetic OAuth provider's address literal: no DNS in tests. @@ -67,7 +68,8 @@ test('plugin REST API and MCP read tools: administrators only, audited, secret-f (req as { sessionID?: string }).sessionID = req.get('x-test-session') ?? 'session-a'; next(); }); const { createPluginActivityRoutes } = await import('../routes/pluginActivityRoutes.js'); - const entries = createPluginRouteEntries(createPluginRoutes(store, { API_PUBLIC_URL: 'https://propr.example.com' }), createPluginActivityRoutes(store, database)); + const { createPluginWebhookAdminRoutes } = await import('../routes/pluginWebhookRoutes.js'); + const entries = createPluginRouteEntries(createPluginRoutes(store, { API_PUBLIC_URL: 'https://propr.example.com' }), createPluginActivityRoutes(store, database), createPluginWebhookAdminRoutes(store, database, {})); registerRouteEntries(app, entries); const server = app.listen(0, '127.0.0.1'); await new Promise(r => server.once('listening', r)); t.after(async () => { await new Promise(r => server.close(() => r())); await database.destroy(); await (await import('@propr/core')).closeConnection(); await fs.rm(root, { recursive: true, force: true }); }); diff --git a/packages/api/test/pluginWebhookRoutes.test.ts b/packages/api/test/pluginWebhookRoutes.test.ts new file mode 100644 index 000000000..8a235b932 --- /dev/null +++ b/packages/api/test/pluginWebhookRoutes.test.ts @@ -0,0 +1,183 @@ +import test, { mock } from 'node:test'; +import assert from 'node:assert/strict'; +import { createHmac } from 'node:crypto'; +import express from 'express'; +import { rateLimit } from 'express-rate-limit'; +import knex from 'knex'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import type { AddressInfo } from 'node:net'; +import { z } from 'zod'; +import { getOfficialPlugin, loadPluginHandlers } from '../../plugins/src/index.js'; +import { PluginStore } from '../../core/src/plugins/pluginStore.js'; +import { PluginWebhookRunner } from '../../core/src/plugins/pluginWebhookRunner.js'; +import { PluginWebhookReceiver } from '../../core/src/plugins/pluginWebhookReceiver.js'; +import coreLogger from '../../core/src/utils/logger.js'; +import { up as identityMigration } from '../../core/src/db/migrations/20261013210000_extension_profiles.js'; +import { up as grantMigration } from '../../core/src/db/migrations/20261013220000_extension_grant_network.js'; +import { up as libraryMigration } from '../../core/src/db/migrations/20261113000000_extension_library.js'; +import { up as oauthMigration } from '../../core/src/db/migrations/20261113120000_extension_oauth.js'; +import { up as pluginMigration } from '../../core/src/db/migrations/20261114000000_plugin_installations.js'; +import { up as pluginStateMigration } from '../../core/src/db/migrations/20261114180000_plugin_state.js'; +import { up as journalMigration } from '../../core/src/db/migrations/20261112160000_create_repository_events.js'; +import { up as taskDomainMigration } from '../../core/src/db/migrations/20261114120000_add_task_repository_event_domain.js'; +import { up as deliveriesMigration } from '../../core/src/db/migrations/20261115000000_plugin_event_deliveries.js'; +import { up as webhookKeysMigration } from '../../core/src/db/migrations/20261116000000_plugin_webhook_keys.js'; + +/** The `fixture-webhooks` plugin of the core webhook tests: one webhook per verification scheme. */ +const REGISTRY = path.join(path.dirname(new URL(import.meta.url).pathname), '..', '..', 'core', 'test', 'fixtures', 'plugins'); +const ID = 'fixture-webhooks'; +const BASE = '/api/instance/plugins/v1'; +const SECRET = 'whsec_route-fixture-secret'; + +function assertConforms(schema: z.ZodType, body: unknown, label: string): void { + const result = schema.safeParse(body); + assert.ok(result.success, `${label} does not match its schema:\n${result.success ? '' : z.prettifyError(result.error)}`); + assert.deepEqual(result.data, body, `${label} sends fields its schema does not allow`); +} + +test('plugin webhook route: verified requests are queued, oversized bodies and unverified requests run nothing, the rate limiter applies, and the Plugins page URL regenerates', async t => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'plugin-webhook-api-')); + process.env.DB_FILENAME = path.join(root, 'fixture.sqlite'); + const { createPluginRoutes } = await import('../routes/pluginRoutes.js'); + const { createPluginActivityRoutes } = await import('../routes/pluginActivityRoutes.js'); + const { createPluginWebhookAdminRoutes, createPluginWebhookHandlers } = await import('../routes/pluginWebhookRoutes.js'); + const { createPluginRouteEntries } = await import('../pluginRouteRegistry.js'); + const { registerRouteEntries } = await import('../routeRegistry.js'); + const schemas = await import('../openapi/schemasPlugins.js'); + const { ROUTE_DOCS } = await import('../openapi/routeDocs.js'); + const { DIRECT_ROUTE_REGISTRATIONS } = await import('../openapi/directRoutes.js'); + + const database = knex({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + await database.schema.createTable('instance_members', c => { c.string('github_user_id').primary(); c.string('role'); }); + await database.schema.createTable('agent_definitions', c => { c.string('id').primary(); c.string('name'); }); + for (const migrate of [identityMigration, grantMigration, libraryMigration, oauthMigration, pluginMigration, pluginStateMigration, journalMigration, taskDomainMigration, deliveriesMigration, webhookKeysMigration]) await migrate(database); + await database('instance_members').insert([{ github_user_id: '1', role: 'admin' }, { github_user_id: '2', role: 'member' }]); + const manifest = getOfficialPlugin(ID, { root: REGISTRY, includeDevelopment: true })!.manifest; + const catalog = async () => [manifest]; + const environment = { SYSTEM_TASK_SECRET: 'plugin-webhook-custody', API_PUBLIC_URL: 'https://propr.example.com', PROPR_WEBHOOK_RATE_LIMIT_MAX: '12' }; + const store = new PluginStore(database, environment, catalog); + const loads: string[] = []; + const handlers = (id: string) => { loads.push(id); return loadPluginHandlers(id, { root: REGISTRY, includeDevelopment: true }); }; + const runner = new PluginWebhookRunner({ database, catalog, handlers, environment, retryDelaysMs: [] }); + const receiver = new PluginWebhookReceiver({ database, runner, catalog, handlers, environment }); + + const lines: string[] = []; + for (const level of ['trace', 'debug', 'info', 'warn', 'error', 'fatal'] as const) mock.method(coreLogger, level, (...args: unknown[]) => { lines.push(JSON.stringify(args)); }); + for (const level of ['log', 'info', 'warn', 'error', 'debug'] as const) mock.method(console, level, (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + + const app = express(); + app.post('/api/plugins/:installationKey/hooks/:webhookName', ...createPluginWebhookHandlers(() => receiver, environment)); + app.use(rateLimit({ windowMs: 60_000, limit: 1000 })); + app.use(express.json()); + app.use((req, _res, next) => { + const actor = req.get('x-test-actor'); if (actor) req.user = { id: actor, username: 'fixture' } as typeof req.user; + const member = req.get('x-test-role') === 'member'; + req.authorization = { role: member ? 'member' : 'admin', permissions: member ? [] : ['instance.manage_settings'], source: 'local' }; + next(); + }); + const entries = createPluginRouteEntries(createPluginRoutes(store, environment), createPluginActivityRoutes(store, database), createPluginWebhookAdminRoutes(store, database, environment)); + registerRouteEntries(app, entries); + const server = app.listen(0, '127.0.0.1'); await new Promise(r => server.once('listening', r)); + t.after(async () => { + mock.restoreAll(); + await runner.stop(); + await new Promise(r => server.close(() => r())); await database.destroy(); await (await import('@propr/core')).closeConnection(); await fs.rm(root, { recursive: true, force: true }); + }); + const origin = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; + const admin = { 'x-test-actor': '1', 'content-type': 'application/json', 'x-propr-plugins': '1' }; + const call = async (method: string, route: string, body?: unknown, headers: Record = admin) => { + const response = await fetch(origin + BASE + route, { method, headers, ...(body === undefined ? {} : { body: JSON.stringify(body) }) }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- response bodies are checked against their schemas + return { status: response.status, body: await response.json() as any }; + }; + const hook = async (url: string, body: string | Buffer, headers: Record) => { + const response = await fetch(url.replace('https://propr.example.com', origin), { method: 'POST', headers, body }); + return { status: response.status, text: await response.text(), retryAfter: response.headers.get('retry-after'), type: response.headers.get('content-type') }; + }; + const signed = (body: string, delivery: string) => ({ 'content-type': 'application/json', 'x-hub-signature-256': `sha256=${createHmac('sha256', SECRET).update(body).digest('hex')}`, 'x-github-delivery': delivery }); + const seen = async () => { + const row = await database('plugin_state').where({ plugin_id: ID, state_key: 'seen' }).first() as { value: string } | undefined; + return row ? (JSON.parse(row.value) as unknown[]).length : 0; + }; + + for (const route of ['/:pluginId/webhooks', '/:pluginId/webhooks/regenerate']) { + const entry = entries.find(([, p]) => p === BASE + route)!; + assert.ok(ROUTE_DOCS[`${entry[0].toUpperCase()} ${entry[1]}`], `${route} has an OpenAPI entry`); + } + assert.ok(DIRECT_ROUTE_REGISTRATIONS.some(r => r.path === '/api/plugins/:installationKey/hooks/:webhookName' && r.auth === 'public')); + assert.ok(ROUTE_DOCS['POST /api/plugins/:installationKey/hooks/:webhookName']); + + assert.equal((await call('GET', `/${ID}/webhooks`)).status, 409, 'not installed'); + await store.install('1', ID); await store.setConnection('1', ID, 'hook-secret', SECRET); await store.setEnabled('1', ID, true); + assert.equal((await call('GET', `/${ID}/webhooks`, undefined, { 'x-test-actor': '2', 'x-test-role': 'member' })).status, 403, 'administrators only'); + const listed = await call('GET', `/${ID}/webhooks`); + assert.equal(listed.status, 200); + assertConforms(schemas.PluginWebhooksResponse, listed.body, 'webhooks'); + const urls = Object.fromEntries((listed.body.webhooks as { name: string; url: string; scheme: string }[]).map(w => [w.name, w.url])); + assert.deepEqual((listed.body.webhooks as { name: string; scheme: string }[]).map(w => [w.name, w.scheme]), [['signed', 'hmac-sha256'], ['slack', 'slack-v0'], ['token', 'token-header'], ['probe', 'token-header']]); + assert.match(urls.signed, /^https:\/\/propr\.example\.com\/api\/plugins\/[A-Za-z0-9_-]{43}\/hooks\/signed$/); + + // Verified: answered 202 at once, then run from the queue exactly once. + const body = '{"action":"opened","text":"ROUTE-SENTINEL-BODY"}'; + const accepted = await hook(urls.signed, body, signed(body, 'route-1')); + assert.equal(accepted.status, 202); + assert.match(accepted.type ?? '', /application\/json/); + await runner.idle(); + assert.equal(await seen(), 1); + assert.equal((await hook(urls.signed, body, signed(body, 'route-1'))).status, 409, 'replay'); + const formBody = 'payload=%7B%7D'; + assert.equal((await hook(urls.signed, formBody, { ...signed(formBody, 'route-form'), 'content-type': 'application/x-www-form-urlencoded' })).status, 202, 'any content type is read raw'); + + // Oversized, unverified and unknown requests load no plugin code. + await runner.idle(); + const loadsBefore = loads.length; + const big = 'x'.repeat(256 * 1024 + 1); + const oversized = await hook(urls.signed, big, signed(big, 'route-big')); + assert.equal(oversized.status, 413); + assert.doesNotMatch(oversized.text, /xxxx/, 'the body is not echoed'); + assert.equal((await hook(urls.signed, body, { ...signed(body, 'route-2'), 'x-hub-signature-256': 'sha256=ROUTE-SENTINEL-SIGNATURE' })).status, 401); + assert.equal((await hook(urls.token, body, { 'x-gitlab-token': 'ROUTE-SENTINEL-TOKEN' })).status, 401); + assert.equal((await hook(urls.signed.replace('/hooks/signed', '/hooks/nope'), body, signed(body, 'route-3'))).status, 404); + assert.equal(loads.length, loadsBefore, 'no plugin module loaded'); + assert.equal(await seen(), 2); + + // The challenge echo. + const challenge = JSON.stringify({ type: 'url_verification', challenge: 'route-challenge-123' }); + const timestamp = String(Math.floor(Date.now() / 1000)); + const echo = await hook(urls.slack, challenge, { 'x-slack-request-timestamp': timestamp, 'x-slack-signature': `v0=${createHmac('sha256', SECRET).update(`v0:${timestamp}:${challenge}`).digest('hex')}` }); + assert.deepEqual([echo.status, echo.text, echo.type?.split(';')[0]], [200, 'route-challenge-123', 'text/plain']); + + // Regenerate: the old URL answers 404, the new one works; audited; members and a non-empty body are refused. + assert.equal((await call('POST', `/${ID}/webhooks/regenerate`, {}, { 'x-test-actor': '1', 'content-type': 'application/json' })).status, 403, 'preflight header required'); + assert.equal((await call('POST', `/${ID}/webhooks/regenerate`, { key: 'mine' })).status, 400); + const regenerated = await call('POST', `/${ID}/webhooks/regenerate`, {}); + assert.equal(regenerated.status, 200); + assertConforms(schemas.PluginWebhooksResponse, regenerated.body, 'regenerated webhooks'); + assert.equal(regenerated.body.keyCreatedBy, '1'); + assert.equal(regenerated.body.rejected.count, 0, 'a new key starts a new count'); + const newSigned = (regenerated.body.webhooks as { name: string; url: string }[]).find(w => w.name === 'signed')!.url; + assert.notEqual(newSigned, urls.signed); + assert.equal((await hook(urls.signed, body, signed(body, 'route-4'))).status, 404, 'the old URL is dead'); + assert.equal((await hook(newSigned, body, signed(body, 'route-5'))).status, 202); + assert.ok((await store.get('1', ID)).audit.some(a => a.action === 'regenerate_webhook_key')); + + // Disabled: 404, and nothing runs. + await runner.idle(); + await store.setEnabled('1', ID, false); + assert.equal((await hook(newSigned, body, signed(body, 'route-6'))).status, 404); + await store.setEnabled('1', ID, true); + + // The webhook rate limiter (12 per window here) applies to the route. + const statuses: number[] = []; + for (let i = 0; i < 12; i++) statuses.push((await hook(newSigned, '{}', { 'x-hub-signature-256': 'sha256=00' })).status); + assert.ok(statuses.includes(429), `the rate limiter answered 429 (${statuses.join(', ')})`); + await runner.idle(); + assert.equal(await seen(), 3, 'route-1, the form request and route-5 ran; nothing else did'); + + // Neither the body nor a signature or token reached a log line. + for (const sentinel of ['ROUTE-SENTINEL-BODY', 'ROUTE-SENTINEL-SIGNATURE', 'ROUTE-SENTINEL-TOKEN', 'route-challenge-123']) { + assert.equal(lines.some(line => line.includes(sentinel)), false, `${sentinel} was logged`); + } +}); diff --git a/packages/client/src/generated/apiTypes.ts b/packages/client/src/generated/apiTypes.ts index 87be9089a..c89bdd31d 100644 --- a/packages/client/src/generated/apiTypes.ts +++ b/packages/client/src/generated/apiTypes.ts @@ -933,14 +933,15 @@ export interface PluginConnectionRequest { export interface PluginDelivery { /** The activity record id, used by the retry route. */ id: number; - /** The stable delivery id the handler receives (`evt--`); the same on every attempt. */ + /** The stable delivery id the handler receives (`evt--`, or `whk--` for a webhook); the same on every attempt. */ deliveryId: string; - /** `event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered. */ - kind: "event" | "gap"; + /** `event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered. `webhook`: one verified inbound webhook request to its handler. */ + kind: "event" | "gap" | "webhook"; + /** The event's repository; empty for a webhook delivery. */ repository: string; - /** The journal `seq` of the event; for a gap, the first `seq` that may be missing. */ + /** The journal `seq` of the event; for a gap, the first `seq` that may be missing; 0 for a webhook delivery. */ eventSeq: number; - /** `.`; null for a gap. */ + /** `.`; `webhook.` for a webhook delivery; null for a gap. */ eventType: string | null; handler: string | null; status: "pending" | "running" | "retrying" | "succeeded" | "failed" | "skipped"; @@ -1017,10 +1018,12 @@ export interface PluginDetailResponse { description: string; }[]; subscriptions: string[]; - webhooks: { + webhooks: Array<{ name: string; description: string; - }[]; + /** How ProPR verifies the webhook's requests. */ + scheme: "hmac-sha256" | "slack-v0" | "token-header"; + }>; }; } | null; state: "available" | "installed" | "enabled"; @@ -1116,7 +1119,7 @@ export interface PluginDetailResponse { }; /** The newest 50 management actions, newest first. */ audit: Array<{ - action: "install" | "update_settings" | "set_repositories" | "set_connection" | "clear_connection" | "enable" | "disable" | "approve" | "uninstall" | "start_oauth" | "complete_oauth" | "disconnect_oauth" | "set_oauth_client" | "clear_oauth_client" | "handler_attempt" | "handler_call" | "handler_refused" | "retry_delivery" | "resume_deliveries"; + action: "install" | "update_settings" | "set_repositories" | "set_connection" | "clear_connection" | "enable" | "disable" | "approve" | "uninstall" | "start_oauth" | "complete_oauth" | "disconnect_oauth" | "set_oauth_client" | "clear_oauth_client" | "handler_attempt" | "handler_call" | "handler_refused" | "retry_delivery" | "resume_deliveries" | "regenerate_webhook_key"; actor: string; at: number; detail: { @@ -1247,10 +1250,12 @@ export interface PluginView { description: string; }[]; subscriptions: string[]; - webhooks: { + webhooks: Array<{ name: string; description: string; - }[]; + /** How ProPR verifies the webhook's requests. */ + scheme: "hmac-sha256" | "slack-v0" | "token-header"; + }>; }; } | null; state: "available" | "installed" | "enabled"; @@ -1346,6 +1351,29 @@ export interface PluginView { }; } +/** The inbound webhook URLs of an installed plugin. */ +export interface PluginWebhooksResponse { + /** One URL per webhook the plugin declares. */ + webhooks: Array<{ + name: string; + description: string; + scheme: "hmac-sha256" | "slack-v0" | "token-header"; + /** `/api/plugins//hooks/`. The key is unguessable: treat the URL as a secret. */ + path: string; + /** The full URL; null when the instance has no public base URL. */ + url: string | null; + }>; + /** Requests refused because they failed verification. Their bodies and headers are never stored. */ + rejected: { + count: number; + lastAt: number | null; + lastReason: string | null; + }; + keyCreatedAt: number | null; + /** The administrator who last regenerated the key, or `system`. */ + keyCreatedBy: string | null; +} + /** Read-only readiness verdict of one pull request. Evaluating it never writes to GitHub or Redis, never updates the branch or merges, and never changes auto-merge state. */ export interface PullRequestReadiness { repository: string; diff --git a/packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js b/packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js new file mode 100644 index 000000000..2d871190f --- /dev/null +++ b/packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js @@ -0,0 +1,30 @@ +// Plugin inbound webhooks: one installation key per installed plugin. +// +// The key is the unguessable segment of the plugin's webhook URLs +// (`/api/plugins//hooks/`). Requests are looked up by `key_hash` +// (SHA-256 of the key); `key` itself is kept so administrators can copy the +// URL from the Plugins page. `installed_at` binds the key to one +// installation: a reinstall gets a new one. Regenerating replaces the row, so +// the old URL stops working at once. +// +// `rejected_count`, `last_rejected_at` and `last_rejected_reason` count +// requests that failed verification. Request bodies and signature headers are +// never stored, here or anywhere else. + +export async function up(knex) { + if (await knex.schema.hasTable('plugin_webhook_keys')) return; + await knex.schema.createTable('plugin_webhook_keys', t => { + t.string('plugin_id').primary(); + t.bigInteger('installed_at').notNullable(); + t.string('key_hash', 64).notNullable().unique(); + t.string('key', 64).notNullable(); + t.string('created_by').notNullable(); + t.bigInteger('created_at').notNullable(); + t.integer('rejected_count').notNullable().defaultTo(0); + t.bigInteger('last_rejected_at'); + t.string('last_rejected_reason'); + }); +} + +// Webhook keys belong to installations, which stay on rollback; older versions ignore them. +export async function down() {} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f54518801..5ec056bbc 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -680,6 +680,9 @@ export * from './services/repositoryEventJournal.js'; // Plugin event subscriptions: the journal dispatcher and its delivery records. export { PluginDispatcher, type PluginDispatcherOptions, type PluginDispatchPass } from './plugins/pluginDispatcher.js'; export { prunePluginDeliveries } from './plugins/pluginDeliveries.js'; +export { PluginWebhookRunner, type PluginWebhookRunnerOptions } from './plugins/pluginWebhookRunner.js'; +export { PluginWebhookReceiver, type PluginWebhookReceiverOptions, type PluginWebhookReply } from './plugins/pluginWebhookReceiver.js'; +export { PLUGIN_WEBHOOK_ROUTE, pluginWebhookEndpoints, regeneratePluginWebhookKey, type PluginWebhookEndpoints } from './plugins/pluginWebhooks.js'; // GitHub user profile cache: maps stored GitHub numeric user ids to the login // and avatar an assignee is rendered with. diff --git a/packages/core/src/plugins/pluginStore.ts b/packages/core/src/plugins/pluginStore.ts index f9fc2db4d..c088407b9 100644 --- a/packages/core/src/plugins/pluginStore.ts +++ b/packages/core/src/plugins/pluginStore.ts @@ -14,6 +14,7 @@ import { PluginContributions, oauthServersOf, pluginContributionViews, type Plug import { pluginTemplateIds } from './pluginTemplates.js'; import { deletePluginState } from './pluginStateStore.js'; import { deletePluginDispatch, restartPluginDispatch } from './pluginDeliveries.js'; +import { deletePluginWebhookKey } from './pluginWebhooks.js'; import { PluginError, enableBlockers, installationRecord, nextAcknowledgements, parseAcknowledgements, pendingAcknowledgements, pendingPermissions, pluginView, type PluginInstallationRow, type PluginSecretRow, @@ -321,6 +322,7 @@ export class PluginStore { await trx('plugin_installations').where({ plugin_id: pluginId }).delete(); const state = await deletePluginState(trx, pluginId); await deletePluginDispatch(trx, pluginId); + await deletePluginWebhookKey(trx, pluginId); return { action: 'uninstall', detail: { version: row.manifest_version, stateKeys: state } }; }); } diff --git a/packages/core/src/plugins/pluginWebhookReceiver.ts b/packages/core/src/plugins/pluginWebhookReceiver.ts new file mode 100644 index 000000000..1cbd0962e --- /dev/null +++ b/packages/core/src/plugins/pluginWebhookReceiver.ts @@ -0,0 +1,125 @@ +import { randomUUID } from 'node:crypto'; +import type { IncomingHttpHeaders } from 'node:http'; +import type { Knex } from 'knex'; +import { PLUGIN_WEBHOOK_LIMITS, type PluginWebhook, type PluginWebhookRequest, type PluginWebhookResponder, type PluginWebhookResponse } from '@propr/shared'; +import logger from '../utils/logger.js'; +import { officialPluginCatalog, type PluginCatalogSource } from './pluginCatalog.js'; +import { officialPluginHandlers, type PluginHandlerSource } from './pluginHandlerRunner.js'; +import { PluginStore } from './pluginStore.js'; +import type { PluginWebhookRunner } from './pluginWebhookRunner.js'; +import { + handlerHeaders, recordPluginWebhookRejection, resolvePluginWebhook, senderDeliveryId, verifyPluginWebhook, + type PluginWebhookRejection, type ResolvedPluginWebhook, +} from './pluginWebhooks.js'; + +/** + * The inbound plugin webhook request, from `POST /api/plugins//hooks/` + * to the delivery queue. In order: + * + * 1. The key and name must address an enabled installation and a declared + * webhook: otherwise 404 and nothing runs. + * 2. ProPR verifies the request with the webhook's scheme and signing secret: + * otherwise 401, counted on the installation, logged without the body or + * any header. + * 3. A `respond` function may answer at once (a challenge echo): it gets the + * frozen request as its only argument, no `ctx`, and must return + * synchronously. + * 4. A delivery id already recorded is a replay: 409. + * 5. Otherwise the delivery is queued (202) and the handler runs from the + * queue. While deliveries are paused, or the queue is full, 503. + * + * The body size limit and the rate limiter are applied by the route before this. + */ +export interface PluginWebhookReceiverOptions { + database: Knex; + runner: PluginWebhookRunner; + catalog?: PluginCatalogSource; + handlers?: PluginHandlerSource; + /** The stored value of a `signing_secret` connection; defaults to the plugin store's sealed connection secrets. */ + connectionSecret?: (pluginId: string, connection: string) => Promise; + environment?: NodeJS.ProcessEnv; + now?: () => number; +} + +export interface PluginWebhookRequestInput { key: string; name: string; headers: IncomingHttpHeaders; body: Buffer } +export interface PluginWebhookReply { status: number; contentType: 'application/json' | 'text/plain'; body: string; retryAfterSeconds?: number } + +const json = (status: number, value: Record, extra: Partial = {}): PluginWebhookReply => ({ status, contentType: 'application/json', body: JSON.stringify(value), ...extra }); +const NOT_FOUND = json(404, { error: 'Not found' }); + +/** A respond function's answer, checked: a 2xx status, a short text body and a supported content type. */ +function checkedResponse(value: unknown): PluginWebhookResponse | null { + if (value === null || value === undefined) return null; + if (typeof value !== 'object' || typeof (value as { then?: unknown }).then === 'function') throw new Error('A respond function must return a response synchronously'); + const { status = 200, contentType = 'text/plain', body } = value as PluginWebhookResponse; + if (!Number.isInteger(status) || status < 200 || status > 299) throw new Error('A respond function answers with a 2xx status'); + if (contentType !== 'text/plain' && contentType !== 'application/json') throw new Error('A respond function answers text/plain or application/json'); + if (typeof body !== 'string' || Buffer.byteLength(body) > PLUGIN_WEBHOOK_LIMITS.responseBytes) throw new Error(`A respond function answers a text body of at most ${PLUGIN_WEBHOOK_LIMITS.responseBytes} bytes`); + return { status, contentType, body }; +} + +export class PluginWebhookReceiver { + private readonly now: () => number; + constructor(private readonly options: PluginWebhookReceiverOptions) { this.now = options.now ?? Date.now; } + + private secret(pluginId: string, connection: string): Promise { + if (this.options.connectionSecret) return this.options.connectionSecret(pluginId, connection); + return new PluginStore(this.options.database, this.options.environment).connectionSecret(pluginId, connection); + } + + private async reject(resolved: ResolvedPluginWebhook, reason: PluginWebhookRejection): Promise { + await recordPluginWebhookRejection(this.options.database, resolved.target.pluginId, reason, this.now()); + // Never the body or a header: only who was addressed and why it was refused. + logger.warn({ pluginId: resolved.target.pluginId, webhook: resolved.webhook.name, reason }, '[plugins] Rejected an unverified webhook request'); + return json(401, { error: 'The request could not be verified' }); + } + + /** Call the webhook's respond function with the frozen request only; never `ctx`. */ + private async respond(resolved: ResolvedPluginWebhook, request: PluginWebhookRequest): Promise { + const { webhook, target } = resolved; + if (!webhook.respond) return null; + try { + const module = await (this.options.handlers ?? officialPluginHandlers)(target.pluginId); + const responder = module && Object.hasOwn(module, webhook.respond) ? module[webhook.respond] as unknown as PluginWebhookResponder : undefined; + if (typeof responder !== 'function') throw new Error(`Plugin ${target.pluginId} has no function ${webhook.respond}`); + const response = checkedResponse(Reflect.apply(responder, undefined, [request])); + return response ? { status: response.status!, contentType: response.contentType!, body: response.body } : null; + } catch (error) { + logger.warn({ pluginId: target.pluginId, webhook: webhook.name, error: error instanceof Error ? error.name : 'Error' }, '[plugins] Webhook respond function failed'); + return json(500, { error: 'The webhook could not answer' }); + } + } + + private deliveryId(webhook: PluginWebhook, headers: IncomingHttpHeaders): string | null { + const sender = senderDeliveryId(headers, webhook); + if (sender === null) return null; + return `whk-${webhook.name}-${sender ?? randomUUID()}`; + } + + async receive(request: PluginWebhookRequestInput): Promise { + const { database } = this.options; + const resolved = await resolvePluginWebhook(database, request.key, request.name, await (this.options.catalog ?? officialPluginCatalog)()); + if (!resolved) return NOT_FOUND; + const { target, webhook } = resolved; + const receivedAt = this.now(); + const secret = await this.secret(target.pluginId, webhook.verification.connection).catch(() => null); + if (!secret) return this.reject(resolved, 'no_secret'); + const verdict = verifyPluginWebhook({ verification: webhook.verification, secret, headers: request.headers, body: request.body, now: receivedAt }); + if (!verdict.ok) return this.reject(resolved, verdict.reason); + + const body = request.body.toString('utf8'); + const headers = Object.freeze(handlerHeaders(request.headers, webhook)); + const answer = await this.respond(resolved, Object.freeze({ webhook: webhook.name, headers, body, receivedAt })); + if (answer) return answer; + + const deliveryId = this.deliveryId(webhook, request.headers); + if (deliveryId === null) return json(400, { error: `Invalid ${webhook.deliveryIdHeader} header` }); + const dispatch = await database('plugin_dispatch').where({ plugin_id: target.pluginId, installed_at: target.record.installedAt }).first('paused_at') as { paused_at: number | null } | undefined; + if (dispatch && dispatch.paused_at !== null) return json(503, { error: 'Deliveries of this plugin are paused' }, { retryAfterSeconds: 300 }); + const accepted = await this.options.runner.accept({ target, handler: webhook.handler, deliveryId, input: { kind: 'webhook', webhook: webhook.name, headers: { ...headers }, body, receivedAt } }); + if (!accepted.accepted) { + return accepted.reason === 'duplicate' ? json(409, { error: 'This delivery was already received' }) : json(503, { error: 'Too many webhook deliveries are waiting' }, { retryAfterSeconds: 60 }); + } + return json(202, { accepted: true, deliveryId }); + } +} diff --git a/packages/core/src/plugins/pluginWebhookRunner.ts b/packages/core/src/plugins/pluginWebhookRunner.ts new file mode 100644 index 000000000..068a0c89e --- /dev/null +++ b/packages/core/src/plugins/pluginWebhookRunner.ts @@ -0,0 +1,167 @@ +import type { Knex } from 'knex'; +import { PLUGIN_DELIVERY_RETRY_DELAYS_MS, PLUGIN_WEBHOOK_LIMITS, type PluginHandlerRun, type PluginWebhookInput } from '@propr/shared'; +import logger from '../utils/logger.js'; +import { runPluginHandler, type RunPluginHandlerOptions } from './pluginHandlerRunner.js'; +import { installationRecord, type PluginInstallationRow } from './pluginState.js'; +import { clipError, storedLogs, type PluginDeliveryRow } from './pluginDeliveries.js'; +import type { PluginInstallationTarget } from './pluginContext.js'; + +/** + * Runs verified inbound webhook requests through their handlers, as + * deliveries of kind `webhook` recorded in the plugin's Activity like event + * deliveries, with the same retry schedule (1 min, 5 min, 30 min, 2 h, then + * failed). + * + * The request (body and headers) is held in this process's memory only, never + * written anywhere: the delivery record carries the webhook and handler + * names, the delivery id, status, timing and the handler's redacted error and + * log lines. A delivery still waiting when the process stops cannot be + * resumed by another one and is marked failed. + */ +export interface PluginWebhookRunnerOptions extends Omit { + database: Knex; + retryDelaysMs?: readonly number[]; + /** Deliveries held in memory at once; further requests are refused (503) until some finish. */ + maxPending?: number; + run?: typeof runPluginHandler; +} + +export interface PluginWebhookJob { + target: PluginInstallationTarget; + handler: string; + /** The delivery id the handler receives: `whk--`. */ + deliveryId: string; + input: Omit; +} +export type PluginWebhookAcceptance = { accepted: true; recordId: number } | { accepted: false; reason: 'duplicate' | 'busy' }; + +interface Held { job: PluginWebhookJob; recordId: number; attempt: number; timer?: NodeJS.Timeout } + +const LOST = 'Not delivered: ProPR stopped before the delivery finished, and webhook requests are not kept'; +const UNFINISHED = ['pending', 'running', 'retrying'] as const; + +export class PluginWebhookRunner { + private readonly held = new Map(); + private readonly running = new Set>(); + /** Deliveries whose next attempt starts at once (not a retry delay). */ + private readonly due = new Set(); + private readonly now: () => number; + private readonly delays: readonly number[]; + private closed = false; + + constructor(private readonly options: PluginWebhookRunnerOptions) { + this.now = options.now ?? Date.now; + this.delays = options.retryDelaysMs ?? PLUGIN_DELIVERY_RETRY_DELAYS_MS; + } + + /** Deliveries held right now (waiting, running or retrying). */ + get pending(): number { return this.held.size; } + + /** Mark webhook deliveries an earlier process left unfinished as failed: their requests were never stored. */ + async recover(): Promise { + return this.options.database('plugin_deliveries').where({ kind: 'webhook' }).whereIn('status', UNFINISHED) + .modify(q => { if (this.held.size) void q.whereNotIn('id', [...this.held.keys()]); }) + .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }); + } + + /** + * Record the delivery and run it soon. Refused as `duplicate` when the + * installation already recorded this delivery id (a replay), and as `busy` + * when too many deliveries are held. + */ + async accept(job: PluginWebhookJob): Promise { + if (this.closed || this.held.size >= (this.options.maxPending ?? PLUGIN_WEBHOOK_LIMITS.pending)) return { accepted: false, reason: 'busy' }; + const now = this.now(); + const inserted = await this.options.database('plugin_deliveries').insert({ + plugin_id: job.target.pluginId, delivery_id: job.deliveryId, kind: 'webhook', repository: '', event_seq: 0, + event_type: `webhook.${job.input.webhook}`, handler: job.handler, status: 'pending', attempt: 0, next_attempt_at: now, created_at: now, logs: '[]', + }).onConflict(['plugin_id', 'delivery_id']).ignore().returning('id') as Array<{ id: number } | number>; + const first = inserted[0]; + if (first === undefined) return { accepted: false, reason: 'duplicate' }; + const recordId = typeof first === 'number' ? first : Number(first.id); + this.held.set(recordId, { job, recordId, attempt: 0 }); + this.schedule(recordId, 0); + return { accepted: true, recordId }; + } + + /** Wait until no attempt is under way or due at once: every held delivery is final or waits for a retry (tests). */ + async idle(): Promise { + while (this.running.size || this.due.size) { + if (this.running.size) await Promise.all([...this.running]); + else await new Promise(resolve => setImmediate(resolve)); + } + } + + /** Stop: no further attempt starts; attempts under way finish; deliveries still waiting are marked failed. */ + async stop(): Promise { + this.closed = true; + for (const held of this.held.values()) clearTimeout(held.timer); + this.due.clear(); + await this.idle(); + const ids = [...this.held.keys()]; + this.held.clear(); + if (ids.length) await this.options.database('plugin_deliveries').whereIn('id', ids).whereIn('status', UNFINISHED) + .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }).catch(() => undefined); + } + + private schedule(recordId: number, delayMs: number): void { + const held = this.held.get(recordId); + if (!held || this.closed) return; + if (delayMs <= 0) this.due.add(recordId); + held.timer = setTimeout(() => { + this.due.delete(recordId); + const attempt = this.attempt(held).catch((error: unknown) => { + logger.warn({ pluginId: held.job.target.pluginId, deliveryId: held.job.deliveryId, error: error instanceof Error ? error.message : String(error) }, '[plugins] Webhook delivery attempt failed to record'); + this.held.delete(recordId); + }).finally(() => { this.running.delete(attempt); }); + this.running.add(attempt); + }, delayMs); + held.timer.unref?.(); + } + + /** The installation as it is now, if it may still receive this delivery: installed as the same installation and enabled. */ + private async current(held: Held): Promise { + const { target } = held.job; + const row = await this.options.database('plugin_installations').where({ plugin_id: target.pluginId }).first() as PluginInstallationRow | undefined; + if (!row || !row.enabled || Number(row.installed_at) !== target.record.installedAt) return null; + return { ...target, record: installationRecord(row) }; + } + + private async finish(held: Held, update: Partial): Promise { + await this.options.database('plugin_deliveries').where({ id: held.recordId }).update(update); + } + + private async attempt(held: Held): Promise { + if (this.closed) return; + const { database } = this.options; + const target = await this.current(held); + const previous = await database('plugin_deliveries').where({ id: held.recordId }).first('status') as Pick | undefined; + // Uninstalled (the record is gone) or finished by someone else (enabled again marks unfinished deliveries failed). + if (!previous || (previous.status !== 'pending' && previous.status !== 'retrying')) { this.held.delete(held.recordId); return; } + if (!target) { + await this.finish(held, { status: 'failed', next_attempt_at: null, finished_at: this.now(), error: 'Not delivered: the plugin was disabled' }); + this.held.delete(held.recordId); + return; + } + const attempt = held.attempt + 1; + const marked = await database('plugin_deliveries').where({ id: held.recordId, status: previous.status }) + .update({ status: 'running', attempt, started_at: this.now(), finished_at: null }); + if (marked !== 1) { this.held.delete(held.recordId); return; } + held.attempt = attempt; + let run: PluginHandlerRun; + try { + run = await (this.options.run ?? runPluginHandler)(target, held.job.handler, { ...held.job.input, kind: 'webhook', deliveryId: held.job.deliveryId, attempt }, { ...this.options, database }); + } catch (error) { + run = { outcome: 'failed', timedOut: false, durationMs: 0, error: error instanceof Error ? error.message : 'The handler failed', logs: [] }; + } + const now = this.now(); + const status = run.outcome === 'succeeded' ? 'succeeded' : attempt > this.delays.length ? 'failed' : 'retrying'; + const nextAttemptAt = status === 'retrying' ? now + this.delays[attempt - 1] : null; + // Only while the record is still this attempt's: a restart (enabled again) or an uninstall since then wins. + const recorded = await database('plugin_deliveries').where({ id: held.recordId, status: 'running', attempt }).update({ + status, finished_at: now, duration_ms: run.durationMs, timed_out: run.timedOut, error: clipError(run.error), logs: storedLogs(run.logs), next_attempt_at: nextAttemptAt, + }); + if (recorded !== 1 || status !== 'retrying') { this.held.delete(held.recordId); return; } + this.schedule(held.recordId, nextAttemptAt! - now); + } +} diff --git a/packages/core/src/plugins/pluginWebhooks.ts b/packages/core/src/plugins/pluginWebhooks.ts new file mode 100644 index 000000000..51c24b4ee --- /dev/null +++ b/packages/core/src/plugins/pluginWebhooks.ts @@ -0,0 +1,213 @@ +import { createHash, createHmac, randomBytes, timingSafeEqual } from 'node:crypto'; +import type { IncomingHttpHeaders } from 'node:http'; +import type { Knex } from 'knex'; +import { + PLUGIN_WEBHOOK_LIMITS, pluginWebhookVerificationHeaders, + type PluginManifest, type PluginWebhook, type PluginWebhookScheme, type PluginWebhookVerification, +} from '@propr/shared'; +import { PluginError, installationRecord, type PluginInstallationRow } from './pluginState.js'; +import type { PluginInstallationTarget } from './pluginContext.js'; + +/** + * Inbound plugin webhooks: the verification schemes, the installation key in + * the webhook URL and the lookup of a request's installation. ProPR verifies a + * request before any plugin code runs; request bodies and signature headers + * are never stored or logged (only a count of rejected requests is kept). + */ + +/** The public path of a plugin webhook. */ +export const PLUGIN_WEBHOOK_ROUTE = '/api/plugins/:installationKey/hooks/:webhookName' as const; +export const pluginWebhookPath = (key: string, name: string) => `/api/plugins/${key}/hooks/${name}`; + +/** 32 random bytes, base64url: 43 characters. */ +const KEY_PATTERN = /^[A-Za-z0-9_-]{43}$/; +const WEBHOOK_NAME = /^[a-z][a-z0-9_-]{0,47}$/; +export const generatePluginWebhookKey = () => randomBytes(32).toString('base64url'); +export const pluginWebhookKeyHash = (key: string) => createHash('sha256').update(key).digest('hex'); + +/** Why a request failed verification; counted, never logged with the request. */ +export type PluginWebhookRejection = 'missing_signature' | 'bad_signature' | 'stale_timestamp' | 'no_secret'; +export type PluginWebhookVerdict = { ok: true } | { ok: false; reason: PluginWebhookRejection }; + +/** One header value as text; a repeated header is not a valid signature. */ +export function headerValue(headers: IncomingHttpHeaders, name: string): string | undefined { + const value = headers[name]; + return typeof value === 'string' ? value : undefined; +} + +/** Equal bytes, in time independent of where they differ. Different lengths are unequal (lengths are public: digests). */ +const sameBytes = (a: Buffer, b: Buffer) => a.length === b.length && timingSafeEqual(a, b); +/** Equal strings in constant time: SHA-256 of both, so neither length nor content leaks. */ +const sameSecret = (a: string, b: string) => timingSafeEqual(createHash('sha256').update(a).digest(), createHash('sha256').update(b).digest()); +const hmac = (secret: string, ...parts: (string | Buffer)[]) => { + const mac = createHmac('sha256', secret); + for (const part of parts) mac.update(part); + return mac.digest(); +}; +/** The digest a header carries in `encoding`; null when it is not that encoding. */ +function decodeDigest(text: string, encoding: 'hex' | 'base64'): Buffer | null { + if (encoding === 'hex') return /^[0-9a-fA-F]{64}$/.test(text) ? Buffer.from(text, 'hex') : null; + return /^[A-Za-z0-9+/]{43}=$/.test(text) ? Buffer.from(text, 'base64') : null; +} + +export interface VerifyPluginWebhookInput { + verification: PluginWebhookVerification; + /** The `signing_secret` connection's value. */ + secret: string; + headers: IncomingHttpHeaders; + /** The raw request body, exactly as received. */ + body: Buffer; + /** Wall clock, for the timestamp tolerance. */ + now: number; +} + +/** + * Verify a request against one of the built-in schemes: + * - `hmac-sha256`: `
: ` in hex or base64. + * - `slack-v0`: `X-Slack-Signature: v0=`, + * with the timestamp within `PLUGIN_WEBHOOK_LIMITS.timestampToleranceSeconds` of now. + * - `token-header`: `
: `, compared in constant time. + */ +export function verifyPluginWebhook({ verification, secret, headers, body, now }: VerifyPluginWebhookInput): PluginWebhookVerdict { + const fail = (reason: PluginWebhookRejection): PluginWebhookVerdict => ({ ok: false, reason }); + if (verification.scheme === 'slack-v0') { + const timestamp = headerValue(headers, 'x-slack-request-timestamp'); + const signature = headerValue(headers, 'x-slack-signature'); + if (!timestamp || !signature) return fail('missing_signature'); + if (!/^[0-9]{1,12}$/.test(timestamp)) return fail('bad_signature'); + if (Math.abs(now / 1000 - Number(timestamp)) > PLUGIN_WEBHOOK_LIMITS.timestampToleranceSeconds) return fail('stale_timestamp'); + const given = signature.startsWith('v0=') ? decodeDigest(signature.slice(3), 'hex') : null; + return given && sameBytes(given, hmac(secret, `v0:${timestamp}:`, body)) ? { ok: true } : fail('bad_signature'); + } + const value = headerValue(headers, verification.header); + if (value === undefined) return fail('missing_signature'); + const prefix = verification.prefix ?? ''; + if (verification.scheme === 'token-header') return sameSecret(value, prefix + secret) ? { ok: true } : fail('bad_signature'); + if (!value.startsWith(prefix)) return fail('bad_signature'); + const given = decodeDigest(value.slice(prefix.length), verification.encoding); + return given && sameBytes(given, hmac(secret, body)) ? { ok: true } : fail('bad_signature'); +} + +/** Request headers never handed to a handler: credentials of the HTTP layer. Verification headers are removed too. */ +const WITHHELD_HEADERS = new Set(['authorization', 'proxy-authorization', 'cookie', 'host', 'connection', 'content-length', 'transfer-encoding']); +/** The headers a handler receives: lower-case names, without the verification headers and HTTP credentials. */ +export function handlerHeaders(headers: IncomingHttpHeaders, webhook: PluginWebhook): Record { + const withheld = new Set([...WITHHELD_HEADERS, ...pluginWebhookVerificationHeaders(webhook.verification)]); + const kept: Record = {}; + for (const [name, value] of Object.entries(headers)) { + if (value === undefined || withheld.has(name.toLowerCase())) continue; + kept[name.toLowerCase()] = Array.isArray(value) ? value.join(', ') : value; + } + return kept; +} + +/** The sender's delivery id from `deliveryIdHeader`: undefined without one, null when it is not 1 to 200 printable characters. */ +export function senderDeliveryId(headers: IncomingHttpHeaders, webhook: PluginWebhook): string | null | undefined { + if (!webhook.deliveryIdHeader) return undefined; + const value = headers[webhook.deliveryIdHeader]; + if (value === undefined) return undefined; + return typeof value === 'string' && new RegExp(`^[\\x21-\\x7e]{1,${PLUGIN_WEBHOOK_LIMITS.deliveryIdChars}}$`).test(value) ? value : null; +} + +/** A `plugin_webhook_keys` row as stored. */ +interface PluginWebhookKeyRow { + plugin_id: string; installed_at: number | string; key_hash: string; key: string; created_by: string; created_at: number | string; + rejected_count: number; last_rejected_at: number | string | null; last_rejected_reason: string | null; +} + +export interface PluginWebhookEndpoint { + name: string; description: string; scheme: PluginWebhookScheme; + /** `/api/plugins//hooks/`. */ + path: string; + /** The full URL when the instance has a public base URL; otherwise null. */ + url: string | null; +} +export interface PluginWebhookEndpoints { + webhooks: PluginWebhookEndpoint[]; + /** Requests refused because they failed verification. */ + rejected: { count: number; lastAt: number | null; lastReason: string | null }; + /** When the current key was created, and by whom. */ + keyCreatedAt: number | null; keyCreatedBy: string | null; +} + +async function installedRow(database: Knex, pluginId: string): Promise { + const row = await database('plugin_installations').where({ plugin_id: pluginId }).first() as PluginInstallationRow | undefined; + if (!row) throw new PluginError(409, 'The plugin is not installed'); + return row; +} + +// eslint-disable-next-line max-params -- the transaction, plugin, installation, actor and time are explicit inputs +async function writeKey(trx: Knex, pluginId: string, installedAt: number, actor: string, now: number): Promise { + const key = generatePluginWebhookKey(); + await trx('plugin_webhook_keys').insert({ + plugin_id: pluginId, installed_at: installedAt, key_hash: pluginWebhookKeyHash(key), key, created_by: actor, created_at: now, + rejected_count: 0, last_rejected_at: null, last_rejected_reason: null, + }).onConflict('plugin_id').merge(); +} + +/** The installation's key row, created on first use; a row left by an earlier installation is replaced. */ +async function currentKey(database: Knex, pluginId: string, now: number): Promise { + return database.transaction(async trx => { + const row = await installedRow(trx, pluginId); + const existing = await trx('plugin_webhook_keys').where({ plugin_id: pluginId }).first() as PluginWebhookKeyRow | undefined; + if (!existing || Number(existing.installed_at) !== Number(row.installed_at)) await writeKey(trx, pluginId, Number(row.installed_at), 'system', now); + return await trx('plugin_webhook_keys').where({ plugin_id: pluginId }).first() as PluginWebhookKeyRow; + }); +} + +function endpoints(manifest: PluginManifest | undefined, row: PluginWebhookKeyRow, origin: string | null): PluginWebhookEndpoints { + return { + webhooks: (manifest?.contributes.webhooks ?? []).map(webhook => { + const path = pluginWebhookPath(row.key, webhook.name); + return { name: webhook.name, description: webhook.description, scheme: webhook.verification.scheme, path, url: origin ? `${origin}${path}` : null }; + }), + rejected: { count: Number(row.rejected_count), lastAt: row.last_rejected_at === null ? null : Number(row.last_rejected_at), lastReason: row.last_rejected_reason }, + keyCreatedAt: Number(row.created_at), keyCreatedBy: row.created_by, + }; +} + +export interface PluginWebhookEndpointOptions { manifest: PluginManifest | undefined; origin: string | null; now?: number } +/** The installed plugin's webhook URLs and its count of rejected requests. Refused (409) when it is not installed. */ +export async function pluginWebhookEndpoints(database: Knex, pluginId: string, options: PluginWebhookEndpointOptions): Promise { + return endpoints(options.manifest, await currentKey(database, pluginId, options.now ?? Date.now()), options.origin); +} + +/** Replace the installation key: every old webhook URL stops working at once. Recorded in the plugin's audit trail. */ + +export async function regeneratePluginWebhookKey(database: Knex, pluginId: string, actor: string, options: PluginWebhookEndpointOptions): Promise { + const now = options.now ?? Date.now(); + const row = await database.transaction(async trx => { + const installation = await installedRow(trx, pluginId); + await writeKey(trx, pluginId, Number(installation.installed_at), actor, now); + await trx('plugin_audit').insert({ plugin_id: pluginId, action: 'regenerate_webhook_key', actor, at: now, detail: JSON.stringify({ webhooks: options.manifest?.contributes.webhooks.map(w => w.name) ?? [] }) }); + return await trx('plugin_webhook_keys').where({ plugin_id: pluginId }).first() as PluginWebhookKeyRow; + }); + return endpoints(options.manifest, row, options.origin); +} + +/** Delete the installation key (uninstall): the URLs stop working and a reinstall gets a new key. */ +export async function deletePluginWebhookKey(trx: Knex, pluginId: string): Promise { + return trx('plugin_webhook_keys').where({ plugin_id: pluginId }).delete(); +} + +/** Count a request that failed verification. Never fails the caller. */ +export async function recordPluginWebhookRejection(database: Knex, pluginId: string, reason: PluginWebhookRejection, now: number): Promise { + await database('plugin_webhook_keys').where({ plugin_id: pluginId }) + .update({ rejected_count: database.raw('rejected_count + 1'), last_rejected_at: now, last_rejected_reason: reason }).catch(() => undefined); +} + +/** The webhook a request addresses: an enabled installation of a registry plugin, its current key and a declared webhook name. */ +export interface ResolvedPluginWebhook { target: PluginInstallationTarget; webhook: PluginWebhook } +/** Null for an unknown or regenerated key, an uninstalled, reinstalled or disabled plugin, or an undeclared webhook: the route answers 404. */ + +export async function resolvePluginWebhook(database: Knex, key: string, name: string, manifests: readonly PluginManifest[]): Promise { + if (!KEY_PATTERN.test(key) || !WEBHOOK_NAME.test(name)) return null; + const keyRow = await database('plugin_webhook_keys').where({ key_hash: pluginWebhookKeyHash(key) }).first() as PluginWebhookKeyRow | undefined; + if (!keyRow) return null; + const row = await database('plugin_installations').where({ plugin_id: keyRow.plugin_id }).first() as PluginInstallationRow | undefined; + if (!row || !row.enabled || Number(row.installed_at) !== Number(keyRow.installed_at)) return null; + const manifest = manifests.find(m => m.id === row.plugin_id); + const webhook = manifest?.contributes.webhooks.find(w => w.name === name); + if (!manifest || !webhook) return null; + return { target: { pluginId: row.plugin_id, manifest, record: installationRecord(row) }, webhook }; +} diff --git a/packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts b/packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts new file mode 100644 index 000000000..ddb3e458d --- /dev/null +++ b/packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts @@ -0,0 +1,45 @@ +import type { PluginContext, PluginWebhookInput, PluginWebhookRequest, PluginWebhookResponse } from '@propr/shared'; + +/** + * Test fixture handlers for packages/core/test/pluginWebhooks.test.ts and + * packages/api/test/pluginWebhookRoutes.test.ts. Each delivery is appended to + * `ctx.state` key `seen` (delivery id, webhook, attempt, the body's length and + * the header names it received; never the body itself), so a test sees what + * ran and how often. Imports only @propr/shared types. + */ +type Seen = { id: string; webhook: string; attempt: number; bodyBytes: number; headers: string[] }[]; + +export async function onWebhook(input: PluginWebhookInput, ctx: PluginContext) { + const seen = (await ctx.state.get('seen')) ?? []; + seen.push({ id: input.deliveryId, webhook: input.webhook, attempt: input.attempt, bodyBytes: Buffer.byteLength(input.body), headers: Object.keys(input.headers).sort() }); + await ctx.state.set('seen', seen); + ctx.log.info(`received webhook ${input.webhook}`, { attempt: input.attempt }); +} + +/** Fails while `fail` (a count of attempts still to fail) is above zero. */ +export async function flakyWebhook(input: PluginWebhookInput, ctx: PluginContext) { + await onWebhook(input, ctx); + const remaining = (await ctx.state.get('fail')) ?? 0; + if (remaining > 0) { + await ctx.state.set('fail', remaining - 1); + throw new Error(`webhook failure, attempt ${input.attempt}`); + } +} + +/** Slack's `url_verification`: echo the challenge at once; any other event goes to the handler. */ +export function slackChallenge(request: PluginWebhookRequest): PluginWebhookResponse | null { + let body: { type?: unknown; challenge?: unknown }; + try { body = JSON.parse(request.body) as typeof body; } catch { return null; } + return body.type === 'url_verification' && typeof body.challenge === 'string' ? { contentType: 'text/plain', body: body.challenge } : null; +} + +const probes: { args: number; keys: string[]; frozen: boolean }[] = []; +/** Records what it was called with, then reaches for a `ctx` it never gets. */ +export function probeRespond(request: PluginWebhookRequest, ...rest: unknown[]): PluginWebhookResponse { + probes.push({ args: 1 + rest.length, keys: Object.keys(request).sort(), frozen: Object.isFrozen(request) && Object.isFrozen(request.headers) }); + const ctx = rest[0] as PluginContext; + void ctx.state.set('respond', true); + return { body: 'unreachable' }; +} +/** What `probeRespond` saw, for the tests. */ +export function respondProbes() { return probes; } diff --git a/packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json b/packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json new file mode 100644 index 000000000..e270b7989 --- /dev/null +++ b/packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json @@ -0,0 +1,34 @@ +{ + "manifestVersion": 1, + "id": "fixture-webhooks", + "name": "Fixture webhooks", + "description": "Test fixture for plugin inbound webhooks: one webhook per verification scheme, a Slack challenge responder, a failing handler and a respond function that reaches for ctx.", + "version": "1.0.0", + "category": "other", + "homepage": "https://example.com/fixture-webhooks", + "icon": "icon.svg", + "connections": [ + { "type": "signing_secret", "name": "hook-secret", "label": "Signing secret", "help": "Verifies inbound webhooks.", "helpUrl": "https://example.com/docs/webhooks" } + ], + "contributes": { + "webhooks": [ + { + "name": "signed", "description": "HMAC-SHA256 signed, as GitHub signs", "handler": "onWebhook", + "verification": { "scheme": "hmac-sha256", "header": "X-Hub-Signature-256", "prefix": "sha256=", "connection": "hook-secret" }, + "deliveryIdHeader": "X-GitHub-Delivery" + }, + { + "name": "slack", "description": "Slack Events API", "handler": "onWebhook", "respond": "slackChallenge", + "verification": { "scheme": "slack-v0", "connection": "hook-secret" } + }, + { + "name": "token", "description": "Constant token header, as GitLab sends", "handler": "flakyWebhook", + "verification": { "scheme": "token-header", "header": "X-Gitlab-Token", "connection": "hook-secret" } + }, + { + "name": "probe", "description": "A respond function that reaches for ctx", "handler": "onWebhook", "respond": "probeRespond", + "verification": { "scheme": "token-header", "header": "X-Gitlab-Token", "connection": "hook-secret" } + } + ] + } +} diff --git a/packages/core/test/fixtures/plugins/registry.json b/packages/core/test/fixtures/plugins/registry.json index 1c546a5fa..f7c48b60a 100644 --- a/packages/core/test/fixtures/plugins/registry.json +++ b/packages/core/test/fixtures/plugins/registry.json @@ -2,6 +2,7 @@ "version": 1, "plugins": [ { "id": "fixture-handlers", "development": true }, - { "id": "fixture-events", "development": true } + { "id": "fixture-events", "development": true }, + { "id": "fixture-webhooks", "development": true } ] } diff --git a/packages/core/test/helpers/pluginHandlerFixture.ts b/packages/core/test/helpers/pluginHandlerFixture.ts index 8ff6ae631..4a70403e6 100644 --- a/packages/core/test/helpers/pluginHandlerFixture.ts +++ b/packages/core/test/helpers/pluginHandlerFixture.ts @@ -13,6 +13,7 @@ import { up as stateMigration } from '../../src/db/migrations/20261114180000_plu import { up as journalMigration } from '../../src/db/migrations/20261112160000_create_repository_events.js'; import { up as taskDomainMigration } from '../../src/db/migrations/20261114120000_add_task_repository_event_domain.js'; import { up as deliveriesMigration } from '../../src/db/migrations/20261115000000_plugin_event_deliveries.js'; +import { up as webhookKeysMigration } from '../../src/db/migrations/20261116000000_plugin_webhook_keys.js'; import { PluginStore } from '../../src/plugins/pluginStore.js'; import type { PluginProprBackend } from '../../src/plugins/pluginProprClient.js'; import type { PluginHttpTransport, PluginHttpTransportRequest } from '../../src/plugins/pluginHttp.js'; @@ -82,7 +83,7 @@ export async function pluginHandlerFixture(t: TestContext, options: { secret?: s await database.schema.createTable('instance_members', c => { c.string('github_user_id').primary(); c.string('role'); }); await database('instance_members').insert({ github_user_id: '1', role: 'admin' }); await database.schema.createTable('agent_definitions', c => { c.string('id').primary(); c.string('name'); }); - for (const migrate of [identityMigration, grantMigration, libraryMigration, oauthMigration, pluginMigration, stateMigration, journalMigration, taskDomainMigration, deliveriesMigration]) await migrate(database); + for (const migrate of [identityMigration, grantMigration, libraryMigration, oauthMigration, pluginMigration, stateMigration, journalMigration, taskDomainMigration, deliveriesMigration, webhookKeysMigration]) await migrate(database); const pluginId = options.pluginId ?? FIXTURE_ID; const manifest = fixtureManifest(pluginId); const catalog = async () => [manifest]; diff --git a/packages/core/test/pluginContributions.test.ts b/packages/core/test/pluginContributions.test.ts index 1cd457e70..4142905d9 100644 --- a/packages/core/test/pluginContributions.test.ts +++ b/packages/core/test/pluginContributions.test.ts @@ -17,6 +17,7 @@ import { up as pluginMigration } from '../src/db/migrations/20261114000000_plugi import { up as pluginStateMigration } from '../src/db/migrations/20261114180000_plugin_state.js'; import { up as journalMigration } from '../src/db/migrations/20261112160000_create_repository_events.js'; import { up as deliveriesMigration } from '../src/db/migrations/20261115000000_plugin_event_deliveries.js'; +import { up as webhookKeysMigration } from '../src/db/migrations/20261116000000_plugin_webhook_keys.js'; import { PluginError, PluginStore } from '../src/plugins/pluginStore.js'; import { PluginContributions } from '../src/plugins/pluginContributions.js'; import { officialPluginAssets, type PluginAssetSource } from '../src/plugins/pluginAssets.js'; @@ -65,7 +66,7 @@ async function fixture(t: TestContext, options: { advertise?: SyntheticOAuthOpti await database.schema.createTable('tasks', c => c.string('task_id').primary()); await database.schema.createTable('task_history', c => { c.string('task_id'); c.string('state'); c.string('timestamp'); c.text('reason'); c.text('metadata'); }); await database('instance_members').insert([{ github_user_id: '1', role: 'admin' }, { github_user_id: '2', role: 'member' }]); - await identityMigration(database); await grantMigration(database); await libraryMigration(database); await oauthMigration(database); await pluginMigration(database); await pluginStateMigration(database); await journalMigration(database); await deliveriesMigration(database); + await identityMigration(database); await grantMigration(database); await libraryMigration(database); await oauthMigration(database); await pluginMigration(database); await pluginStateMigration(database); await journalMigration(database); await deliveriesMigration(database); await webhookKeysMigration(database); const environment = { SYSTEM_TASK_SECRET: 'synthetic-plugin-custody', PROPR_EXTENSION_NPM_REGISTRY: registry.url, PROPR_EGRESS_SOCKET_DIR: path.join(root, 'egress'), PATH: process.env.PATH }; let manifests = [exampleManifest()]; const catalog = async () => manifests; diff --git a/packages/core/test/pluginStore.test.ts b/packages/core/test/pluginStore.test.ts index 4d1b575df..21bcdbe1c 100644 --- a/packages/core/test/pluginStore.test.ts +++ b/packages/core/test/pluginStore.test.ts @@ -8,6 +8,7 @@ import { up as pluginMigration } from '../src/db/migrations/20261114000000_plugi import { up as pluginStateMigration } from '../src/db/migrations/20261114180000_plugin_state.js'; import { up as journalMigration } from '../src/db/migrations/20261112160000_create_repository_events.js'; import { up as deliveriesMigration } from '../src/db/migrations/20261115000000_plugin_event_deliveries.js'; +import { up as webhookKeysMigration } from '../src/db/migrations/20261116000000_plugin_webhook_keys.js'; import { up as grantMigration } from '../src/db/migrations/20261013220000_extension_grant_network.js'; import { up as libraryMigration } from '../src/db/migrations/20261113000000_extension_library.js'; import { up as oauthMigration } from '../src/db/migrations/20261113120000_extension_oauth.js'; @@ -31,7 +32,7 @@ function upgraded(change: { permissions?: boolean; packageVersion?: string } = { const withSigningSecret = (): PluginManifest => pluginManifestSchema.parse({ ...structuredClone(example), connections: [...example.connections, { type: 'signing_secret', name: 'example-signing', label: 'Signing secret', help: 'The webhook signing secret.', helpUrl: 'https://example.com/docs/webhooks' }], - contributes: { ...structuredClone(example.contributes), webhooks: [{ name: 'events', description: 'Example events', connection: 'example-signing' }] }, + contributes: { ...structuredClone(example.contributes), webhooks: [{ name: 'events', description: 'Example events', handler: 'onEvent', verification: { scheme: 'token-header', header: 'x-example-token', connection: 'example-signing' } }] }, }); async function fixture(t: TestContext, initial: PluginManifest = example) { @@ -39,7 +40,7 @@ async function fixture(t: TestContext, initial: PluginManifest = example) { await database.schema.createTable('instance_members', c => { c.string('github_user_id').primary(); c.string('role'); }); await database('instance_members').insert([{ github_user_id: '1', role: 'admin' }, { github_user_id: '2', role: 'member' }]); await database.schema.createTable('agent_definitions', c => { c.string('id').primary(); c.string('name'); }); - await identityMigration(database); await grantMigration(database); await libraryMigration(database); await oauthMigration(database); await pluginMigration(database); await pluginStateMigration(database); await journalMigration(database); await deliveriesMigration(database); + await identityMigration(database); await grantMigration(database); await libraryMigration(database); await oauthMigration(database); await pluginMigration(database); await pluginStateMigration(database); await journalMigration(database); await deliveriesMigration(database); await webhookKeysMigration(database); let manifests: PluginManifest[] = [initial]; let clock = 1_700_000_000_000; const store = new PluginStore(database, { SYSTEM_TASK_SECRET: 'plugin-store-test-root' }, async () => manifests, () => clock); diff --git a/packages/core/test/pluginWebhooks.test.ts b/packages/core/test/pluginWebhooks.test.ts new file mode 100644 index 000000000..21ccdf108 --- /dev/null +++ b/packages/core/test/pluginWebhooks.test.ts @@ -0,0 +1,257 @@ +import test, { mock, type TestContext } from 'node:test'; +import assert from 'node:assert/strict'; +import { createHmac } from 'node:crypto'; +import type { IncomingHttpHeaders } from 'node:http'; +import { parsePluginManifest, type PluginWebhookVerification } from '@propr/shared'; +import { loadPluginHandlers } from '../../plugins/src/index.js'; +import { FIXTURE_REGISTRY, pluginHandlerFixture } from './helpers/pluginHandlerFixture.js'; +import { verifyPluginWebhook, pluginWebhookEndpoints, regeneratePluginWebhookKey } from '../src/plugins/pluginWebhooks.js'; +import { PluginWebhookRunner } from '../src/plugins/pluginWebhookRunner.js'; +import { PluginWebhookReceiver } from '../src/plugins/pluginWebhookReceiver.js'; +import { listPluginActivity } from '../src/plugins/pluginDeliveries.js'; +import logger from '../src/utils/logger.js'; + +const ID = 'fixture-webhooks'; +const SECRET = 'whsec_fixture-signing-secret'; +const NOW = 1_700_000_000_000; +const verify = (verification: PluginWebhookVerification, secret: string, headers: IncomingHttpHeaders, body: string, now = NOW) => + verifyPluginWebhook({ verification, secret, headers, body: Buffer.from(body), now }); +const scheme = (value: Record) => { + const parsed = parsePluginManifest({ + manifestVersion: 1, id: 'vectors', name: 'Vectors', description: 'Test vectors', version: '1.0.0', category: 'other', homepage: 'https://example.com', icon: 'icon.svg', + connections: [{ type: 'signing_secret', name: 'secret', label: 'Secret', help: 'Secret', helpUrl: 'https://example.com' }], + contributes: { webhooks: [{ name: 'hook', description: 'Hook', handler: 'run', verification: { ...value, connection: 'secret' } }] }, + }); + assert.ok(parsed.ok, parsed.ok ? '' : parsed.errors.join('; ')); + return parsed.manifest.contributes.webhooks[0].verification; +}; + +test('each verification scheme accepts its documented test vector and refuses a tampered one', () => { + // GitHub, "Validating webhook deliveries": secret "It's a Secret to Everybody", payload "Hello, World!". + const github = scheme({ scheme: 'hmac-sha256', header: 'X-Hub-Signature-256', prefix: 'sha256=' }); + const githubSignature = 'sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17'; + assert.deepEqual(verify(github, "It's a Secret to Everybody", { 'x-hub-signature-256': githubSignature }, 'Hello, World!'), { ok: true }); + assert.deepEqual(verify(github, "It's a Secret to Everybody", { 'x-hub-signature-256': githubSignature }, 'Hello, World?'), { ok: false, reason: 'bad_signature' }); + assert.deepEqual(verify(github, "It's a Secret to Everybody", { 'x-hub-signature-256': githubSignature.replace('sha256=', 'sha1=') }, 'Hello, World!'), { ok: false, reason: 'bad_signature' }); + assert.deepEqual(verify(github, 'another secret', { 'x-hub-signature-256': githubSignature }, 'Hello, World!'), { ok: false, reason: 'bad_signature' }); + assert.deepEqual(verify(github, "It's a Secret to Everybody", {}, 'Hello, World!'), { ok: false, reason: 'missing_signature' }); + assert.deepEqual(verify(github, "It's a Secret to Everybody", { 'x-hub-signature-256': [githubSignature, githubSignature] }, 'Hello, World!'), { ok: false, reason: 'missing_signature' }, 'a repeated header is no signature'); + + // RFC 4231 test case 2: key "Jefe", data "what do ya want for nothing?"; no prefix, hex and base64. + const rfcHex = '5bdcc146bf60754e6a042426089575c75a003f089d2739839dec58b964ec3843'; + assert.deepEqual(verify(scheme({ scheme: 'hmac-sha256', header: 'x-signature' }), 'Jefe', { 'x-signature': rfcHex }, 'what do ya want for nothing?'), { ok: true }); + assert.deepEqual(verify(scheme({ scheme: 'hmac-sha256', header: 'x-signature' }), 'Jefe', { 'x-signature': rfcHex.toUpperCase() }, 'what do ya want for nothing?'), { ok: true }); + const base64 = scheme({ scheme: 'hmac-sha256', header: 'x-signature', encoding: 'base64' }); + assert.deepEqual(verify(base64, 'Jefe', { 'x-signature': Buffer.from(rfcHex, 'hex').toString('base64') }, 'what do ya want for nothing?'), { ok: true }); + assert.deepEqual(verify(base64, 'Jefe', { 'x-signature': rfcHex }, 'what do ya want for nothing?'), { ok: false, reason: 'bad_signature' }, 'hex is not base64'); + + // Slack, "Verifying requests from Slack": the documented slash command request. + const slack = scheme({ scheme: 'slack-v0' }); + const slackSecret = '8f742231b10e8888abcd99yyyzzz85a5'; + const slackBody = 'token=xyzz0WbapA4vBCDEFasx0q6G&team_id=T1DC2JH3J&team_domain=testteamnow&channel_id=G8PSS9T3V&channel_name=foobar&user_id=U2CERLKJA&user_name=roadrunner&command=%2Fwebhook-collect&text=&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT1DC2JH3J%2F397700885554%2F96rGlfmibIGlgcZRskXaIFfN&trigger_id=398738663015.47445629121.803a0bc887a14d10d2c447fce8b6703c'; + const slackHeaders = { 'x-slack-request-timestamp': '1531420618', 'x-slack-signature': 'v0=a2114d57b48eac39b9ad189dd8316235a7b4a8d21a10bd27519666489c69b503' }; + const slackNow = 1531420618 * 1000; + assert.deepEqual(verify(slack, slackSecret, slackHeaders, slackBody, slackNow), { ok: true }); + assert.deepEqual(verify(slack, slackSecret, slackHeaders, slackBody, slackNow + 300_000), { ok: true }, 'five minutes is within the tolerance'); + assert.deepEqual(verify(slack, slackSecret, slackHeaders, slackBody, slackNow + 301_000), { ok: false, reason: 'stale_timestamp' }); + assert.deepEqual(verify(slack, slackSecret, slackHeaders, slackBody, slackNow - 301_000), { ok: false, reason: 'stale_timestamp' }, 'a timestamp from the future is refused too'); + assert.deepEqual(verify(slack, slackSecret, { ...slackHeaders, 'x-slack-request-timestamp': '1531420619' }, slackBody, slackNow), { ok: false, reason: 'bad_signature' }, 'the timestamp is signed'); + assert.deepEqual(verify(slack, slackSecret, slackHeaders, `${slackBody}&x=1`, slackNow), { ok: false, reason: 'bad_signature' }); + assert.deepEqual(verify(slack, slackSecret, { 'x-slack-signature': slackHeaders['x-slack-signature'] }, slackBody, slackNow), { ok: false, reason: 'missing_signature' }); + + // A token header, as GitLab's X-Gitlab-Token: the header is the secret itself (optionally behind a prefix). + const gitlab = scheme({ scheme: 'token-header', header: 'X-Gitlab-Token' }); + assert.deepEqual(verify(gitlab, 'glwt-3f9a1c', { 'x-gitlab-token': 'glwt-3f9a1c' }, '{}'), { ok: true }); + assert.deepEqual(verify(gitlab, 'glwt-3f9a1c', { 'x-gitlab-token': 'glwt-3f9a1' }, '{}'), { ok: false, reason: 'bad_signature' }); + assert.deepEqual(verify(gitlab, 'glwt-3f9a1c', { 'x-gitlab-token': 'glwt-3f9a1cc' }, '{}'), { ok: false, reason: 'bad_signature' }); + const bearer = scheme({ scheme: 'token-header', header: 'authorization', prefix: 'Bearer ' }); + assert.deepEqual(verify(bearer, 'glwt-3f9a1c', { authorization: 'Bearer glwt-3f9a1c' }, ''), { ok: true }); + assert.deepEqual(verify(bearer, 'glwt-3f9a1c', { authorization: 'glwt-3f9a1c' }, ''), { ok: false, reason: 'bad_signature' }); +}); + +const hubSignature = (body: string, secret = SECRET) => `sha256=${createHmac('sha256', secret).update(body).digest('hex')}`; +const slackSignature = (timestamp: string, body: string, secret = SECRET) => `v0=${createHmac('sha256', secret).update(`v0:${timestamp}:${body}`).digest('hex')}`; + +async function setup(t: TestContext, options: { retryDelaysMs?: number[] } = {}) { + const f = await pluginHandlerFixture(t, { pluginId: ID, repositories: null }); + await f.store.setConnection('1', ID, 'hook-secret', SECRET); + const clock = { now: NOW }; + const loads: string[] = []; + const handlers = (id: string) => { loads.push(id); return loadPluginHandlers(id, { root: FIXTURE_REGISTRY, includeDevelopment: true }); }; + const runner = new PluginWebhookRunner({ ...f.options, handlers, now: () => clock.now, timeoutMs: 500, retryDelaysMs: options.retryDelaysMs ?? [5, 5] }); + t.after(() => runner.stop()); + const receiver = new PluginWebhookReceiver({ database: f.database, runner, catalog: f.options.catalog, handlers, environment: f.options.environment, now: () => clock.now }); + const { webhooks } = await pluginWebhookEndpoints(f.database, ID, { manifest: f.manifest, origin: null }); + const key = webhooks[0].path.split('/')[3]; + const send = (name: string, body: string, headers: IncomingHttpHeaders, onKey = key) => receiver.receive({ key: onKey, name, headers, body: Buffer.from(body) }); + const seen = async () => { + const row = await f.database('plugin_state').where({ plugin_id: ID, state_key: 'seen' }).first() as { value: string } | undefined; + return row ? JSON.parse(row.value) as { id: string; webhook: string; attempt: number; bodyBytes: number; headers: string[] }[] : []; + }; + const deliveries = async () => (await listPluginActivity(f.database, ID)).deliveries; + const rejected = async () => Number((await f.database('plugin_webhook_keys').where({ plugin_id: ID }).first('rejected_count')).rejected_count); + return { f, clock, loads, runner, receiver, key, send, seen, deliveries, rejected }; +} + +test('a verified request runs its handler once; a bad signature, a stale timestamp and a replayed delivery id run no plugin code', async t => { + const s = await setup(t); + const body = '{"action":"opened"}'; + const accepted = await s.send('signed', body, { 'x-hub-signature-256': hubSignature(body), 'x-github-delivery': 'd-1', 'content-type': 'application/json', cookie: 'sid=1', authorization: 'Basic eA==' }); + assert.equal(accepted.status, 202); + assert.deepEqual(JSON.parse(accepted.body), { accepted: true, deliveryId: 'whk-signed-d-1' }); + await s.runner.idle(); + const seen = await s.seen(); + assert.equal(seen.length, 1); + assert.deepEqual(seen[0], { id: 'whk-signed-d-1', webhook: 'signed', attempt: 1, bodyBytes: Buffer.byteLength(body), headers: ['content-type', 'x-github-delivery'] }, + 'the handler gets the body, but not the signature, cookie or authorization headers'); + const [delivery] = await s.deliveries(); + assert.deepEqual({ kind: delivery.kind, status: delivery.status, eventType: delivery.eventType, handler: delivery.handler, attempt: delivery.attempt, deliveryId: delivery.deliveryId }, + { kind: 'webhook', status: 'succeeded', eventType: 'webhook.signed', handler: 'onWebhook', attempt: 1, deliveryId: 'whk-signed-d-1' }); + + const loadsBefore = s.loads.length; + const replay = await s.send('signed', body, { 'x-hub-signature-256': hubSignature(body), 'x-github-delivery': 'd-1' }); + assert.equal(replay.status, 409, 'a replayed delivery id is refused'); + const forged = await s.send('signed', body, { 'x-hub-signature-256': hubSignature(body, 'wrong secret'), 'x-github-delivery': 'd-2' }); + assert.equal(forged.status, 401); + const tampered = await s.send('signed', `${body} `, { 'x-hub-signature-256': hubSignature(body), 'x-github-delivery': 'd-3' }); + assert.equal(tampered.status, 401); + const unsigned = await s.send('signed', body, { 'x-github-delivery': 'd-4' }); + assert.equal(unsigned.status, 401); + const slackBody = '{"type":"event_callback"}'; + const stale = String(Math.floor(NOW / 1000) - 301); + const staleReply = await s.send('slack', slackBody, { 'x-slack-request-timestamp': stale, 'x-slack-signature': slackSignature(stale, slackBody) }); + assert.equal(staleReply.status, 401, 'a correctly signed but stale Slack request is refused'); + const malformed = await s.send('signed', body, { 'x-hub-signature-256': hubSignature(body), 'x-github-delivery': 'has space' }); + assert.equal(malformed.status, 400); + await s.runner.idle(); + assert.equal(s.loads.length, loadsBefore, 'no plugin module was loaded for any refused request'); + assert.equal((await s.seen()).length, 1, 'the handler ran exactly once'); + assert.equal((await s.deliveries()).length, 1); + assert.equal(await s.rejected(), 4, 'each unverified request is counted'); + const endpoints = await pluginWebhookEndpoints(s.f.database, ID, { manifest: s.f.manifest, origin: 'https://propr.example.com' }); + assert.deepEqual({ count: endpoints.rejected.count, lastReason: endpoints.rejected.lastReason }, { count: 4, lastReason: 'stale_timestamp' }); + assert.equal(endpoints.webhooks[0].url, `https://propr.example.com/api/plugins/${s.key}/hooks/signed`); + + const fresh = String(Math.floor(NOW / 1000) - 10); + const slack = await s.send('slack', slackBody, { 'x-slack-request-timestamp': fresh, 'x-slack-signature': slackSignature(fresh, slackBody) }); + assert.equal(slack.status, 202); + const noDeliveryHeader = await s.send('signed', body, { 'x-hub-signature-256': hubSignature(body) }); + assert.equal(noDeliveryHeader.status, 202, 'without a delivery id header ProPR assigns one'); + await s.runner.idle(); + assert.equal((await s.seen()).length, 3); +}); + +test('the synchronous challenge response answers at once, runs no handler and cannot reach ctx', async t => { + const s = await setup(t); + const body = JSON.stringify({ type: 'url_verification', challenge: '3eZbrw1aBm2rZgRNFdxV2595E9CY3gmdALWMmHkvFXO7tYXAYM8P' }); + const timestamp = String(Math.floor(NOW / 1000)); + const reply = await s.send('slack', body, { 'x-slack-request-timestamp': timestamp, 'x-slack-signature': slackSignature(timestamp, body) }); + assert.deepEqual(reply, { status: 200, contentType: 'text/plain', body: '3eZbrw1aBm2rZgRNFdxV2595E9CY3gmdALWMmHkvFXO7tYXAYM8P' }); + const unsigned = await s.send('slack', body, { 'x-slack-request-timestamp': timestamp, 'x-slack-signature': slackSignature(timestamp, body, 'wrong') }); + assert.equal(unsigned.status, 401, 'the challenge is answered only after verification'); + + const probe = await s.send('probe', '{"hello":1}', { 'x-gitlab-token': SECRET }); + assert.equal(probe.status, 500, 'the respond function failed: it got no ctx to call'); + const module = await loadPluginHandlers(ID, { root: FIXTURE_REGISTRY, includeDevelopment: true }); + assert.deepEqual((module!.respondProbes as unknown as () => unknown[])(), [{ args: 1, keys: ['body', 'headers', 'receivedAt', 'webhook'], frozen: true }], + 'it was called with the frozen request as its only argument'); + await s.runner.idle(); + assert.equal(await s.f.database('plugin_state').where({ plugin_id: ID, state_key: 'respond' }).first(), undefined); + assert.deepEqual(await s.seen(), [], 'no handler ran'); + assert.deepEqual(await s.deliveries(), [], 'nothing was queued'); +}); + +test('a disabled or uninstalled plugin answers 404, and regenerating the key invalidates the old URL', async t => { + const s = await setup(t); + const send = (key = s.key) => s.send('token', '{}', { 'x-gitlab-token': SECRET }, key); + assert.equal((await send()).status, 202); + await s.runner.idle(); + assert.equal((await s.send('missing', '{}', { 'x-gitlab-token': SECRET })).status, 404, 'an undeclared webhook'); + assert.equal((await send('A'.repeat(43))).status, 404, 'an unknown key'); + + const regenerated = await regeneratePluginWebhookKey(s.f.database, ID, '1', { manifest: s.f.manifest, origin: null }); + const newKey = regenerated.webhooks[0].path.split('/')[3]; + assert.notEqual(newKey, s.key); + assert.equal((await send()).status, 404, 'the old URL is dead'); + assert.equal((await send(newKey)).status, 202); + await s.runner.idle(); + const audit = await s.f.database('plugin_audit').where({ plugin_id: ID, action: 'regenerate_webhook_key' }).select('actor'); + assert.deepEqual(audit, [{ actor: '1' }]); + + await s.f.store.setEnabled('1', ID, false); + assert.equal((await send(newKey)).status, 404, 'a disabled plugin runs nothing'); + await s.f.store.setEnabled('1', ID, true); + assert.equal((await send(newKey)).status, 202, 'enabled again, the same URL works'); + await s.runner.idle(); + const ran = (await s.seen()).length; + await s.f.store.uninstall('1', ID, ID); + assert.equal((await send(newKey)).status, 404, 'an uninstalled plugin runs nothing'); + assert.equal(await s.f.database('plugin_webhook_keys').where({ plugin_id: ID }).first(), undefined, 'the key is deleted with the installation'); + await s.f.store.install('1', ID); + await s.f.store.setConnection('1', ID, 'hook-secret', SECRET); + await s.f.store.setEnabled('1', ID, true); + assert.equal((await send(newKey)).status, 404, 'a reinstall gets a new key'); + assert.notEqual((await pluginWebhookEndpoints(s.f.database, ID, { manifest: s.f.manifest, origin: null })).webhooks[0].path.split('/')[3], newKey); + assert.equal(ran, 3); +}); + +test('a failing webhook handler is retried on the delivery schedule, then marked failed; deliveries left by a stopped process are failed', async t => { + const s = await setup(t, { retryDelaysMs: [5, 5] }); + await s.f.database('plugin_state').insert({ plugin_id: ID, state_key: 'fail', value: '1', bytes: 1, updated_at: 0 }); + assert.equal((await s.send('token', '{"n":1}', { 'x-gitlab-token': SECRET })).status, 202); + const settle = async () => { for (let i = 0; i < 50 && (s.runner.pending || (await s.runner.idle(), s.runner.pending)); i++) await new Promise(r => setTimeout(r, 10)); }; + await settle(); + let [delivery] = await s.deliveries(); + assert.deepEqual({ status: delivery.status, attempt: delivery.attempt }, { status: 'succeeded', attempt: 2 }, 'the second attempt succeeds'); + assert.deepEqual((await s.seen()).map(x => x.attempt), [1, 2]); + assert.equal(new Set((await s.seen()).map(x => x.id)).size, 1, 'the delivery id is the same on every attempt'); + + await s.f.database('plugin_state').where({ plugin_id: ID, state_key: 'fail' }).update({ value: '10' }); + assert.equal((await s.send('token', '{"n":2}', { 'x-gitlab-token': SECRET })).status, 202); + await settle(); + [delivery] = await s.deliveries(); + assert.deepEqual({ status: delivery.status, attempt: delivery.attempt, error: delivery.error }, { status: 'failed', attempt: 3, error: 'webhook failure, attempt 3' }, 'three attempts with two retry delays, then failed'); + + const orphan = new PluginWebhookRunner({ ...s.f.options, retryDelaysMs: [60_000] }); + await s.f.database('plugin_deliveries').insert({ plugin_id: ID, delivery_id: 'whk-token-orphan', kind: 'webhook', repository: '', event_seq: 0, event_type: 'webhook.token', handler: 'flakyWebhook', status: 'retrying', attempt: 1, created_at: NOW, logs: '[]' }); + assert.equal(await orphan.recover(), 1); + const lost = (await s.deliveries()).find(d => d.deliveryId === 'whk-token-orphan')!; + assert.equal(lost.status, 'failed'); + assert.match(lost.error!, /not kept/); +}); + +test('request bodies and signature headers are never stored or logged (sentinel)', async t => { + const s = await setup(t); + const lines: string[] = []; + for (const level of ['trace', 'debug', 'info', 'warn', 'error', 'fatal'] as const) { + mock.method(logger, level, (...args: unknown[]) => { lines.push(JSON.stringify(args, (_k, v: unknown) => (v instanceof Error ? `${v.name}: ${v.message}` : v))); }); + } + for (const level of ['log', 'info', 'warn', 'error', 'debug'] as const) mock.method(console, level, (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + t.after(() => mock.restoreAll()); + const BODY = 'SENTINEL-BODY-5f0c2e'; + const TOKEN_GUESS = 'SENTINEL-TOKEN-91ab'; + const body = JSON.stringify({ text: BODY }); + const signature = hubSignature(body); + const forgedSignature = hubSignature(body, 'SENTINEL-WRONG'); + const timestamp = String(Math.floor(NOW / 1000)); + const slackSig = slackSignature(timestamp, body); + const replies = [ + await s.send('signed', body, { 'x-hub-signature-256': signature, 'x-github-delivery': 'sentinel-1' }), + await s.send('signed', body, { 'x-hub-signature-256': signature, 'x-github-delivery': 'sentinel-1' }), + await s.send('signed', body, { 'x-hub-signature-256': forgedSignature }), + await s.send('token', body, { 'x-gitlab-token': TOKEN_GUESS }), + await s.send('slack', body, { 'x-slack-request-timestamp': timestamp, 'x-slack-signature': slackSig }), + await s.send('probe', body, { 'x-gitlab-token': SECRET }), + ]; + assert.deepEqual(replies.map(r => r.status), [202, 409, 401, 401, 202, 500]); + await s.runner.idle(); + assert.equal((await s.seen()).length, 2); + assert.ok(lines.length > 0, 'the sentinel watched real log output'); + const tables = (await s.f.database.raw("SELECT name FROM sqlite_master WHERE type = 'table'") as { name: string }[]).map(row => row.name); + const dump: string[] = []; + for (const table of tables) dump.push(JSON.stringify(await s.f.database(table).select())); + for (const sentinel of [BODY, TOKEN_GUESS, signature, signature.slice(7), forgedSignature.slice(7), slackSig.slice(3), 'SENTINEL-WRONG']) { + assert.ok(!dump.some(text => text.includes(sentinel)), `${sentinel} is not stored in any table`); + assert.ok(!lines.some(line => line.includes(sentinel)), `${sentinel} is not logged`); + } +}); diff --git a/packages/shared/src/pluginDeliveries.ts b/packages/shared/src/pluginDeliveries.ts index 90d6c87dd..6653ca2f9 100644 --- a/packages/shared/src/pluginDeliveries.ts +++ b/packages/shared/src/pluginDeliveries.ts @@ -25,20 +25,25 @@ export const PLUGIN_ACTIVITY_LIMIT = 100; */ export const PLUGIN_DELIVERY_STATUSES = ['pending', 'running', 'retrying', 'succeeded', 'failed', 'skipped'] as const; export type PluginDeliveryStatus = typeof PLUGIN_DELIVERY_STATUSES[number]; -/** `event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered. */ -export const PLUGIN_DELIVERY_KINDS = ['event', 'gap'] as const; +/** + * `event`: one journal event to one handler. `gap`: journal retention removed + * events before they were delivered. `webhook`: one verified inbound webhook + * request to its handler (its body is held in memory only, never stored). + */ +export const PLUGIN_DELIVERY_KINDS = ['event', 'gap', 'webhook'] as const; export type PluginDeliveryKind = typeof PLUGIN_DELIVERY_KINDS[number]; export interface PluginDeliveryView { /** The activity record id, used by the retry route. */ id: number; - /** The stable delivery id handlers receive (`evt--`); the same on every attempt. */ + /** The stable delivery id handlers receive (`evt--`, `whk--`); the same on every attempt. */ deliveryId: string; kind: PluginDeliveryKind; + /** The event's repository; empty for a webhook delivery. */ repository: string; - /** The journal `seq` of the event; for a gap, the first `seq` that may be missing. */ + /** The journal `seq` of the event; for a gap, the first `seq` that may be missing; 0 for a webhook. */ eventSeq: number; - /** `.`; null for a gap. */ + /** `.`; `webhook.` for a webhook delivery; null for a gap. */ eventType: string | null; handler: string | null; status: PluginDeliveryStatus; diff --git a/packages/shared/src/pluginInstallations.ts b/packages/shared/src/pluginInstallations.ts index 71f691725..2c5d6d575 100644 --- a/packages/shared/src/pluginInstallations.ts +++ b/packages/shared/src/pluginInstallations.ts @@ -87,7 +87,7 @@ export interface PluginView { * (refused or failed) record a handler's `ctx.propr` calls with `plugin:` as the actor. */ export interface PluginAuditEvent { action: PluginAuditAction; actor: string; at: number; detail: Record } -export const PLUGIN_AUDIT_ACTIONS = ['install', 'update_settings', 'set_repositories', 'set_connection', 'clear_connection', 'enable', 'disable', 'approve', 'uninstall', 'start_oauth', 'complete_oauth', 'disconnect_oauth', 'set_oauth_client', 'clear_oauth_client', 'handler_attempt', 'handler_call', 'handler_refused', 'retry_delivery', 'resume_deliveries'] as const; +export const PLUGIN_AUDIT_ACTIONS = ['install', 'update_settings', 'set_repositories', 'set_connection', 'clear_connection', 'enable', 'disable', 'approve', 'uninstall', 'start_oauth', 'complete_oauth', 'disconnect_oauth', 'set_oauth_client', 'clear_oauth_client', 'handler_attempt', 'handler_call', 'handler_refused', 'retry_delivery', 'resume_deliveries', 'regenerate_webhook_key'] as const; export type PluginAuditAction = typeof PLUGIN_AUDIT_ACTIONS[number]; export interface PluginDetail extends PluginView { audit: PluginAuditEvent[] } diff --git a/packages/shared/src/pluginSdk.ts b/packages/shared/src/pluginSdk.ts index 19c9bf709..610b9dff4 100644 --- a/packages/shared/src/pluginSdk.ts +++ b/packages/shared/src/pluginSdk.ts @@ -145,7 +145,15 @@ export interface PluginContext { * counts from 1. */ export interface PluginEventInput { kind: 'event'; event: RepositoryEvent; deliveryId: string; attempt: number } -export interface PluginWebhookInput { kind: 'webhook'; webhook: string; headers: Record; body: string; receivedAt: number } +/** + * A verified inbound webhook request delivered to its handler. `headers` are + * the request's headers (lower-case names) without the verification headers, + * cookies and authorization; `body` is the raw body as text. Neither is + * stored by ProPR. `deliveryId` is the same on every attempt: the sender's + * delivery id when the webhook declares `deliveryIdHeader`, otherwise one ProPR + * assigns. `attempt` counts from 1. + */ +export interface PluginWebhookInput { kind: 'webhook'; webhook: string; headers: Record; body: string; receivedAt: number; deliveryId: string; attempt: number } export interface PluginCommandInput { kind: 'command'; command: string; args: string; repository: string | null; actor: string } export interface PluginInvokeInput { kind: 'invoke'; payload: PluginJsonValue } export type PluginHandlerInput = PluginEventInput | PluginWebhookInput | PluginCommandInput | PluginInvokeInput; @@ -157,6 +165,29 @@ export type PluginHandlerModule = Record; /** Declare a handler with typed input; identity at runtime. */ export const definePluginHandler = (handler: PluginHandler): PluginHandler => handler; +/** What a webhook's `respond` function receives: the verified request, frozen. It gets no `ctx`. */ +export interface PluginWebhookRequest { webhook: string; headers: Readonly>; body: string; receivedAt: number } +/** A synchronous answer: sent as the response, and the handler does not run. */ +export interface PluginWebhookResponse { + /** 200 by default; a 2xx status. */ + status?: number; + /** `text/plain` by default; `application/json` for a JSON body. */ + contentType?: 'text/plain' | 'application/json'; + /** At most `PLUGIN_WEBHOOK_LIMITS.responseBytes`. */ + body: string; +} +/** + * A webhook's `respond` function: a pure, synchronous function of the + * request, for services that require an immediate challenge echo (Slack's + * `url_verification`). Return a response to answer with it (the handler does + * not run), or null to deliver the request to the handler as usual. + */ +export type PluginWebhookResponder = (request: PluginWebhookRequest) => PluginWebhookResponse | null | undefined; +/** Declare a webhook `respond` function; identity at runtime. */ +export const definePluginWebhookResponder = (responder: PluginWebhookResponder): PluginWebhookResponder => responder; +/** Limits of inbound webhooks: the raw body size, the response a `respond` function may give, the delivery id length and Slack's timestamp tolerance. */ +export const PLUGIN_WEBHOOK_LIMITS = { bodyBytes: 256 * 1024, responseBytes: 4 * 1024, deliveryIdChars: 200, timestampToleranceSeconds: 5 * 60, pending: 1_000 } as const; + /** What `runPluginHandler` returns; it never throws. */ export interface PluginHandlerRun { outcome: 'succeeded' | 'failed'; diff --git a/packages/shared/src/plugins.ts b/packages/shared/src/plugins.ts index 48e900307..698d08ae9 100644 --- a/packages/shared/src/plugins.ts +++ b/packages/shared/src/plugins.ts @@ -187,8 +187,44 @@ export const pluginSubscriptionSchema = z.strictObject({ filter: pluginSubscriptionFilterSchema.optional(), }); export type PluginSubscription = z.infer; -/** Accepted and validated now; ignored at runtime until webhooks land. Each webhook is verified with a `signing_secret` connection. */ -export const pluginWebhookSchema = z.strictObject({ name: keySchema, description: textSchema(200), connection: keySchema }); +/** An HTTP header name, matched without letter case (stored lower-case). */ +export const pluginHeaderNameSchema = z.string().regex(/^[A-Za-z0-9][A-Za-z0-9-]{0,63}$/, 'Enter an HTTP header name such as x-signature').transform(name => name.toLowerCase()); +const headerPrefixSchema = z.string().min(1).max(32).regex(/^[\x20-\x7e]+$/, 'A prefix is printable ASCII'); +/** Headers a webhook never names: ProPR or the HTTP layer own them. */ +const RESERVED_WEBHOOK_HEADERS = ['host', 'content-length', 'content-type', 'cookie', 'connection', 'transfer-encoding']; +/** Inbound webhook verification schemes; ProPR verifies a request before any plugin code runs. */ +export const PLUGIN_WEBHOOK_SCHEMES = ['hmac-sha256', 'slack-v0', 'token-header'] as const; +export type PluginWebhookScheme = typeof PLUGIN_WEBHOOK_SCHEMES[number]; +/** + * How ProPR verifies an inbound webhook, with a `signing_secret` connection: + * - `hmac-sha256`: `header` carries `prefix` + the HMAC-SHA256 of the raw body (`hex` or `base64`), as GitHub's `X-Hub-Signature-256: sha256=…`. + * - `slack-v0`: Slack's `X-Slack-Signature: v0=` + HMAC-SHA256 of `v0::`; the timestamp must be within 5 minutes. + * - `token-header`: `header` carries `prefix` + the secret itself, compared in constant time (as GitLab's `X-Gitlab-Token`). + */ +export const pluginWebhookVerificationSchema = z.discriminatedUnion('scheme', [ + z.strictObject({ scheme: z.literal('hmac-sha256'), header: pluginHeaderNameSchema, prefix: headerPrefixSchema.optional(), encoding: z.enum(['hex', 'base64']).default('hex'), connection: keySchema }), + z.strictObject({ scheme: z.literal('slack-v0'), connection: keySchema }), + z.strictObject({ scheme: z.literal('token-header'), header: pluginHeaderNameSchema, prefix: headerPrefixSchema.optional(), connection: keySchema }), +]); +export type PluginWebhookVerification = z.infer; +/** + * An inbound webhook: `POST /api/plugins//hooks/`. + * A verified request runs `handler` from the delivery queue. `deliveryIdHeader` + * names the header carrying the sender's delivery id, used to refuse replays. + * `respond` names an exported function of `handlers.ts` that may answer the + * request at once (a challenge echo): it receives the request only, never `ctx`. + */ +export const pluginWebhookSchema = z.strictObject({ + name: keySchema, description: textSchema(200), handler: pluginHandlerNameSchema, + verification: pluginWebhookVerificationSchema, + deliveryIdHeader: pluginHeaderNameSchema.optional(), + respond: pluginHandlerNameSchema.optional(), +}); +export type PluginWebhook = z.infer; +/** The headers a webhook's verification reads: never stored, logged or handed to the handler. */ +export function pluginWebhookVerificationHeaders(verification: PluginWebhookVerification): string[] { + return verification.scheme === 'slack-v0' ? ['x-slack-signature', 'x-slack-request-timestamp'] : [verification.header]; +} export const pluginContributesSchema = z.strictObject({ mcpServers: z.array(pluginMcpServerSchema).max(PLUGIN_LIMITS.mcpServers).default([]), @@ -223,7 +259,14 @@ function checkConnectionReferences(manifest: z.infer e.name), ...server.secretEnv.map(e => e.name)]; for (const name of duplicates(names)) issue(`Variable ${name} of server ${server.name} is listed more than once`, [...at, 'env']); }); - manifest.contributes.webhooks.forEach((webhook, index) => expect(webhook.connection, 'signing_secret', ['contributes', 'webhooks', index, 'connection'], `Webhook ${webhook.name}`)); + manifest.contributes.webhooks.forEach((webhook, index) => { + const at = ['contributes', 'webhooks', index]; + expect(webhook.verification.connection, 'signing_secret', [...at, 'verification', 'connection'], `Webhook ${webhook.name}`); + const verified = pluginWebhookVerificationHeaders(webhook.verification); + for (const header of [...verified, ...(webhook.deliveryIdHeader ? [webhook.deliveryIdHeader] : [])].filter(h => RESERVED_WEBHOOK_HEADERS.includes(h))) issue(`Webhook ${webhook.name} cannot use header ${header}`, at); + if (webhook.deliveryIdHeader && verified.includes(webhook.deliveryIdHeader)) issue(`Webhook ${webhook.name} uses ${webhook.deliveryIdHeader} both for verification and as its delivery id`, [...at, 'deliveryIdHeader']); + if (webhook.respond && webhook.respond === webhook.handler) issue(`Webhook ${webhook.name} needs a respond function other than its handler`, [...at, 'respond']); + }); } type SettingProblem = { message: string; field?: string }; @@ -335,7 +378,7 @@ export interface PublicPluginManifest { automationTemplates: string[]; commands: { name: string; description: string }[]; subscriptions: string[]; - webhooks: { name: string; description: string }[]; + webhooks: { name: string; description: string; scheme: PluginWebhookScheme }[]; }; } @@ -356,7 +399,7 @@ export function publicPluginManifest(manifest: PluginManifest): PublicPluginMani automationTemplates: contributes.automationTemplates.map(t => t.id), commands: contributes.commands.map(c => ({ name: c.name, description: c.description })), subscriptions: contributes.subscriptions.map(s => s.on), - webhooks: contributes.webhooks.map(w => ({ name: w.name, description: w.description })), + webhooks: contributes.webhooks.map(w => ({ name: w.name, description: w.description, scheme: w.verification.scheme })), }, }; } diff --git a/packages/shared/test/plugins.test.ts b/packages/shared/test/plugins.test.ts index 470005a40..9505e8e12 100644 --- a/packages/shared/test/plugins.test.ts +++ b/packages/shared/test/plugins.test.ts @@ -40,7 +40,7 @@ const valid = (): PluginManifestInput => ({ automationTemplates: [{ id: 'acme-digest', path: 'templates/acme-digest' }], commands: [{ name: 'notify', description: 'Post a message', permissions: ['pull_requests.read'] }], subscriptions: [{ on: 'task.completed', handler: 'onTask' }], - webhooks: [{ name: 'events', description: 'Acme event callbacks', connection: 'events-secret' }], + webhooks: [{ name: 'events', description: 'Acme event callbacks', handler: 'onAcmeEvent', verification: { scheme: 'hmac-sha256', header: 'X-Acme-Signature', prefix: 'sha256=', connection: 'events-secret' }, deliveryIdHeader: 'x-acme-delivery' }], }, }); type Mutable = Record; @@ -91,10 +91,28 @@ test('contributions must reference declared connections of the right type', () = rejects(variant(m => { m.contributes.mcpServers[0].connection = 'bot-token'; }), /which is token, not oauth/); rejects(variant(m => { m.contributes.mcpServers[0].authentication = 'bearer'; }), /which is oauth, not token/); rejects(variant(m => { m.contributes.mcpServers[1].connection = 'bot-token'; }), /no authentication and takes no connection/); - rejects(variant(m => { m.contributes.webhooks[0].connection = 'bot-token'; }), /Webhook events references connection bot-token, which is token, not signing_secret/); + rejects(variant(m => { m.contributes.webhooks[0].verification.connection = 'bot-token'; }), /Webhook events references connection bot-token, which is token, not signing_secret/); rejects(variant(m => { m.contributes.mcpServers[2].secretEnv[0].connection = 'nowhere'; }), /Variable ACME_TOKEN of server acme-local references connection nowhere/); }); +test('webhooks declare a handler and a built-in verification scheme', () => { + const parsed = parsePluginManifest(valid()); + assert.ok(parsed.ok); + const [webhook] = parsed.manifest.contributes.webhooks; + assert.deepEqual(webhook.verification, { scheme: 'hmac-sha256', header: 'x-acme-signature', prefix: 'sha256=', encoding: 'hex', connection: 'events-secret' }, 'header names are lower-cased; hex is the default encoding'); + for (const verification of [{ scheme: 'slack-v0', connection: 'events-secret' }, { scheme: 'token-header', header: 'x-gitlab-token', connection: 'events-secret' }]) { + assert.deepEqual(errors(variant(m => { m.contributes.webhooks[0].verification = verification; })), []); + } + rejects(variant(m => { m.contributes.webhooks[0].verification = { scheme: 'md5', header: 'x-sig', connection: 'events-secret' }; }), /scheme/); + rejects(variant(m => { delete m.contributes.webhooks[0].handler; }), /handler/); + rejects(variant(m => { delete m.contributes.webhooks[0].verification.header; }), /header/); + rejects(variant(m => { m.contributes.webhooks[0].verification.header = 'x sig'; }), /HTTP header name/); + rejects(variant(m => { m.contributes.webhooks[0].verification.header = 'Cookie'; }), /Webhook events cannot use header cookie/); + rejects(variant(m => { m.contributes.webhooks[0].deliveryIdHeader = 'X-Acme-Signature'; }), /both for verification and as its delivery id/); + rejects(variant(m => { m.contributes.webhooks[0].respond = 'onAcmeEvent'; }), /respond function other than its handler/); + rejects(variant(m => { m.contributes.webhooks[0].connection = 'events-secret'; }), /connection/); +}); + test('secretEnv must map to a token connection', () => { rejects(variant(m => { m.contributes.mcpServers[2].secretEnv[0].connection = 'workspace'; }), /Variable ACME_TOKEN of server acme-local references connection workspace, which is oauth, not token/); rejects(variant(m => { m.contributes.mcpServers[2].secretEnv[0].connection = 'events-secret'; }), /which is signing_secret, not token/); diff --git a/propr-ui/e2e/plugin-webhooks.pw.ts b/propr-ui/e2e/plugin-webhooks.pw.ts new file mode 100644 index 000000000..765ba1960 --- /dev/null +++ b/propr-ui/e2e/plugin-webhooks.pw.ts @@ -0,0 +1,86 @@ +import { expect, test, type Page } from '@playwright/test'; +import { BASE, DEFAULTS, NOW, capture, installFixture, step, view, type State } from './plugins.fixture'; + +/** + * Webhook URLs in the plugin's Connect step against the plugin API fixture: + * one URL per declared webhook with Copy, the rejected-request count and + * Regenerate behind a confirmation, at desktop and phone widths. With + * PROPR_CAPTURE_PREVIEWS set it captures the Connect step. + */ + +const KEYS = ['Q3b9xYw1t0P8mN2k5JhGfDsAaZxCvBnMlKjHgFdSaQw', 'n7Fq2Lr8Xc4Vb1Mz9Kp3Wd6Ys0Gh5Jt2Ue8Ri4Oa1Qs']; +const WEBHOOKS = [ + { name: 'events', description: 'Issue tracker updates', scheme: 'hmac-sha256' }, + { name: 'slack', description: 'Slack Events API: mentions of the bot', scheme: 'slack-v0' }, +]; + +/** The example plugin with two webhooks verified with a signing secret, its webhook routes, and the key generation in use. */ +async function withWebhooks(page: Page, state: State) { + const current = { key: 0, rejected: { count: 3, lastAt: NOW - 42 * 60_000, lastReason: 'bad_signature' as string | null } }; + const endpoints = () => ({ + webhooks: WEBHOOKS.map(w => ({ ...w, path: `/api/plugins/${KEYS[current.key]}/hooks/${w.name}`, url: `https://propr.example.com/api/plugins/${KEYS[current.key]}/hooks/${w.name}` })), + rejected: current.rejected, keyCreatedAt: NOW, keyCreatedBy: current.key ? '1042' : 'system', + }); + const plugin = () => { + const base = view(state); + return { + ...base, + manifest: { + ...base.manifest, + connections: [...base.manifest.connections, { type: 'signing_secret', name: 'example-signing', label: 'Webhook signing secret', help: 'Copy the signing secret from the service\'s webhook settings.', helpUrl: 'https://example.com/docs/webhooks' }], + contributes: { ...base.manifest.contributes, webhooks: WEBHOOKS }, + }, + connections: [...base.connections, { name: 'example-signing', type: 'signing_secret', label: 'Webhook signing secret', state: 'connected', required: false, setBy: '1042', setAt: NOW }], + }; + }; + await page.route('**/api/instance/plugins/v1/example-pack**', async route => { + const request = route.request(); + const { pathname } = new URL(request.url()); + if (pathname === `${BASE}/example-pack` && request.method() === 'GET') return route.fulfill({ json: { plugin: { ...plugin(), audit: [] } } }); + if (pathname === `${BASE}/example-pack/webhooks` && request.method() === 'GET') return route.fulfill({ json: endpoints() }); + if (pathname === `${BASE}/example-pack/webhooks/regenerate` && request.method() === 'POST') { + if (request.headers()['x-propr-plugins'] !== '1') return route.fulfill({ status: 403, json: { error: 'Plugin changes require the preflight-protected JSON contract' } }); + current.key = 1; current.rejected = { count: 0, lastAt: null, lastReason: null }; + return route.fulfill({ json: endpoints() }); + } + return route.fallback(); + }); + return current; +} + +test('shows each webhook URL with Copy, the rejected count and Regenerate in the Connect step', async ({ page, context }) => { + await context.grantPermissions(['clipboard-read', 'clipboard-write']); + const state: State = { installed: true, enabled: true, token: true, oauth: true, repositories: ['integry/propr'], refuseUninstall: false, settings: { ...DEFAULTS } }; + await page.setViewportSize({ width: 1440, height: 900 }); + await installFixture(page, state); + await withWebhooks(page, state); + await page.goto('/plugins/example-pack'); + const connect = step(page, 'Connect'); + const webhooks = connect.getByRole('region', { name: 'Webhook URLs' }); + await expect(webhooks.getByTestId('plugin-webhook')).toHaveCount(2); + const events = webhooks.getByRole('textbox', { name: 'URL of webhook events' }); + await expect(events).toHaveValue(`https://propr.example.com/api/plugins/${KEYS[0]}/hooks/events`); + await expect(webhooks).toContainText('HMAC-SHA256 signature'); + await expect(webhooks).toContainText('Slack signing secret'); + await expect(webhooks).toContainText('3 unverified requests rejected'); + + await webhooks.getByRole('button', { name: 'Copy URL of webhook slack' }).click(); + await expect(webhooks.getByRole('button', { name: 'Copy URL of webhook slack' })).toHaveText('Copied'); + expect(await page.evaluate(() => navigator.clipboard.readText())).toBe(`https://propr.example.com/api/plugins/${KEYS[0]}/hooks/slack`); + await capture(page, 'plugins-webhook-urls-desktop', connect.getByTestId('plugin-connection').last()); + + await webhooks.getByRole('button', { name: 'Regenerate' }).click(); + await expect(webhooks.getByRole('group', { name: 'Confirm regenerate' })).toContainText('Every URL above stops working at once'); + await capture(page, 'plugins-webhook-urls-regenerate-confirm-desktop', webhooks); + await webhooks.getByRole('button', { name: 'Regenerate URLs' }).click(); + await expect(events).toHaveValue(`https://propr.example.com/api/plugins/${KEYS[1]}/hooks/events`); + await expect(webhooks.getByRole('status')).toContainText('the old URLs no longer work'); + await expect(webhooks).toContainText('No unverified requests.'); + + await page.setViewportSize({ width: 390, height: 844 }); + await page.reload(); + await expect(events).toHaveValue(`https://propr.example.com/api/plugins/${KEYS[1]}/hooks/events`); + const overflow = await page.evaluate(() => document.documentElement.scrollWidth - document.documentElement.clientWidth); + expect(overflow).toBeLessThanOrEqual(0); + await capture(page, 'plugins-webhook-urls-phone', webhooks); +}); diff --git a/propr-ui/src/api/pluginsApi.ts b/propr-ui/src/api/pluginsApi.ts index 9e9114d7b..9ecfbe247 100644 --- a/propr-ui/src/api/pluginsApi.ts +++ b/propr-ui/src/api/pluginsApi.ts @@ -75,3 +75,10 @@ export const retryPluginDelivery = (id: string, recordId: number) => request(plugin(id, `/activity${segment(String(recordId))}/retry`), { method: 'POST', body: {} }); /** Resume deliveries paused after repeated failures. */ export const resumePluginDeliveries = (id: string) => request(plugin(id, '/resume'), { method: 'POST', body: {} }); + +export type PluginWebhooksView = ProprApi.PluginWebhooksResponse; +/** The installed plugin's inbound webhook URLs (the installation key is part of each) and its count of rejected requests. */ +export const getPluginWebhooks = (id: string) => request(plugin(id, '/webhooks')); +/** Replace the installation key: every webhook URL changes and the old ones stop working at once. */ +export const regeneratePluginWebhookKey = (id: string) => + request(plugin(id, '/webhooks/regenerate'), { method: 'POST', body: {} }); diff --git a/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx b/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx index 3415cb251..94ee03f39 100644 --- a/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx +++ b/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx @@ -105,6 +105,17 @@ describe('Plugin activity', () => { expect(within(region()).getByRole('status')).toHaveTextContent('Deliveries resumed.'); }); + it('lists a webhook delivery by its webhook, without a repository, event number or Retry (its request is not kept)', async () => { + const webhook = delivery({ id: 5, deliveryId: 'whk-events-d-1', kind: 'webhook', repository: '', eventSeq: 0, eventType: 'webhook.events', handler: 'onEvent', status: 'failed', attempt: 5, error: 'chat API answered 503', logs: [] }); + await open({ health: healthy, deliveries: [webhook] }); + const [item] = items(); + expect(item).toHaveTextContent('webhook.events'); + expect(item).toHaveTextContent('onEvent'); + expect(item).toHaveTextContent('Attempt 5'); + expect(item).not.toHaveTextContent('Event #'); + expect(within(item).queryByRole('button', { name: /Retry/ })).toBeNull(); + }); + it('says so when nothing was delivered yet', async () => { await open({ health: healthy, deliveries: [] }); expect(await within(region()).findByText(/No activity yet/)).toBeInTheDocument(); diff --git a/propr-ui/src/pages/PluginsPage/PluginActivity.tsx b/propr-ui/src/pages/PluginsPage/PluginActivity.tsx index d242a8e9d..73e5d576b 100644 --- a/propr-ui/src/pages/PluginsPage/PluginActivity.tsx +++ b/propr-ui/src/pages/PluginsPage/PluginActivity.tsx @@ -48,20 +48,22 @@ function HealthNotice({ health, busy, onResume }: { health: PluginHealthView; bu function DeliveryItem({ delivery, busy, onRetry }: { delivery: PluginDeliveryView; busy: boolean; onRetry: () => void }) { const status = STATUS[delivery.status]; const gap = delivery.kind === 'gap'; + // A webhook request is never stored, so a failed webhook delivery cannot be retried from here. + const webhook = delivery.kind === 'webhook'; return (
  • {gap ? 'Events not delivered' : delivery.eventType} {delivery.handler && {delivery.handler}} {status.label} - {delivery.status === 'failed' && !gap && ( + {delivery.status === 'failed' && !gap && !webhook && ( )}

    {time(delivery.startedAt ?? delivery.createdAt)} - {delivery.repository} - {!gap && Event #{delivery.eventSeq}} + {delivery.repository && {delivery.repository}} + {!gap && !webhook && Event #{delivery.eventSeq}} {!gap && Attempt {delivery.attempt}} {delivery.durationMs !== null && {duration(delivery.durationMs)}{delivery.timedOut ? ' (timed out)' : ''}} {delivery.nextAttemptAt !== null && delivery.status === 'retrying' && Next attempt {time(delivery.nextAttemptAt)}} @@ -80,8 +82,8 @@ function DeliveryItem({ delivery, busy, onRetry }: { delivery: PluginDeliveryVie } /** - * Activity: each event delivered to the plugin's handlers and its result, newest first, with Retry for a failed - * delivery, and the plugin's delivery health with Resume once deliveries are paused. + * Activity: each event and webhook request delivered to the plugin's handlers and its result, newest first, with Retry + * for a failed event delivery, and the plugin's delivery health with Resume once deliveries are paused. */ export function PluginActivity({ activity: state }: { activity: Activity }) { const { activity, error, notice, busy } = state; @@ -107,7 +109,7 @@ export function PluginActivity({ activity: state }: { activity: Activity }) { ))} )} -

    Records are kept for 14 days. They hold the event id and type, never its contents.

    +

    Records are kept for 14 days. They hold the event id and type, never its contents, and never a webhook request's body or headers.

    ); } diff --git a/propr-ui/src/pages/PluginsPage/PluginConnections.tsx b/propr-ui/src/pages/PluginsPage/PluginConnections.tsx index 98a8de76d..6af5d8c58 100644 --- a/propr-ui/src/pages/PluginsPage/PluginConnections.tsx +++ b/propr-ui/src/pages/PluginsPage/PluginConnections.tsx @@ -2,6 +2,7 @@ import { useState } from 'react'; import type { PluginConnectionView, PluginView } from '../../api/pluginsApi'; import { ExtensionOAuthConnection } from '../SettingsPage/extensions/ExtensionOAuthConnection'; import { ExternalLink, PluginStep } from './PluginBits'; +import { PluginWebhookUrls } from './PluginWebhookUrls'; import { dangerButtonClass, formatWhen, inputClass, primaryButtonClass, safeHttpsUrl } from './pluginUi'; export interface PluginConnectionActions { @@ -62,7 +63,7 @@ function ConnectionRow({ connection, help, helpUrl, actions, locked }: { ); } -/** Connect: one row per connection with its state and the action its type needs. */ +/** Connect: one row per connection with its state and the action its type needs, then the URL of each webhook the plugin receives. */ export function PluginConnections({ view, actions }: { view: PluginView; actions: PluginConnectionActions }) { const installed = Boolean(view.installation); const declared = view.manifest?.connections ?? []; @@ -85,6 +86,7 @@ export function PluginConnections({ view, actions }: { view: PluginView; actions {installed && view.connections.some(c => c.type === 'oauth' && c.state !== 'connected') && (

    Sign-in opens in a new tab. This page refreshes when you come back.

    )} + {installed && Boolean(view.manifest?.contributes.webhooks.length) && } ); } diff --git a/propr-ui/src/pages/PluginsPage/PluginWebhookUrls.test.tsx b/propr-ui/src/pages/PluginsPage/PluginWebhookUrls.test.tsx new file mode 100644 index 000000000..63d73eefc --- /dev/null +++ b/propr-ui/src/pages/PluginsPage/PluginWebhookUrls.test.tsx @@ -0,0 +1,113 @@ +import { fireEvent, screen, waitFor, within } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { getPlugin, getPluginActivity, getPluginWebhooks, PluginApiError, regeneratePluginWebhookKey, type PluginView, type PluginWebhooksView } from '../../api/pluginsApi'; +import { exampleAt, renderPluginsPage } from './pluginFixtures.testUtils'; + +vi.mock('../../api/pluginsApi', async importOriginal => ({ + ...(await importOriginal()), + getPlugin: vi.fn(), getPluginActivity: vi.fn(), getPluginWebhooks: vi.fn(), regeneratePluginWebhookKey: vi.fn(), +})); +vi.mock('../../api/proprApi', () => ({ getInstanceCatalog: vi.fn().mockResolvedValue({ agents: [], repositories: [{ name: 'acme/web' }] }) })); +vi.mock('../../api/extensionProfilesApi', () => ({ + getExtensionLibrary: vi.fn().mockResolvedValue({ oauth: { redirectUri: 'https://propr.example.com/api/instance/extensions/v1/oauth/callback', clientMetadataUrl: null, connections: {} } }), +})); + +const OLD_KEY = 'Q3b9xYw1t0P8mN2k5JhGfDsAaZxCvBnMlKjHgFdSaQw'; +const NEW_KEY = 'n7Fq2Lr8Xc4Vb1Mz9Kp3Wd6Ys0Gh5Jt2Ue8Ri4Oa1Qs'; +const urls = (key: string, url = true): PluginWebhooksView => ({ + webhooks: [ + { name: 'events', description: 'Example service events', scheme: 'hmac-sha256', path: `/api/plugins/${key}/hooks/events`, url: url ? `https://propr.example.com/api/plugins/${key}/hooks/events` : null }, + { name: 'slack', description: 'Slack Events API', scheme: 'slack-v0', path: `/api/plugins/${key}/hooks/slack`, url: url ? `https://propr.example.com/api/plugins/${key}/hooks/slack` : null }, + ], + rejected: { count: 2, lastAt: Date.UTC(2026, 10, 15, 9), lastReason: 'bad_signature' }, + keyCreatedAt: Date.UTC(2026, 10, 14), keyCreatedBy: 'system', +}); +/** The example plugin, installed, declaring two webhooks verified with a signing secret. */ +function withWebhooks(): PluginView { + const view = exampleAt('connected'); + const manifest = view.manifest!; + return { + ...view, + manifest: { + ...manifest, + connections: [...manifest.connections, { type: 'signing_secret', name: 'example-signing', label: 'Signing secret', help: 'The webhook signing secret.', helpUrl: 'https://example.com/docs/webhooks' }], + contributes: { ...manifest.contributes, webhooks: [{ name: 'events', description: 'Example service events', scheme: 'hmac-sha256' }, { name: 'slack', description: 'Slack Events API', scheme: 'slack-v0' }] }, + }, + connections: [...view.connections, { name: 'example-signing', type: 'signing_secret', label: 'Signing secret', state: 'connected', required: false, setBy: '42', setAt: 1 }], + }; +} +const open = async (view: PluginView = withWebhooks()) => { + vi.mocked(getPlugin).mockResolvedValue({ plugin: { ...view, audit: [] } }); + renderPluginsPage('/plugins/example-pack'); + await screen.findByRole('heading', { level: 1, name: /Example pack/ }); +}; +const region = () => screen.getByRole('region', { name: 'Webhook URLs' }); + +describe('Plugin webhook URLs', () => { + const writeText = vi.fn().mockResolvedValue(undefined); + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getPluginActivity).mockResolvedValue({ health: { state: 'healthy', consecutiveFailures: 0, pauseAfterFailures: 10, lastError: null, lastErrorAt: null, degradedAt: null, pausedAt: null }, deliveries: [] }); + vi.mocked(getPluginWebhooks).mockResolvedValue(urls(OLD_KEY)); + Object.defineProperty(navigator, 'clipboard', { value: { writeText }, configurable: true }); + }); + afterEach(() => { vi.restoreAllMocks(); }); + + it('shows the URL of each webhook in the Connect step, with its verification scheme, Copy and the rejected count', async () => { + await open(); + const events = await screen.findByRole('textbox', { name: 'URL of webhook events' }); + expect(getPluginWebhooks).toHaveBeenCalledWith('example-pack'); + expect(events).toHaveValue(`https://propr.example.com/api/plugins/${OLD_KEY}/hooks/events`); + expect(events).toHaveAttribute('readonly'); + expect(screen.getByRole('textbox', { name: 'URL of webhook slack' })).toHaveValue(`https://propr.example.com/api/plugins/${OLD_KEY}/hooks/slack`); + const items = within(region()).getAllByTestId('plugin-webhook'); + expect(items[0]).toHaveTextContent('Example service events'); + expect(items[0]).toHaveTextContent('HMAC-SHA256 signature'); + expect(items[1]).toHaveTextContent('Slack signing secret'); + expect(region()).toHaveTextContent('2 unverified requests rejected'); + expect(region()).toHaveTextContent('(wrong signature)'); + expect(within(screen.getByRole('heading', { name: /Connect/ }).closest('section')!).getByRole('region', { name: 'Webhook URLs' })).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Copy URL of webhook slack' })); + await waitFor(() => expect(writeText).toHaveBeenCalledWith(`https://propr.example.com/api/plugins/${OLD_KEY}/hooks/slack`)); + expect(await within(region()).findByText('Copied')).toBeInTheDocument(); + }); + + it('regenerates the key after a confirmation and shows the new URLs', async () => { + vi.mocked(regeneratePluginWebhookKey).mockResolvedValue({ ...urls(NEW_KEY), rejected: { count: 0, lastAt: null, lastReason: null } }); + await open(); + await screen.findByRole('textbox', { name: 'URL of webhook events' }); + fireEvent.click(within(region()).getByRole('button', { name: 'Regenerate' })); + expect(regeneratePluginWebhookKey).not.toHaveBeenCalled(); + const confirmation = within(region()).getByRole('group', { name: 'Confirm regenerate' }); + expect(confirmation).toHaveTextContent('Every URL above stops working at once'); + fireEvent.click(within(confirmation).getByRole('button', { name: 'Cancel' })); + expect(regeneratePluginWebhookKey).not.toHaveBeenCalled(); + + fireEvent.click(within(region()).getByRole('button', { name: 'Regenerate' })); + fireEvent.click(within(region()).getByRole('button', { name: 'Regenerate URLs' })); + await waitFor(() => expect(screen.getByRole('textbox', { name: 'URL of webhook events' })).toHaveValue(`https://propr.example.com/api/plugins/${NEW_KEY}/hooks/events`)); + expect(regeneratePluginWebhookKey).toHaveBeenCalledWith('example-pack'); + expect(within(region()).getByRole('status')).toHaveTextContent('the old URLs no longer work'); + expect(region()).toHaveTextContent('No unverified requests.'); + }); + + it('shows the path when the instance has no public base URL, and a refusal verbatim', async () => { + vi.mocked(getPluginWebhooks).mockResolvedValue(urls(OLD_KEY, false)); + vi.mocked(regeneratePluginWebhookKey).mockRejectedValue(new PluginApiError(403, 'Current durable instance administrator access is required')); + await open(); + expect(await screen.findByRole('textbox', { name: 'URL of webhook events' })).toHaveValue(`/api/plugins/${OLD_KEY}/hooks/events`); + expect(region()).toHaveTextContent('no public base URL'); + fireEvent.click(within(region()).getByRole('button', { name: 'Regenerate' })); + fireEvent.click(within(region()).getByRole('button', { name: 'Regenerate URLs' })); + expect(await within(region()).findByRole('alert')).toHaveTextContent('Current durable instance administrator access is required'); + expect(screen.getByRole('textbox', { name: 'URL of webhook events' })).toHaveValue(`/api/plugins/${OLD_KEY}/hooks/events`); + }); + + it('is not shown for a plugin without webhooks or before it is installed', async () => { + await open(exampleAt('connected')); + await screen.findByRole('heading', { name: /Connect/ }); + expect(screen.queryByRole('region', { name: 'Webhook URLs' })).toBeNull(); + expect(getPluginWebhooks).not.toHaveBeenCalled(); + }); +}); diff --git a/propr-ui/src/pages/PluginsPage/PluginWebhookUrls.tsx b/propr-ui/src/pages/PluginsPage/PluginWebhookUrls.tsx new file mode 100644 index 000000000..a66d9a968 --- /dev/null +++ b/propr-ui/src/pages/PluginsPage/PluginWebhookUrls.tsx @@ -0,0 +1,95 @@ +import { useState } from 'react'; +import { Check, Copy, RefreshCw } from 'lucide-react'; +import type { PluginWebhooksView } from '../../api/pluginsApi'; +import { usePluginWebhooks } from './usePluginWebhooks'; +import { buttonClass, dangerButtonClass, formatWhen } from './pluginUi'; + +type Endpoint = PluginWebhooksView['webhooks'][number]; +const SCHEME_LABELS: Record = { + 'hmac-sha256': 'HMAC-SHA256 signature', + 'slack-v0': 'Slack signing secret', + 'token-header': 'Secret token header', +}; +const REASONS: Record = { + missing_signature: 'no signature', bad_signature: 'wrong signature', stale_timestamp: 'stale timestamp', no_secret: 'signing secret not set', +}; + +function CopyUrl({ endpoint }: { endpoint: Endpoint }) { + const [copied, setCopied] = useState(false); + const value = endpoint.url ?? endpoint.path; + const copy = () => { + void navigator.clipboard?.writeText(value).then(() => { setCopied(true); setTimeout(() => setCopied(false), 2000); }, () => setCopied(false)); + }; + const fieldId = `plugin-webhook-${endpoint.name}`; + return ( +
    + + event.currentTarget.select()} + className="min-w-0 flex-1 rounded-md border border-slate-300 bg-slate-50 px-2 py-1.5 font-mono text-xs text-slate-800" /> + +
    + ); +} + +/** + * Webhooks, in the Connect step: the URL to give each sending service, with Copy, and Regenerate, + * which replaces the installation key in every URL (the old ones stop working at once). + */ +export function PluginWebhookUrls({ pluginId, installed }: { pluginId: string; installed: boolean }) { + const { webhooks, error, busy, regenerate } = usePluginWebhooks(pluginId, installed); + const [confirming, setConfirming] = useState(false); + const [notice, setNotice] = useState(''); + if (!installed) return null; + const confirm = () => void regenerate().then(ok => { setConfirming(false); if (ok) setNotice('New URLs created. Update them at each sending service; the old URLs no longer work.'); }); + return ( +
    +
    +

    Webhook URLs

    +

    Give each service its URL. ProPR checks every request against the signing secret before the plugin sees it. Keep the URLs private.

    +
    + {error &&

    {error}

    } + {!webhooks && !error &&

    Loading webhook URLs…

    } + {webhooks && ( + <> +
      + {webhooks.webhooks.map(endpoint => ( +
    • +

      + {endpoint.name} + {endpoint.description} + {SCHEME_LABELS[endpoint.scheme]} +

      + +
      • + ))} +
    + {webhooks.webhooks.some(endpoint => endpoint.url === null) && ( +

    This instance has no public base URL, so only the path is shown. Prefix it with the address services reach ProPR at.

    + )} +

    + {webhooks.rejected.count === 0 + ? 'No unverified requests.' + : `${webhooks.rejected.count} unverified ${webhooks.rejected.count === 1 ? 'request' : 'requests'} rejected${webhooks.rejected.lastAt ? `, the last ${formatWhen(webhooks.rejected.lastAt)}` : ''}${webhooks.rejected.lastReason ? ` (${REASONS[webhooks.rejected.lastReason] ?? webhooks.rejected.lastReason})` : ''}.`} +

    + {notice &&

    {notice}

    } + {confirming ? ( +
    +

    Every URL above stops working at once. Services keep failing until you give them the new URLs.

    +
    + + +
    +
    + ) : ( + + )} + + )} +
    + ); +} diff --git a/propr-ui/src/pages/PluginsPage/usePluginWebhooks.ts b/propr-ui/src/pages/PluginsPage/usePluginWebhooks.ts new file mode 100644 index 000000000..016cd879b --- /dev/null +++ b/propr-ui/src/pages/PluginsPage/usePluginWebhooks.ts @@ -0,0 +1,33 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; +import { getPluginWebhooks, regeneratePluginWebhookKey, type PluginWebhooksView } from '../../api/pluginsApi'; + +const message = (error: unknown) => (error instanceof Error ? error.message : 'Webhook URLs unavailable.'); + +/** The installed plugin's webhook URLs, with Regenerate; read while it is installed. */ +export function usePluginWebhooks(id: string, installed: boolean) { + const [webhooks, setWebhooks] = useState(null); + const [error, setError] = useState(''); + const [busy, setBusy] = useState(false); + const sequence = useRef(0); + const load = useCallback(async () => { + const at = ++sequence.current; + try { + const result = await getPluginWebhooks(id); + if (at === sequence.current) { setWebhooks(result); setError(''); } + } catch (failure) { if (at === sequence.current) setError(message(failure)); } + }, [id]); + useEffect(() => { + setWebhooks(null); setError(''); + if (installed) void load(); else sequence.current += 1; + }, [installed, load]); + const regenerate = useCallback(async () => { + setBusy(true); setError(''); + const at = ++sequence.current; + try { + const result = await regeneratePluginWebhookKey(id); + if (at === sequence.current) setWebhooks(result); + return true; + } catch (failure) { setError(message(failure)); return false; } finally { setBusy(false); } + }, [id]); + return { webhooks, error, busy, regenerate }; +} From 3da3c8eb9f0183d20ec6e8689eb6c8bb69d7cad4 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Sun, 15 Nov 2026 18:34:13 +0000 Subject: [PATCH 2/5] feat(ai): I fixed the CI failure: the `propr-ui` typecheck (`tsc --noEmit`) was failing on a test file this PR added. The packaging job runs that typecheck, so it went red too. I fixed the CI failure: the `propr-ui` typecheck (`tsc --noEmit`) was failing on a test file this PR added. The packaging job runs that typecheck, so it went red too. **Cause:** In `propr-ui/e2e/plugin-webhooks.pw.ts`, the mock state for the rejected-request count starts with `lastAt` set to a number. TypeScript therefore typed `lastAt` as `number`. The Regenerate route mock then sets it to `null` (line 43), which the type didn't allow. **Fix:** I declared `lastAt` on line 19 as `number | null`, the same way `lastReason` is already declared as `string | null`. This only changes a type, so the test behaves exactly as before. `tsc --noEmit` and ESLint on that file now pass in `propr-ui`. I didn't rerun the Playwright test or the full packaging job, since the test's behaviour didn't change. There are no new previews because nothing visible changed. PR: #3579 Comment by: @propr-dev[bot] (ID: 6899204646) Model: Claude Opus 5.5 --- propr-ui/e2e/plugin-webhooks.pw.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/propr-ui/e2e/plugin-webhooks.pw.ts b/propr-ui/e2e/plugin-webhooks.pw.ts index 765ba1960..adb0d8057 100644 --- a/propr-ui/e2e/plugin-webhooks.pw.ts +++ b/propr-ui/e2e/plugin-webhooks.pw.ts @@ -16,7 +16,7 @@ const WEBHOOKS = [ /** The example plugin with two webhooks verified with a signing secret, its webhook routes, and the key generation in use. */ async function withWebhooks(page: Page, state: State) { - const current = { key: 0, rejected: { count: 3, lastAt: NOW - 42 * 60_000, lastReason: 'bad_signature' as string | null } }; + const current = { key: 0, rejected: { count: 3, lastAt: (NOW - 42 * 60_000) as number | null, lastReason: 'bad_signature' as string | null } }; const endpoints = () => ({ webhooks: WEBHOOKS.map(w => ({ ...w, path: `/api/plugins/${KEYS[current.key]}/hooks/${w.name}`, url: `https://propr.example.com/api/plugins/${KEYS[current.key]}/hooks/${w.name}` })), rejected: current.rejected, keyCreatedAt: NOW, keyCreatedBy: current.key ? '1042' : 'system', From 2031768c30ad32dbd5aea5bf38b1debfc82014ec Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Sun, 15 Nov 2026 19:16:05 +0000 Subject: [PATCH 3/5] =?UTF-8?q?feat(ai):=20I=20fixed=20F1=E2=80=93F4.=20Th?= =?UTF-8?q?e=20core=20plugin=20suites=20pass=20(35=20tests=20across=20`plu?= =?UTF-8?q?ginStore`,=20`pluginContributions`=20and=20`pluginWebhooks`),?= =?UTF-8?q?=20and=20so=20do=20the=20three=20API=20route=20test=20files.=20?= =?UTF-8?q?`pluginRoutes`=20passed=20and=20then=20hung=20at=20exit=20until?= =?UTF-8?q?=20the=20timeout=20killed=20it,=20which=20is=20a=20known=20patt?= =?UTF-8?q?ern=20for=20API=20tests=20in=20this=20container.=20Typecheck=20?= =?UTF-8?q?for=20core=20and=20api,=20the=20zero-warning=20lint=20on=20the?= =?UTF-8?q?=20changed=20source=20files,=20the=20OpenAPI=20check=20and=20th?= =?UTF-8?q?e=20`PluginActivity`=20UI=20test=20are=20clean.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I fixed F1–F4. The core plugin suites pass (35 tests across `pluginStore`, `pluginContributions` and `pluginWebhooks`), and so do the three API route test files. `pluginRoutes` passed and then hung at exit until the timeout killed it, which is a known pattern for API tests in this container. Typecheck for core and api, the zero-warning lint on the changed source files, the OpenAPI check and the `PluginActivity` UI test are clean. **F1 – a replay could run the responder.** The receiver now works out the delivery id first (400 if malformed). It then reserves it in `plugin_deliveries` before any plugin code runs, so a replay gets 409 without loading the module or calling `respond`. The runner gained `reserve`, `start`, `answered` and `release`, and `accept` is now just reserve then start. - **Challenge answered:** if the id came from the sender, the record is marked succeeded under the respond function's name, so a replayed challenge also gets 409. A ProPR-assigned id can't be sent again, so its record is deleted. - **Refused before delivery:** a respond failure (500), a paused plugin (503) or an error releases the reservation, so the sender's retry is accepted rather than treated as a replay. - **New test:** a responder combined with `X-Delivery-Id`, checking that it ran once on a replayed ordinary event and on a replayed challenge, and that a paused-then-retried request is delivered. **F2 – handler output could store the body.** A webhook attempt now keeps only each log line's level and time, and a fixed error: "The webhook handler failed (its error is not kept: it may contain the request)" or "…timed out". Matching the body text isn't enough, because a handler can log any part or transformation of it. The same change applies where a respond function fails: that log no longer includes the error's name, since a plugin can set it to anything. A new `leakyWebhook` fixture logs the body as a message and in fields and throws it, and the sentinel test checks that no table or log line contains `customer-private-message-123`. **F3 – a starting process could fail another's deliveries.** Each runner now has an owner name and a heartbeat in a new `plugin_webhook_runners` table, renewed every 30 s. Each delivery records its owner in a new `webhook_owner` column. Recovery runs at startup and with every heartbeat, as a single UPDATE, and fails only deliveries with no owner or whose owner's heartbeat is more than 2 minutes old. A new test runs two runners: starting B fails nothing of A, A's retry still succeeds, and a delivery owned by a runner with a stale heartbeat is failed. **F4 – delivery ids collided across webhooks.** Ids are now `whk-:`, or `whk-/` when ProPR assigns one. Webhook names can't contain `:` or `/`, so the name always ends where the separator is. The new test sends `a` with `b-c` and `a-b` with `c`, and both are delivered. Things to know: - I added the new table and column to this PR's existing migration, `20261116000000_plugin_webhook_keys.js`, rather than a new file, because tests list migrations by name. Each step is guarded, so it is safe to re-run. - Webhook Activity entries no longer show the handler's error or log text. That costs debugging detail, and it's the only way to meet AC5 for arbitrary request content. - A process whose event loop stalls for more than 2 minutes would be taken for stopped, and its deliveries failed. That's inherent in using a heartbeat. - I updated the docs (`plugins.md`, `plugin-runtime.md`), the `deliveryId` description in the OpenAPI spec and generated client types, and the existing tests that expected the old id format and error text. The fixture manifest has four new webhooks, so the route test's list of webhooks changed too. - There are no visible UI changes, so I made no previews. Not fixed, outside these findings: the audit record that `ctx.propr` calls write to `plugin_audit` stores the call's details. A webhook handler that puts request content into such a call (for example a comment body) would have it stored there. PR: #3579 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5 --- docs/docs/architecture/plugin-runtime.md | 51 ++++-- docs/docs/features/plugins.md | 30 ++-- docs/static/openapi/propr-api.yaml | 2 +- packages/api/openapi/schemasPlugins.ts | 2 +- packages/api/server.ts | 2 +- packages/api/test/pluginWebhookRoutes.test.ts | 2 +- packages/client/src/generated/apiTypes.ts | 2 +- .../20261116000000_plugin_webhook_keys.js | 18 ++- .../core/src/plugins/pluginWebhookReceiver.ts | 68 +++++--- .../core/src/plugins/pluginWebhookRunner.ts | 152 ++++++++++++++++-- .../plugins/fixture-webhooks/handlers.ts | 16 ++ .../plugins/fixture-webhooks/manifest.json | 21 ++- packages/core/test/pluginWebhooks.test.ts | 101 +++++++++++- packages/shared/src/pluginDeliveries.ts | 2 +- .../pages/PluginsPage/PluginActivity.test.tsx | 2 +- 15 files changed, 391 insertions(+), 80 deletions(-) diff --git a/docs/docs/architecture/plugin-runtime.md b/docs/docs/architecture/plugin-runtime.md index 4c9a5efa6..a756671c1 100644 --- a/docs/docs/architecture/plugin-runtime.md +++ b/docs/docs/architecture/plugin-runtime.md @@ -241,17 +241,25 @@ before the JSON parser and the session middleware: `verifyPluginWebhook`. A failure gets 401 and is counted on the key row (`recordPluginWebhookRejection`). It is logged as `{ pluginId, webhook, reason }`, never with the body or a header. - 3. If the webhook has `respond`, call it (see below). A response is sent as - it is and the request ends there. - 4. Derive the delivery id: `whk--`, or a - random UUID in place of the value when the webhook declares no header or - the request does not send it. A malformed value gets 400. - 5. Refuse with 503 while the installation's event deliveries are paused. - 6. `PluginWebhookRunner.accept` inserts the `plugin_deliveries` row (kind - `webhook`, `event_type` `webhook.`, empty repository, `event_seq` 0). - The unique `(plugin_id, delivery_id)` index turns a replay into a 409. - When 1,000 deliveries are already held, the answer is 503. Otherwise it - answers 202. + 3. Derive the delivery id: `whk-:`, or + `whk-/` when the webhook declares no header or the + request does not send it. A webhook name contains neither `:` nor `/`, so + the character after the name ends it: webhook `a` with id `b-c` and + webhook `a-b` with id `c` get different ids. A malformed value gets 400. + 4. `PluginWebhookRunner.reserve` inserts the `plugin_deliveries` row (kind + `webhook`, `event_type` `webhook.`, empty repository, `event_seq` 0, + status `pending`, `webhook_owner` this runner) before any plugin code + runs. The unique `(plugin_id, delivery_id)` index turns a replay into a + 409, so a replay never reaches the `respond` function either. When 1,000 + deliveries are already held or reserved, the answer is 503. + 5. If the webhook has `respond`, call it (see below). A response is sent as + it is and the request ends there. A 2xx answer to a request with the + sender's delivery id marks the row succeeded (handler: the respond + function), so the id stays taken; otherwise the row is deleted + (`release`), and a failed respond function's request may be sent again. + 6. Refuse with 503 while the installation's event deliveries are paused, + releasing the row so the sender's retry is not a replay. + 7. `start` hands the delivery to the runner, and the answer is 202. | Scheme | Computation | Notes | | --- | --- | --- | @@ -277,8 +285,8 @@ argument fails. A returned thenable is refused, so the function must be synchronous. The response is checked: a 2xx status, `text/plain` or `application/json`, and a string body of at most `PLUGIN_WEBHOOK_LIMITS.responseBytes` (4 KiB). A refusal -or a throw gets 500. The error is logged by name only, because its message -could quote the request. +or a throw gets 500. Nothing of the error is logged (not even its name), +because it could quote the request. ### Webhook delivery runner @@ -302,9 +310,14 @@ The handler's headers are the request's minus the verification headers, `authorization`, `proxy-authorization`, `cookie`, `host`, `connection`, `content-length` and `transfer-encoding`. -When the API starts, `recover()` marks webhook deliveries that an earlier -process left unfinished as failed: *ProPR stopped before the delivery finished, -and webhook requests are not kept*. `stop()` (on SIGTERM) lets attempts under +Each runner has an owner name (`webhooks--`) and a heartbeat row in +`plugin_webhook_runners`, renewed every 30 seconds; each delivery record names +its owner in `webhook_owner`. When the API starts, and with every heartbeat, +`recover()` marks failed the unfinished webhook deliveries whose owner has +stopped: its heartbeat is older than 2 minutes, or the record has no owner. +They read *ProPR stopped before the delivery finished, and webhook requests are +not kept*. This is one `UPDATE`, so another API process starting while one is +alive never fails that process's pending or retrying deliveries. `stop()` (on SIGTERM) lets attempts under way finish and fails the deliveries still waiting. A failed webhook delivery cannot be retried from Activity (`retryPluginDelivery` only accepts event deliveries), because the request is gone. @@ -315,7 +328,11 @@ installation refuses new webhook requests with 503. **What is stored.** The key row (the key, its hash and the rejection count) and the delivery record (the ids, the webhook and handler names, status, timing, -and the handler's redacted error and log lines). The body, the signature and +and the level and time of each log line). A handler may log or throw any part +of the request, which the credential redaction does not recognize, so a +webhook attempt keeps no handler log message or field and no error text: the +error is *The webhook handler failed* or *The webhook handler timed out* +(`webhookDiagnostics`). The body, the signature and token headers, and the other request headers are never written to the database or a log. The sentinel tests in `pluginWebhooks.test.ts` and `packages/api/test/pluginWebhookRoutes.test.ts` dump every table and capture diff --git a/docs/docs/features/plugins.md b/docs/docs/features/plugins.md index 8aa43faf6..1b23b2269 100644 --- a/docs/docs/features/plugins.md +++ b/docs/docs/features/plugins.md @@ -839,32 +839,37 @@ failed. The handler receives: - `headers` are the request's headers with lower-case names. ProPR removes the verification headers, `Authorization`, `Cookie` and the hop-by-hop headers first. -- `deliveryId` is `whk--` and is the same on every attempt. When - the webhook declares `deliveryIdHeader` and the service sends it, `` is - that value. Otherwise ProPR assigns one. +- `deliveryId` is the same on every attempt. When the webhook declares + `deliveryIdHeader` and the service sends it, it is `whk-:` with + that value as ``. Otherwise ProPR assigns one: `whk-/`. Other limits and refusals: | Situation | Answer | | --- | --- | | Body larger than 256 KiB | **413**. Nothing runs. | -| A `deliveryIdHeader` value already received | **409** (a replay). Nothing runs. Delivery ids are remembered for as long as Activity records are kept (14 days). | +| A `deliveryIdHeader` value already received | **409** (a replay). Nothing runs, not even the `respond` function. Delivery ids are remembered for as long as Activity records are kept (14 days). | | A `deliveryIdHeader` value that is not 1 to 200 printable characters | **400**. | | Event deliveries of the plugin are [paused](#health-degraded-and-resume) | **503** with `Retry-After`. | | 1,000 webhook deliveries already waiting | **503** with `Retry-After`. | | More requests than the webhook rate limit allows | **429**. This is the limiter `POST /webhook` uses: `PROPR_WEBHOOK_RATE_LIMIT_MAX` requests per `PROPR_WEBHOOK_RATE_LIMIT_WINDOW_MS`, 300 per minute by default. | Each webhook delivery appears in the plugin's [Activity](#activity) as -`webhook.`, with its handler, outcome, attempts, error and log lines. -ProPR never stores or logs request bodies, signature headers or tokens. The +`webhook.`, with its handler, outcome and attempts. ProPR never stores +or logs request bodies, signature headers or tokens. Because a handler could +log or throw part of the request, a webhook delivery keeps only the level and +time of each log line, not its message or fields, and its error says only that +the handler failed or timed out. The request is held in the API process's memory only until its delivery is final. Two things follow from that: - A failed webhook delivery has no **Retry** button. The service has to send it again. -- A delivery still waiting when the API stops is marked failed with - *ProPR stopped before the delivery finished*. Most services redeliver on - their own. +- A delivery still waiting when its API process stops is marked failed with + *ProPR stopped before the delivery finished*, by the next API process to + start or by another one running (within about two minutes). Starting an API + process never fails the deliveries of another one still running. Most + services redeliver on their own. Webhook failures do not count towards the plugin's Degraded and paused [health](#health-degraded-and-resume), which tracks event deliveries. @@ -873,8 +878,8 @@ Webhook failures do not count towards the plugin's Degraded and paused Some services check a new URL by sending a challenge that must be echoed in the response, such as Slack's `url_verification`. `respond` names another exported -function of `handlers.ts`. It runs after verification and before the request is -queued, and it receives only the frozen request +function of `handlers.ts`. It runs after verification and the replay check, and +before the request is queued, and it receives only the frozen request `{ webhook, headers, body, receivedAt }`. It gets no `ctx`, so it cannot read state, call ProPR or send HTTP requests. It must return synchronously: @@ -891,7 +896,8 @@ export const slackChallenge = definePluginWebhookResponder(request => { When it returns a response (a 2xx `status`, 200 by default, a `text/plain` or `application/json` `contentType` and a `body` of at most 4 KiB), ProPR sends -that response and the handler does not run. When it returns `null`, the request +that response and the handler does not run; the delivery id stays taken, so a +replayed challenge gets 409. When it returns `null`, the request is queued as usual. If it throws, returns a promise or returns an invalid response, ProPR answers **500** and runs nothing. diff --git a/docs/static/openapi/propr-api.yaml b/docs/static/openapi/propr-api.yaml index 5490b9a05..3d10be4a3 100644 --- a/docs/static/openapi/propr-api.yaml +++ b/docs/static/openapi/propr-api.yaml @@ -13300,7 +13300,7 @@ components: description: The activity record id, used by the retry route. deliveryId: type: string - description: The stable delivery id the handler receives (`evt--`, or `whk--` for a webhook); the same on every attempt. + description: The stable delivery id the handler receives (`evt--`, or `whk-:` or `whk-/` for a webhook); the same on every attempt. kind: type: string enum: diff --git a/packages/api/openapi/schemasPlugins.ts b/packages/api/openapi/schemasPlugins.ts index aa54e0f45..e4a397096 100644 --- a/packages/api/openapi/schemasPlugins.ts +++ b/packages/api/openapi/schemasPlugins.ts @@ -89,7 +89,7 @@ export const PluginOAuthStartResponse = component('PluginOAuthStartResponse', z. const PluginLogLine = z.object({ level: z.enum(['debug', 'info', 'warn', 'error']), message: z.string(), fields: z.record(z.string(), z.unknown()).optional(), at: z.number() }); export const PluginDelivery = component('PluginDelivery', z.object({ id: z.number().int().describe('The activity record id, used by the retry route.'), - deliveryId: z.string().describe('The stable delivery id the handler receives (`evt--`, or `whk--` for a webhook); the same on every attempt.'), + deliveryId: z.string().describe('The stable delivery id the handler receives (`evt--`, or `whk-:` or `whk-/` for a webhook); the same on every attempt.'), kind: z.enum(PLUGIN_DELIVERY_KINDS).describe('`event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered. `webhook`: one verified inbound webhook request to its handler.'), repository: z.string().describe('The event\'s repository; empty for a webhook delivery.'), eventSeq: z.number().int().describe('The journal `seq` of the event; for a gap, the first `seq` that may be missing; 0 for a webhook delivery.'), diff --git a/packages/api/server.ts b/packages/api/server.ts index fd7328dac..b292211f4 100644 --- a/packages/api/server.ts +++ b/packages/api/server.ts @@ -690,7 +690,7 @@ async function start(): Promise { if (!demoMode) { // Inbound plugin webhooks run in this process; their requests are held in memory only. const runner = new PluginWebhookRunner({ database: db }); - await runner.recover().catch(error => console.warn('Could not close webhook deliveries left by an earlier process:', (error as Error).message)); + await runner.recover().catch(error => console.warn('Could not close webhook deliveries left by a stopped API process:', (error as Error).message)); pluginWebhooks = { runner, receiver: new PluginWebhookReceiver({ database: db, runner }) }; await initializeNotificationBackground(); // Every API process drops its MCP cache on a published config event, so an diff --git a/packages/api/test/pluginWebhookRoutes.test.ts b/packages/api/test/pluginWebhookRoutes.test.ts index 8a235b932..d52591dea 100644 --- a/packages/api/test/pluginWebhookRoutes.test.ts +++ b/packages/api/test/pluginWebhookRoutes.test.ts @@ -116,7 +116,7 @@ test('plugin webhook route: verified requests are queued, oversized bodies and u assert.equal(listed.status, 200); assertConforms(schemas.PluginWebhooksResponse, listed.body, 'webhooks'); const urls = Object.fromEntries((listed.body.webhooks as { name: string; url: string; scheme: string }[]).map(w => [w.name, w.url])); - assert.deepEqual((listed.body.webhooks as { name: string; scheme: string }[]).map(w => [w.name, w.scheme]), [['signed', 'hmac-sha256'], ['slack', 'slack-v0'], ['token', 'token-header'], ['probe', 'token-header']]); + assert.deepEqual((listed.body.webhooks as { name: string; scheme: string }[]).map(w => [w.name, w.scheme]), [['signed', 'hmac-sha256'], ['slack', 'slack-v0'], ['token', 'token-header'], ['probe', 'token-header'], ['answered', 'token-header'], ['a', 'token-header'], ['a-b', 'token-header'], ['leaky', 'token-header']]); assert.match(urls.signed, /^https:\/\/propr\.example\.com\/api\/plugins\/[A-Za-z0-9_-]{43}\/hooks\/signed$/); // Verified: answered 202 at once, then run from the queue exactly once. diff --git a/packages/client/src/generated/apiTypes.ts b/packages/client/src/generated/apiTypes.ts index c89bdd31d..e767b6eb1 100644 --- a/packages/client/src/generated/apiTypes.ts +++ b/packages/client/src/generated/apiTypes.ts @@ -933,7 +933,7 @@ export interface PluginConnectionRequest { export interface PluginDelivery { /** The activity record id, used by the retry route. */ id: number; - /** The stable delivery id the handler receives (`evt--`, or `whk--` for a webhook); the same on every attempt. */ + /** The stable delivery id the handler receives (`evt--`, or `whk-:` or `whk-/` for a webhook); the same on every attempt. */ deliveryId: string; /** `event`: one journal event to one handler. `gap`: journal retention removed events before they were delivered. `webhook`: one verified inbound webhook request to its handler. */ kind: "event" | "gap" | "webhook"; diff --git a/packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js b/packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js index 2d871190f..aa1ec1d07 100644 --- a/packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js +++ b/packages/core/src/db/migrations/20261116000000_plugin_webhook_keys.js @@ -10,10 +10,15 @@ // `rejected_count`, `last_rejected_at` and `last_rejected_reason` count // requests that failed verification. Request bodies and signature headers are // never stored, here or anywhere else. +// +// A webhook delivery's request is held only in the memory of the API process +// that accepted it. `plugin_deliveries.webhook_owner` names that process's +// webhook runner, and `plugin_webhook_runners` holds each live runner's +// heartbeat: another process fails an unfinished webhook delivery only once its +// owner's heartbeat has lapsed (or it has no owner). export async function up(knex) { - if (await knex.schema.hasTable('plugin_webhook_keys')) return; - await knex.schema.createTable('plugin_webhook_keys', t => { + if (!await knex.schema.hasTable('plugin_webhook_keys')) await knex.schema.createTable('plugin_webhook_keys', t => { t.string('plugin_id').primary(); t.bigInteger('installed_at').notNullable(); t.string('key_hash', 64).notNullable().unique(); @@ -24,6 +29,15 @@ export async function up(knex) { t.bigInteger('last_rejected_at'); t.string('last_rejected_reason'); }); + if (!await knex.schema.hasTable('plugin_webhook_runners')) { + await knex.schema.createTable('plugin_webhook_runners', t => { + t.string('owner').primary(); + t.bigInteger('heartbeat_at').notNullable(); + }); + } + if (await knex.schema.hasTable('plugin_deliveries') && !await knex.schema.hasColumn('plugin_deliveries', 'webhook_owner')) { + await knex.schema.alterTable('plugin_deliveries', t => { t.string('webhook_owner'); }); + } } // Webhook keys belong to installations, which stay on rollback; older versions ignore them. diff --git a/packages/core/src/plugins/pluginWebhookReceiver.ts b/packages/core/src/plugins/pluginWebhookReceiver.ts index 1cbd0962e..d5fcbfc53 100644 --- a/packages/core/src/plugins/pluginWebhookReceiver.ts +++ b/packages/core/src/plugins/pluginWebhookReceiver.ts @@ -21,12 +21,15 @@ import { * 2. ProPR verifies the request with the webhook's scheme and signing secret: * otherwise 401, counted on the installation, logged without the body or * any header. - * 3. A `respond` function may answer at once (a challenge echo): it gets the + * 3. The delivery id is derived (400 when the sender's is malformed) and + * reserved: one already recorded is a replay, 409, and no plugin code runs. + * 4. A `respond` function may answer at once (a challenge echo): it gets the * frozen request as its only argument, no `ctx`, and must return - * synchronously. - * 4. A delivery id already recorded is a replay: 409. + * synchronously. Its answer keeps the delivery id taken; when it fails + * (500) the reservation is given back, so the sender may send it again. * 5. Otherwise the delivery is queued (202) and the handler runs from the - * queue. While deliveries are paused, or the queue is full, 503. + * queue. While deliveries are paused, or the queue is full, 503 (and the + * reservation is given back). * * The body size limit and the rate limiter are applied by the route before this. */ @@ -46,6 +49,7 @@ export interface PluginWebhookReply { status: number; contentType: 'application/ const json = (status: number, value: Record, extra: Partial = {}): PluginWebhookReply => ({ status, contentType: 'application/json', body: JSON.stringify(value), ...extra }); const NOT_FOUND = json(404, { error: 'Not found' }); +const BUSY = json(503, { error: 'Too many webhook deliveries are waiting' }, { retryAfterSeconds: 60 }); /** A respond function's answer, checked: a 2xx status, a short text body and a supported content type. */ function checkedResponse(value: unknown): PluginWebhookResponse | null { @@ -84,16 +88,23 @@ export class PluginWebhookReceiver { if (typeof responder !== 'function') throw new Error(`Plugin ${target.pluginId} has no function ${webhook.respond}`); const response = checkedResponse(Reflect.apply(responder, undefined, [request])); return response ? { status: response.status!, contentType: response.contentType!, body: response.body } : null; - } catch (error) { - logger.warn({ pluginId: target.pluginId, webhook: webhook.name, error: error instanceof Error ? error.name : 'Error' }, '[plugins] Webhook respond function failed'); + } catch { + // Nothing of the error: a respond function may throw (or name its error after) any part of the request. + logger.warn({ pluginId: target.pluginId, webhook: webhook.name }, '[plugins] Webhook respond function failed'); return json(500, { error: 'The webhook could not answer' }); } } - private deliveryId(webhook: PluginWebhook, headers: IncomingHttpHeaders): string | null { + /** + * `whk-:`, or `whk-/` without one. + * A webhook name has neither `:` nor `/`, so the character after it ends the + * name unambiguously and tells a sender's id from ProPR's: distinct webhooks + * and ids never share a delivery id (`a` with `b-c` and `a-b` with `c` differ). + */ + private deliveryId(webhook: PluginWebhook, headers: IncomingHttpHeaders): { id: string; fromSender: boolean } | null { const sender = senderDeliveryId(headers, webhook); if (sender === null) return null; - return `whk-${webhook.name}-${sender ?? randomUUID()}`; + return sender === undefined ? { id: `whk-${webhook.name}/${randomUUID()}`, fromSender: false } : { id: `whk-${webhook.name}:${sender}`, fromSender: true }; } async receive(request: PluginWebhookRequestInput): Promise { @@ -107,19 +118,34 @@ export class PluginWebhookReceiver { const verdict = verifyPluginWebhook({ verification: webhook.verification, secret, headers: request.headers, body: request.body, now: receivedAt }); if (!verdict.ok) return this.reject(resolved, verdict.reason); - const body = request.body.toString('utf8'); - const headers = Object.freeze(handlerHeaders(request.headers, webhook)); - const answer = await this.respond(resolved, Object.freeze({ webhook: webhook.name, headers, body, receivedAt })); - if (answer) return answer; - - const deliveryId = this.deliveryId(webhook, request.headers); - if (deliveryId === null) return json(400, { error: `Invalid ${webhook.deliveryIdHeader} header` }); - const dispatch = await database('plugin_dispatch').where({ plugin_id: target.pluginId, installed_at: target.record.installedAt }).first('paused_at') as { paused_at: number | null } | undefined; - if (dispatch && dispatch.paused_at !== null) return json(503, { error: 'Deliveries of this plugin are paused' }, { retryAfterSeconds: 300 }); - const accepted = await this.options.runner.accept({ target, handler: webhook.handler, deliveryId, input: { kind: 'webhook', webhook: webhook.name, headers: { ...headers }, body, receivedAt } }); - if (!accepted.accepted) { - return accepted.reason === 'duplicate' ? json(409, { error: 'This delivery was already received' }) : json(503, { error: 'Too many webhook deliveries are waiting' }, { retryAfterSeconds: 60 }); + // The delivery id is checked and taken before any plugin code runs (the respond function included): a replay runs nothing. + const derived = this.deliveryId(webhook, request.headers); + if (derived === null) return json(400, { error: `Invalid ${webhook.deliveryIdHeader} header` }); + const { runner } = this.options; + const deliveryId = derived.id; + const reserved = await runner.reserve({ target, handler: webhook.handler, deliveryId, webhook: webhook.name }); + if (!reserved.accepted) return reserved.reason === 'duplicate' ? json(409, { error: 'This delivery was already received' }) : BUSY; + const { recordId } = reserved; + try { + const body = request.body.toString('utf8'); + const headers = Object.freeze(handlerHeaders(request.headers, webhook)); + const answer = await this.respond(resolved, Object.freeze({ webhook: webhook.name, headers, body, receivedAt })); + if (answer) { + // Answered: a sender's delivery id stays taken, so its replay is refused; ProPR's own id is never sent again. A failed respond function delivered nothing. + if (answer.status < 300 && derived.fromSender) await runner.answered(recordId, webhook.respond!); + else await runner.release(recordId); + return answer; + } + const dispatch = await database('plugin_dispatch').where({ plugin_id: target.pluginId, installed_at: target.record.installedAt }).first('paused_at') as { paused_at: number | null } | undefined; + if (dispatch && dispatch.paused_at !== null) { + await runner.release(recordId); + return json(503, { error: 'Deliveries of this plugin are paused' }, { retryAfterSeconds: 300 }); + } + if (!await runner.start(recordId, { target, handler: webhook.handler, deliveryId, input: { kind: 'webhook', webhook: webhook.name, headers: { ...headers }, body, receivedAt } })) return BUSY; + return json(202, { accepted: true, deliveryId }); + } catch (error) { + await runner.release(recordId).catch(() => undefined); + throw error; } - return json(202, { accepted: true, deliveryId }); } } diff --git a/packages/core/src/plugins/pluginWebhookRunner.ts b/packages/core/src/plugins/pluginWebhookRunner.ts index 068a0c89e..726114647 100644 --- a/packages/core/src/plugins/pluginWebhookRunner.ts +++ b/packages/core/src/plugins/pluginWebhookRunner.ts @@ -1,9 +1,10 @@ +import { randomUUID } from 'node:crypto'; import type { Knex } from 'knex'; -import { PLUGIN_DELIVERY_RETRY_DELAYS_MS, PLUGIN_WEBHOOK_LIMITS, type PluginHandlerRun, type PluginWebhookInput } from '@propr/shared'; +import { PLUGIN_DELIVERY_RETRY_DELAYS_MS, PLUGIN_WEBHOOK_LIMITS, type PluginHandlerRun, type PluginLogLine, type PluginWebhookInput } from '@propr/shared'; import logger from '../utils/logger.js'; import { runPluginHandler, type RunPluginHandlerOptions } from './pluginHandlerRunner.js'; import { installationRecord, type PluginInstallationRow } from './pluginState.js'; -import { clipError, storedLogs, type PluginDeliveryRow } from './pluginDeliveries.js'; +import { storedLogs, type PluginDeliveryRow } from './pluginDeliveries.js'; import type { PluginInstallationTarget } from './pluginContext.js'; /** @@ -14,13 +15,26 @@ import type { PluginInstallationTarget } from './pluginContext.js'; * * The request (body and headers) is held in this process's memory only, never * written anywhere: the delivery record carries the webhook and handler - * names, the delivery id, status, timing and the handler's redacted error and - * log lines. A delivery still waiting when the process stops cannot be - * resumed by another one and is marked failed. + * names, the delivery id, status and timing. The handler's error and log + * messages are not kept either, since a handler may log or throw any part of + * the request: only each log line's level and time, and whether the attempt + * failed or timed out (see `webhookDiagnostics`). + * + * Each delivery record names the runner holding its request + * (`webhook_owner`), and each runner keeps a heartbeat in + * `plugin_webhook_runners`. A delivery whose owner stopped cannot be resumed + * by another process: once the owner's heartbeat has lapsed, any runner marks + * it failed. A live runner's deliveries are never touched by another. */ export interface PluginWebhookRunnerOptions extends Omit { database: Knex; retryDelaysMs?: readonly number[]; + /** This runner's owner name; unique per process by default. */ + owner?: string; + /** Interval between heartbeats (and recoveries of deliveries whose owner stopped). */ + heartbeatMs?: number; + /** A runner whose heartbeat is older than this is taken to have stopped. */ + ownerTtlMs?: number; /** Deliveries held in memory at once; further requests are refused (503) until some finish. */ maxPending?: number; run?: typeof runPluginHandler; @@ -29,7 +43,7 @@ export interface PluginWebhookRunnerOptions extends Omit-`. */ + /** The delivery id the handler receives: `whk-:`, or `whk-/` without one. */ deliveryId: string; input: Omit; } @@ -39,6 +53,20 @@ interface Held { job: PluginWebhookJob; recordId: number; attempt: number; timer const LOST = 'Not delivered: ProPR stopped before the delivery finished, and webhook requests are not kept'; const UNFINISHED = ['pending', 'running', 'retrying'] as const; +const DEFAULT_HEARTBEAT_MS = 30_000; +const DEFAULT_OWNER_TTL_MS = 120_000; +const WITHHELD_LOG = 'Message not kept: webhook handler output may contain the request'; + +/** + * What a webhook attempt keeps of the handler's diagnostics: its log lines' + * levels and times, and a fixed error. Never a message, field or error text + * the handler wrote, which may repeat any part of the request (the + * credential redaction does not recognize arbitrary request content). + */ +export function webhookDiagnostics(run: Pick): { error: string | null; logs: PluginLogLine[] } { + const error = run.outcome === 'succeeded' ? null : run.timedOut ? 'The webhook handler timed out' : 'The webhook handler failed (its error is not kept: it may contain the request)'; + return { error, logs: run.logs.map(line => ({ level: line.level, message: WITHHELD_LOG, at: line.at })) }; +} export class PluginWebhookRunner { private readonly held = new Map(); @@ -47,41 +75,128 @@ export class PluginWebhookRunner { private readonly due = new Set(); private readonly now: () => number; private readonly delays: readonly number[]; + /** Records reserved (`reserve`) and not yet started, released or answered. */ + private readonly reserved = new Set(); private closed = false; + /** This runner's name in `plugin_deliveries.webhook_owner` and `plugin_webhook_runners`. */ + readonly owner: string; + private registered?: Promise; + private heartbeat?: NodeJS.Timeout; constructor(private readonly options: PluginWebhookRunnerOptions) { this.now = options.now ?? Date.now; this.delays = options.retryDelaysMs ?? PLUGIN_DELIVERY_RETRY_DELAYS_MS; + this.owner = options.owner ?? `webhooks-${process.pid}-${randomUUID()}`; } /** Deliveries held right now (waiting, running or retrying). */ get pending(): number { return this.held.size; } - /** Mark webhook deliveries an earlier process left unfinished as failed: their requests were never stored. */ - async recover(): Promise { - return this.options.database('plugin_deliveries').where({ kind: 'webhook' }).whereIn('status', UNFINISHED) - .modify(q => { if (this.held.size) void q.whereNotIn('id', [...this.held.keys()]); }) + /** Record this runner's heartbeat; the first call also starts renewing it. Before any record names this runner as its owner. */ + private register(): Promise { + this.registered ??= this.beat().then(() => { + if (this.closed) return; + this.heartbeat = setInterval(() => { + void this.beat().then(() => this.recoverStopped()).catch((error: unknown) => { + logger.warn({ owner: this.owner, error: error instanceof Error ? error.message : String(error) }, '[plugins] Webhook runner heartbeat failed'); + }); + }, this.options.heartbeatMs ?? DEFAULT_HEARTBEAT_MS); + this.heartbeat.unref?.(); + }, (error: unknown) => { this.registered = undefined; throw error; }); + return this.registered; + } + + private async beat(): Promise { + if (this.closed) return; + await this.options.database('plugin_webhook_runners').insert({ owner: this.owner, heartbeat_at: this.now() }).onConflict('owner').merge(['heartbeat_at']); + } + + /** + * Mark failed the unfinished webhook deliveries whose owner has stopped + * (its heartbeat lapsed, or the record has no owner): their requests were + * never stored. One statement, so a live owner's renewed heartbeat and its + * deliveries are read together; this runner's own are never touched. + */ + private async recoverStopped(): Promise { + const { database } = this.options; + const cutoff = this.now() - (this.options.ownerTtlMs ?? DEFAULT_OWNER_TTL_MS); + const failed = await database('plugin_deliveries').where({ kind: 'webhook' }).whereIn('status', UNFINISHED) + .where(q => { + void q.whereNull('webhook_owner').orWhere(owner => { + void owner.whereNot('webhook_owner', this.owner) + .whereNotIn('webhook_owner', database('plugin_webhook_runners').where('heartbeat_at', '>=', cutoff).select('owner')); + }); + }) .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }); + await database('plugin_webhook_runners').where('heartbeat_at', '<', cutoff).whereNot('owner', this.owner).delete(); + return failed; } /** - * Record the delivery and run it soon. Refused as `duplicate` when the + * Start this runner's heartbeat and mark failed the webhook deliveries left + * unfinished by runners that have stopped. Deliveries of runners still alive + * (in other API processes) are left alone. Repeated with every heartbeat. + */ + async recover(): Promise { + await this.register(); + return this.recoverStopped(); + } + + /** + * Record the delivery, owned by this runner, without running it yet: the + * delivery id is taken, so a replay is refused while the receiver decides. + * Then `start`, `answered` or `release` it. Refused as `duplicate` when the * installation already recorded this delivery id (a replay), and as `busy` * when too many deliveries are held. */ - async accept(job: PluginWebhookJob): Promise { - if (this.closed || this.held.size >= (this.options.maxPending ?? PLUGIN_WEBHOOK_LIMITS.pending)) return { accepted: false, reason: 'busy' }; + async reserve(job: Pick & { webhook: string }): Promise { + if (this.closed || this.held.size + this.reserved.size >= (this.options.maxPending ?? PLUGIN_WEBHOOK_LIMITS.pending)) return { accepted: false, reason: 'busy' }; + await this.register(); const now = this.now(); const inserted = await this.options.database('plugin_deliveries').insert({ plugin_id: job.target.pluginId, delivery_id: job.deliveryId, kind: 'webhook', repository: '', event_seq: 0, - event_type: `webhook.${job.input.webhook}`, handler: job.handler, status: 'pending', attempt: 0, next_attempt_at: now, created_at: now, logs: '[]', + event_type: `webhook.${job.webhook}`, handler: job.handler, status: 'pending', attempt: 0, next_attempt_at: now, created_at: now, logs: '[]', + webhook_owner: this.owner, }).onConflict(['plugin_id', 'delivery_id']).ignore().returning('id') as Array<{ id: number } | number>; const first = inserted[0]; if (first === undefined) return { accepted: false, reason: 'duplicate' }; const recordId = typeof first === 'number' ? first : Number(first.id); + this.reserved.add(recordId); + return { accepted: true, recordId }; + } + + /** Give a reserved delivery id back (the request was refused before delivery): the sender may send it again. */ + async release(recordId: number): Promise { + if (!this.reserved.delete(recordId)) return; + await this.options.database('plugin_deliveries').where({ id: recordId, status: 'pending', webhook_owner: this.owner }).delete(); + } + + /** A reserved delivery answered at once by the webhook's respond function: final, and its delivery id stays taken. */ + async answered(recordId: number, respond: string): Promise { + if (!this.reserved.delete(recordId)) return; + const now = this.now(); + await this.options.database('plugin_deliveries').where({ id: recordId, status: 'pending', webhook_owner: this.owner }) + .update({ status: 'succeeded', handler: respond, next_attempt_at: null, started_at: now, finished_at: now, duration_ms: 0 }); + } + + /** Run a reserved delivery soon. False when this runner has stopped since: the record is marked failed. */ + async start(recordId: number, job: PluginWebhookJob): Promise { + if (!this.reserved.delete(recordId)) return false; + if (this.closed) { + await this.options.database('plugin_deliveries').where({ id: recordId, status: 'pending', webhook_owner: this.owner }) + .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }); + return false; + } this.held.set(recordId, { job, recordId, attempt: 0 }); this.schedule(recordId, 0); - return { accepted: true, recordId }; + return true; + } + + /** `reserve` then `start`: record the delivery and run it soon. */ + async accept(job: PluginWebhookJob): Promise { + const reserved = await this.reserve({ ...job, webhook: job.input.webhook }); + if (!reserved.accepted) return reserved; + return await this.start(reserved.recordId, job) ? reserved : { accepted: false, reason: 'busy' }; } /** Wait until no attempt is under way or due at once: every held delivery is final or waits for a retry (tests). */ @@ -95,6 +210,7 @@ export class PluginWebhookRunner { /** Stop: no further attempt starts; attempts under way finish; deliveries still waiting are marked failed. */ async stop(): Promise { this.closed = true; + clearInterval(this.heartbeat); for (const held of this.held.values()) clearTimeout(held.timer); this.due.clear(); await this.idle(); @@ -102,6 +218,9 @@ export class PluginWebhookRunner { this.held.clear(); if (ids.length) await this.options.database('plugin_deliveries').whereIn('id', ids).whereIn('status', UNFINISHED) .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }).catch(() => undefined); + // Wait for a heartbeat under way, then withdraw it: another process may fail what is left at once. + await this.registered?.catch(() => undefined); + await this.options.database('plugin_webhook_runners').where({ owner: this.owner }).delete().catch(() => undefined); } private schedule(recordId: number, delayMs: number): void { @@ -158,8 +277,9 @@ export class PluginWebhookRunner { const status = run.outcome === 'succeeded' ? 'succeeded' : attempt > this.delays.length ? 'failed' : 'retrying'; const nextAttemptAt = status === 'retrying' ? now + this.delays[attempt - 1] : null; // Only while the record is still this attempt's: a restart (enabled again) or an uninstall since then wins. + const diagnostics = webhookDiagnostics(run); const recorded = await database('plugin_deliveries').where({ id: held.recordId, status: 'running', attempt }).update({ - status, finished_at: now, duration_ms: run.durationMs, timed_out: run.timedOut, error: clipError(run.error), logs: storedLogs(run.logs), next_attempt_at: nextAttemptAt, + status, finished_at: now, duration_ms: run.durationMs, timed_out: run.timedOut, error: diagnostics.error, logs: storedLogs(diagnostics.logs), next_attempt_at: nextAttemptAt, }); if (recorded !== 1 || status !== 'retrying') { this.held.delete(held.recordId); return; } this.schedule(held.recordId, nextAttemptAt! - now); diff --git a/packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts b/packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts index ddb3e458d..4be099631 100644 --- a/packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts +++ b/packages/core/test/fixtures/plugins/fixture-webhooks/handlers.ts @@ -33,6 +33,22 @@ export function slackChallenge(request: PluginWebhookRequest): PluginWebhookResp return body.type === 'url_verification' && typeof body.challenge === 'string' ? { contentType: 'text/plain', body: body.challenge } : null; } +/** Deliberately unsafe: writes the request body into its log message, its log fields and its error. */ +export async function leakyWebhook(input: PluginWebhookInput, ctx: PluginContext) { + ctx.log.info(input.body); + ctx.log.warn('unexpected payload', { body: input.body, text: (JSON.parse(input.body) as { text?: string }).text ?? null }); + throw new Error(`unexpected payload: ${input.body}`); +} + +const challenges: string[] = []; +/** `slackChallenge`, counting its calls (it has no ctx to count with). */ +export function countedChallenge(request: PluginWebhookRequest): PluginWebhookResponse | null { + challenges.push(request.webhook); + return slackChallenge(request); +} +/** How many times `countedChallenge` ran, for the tests. */ +export function challengeCalls() { return challenges.length; } + const probes: { args: number; keys: string[]; frozen: boolean }[] = []; /** Records what it was called with, then reaches for a `ctx` it never gets. */ export function probeRespond(request: PluginWebhookRequest, ...rest: unknown[]): PluginWebhookResponse { diff --git a/packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json b/packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json index e270b7989..018253ada 100644 --- a/packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json +++ b/packages/core/test/fixtures/plugins/fixture-webhooks/manifest.json @@ -2,7 +2,7 @@ "manifestVersion": 1, "id": "fixture-webhooks", "name": "Fixture webhooks", - "description": "Test fixture for plugin inbound webhooks: one webhook per verification scheme, a Slack challenge responder, a failing handler and a respond function that reaches for ctx.", + "description": "Test fixture for plugin inbound webhooks: one webhook per verification scheme, a Slack challenge responder, a failing handler and a respond function that reaches for ctx, a counted challenge responder with a delivery id header, two names one extending the other, and a handler that leaks the body into its log and error.", "version": "1.0.0", "category": "other", "homepage": "https://example.com/fixture-webhooks", @@ -28,6 +28,25 @@ { "name": "probe", "description": "A respond function that reaches for ctx", "handler": "onWebhook", "respond": "probeRespond", "verification": { "scheme": "token-header", "header": "X-Gitlab-Token", "connection": "hook-secret" } + }, + { + "name": "answered", "description": "A counted challenge responder with a sender delivery id", "handler": "onWebhook", "respond": "countedChallenge", + "verification": { "scheme": "token-header", "header": "X-Gitlab-Token", "connection": "hook-secret" }, + "deliveryIdHeader": "X-Delivery-Id" + }, + { + "name": "a", "description": "A short name, for delivery id collisions", "handler": "onWebhook", + "verification": { "scheme": "token-header", "header": "X-Gitlab-Token", "connection": "hook-secret" }, + "deliveryIdHeader": "X-Delivery-Id" + }, + { + "name": "a-b", "description": "A name extending another, for delivery id collisions", "handler": "onWebhook", + "verification": { "scheme": "token-header", "header": "X-Gitlab-Token", "connection": "hook-secret" }, + "deliveryIdHeader": "X-Delivery-Id" + }, + { + "name": "leaky", "description": "A handler that logs and throws the request body", "handler": "leakyWebhook", + "verification": { "scheme": "token-header", "header": "X-Gitlab-Token", "connection": "hook-secret" } } ] } diff --git a/packages/core/test/pluginWebhooks.test.ts b/packages/core/test/pluginWebhooks.test.ts index 21ccdf108..06bf0a744 100644 --- a/packages/core/test/pluginWebhooks.test.ts +++ b/packages/core/test/pluginWebhooks.test.ts @@ -98,15 +98,15 @@ test('a verified request runs its handler once; a bad signature, a stale timesta const body = '{"action":"opened"}'; const accepted = await s.send('signed', body, { 'x-hub-signature-256': hubSignature(body), 'x-github-delivery': 'd-1', 'content-type': 'application/json', cookie: 'sid=1', authorization: 'Basic eA==' }); assert.equal(accepted.status, 202); - assert.deepEqual(JSON.parse(accepted.body), { accepted: true, deliveryId: 'whk-signed-d-1' }); + assert.deepEqual(JSON.parse(accepted.body), { accepted: true, deliveryId: 'whk-signed:d-1' }); await s.runner.idle(); const seen = await s.seen(); assert.equal(seen.length, 1); - assert.deepEqual(seen[0], { id: 'whk-signed-d-1', webhook: 'signed', attempt: 1, bodyBytes: Buffer.byteLength(body), headers: ['content-type', 'x-github-delivery'] }, + assert.deepEqual(seen[0], { id: 'whk-signed:d-1', webhook: 'signed', attempt: 1, bodyBytes: Buffer.byteLength(body), headers: ['content-type', 'x-github-delivery'] }, 'the handler gets the body, but not the signature, cookie or authorization headers'); const [delivery] = await s.deliveries(); assert.deepEqual({ kind: delivery.kind, status: delivery.status, eventType: delivery.eventType, handler: delivery.handler, attempt: delivery.attempt, deliveryId: delivery.deliveryId }, - { kind: 'webhook', status: 'succeeded', eventType: 'webhook.signed', handler: 'onWebhook', attempt: 1, deliveryId: 'whk-signed-d-1' }); + { kind: 'webhook', status: 'succeeded', eventType: 'webhook.signed', handler: 'onWebhook', attempt: 1, deliveryId: 'whk-signed:d-1' }); const loadsBefore = s.loads.length; const replay = await s.send('signed', body, { 'x-hub-signature-256': hubSignature(body), 'x-github-delivery': 'd-1' }); @@ -210,9 +210,10 @@ test('a failing webhook handler is retried on the delivery schedule, then marked assert.equal((await s.send('token', '{"n":2}', { 'x-gitlab-token': SECRET })).status, 202); await settle(); [delivery] = await s.deliveries(); - assert.deepEqual({ status: delivery.status, attempt: delivery.attempt, error: delivery.error }, { status: 'failed', attempt: 3, error: 'webhook failure, attempt 3' }, 'three attempts with two retry delays, then failed'); + assert.deepEqual({ status: delivery.status, attempt: delivery.attempt, error: delivery.error }, { status: 'failed', attempt: 3, error: 'The webhook handler failed (its error is not kept: it may contain the request)' }, 'three attempts with two retry delays, then failed, without the error text of the handler'); const orphan = new PluginWebhookRunner({ ...s.f.options, retryDelaysMs: [60_000] }); + t.after(() => orphan.stop()); await s.f.database('plugin_deliveries').insert({ plugin_id: ID, delivery_id: 'whk-token-orphan', kind: 'webhook', repository: '', event_seq: 0, event_type: 'webhook.token', handler: 'flakyWebhook', status: 'retrying', attempt: 1, created_at: NOW, logs: '[]' }); assert.equal(await orphan.recover(), 1); const lost = (await s.deliveries()).find(d => d.deliveryId === 'whk-token-orphan')!; @@ -255,3 +256,95 @@ test('request bodies and signature headers are never stored or logged (sentinel) assert.ok(!lines.some(line => line.includes(sentinel)), `${sentinel} is not logged`); } }); + +test('a responder runs only after its delivery id is reserved: a replay, challenge or not, runs no plugin code', async t => { + const s = await setup(t); + const module = await loadPluginHandlers(ID, { root: FIXTURE_REGISTRY, includeDevelopment: true }); + const calls = () => (module!.challengeCalls as unknown as () => number)(); + const send = (body: string, id: string) => s.send('answered', body, { 'x-gitlab-token': SECRET, 'x-delivery-id': id }); + const event = '{"type":"event_callback"}'; + const before = calls(); + assert.equal((await send(event, 'r-1')).status, 202); + await s.runner.idle(); + assert.equal(calls(), before + 1); + const loads = s.loads.length; + assert.equal((await send(event, 'r-1')).status, 409, 'the replayed ordinary event is refused'); + assert.equal(calls(), before + 1, 'without running the responder again'); + assert.equal(s.loads.length, loads, 'or loading the plugin module'); + + const challenge = JSON.stringify({ type: 'url_verification', challenge: 'c-123' }); + assert.deepEqual(await send(challenge, 'r-2'), { status: 200, contentType: 'text/plain', body: 'c-123' }); + assert.equal(calls(), before + 2); + assert.equal((await send(challenge, 'r-2')).status, 409, 'an answered challenge keeps its delivery id taken'); + assert.equal(calls(), before + 2, 'its replay runs no responder'); + assert.equal(s.loads.length, loads + 1); + const answered = (await s.deliveries()).find(d => d.deliveryId === 'whk-answered:r-2')!; + assert.deepEqual({ status: answered.status, handler: answered.handler }, { status: 'succeeded', handler: 'countedChallenge' }); + + // Refused before delivery (paused): the reservation is given back, so the sender's retry is delivered. + const installedAt = Number((await s.f.database('plugin_installations').where({ plugin_id: ID }).first('installed_at')).installed_at); + await s.f.database('plugin_dispatch').insert({ plugin_id: ID, installed_at: installedAt, start_seq: 0, updated_at: NOW, paused_at: NOW }).onConflict('plugin_id').merge(['paused_at']); + assert.equal((await send(event, 'r-3')).status, 503); + await s.f.database('plugin_dispatch').where({ plugin_id: ID }).update({ paused_at: null }); + assert.equal((await send(event, 'r-3')).status, 202, 'the retry of a refused request is not a replay'); + await s.runner.idle(); + assert.deepEqual((await s.seen()).map(x => x.id), ['whk-answered:r-1', 'whk-answered:r-3']); +}); + +test('delivery ids of different webhooks never collide', async t => { + const s = await setup(t); + const a = await s.send('a', '{}', { 'x-gitlab-token': SECRET, 'x-delivery-id': 'b-c' }); + const ab = await s.send('a-b', '{}', { 'x-gitlab-token': SECRET, 'x-delivery-id': 'c' }); + assert.deepEqual([a.status, ab.status], [202, 202], 'webhook a with id b-c and webhook a-b with id c are two deliveries'); + assert.notEqual(JSON.parse(a.body).deliveryId, JSON.parse(ab.body).deliveryId); + const propr = await s.send('a', '{}', { 'x-gitlab-token': SECRET }); + assert.match(JSON.parse(propr.body).deliveryId as string, /^whk-a\/[0-9a-f-]{36}$/, 'a ProPR id is spelled apart from any sender id'); + await s.runner.idle(); + assert.deepEqual((await s.seen()).map(x => [x.webhook, x.id]).slice(0, 2), [['a', 'whk-a:b-c'], ['a-b', 'whk-a-b:c']]); +}); + +test('a handler that logs or throws the request body has neither stored nor logged (sentinel)', async t => { + const s = await setup(t, { retryDelaysMs: [] }); + const lines: string[] = []; + for (const level of ['trace', 'debug', 'info', 'warn', 'error', 'fatal'] as const) { + mock.method(logger, level, (...args: unknown[]) => { lines.push(JSON.stringify(args, (_k, v: unknown) => (v instanceof Error ? `${v.name}: ${v.message}` : v))); }); + } + t.after(() => mock.restoreAll()); + const BODY = 'customer-private-message-123'; + assert.equal((await s.send('leaky', JSON.stringify({ text: BODY }), { 'x-gitlab-token': SECRET })).status, 202); + await s.runner.idle(); + const [delivery] = await s.deliveries(); + assert.equal(delivery.status, 'failed'); + assert.equal(delivery.error, 'The webhook handler failed (its error is not kept: it may contain the request)'); + assert.deepEqual(delivery.logs.map(line => [line.level, line.fields]), [['info', undefined], ['warn', undefined]], 'the lines are kept as levels and times only'); + const tables = (await s.f.database.raw("SELECT name FROM sqlite_master WHERE type = 'table'") as { name: string }[]).map(row => row.name); + for (const table of tables) assert.ok(!JSON.stringify(await s.f.database(table).select()).includes(BODY), `the body is not stored in ${table}`); + assert.ok(lines.length > 0); + assert.ok(!lines.some(line => line.includes(BODY)), 'the body is not logged'); +}); + +test('starting another runner leaves a live runner\'s deliveries alone and fails only those of a stopped one', async t => { + const s = await setup(t, { retryDelaysMs: [30] }); + await s.f.database('plugin_state').insert({ plugin_id: ID, state_key: 'fail', value: '1', bytes: 1, updated_at: 0 }); + assert.equal((await s.send('token', '{}', { 'x-gitlab-token': SECRET })).status, 202); + await s.runner.idle(); + assert.equal((await s.deliveries())[0].status, 'retrying', 'A holds a delivery waiting for its retry'); + + // B starts against the same database while A is alive. + const other = new PluginWebhookRunner({ ...s.f.options, now: () => s.clock.now }); + t.after(() => other.stop()); + assert.equal(await other.recover(), 0, 'B fails nothing of A'); + assert.equal((await s.deliveries())[0].status, 'retrying'); + for (let i = 0; i < 50 && (s.runner.pending || (await s.runner.idle(), s.runner.pending)); i++) await new Promise(r => setTimeout(r, 10)); + const [delivery] = await s.deliveries(); + assert.deepEqual({ status: delivery.status, attempt: delivery.attempt }, { status: 'succeeded', attempt: 2 }, 'A\'s retry ran'); + + // A runner whose heartbeat lapsed has stopped: its unfinished delivery is failed by the next recovery. + await s.f.database('plugin_webhook_runners').insert({ owner: 'webhooks-stopped', heartbeat_at: NOW - 121_000 }); + await s.f.database('plugin_deliveries').insert({ plugin_id: ID, delivery_id: 'whk-token/stopped', kind: 'webhook', repository: '', event_seq: 0, event_type: 'webhook.token', handler: 'flakyWebhook', status: 'retrying', attempt: 1, created_at: NOW, logs: '[]', webhook_owner: 'webhooks-stopped' }); + assert.equal(await other.recover(), 1); + const lost = (await s.deliveries()).find(d => d.deliveryId === 'whk-token/stopped')!; + assert.equal(lost.status, 'failed'); + assert.equal(await s.f.database('plugin_webhook_runners').where({ owner: 'webhooks-stopped' }).first(), undefined, 'the stopped runner is forgotten'); + assert.ok(await s.f.database('plugin_webhook_runners').where({ owner: s.runner.owner }).first(), 'A is still registered'); +}); diff --git a/packages/shared/src/pluginDeliveries.ts b/packages/shared/src/pluginDeliveries.ts index 6653ca2f9..80099d6ae 100644 --- a/packages/shared/src/pluginDeliveries.ts +++ b/packages/shared/src/pluginDeliveries.ts @@ -36,7 +36,7 @@ export type PluginDeliveryKind = typeof PLUGIN_DELIVERY_KINDS[number]; export interface PluginDeliveryView { /** The activity record id, used by the retry route. */ id: number; - /** The stable delivery id handlers receive (`evt--`, `whk--`); the same on every attempt. */ + /** The stable delivery id handlers receive (`evt--`, `whk-:`, `whk-/`); the same on every attempt. */ deliveryId: string; kind: PluginDeliveryKind; /** The event's repository; empty for a webhook delivery. */ diff --git a/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx b/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx index 94ee03f39..0d216d751 100644 --- a/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx +++ b/propr-ui/src/pages/PluginsPage/PluginActivity.test.tsx @@ -106,7 +106,7 @@ describe('Plugin activity', () => { }); it('lists a webhook delivery by its webhook, without a repository, event number or Retry (its request is not kept)', async () => { - const webhook = delivery({ id: 5, deliveryId: 'whk-events-d-1', kind: 'webhook', repository: '', eventSeq: 0, eventType: 'webhook.events', handler: 'onEvent', status: 'failed', attempt: 5, error: 'chat API answered 503', logs: [] }); + const webhook = delivery({ id: 5, deliveryId: 'whk-events:d-1', kind: 'webhook', repository: '', eventSeq: 0, eventType: 'webhook.events', handler: 'onEvent', status: 'failed', attempt: 5, error: 'chat API answered 503', logs: [] }); await open({ health: healthy, deliveries: [webhook] }); const [item] = items(); expect(item).toHaveTextContent('webhook.events'); From c8af15388443e5ac79f0923f8058ea3c0d7ca1f2 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Sun, 15 Nov 2026 19:44:24 +0000 Subject: [PATCH 4/5] feat(ai): I fixed F5, F6 and F7. Three new regression tests pass and fail when I put the old runner and receiver code back. All 13 tests in `pluginWebhooks.test.ts` pass, and so do `pluginWebhookRoutes.test.ts` and `pluginStore.test.ts`. Core typecheck (including the changed test file) and the zero-warning lint on the two changed source files are clean. I fixed F5, F6 and F7. Three new regression tests pass and fail when I put the old runner and receiver code back. All 13 tests in `pluginWebhooks.test.ts` pass, and so do `pluginWebhookRoutes.test.ts` and `pluginStore.test.ts`. Core typecheck (including the changed test file) and the zero-warning lint on the two changed source files are clean. **F5: async responder rejections.** The check that refuses a promise-shaped answer (`checkedResponse` in `pluginWebhookReceiver.ts`) now attaches a no-op rejection handler to the promise before throwing. The request still gets the same 500 and its reservation is still given back. The regression uses an `async` responder that throws at once and one that throws after an await. It records unhandled rejections and also checks that nothing was queued. On the old code Node reported the after-await rejection as unhandled. **F6: queue capacity under concurrency.** `reserve` in `pluginWebhookRunner.ts` now takes a capacity slot before its first await. That slot counts towards the limit along with held and reserved deliveries. It turns into a reservation at the same moment the record is added, and is given back on every other path: duplicate, stale installation, shutdown during registration, or an error. The regression sends a burst of 5 concurrent reserves at `maxPending: 2` and exactly 2 get in. It also checks that a duplicate or a stale request leaves its slot free, and that a released slot can be used once. **F7: stale requests after uninstall.** `reserve` now checks the installation and inserts the record in the same transaction. If the installation is gone, has a different `installed_at`, or is disabled, it refuses with a new reason, `gone`, and records nothing; the receiver answers that with 404, as it would for a dead URL. The database is a single SQLite connection, so this transaction runs strictly before or after uninstall's transaction: either nothing is inserted, or uninstall deletes the row. The regression holds a verified request after its key was resolved, then uninstalls, reinstalls and re-enables the plugin before letting it continue. The held request gets 404 and leaves no record, and the same delivery id sent to the new URL gets 202 and runs. I looked at the other places the runner acts on a reserved record (`start`, `answered`, `release` and the attempt loop). After an uninstall they find no record and do nothing, so the same defect isn't there and they needed no change. There are no visual changes, so I made no previews. PR: #3579 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5 --- .../core/src/plugins/pluginWebhookReceiver.ts | 14 +++- .../core/src/plugins/pluginWebhookRunner.ts | 55 +++++++++---- packages/core/test/pluginWebhooks.test.ts | 82 ++++++++++++++++++- 3 files changed, 132 insertions(+), 19 deletions(-) diff --git a/packages/core/src/plugins/pluginWebhookReceiver.ts b/packages/core/src/plugins/pluginWebhookReceiver.ts index d5fcbfc53..0c0857850 100644 --- a/packages/core/src/plugins/pluginWebhookReceiver.ts +++ b/packages/core/src/plugins/pluginWebhookReceiver.ts @@ -54,7 +54,13 @@ const BUSY = json(503, { error: 'Too many webhook deliveries are waiting' }, { r /** A respond function's answer, checked: a 2xx status, a short text body and a supported content type. */ function checkedResponse(value: unknown): PluginWebhookResponse | null { if (value === null || value === undefined) return null; - if (typeof value !== 'object' || typeof (value as { then?: unknown }).then === 'function') throw new Error('A respond function must return a response synchronously'); + if (typeof value !== 'object') throw new Error('A respond function must return a response synchronously'); + const then = (value as { then?: unknown }).then; + if (typeof then === 'function') { + // Refused, but its rejection is consumed: an async respond function that throws must not leave an unhandled rejection behind. + try { Reflect.apply(then, value, [undefined, () => undefined]); } catch { /* a thenable whose then throws: nothing to consume */ } + throw new Error('A respond function must return a response synchronously'); + } const { status = 200, contentType = 'text/plain', body } = value as PluginWebhookResponse; if (!Number.isInteger(status) || status < 200 || status > 299) throw new Error('A respond function answers with a 2xx status'); if (contentType !== 'text/plain' && contentType !== 'application/json') throw new Error('A respond function answers text/plain or application/json'); @@ -124,7 +130,11 @@ export class PluginWebhookReceiver { const { runner } = this.options; const deliveryId = derived.id; const reserved = await runner.reserve({ target, handler: webhook.handler, deliveryId, webhook: webhook.name }); - if (!reserved.accepted) return reserved.reason === 'duplicate' ? json(409, { error: 'This delivery was already received' }) : BUSY; + if (!reserved.accepted) { + // Uninstalled, reinstalled or disabled since the key was resolved: as a request to a dead URL. + if (reserved.reason === 'gone') return NOT_FOUND; + return reserved.reason === 'duplicate' ? json(409, { error: 'This delivery was already received' }) : BUSY; + } const { recordId } = reserved; try { const body = request.body.toString('utf8'); diff --git a/packages/core/src/plugins/pluginWebhookRunner.ts b/packages/core/src/plugins/pluginWebhookRunner.ts index 726114647..3d1b174f5 100644 --- a/packages/core/src/plugins/pluginWebhookRunner.ts +++ b/packages/core/src/plugins/pluginWebhookRunner.ts @@ -47,7 +47,8 @@ export interface PluginWebhookJob { deliveryId: string; input: Omit; } -export type PluginWebhookAcceptance = { accepted: true; recordId: number } | { accepted: false; reason: 'duplicate' | 'busy' }; +/** `gone`: the installation that accepted the request was uninstalled, reinstalled or disabled since; nothing was recorded. */ +export type PluginWebhookAcceptance = { accepted: true; recordId: number } | { accepted: false; reason: 'duplicate' | 'busy' | 'gone' }; interface Held { job: PluginWebhookJob; recordId: number; attempt: number; timer?: NodeJS.Timeout } @@ -77,6 +78,8 @@ export class PluginWebhookRunner { private readonly delays: readonly number[]; /** Records reserved (`reserve`) and not yet started, released or answered. */ private readonly reserved = new Set(); + /** Capacity slots claimed by `reserve` calls still awaiting their record: counted with `held` and `reserved`, so concurrent requests cannot exceed `maxPending`. */ + private claiming = 0; private closed = false; /** This runner's name in `plugin_deliveries.webhook_owner` and `plugin_webhook_runners`. */ readonly owner: string; @@ -146,23 +149,43 @@ export class PluginWebhookRunner { * Record the delivery, owned by this runner, without running it yet: the * delivery id is taken, so a replay is refused while the receiver decides. * Then `start`, `answered` or `release` it. Refused as `duplicate` when the - * installation already recorded this delivery id (a replay), and as `busy` - * when too many deliveries are held. + * installation already recorded this delivery id (a replay), as `busy` when + * too many deliveries are held, and as `gone` when the target installation + * is no longer installed (as the same installation) and enabled. + * + * The capacity slot is claimed before the first await and handed to + * `reserved` (or given back) when the record is written or refused, so + * concurrent requests are admitted against one count. The installation is + * checked in the transaction that writes the record: an uninstall either + * commits first (nothing is recorded) or after (and deletes the record). */ async reserve(job: Pick & { webhook: string }): Promise { - if (this.closed || this.held.size + this.reserved.size >= (this.options.maxPending ?? PLUGIN_WEBHOOK_LIMITS.pending)) return { accepted: false, reason: 'busy' }; - await this.register(); - const now = this.now(); - const inserted = await this.options.database('plugin_deliveries').insert({ - plugin_id: job.target.pluginId, delivery_id: job.deliveryId, kind: 'webhook', repository: '', event_seq: 0, - event_type: `webhook.${job.webhook}`, handler: job.handler, status: 'pending', attempt: 0, next_attempt_at: now, created_at: now, logs: '[]', - webhook_owner: this.owner, - }).onConflict(['plugin_id', 'delivery_id']).ignore().returning('id') as Array<{ id: number } | number>; - const first = inserted[0]; - if (first === undefined) return { accepted: false, reason: 'duplicate' }; - const recordId = typeof first === 'number' ? first : Number(first.id); - this.reserved.add(recordId); - return { accepted: true, recordId }; + if (this.closed || this.held.size + this.reserved.size + this.claiming >= (this.options.maxPending ?? PLUGIN_WEBHOOK_LIMITS.pending)) return { accepted: false, reason: 'busy' }; + this.claiming++; + try { + await this.register(); + if (this.closed) return { accepted: false, reason: 'busy' }; + const { target } = job; + const recordId = await this.options.database.transaction(async trx => { + const installed = await trx('plugin_installations').where({ plugin_id: target.pluginId }).first('enabled', 'installed_at') as Pick | undefined; + if (!installed || !installed.enabled || Number(installed.installed_at) !== target.record.installedAt) return 'gone' as const; + const now = this.now(); + const inserted = await trx('plugin_deliveries').insert({ + plugin_id: target.pluginId, delivery_id: job.deliveryId, kind: 'webhook', repository: '', event_seq: 0, + event_type: `webhook.${job.webhook}`, handler: job.handler, status: 'pending', attempt: 0, next_attempt_at: now, created_at: now, logs: '[]', + webhook_owner: this.owner, + }).onConflict(['plugin_id', 'delivery_id']).ignore().returning('id') as Array<{ id: number } | number>; + const first = inserted[0]; + if (first === undefined) return 'duplicate' as const; + return typeof first === 'number' ? first : Number(first.id); + }); + if (typeof recordId !== 'number') return { accepted: false, reason: recordId }; + // Synchronously with giving the claimed slot back (finally): the slot passes to the record. + this.reserved.add(recordId); + return { accepted: true, recordId }; + } finally { + this.claiming--; + } } /** Give a reserved delivery id back (the request was refused before delivery): the sender may send it again. */ diff --git a/packages/core/test/pluginWebhooks.test.ts b/packages/core/test/pluginWebhooks.test.ts index 06bf0a744..d102ddbbe 100644 --- a/packages/core/test/pluginWebhooks.test.ts +++ b/packages/core/test/pluginWebhooks.test.ts @@ -5,7 +5,7 @@ import type { IncomingHttpHeaders } from 'node:http'; import { parsePluginManifest, type PluginWebhookVerification } from '@propr/shared'; import { loadPluginHandlers } from '../../plugins/src/index.js'; import { FIXTURE_REGISTRY, pluginHandlerFixture } from './helpers/pluginHandlerFixture.js'; -import { verifyPluginWebhook, pluginWebhookEndpoints, regeneratePluginWebhookKey } from '../src/plugins/pluginWebhooks.js'; +import { verifyPluginWebhook, pluginWebhookEndpoints, regeneratePluginWebhookKey, resolvePluginWebhook } from '../src/plugins/pluginWebhooks.js'; import { PluginWebhookRunner } from '../src/plugins/pluginWebhookRunner.js'; import { PluginWebhookReceiver } from '../src/plugins/pluginWebhookReceiver.js'; import { listPluginActivity } from '../src/plugins/pluginDeliveries.js'; @@ -348,3 +348,83 @@ test('starting another runner leaves a live runner\'s deliveries alone and fails assert.equal(await s.f.database('plugin_webhook_runners').where({ owner: 'webhooks-stopped' }).first(), undefined, 'the stopped runner is forgotten'); assert.ok(await s.f.database('plugin_webhook_runners').where({ owner: s.runner.owner }).first(), 'A is still registered'); }); + +test('an async respond function that throws, before or after an await, answers 500 and leaves no unhandled rejection', async t => { + const s = await setup(t); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + t.after(() => { process.off('unhandledRejection', onUnhandled); }); + const throwers = { + // eslint-disable-next-line @typescript-eslint/require-await -- deliberately async: it rejects without awaiting + async throwsAtOnce(): Promise { throw new Error('responder failed at once'); }, + async throwsAfterAwait(): Promise { await new Promise(resolve => setTimeout(resolve, 5)); throw new Error('responder failed after an await'); }, + }; + for (const [name, responder] of Object.entries(throwers)) { + const handlers = async (id: string) => ({ ...(await loadPluginHandlers(id, { root: FIXTURE_REGISTRY, includeDevelopment: true }))!, slackChallenge: responder as never }); + const receiver = new PluginWebhookReceiver({ database: s.f.database, runner: s.runner, catalog: s.f.options.catalog, handlers, environment: s.f.options.environment, now: () => s.clock.now }); + const body = JSON.stringify({ type: 'url_verification', challenge: name }); + const timestamp = String(Math.floor(NOW / 1000)); + const reply = await receiver.receive({ key: s.key, name: 'slack', headers: { 'x-slack-request-timestamp': timestamp, 'x-slack-signature': slackSignature(timestamp, body) }, body: Buffer.from(body) }); + assert.deepEqual(reply, { status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'The webhook could not answer' }) }, `${name}: an asynchronous answer is refused`); + } + // Let both rejections settle, and Node report any left unhandled. + await new Promise(resolve => setTimeout(resolve, 30)); + await new Promise(resolve => setImmediate(resolve)); + assert.deepEqual(unhandled, [], 'each rejection was consumed'); + await s.runner.idle(); + assert.deepEqual(await s.deliveries(), [], 'nothing was queued, and the reservations were given back'); + assert.deepEqual(await s.seen(), []); +}); + +test('concurrent requests at the limit are admitted against one count; a duplicate or refused reservation gives its slot back', async t => { + const s = await setup(t); + const runner = new PluginWebhookRunner({ ...s.f.options, now: () => s.clock.now, maxPending: 2 }); + t.after(() => runner.stop()); + const resolved = (await resolvePluginWebhook(s.f.database, s.key, 'a', await s.f.options.catalog!()))!; + const reserve = (id: string, target = resolved.target) => runner.reserve({ target, handler: 'onWebhook', deliveryId: `whk-a:${id}`, webhook: 'a' }); + const burst = await Promise.all(['c-1', 'c-2', 'c-3', 'c-4', 'c-5'].map(id => reserve(id))); + assert.equal(burst.filter(r => r.accepted).length, 2, 'only maxPending of a concurrent burst are admitted'); + assert.deepEqual(burst.filter(r => !r.accepted).map(r => !r.accepted && r.reason), ['busy', 'busy', 'busy']); + assert.equal((await s.deliveries()).length, 2, 'the refused requests recorded nothing'); + assert.deepEqual(await reserve('c-6'), { accepted: false, reason: 'busy' }); + + const [first, second] = burst.filter(r => r.accepted).map(r => (r as { recordId: number }).recordId); + await runner.release(first); + const secondId = (await s.deliveries()).find(d => d.id === second)!.deliveryId.slice('whk-a:'.length); + assert.deepEqual(await reserve(secondId), { accepted: false, reason: 'duplicate' }, 'a replay takes no slot'); + const stale = { ...resolved.target, record: { ...resolved.target.record, installedAt: resolved.target.record.installedAt - 1 } }; + assert.deepEqual(await reserve('c-7', stale), { accepted: false, reason: 'gone' }, 'a stale installation takes no slot'); + const again = await Promise.all([reserve('c-8'), reserve('c-9')]); + assert.deepEqual(again.map(r => r.accepted), [true, false], 'the released slot, and only it, is free again'); +}); + +test('a request resolved before an uninstall and reinstall records nothing, and does not poison the new installation\'s delivery ids', async t => { + const s = await setup(t); + let open!: () => void; + const gate = new Promise(resolve => { open = resolve; }); + let waiting!: () => void; + const resolvedA = new Promise(resolve => { waiting = resolve; }); + // Suspends a verified request of installation A after its key was resolved, before its delivery is reserved. + const connectionSecret = async () => { waiting(); await gate; return SECRET; }; + const receiver = new PluginWebhookReceiver({ database: s.f.database, runner: s.runner, catalog: s.f.options.catalog, connectionSecret, now: () => s.clock.now }); + const headers = { 'x-gitlab-token': SECRET, 'x-delivery-id': 'u-1' }; + const old = receiver.receive({ key: s.key, name: 'a', headers, body: Buffer.from('{}') }); + await resolvedA; + + await s.f.store.uninstall('1', ID, ID); + await s.f.store.install('1', ID); + await s.f.store.setConnection('1', ID, 'hook-secret', SECRET); + await s.f.store.setEnabled('1', ID, true); + open(); + assert.equal((await old).status, 404, 'A\'s request is answered as to a dead URL'); + assert.deepEqual(await s.f.database('plugin_deliveries').where({ plugin_id: ID }).select('delivery_id'), [], 'and records nothing'); + + const { webhooks } = await pluginWebhookEndpoints(s.f.database, ID, { manifest: s.f.manifest, origin: null }); + const keyB = webhooks.find(w => w.name === 'a')!.path.split('/')[3]; + assert.notEqual(keyB, s.key); + const fresh = await s.send('a', '{}', headers, keyB); + assert.equal(fresh.status, 202, 'B receives the delivery id A never delivered'); + await s.runner.idle(); + assert.deepEqual((await s.seen()).map(x => x.id), ['whk-a:u-1']); +}); From d74a0bcda2f580f40749f163c60224f87b5c8cd9 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Sun, 15 Nov 2026 20:09:19 +0000 Subject: [PATCH 5/5] feat(ai): I fixed F8: a webhook delivery is no longer dropped from memory when a database write fails. The runner keeps the request (or the reservation) until the write that moves the delivery on has succeeded. I fixed F8: a webhook delivery is no longer dropped from memory when a database write fails. The runner keeps the request (or the reservation) until the write that moves the delivery on has succeeded. **What changed** (`packages/core/src/plugins/pluginWebhookRunner.ts`): - **Failed writes are retried.** When a write fails during an attempt, the delivery stays held and the step is tried again after 1 s, 5 s, 30 s, then every minute. The delay is configurable with a new `persistRetryMs` option. - **The handler never runs twice.** If the handler had already run, only its saved outcome is written again. If the earlier write actually went through but reported an error, the retry sees that and treats the outcome as recorded. If the failed write was the one marking the attempt `running` and it did go through, the retry recognises the mark and runs the handler without counting a new attempt. - **`release` keeps its reservation until the record is deleted.** A failed delete is retried in the background. **`answered` keeps the reservation until its update succeeds;** if the update fails it throws, and the receiver's existing catch then calls `release`, which now finds the reservation and gives the record back. - **A runner now cleans up its own lost records.** Each heartbeat marks failed any unfinished record this runner owns but no longer holds or has reserved. It skips this while a reservation is being written, because that new record may not be tracked yet. - **Shutdown and tests:** `stop()` cancels pending release retries, and `idle()` now also waits for write retries and releases in progress. I also added a paragraph on this to `docs/docs/architecture/plugin-runtime.md`. **Tests:** four new regressions in `packages/core/test/pluginWebhooks.test.ts`. They wrap the runner's database so chosen writes fail once, either before the write or after it went through: - the final outcome write and the `retrying` write fail: the handler still ran once per attempt and the delivery ends `succeeded`. - the `running` mark fails, in both modes: the handler runs once, as attempt 1. - a release delete fails on the paused (503) path and an answered-challenge update fails: the sender's resend is delivered or answered, with no 409 and no unfinished record left. - an unfinished record this runner owns but doesn't hold is marked failed, and the delivery it does hold is left alone. All four fail when I put the old runner back. All 19 tests in `pluginWebhooks.test.ts` pass, as do `pluginStore` and `pluginContributions` (25 tests) and `pluginWebhookRoutes.test.ts` in the API package. Core typecheck is clean, and so is the zero-warning lint on the runner. In a typecheck of the test file alone, the only errors were missing type declarations for the migration files the shared fixture imports, none in the test file. **Two cases that still can't be fully fixed:** - If the challenge answer's write goes through but reports an error, the sender gets a 500, and resending the same delivery id gets a 409. There's no way to tell from the error whether the write landed. - If release deletes keep failing until shutdown, the record isn't cleaned up by this runner. After the runner stops, another runner marks it failed on its next heartbeat. I found no other problems outside F8. There's nothing visible to users, so I made no preview. PR: #3579 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5 --- docs/docs/architecture/plugin-runtime.md | 10 +- .../core/src/plugins/pluginWebhookRunner.ts | 149 +++++++++++++++--- packages/core/test/pluginWebhooks.test.ts | 110 ++++++++++++- 3 files changed, 240 insertions(+), 29 deletions(-) diff --git a/docs/docs/architecture/plugin-runtime.md b/docs/docs/architecture/plugin-runtime.md index a756671c1..a604ce3fe 100644 --- a/docs/docs/architecture/plugin-runtime.md +++ b/docs/docs/architecture/plugin-runtime.md @@ -317,7 +317,15 @@ its owner in `webhook_owner`. When the API starts, and with every heartbeat, stopped: its heartbeat is older than 2 minutes, or the record has no owner. They read *ProPR stopped before the delivery finished, and webhook requests are not kept*. This is one `UPDATE`, so another API process starting while one is -alive never fails that process's pending or retrying deliveries. `stop()` (on SIGTERM) lets attempts under +alive never fails that process's pending or retrying deliveries. + +A runner keeps a request in memory until the database write that moves its +delivery on has succeeded. A failed write is retried after 1 s, 5 s, 30 s and +then every minute. If the handler had already run, only the outcome is +written again; the handler does not run a second time. A reservation (a +refused or answered request) stays counted until its record is deleted or +marked answered. Each heartbeat also marks failed any of the runner's own +unfinished records that it no longer holds. `stop()` (on SIGTERM) lets attempts under way finish and fails the deliveries still waiting. A failed webhook delivery cannot be retried from Activity (`retryPluginDelivery` only accepts event deliveries), because the request is gone. diff --git a/packages/core/src/plugins/pluginWebhookRunner.ts b/packages/core/src/plugins/pluginWebhookRunner.ts index 3d1b174f5..77bed624d 100644 --- a/packages/core/src/plugins/pluginWebhookRunner.ts +++ b/packages/core/src/plugins/pluginWebhookRunner.ts @@ -25,6 +25,13 @@ import type { PluginInstallationTarget } from './pluginContext.js'; * `plugin_webhook_runners`. A delivery whose owner stopped cannot be resumed * by another process: once the owner's heartbeat has lapsed, any runner marks * it failed. A live runner's deliveries are never touched by another. + * + * A delivery stays this runner's obligation until its database transition + * succeeds: a failed write (a transient database error) is retried on + * `persistRetryMs`, without running the handler again, and a reservation is + * kept until its record is given back or answered. A record of this runner's + * that it no longer holds (nor reserved) is marked failed with the next + * heartbeat. */ export interface PluginWebhookRunnerOptions extends Omit { database: Knex; @@ -37,6 +44,8 @@ export interface PluginWebhookRunnerOptions extends Omit; finishedAt: number; nextAttemptAt: number | null } +interface Held { + job: PluginWebhookJob; recordId: number; attempt: number; timer?: NodeJS.Timeout; + /** The attempt number being marked `running`: set before the write, so a mark that committed but reported failure is recognized. */ + marking?: number; + outcome?: Outcome; + /** Consecutive failed database writes. */ + failures: number; +} const LOST = 'Not delivered: ProPR stopped before the delivery finished, and webhook requests are not kept'; const UNFINISHED = ['pending', 'running', 'retrying'] as const; const DEFAULT_HEARTBEAT_MS = 30_000; const DEFAULT_OWNER_TTL_MS = 120_000; +const DEFAULT_PERSIST_RETRY_MS = [1_000, 5_000, 30_000, 60_000] as const; const WITHHELD_LOG = 'Message not kept: webhook handler output may contain the request'; /** @@ -80,6 +99,8 @@ export class PluginWebhookRunner { private readonly reserved = new Set(); /** Capacity slots claimed by `reserve` calls still awaiting their record: counted with `held` and `reserved`, so concurrent requests cannot exceed `maxPending`. */ private claiming = 0; + /** Reserved records being given back (`release`), with the timer of the next try after a failed delete. */ + private readonly releasing = new Map(); private closed = false; /** This runner's name in `plugin_deliveries.webhook_owner` and `plugin_webhook_runners`. */ readonly owner: string; @@ -132,7 +153,22 @@ export class PluginWebhookRunner { }) .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }); await database('plugin_webhook_runners').where('heartbeat_at', '<', cutoff).whereNot('owner', this.owner).delete(); - return failed; + return failed + await this.recoverOrphaned(); + } + + /** + * Mark failed this runner's unfinished deliveries it no longer holds or has + * reserved: their request is gone (a write lost track of them). Skipped + * while a reservation is being written, whose record may not be known yet. + */ + private async recoverOrphaned(): Promise { + const { database } = this.options; + const own = (await database('plugin_deliveries').where({ kind: 'webhook', webhook_owner: this.owner }).whereIn('status', UNFINISHED).pluck('id') as unknown[]).map(Number); + if (this.claiming) return 0; + const orphans = own.filter(id => !this.held.has(id) && !this.reserved.has(id)); + if (!orphans.length) return 0; + return database('plugin_deliveries').whereIn('id', orphans).where({ webhook_owner: this.owner }).whereIn('status', UNFINISHED) + .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }); } /** @@ -188,18 +224,44 @@ export class PluginWebhookRunner { } } - /** Give a reserved delivery id back (the request was refused before delivery): the sender may send it again. */ + /** + * Give a reserved delivery id back (the request was refused before + * delivery): the sender may send it again. The reservation is kept until + * the record is deleted; a failed delete is retried in the background + * (until this runner stops, when its records are failed by recovery). + */ async release(recordId: number): Promise { - if (!this.reserved.delete(recordId)) return; - await this.options.database('plugin_deliveries').where({ id: recordId, status: 'pending', webhook_owner: this.owner }).delete(); + if (!this.reserved.has(recordId) || this.releasing.has(recordId)) return; + this.releasing.set(recordId, undefined); + await this.giveBack(recordId, 0); + } + + private async giveBack(recordId: number, failures: number): Promise { + this.releasing.set(recordId, undefined); + try { + await this.options.database('plugin_deliveries').where({ id: recordId, status: 'pending', webhook_owner: this.owner }).delete(); + this.releasing.delete(recordId); + this.reserved.delete(recordId); + } catch (error) { + logger.warn({ recordId, error: error instanceof Error ? error.message : String(error) }, '[plugins] Webhook reservation failed to release; retrying'); + if (this.closed) { this.releasing.delete(recordId); return; } + const timer = setTimeout(() => { void this.giveBack(recordId, failures + 1); }, this.persistDelay(failures)); + timer.unref?.(); + this.releasing.set(recordId, timer); + } } - /** A reserved delivery answered at once by the webhook's respond function: final, and its delivery id stays taken. */ + /** + * A reserved delivery answered at once by the webhook's respond function: + * final, and its delivery id stays taken. When the write fails it throws + * and the reservation is kept, for the caller to `release`. + */ async answered(recordId: number, respond: string): Promise { - if (!this.reserved.delete(recordId)) return; + if (!this.reserved.has(recordId) || this.releasing.has(recordId)) return; const now = this.now(); await this.options.database('plugin_deliveries').where({ id: recordId, status: 'pending', webhook_owner: this.owner }) .update({ status: 'succeeded', handler: respond, next_attempt_at: null, started_at: now, finished_at: now, duration_ms: 0 }); + this.reserved.delete(recordId); } /** Run a reserved delivery soon. False when this runner has stopped since: the record is marked failed. */ @@ -210,7 +272,7 @@ export class PluginWebhookRunner { .update({ status: 'failed', next_attempt_at: null, finished_at: this.now(), error: LOST }); return false; } - this.held.set(recordId, { job, recordId, attempt: 0 }); + this.held.set(recordId, { job, recordId, attempt: 0, failures: 0 }); this.schedule(recordId, 0); return true; } @@ -222,9 +284,9 @@ export class PluginWebhookRunner { return await this.start(reserved.recordId, job) ? reserved : { accepted: false, reason: 'busy' }; } - /** Wait until no attempt is under way or due at once: every held delivery is final or waits for a retry (tests). */ + /** Wait until no attempt, write or release is under way or due: every held delivery is final or waits for a retry (tests). */ async idle(): Promise { - while (this.running.size || this.due.size) { + while (this.running.size || this.due.size || this.releasing.size) { if (this.running.size) await Promise.all([...this.running]); else await new Promise(resolve => setImmediate(resolve)); } @@ -235,6 +297,8 @@ export class PluginWebhookRunner { this.closed = true; clearInterval(this.heartbeat); for (const held of this.held.values()) clearTimeout(held.timer); + for (const timer of this.releasing.values()) clearTimeout(timer); + this.releasing.clear(); this.due.clear(); await this.idle(); const ids = [...this.held.keys()]; @@ -246,15 +310,26 @@ export class PluginWebhookRunner { await this.options.database('plugin_webhook_runners').where({ owner: this.owner }).delete().catch(() => undefined); } - private schedule(recordId: number, delayMs: number): void { + private persistDelay(failures: number): number { + const delays = this.options.persistRetryMs ?? DEFAULT_PERSIST_RETRY_MS; + return delays.length ? delays[Math.min(failures, delays.length - 1)] : 0; + } + + /** + * Run the delivery's next step after `delayMs`. A database write that fails + * keeps the delivery held and tries again on `persistRetryMs`: before its + * handler ran, the attempt starts over; after, only its outcome is written. + */ + private schedule(recordId: number, delayMs: number, writeRetry = false): void { const held = this.held.get(recordId); if (!held || this.closed) return; - if (delayMs <= 0) this.due.add(recordId); + // A write being retried counts as under way, like an attempt due at once. + if (delayMs <= 0 || writeRetry) this.due.add(recordId); held.timer = setTimeout(() => { this.due.delete(recordId); - const attempt = this.attempt(held).catch((error: unknown) => { - logger.warn({ pluginId: held.job.target.pluginId, deliveryId: held.job.deliveryId, error: error instanceof Error ? error.message : String(error) }, '[plugins] Webhook delivery attempt failed to record'); - this.held.delete(recordId); + const attempt = this.attempt(held).then(() => { held.failures = 0; }, (error: unknown) => { + logger.warn({ pluginId: held.job.target.pluginId, deliveryId: held.job.deliveryId, error: error instanceof Error ? error.message : String(error) }, '[plugins] Webhook delivery attempt failed to record; retrying'); + this.schedule(recordId, this.persistDelay(held.failures++), true); }).finally(() => { this.running.delete(attempt); }); this.running.add(attempt); }, delayMs); @@ -275,20 +350,27 @@ export class PluginWebhookRunner { private async attempt(held: Held): Promise { if (this.closed) return; + if (held.outcome) return this.record(held, held.outcome); const { database } = this.options; const target = await this.current(held); - const previous = await database('plugin_deliveries').where({ id: held.recordId }).first('status') as Pick | undefined; + const previous = await database('plugin_deliveries').where({ id: held.recordId }).first('status', 'attempt') as Pick | undefined; + const attempt = held.attempt + 1; + // This attempt's mark committed although its write reported failure: the handler has not run yet. + const marked = previous?.status === 'running' && held.marking === attempt && Number(previous.attempt) === attempt; // Uninstalled (the record is gone) or finished by someone else (enabled again marks unfinished deliveries failed). - if (!previous || (previous.status !== 'pending' && previous.status !== 'retrying')) { this.held.delete(held.recordId); return; } + if (!previous || (!marked && previous.status !== 'pending' && previous.status !== 'retrying')) { this.held.delete(held.recordId); return; } if (!target) { await this.finish(held, { status: 'failed', next_attempt_at: null, finished_at: this.now(), error: 'Not delivered: the plugin was disabled' }); this.held.delete(held.recordId); return; } - const attempt = held.attempt + 1; - const marked = await database('plugin_deliveries').where({ id: held.recordId, status: previous.status }) - .update({ status: 'running', attempt, started_at: this.now(), finished_at: null }); - if (marked !== 1) { this.held.delete(held.recordId); return; } + if (!marked) { + held.marking = attempt; + const updated = await database('plugin_deliveries').where({ id: held.recordId, status: previous.status }) + .update({ status: 'running', attempt, started_at: this.now(), finished_at: null }); + if (updated !== 1) { this.held.delete(held.recordId); return; } + } + held.marking = undefined; held.attempt = attempt; let run: PluginHandlerRun; try { @@ -301,10 +383,25 @@ export class PluginWebhookRunner { const nextAttemptAt = status === 'retrying' ? now + this.delays[attempt - 1] : null; // Only while the record is still this attempt's: a restart (enabled again) or an uninstall since then wins. const diagnostics = webhookDiagnostics(run); - const recorded = await database('plugin_deliveries').where({ id: held.recordId, status: 'running', attempt }).update({ - status, finished_at: now, duration_ms: run.durationMs, timed_out: run.timedOut, error: diagnostics.error, logs: storedLogs(diagnostics.logs), next_attempt_at: nextAttemptAt, - }); - if (recorded !== 1 || status !== 'retrying') { this.held.delete(held.recordId); return; } - this.schedule(held.recordId, nextAttemptAt! - now); + // Kept until written: a failed write is retried without running the handler again. + held.outcome = { + attempt, status, finishedAt: now, nextAttemptAt, + update: { status, finished_at: now, duration_ms: run.durationMs, timed_out: run.timedOut, error: diagnostics.error, logs: storedLogs(diagnostics.logs), next_attempt_at: nextAttemptAt }, + }; + await this.record(held, held.outcome); + } + + /** Write an attempt's outcome, then retry later or let the delivery go. */ + private async record(held: Held, outcome: Outcome): Promise { + const { database } = this.options; + let recorded = await database('plugin_deliveries').where({ id: held.recordId, status: 'running', attempt: outcome.attempt }).update(outcome.update) === 1; + if (!recorded && held.failures > 0) { + // A previous write may have committed although it reported failure. + const row = await database('plugin_deliveries').where({ id: held.recordId }).first('status', 'attempt', 'finished_at') as Pick | undefined; + recorded = row?.status === outcome.status && Number(row.attempt) === outcome.attempt && Number(row.finished_at) === outcome.finishedAt; + } + held.outcome = undefined; + if (!recorded || outcome.status !== 'retrying') { this.held.delete(held.recordId); return; } + this.schedule(held.recordId, outcome.nextAttemptAt! - this.now()); } } diff --git a/packages/core/test/pluginWebhooks.test.ts b/packages/core/test/pluginWebhooks.test.ts index d102ddbbe..5048c14dc 100644 --- a/packages/core/test/pluginWebhooks.test.ts +++ b/packages/core/test/pluginWebhooks.test.ts @@ -2,6 +2,7 @@ import test, { mock, type TestContext } from 'node:test'; import assert from 'node:assert/strict'; import { createHmac } from 'node:crypto'; import type { IncomingHttpHeaders } from 'node:http'; +import type { Knex } from 'knex'; import { parsePluginManifest, type PluginWebhookVerification } from '@propr/shared'; import { loadPluginHandlers } from '../../plugins/src/index.js'; import { FIXTURE_REGISTRY, pluginHandlerFixture } from './helpers/pluginHandlerFixture.js'; @@ -72,13 +73,14 @@ test('each verification scheme accepts its documented test vector and refuses a const hubSignature = (body: string, secret = SECRET) => `sha256=${createHmac('sha256', secret).update(body).digest('hex')}`; const slackSignature = (timestamp: string, body: string, secret = SECRET) => `v0=${createHmac('sha256', secret).update(`v0:${timestamp}:${body}`).digest('hex')}`; -async function setup(t: TestContext, options: { retryDelaysMs?: number[] } = {}) { +async function setup(t: TestContext, options: { retryDelaysMs?: number[]; wrap?: (database: Knex) => Knex } = {}) { const f = await pluginHandlerFixture(t, { pluginId: ID, repositories: null }); await f.store.setConnection('1', ID, 'hook-secret', SECRET); const clock = { now: NOW }; const loads: string[] = []; const handlers = (id: string) => { loads.push(id); return loadPluginHandlers(id, { root: FIXTURE_REGISTRY, includeDevelopment: true }); }; - const runner = new PluginWebhookRunner({ ...f.options, handlers, now: () => clock.now, timeoutMs: 500, retryDelaysMs: options.retryDelaysMs ?? [5, 5] }); + const database = options.wrap ? options.wrap(f.database) : f.database; + const runner = new PluginWebhookRunner({ ...f.options, database, handlers, now: () => clock.now, timeoutMs: 500, retryDelaysMs: options.retryDelaysMs ?? [5, 5], persistRetryMs: [5] }); t.after(() => runner.stop()); const receiver = new PluginWebhookReceiver({ database: f.database, runner, catalog: f.options.catalog, handlers, environment: f.options.environment, now: () => clock.now }); const { webhooks } = await pluginWebhookEndpoints(f.database, ID, { manifest: f.manifest, origin: null }); @@ -428,3 +430,107 @@ test('a request resolved before an uninstall and reinstall records nothing, and await s.runner.idle(); assert.deepEqual((await s.seen()).map(x => x.id), ['whk-a:u-1']); }); + +/** + * The runner's database, failing chosen `plugin_deliveries` writes once each: + * `reject` before the write, `lost` after it committed (the reply was lost). + */ +function failingWrites(rules: Array<{ method: 'update' | 'delete'; when: (values: Record | undefined) => boolean; mode: 'reject' | 'lost' }>) { + const fired: string[] = []; + const wrap = (real: Knex) => new Proxy(real, { + apply(target, self, args: unknown[]) { + const query = Reflect.apply(target, self, args) as Knex.QueryBuilder; + if (args[0] !== 'plugin_deliveries') return query; + for (const method of ['update', 'delete'] as const) { + const original = query[method].bind(query) as (...a: unknown[]) => Promise; + (query as unknown as Record)[method] = (...a: unknown[]) => { + const values = a[0] as Record | undefined; + const rule = rules.find(r => r.method === method && r.when(values)); + if (!rule) return original(...a); + rules.splice(rules.indexOf(rule), 1); + fired.push(`${method}:${rule.mode}`); + if (rule.mode === 'reject') return Promise.reject(new Error('transient database write failure')); + return original(...a).then(() => { throw new Error('connection reset after commit'); }); + }; + } + return query; + }, + }); + return { wrap, fired }; +} + +const settled = async (s: Awaited>) => { + for (let i = 0; i < 100 && (s.runner.pending || (await s.runner.idle(), s.runner.pending)); i++) await new Promise(r => setTimeout(r, 10)); +}; + +test('a failed outcome write is retried without running the handler again, and the delivery reaches its final state', async t => { + const writes = failingWrites([ + { method: 'update', mode: 'reject', when: v => v?.status === 'retrying' }, + { method: 'update', mode: 'lost', when: v => v?.status === 'succeeded' && 'duration_ms' in v }, + ]); + const s = await setup(t, { retryDelaysMs: [5], wrap: writes.wrap }); + await s.f.database('plugin_state').insert({ plugin_id: ID, state_key: 'fail', value: '1', bytes: 1, updated_at: 0 }); + assert.equal((await s.send('token', '{}', { 'x-gitlab-token': SECRET })).status, 202); + await settled(s); + assert.deepEqual(writes.fired, ['update:reject', 'update:lost'], 'both outcome writes failed once'); + assert.deepEqual((await s.seen()).map(x => x.attempt), [1, 2], 'one handler run per attempt: the failed writes ran nothing again'); + const [delivery] = await s.deliveries(); + assert.deepEqual({ status: delivery.status, attempt: delivery.attempt }, { status: 'succeeded', attempt: 2 }); + assert.equal(s.runner.pending, 0, 'nothing is held once the delivery is final'); +}); + +test('a failed write before the handler ran retries the attempt, once, even when the write committed', async t => { + for (const mode of ['reject', 'lost'] as const) { + await t.test(mode, async t => { + const writes = failingWrites([{ method: 'update', mode, when: v => v?.status === 'running' }]); + const s = await setup(t, { wrap: writes.wrap }); + assert.equal((await s.send('token', '{}', { 'x-gitlab-token': SECRET })).status, 202); + await settled(s); + assert.deepEqual(writes.fired, [`update:${mode}`]); + assert.deepEqual((await s.seen()).map(x => x.attempt), [1], 'the handler ran once, as attempt 1'); + const [delivery] = await s.deliveries(); + assert.deepEqual({ status: delivery.status, attempt: delivery.attempt }, { status: 'succeeded', attempt: 1 }); + }); + } +}); + +test('a reservation is kept until its release or answer is written: the sender can deliver again', async t => { + const writes = failingWrites([ + { method: 'delete', mode: 'reject', when: () => true }, + { method: 'update', mode: 'reject', when: v => v?.handler === 'countedChallenge' }, + ]); + const s = await setup(t, { wrap: writes.wrap }); + const send = (body: string, id: string) => s.send('answered', body, { 'x-gitlab-token': SECRET, 'x-delivery-id': id }); + + // Refused while paused, and the reservation's delete fails once: it is retried, so the sender's retry is delivered. + const installedAt = Number((await s.f.database('plugin_installations').where({ plugin_id: ID }).first('installed_at')).installed_at); + await s.f.database('plugin_dispatch').insert({ plugin_id: ID, installed_at: installedAt, start_seq: 0, updated_at: NOW, paused_at: NOW }).onConflict('plugin_id').merge(['paused_at']); + assert.equal((await send('{}', 'p-1')).status, 503); + await s.runner.idle(); + assert.deepEqual(writes.fired, ['delete:reject']); + assert.deepEqual(await s.deliveries(), [], 'the release was retried until the record was given back'); + await s.f.database('plugin_dispatch').where({ plugin_id: ID }).update({ paused_at: null }); + assert.equal((await send('{}', 'p-1')).status, 202, 'not a replay'); + await s.runner.idle(); + + // The challenge answer fails to record: the request fails, and its reservation is given back rather than left pending. + const challenge = JSON.stringify({ type: 'url_verification', challenge: 'c-9' }); + await assert.rejects(send(challenge, 'p-2'), /transient database write failure/); + await s.runner.idle(); + assert.equal((await s.deliveries()).find(d => d.deliveryId === 'whk-answered:p-2'), undefined, 'no unfinished record is left'); + assert.deepEqual(await send(challenge, 'p-2'), { status: 200, contentType: 'text/plain', body: 'c-9' }, 'the sender\'s retry is answered'); + assert.equal((await s.deliveries()).find(d => d.deliveryId === 'whk-answered:p-2')!.status, 'succeeded'); +}); + +test('a live runner fails its own unfinished records it no longer holds, and leaves the ones it holds', async t => { + const s = await setup(t, { retryDelaysMs: [60_000] }); + await s.f.database('plugin_state').insert({ plugin_id: ID, state_key: 'fail', value: '1', bytes: 1, updated_at: 0 }); + assert.equal((await s.send('token', '{}', { 'x-gitlab-token': SECRET })).status, 202); + await s.runner.idle(); + assert.equal((await s.deliveries())[0].status, 'retrying', 'held, waiting for its retry'); + await s.f.database('plugin_deliveries').insert({ plugin_id: ID, delivery_id: 'whk-token/orphaned', kind: 'webhook', repository: '', event_seq: 0, event_type: 'webhook.token', handler: 'flakyWebhook', status: 'running', attempt: 1, created_at: NOW, logs: '[]', webhook_owner: s.runner.owner }); + assert.equal(await s.runner.recover(), 1); + const rows = await s.deliveries(); + assert.equal(rows.find(d => d.deliveryId === 'whk-token/orphaned')!.status, 'failed', 'the record this runner lost track of is final'); + assert.equal(rows.find(d => d.deliveryId !== 'whk-token/orphaned')!.status, 'retrying', 'the held delivery is untouched'); +});