diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index d14e0ed5..040a15b1 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -325,6 +325,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 d179fc1e..19157c17 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[] exceptions Exception[] seatMeterEvents SeatMeterEvent[] registrySeal RestrictedRegistrySeal? @@ -462,10 +464,10 @@ model ApprovalRequest { createdAt DateTime @default(now()) updatedAt DateTime @updatedAt - steps ApprovalStep[] - conversation Conversation? @relation("ApprovalConversation") - event Event? @relation("ApprovalEvent") - ledgerEntries LedgerEntry[] + steps ApprovalStep[] + conversation Conversation? @relation("ApprovalConversation") + event Event? @relation("ApprovalEvent") + ledgerEntries LedgerEntry[] // Exceptions whose waiver this request authorised. A back-relation only — // nothing about this table changes, which is the whole point of ADR-0015. waivedExceptions Exception[] @@ -1202,6 +1204,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]) @@ -1388,7 +1394,7 @@ model Exception { status ExceptionStatus @default(OPEN) /// One line, for the worklist row. - title String + title String /// The pair that makes this an exception rather than an error: what was /// supposed to happen, and what happened instead. An operator who reads only /// these two sentences knows what is wrong. @@ -1445,7 +1451,7 @@ model Exception { /// A waiver's end. Mandatory when the status is WAIVED — a waiver with no /// expiry is a policy change wearing an exception's clothes. - expiresAt DateTime? + expiresAt DateTime? /// The approval that authorised the waiver, where one exists. /// /// NULLABLE and, today, unusable for an exception with no club: creating an @@ -1712,3 +1718,177 @@ model RestrictedRegistrySeal { institution Institution @relation(fields: [institutionId], references: [id], onDelete: Cascade) } + +// ─── 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. + /// + /// 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 + /// 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 eeab223d..2bd86a06 100644 --- a/apps/web/src/app/api/integrations/slack/callback/route.ts +++ b/apps/web/src/app/api/integrations/slack/callback/route.ts @@ -7,6 +7,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 { classifySlackInstallFailure } from "@/lib/integrations/slack/install-exceptions" import { exchangeCodeForToken } from "@/lib/integrations/slack/oauth" @@ -35,9 +39,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. * * ── Where a failure goes ──────────────────────────────────────────────────── * @@ -173,6 +178,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..9eb66c3d --- /dev/null +++ b/apps/web/src/app/api/integrations/slack/events/route.test.ts @@ -0,0 +1,456 @@ +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", + externalId: "T0001", + externalName: "Student Life Workspace", + 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", + 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", + }), + }), + }), + ) + }) + + 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..35ac38e1 --- /dev/null +++ b/apps/web/src/app/api/integrations/slack/events/route.ts @@ -0,0 +1,341 @@ +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, + externalId: 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, + // 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, + }) + } + + 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/__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"), + ]) + }) +}) 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/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) + }) +}) diff --git a/apps/web/src/lib/tenancy/registry.test.ts b/apps/web/src/lib/tenancy/registry.test.ts index 4d3a76a5..88500016 100644 --- a/apps/web/src/lib/tenancy/registry.test.ts +++ b/apps/web/src/lib/tenancy/registry.test.ts @@ -147,7 +147,7 @@ describe("the registry matches prisma/schema.prisma", () => { // are both properties of being scoped. Where Tenure's money lives instead // is ADR-0017. // - // 2026-08-21 (this merge): 24 → 25 tenant-scoped, 43 → 44 models. + // 2026-08-21 (landed on main): 24 → 25 tenant-scoped, 43 → 44 models. // `Exception` is the platform exception register — one object for what four // domains were each about to build separately. It is scoped rather than // global because all four subclasses raise inside a tenant: a control test @@ -159,16 +159,35 @@ describe("the registry matches prisma/schema.prisma", () => { // club — which is why it is a new table rather than an `ApprovalRequest`; // see ADR-0015. // + // 2026-08-21 (this merge): 25 → 27 tenant-scoped, 44 → 46 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. + // // These four numbers were MEASURED against `schema.prisma` at merge time, - // not incremented from either side. This branch was written against 41/22 - // and main had moved to 43/24; incrementing either would have been wrong by - // two, and the four assertions auto-merge silently from one side, so - // nothing but measuring would have caught it. The buckets are asserted to - // sum to the model count immediately below for the same reason. - expect(TENANT_SCOPED).toHaveLength(25) + // not incremented from either side. `grep -c '^model ' prisma/schema.prisma` + // gives 46; the models whose body declares an `institutionId` field number + // 27. This branch was written against 43/24 and main had moved to 44/25; + // taking either side's four assertions would have been wrong by two or by + // three, and they auto-merge silently from one side, so nothing but + // measuring would have caught it. The buckets are asserted to sum to the + // model count immediately below for the same reason. + expect(TENANT_SCOPED).toHaveLength(27) expect(PLATFORM_GLOBAL).toHaveLength(5) expect(Object.keys(UNENFORCEABLE)).toHaveLength(14) - expect(schemaModels).toHaveLength(44) + expect(schemaModels).toHaveLength(46) }) it("registry.ts's own prose agrees with these pins", () => { diff --git a/apps/web/src/lib/tenancy/registry.ts b/apps/web/src/lib/tenancy/registry.ts index 50222c9f..301021c9 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 25 of 44 models + * The three buckets are honest about a real limitation: only 27 of 46 models * carry `institutionId`, so only those can be filtered by the query layer * today. The rest are named in UNENFORCEABLE rather than quietly omitted. * @@ -32,11 +32,16 @@ * schema — so the suite stayed green while the sentence a reader actually * consults was wrong about the tenancy boundary. * - * 2026-08-21: 24 of 43 → 25 of 44 on merging `main` into the exception - * register. MEASURED against `schema.prisma`, not incremented — this branch was - * written against 23 of 42 and main had already moved twice underneath it, so - * either side's number carried forward alone would have been wrong. `Exception` - * is the model added, and it carries `institutionId`. + * 2026-08-21: 25 of 44 → 27 of 46 on merging `main` into the inbound webhook + * endpoint. MEASURED against `schema.prisma` — `grep -c '^model '` gives 46, and + * counting the models whose body declares an `institutionId` field gives 27 — + * not incremented from either side. Both sides of this merge added models and + * both numbers were therefore stale: main was at 25 of 44 without the two + * webhook models, this branch was at 24 of 43 without `Exception`, + * `RestrictedRegistrySeal` and `SeatMeterEvent`. Taking either side alone would + * have been wrong by two, which is exactly how this sentence went wrong before. + * `WebhookSubscription` and `WebhookReceipt` are the models added here; both + * carry `institutionId`, nullably in `WebhookReceipt`'s case (see its entry). */ /** @@ -55,6 +60,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", // Its institutionId is its own, with a real foreign key to Institution — the // club half is the optional one. Every subclass of the exception register // raises inside a tenant, so there is no such thing as a platform-wide row diff --git a/docs/RUNBOOK.md b/docs/RUNBOOK.md index 6ed9b6b6..c8cb38be 100644 --- a/docs/RUNBOOK.md +++ b/docs/RUNBOOK.md @@ -460,6 +460,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.** diff --git a/docs/implementation/global-engine-execution-ledger.md b/docs/implementation/global-engine-execution-ledger.md index e820c543..169e1dc0 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,13 +121,14 @@ 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 - 25 TENANT_SCOPED models — the roster itself (seats, assignments, seat + 27 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, and `RestrictedRegistrySeal`, - `SeatMeterEvent` and `Exception` joined them as the access seal, the seat - meter and the platform exception register. It is not proven per-path for API + `SeatMeterEvent`, `Exception`, `WebhookSubscription` and `WebhookReceipt` + joined them as the access seal, the seat meter, the platform exception + register and the inbound-webhook pair. It is not proven per-path for API routes, background jobs, exports, search or AI answers, and - 14 of 44 models are UNENFORCEABLE at the chokepoint. + 14 of 46 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 e94b601b..fa27b4f5 100644 --- a/infrastructure/terraform/ecs.tf +++ b/infrastructure/terraform/ecs.tf @@ -333,6 +333,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::" + }, { # HMAC key for the opaque r+@reply. addresses # (ses-inbound.tf). Without it the application cannot verify a reply 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 }) }