From 43ce006e7185b800e44ef72547660b052e760cf9 Mon Sep 17 00:00:00 2001 From: satvikOS Date: Thu, 20 Aug 2026 20:36:34 -0400 Subject: [PATCH 1/5] feat(integrations): inbound Slack events endpoint for the existing verifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `verify.ts` implemented Slack request verification — signature plus a two-sided five-minute window — and had zero callers. A security control that has never run is not a control. This is the endpoint it was written for. POST /api/integrations/slack/events reads the raw bytes, verifies them against the current signing key and then the previous one (the rotation window Integration §12 asks for), and refuses every failure with one identical 401 so a caller cannot learn which part it got wrong. Deduplicated on two identities: Slack's `event_id`, which covers the retry storm, and a semantic key `team:type:event_ts`, which covers the same occurrence arriving under a new id. The receipt row and any effect are written in ONE transaction — recording first and committing separately would let a failed effect leave a receipt behind, and our own dedupe would then turn away the retry that could have fixed it. `app_uninstalled` and `tokens_revoked` are handled inline, because both mean the bot token this deployment holds is dead and every second the connection still reads ACTIVE is a second the product will hand a revoked credential to Slack. `tokens_revoked` revokes only when a BOT token is in the payload; a member revoking their own user token must not disconnect the club's Slack. Everything else is acknowledged, recorded and stated as not processed. The durable carrier for deferred work is the transactional outbox, which is BLOCKED on the envelope conflict between Identity §21.2 and Integration §9 — so the receipt keeps a digest and no payload, and cannot become a second carrier by accident. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/deploy.yml | 5 + .../migration.sql | 71 +++ apps/web/prisma/schema.prisma | 172 +++++++ .../api/integrations/slack/callback/route.ts | 43 +- .../integrations/slack/events/route.test.ts | 448 ++++++++++++++++++ .../api/integrations/slack/events/route.ts | 331 +++++++++++++ .../src/lib/integrations/connection-audit.ts | 34 +- .../lib/integrations/slack/announce.test.ts | 58 +++ .../src/lib/integrations/slack/events.test.ts | 357 ++++++++++++++ apps/web/src/lib/integrations/slack/events.ts | 369 +++++++++++++++ apps/web/src/lib/tenancy/registry.test.ts | 21 +- apps/web/src/lib/tenancy/registry.ts | 10 +- .../global-engine-execution-ledger.md | 11 +- infrastructure/terraform/ecs.tf | 7 + infrastructure/terraform/integrations.tf | 26 +- 15 files changed, 1943 insertions(+), 20 deletions(-) create mode 100644 apps/web/prisma/migrations/20260820150000_inbound_webhook_receipts/migration.sql create mode 100644 apps/web/src/app/api/integrations/slack/events/route.test.ts create mode 100644 apps/web/src/app/api/integrations/slack/events/route.ts create mode 100644 apps/web/src/lib/integrations/slack/events.test.ts create mode 100644 apps/web/src/lib/integrations/slack/events.ts diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 92a68335..ba9fe2e8 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -306,6 +306,11 @@ jobs: TF_VAR_slack_client_id: ${{ secrets.SLACK_CLIENT_ID }} TF_VAR_slack_client_secret: ${{ secrets.SLACK_CLIENT_SECRET }} TF_VAR_slack_signing_secret: ${{ secrets.SLACK_SIGNING_SECRET }} + # Set only while a signing secret is being rotated, and cleared after. + # An unset repository secret arrives as an empty string, which the + # endpoint treats as "no previous key" — so the normal state needs no + # action, and a rotation is two secret edits rather than a code change. + TF_VAR_slack_signing_secret_previous: ${{ secrets.SLACK_SIGNING_SECRET_PREVIOUS }} TF_VAR_slack_app_id: ${{ secrets.SLACK_APP_ID }} run: | terraform apply \ diff --git a/apps/web/prisma/migrations/20260820150000_inbound_webhook_receipts/migration.sql b/apps/web/prisma/migrations/20260820150000_inbound_webhook_receipts/migration.sql new file mode 100644 index 00000000..302ff1c6 --- /dev/null +++ b/apps/web/prisma/migrations/20260820150000_inbound_webhook_receipts/migration.sql @@ -0,0 +1,71 @@ +-- CreateEnum +CREATE TYPE "WebhookReceiptOutcome" AS ENUM ('HANDLED', 'NO_ACTION', 'NOT_PROCESSED'); + +-- CreateTable +CREATE TABLE "WebhookSubscription" ( + "id" TEXT NOT NULL, + "institutionId" TEXT NOT NULL, + "connectionId" TEXT NOT NULL, + "providerId" TEXT NOT NULL, + "endpoint" TEXT NOT NULL, + "events" TEXT[], + "expiresAt" TIMESTAMP(3), + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "updatedAt" TIMESTAMP(3) NOT NULL, + + CONSTRAINT "WebhookSubscription_pkey" PRIMARY KEY ("id") +); + +-- CreateTable +CREATE TABLE "WebhookReceipt" ( + "id" TEXT NOT NULL, + "institutionId" TEXT, + "connectionId" TEXT, + "providerId" TEXT NOT NULL, + "externalId" TEXT, + "externalEventId" TEXT, + "semanticKey" TEXT NOT NULL, + "envelopeType" TEXT NOT NULL, + "eventType" TEXT, + "bodyDigest" TEXT NOT NULL, + "bodySize" INTEGER NOT NULL, + "retryNum" INTEGER, + "retryReason" TEXT, + "outcome" "WebhookReceiptOutcome" NOT NULL, + "note" TEXT NOT NULL, + "connectionsMatched" INTEGER NOT NULL DEFAULT 0, + "receivedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + + CONSTRAINT "WebhookReceipt_pkey" PRIMARY KEY ("id") +); + +-- CreateIndex +CREATE INDEX "WebhookSubscription_institutionId_providerId_idx" ON "WebhookSubscription"("institutionId", "providerId"); + +-- CreateIndex +CREATE UNIQUE INDEX "WebhookSubscription_connectionId_key" ON "WebhookSubscription"("connectionId"); + +-- CreateIndex +CREATE INDEX "WebhookReceipt_institutionId_receivedAt_idx" ON "WebhookReceipt"("institutionId", "receivedAt"); + +-- CreateIndex +CREATE INDEX "WebhookReceipt_providerId_receivedAt_idx" ON "WebhookReceipt"("providerId", "receivedAt"); + +-- CreateIndex +CREATE UNIQUE INDEX "WebhookReceipt_providerId_externalEventId_key" ON "WebhookReceipt"("providerId", "externalEventId"); + +-- CreateIndex +CREATE UNIQUE INDEX "WebhookReceipt_providerId_semanticKey_key" ON "WebhookReceipt"("providerId", "semanticKey"); + +-- AddForeignKey +ALTER TABLE "WebhookSubscription" ADD CONSTRAINT "WebhookSubscription_institutionId_fkey" FOREIGN KEY ("institutionId") REFERENCES "Institution"("id") ON DELETE CASCADE ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "WebhookSubscription" ADD CONSTRAINT "WebhookSubscription_connectionId_fkey" FOREIGN KEY ("connectionId") REFERENCES "Connection"("id") ON DELETE CASCADE ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "WebhookReceipt" ADD CONSTRAINT "WebhookReceipt_institutionId_fkey" FOREIGN KEY ("institutionId") REFERENCES "Institution"("id") ON DELETE SET NULL ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "WebhookReceipt" ADD CONSTRAINT "WebhookReceipt_connectionId_fkey" FOREIGN KEY ("connectionId") REFERENCES "Connection"("id") ON DELETE SET NULL ON UPDATE CASCADE; + diff --git a/apps/web/prisma/schema.prisma b/apps/web/prisma/schema.prisma index 538fe8c4..9a6e38c5 100644 --- a/apps/web/prisma/schema.prisma +++ b/apps/web/prisma/schema.prisma @@ -78,6 +78,8 @@ model Institution { resources Resource[] directoryPeople DirectoryPerson[] restrictedIdentities RestrictedIdentity[] + webhookSubscriptions WebhookSubscription[] + webhookReceipts WebhookReceipt[] } enum InstitutionRole { @@ -1157,6 +1159,10 @@ model Connection { revokedAt DateTime? updatedAt DateTime @updatedAt + /// What this connection is subscribed to receive, and what arrived. + webhookSubscription WebhookSubscription? + webhookReceipts WebhookReceipt[] + /// Re-installing the same workspace updates rather than duplicates, so a /// retry converges instead of leaving two rows that disagree. @@unique([institutionId, providerId, externalId]) @@ -1199,3 +1205,169 @@ model RestrictedIdentity { @@unique([institutionId, emailNormalized]) @@index([institutionId, status]) } + +// ─── Inbound webhooks ──────────────────────────────────────────────────────── + +/// What this deployment expects a provider to deliver, and where. +/// +/// Integration §12 asks for subscription renewal and deletion, and both need a +/// row: an installation that is subscribed is a fact about the connection, not +/// about the code, and offboarding has to be able to find and remove it. +/// +/// ── What "renewal" means here, honestly ───────────────────────────────────── +/// +/// `expiresAt` is null for Slack, and that is not an omission. Slack's Events +/// API subscription is configured on the app and delivered for as long as the +/// app is installed — there is no lifetime and no renewal call, so a renewal +/// job for Slack would renew nothing. Providers whose subscriptions do expire +/// (Microsoft Graph tops out at three days, Google Drive's watch channels at +/// one week) set this, and the column is what a renewal pass would read. It is +/// declared now rather than added later because the row's whole purpose is to +/// survive an offboarding, and a subscription record that cannot express "this +/// one dies on Thursday" would have to be migrated the day Outlook arrives. +/// +/// ── One row per connection ────────────────────────────────────────────────── +/// +/// Slack has one subscription per installed workspace, so the connection is the +/// key. A provider with several — Graph subscribes per resource — needs this +/// widened, and that is a schema change made when that provider lands rather +/// than a column nothing sets today. +model WebhookSubscription { + id String @id @default(cuid()) + institutionId String + institution Institution @relation(fields: [institutionId], references: [id], onDelete: Cascade) + + /// The installation this subscription belongs to. Cascades, because a + /// subscription outliving its connection is a delivery nobody can attribute. + connectionId String + connection Connection @relation(fields: [connectionId], references: [id], onDelete: Cascade) + + /// Catalog product id, e.g. "slack.workspace". Denormalised from the + /// connection so a query for "every Slack subscription" needs no join. + providerId String + + /// Where this deployment expects delivery. Slack's Request URL is app-level + /// configuration rather than something Tenure sets, so this is not a command + /// — it is the value to compare against when an app configured for staging + /// starts delivering into production, which is otherwise invisible. + endpoint String + + /// The event names asked for. Kept so a receipt for something unsubscribed + /// can be recognised as a configuration drift rather than a code gap. + events String[] + + /// When the provider stops delivering unless renewed. Null means the provider + /// does not expire this subscription — see the note above. + expiresAt DateTime? + + createdAt DateTime @default(now()) + updatedAt DateTime @updatedAt + + @@unique([connectionId]) + @@index([institutionId, providerId]) +} + +/// One verified inbound delivery, recorded before anything acts on it. +/// +/// Integration §12 asks for event-id dedupe and a minimised immutable receipt. +/// This is both: the two unique indexes below are the dedupe — a provider event +/// id and a semantic key derived from the delivery's content — and the columns +/// are deliberately everything except the payload. +/// +/// ── Why the body is not here ──────────────────────────────────────────────── +/// +/// `bodyDigest` proves what arrived without this table becoming a copy of a +/// tenant's Slack traffic. A `message` event carries what somebody typed and +/// `tokens_revoked` carries the member ids whose tokens went away; none of that +/// is needed to know that a delivery happened, was verified, and was acted on. +/// +/// It is also the reason this table is not a queue in disguise. A row here +/// cannot be replayed, by design: replaying needs the payload, and the carrier +/// that is supposed to hold one is the transactional outbox — blocked on the +/// envelope conflict between Identity §21.2 and Integration §9, and requiring +/// (per §9) that a payload live behind a governed external reference rather than +/// inline in a row like this one. Storing the body here to be drained later +/// would be that second carrier, built without the decision that governs it. +/// +/// ── Why institutionId can be null ─────────────────────────────────────────── +/// +/// A delivery's tenant is derived, not given: the envelope names a Slack team, +/// and the team is matched to a `Connection`. That derivation can find nothing +/// — an install that failed after the OAuth exchange, or an app still installed +/// in a workspace whose connection was deleted — and it can find more than one, +/// because one workspace may legitimately be connected by two institutions. In +/// both cases the tenant is genuinely unknown and a guessed one would be worse +/// than none; `connectionsAffected` records what the lookup actually found. +model WebhookReceipt { + id String @id @default(cuid()) + + /// Resolved from the workspace, when exactly one connection matched. + institutionId String? + institution Institution? @relation(fields: [institutionId], references: [id], onDelete: SetNull) + + /// The matched connection, on the same condition. Set null rather than + /// cascade-deleted: a receipt is evidence that a delivery arrived, and it + /// stays true after the connection it was about is gone. + connectionId String? + connection Connection? @relation(fields: [connectionId], references: [id], onDelete: SetNull) + + /// Catalog product id, e.g. "slack.workspace". + providerId String + + /// The provider's own identifier for the workspace the delivery is about. + externalId String? + + /// The provider's id for this delivery — Slack's `event_id`. Null when the + /// envelope carried none, and NULLs do not collide in a Postgres unique + /// index, which is the behaviour wanted here: an envelope with no id cannot + /// be deduplicated on one, and the semantic key covers it instead. + externalEventId String? + + /// Content identity, always present. `team:type:event_ts` where the envelope + /// supplies all three, a content digest otherwise. + semanticKey String + + /// Envelope `type` and inner `event.type`, kept separately because the second + /// is what decides handling and the first is what says the envelope was a + /// shape this endpoint understands at all. + envelopeType String + eventType String? + + /// SHA-256 of the exact bytes received, and their length. What this table + /// keeps instead of the payload. + bodyDigest String + bodySize Int + + /// Slack's retry counter and reason, when it is retrying. Present means an + /// earlier delivery of the same event did not get a 2xx, which is worth + /// seeing next to a receipt that says it was handled. + retryNum Int? + retryReason String? + + outcome WebhookReceiptOutcome + + /// Why, in a sentence somebody reads. For NOT_PROCESSED this is the plain + /// statement that something arrived and nothing acted on it. + note String + + /// How many connections the workspace resolved to. Zero and two are both real + /// and both leave `connectionId` null; the number is what tells them apart. + /// What was DONE about them is `outcome` and `note`, not this. + connectionsMatched Int @default(0) + + receivedAt DateTime @default(now()) + + @@unique([providerId, externalEventId]) + @@unique([providerId, semanticKey]) + @@index([institutionId, receivedAt]) + @@index([providerId, receivedAt]) +} + +enum WebhookReceiptOutcome { + /// Understood, and an effect was applied in the same transaction as this row. + HANDLED + /// Understood, and correctly nothing to do. + NO_ACTION + /// Accepted and recorded; no code path acts on it. `note` says which. + NOT_PROCESSED +} diff --git a/apps/web/src/app/api/integrations/slack/callback/route.ts b/apps/web/src/app/api/integrations/slack/callback/route.ts index 8bce753e..7dbec8f1 100644 --- a/apps/web/src/app/api/integrations/slack/callback/route.ts +++ b/apps/web/src/app/api/integrations/slack/callback/route.ts @@ -5,6 +5,10 @@ import { CONNECTION_INSTALLED_ACTION, recordConnectionEvent, } from "@/lib/integrations/connection-audit" +import { + SLACK_EVENTS_PATH, + SLACK_SUBSCRIBED_EVENTS, +} from "@/lib/integrations/slack/events" import { verifyInstallState } from "@/lib/integrations/slack/install" import { exchangeCodeForToken } from "@/lib/integrations/slack/oauth" import { SLACK_WORKSPACE_PRODUCT_ID } from "@/lib/integrations/slack/provider" @@ -32,9 +36,10 @@ import { awsSecretStore, secretNameFor } from "@/lib/integrations/secret-store" * as connected and fails on first use; a secret with no row is invisible, * harmless, and overwritten by the next install of the same workspace. * - * The row and its audit record then go in one transaction, because a connection - * nobody can account for is the thing §4 exists to prevent — see - * `connection-audit.ts`. + * The row, its webhook subscription and its audit record then go in one + * transaction, because a connection nobody can account for is the thing §4 + * exists to prevent — see `connection-audit.ts` — and a connection with no + * subscription row is one that offboarding cannot find anything to delete for. */ export const dynamic = "force-dynamic" @@ -124,6 +129,38 @@ export async function GET(req: Request) { }, }) + // The installation IS the subscription, for Slack. Event delivery starts + // the moment the app is in the workspace and stops when it leaves, so the + // row is created here rather than by a separate call there is no API for + // — and the events endpoint deletes it on `app_uninstalled`, which is the + // offboarding half Integration §12 asks for. + // + // Upserted on the connection, so re-installing the same workspace + // converges instead of failing on the unique key or leaving a stale + // endpoint behind from a previous deployment host. + await tx.webhookSubscription.upsert({ + where: { connectionId: connection.id }, + update: { + endpoint: new URL(SLACK_EVENTS_PATH, req.url).toString(), + events: [...SLACK_SUBSCRIBED_EVENTS], + }, + create: { + institutionId, + connectionId: connection.id, + providerId: SLACK_WORKSPACE_PRODUCT_ID, + // Derived from the request rather than hard-coded: this is the host + // the administrator is actually installing against, and comparing it + // to the Request URL configured on the Slack app is how you find a + // production install pointed at a staging endpoint. + endpoint: new URL(SLACK_EVENTS_PATH, req.url).toString(), + events: [...SLACK_SUBSCRIBED_EVENTS], + // No expiry. Slack's Events API subscription has no lifetime and no + // renewal call — it lasts exactly as long as the installation. A + // provider whose subscriptions do expire sets this. + expiresAt: null, + }, + }) + // Re-installing updates the row in place, so without this the trail would // show one connection and no history — including the case that matters // most, a workspace reconnected by somebody other than whoever installed it. diff --git a/apps/web/src/app/api/integrations/slack/events/route.test.ts b/apps/web/src/app/api/integrations/slack/events/route.test.ts new file mode 100644 index 00000000..12556191 --- /dev/null +++ b/apps/web/src/app/api/integrations/slack/events/route.test.ts @@ -0,0 +1,448 @@ +import { computeSlackSignature, MAX_TIMESTAMP_SKEW_SECONDS } from "@/lib/integrations/slack/verify" + +/** + * The endpoint, end to end, with the database replaced. + * + * `events.test.ts` proves the decisions. This proves the two things a pure + * function cannot: that an unverified request never reaches a write, and that a + * verified one lands as exactly one receipt inside one transaction. + * + * The negative control that motivated the first of those: moving `parseInbound` + * above the verification — the arrangement any reader would call harmless, + * because "parsing is not acting" — makes every assertion here still pass + * except `leaves the database untouched`, which is why that assertion exists on + * every refusal rather than once. + */ + +const findMany = jest.fn() +const update = jest.fn() +const deleteMany = jest.fn() +const createReceipt = jest.fn() +const createAudit = jest.fn() +const transaction = jest.fn() + +const tx = { + connection: { findMany, update }, + webhookSubscription: { deleteMany }, + webhookReceipt: { create: createReceipt }, + auditEvent: { create: createAudit }, +} + +jest.mock("@/lib/db", () => ({ + db: { $transaction: (...args: unknown[]) => transaction(...args) }, +})) + +// The real one opens an AsyncLocalStorage grant; here it only has to invoke the +// callback. That the grant is `control-plane` and named is asserted separately, +// below, by reading the call arguments. +const runUnscoped = jest.fn() +jest.mock("@/lib/tenancy/context", () => ({ + runUnscoped: (...args: unknown[]) => runUnscoped(...args), +})) + +import { POST } from "./route" + +const CURRENT = "8f742231b10e8888abcd99yyyzzz85a5" +const PREVIOUS = "0e1e0e2e0e3e0e4e0e5e0e6e0e7e0e8e" +const NOW_MS = 1_760_000_000_000 +const NOW = Math.floor(NOW_MS / 1000) + +const CONNECTION = { + id: "conn_1", + institutionId: "inst_1", + externalName: "Simon Student Life", + status: "ACTIVE" as const, +} + +const envelope = (event: Record, over: Record = {}) => + JSON.stringify({ + token: "z26uFbvR1xHJEdHE1OQiO6t8", + team_id: "T0001", + api_app_id: "A0001", + event, + type: "event_callback", + event_id: "Ev0001", + event_time: NOW, + ...over, + }) + +const UNINSTALLED = envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" }) + +function post( + body: string, + { + secret = CURRENT, + timestamp = String(NOW), + signature, + headers = {}, + }: { + secret?: string | null + timestamp?: string | null + signature?: string | null + headers?: Record + } = {}, +) { + const all: Record = { "content-type": "application/json", ...headers } + if (timestamp !== null) all["x-slack-request-timestamp"] = timestamp + const sig = signature ?? (secret === null ? null : computeSlackSignature(body, timestamp ?? "", secret)) + if (sig !== null) all["x-slack-signature"] = sig + + return POST(new Request("https://tenure.test/api/integrations/slack/events", { + method: "POST", + headers: all, + body, + })) +} + +/** Nothing was written. The assertion every refusal has to make. */ +function expectNoWrites() { + expect(transaction).not.toHaveBeenCalled() + expect(createReceipt).not.toHaveBeenCalled() + expect(update).not.toHaveBeenCalled() + expect(deleteMany).not.toHaveBeenCalled() + expect(createAudit).not.toHaveBeenCalled() +} + +/** The single `data` object the receipt was written with. */ +const receipt = () => createReceipt.mock.calls[0][0].data + +beforeEach(() => { + jest.clearAllMocks() + jest.spyOn(Date, "now").mockReturnValue(NOW_MS) + jest.spyOn(console, "warn").mockImplementation(() => {}) + jest.spyOn(console, "error").mockImplementation(() => {}) + + process.env.SLACK_SIGNING_SECRET = CURRENT + delete process.env.SLACK_SIGNING_SECRET_PREVIOUS + + findMany.mockResolvedValue([CONNECTION]) + update.mockResolvedValue({}) + deleteMany.mockResolvedValue({ count: 1 }) + createReceipt.mockResolvedValue({}) + createAudit.mockResolvedValue({}) + runUnscoped.mockImplementation((_reason: string, _detail: string, fn: () => unknown) => fn()) + transaction.mockImplementation((fn: (client: typeof tx) => unknown) => fn(tx)) +}) + +afterEach(() => jest.restoreAllMocks()) + +describe("refusing what Slack did not send", () => { + it("refuses a request with no signature, and writes nothing", async () => { + const response = await post(UNINSTALLED, { secret: null }) + expect(response.status).toBe(401) + expectNoWrites() + }) + + it("refuses a signature from a key nobody issued", async () => { + const response = await post(UNINSTALLED, { secret: "a-secret-we-never-issued" }) + expect(response.status).toBe(401) + expectNoWrites() + }) + + it("refuses a REPLAY from outside the five-minute window", async () => { + // The captured request, signed correctly for its own timestamp, sent again + // six minutes later. This is the attack the timestamp exists for: a + // signature alone is valid forever. + const stale = String(NOW - MAX_TIMESTAMP_SKEW_SECONDS - 1) + const response = await post(UNINSTALLED, { timestamp: stale }) + expect(response.status).toBe(401) + expectNoWrites() + }) + + it("refuses a body whose bytes were re-serialised after signing", async () => { + // The signature covers the exact bytes. A proxy or a middleware that parses + // and re-emits the same JSON breaks it, which is why the route reads + // `text()` and never `json()`. + const reserialised = JSON.stringify(JSON.parse(UNINSTALLED), null, 2) + const response = await post(reserialised, { + signature: computeSlackSignature(UNINSTALLED, String(NOW), CURRENT), + }) + expect(response.status).toBe(401) + expectNoWrites() + }) + + it("refuses everything when no signing secret is configured", async () => { + delete process.env.SLACK_SIGNING_SECRET + const response = await post(UNINSTALLED) + expect(response.status).toBe(401) + expectNoWrites() + }) + + it("tells the caller nothing about WHY", async () => { + // Five different refusals, one indistinguishable answer. A caller who could + // tell "no signature" from "wrong key" from "too old" would know which of + // those to fix next. + const stale = String(NOW - MAX_TIMESTAMP_SKEW_SECONDS - 1) + const responses = await Promise.all([ + post(UNINSTALLED, { secret: null }), + post(UNINSTALLED, { secret: "wrong" }), + post(UNINSTALLED, { timestamp: stale }), + post(UNINSTALLED, { timestamp: null }), + post(UNINSTALLED, { signature: `v9=${"0".repeat(64)}` }), + ]) + + const rendered = await Promise.all( + responses.map(async (r) => ({ + status: r.status, + body: await r.text(), + type: r.headers.get("content-type"), + })), + ) + expect(new Set(rendered.map((r) => JSON.stringify(r))).size).toBe(1) + expect(rendered[0].status).toBe(401) + }) + + it("refuses a body larger than the ceiling without reading it into a signature", async () => { + const oversize = "x".repeat(1024 * 1024 + 1) + const response = await post(oversize) + expect(response.status).toBe(413) + expectNoWrites() + }) +}) + +describe("the rotation window", () => { + it("accepts a delivery signed with the previous key while both are set", async () => { + process.env.SLACK_SIGNING_SECRET_PREVIOUS = PREVIOUS + const response = await post(UNINSTALLED, { secret: PREVIOUS }) + + expect(response.status).toBe(200) + expect(createReceipt).toHaveBeenCalledTimes(1) + }) + + it("says so in the log, because a half-finished rotation is nobody's steady state", async () => { + process.env.SLACK_SIGNING_SECRET_PREVIOUS = PREVIOUS + await post(UNINSTALLED, { secret: PREVIOUS }) + + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining("PREVIOUS signing key"), + ) + }) + + it("refuses the previous key once the slot is cleared", async () => { + // Clearing it is what ends the retired key's life. If this passed, rotation + // would be a thing that never finished. + const response = await post(UNINSTALLED, { secret: PREVIOUS }) + expect(response.status).toBe(401) + expectNoWrites() + }) +}) + +describe("the URL handshake", () => { + it("echoes a verified challenge and touches nothing", async () => { + const body = JSON.stringify({ type: "url_verification", token: "t", challenge: "3eZbrw1a" }) + const response = await post(body) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ challenge: "3eZbrw1a" }) + // Deliberate: saving a Request URL must not depend on a healthy database. + expectNoWrites() + }) + + it("will not echo a challenge for an unverified caller", async () => { + const body = JSON.stringify({ type: "url_verification", challenge: "3eZbrw1a" }) + const response = await post(body, { secret: "wrong" }) + + expect(response.status).toBe(401) + await expect(response.text()).resolves.not.toContain("3eZbrw1a") + }) +}) + +describe("app_uninstalled", () => { + it("revokes the connection, deletes the subscription and records both", async () => { + const response = await post(UNINSTALLED) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ + ok: true, + outcome: "HANDLED", + connectionsMatched: 1, + }) + + expect(update).toHaveBeenCalledWith({ + where: { id: "conn_1" }, + data: { status: "REVOKED", revokedAt: expect.any(Date) }, + }) + expect(deleteMany).toHaveBeenCalledWith({ where: { connectionId: "conn_1" } }) + expect(createAudit).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + action: "Integration.Connection.Revoked", + // Slack did this. Attributing it to whoever installed the connection + // would put a person's name on an act they did not perform. + actorId: null, + institutionId: "inst_1", + }), + }), + ) + }) + + it("writes the receipt and the revocation in ONE transaction", async () => { + // Not decoration. Committing the receipt separately would let a failed + // revocation leave a receipt behind, and the dedupe would then turn away + // Slack's retry — the one delivery that could still have fixed it. + await post(UNINSTALLED) + + expect(transaction).toHaveBeenCalledTimes(1) + expect(createReceipt).toHaveBeenCalledTimes(1) + expect(update).toHaveBeenCalledTimes(1) + }) + + it("runs unscoped, by name", async () => { + // A Slack delivery has no session and no tenant; the tenant is derived from + // the workspace inside. `detail` is what makes an audit of unscoped work + // read as a list of operations. + await post(UNINSTALLED) + expect(runUnscoped).toHaveBeenCalledWith( + "control-plane", + "slack inbound event", + expect.any(Function), + ) + }) + + it("keeps a minimised receipt — identity, not payload", async () => { + await post(UNINSTALLED) + + expect(receipt()).toMatchObject({ + institutionId: "inst_1", + connectionId: "conn_1", + providerId: "slack.workspace", + externalId: "T0001", + externalEventId: "Ev0001", + semanticKey: "T0001:app_uninstalled:1760000000.000100", + envelopeType: "event_callback", + eventType: "app_uninstalled", + outcome: "HANDLED", + connectionsMatched: 1, + }) + expect(JSON.stringify(receipt())).not.toContain("z26uFbvR1xHJEdHE1OQiO6t8") + }) + + it("revokes EVERY institution's connection to the workspace, not the first", async () => { + // `Connection` is unique per (institution, provider, external id), so one + // Slack workspace connected by two institutions is two legitimate rows. An + // uninstall is a fact about the workspace, so it is a fact about both — a + // `findFirst` here would leave the other holding a dead token. + findMany.mockResolvedValue([ + CONNECTION, + { ...CONNECTION, id: "conn_2", institutionId: "inst_2" }, + ]) + + await post(UNINSTALLED) + + expect(update).toHaveBeenCalledTimes(2) + expect(deleteMany).toHaveBeenCalledTimes(2) + expect(createAudit).toHaveBeenCalledTimes(2) + // No single tenant owns this delivery, so none is claimed. + expect(receipt()).toMatchObject({ + institutionId: null, + connectionId: null, + connectionsMatched: 2, + }) + }) + + it("records a workspace it has no connection for instead of dropping it", async () => { + // A verified delivery for a workspace with no row — an install abandoned + // after the OAuth exchange looks exactly like this, and it is invisible + // unless something writes it down. + findMany.mockResolvedValue([]) + + const response = await post(UNINSTALLED) + expect(response.status).toBe(200) + expect(update).not.toHaveBeenCalled() + expect(receipt()).toMatchObject({ + institutionId: null, + connectionId: null, + connectionsMatched: 0, + outcome: "NOT_PROCESSED", + }) + expect(receipt().note).toContain("no connection matches workspace T0001") + }) + + it("is a no-op when the connection is already revoked", async () => { + // Slack sends `app_uninstalled` and `tokens_revoked` for a single uninstall. + // They are different events with different ids, so dedupe does not merge + // them and the second correctly finds the work already done. + findMany.mockResolvedValue([{ ...CONNECTION, status: "REVOKED" }]) + + const response = await post(UNINSTALLED) + expect(response.status).toBe(200) + expect(update).not.toHaveBeenCalled() + expect(receipt()).toMatchObject({ outcome: "NO_ACTION" }) + expect(receipt().note).toContain("already revoked") + }) +}) + +describe("tokens_revoked", () => { + const withTokens = (tokens: Record) => + envelope({ type: "tokens_revoked", event_ts: "1760000000.000200", tokens }) + + it("revokes when the BOT token is gone", async () => { + const response = await post(withTokens({ oauth: ["U1"], bot: ["U2"] })) + expect(response.status).toBe(200) + expect(update).toHaveBeenCalledTimes(1) + }) + + it("leaves the connection alone when only a member's own token was revoked", async () => { + const response = await post(withTokens({ oauth: ["U1"] })) + + expect(response.status).toBe(200) + expect(update).not.toHaveBeenCalled() + expect(deleteMany).not.toHaveBeenCalled() + expect(receipt()).toMatchObject({ outcome: "NO_ACTION" }) + }) + + it("keeps no member ids in the receipt", async () => { + await post(withTokens({ oauth: ["U0001"], bot: ["U0002"] })) + const written = JSON.stringify(receipt()) + expect(written).not.toContain("U0001") + expect(written).not.toContain("U0002") + }) +}) + +describe("events nothing here handles", () => { + it("acknowledges, records, and says plainly that nothing processed it", async () => { + const response = await post( + envelope({ type: "channel_archive", channel: "C0001", event_ts: "1760000000.000300" }), + ) + + expect(response.status).toBe(200) + expect(update).not.toHaveBeenCalled() + expect(receipt()).toMatchObject({ outcome: "NOT_PROCESSED", eventType: "channel_archive" }) + expect(receipt().note).toContain("not an event this endpoint subscribes to") + }) +}) + +describe("idempotence, because providers retry", () => { + it("acknowledges a delivery it has already recorded", async () => { + // The unique index refused the write. The event is already recorded and + // already handled, by the delivery this one repeats — so a 2xx is the + // truthful answer, and anything else asks Slack to send it again. + createReceipt.mockRejectedValue(Object.assign(new Error("unique"), { code: "P2002" })) + + const response = await post(UNINSTALLED) + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ ok: true, duplicate: true }) + }) + + it("records Slack's retry counter when it is retrying", async () => { + await post(UNINSTALLED, { headers: { "x-slack-retry-num": "2", "x-slack-retry-reason": "http_timeout" } }) + expect(receipt()).toMatchObject({ retryNum: 2, retryReason: "http_timeout" }) + }) + + it("does not record a first delivery as retry zero", async () => { + // `Number(null)` is 0 and `Number.isInteger(0)` is true, so a parse without + // a presence check makes every event that ever arrives look like a retry. + await post(UNINSTALLED) + expect(receipt()).toMatchObject({ retryNum: null, retryReason: null }) + }) + + it("asks for a redelivery when it could not record the event", async () => { + // Nothing was written, so nothing may be acknowledged. A 500 is the request + // for a retry; a 200 here loses the uninstall permanently. + createReceipt.mockRejectedValue(new Error("connection terminated unexpectedly")) + + const response = await post(UNINSTALLED) + expect(response.status).toBe(500) + await expect(response.json()).resolves.toEqual({ error: "not_recorded" }) + }) +}) diff --git a/apps/web/src/app/api/integrations/slack/events/route.ts b/apps/web/src/app/api/integrations/slack/events/route.ts new file mode 100644 index 00000000..9f27456e --- /dev/null +++ b/apps/web/src/app/api/integrations/slack/events/route.ts @@ -0,0 +1,331 @@ +import type { Prisma } from "@prisma/client" +import { db, type TxClient } from "@/lib/db" +import { runUnscoped } from "@/lib/tenancy/context" +import { + CONNECTION_REVOKED_ACTION, + recordConnectionEvent, +} from "@/lib/integrations/connection-audit" +import { + parseInbound, + planFor, + slackSigningKeys, + verifyWithRotation, + type InboundDelivery, + type InboundPlan, +} from "@/lib/integrations/slack/events" +import { SLACK_WORKSPACE_PRODUCT_ID } from "@/lib/integrations/slack/provider" + +/** + * Slack's inbound events endpoint. The first caller `verify.ts` has ever had. + * + * ── Order of operations, and why it is not negotiable ─────────────────────── + * + * The body is read as bytes and verified before anything reads a field out of + * it. That ordering is the whole control: the signature covers the exact bytes, + * so parsing first and re-serialising produces a different string and a + * signature that never matches — and, worse, acting on a parsed field before + * the check means the check is decoration. Anyone on the internet can POST + * here. + * + * ── One refusal, no reasons ───────────────────────────────────────────────── + * + * Every verification failure returns the identical 401. A caller cannot learn + * whether the signature was absent, malformed, from the wrong key, or merely + * outside the replay window, because each of those is a hint about how to get + * closer. `not-configured` is folded in with the rest for the same reason: a + * distinct 503 would tell a stranger this deployment has no signing secret, + * which is precisely when it is most worth probing. The reason goes to the log, + * where the operator is. + * + * ── What happens inline, and why ──────────────────────────────────────────── + * + * Slack retries on any non-2xx — 1s, 1m, 5m — and expects an acknowledgement + * within three seconds. The temptation is to acknowledge first and work later. + * This does the opposite for exactly two events, and the reason is that both + * are about a credential: + * + * `app_uninstalled` and `tokens_revoked` mean the bot token this deployment + * holds is dead. Every second the connection still reads `ACTIVE` is a second + * in which the product will hand a revoked credential to Slack and show an + * officer a failure it cannot explain. The work is one indexed read, one + * update, one delete and one audit insert — bounded, in a transaction capped + * below Slack's deadline. Deferring that costs more than doing it. + * + * Everything else is recorded and not processed. That is a statement, not a + * gap: see `recordAndHandle`. + * + * ── Idempotence ───────────────────────────────────────────────────────────── + * + * The receipt row and the effect are written in ONE transaction, which is + * stronger than "record, then process" and deliberately so. Recording first and + * committing separately would produce the worst outcome available here: the + * receipt lands, dedupe now refuses the delivery, the processing that failed + * never happens, and Slack's retry is turned away by our own de-duplication. In + * one transaction a failure rolls back both, Slack retries the same `event_id`, + * and the second attempt is indistinguishable from the first. + * + * A duplicate is therefore a unique-constraint violation, and answering it with + * 200 is correct: the event is already recorded and already handled. + */ + +export const dynamic = "force-dynamic" + +/** + * The largest body this will read. + * + * A public endpoint that awaits `text()` with no ceiling will read whatever it + * is sent, and the HMAC cannot be computed on a prefix — verification needs all + * of it, so an attacker choosing the size chooses the memory. Slack's own event + * payloads are kilobytes; a megabyte is far above anything real and far below + * anything that hurts. + */ +const MAX_BODY_BYTES = 1024 * 1024 + +/** + * How long the write may take. + * + * Prisma's defaults — 2s waiting for a connection, 5s running — add to more + * than Slack's three-second deadline, so a saturated pool would produce a + * request Slack has already given up on while it still holds a transaction + * open. Capped under the deadline instead: a slow database becomes a fast 500, + * and a 500 is what makes Slack redeliver. + */ +const TRANSACTION_LIMITS = { maxWait: 800, timeout: 1_500 } as const + +export async function POST(req: Request) { + const declaredLength = Number(req.headers.get("content-length") ?? "") + if (Number.isFinite(declaredLength) && declaredLength > MAX_BODY_BYTES) { + return Response.json({ error: "payload_too_large" }, { status: 413 }) + } + + // Bytes, before anything is believed about them. + const rawBody = await req.text() + if (Buffer.byteLength(rawBody, "utf8") > MAX_BODY_BYTES) { + return Response.json({ error: "payload_too_large" }, { status: 413 }) + } + + const verification = verifyWithRotation({ + rawBody, + signature: req.headers.get("x-slack-signature"), + timestamp: req.headers.get("x-slack-request-timestamp"), + keys: slackSigningKeys(process.env), + nowSeconds: Math.floor(Date.now() / 1000), + }) + + if (!verification.valid) { + // The only place the reason exists. `stale-timestamp` here is a replayed or + // clock-skewed request and reads differently from `signature-mismatch`, + // which during a rotation means both keys were tried and neither matched. + console.warn(`[slack] inbound event refused: ${verification.reason}`) + return refuse() + } + + if (verification.key === "previous") { + // Worth a line every time. It means Slack is signing with a key this + // deployment considers superseded, so the rotation is half-finished — the + // window this second slot exists to cover, and one somebody has to close. + console.warn("[slack] inbound event verified with the PREVIOUS signing key") + } + + const parsed = parseInbound(rawBody) + + if (parsed.kind === "url_verification") { + // The handshake, answered inline and without touching the database. It is + // not an event: there is nothing to process later and nothing to + // deduplicate, and making the endpoint's own configuration depend on a + // healthy database would mean a Request URL that cannot be saved during an + // incident. The signature was still checked — a stranger cannot make this + // deployment echo a value of their choosing. + return Response.json({ challenge: parsed.challenge }) + } + + const delivery = parsed.delivery + const plan = planFor(delivery, rawBody) + + try { + // Unscoped: a Slack delivery arrives with no session and no tenant. The + // tenant, if there is one, is derived below from the workspace the envelope + // names — which is why this cannot open a scope first. Named so an audit of + // what runs unscoped reads as a list of specific operations. + const result = await runUnscoped("control-plane", "slack inbound event", async () => + db.$transaction( + async (tx: TxClient) => recordAndHandle(tx, delivery, plan, retryOf(req)), + TRANSACTION_LIMITS, + ), + ) + return Response.json({ ok: true, ...result }) + } catch (error) { + if (isDuplicate(error)) { + // Already recorded and already handled — by the delivery this one is a + // repeat of. The two unique indexes on WebhookReceipt are the only unique + // constraints anything in that transaction touches, so a P2002 here has + // exactly one meaning. + return Response.json({ ok: true, duplicate: true }) + } + + // Nothing was recorded, so nothing may be acknowledged. A 500 is the + // request for a redelivery, and the transaction rolled back, so the retry + // will not find a half-written receipt in its way. + console.error("[slack] failed to record an inbound event:", error) + return Response.json({ error: "not_recorded" }, { status: 500 }) + } +} + +/** Every verification failure, indistinguishable from every other. */ +function refuse() { + return Response.json({ error: "unauthorized" }, { status: 401 }) +} + +/** Slack's retry counter, when it is retrying. */ +function retryOf(req: Request): { num: number | null; reason: string | null } { + const header = req.headers.get("x-slack-retry-num") + // Presence first: `Number(null)` is 0 and `Number.isInteger(0)` is true, so a + // parse without this records every first delivery as retry zero — which reads + // as "Slack retried" for every event that ever arrives. + const num = header === null ? null : Number(header) + return { + num: num !== null && Number.isInteger(num) ? num : null, + reason: req.headers.get("x-slack-retry-reason"), + } +} + +type HandledResult = { + outcome: "HANDLED" | "NO_ACTION" | "NOT_PROCESSED" + connectionsMatched: number +} + +/** + * Write the receipt and apply whatever this endpoint can honestly apply. + * + * ── Why the workspace is looked up as a list ──────────────────────────────── + * + * `Connection`'s unique key is `(institutionId, providerId, externalId)`, so + * one Slack workspace connected by two institutions is two legitimate rows — + * `announce.ts` already refuses to pick between them for exactly this reason. + * An uninstall is a fact about the workspace, so it is a fact about every one + * of those connections, and a `findFirst` would have revoked whichever row the + * database happened to return and left the other holding a dead token. + * + * ── What "not processed" means ────────────────────────────────────────────── + * + * The receipt records that a verified delivery arrived and that no code path + * acted on it, in a sentence. It is not a queue and cannot become one: the + * payload is deliberately not stored (see the model), so nothing can be + * replayed out of this table. + * + * The durable carrier for deferred work is the transactional outbox, and it is + * BLOCKED — Identity §21.2 demands tenant/cell, aggregate and actor identity on + * the envelope while Integration §9 demands connectionId/integrationId/runId/ + * mapping, and one envelope cannot satisfy both as written; that needs an ADR, + * not a workaround. §9 also requires a payload to live behind a governed + * external reference rather than inline in a row. Keeping bodies here to drain + * later would be a second carrier built without the decision that governs the + * first, so this says plainly what it did not do instead. + * + * The events this deployment actually subscribes to are both handled inline, so + * nothing subscribed is going unprocessed today. A NOT_PROCESSED receipt means + * Slack sent something the app config asked for and this code does not — which + * is a configuration drift worth seeing, and is why the note names the type. + */ +async function recordAndHandle( + tx: TxClient, + delivery: InboundDelivery, + plan: InboundPlan, + retry: { num: number | null; reason: string | null }, +): Promise { + const connections = delivery.teamId + ? await tx.connection.findMany({ + where: { providerId: SLACK_WORKSPACE_PRODUCT_ID, externalId: delivery.teamId }, + select: { id: true, institutionId: true, externalName: true, status: true }, + }) + : [] + + // Only when the workspace resolves to exactly one connection is there a + // single tenant this delivery is about. Zero and two both leave it null + // rather than guessed; `connectionsMatched` is what tells them apart. + const sole = connections.length === 1 ? connections[0] : null + + let outcome: HandledResult["outcome"] + let note: string + + if (plan.action === "record-only") { + outcome = plan.outcome === "no-action" ? "NO_ACTION" : "NOT_PROCESSED" + note = plan.note + } else { + const live = connections.filter((connection) => connection.status !== "REVOKED") + + if (connections.length === 0) { + // Verified, and about a workspace this deployment has no connection for. + // An install abandoned after the OAuth exchange looks exactly like this, + // and it is the reason a receipt is written for a tenant that cannot be + // named rather than dropped. + outcome = "NOT_PROCESSED" + note = `${plan.note}, but no connection matches workspace ${delivery.teamId} — nothing to revoke` + } else if (live.length === 0) { + // Slack sends both `app_uninstalled` and `tokens_revoked` for a single + // uninstall. They are different events with different ids, so dedupe does + // not merge them and the second one correctly finds the work already done. + outcome = "NO_ACTION" + note = `${plan.note}; the connection was already revoked` + } else { + for (const connection of live) { + await tx.connection.update({ + where: { id: connection.id }, + data: { status: "REVOKED", revokedAt: new Date() }, + }) + + // Offboarding deletion. The subscription is what says this deployment + // expects deliveries for this workspace; once the app is gone from it, + // leaving the row would state something that stopped being true. + await tx.webhookSubscription.deleteMany({ where: { connectionId: connection.id } }) + + await recordConnectionEvent(tx, { + institutionId: connection.institutionId, + // No actor. Slack did this, and attributing it to whoever installed + // the connection would put a person's name on an act they did not + // perform. + actorId: null, + action: CONNECTION_REVOKED_ACTION, + connectionId: connection.id, + providerId: SLACK_WORKSPACE_PRODUCT_ID, + externalId: delivery.teamId ?? "", + externalName: connection.externalName, + reason: plan.reason, + }) + } + + outcome = "HANDLED" + note = + live.length === connections.length + ? plan.note + : `${plan.note}; ${live.length} of ${connections.length} connections were still active` + } + } + + await tx.webhookReceipt.create({ + data: { + institutionId: sole?.institutionId ?? null, + connectionId: sole?.id ?? null, + providerId: SLACK_WORKSPACE_PRODUCT_ID, + externalId: delivery.teamId, + externalEventId: delivery.externalEventId, + semanticKey: delivery.semanticKey, + envelopeType: delivery.envelopeType, + eventType: delivery.eventType, + bodyDigest: delivery.bodyDigest, + bodySize: delivery.bodySize, + retryNum: retry.num, + retryReason: retry.reason, + outcome, + note, + connectionsMatched: connections.length, + }, + }) + + return { outcome, connectionsMatched: connections.length } +} + +/** A delivery this endpoint has already recorded. */ +function isDuplicate(error: unknown): boolean { + return (error as Prisma.PrismaClientKnownRequestError)?.code === "P2002" +} diff --git a/apps/web/src/lib/integrations/connection-audit.ts b/apps/web/src/lib/integrations/connection-audit.ts index e07926d7..ebb502a1 100644 --- a/apps/web/src/lib/integrations/connection-audit.ts +++ b/apps/web/src/lib/integrations/connection-audit.ts @@ -25,19 +25,35 @@ import type { TxClient } from "@/lib/db" /** The audit action a completed install writes. */ export const CONNECTION_INSTALLED_ACTION = "Integration.Connection.Installed" +/** + * The audit action a revocation writes. + * + * Provider-initiated: the inbound events endpoint writes this when Slack says + * the app was removed from a workspace or its bot token was revoked. There is + * no actor — nobody at this institution did it — which is why + * `ConnectionAuditEntry.actorId` is nullable and why `reason` below exists. A + * revocation with no explanation reads identically whether the workspace + * uninstalled the app or somebody here made a mistake. + */ +export const CONNECTION_REVOKED_ACTION = "Integration.Connection.Revoked" + /** What these rows are about. Matches the model name. */ export const CONNECTION_RESOURCE_TYPE = "Connection" /** * The `Integration.Connection.*` vocabulary. * - * One member, because one lifecycle event has a code path. Revoking a - * connection and changing its scopes are real events and neither is implemented - * anywhere in this application yet; naming them here would put actions in the - * vocabulary that can never appear, and someone reading the trail cannot tell an - * action nobody writes from a gap in the record. + * Two members, because two lifecycle events have code paths: install + * (`slack/callback`) and revocation (`slack/events`). Changing a connection's + * scopes is a real event and is still not implemented anywhere — Slack has no + * event for it, and a scope change arrives as a fresh install through the OAuth + * callback — so it is not named here. An action in the vocabulary that nothing + * ever writes is worse than an absent one: a reader of the trail cannot tell it + * from a gap in the record. */ -export type ConnectionAuditAction = typeof CONNECTION_INSTALLED_ACTION +export type ConnectionAuditAction = + | typeof CONNECTION_INSTALLED_ACTION + | typeof CONNECTION_REVOKED_ACTION export interface ConnectionAuditEntry { institutionId: string @@ -50,6 +66,11 @@ export interface ConnectionAuditEntry { /** The provider's own id for what was connected — a Slack team id. */ externalId: string externalName?: string | null + /** + * Why, for an action that can happen for more than one reason. Absent on an + * install, which has exactly one. + */ + reason?: string | null } export async function recordConnectionEvent( @@ -71,6 +92,7 @@ export async function recordConnectionEvent( providerId: entry.providerId, externalId: entry.externalId, externalName: entry.externalName ?? null, + reason: entry.reason ?? null, }, }, }) diff --git a/apps/web/src/lib/integrations/slack/announce.test.ts b/apps/web/src/lib/integrations/slack/announce.test.ts index 0a800e5a..1c404947 100644 --- a/apps/web/src/lib/integrations/slack/announce.test.ts +++ b/apps/web/src/lib/integrations/slack/announce.test.ts @@ -27,6 +27,7 @@ jest.mock("@/lib/db", () => { const connections: Row[] = [] const auditEvents: Row[] = [] + const subscriptions: Row[] = [] // Field-by-field equality against whatever predicate the caller passed. This // is the whole point of the fake: it knows nothing about Slack, so a reader @@ -79,8 +80,27 @@ jest.mock("@/lib/db", () => { }, } + // The install writes one of these beside the connection, and the events + // endpoint deletes it on `app_uninstalled`. Keyed on `connectionId` like the + // real unique index, so a re-install converges on the row it already wrote + // instead of leaving a second one pointed at a stale endpoint. + const webhookSubscription = { + ...table(subscriptions, "subscription-"), + async upsert({ where, update, create }: { where: Row; update: Row; create: Row }) { + const existing = subscriptions.find((row) => matches(row, where)) + if (existing) { + Object.assign(existing, update) + return existing + } + const row = { id: `subscription-${subscriptions.length + 1}`, ...create } + subscriptions.push(row) + return row + }, + } + const db = { connection, + webhookSubscription, auditEvent: table(auditEvents, "audit-"), async $transaction(work: (tx: unknown) => Promise) { return work(db) @@ -118,6 +138,7 @@ import { currentUnscopedGrant, hasNoContext } from "@/lib/tenancy/context" import { findProduct } from "@/lib/integrations/catalog" import { CONNECTION_INSTALLED_ACTION } from "@/lib/integrations/connection-audit" import { announceEventToSlack, SLACK_POST_ACTION } from "./announce" +import { SLACK_SUBSCRIBED_EVENTS } from "./events" import { createInstallState } from "./install" import type { SlackPoster } from "./post" import { SLACK_WORKSPACE_PRODUCT_ID } from "./provider" @@ -152,6 +173,7 @@ const realEnv = { ...process.env } beforeEach(async () => { await db.connection.deleteMany() await db.auditEvent.deleteMany() + await db.webhookSubscription.deleteMany() process.env.SLACK_CLIENT_ID = "client-id" process.env.SLACK_CLIENT_SECRET = "client-secret" @@ -318,6 +340,42 @@ describe("a connection written by the install callback", () => { expect(audit).toHaveLength(2) expect(audit.every((row) => row.resourceId === rows[0].id)).toBe(true) }) + + it("carries a webhook subscription, because a connection is what gets delivered to", async () => { + // For Slack the installation IS the subscription — event delivery starts + // when the app enters the workspace and stops when it leaves, and there is + // no API call to make. Without this row, offboarding has nothing to find + // and delete when `app_uninstalled` arrives. + await completeTheInstall() + + const connection = await db.connection.findFirst({ where: { institutionId: INSTITUTION } }) + const rows = await db.webhookSubscription.findMany({ where: { connectionId: connection?.id } }) + + expect(rows).toHaveLength(1) + expect(rows[0]).toMatchObject({ + institutionId: INSTITUTION, + providerId: SLACK_WORKSPACE_PRODUCT_ID, + endpoint: "https://tenure.example/api/integrations/slack/events", + events: [...SLACK_SUBSCRIBED_EVENTS], + // Slack's Events API subscription has no lifetime and no renewal call, so + // there is nothing here to renew. A provider whose subscriptions expire — + // Microsoft Graph tops out at three days — sets this, and that is the + // whole reason the column exists. + expiresAt: null, + }) + }) + + it("converges on re-install rather than leaving a second subscription", async () => { + // The unique key is the connection, so a second install must take the + // update branch. A stale row here would point at whatever endpoint the + // previous deployment used, which is exactly the drift this row exists to + // make visible. + await completeTheInstall() + await completeTheInstall() + + const rows = await db.webhookSubscription.findMany({}) + expect(rows).toHaveLength(1) + }) }) describe("a second Slack workspace connected to the same institution", () => { diff --git a/apps/web/src/lib/integrations/slack/events.test.ts b/apps/web/src/lib/integrations/slack/events.test.ts new file mode 100644 index 00000000..a38ccb45 --- /dev/null +++ b/apps/web/src/lib/integrations/slack/events.test.ts @@ -0,0 +1,357 @@ +import { computeSlackSignature, MAX_TIMESTAMP_SKEW_SECONDS } from "./verify" +import { + parseInbound, + planFor, + slackSigningKeys, + verifyWithRotation, + SLACK_SUBSCRIBED_EVENTS, + type InboundDelivery, +} from "./events" + +/** + * The decisions the inbound endpoint makes before it touches a database. + * + * Everything here is reachable from an unauthenticated POST, which is why it is + * pure: the refusals, the dedupe keys and the "there is nothing I can do about + * this" answers are the entire security and correctness surface, and none of + * them should need a Postgres to assert. + */ + +const CURRENT = "8f742231b10e8888abcd99yyyzzz85a5" +const PREVIOUS = "0e1e0e2e0e3e0e4e0e5e0e6e0e7e0e8e" +const NOW = 1_760_000_000 + +const sign = (body: string, secret: string, timestamp = String(NOW)) => + computeSlackSignature(body, timestamp, secret) + +/** An `event_callback` envelope, byte-exact, as Slack would send it. */ +const envelope = (event: Record, over: Record = {}) => + JSON.stringify({ + token: "z26uFbvR1xHJEdHE1OQiO6t8", + team_id: "T0001", + api_app_id: "A0001", + event, + type: "event_callback", + event_id: "Ev0001", + event_time: NOW, + ...over, + }) + +describe("verifyWithRotation", () => { + const verify = (over: Partial[0]> = {}) => { + const rawBody = envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" }) + return verifyWithRotation({ + rawBody, + timestamp: String(NOW), + signature: sign(rawBody, CURRENT), + keys: { current: CURRENT, previous: undefined }, + nowSeconds: NOW, + ...over, + }) + } + + it("accepts a request signed with the current key", () => { + expect(verify()).toEqual({ valid: true, key: "current" }) + }) + + it("accepts a request signed with the PREVIOUS key during a rotation", () => { + // The window this second slot exists for: Slack has already been + // regenerated, or the deploy carrying the new value was rolled back, and + // deliveries in flight are signed with a key this task calls superseded. + const rawBody = envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" }) + expect( + verify({ + rawBody, + signature: sign(rawBody, PREVIOUS), + keys: { current: CURRENT, previous: PREVIOUS }, + }), + ).toEqual({ valid: true, key: "previous" }) + }) + + it("refuses a key that is neither, even with both slots filled", () => { + const rawBody = envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" }) + expect( + verify({ + rawBody, + signature: sign(rawBody, "a-third-secret-nobody-issued"), + keys: { current: CURRENT, previous: PREVIOUS }, + }), + ).toEqual({ valid: false, reason: "signature-mismatch" }) + }) + + it("refuses when neither key is configured", () => { + expect(verify({ keys: { current: undefined, previous: undefined } })).toEqual({ + valid: false, + reason: "not-configured", + }) + }) + + it("does not let the previous key widen the replay window", () => { + // The rotation slot is about WHICH key, never about how old a request may + // be. A capture replayed six minutes later must still be refused, and it + // must be refused before a second HMAC is computed against the old key. + const rawBody = envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" }) + const stale = String(NOW - MAX_TIMESTAMP_SKEW_SECONDS - 1) + expect( + verify({ + rawBody, + timestamp: stale, + signature: sign(rawBody, PREVIOUS, stale), + keys: { current: CURRENT, previous: PREVIOUS }, + }), + ).toEqual({ valid: false, reason: "stale-timestamp" }) + }) + + it("reports the current key's reason, not the previous key's", () => { + // A missing header is a property of the request. Reporting whatever the + // second attempt happened to say would make the log describe the wrong + // thing during every rotation. + expect(verify({ signature: null, keys: { current: CURRENT, previous: PREVIOUS } })).toEqual({ + valid: false, + reason: "missing-signature", + }) + }) +}) + +describe("slackSigningKeys", () => { + it("reads both slots and treats whitespace as absent", () => { + expect( + slackSigningKeys({ + SLACK_SIGNING_SECRET: ` ${CURRENT} `, + SLACK_SIGNING_SECRET_PREVIOUS: " ", + }), + ).toEqual({ current: CURRENT, previous: undefined }) + }) + + it("reports no keys at all when nothing is configured", () => { + expect(slackSigningKeys({})).toEqual({ + current: undefined, + previous: undefined, + }) + }) +}) + +describe("parseInbound", () => { + it("recognises the URL handshake and carries the challenge out", () => { + const parsed = parseInbound( + JSON.stringify({ type: "url_verification", token: "t", challenge: "3eZbrw1a" }), + ) + expect(parsed).toEqual({ kind: "url_verification", challenge: "3eZbrw1a" }) + }) + + it("does not treat a handshake with no challenge as a handshake", () => { + // Answering `{"challenge": null}` would be rejected by Slack and would read + // to an operator as a network fault rather than a malformed handshake. + const parsed = parseInbound(JSON.stringify({ type: "url_verification" })) + expect(parsed.kind).toBe("delivery") + }) + + it("keeps both identities for an event_callback", () => { + const raw = envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" }) + const parsed = parseInbound(raw) + + expect(parsed.kind).toBe("delivery") + if (parsed.kind !== "delivery") throw new Error("unreachable") + + expect(parsed.delivery.externalEventId).toBe("Ev0001") + expect(parsed.delivery.semanticKey).toBe("T0001:app_uninstalled:1760000000.000100") + expect(parsed.delivery.teamId).toBe("T0001") + expect(parsed.delivery.eventType).toBe("app_uninstalled") + expect(parsed.delivery.bodySize).toBe(Buffer.byteLength(raw, "utf8")) + expect(parsed.delivery.bodyDigest).toMatch(/^[0-9a-f]{64}$/) + }) + + it("gives a redelivery under a NEW event id the SAME semantic key", () => { + // The case `event_id` alone cannot catch, and the reason there are two + // indexes: Slack re-sending an occurrence after a reinstall stamps it with + // a new delivery id but the same `event_ts`. + const event = { type: "app_uninstalled", event_ts: "1760000000.000100" } + const first = parseInbound(envelope(event, { event_id: "Ev0001" })) + const second = parseInbound(envelope(event, { event_id: "Ev0002" })) + + if (first.kind !== "delivery" || second.kind !== "delivery") throw new Error("unreachable") + expect(second.delivery.externalEventId).not.toBe(first.delivery.externalEventId) + expect(second.delivery.semanticKey).toBe(first.delivery.semanticKey) + }) + + it("gives two genuinely different occurrences different semantic keys", () => { + // The inverse, and the more important direction: collapsing two real + // uninstalls into one would leave a live credential nobody revoked. + const first = parseInbound(envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" })) + const second = parseInbound(envelope({ type: "app_uninstalled", event_ts: "1760000900.000100" })) + + if (first.kind !== "delivery" || second.kind !== "delivery") throw new Error("unreachable") + expect(second.delivery.semanticKey).not.toBe(first.delivery.semanticKey) + }) + + it("falls back to the body digest when the envelope carries no event_ts", () => { + const parsed = parseInbound(envelope({ type: "app_uninstalled" })) + if (parsed.kind !== "delivery") throw new Error("unreachable") + expect(parsed.delivery.semanticKey).toBe( + `event_callback:digest:${parsed.delivery.bodyDigest}`, + ) + }) + + it("records a body that will not parse rather than throwing", () => { + // The signature already proved Slack sent it, so this is evidence of + // something wrong upstream, and evidence belongs in the receipt. + const parsed = parseInbound("not json at all") + if (parsed.kind !== "delivery") throw new Error("unreachable") + + expect(parsed.delivery.envelopeType).toBe("unparseable") + expect(parsed.delivery.externalEventId).toBeNull() + expect(parsed.delivery.semanticKey).toBe(`unparseable:digest:${parsed.delivery.bodyDigest}`) + }) + + it("survives a JSON body that is not an object", () => { + const parsed = parseInbound("[1,2,3]") + if (parsed.kind !== "delivery") throw new Error("unreachable") + expect(parsed.delivery.envelopeType).toBe("unknown") + expect(parsed.delivery.teamId).toBeNull() + }) +}) + +describe("planFor", () => { + const planOf = (raw: string) => { + const parsed = parseInbound(raw) + if (parsed.kind !== "delivery") throw new Error("expected a delivery") + return { plan: planFor(parsed.delivery, raw), delivery: parsed.delivery } + } + + it("revokes on app_uninstalled", () => { + const { plan } = planOf(envelope({ type: "app_uninstalled", event_ts: "1760000000.000100" })) + expect(plan).toMatchObject({ action: "revoke-connection", reason: "app-uninstalled" }) + }) + + it("revokes on tokens_revoked ONLY when a bot token is in the list", () => { + const { plan } = planOf( + envelope({ + type: "tokens_revoked", + event_ts: "1760000000.000200", + tokens: { oauth: ["U0001"], bot: ["U0002"] }, + }), + ) + expect(plan).toMatchObject({ action: "revoke-connection", reason: "bot-token-revoked" }) + }) + + it("leaves the connection alone when only USER tokens were revoked", () => { + // The defect this prevents: one member revoking their own access + // disconnects the whole club's Slack, and the club sees announcements stop + // with no way to tell it from Slack being down. + const { plan } = planOf( + envelope({ + type: "tokens_revoked", + event_ts: "1760000000.000200", + tokens: { oauth: ["U0001"] }, + }), + ) + expect(plan).toEqual({ + action: "record-only", + outcome: "no-action", + note: "no action: only user tokens were revoked; the bot token is unaffected", + }) + }) + + it("treats an empty bot array as no bot token revoked", () => { + const { plan } = planOf( + envelope({ + type: "tokens_revoked", + event_ts: "1760000000.000200", + tokens: { oauth: ["U0001"], bot: [] }, + }), + ) + expect(plan).toMatchObject({ action: "record-only", outcome: "no-action" }) + }) + + it("records an event nobody here handles, and names it", () => { + const { plan } = planOf( + envelope({ type: "channel_archive", channel: "C0001", event_ts: "1760000000.000300" }), + ) + expect(plan).toEqual({ + action: "record-only", + outcome: "not-processed", + note: 'not processed: "channel_archive" is not an event this endpoint subscribes to', + }) + }) + + it("records an envelope type this endpoint does not handle", () => { + const { plan } = planOf(JSON.stringify({ type: "app_rate_limited", team_id: "T0001" })) + expect(plan).toMatchObject({ action: "record-only", outcome: "not-processed" }) + if (plan.action !== "record-only") throw new Error("unreachable") + expect(plan.note).toContain("app_rate_limited") + }) + + it("refuses to act on an envelope that names no workspace", () => { + // Without a team id there is nothing to resolve a connection from, so + // "revoke" would have to guess which one. + const { plan } = planOf( + JSON.stringify({ + type: "event_callback", + event: { type: "app_uninstalled", event_ts: "1760000000.000100" }, + }), + ) + expect(plan).toEqual({ + action: "record-only", + outcome: "not-processed", + note: "not processed: the envelope names no workspace", + }) + }) + + it("records an unparseable body", () => { + const { plan } = planOf("}{") + expect(plan).toEqual({ + action: "record-only", + outcome: "not-processed", + note: "not processed: the body is not JSON", + }) + }) + + it("acts on every event the app subscribes to, and only those", () => { + // The rule the subscription list is held to. An event we ask Slack for and + // then do nothing with produces receipts that read as coverage; the reverse + // is worse still, an event handled here that Slack never sends. + const acted = SLACK_SUBSCRIBED_EVENTS.filter((type) => { + const raw = envelope({ + type, + event_ts: "1760000000.000100", + tokens: { bot: ["U0002"] }, + }) + return planOf(raw).plan.action === "revoke-connection" + }) + expect([...acted]).toEqual([...SLACK_SUBSCRIBED_EVENTS]) + }) +}) + +describe("the delivery kept for the receipt", () => { + it("carries no payload — only its digest and size", () => { + // The minimisation, asserted rather than described. `tokens_revoked` names + // the members whose tokens went away and a `message` event carries what + // somebody typed; neither belongs in a table that exists to say a delivery + // happened. + const raw = envelope({ + type: "tokens_revoked", + event_ts: "1760000000.000200", + tokens: { oauth: ["U0001"], bot: ["U0002"] }, + }) + const parsed = parseInbound(raw) + if (parsed.kind !== "delivery") throw new Error("unreachable") + + const serialised = JSON.stringify(parsed.delivery) + expect(serialised).not.toContain("U0001") + expect(serialised).not.toContain("U0002") + expect(serialised).not.toContain("z26uFbvR1xHJEdHE1OQiO6t8") + + // And the fields are exactly the ones declared, so a payload cannot arrive + // here by someone widening the type later without noticing. + const fields: Record = { + envelopeType: true, + externalEventId: true, + semanticKey: true, + teamId: true, + eventType: true, + bodyDigest: true, + bodySize: true, + } + expect(Object.keys(parsed.delivery).sort()).toEqual(Object.keys(fields).sort()) + }) +}) diff --git a/apps/web/src/lib/integrations/slack/events.ts b/apps/web/src/lib/integrations/slack/events.ts new file mode 100644 index 00000000..c23d69b3 --- /dev/null +++ b/apps/web/src/lib/integrations/slack/events.ts @@ -0,0 +1,369 @@ +import { createHash } from "node:crypto" +import { verifySlackRequest, type SlackVerification } from "./verify" + +/** + * What arrives at the inbound Slack endpoint, decided before any of it is + * believed. + * + * `verify.ts` answers "did Slack send this". Everything after that is a second + * question — "what is it, have we seen it before, and is there anything we can + * honestly do about it" — and it lives here, pure, for the same reason the + * verifier does: these are refusals and de-duplications, and a decision that + * cannot be asserted without a database is a decision nobody checks. + * + * ── Two identities per delivery, not one ──────────────────────────────────── + * + * Slack stamps each delivery with an `event_id` (`Ev…`) and re-sends the *same* + * id when it retries, so that alone dedupes the retry storm — three retries at + * 1s, 1m and 5m for any non-2xx, which is the common case. It does not dedupe + * the same occurrence arriving under a new id, which happens when an app is + * reinstalled or a workspace ends up subscribed twice, and it does not exist at + * all on an envelope Slack sends without one. + * + * So every delivery also carries a *semantic* key derived from its content: + * team, event type and `event_ts`. `event_ts` is Slack's stamp for the + * occurrence itself, so two deliveries agreeing on all three are the same thing + * happening once, whatever ids they were given. The receipt table holds a + * unique index on each, and whichever conflicts first refuses the write. + * + * ── Which events this endpoint subscribes to ──────────────────────────────── + * + * `SLACK_SUBSCRIBED_EVENTS`, and deliberately nothing else. See its comment: + * subscribing to an event no code path acts on produces receipts that read as + * coverage and are not. + */ + +/** Where Slack delivers. Also what the subscription record says it expects. */ +export const SLACK_EVENTS_PATH = "/api/integrations/slack/events" + +/** + * The events this app asks Slack for. + * + * Both are lifecycle facts about the *installation* rather than workspace + * traffic, and both are acted on inline (see `planFor`). That pairing is the + * rule this list is held to: an event is subscribed because something here does + * something with it. + * + * Work Graph §10.3 also asks for channel archive and scope change, and neither + * is on this list — for reasons that are about Slack rather than about effort: + * + * - **Channel archive.** Nothing in this application persists a channel + * selection. `routing.ts` derives the destination from the audience and + * takes the club's channel as a caller argument, and `schema.prisma` holds + * no column for one. There is therefore no stored selection for + * `channel_archive` to invalidate; subscribing would write receipts nobody + * reads and let the section look covered. The fact still reaches a person, + * from the other direction: `post.ts` passes Slack's own refusal through, so + * a post to an archived channel comes back saying `is_archived`. + * - **Scope change.** Slack has no event for it. Granting a scope requires + * re-running the install, which arrives at the OAuth callback with a fresh + * grant — that path, not this one, is where a scope change is observed. + */ +export const SLACK_SUBSCRIBED_EVENTS = ["app_uninstalled", "tokens_revoked"] as const + +/** Both signing keys a deployment may hold. Current is tried first. */ +export interface SlackSigningKeys { + current: string | undefined + previous: string | undefined +} + +/** + * The signing keys, from the environment. + * + * Two slots because a signing secret is rotated at Slack, not here. The moment + * an operator presses Regenerate, Slack starts signing with the new value and + * every task still holding the old one refuses every delivery — and rolling + * back the deploy that carried the new value re-opens the same window in the + * other direction. Holding both means neither ordering has a gap, which is what + * Integration §12 is asking for when it says current *and* previous rotation + * keys. + * + * Takes the environment rather than reading `process.env` directly, so a test + * states the rotation it is exercising instead of mutating global state. + */ +export function slackSigningKeys(env: Record): SlackSigningKeys { + return { + current: env.SLACK_SIGNING_SECRET?.trim() || undefined, + previous: env.SLACK_SIGNING_SECRET_PREVIOUS?.trim() || undefined, + } +} + +export type RotationVerification = + | { valid: true; key: "current" | "previous" } + | { valid: false; reason: Exclude["reason"] } + +/** + * Verify against the current key, then the previous one. + * + * Only a `signature-mismatch` is retried. Every other refusal — no secret + * configured, no signature header, a timestamp outside the window, a version + * this code does not implement — is a property of the request rather than of + * the key, so a second pass would compute another HMAC to reach the identical + * answer. + * + * The reason reported is the current key's. The previous key exists to cover a + * rotation window, and "this did not match the key it should have matched" is + * what an operator needs in the log; the caller gets a refusal with no reason + * attached either way. + */ +export function verifyWithRotation(input: { + rawBody: string + signature: string | null | undefined + timestamp: string | null | undefined + keys: SlackSigningKeys + nowSeconds: number +}): RotationVerification { + const attempt = (signingSecret: string | undefined) => + verifySlackRequest({ + rawBody: input.rawBody, + signature: input.signature, + timestamp: input.timestamp, + signingSecret, + nowSeconds: input.nowSeconds, + }) + + const withCurrent = attempt(input.keys.current) + if (withCurrent.valid) return { valid: true, key: "current" } + + if (withCurrent.reason === "signature-mismatch" && input.keys.previous) { + if (attempt(input.keys.previous).valid) return { valid: true, key: "previous" } + } + + return { valid: false, reason: withCurrent.reason } +} + +/** A delivery, reduced to the facts a receipt is allowed to keep. */ +export interface InboundDelivery { + /** Envelope `type` — `event_callback` for everything we subscribe to. */ + envelopeType: string + /** Slack's own id for the delivery. Null when the envelope carries none. */ + externalEventId: string | null + /** Content identity. Always present, so dedupe never depends on the id above. */ + semanticKey: string + /** The Slack team the delivery is about. Null when the envelope names none. */ + teamId: string | null + /** Inner `event.type`; null when the envelope is not an `event_callback`. */ + eventType: string | null + /** SHA-256 of the exact bytes received. What the receipt keeps instead of the body. */ + bodyDigest: string + /** Bytes received. Cheap, and the one number that says "this was not what we expect". */ + bodySize: number +} + +export type ParsedInbound = + /** Slack proving it owns the URL. Answered inline; see the route. */ + | { kind: "url_verification"; challenge: string } + | { kind: "delivery"; delivery: InboundDelivery } + +/** + * Read a verified body. + * + * Called only after `verifyWithRotation` returns valid. The raw bytes are what + * the signature covers and re-serialising them breaks it, so parsing after + * verifying is not a style preference — it is the thing that makes the check + * work at all, and it is why this function takes a string rather than a + * `Request`. + * + * A body that will not parse still produces a delivery rather than an error. + * The signature already proved Slack sent it, so it is evidence of something + * going wrong at the provider or in transit, and a receipt is where that + * belongs; its identity falls back to the content digest, which is the only + * identity such a body has. + */ +export function parseInbound(rawBody: string): ParsedInbound { + const bodyDigest = sha256(rawBody) + const bodySize = Buffer.byteLength(rawBody, "utf8") + + let body: unknown + try { + body = JSON.parse(rawBody) + } catch { + return { + kind: "delivery", + delivery: { + envelopeType: "unparseable", + externalEventId: null, + semanticKey: `unparseable:digest:${bodyDigest}`, + teamId: null, + eventType: null, + bodyDigest, + bodySize, + }, + } + } + + const envelope = isRecord(body) ? body : {} + const envelopeType = stringOrNull(envelope.type) ?? "unknown" + + if (envelopeType === "url_verification") { + const challenge = stringOrNull(envelope.challenge) + // A handshake with nothing to echo is not a handshake. Falling through to a + // delivery records it instead of answering `{"challenge": null}`, which + // Slack rejects and which reads to an operator as a network problem. + if (challenge) return { kind: "url_verification", challenge } + } + + const event = isRecord(envelope.event) ? envelope.event : null + const eventType = event ? stringOrNull(event.type) : null + const teamId = stringOrNull(envelope.team_id) + + return { + kind: "delivery", + delivery: { + envelopeType, + externalEventId: stringOrNull(envelope.event_id), + semanticKey: semanticKeyFor({ envelopeType, teamId, eventType, event, bodyDigest }), + teamId, + eventType, + bodyDigest, + bodySize, + }, + } +} + +/** + * The content identity of an occurrence. + * + * `team:type:event_ts`, because `event_ts` describes the occurrence rather than + * the delivery: the same uninstall re-sent under a new `event_id` carries the + * same `event_ts`, and two different uninstalls cannot share one. + * + * When any part is missing the key falls back to the body digest, which is + * exact for a retry — Slack re-sends byte-identical bodies — and merely + * conservative otherwise. It can fail to notice that two deliveries were the + * same occurrence; it cannot claim two different occurrences were one. That is + * the safe direction to be wrong in: a duplicate receipt is noise, a swallowed + * uninstall is a live credential nobody revoked. + */ +function semanticKeyFor(input: { + envelopeType: string + teamId: string | null + eventType: string | null + event: Record | null + bodyDigest: string +}): string { + const eventTs = input.event ? stringOrNull(input.event.event_ts) : null + if (input.teamId && input.eventType && eventTs) { + return `${input.teamId}:${input.eventType}:${eventTs}` + } + return `${input.envelopeType}:digest:${input.bodyDigest}` +} + +/** + * What, if anything, this endpoint does about a delivery. + * + * `record-only` is not a failure state and it is not a queue. It means the + * delivery is durably recorded, nothing acted on it, and `note` says which of + * those in a sentence an operator can read — see the route's header for why + * there is no carrier to hand it to instead. + */ +export type InboundPlan = + | { action: "revoke-connection"; reason: "app-uninstalled" | "bot-token-revoked"; note: string } + | { + action: "record-only" + /** + * Which of the two `record-only` means. Kept as a field rather than + * inferred from `note`, because the route writes it to a column and a + * caller reading intent out of prose is how a note reworded for clarity + * silently changes what the receipt claims happened. + */ + outcome: "no-action" | "not-processed" + note: string + } + +/** + * Decide from the envelope alone. + * + * Takes the raw body a second time rather than carrying the whole payload on + * `InboundDelivery`, because the delivery is what gets written down and it is + * deliberately minimised: `tokens_revoked` names the members whose tokens went + * away, and those ids have no business in a receipt table. + */ +export function planFor(delivery: InboundDelivery, rawBody: string): InboundPlan { + if (delivery.envelopeType === "unparseable") { + return { + action: "record-only", + outcome: "not-processed", + note: "not processed: the body is not JSON", + } + } + + if (delivery.envelopeType !== "event_callback") { + return { + action: "record-only", + outcome: "not-processed", + note: `not processed: envelope type "${delivery.envelopeType}" is not one this endpoint handles`, + } + } + + if (!delivery.teamId) { + return { + action: "record-only", + outcome: "not-processed", + note: "not processed: the envelope names no workspace", + } + } + + switch (delivery.eventType) { + case "app_uninstalled": + return { + action: "revoke-connection", + reason: "app-uninstalled", + note: "the app was removed from the workspace", + } + + case "tokens_revoked": { + // Only the bot token matters. `tokens_revoked` fires when ANY token is + // revoked, including a departing member's user token, and the payload + // keeps them apart: `{"tokens": {"oauth": [...], "bot": [...]}}`. Reading + // every one of these as a revocation would disconnect a workspace whose + // posting credential is perfectly good, the first time any one person + // revoked their own access — and the club whose announcements stopped + // would have no way to tell that from Slack being down. + if (botTokensRevoked(rawBody)) { + return { + action: "revoke-connection", + reason: "bot-token-revoked", + note: "the bot token this connection posts with was revoked", + } + } + return { + action: "record-only", + outcome: "no-action", + note: "no action: only user tokens were revoked; the bot token is unaffected", + } + } + + default: + return { + action: "record-only", + outcome: "not-processed", + note: `not processed: "${delivery.eventType ?? "unknown"}" is not an event this endpoint subscribes to`, + } + } +} + +/** Whether a `tokens_revoked` payload lists at least one bot token. */ +function botTokensRevoked(rawBody: string): boolean { + try { + const body: unknown = JSON.parse(rawBody) + if (!isRecord(body) || !isRecord(body.event)) return false + const tokens = body.event.tokens + if (!isRecord(tokens)) return false + return Array.isArray(tokens.bot) && tokens.bot.length > 0 + } catch { + return false + } +} + +function sha256(value: string): string { + return createHash("sha256").update(value, "utf8").digest("hex") +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value) +} + +function stringOrNull(value: unknown): string | null { + return typeof value === "string" && value.trim() !== "" ? value : null +} diff --git a/apps/web/src/lib/tenancy/registry.test.ts b/apps/web/src/lib/tenancy/registry.test.ts index 053a4c7b..44ac55db 100644 --- a/apps/web/src/lib/tenancy/registry.test.ts +++ b/apps/web/src/lib/tenancy/registry.test.ts @@ -121,10 +121,27 @@ describe("the registry matches prisma/schema.prisma", () => { // of that bucket's `Organization.institutionId`/`Role -> Organization` // reachability; what remains is reachable through a Conversation, a Budget, // an ApprovalRequest, a FeedPost, a Deliverable or a User. - expect(TENANT_SCOPED).toHaveLength(22) + // + // 2026-08-20: 22 → 24 tenant-scoped, 41 → 43 models. `WebhookSubscription` + // and `WebhookReceipt` are the inbound-webhook pair — what a connection is + // subscribed to receive, and what verifiably arrived. Both are scoped: a + // subscription belongs to the institution whose administrator installed the + // app, and a receipt names the institution whose workspace the delivery was + // about. + // + // `WebhookReceipt.institutionId` is NULLABLE, which is a first for this + // bucket. Its tenant is derived from the provider's workspace id rather + // than supplied, and the derivation genuinely fails — no connection matched + // (an install abandoned after the OAuth exchange), or two did (one Slack + // workspace connected by two institutions, which the Connection unique key + // permits). It is still scoped rather than unenforceable, because the rows + // that do carry a tenant must be filtered by it, and the query layer's + // `institutionId: ` predicate correctly hides a null-tenant row from + // every tenant. + expect(TENANT_SCOPED).toHaveLength(24) expect(PLATFORM_GLOBAL).toHaveLength(5) expect(Object.keys(UNENFORCEABLE)).toHaveLength(14) - expect(schemaModels).toHaveLength(41) + expect(schemaModels).toHaveLength(43) }) }) diff --git a/apps/web/src/lib/tenancy/registry.ts b/apps/web/src/lib/tenancy/registry.ts index 5b5d6b09..0924f29e 100644 --- a/apps/web/src/lib/tenancy/registry.ts +++ b/apps/web/src/lib/tenancy/registry.ts @@ -21,7 +21,7 @@ * decision about its tenancy at the moment it is added, rather than leaving one * to be discovered later by a customer. * - * The three buckets are honest about a real limitation: only 22 of 41 models + * The three buckets are honest about a real limitation: only 24 of 43 models * carry `institutionId`, so only those can be filtered by the query layer * today. The rest are named in UNENFORCEABLE rather than quietly omitted. */ @@ -42,6 +42,14 @@ export const TENANT_SCOPED = [ "Connection", "DirectoryPerson", "RestrictedIdentity", + "WebhookSubscription", + // institutionId is NULLABLE here, and deliberately. A webhook receipt's + // tenant is derived from the workspace the provider named, and that + // derivation can find no connection or two — see the model's own comment. It + // still belongs in this bucket: the rows that DO carry a tenant must be + // filtered by it, and a null one is invisible to every tenant-scoped read, + // which is the correct answer for a delivery nobody owns. + "WebhookReceipt", // institutionId denormalised from a parent, held to it by a COMPOSITE foreign // key — `(organizationId, institutionId) -> Organization(id, institutionId)` // and so on. These are the identity tables: the roster itself. The composite diff --git a/docs/implementation/global-engine-execution-ledger.md b/docs/implementation/global-engine-execution-ledger.md index 4b847097..9466ba4b 100644 --- a/docs/implementation/global-engine-execution-ledger.md +++ b/docs/implementation/global-engine-execution-ledger.md @@ -1,6 +1,6 @@ # Tenure pilot (Simon OSE) — execution ledger - + The authoritative record of what this repository has actually implemented, with evidence. This is the repository that builds and deploys the live tenant, so @@ -121,11 +121,12 @@ citing parent-only tests. ADR-0012 records this. - Status: FAIL - Evidence: cross-tenant denial is enforced at the query layer and tested (ADR-0002; `apps/web/src/lib/tenancy/`), which covers the database path for the - 22 TENANT_SCOPED models — the roster itself (seats, assignments, seat + 24 TENANT_SCOPED models — the roster itself (seats, assignments, seat holdings and advisor links) joined them, held to their parent's tenant by - composite foreign keys rather than by convention. It is not proven per-path - for API routes, background jobs, exports, search or AI answers, and 14 of 41 - models are UNENFORCEABLE at the chokepoint. + composite foreign keys rather than by convention, and the inbound-webhook + pair joined them after. It is not proven per-path for API routes, background + jobs, exports, search or AI answers, and 14 of 43 models are UNENFORCEABLE + at the chokepoint. - [ ] **SIMON-070-003** — Render all target resources through Parent IaC with deterministic tags, ownership, stack boundaries, deletion/retention policy, and drift detection. - Status: FAIL diff --git a/infrastructure/terraform/ecs.tf b/infrastructure/terraform/ecs.tf index 562a7245..f8c227b5 100644 --- a/infrastructure/terraform/ecs.tf +++ b/infrastructure/terraform/ecs.tf @@ -297,6 +297,13 @@ resource "aws_ecs_task_definition" "app" { name = "SLACK_SIGNING_SECRET" valueFrom = "${aws_secretsmanager_secret.integrations.arn}:SLACK_SIGNING_SECRET::" }, + { + # The key being rotated out, normally empty. The events endpoint tries + # it only when the current one does not match — see the variable's + # comment in integrations.tf for why a rotation has a window at all. + name = "SLACK_SIGNING_SECRET_PREVIOUS" + valueFrom = "${aws_secretsmanager_secret.integrations.arn}:SLACK_SIGNING_SECRET_PREVIOUS::" + }, { # RDS-managed secret value is JSON {"username","password"} — the # entrypoint parses it and exports a proper DATABASE_URL. diff --git a/infrastructure/terraform/integrations.tf b/infrastructure/terraform/integrations.tf index 9eec5542..bfef26cb 100644 --- a/infrastructure/terraform/integrations.tf +++ b/infrastructure/terraform/integrations.tf @@ -54,6 +54,25 @@ variable "slack_signing_secret" { sensitive = true } +variable "slack_signing_secret_previous" { + description = <<-EOT + The signing secret being rotated OUT. From the GitHub secret + SLACK_SIGNING_SECRET_PREVIOUS. Normally empty. + + A signing secret is rotated at Slack, not here: the moment an operator + presses Regenerate, Slack signs with the new value and every task still + holding the old one refuses every delivery — and rolling the deploy back + re-opens the same window in the other direction. The endpoint tries this + value only when the current one does not match, so setting it for the length + of a rotation removes both gaps, and clearing it afterwards is what ends the + old key's life. Leaving it set indefinitely keeps a retired key valid, which + is the thing rotation exists to stop. + EOT + type = string + default = "" + sensitive = true +} + variable "slack_app_id" { description = "Slack app id. Not a credential; included so the app can link to its own install." type = string @@ -76,9 +95,10 @@ resource "aws_secretsmanager_secret_version" "integrations" { # apps/web/src/lib/integrations/catalog.ts and added here when the values # exist. secret_string = jsonencode({ - SLACK_CLIENT_ID = var.slack_client_id - SLACK_CLIENT_SECRET = var.slack_client_secret - SLACK_SIGNING_SECRET = var.slack_signing_secret + SLACK_CLIENT_ID = var.slack_client_id + SLACK_CLIENT_SECRET = var.slack_client_secret + SLACK_SIGNING_SECRET = var.slack_signing_secret + SLACK_SIGNING_SECRET_PREVIOUS = var.slack_signing_secret_previous }) } From 08dba22707757415d4507e530763d73e75a84e84 Mon Sep 17 00:00:00 2001 From: satvikOS Date: Thu, 20 Aug 2026 20:41:22 -0400 Subject: [PATCH 2/5] test(integrations): pin the receipt indexes against a real PostgreSQL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the endpoint's claims are properties of two unique indexes and a foreign key, and an index refuses where it exists rather than where a mock says it does. In particular the dedupe has to hold when two tasks handle Slack's retry at the same moment, which no in-process check can. Writing it found something. `(providerId, externalEventId)` does not include the workspace, so two institutions cannot both record event `Ev0001`. That is correct for Slack — and correct only because Slack documents `event_id` as unique across all workspaces, not per workspace. A provider that numbers events per tenant would break it in the worst direction available: the second tenant's uninstall answered 200 as a duplicate and never processed, leaving a dead credential reading ACTIVE. The constraint is now asserted rather than assumed, and named in the schema, so widening the key is a failing test instead of a deployment. Co-Authored-By: Claude Opus 5 (1M context) --- apps/web/prisma/schema.prisma | 8 + .../lib/integrations/webhook-receipt.itest.ts | 298 ++++++++++++++++++ 2 files changed, 306 insertions(+) create mode 100644 apps/web/src/lib/integrations/webhook-receipt.itest.ts diff --git a/apps/web/prisma/schema.prisma b/apps/web/prisma/schema.prisma index 9a6e38c5..e8938cad 100644 --- a/apps/web/prisma/schema.prisma +++ b/apps/web/prisma/schema.prisma @@ -1321,6 +1321,14 @@ model WebhookReceipt { /// envelope carried none, and NULLs do not collide in a Postgres unique /// index, which is the behaviour wanted here: an envelope with no id cannot /// be deduplicated on one, and the semantic key covers it instead. + /// + /// Its unique index does NOT include the workspace, which is an assertion + /// about the provider rather than an oversight: Slack documents `event_id` as + /// unique across all workspaces. A provider that numbers events per tenant + /// needs that key widened before it is added, or one tenant's delivery would + /// be answered as a duplicate of another's and never processed — + /// `webhook-receipt.itest.ts` pins the constraint so the widening cannot be + /// forgotten. externalEventId String? /// Content identity, always present. `team:type:event_ts` where the envelope diff --git a/apps/web/src/lib/integrations/webhook-receipt.itest.ts b/apps/web/src/lib/integrations/webhook-receipt.itest.ts new file mode 100644 index 00000000..981274c0 --- /dev/null +++ b/apps/web/src/lib/integrations/webhook-receipt.itest.ts @@ -0,0 +1,298 @@ +import { PrismaClient } from "@prisma/client" +import { tenancyExtension } from "@/lib/tenancy/extension" +import { runInTenantScope, runUnscoped } from "@/lib/tenancy/context" + +/** + * The inbound webhook receipt, against a real PostgreSQL. + * + * The route's tests prove what the endpoint decides. Three of its claims are + * not decisions at all — they are properties of two unique indexes and a + * foreign key, and an index refuses where it exists rather than where a mock + * says it does: + * + * - A repeated delivery is refused by the database, not by an `if`. That is + * what makes the endpoint idempotent under two tasks handling Slack's retry + * at the same moment, which no in-process check can be. + * - A NULL `externalEventId` must NOT collide with another NULL. Postgres + * treats NULLs as distinct in a unique index, and the design leans on that: + * an envelope with no provider id cannot be deduplicated on one, so the + * semantic key has to be free to do it instead. If that behaviour were the + * other way round, the second id-less delivery of any kind would be silently + * swallowed as a duplicate of the first. + * - Deleting a connection must not delete the evidence that deliveries + * arrived for it. + * + * Run with: npm run test:isolation (needs DATABASE_URL) + */ + +const db = new PrismaClient({ log: ["error"] }).$extends(tenancyExtension("enforce")) + +const SUFFIX = "itest-webhook-receipt" +const INST = `inst-${SUFFIX}` +const OTHER_INST = `other-${SUFFIX}` +const PROVIDER = "slack.workspace" +const TEAM = `T-${SUFFIX}` + +let connectionId = "" + +/** A receipt, with only the fields that are not being varied. */ +const receipt = (over: Record) => ({ + providerId: PROVIDER, + externalId: TEAM, + envelopeType: "event_callback", + eventType: "app_uninstalled", + bodyDigest: "0".repeat(64), + bodySize: 512, + outcome: "HANDLED" as const, + note: "the app was removed from the workspace", + connectionsMatched: 1, + ...over, +}) + +async function cleanup() { + await runUnscoped("migration", "webhook receipt test cleanup", async () => { + await db.webhookReceipt.deleteMany({ where: { providerId: PROVIDER, externalId: TEAM } }) + await db.webhookSubscription.deleteMany({ where: { institutionId: { in: [INST, OTHER_INST] } } }) + await db.connection.deleteMany({ where: { institutionId: { in: [INST, OTHER_INST] } } }) + await db.institution.deleteMany({ where: { id: { in: [INST, OTHER_INST] } } }) + }) +} + +beforeAll(async () => { + await cleanup() + await runUnscoped("seed", "webhook receipt fixture", async () => { + await db.institution.create({ + data: { id: INST, name: "Receipt Test University", slug: INST }, + }) + await db.institution.create({ + data: { id: OTHER_INST, name: "Other Test University", slug: OTHER_INST }, + }) + const connection = await db.connection.create({ + data: { institutionId: INST, providerId: PROVIDER, externalId: TEAM }, + }) + connectionId = connection.id + }) +}) + +afterAll(async () => { + await cleanup() + await db.$disconnect() +}) + +beforeEach(async () => { + await runUnscoped("migration", "clear receipts between cases", async () => { + await db.webhookReceipt.deleteMany({ where: { providerId: PROVIDER, externalId: TEAM } }) + }) +}) + +/** + * The fixture supplies `semanticKey` through `over`, so the compiler cannot see + * that every case provides it. Same cast, same reason, as `ledger.itest.ts`: + * the alternative is repeating every required field at every call and losing + * the one line that says what this case is actually varying. + */ +type ReceiptCreateArgs = Parameters[0] + +const create = (over: Record) => + runUnscoped("control-plane", "slack inbound event", async () => + db.webhookReceipt.create({ data: receipt(over) } as unknown as ReceiptCreateArgs), + ) + +describe("the dedupe is the index, not a check", () => { + it("refuses a second receipt with the same provider event id", async () => { + await create({ + institutionId: INST, + connectionId, + externalEventId: "Ev0001", + semanticKey: `${TEAM}:app_uninstalled:1760000000.000100`, + }) + + // Slack's retry: same event id, and — because it is a fresh HTTP request — + // a new signature and timestamp, so verification cannot be what stops it. + await expect( + create({ + institutionId: INST, + connectionId, + externalEventId: "Ev0001", + semanticKey: `${TEAM}:app_uninstalled:1760000000.000100`, + }), + ).rejects.toMatchObject({ code: "P2002" }) + }) + + it("refuses the same occurrence arriving under a NEW event id", async () => { + // The half `event_id` cannot catch, and the reason for the second index. + await create({ + institutionId: INST, + connectionId, + externalEventId: "Ev0001", + semanticKey: `${TEAM}:app_uninstalled:1760000000.000100`, + }) + + await expect( + create({ + institutionId: INST, + connectionId, + externalEventId: "Ev0002", + semanticKey: `${TEAM}:app_uninstalled:1760000000.000100`, + }), + ).rejects.toMatchObject({ code: "P2002" }) + }) + + it("does NOT collide two deliveries that carry no provider event id", async () => { + // Postgres treats NULLs as distinct in a unique index, and this design + // depends on it: an envelope with no id must fall through to the semantic + // key rather than being swallowed as a duplicate of every other id-less + // envelope ever received. + await create({ + externalEventId: null, + semanticKey: `unparseable:digest:${"a".repeat(64)}`, + envelopeType: "unparseable", + eventType: null, + outcome: "NOT_PROCESSED", + note: "not processed: the body is not JSON", + connectionsMatched: 0, + institutionId: null, + connectionId: null, + }) + + const second = await create({ + externalEventId: null, + semanticKey: `unparseable:digest:${"b".repeat(64)}`, + envelopeType: "unparseable", + eventType: null, + outcome: "NOT_PROCESSED", + note: "not processed: the body is not JSON", + connectionsMatched: 0, + institutionId: null, + connectionId: null, + }) + + expect(second.id).toBeTruthy() + }) + + it("holds the provider event id unique ACROSS tenants, deliberately", async () => { + // Written after this test caught the property by surprise, and kept because + // the property is load-bearing rather than accidental. + // + // `(providerId, externalEventId)` does not include the workspace, so two + // institutions cannot both record event `Ev0001`. That is correct for Slack + // and correct only because Slack says so: an `event_id` is documented as + // unique across all workspaces, not per workspace. The index is therefore + // an assertion about the provider's contract. + // + // A provider that numbers events per tenant would break it, and would break + // it in the worst available direction — the second tenant's delivery would + // be answered 200 as a duplicate and never processed, so an uninstall for + // that tenant would leave a dead credential reading ACTIVE. Widening the + // key to include the workspace is the migration that provider needs, and + // this failing here is how it gets noticed rather than deployed. + // + // The semantic key has no such dependency: the workspace is inside the + // value (`team:type:event_ts`), so it is workspace-qualified whatever the + // provider does with its ids. + await create({ + institutionId: INST, + connectionId, + externalEventId: "Ev0001", + semanticKey: `${TEAM}:app_uninstalled:1760000000.000100`, + }) + + await expect( + create({ + institutionId: OTHER_INST, + connectionId: null, + externalId: `${TEAM}-2`, + externalEventId: "Ev0001", + semanticKey: `${TEAM}-2:app_uninstalled:1760000000.000100`, + }), + ).rejects.toMatchObject({ code: "P2002" }) + }) +}) + +describe("a receipt with no tenant", () => { + it("is invisible to a tenant-scoped read, and present to an unscoped one", async () => { + await create({ + institutionId: null, + connectionId: null, + externalEventId: "Ev0009", + semanticKey: `${TEAM}:app_uninstalled:1760000009.000100`, + outcome: "NOT_PROCESSED", + note: "no connection matches the workspace", + connectionsMatched: 0, + }) + + const scoped = await runInTenantScope( + { institutionId: INST, actor: { principalId: "ose", principalType: "user" } }, + async () => db.webhookReceipt.findMany({ where: { externalId: TEAM } }), + ) + expect(scoped).toHaveLength(0) + + const all = await runUnscoped("control-plane", "assert", async () => + db.webhookReceipt.findMany({ where: { externalId: TEAM } }), + ) + expect(all).toHaveLength(1) + }) +}) + +describe("evidence outlives what it is about", () => { + it("keeps the receipt when the connection is deleted, and drops the pointer", async () => { + // A receipt says a verified delivery arrived. That stays true after the + // connection it was about is gone, which is why the foreign key is SET NULL + // and not CASCADE — the opposite choice would let removing a connection + // erase the record of everything it ever received. + const doomed = await runUnscoped("seed", "a connection to delete", async () => + db.connection.create({ + data: { institutionId: OTHER_INST, providerId: PROVIDER, externalId: `${TEAM}-doomed` }, + }), + ) + + await create({ + institutionId: OTHER_INST, + connectionId: doomed.id, + externalEventId: "Ev0010", + semanticKey: `${TEAM}:app_uninstalled:1760000010.000100`, + }) + + await runUnscoped("migration", "delete the connection", async () => { + await db.connection.delete({ where: { id: doomed.id } }) + }) + + const surviving = await runUnscoped("control-plane", "assert", async () => + db.webhookReceipt.findMany({ where: { externalEventId: "Ev0010" } }), + ) + expect(surviving).toHaveLength(1) + expect(surviving[0].connectionId).toBeNull() + expect(surviving[0].institutionId).toBe(OTHER_INST) + }) + + it("takes the subscription with the connection, because it is a live claim", async () => { + // The inverse of the receipt, and deliberately so: a subscription says this + // deployment EXPECTS deliveries for a connection. Once the connection is + // gone that claim is false, so it cascades rather than lingering. + const doomed = await runUnscoped("seed", "a connection to delete", async () => + db.connection.create({ + data: { institutionId: OTHER_INST, providerId: PROVIDER, externalId: `${TEAM}-sub` }, + }), + ) + await runUnscoped("seed", "its subscription", async () => + db.webhookSubscription.create({ + data: { + institutionId: OTHER_INST, + connectionId: doomed.id, + providerId: PROVIDER, + endpoint: "https://tenure.test/api/integrations/slack/events", + events: ["app_uninstalled", "tokens_revoked"], + }, + }), + ) + + await runUnscoped("migration", "delete the connection", async () => { + await db.connection.delete({ where: { id: doomed.id } }) + }) + + const left = await runUnscoped("control-plane", "assert", async () => + db.webhookSubscription.findMany({ where: { connectionId: doomed.id } }), + ) + expect(left).toHaveLength(0) + }) +}) From e900a908b8dd0e4b93a83f25853c134baaee62bd Mon Sep 17 00:00:00 2001 From: satvikOS Date: Thu, 20 Aug 2026 20:43:00 -0400 Subject: [PATCH 3/5] test(integrations): make "immutable receipt" a checked claim, not a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integration §12 asks for an immutable receipt, and "immutable" costs nothing to write in a schema comment. A static guard makes it true: no source file may update, upsert or delete a WebhookReceipt. The pressure to mutate one will arrive — a receipt says NOT_PROCESSED, somebody builds the thing that processes it, and the natural next line is `update({ outcome: "HANDLED" })`. That one line turns the table into a work queue with a status column, which is the second durable carrier this design refuses to become while the outbox is blocked, and it destroys the only thing a receipt is for: what this deployment decided at the moment a verified delivery arrived. The same file holds the inverse claim for WebhookSubscription, which is a live claim rather than evidence and therefore must be deleted — from exactly one place, so a settings page cannot drop the row while Slack is still delivering. Co-Authored-By: Claude Opus 5 (1M context) --- .../webhook-receipt-is-immutable.test.ts | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 apps/web/src/lib/__tests__/webhook-receipt-is-immutable.test.ts diff --git a/apps/web/src/lib/__tests__/webhook-receipt-is-immutable.test.ts b/apps/web/src/lib/__tests__/webhook-receipt-is-immutable.test.ts new file mode 100644 index 00000000..6aa99c07 --- /dev/null +++ b/apps/web/src/lib/__tests__/webhook-receipt-is-immutable.test.ts @@ -0,0 +1,121 @@ +import { readdirSync, readFileSync, statSync } from "node:fs" +import path from "node:path" + +/** + * A receipt is written once and never touched again. + * + * Integration §12 asks for a *minimised immutable* receipt, and "immutable" is + * a word that costs nothing to write in a schema comment. This is what makes it + * true: no source file may update, upsert or delete a `WebhookReceipt`, so the + * only shape a row can ever have is the one the delivery arrived with. + * + * ── Why it needs a static check ───────────────────────────────────────────── + * + * The pressure to mutate one is obvious and will arrive. A receipt says + * NOT_PROCESSED, somebody builds the thing that processes it, and the natural + * next line is `update({ outcome: "HANDLED" })`. That single line turns this + * table into a work queue with a status column — which is the second durable + * carrier the whole design refuses to become while the transactional outbox is + * blocked on its envelope conflict (Identity §21.2 vs Integration §9). + * + * It also quietly destroys the evidence. A receipt's value is that it records + * what was decided at the moment a verified delivery arrived; a row edited + * afterwards can no longer answer "what did this deployment do about it, and + * when", which is the only question it exists for. + * + * A future writer that genuinely needs one has to amend this test and say why — + * the same bargain `ledger-single-writer.test.ts` strikes with posted history. + * + * Same shape as that file, and for the same reason: the code this is about is a + * route handler holding Prisma and there is no harness in this app that can + * exercise every call site of a model. + */ + +const SRC = path.join(__dirname, "..", "..") + +function filesUnder(dir: string, match: (file: string) => boolean): string[] { + const out: string[] = [] + for (const entry of readdirSync(dir)) { + const full = path.join(dir, entry) + if (statSync(full).isDirectory()) out.push(...filesUnder(full, match)) + else if (match(entry)) out.push(full) + } + return out +} + +/** + * Product code only. + * + * `webhook-receipt.itest.ts` deletes rows to clean up after itself, and a test + * fixture tidying its own database is not the product editing history. + */ +const sourceFiles = () => + filesUnder(SRC, (file) => /\.tsx?$/.test(file) && !/\.test\.tsx?$|\.itest\.ts$/.test(file)) + +/** Every Prisma operation on the model, as `webhookReceipt.`. */ +function operationsOn(model: string): { file: string; operation: string }[] { + const found: { file: string; operation: string }[] = [] + for (const file of sourceFiles()) { + const source = readFileSync(file, "utf8") + for (const match of source.matchAll(new RegExp(`\\b${model}\\.(\\w+)\\s*\\(`, "g"))) { + found.push({ file: path.relative(SRC, file), operation: match[1] }) + } + } + return found +} + +/** Anything that changes or removes a row that already exists. */ +const MUTATING = new Set([ + "update", + "updateMany", + "updateManyAndReturn", + "upsert", + "delete", + "deleteMany", +]) + +describe("WebhookReceipt is append-only", () => { + const operations = operationsOn("webhookReceipt") + + it("finds the writes at all, so this is not vacuously passing", () => { + // The failure mode this guards: renaming the model, or moving the create + // behind a helper, would leave a scan that matches nothing and a test that + // passes for the wrong reason forever. + expect(operations.map((o) => o.operation)).toContain("create") + }) + + it("is never updated, upserted or deleted", () => { + const mutations = operations + .filter((o) => MUTATING.has(o.operation)) + .map((o) => `${o.file}: webhookReceipt.${o.operation}()`) + + expect(mutations).toEqual([]) + }) +}) + +describe("WebhookSubscription is deleted, and only on offboarding", () => { + const operations = operationsOn("webhookSubscription") + + it("finds them, so this is not vacuously passing", () => { + expect(operations.map((o) => o.operation)).toEqual( + expect.arrayContaining(["upsert", "deleteMany"]), + ) + }) + + it("is deleted from exactly one place — the inbound events endpoint", () => { + // Unlike a receipt, a subscription is a live claim rather than evidence: + // it says this deployment EXPECTS deliveries for a connection, so it must + // go when the app leaves the workspace. What must not happen is a second + // deletion path — a settings page or a cleanup job dropping the row while + // Slack is still delivering, which would leave the endpoint accepting + // events for a subscription nothing records. + const deleters = [ + ...new Set( + operations.filter((o) => o.operation.startsWith("delete")).map((o) => o.file), + ), + ] + expect(deleters).toEqual([ + path.join("app", "api", "integrations", "slack", "events", "route.ts"), + ]) + }) +}) From 91a56598b8365f6dac13d6f356b5b066bed01c47 Mon Sep 17 00:00:00 2001 From: satvikOS Date: Thu, 20 Aug 2026 20:44:12 -0400 Subject: [PATCH 4/5] docs(runbook): the Slack app configuration this endpoint needs, and how to rotate its key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An endpoint nothing points at receives nothing, and an unconfigured Request URL is indistinguishable from a workspace that never uninstalls anything — the same shape of silence that left the verifier with zero callers in the first place. This is the step that turns it on, plus the three-deploy signing-secret rotation the second key slot exists for. Co-Authored-By: Claude Opus 5 (1M context) --- docs/RUNBOOK.md | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/docs/RUNBOOK.md b/docs/RUNBOOK.md index 1a1982ca..68620b26 100644 --- a/docs/RUNBOOK.md +++ b/docs/RUNBOOK.md @@ -404,6 +404,49 @@ The gate adds no sign-in path of its own, so removing `AUTH_DEV_LOGIN` removes the provider it guards and there is nothing left behind. Also delete the seeded `@tenure.demo` users at that point — they are accounts with no credential. +## Slack inbound events + +The endpoint is `POST /api/integrations/slack/events`. It exists, it is +verified, and it receives nothing until somebody points Slack at it — an +unconfigured Request URL is indistinguishable from a workspace that never +uninstalls anything, so this is the step that turns the code on. + +In the Slack app configuration (api.slack.com/apps → the Tenure app): + +1. **Event Subscriptions → Request URL**: `https:///api/integrations/slack/events`. + Slack immediately POSTs a `url_verification` challenge; the endpoint answers it + inline and writes nothing, so a database problem cannot stop you saving the URL. + A refusal here is a signature problem, and the reason is in the container log + (`[slack] inbound event refused: …`) — never in the response, deliberately. +2. **Subscribe to bot events**: `app_uninstalled` and `tokens_revoked`, and nothing + else. Those two are the ones this deployment acts on; anything else subscribed + arrives, is recorded as `NOT_PROCESSED`, and does nothing — which reads as + coverage and is not. `apps/web/src/lib/integrations/slack/events.ts` explains + why channel archive and scope change are not on the list. + +Both events revoke the connection and delete its `WebhookSubscription` row, so a +workspace that removes the app stops being posted to without anyone doing +anything. `tokens_revoked` revokes only when a *bot* token is in the payload — a +member revoking their own access is not a disconnection. + +### Rotating the signing secret + +Slack switches the moment you press Regenerate, so the window between that and +every task carrying the new value is a window in which every delivery is +refused. Two slots exist so the window can be closed from either side: + +1. Set the repository secret `SLACK_SIGNING_SECRET_PREVIOUS` to the CURRENT + value and deploy. Nothing changes yet — the previous key is only tried when + the current one does not match. +2. Regenerate at Slack, set `SLACK_SIGNING_SECRET` to the new value, deploy. + Deliveries signed with either key now verify, in either deploy order and + through a rollback. +3. Confirm in the log that `[slack] inbound event verified with the PREVIOUS + signing key` has stopped appearing, then **clear** + `SLACK_SIGNING_SECRET_PREVIOUS` and deploy. Clearing it is what ends the old + key's life; left set, a retired key stays valid forever, which is the thing + rotation exists to stop. + ## Known pilot limitations - **Passwordless dev sign-in is on in production, behind an interim gate.** From d92ffdb4a896ef68d46e08ce5bbd7446af44f0f1 Mon Sep 17 00:00:00 2001 From: satvikOS Date: Thu, 20 Aug 2026 20:45:06 -0400 Subject: [PATCH 5/5] fix(integrations): take the revoked workspace id from the row, not the envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `externalId: delivery.teamId ?? ""` compiled only because of a fallback that cannot be reached — the loop runs solely when a workspace matched — but an empty string in an audit row is a silent lie, and the next edit that widens the branch would reach it. The connection was found by matching on its own `externalId`, so the row is the authority and no fallback is needed. The audit metadata is now asserted, including the revocation reason. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/integrations/slack/events/route.test.ts | 10 +++++++++- .../src/app/api/integrations/slack/events/route.ts | 14 ++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/apps/web/src/app/api/integrations/slack/events/route.test.ts b/apps/web/src/app/api/integrations/slack/events/route.test.ts index 12556191..9eb66c3d 100644 --- a/apps/web/src/app/api/integrations/slack/events/route.test.ts +++ b/apps/web/src/app/api/integrations/slack/events/route.test.ts @@ -50,7 +50,8 @@ const NOW = Math.floor(NOW_MS / 1000) const CONNECTION = { id: "conn_1", institutionId: "inst_1", - externalName: "Simon Student Life", + externalId: "T0001", + externalName: "Student Life Workspace", status: "ACTIVE" as const, } @@ -271,6 +272,13 @@ describe("app_uninstalled", () => { // would put a person's name on an act they did not perform. actorId: null, institutionId: "inst_1", + metadata: expect.objectContaining({ + externalId: "T0001", + // Why, not just that. A revocation with no explanation reads + // identically whether the workspace uninstalled the app or somebody + // here made a mistake. + reason: "app-uninstalled", + }), }), }), ) diff --git a/apps/web/src/app/api/integrations/slack/events/route.ts b/apps/web/src/app/api/integrations/slack/events/route.ts index 9f27456e..35ac38e1 100644 --- a/apps/web/src/app/api/integrations/slack/events/route.ts +++ b/apps/web/src/app/api/integrations/slack/events/route.ts @@ -236,7 +236,13 @@ async function recordAndHandle( const connections = delivery.teamId ? await tx.connection.findMany({ where: { providerId: SLACK_WORKSPACE_PRODUCT_ID, externalId: delivery.teamId }, - select: { id: true, institutionId: true, externalName: true, status: true }, + select: { + id: true, + institutionId: true, + externalId: true, + externalName: true, + status: true, + }, }) : [] @@ -288,7 +294,11 @@ async function recordAndHandle( action: CONNECTION_REVOKED_ACTION, connectionId: connection.id, providerId: SLACK_WORKSPACE_PRODUCT_ID, - externalId: delivery.teamId ?? "", + // The row's own workspace id, not the envelope's. They are equal — + // the row was found by matching on it — and taking it from the row + // means the audit entry needs no fallback for a `teamId` the types + // still think can be null here. + externalId: connection.externalId, externalName: connection.externalName, reason: plan.reason, })