From edd6aaa3b760bb149534bf367f80bb984363d92e Mon Sep 17 00:00:00 2001 From: Muhammad Ahmed Cheema Date: Mon, 27 Apr 2026 23:38:37 -0700 Subject: [PATCH 1/3] channels: mount Slack HTTP receiver on shared Bun.serve The HTTP-mode Slack channel was binding ExpressReceiver to the same port as Phantom's main Bun.serve listener. The bind silently failed, the router caught the rejection, and /slack/* never came up; live canary traffic verified `POST /slack/events` returned 404 and `/health.channels.slack` was false. Refactor SlackHttpChannel into a Bolt App driven by a tiny BunReceiver whose start/stop are no-ops. The three Slack ingress paths now mount on the existing Bun.serve via slack-http-routes.ts; the channel exposes handleEvent, handleInteractivity, and handleCommand, each running the existing slack-gateway-verifier guard before dispatching the parsed body into app.processEvent. The verifier file is unchanged. Slash commands (urlencoded with top-level team_id) are picked up by a small extractor in the dispatcher so the third Slack endpoint can satisfy team-id binding without touching the verifier. Tests cover the verifier guard happy and failure paths via real Request objects, plus a hermetic Bun.serve test that proves the routes are alive end-to-end. --- .../__tests__/slack-channel-factory.test.ts | 8 - .../__tests__/slack-http-receiver.test.ts | 427 ++++++++++-------- src/channels/slack-channel-factory.ts | 3 - src/channels/slack-http-handlers.ts | 276 +++++++++++ src/channels/slack-http-receiver.ts | 162 +++---- src/channels/slack-http-routes.ts | 57 +++ .../__tests__/server-slack-routes.test.ts | 135 ++++++ src/core/server.ts | 9 + src/index.ts | 15 +- 9 files changed, 794 insertions(+), 298 deletions(-) create mode 100644 src/channels/slack-http-handlers.ts create mode 100644 src/channels/slack-http-routes.ts create mode 100644 src/core/__tests__/server-slack-routes.test.ts diff --git a/src/channels/__tests__/slack-channel-factory.test.ts b/src/channels/__tests__/slack-channel-factory.test.ts index 7797ac4..91a9e4d 100644 --- a/src/channels/__tests__/slack-channel-factory.test.ts +++ b/src/channels/__tests__/slack-channel-factory.test.ts @@ -125,7 +125,6 @@ describe("createSlackChannel", () => { const ch = await createSlackChannel({ transport: "socket", channelsConfig: null, - port: 3100, }); expect(ch).toBeNull(); }); @@ -137,7 +136,6 @@ describe("createSlackChannel", () => { const ch = await createSlackChannel({ transport: "socket", channelsConfig: disabled, - port: 3100, }); expect(ch).toBeNull(); }); @@ -146,7 +144,6 @@ describe("createSlackChannel", () => { const ch = await createSlackChannel({ transport: "socket", channelsConfig: SOCKET_CONFIG, - port: 3100, }); expect(ch).toBeInstanceOf(SlackChannel); }); @@ -158,7 +155,6 @@ describe("createSlackChannel", () => { createSlackChannel({ transport: "http", channelsConfig: null, - port: 3100, identityFetcher: idFetcher, secretsFetcher: secFetcher, }), @@ -171,7 +167,6 @@ describe("createSlackChannel", () => { const ch = await createSlackChannel({ transport: "http", channelsConfig: null, - port: 3100, identityFetcher: idFetcher, secretsFetcher: secFetcher, }); @@ -190,7 +185,6 @@ describe("createSlackChannel", () => { await createSlackChannel({ transport: "http", channelsConfig: null, - port: 3100, identityFetcher: idFetcher, secretsFetcher: secFetcher, }); @@ -211,7 +205,6 @@ describe("createSlackChannel", () => { const ch = await createSlackChannel({ transport: "http", channelsConfig: SOCKET_CONFIG, - port: 3100, identityFetcher: idFetcher, secretsFetcher: secFetcher, }); @@ -232,7 +225,6 @@ describe("createSlackChannel", () => { const ch = await createSlackChannel({ transport: "http", channelsConfig: null, - port: 3100, metadataBaseUrl: "http://gateway.test", identityFetcher: idFetcher, secretsFetcher: secFetcher, diff --git a/src/channels/__tests__/slack-http-receiver.test.ts b/src/channels/__tests__/slack-http-receiver.test.ts index 5d0568b..40589e9 100644 --- a/src/channels/__tests__/slack-http-receiver.test.ts +++ b/src/channels/__tests__/slack-http-receiver.test.ts @@ -1,13 +1,14 @@ import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; import { signGatewayRequest } from "../slack-gateway-verifier.ts"; -// Mock Slack Bolt's App and ExpressReceiver. The receiver shape we replace -// captures the registered Bolt handlers so the test driver can invoke them -// directly, mirroring the slack.test.ts pattern. The Express app sits on -// the receiver and exposes `use(path, mw)` so the channel's installVerifier -// can register guards we then drive end-to-end. -const mockStart = mock(() => Promise.resolve({})); -const mockStop = mock(() => Promise.resolve()); +// Mock Slack Bolt's App. The receiver no longer instantiates +// ExpressReceiver (the previous implementation tried to bind a second +// HTTP server and collided with the shared Bun.serve), so the test +// surface drives the new public methods on `SlackHttpChannel` directly: +// handleEvent, handleInteractivity, handleCommand. The mock captures +// registered Bolt event/action handlers AND records every processEvent +// invocation so tests can assert exactly what got dispatched into +// Bolt's pipeline. const mockAuthTest = mock(() => Promise.resolve({ user_id: "U_BOT123" })); const mockPostMessage = mock(() => Promise.resolve({ ts: "1234567890.123456" })); const mockChatUpdate = mock(() => Promise.resolve({ ok: true })); @@ -16,50 +17,45 @@ const mockReactionsRemove = mock(() => Promise.resolve({ ok: true })); const mockConversationsOpen = mock(() => Promise.resolve({ channel: { id: "D_DM_OPEN" } })); type EventHandler = (...args: unknown[]) => Promise; -type Middleware = (req: unknown, res: unknown, next: () => void) => unknown | Promise; +type ProcessEventCall = { body: Record; ack: (resp?: unknown) => Promise }; const eventHandlers = new Map(); const actionHandlers = new Map(); -const guards = new Map(); +const processEventCalls: ProcessEventCall[] = []; -const expressApp = { - use(path: string, mw: Middleware): void { - guards.set(path, mw); - }, -}; - -const mockReceiver = { - app: expressApp, - start: mockStart, - stop: mockStop, -}; +let initReceived: { receiver: { init?: (app: unknown) => void } | null } = { receiver: null }; -const MockExpressReceiver = mock(() => mockReceiver); - -const MockApp = mock(() => ({ - event: (name: string, handler: EventHandler) => { - eventHandlers.set(name, handler); - }, - action: (pattern: string | RegExp, handler: EventHandler) => { - const key = pattern instanceof RegExp ? pattern.source : pattern; - actionHandlers.set(key, handler); - }, - client: { - auth: { test: mockAuthTest }, - chat: { postMessage: mockPostMessage, update: mockChatUpdate }, - conversations: { open: mockConversationsOpen }, - reactions: { add: mockReactionsAdd, remove: mockReactionsRemove }, - }, -})); +const MockApp = mock((opts: { receiver?: { init?: (app: unknown) => void } }) => { + const app = { + event: (name: string, handler: EventHandler) => { + eventHandlers.set(name, handler); + }, + action: (pattern: string | RegExp, handler: EventHandler) => { + const key = pattern instanceof RegExp ? pattern.source : pattern; + actionHandlers.set(key, handler); + }, + processEvent: async (event: ProcessEventCall) => { + processEventCalls.push(event); + await event.ack(); + }, + client: { + auth: { test: mockAuthTest }, + chat: { postMessage: mockPostMessage, update: mockChatUpdate }, + conversations: { open: mockConversationsOpen }, + reactions: { add: mockReactionsAdd, remove: mockReactionsRemove }, + }, + }; + if (opts.receiver?.init) opts.receiver.init(app); + initReceived.receiver = opts.receiver ?? null; + return app; +}); mock.module("@slack/bolt", () => ({ App: MockApp, - ExpressReceiver: MockExpressReceiver, })); // Import the channel AFTER the module mock so the constructor uses our doubles. const { SlackHttpChannel } = await import("../slack-http-receiver.ts"); -type SlackHttpChannelType = InstanceType; const SECRET = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; const TEAM_ID = "T9TK3CUKW"; @@ -71,40 +67,9 @@ const baseConfig = { teamId: TEAM_ID, installerUserId: "U_INSTALLER", teamName: "Acme Corp", - listenPort: 3100, - listenPath: "/slack", -}; - -type MockReq = { - headers: Record; - rawBody?: Buffer; - body?: unknown; - on?: (event: string, listener: (chunk?: Buffer) => void) => void; -}; - -type MockRes = { - statusCode: number; - body: string | undefined; - status(code: number): MockRes; - end(body?: string): void; }; -function makeRes(): MockRes { - const res: MockRes = { - statusCode: 0, - body: undefined, - status(code) { - res.statusCode = code; - return res; - }, - end(body) { - res.body = body; - }, - }; - return res; -} - -function makeReq(args: { +type MakeReqArgs = { bodyJson: unknown; contentType?: string; now?: number; @@ -115,7 +80,15 @@ function makeReq(args: { wrongSecret?: boolean; staleMs?: number; futureMs?: number; -}): MockReq { + method?: string; + path?: string; +}; + +// Build a real `Request` (Bun-native, fetch-spec) signed with the gateway +// helper so the verifier path runs end-to-end against the same code that +// runs in production. Tampering options exist on every axis the verifier +// guards (signature, timestamp, secret, team_id, body bytes). +function makeReq(args: MakeReqArgs): Request { const ctype = args.contentType ?? "application/json"; const bodyText = typeof args.bodyJson === "string" @@ -123,53 +96,34 @@ function makeReq(args: { : ctype.includes("urlencoded") ? `payload=${encodeURIComponent(JSON.stringify(args.bodyJson))}` : JSON.stringify(args.bodyJson); - const raw = Buffer.from(args.tamperedBody ?? bodyText); + const rawForSig = Buffer.from(bodyText); + const sentBody = args.tamperedBody ?? bodyText; const now = args.now ?? Date.now(); const fwd = args.staleMs !== undefined ? now - args.staleMs : args.futureMs !== undefined ? now + args.futureMs : now; const eventId = args.eventId ?? "Ev1"; const sig = signGatewayRequest({ forwardedAt: fwd, eventId, - rawBody: Buffer.from(bodyText), + rawBody: rawForSig, secret: args.wrongSecret ? "wrong-secret" : SECRET, }); const headers: Record = { "content-type": ctype }; if (!args.skipSig) headers["x-phantom-signature"] = sig; if (!args.skipFwd) headers["x-phantom-forwarded-at"] = String(fwd); headers["x-phantom-slack-event-id"] = eventId; - const req: MockReq = { headers, rawBody: raw }; - // Provide req.on for the readRequestBody fallback path; not used when - // rawBody is preset, but keeps the type contract honest for any guard - // that re-reads the body. - req.on = () => {}; - return req; -} - -async function callGuard(channel: SlackHttpChannelType, path: string, req: MockReq): Promise { - void channel; - const guard = guards.get(path); - if (!guard) throw new Error(`no guard at ${path}`); - const res = makeRes(); - await new Promise((resolve) => { - const next = () => resolve(); - const out = guard(req, res, next); - if (out instanceof Promise) { - out.then(() => { - if (res.statusCode !== 0) resolve(); - }); - } else if (res.statusCode !== 0) { - resolve(); - } + const url = `http://localhost:3100${args.path ?? "/slack/events"}`; + return new Request(url, { + method: args.method ?? "POST", + headers, + body: sentBody, }); - return res; } beforeEach(() => { eventHandlers.clear(); actionHandlers.clear(); - guards.clear(); - mockStart.mockClear(); - mockStop.mockClear(); + processEventCalls.length = 0; + initReceived = { receiver: null }; mockAuthTest.mockClear(); mockPostMessage.mockClear(); mockChatUpdate.mockClear(); @@ -187,22 +141,17 @@ afterEach(() => { // ----- constructor --------------------------------------------------------- describe("SlackHttpChannel constructor", () => { - test("wires receiver and Bolt App with the provided config", () => { + test("wires Bolt App with the provided config and a custom Receiver", () => { const channel = new SlackHttpChannel(baseConfig); expect(channel.id).toBe("slack"); expect(channel.name).toBe("Slack"); expect(channel.getTeamId()).toBe(TEAM_ID); expect(channel.getInstallerUserId()).toBe("U_INSTALLER"); expect(channel.getTeamName()).toBe("Acme Corp"); - expect(MockExpressReceiver).toHaveBeenCalled(); expect(MockApp).toHaveBeenCalled(); - }); - - test("registers verifier guards on the three Slack endpoints", () => { - new SlackHttpChannel(baseConfig); - expect(guards.has("/slack/events")).toBe(true); - expect(guards.has("/slack/interactivity")).toBe(true); - expect(guards.has("/slack/commands")).toBe(true); + // Bolt called receiver.init synchronously during construction. + expect(initReceived.receiver).not.toBeNull(); + expect(typeof initReceived.receiver?.init).toBe("function"); }); test("rejects empty botToken with a clear error", () => { @@ -241,94 +190,149 @@ describe("SlackHttpChannel constructor", () => { }); }); -// ----- guard middleware ---------------------------------------------------- +// ----- handler entrypoints (HMAC + replay window + team_id + dispatch) ---- -describe("guard middleware (HMAC + replay window + team_id)", () => { - test("accepts a correctly-signed request and calls next()", async () => { +describe("handleEvent / handleInteractivity / handleCommand (verifier guard + dispatch)", () => { + test("accepts a correctly-signed event and dispatches into Bolt's processEvent", async () => { const channel = new SlackHttpChannel(baseConfig); - const req = makeReq({ bodyJson: { type: "event_callback", team_id: TEAM_ID } }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(0); + const req = makeReq({ bodyJson: { type: "event_callback", team_id: TEAM_ID, event: { type: "app_mention" } } }); + const res = await channel.handleEvent(req); + expect(res.status).toBe(200); + expect(processEventCalls).toHaveLength(1); + expect(processEventCalls[0]?.body.team_id).toBe(TEAM_ID); }); test("rejects 401 when X-Phantom-Signature is missing", async () => { const channel = new SlackHttpChannel(baseConfig); const req = makeReq({ bodyJson: { team_id: TEAM_ID }, skipSig: true }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(401); + const res = await channel.handleEvent(req); + expect(res.status).toBe(401); + expect(processEventCalls).toHaveLength(0); }); test("rejects 401 when X-Phantom-Forwarded-At is missing", async () => { const channel = new SlackHttpChannel(baseConfig); const req = makeReq({ bodyJson: { team_id: TEAM_ID }, skipFwd: true }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(401); + const res = await channel.handleEvent(req); + expect(res.status).toBe(401); }); test("rejects 401 on a stale forwarded-at (>5 minutes old)", async () => { const channel = new SlackHttpChannel(baseConfig); - const req = makeReq({ - bodyJson: { team_id: TEAM_ID }, - staleMs: 6 * 60 * 1000, - }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(401); + const req = makeReq({ bodyJson: { team_id: TEAM_ID }, staleMs: 6 * 60 * 1000 }); + const res = await channel.handleEvent(req); + expect(res.status).toBe(401); }); test("rejects 401 on a future forwarded-at (>5 minutes ahead)", async () => { const channel = new SlackHttpChannel(baseConfig); - const req = makeReq({ - bodyJson: { team_id: TEAM_ID }, - futureMs: 6 * 60 * 1000, - }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(401); + const req = makeReq({ bodyJson: { team_id: TEAM_ID }, futureMs: 6 * 60 * 1000 }); + const res = await channel.handleEvent(req); + expect(res.status).toBe(401); }); - test("rejects 401 on tampered body (signature over original bytes)", async () => { + test("rejects 401 on tampered body (signature was over original bytes)", async () => { const channel = new SlackHttpChannel(baseConfig); const req = makeReq({ bodyJson: { team_id: TEAM_ID }, tamperedBody: '{"team_id":"T_OTHER"}', }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(401); + const res = await channel.handleEvent(req); + expect(res.status).toBe(401); }); test("rejects 401 when signature was computed with a different secret", async () => { const channel = new SlackHttpChannel(baseConfig); const req = makeReq({ bodyJson: { team_id: TEAM_ID }, wrongSecret: true }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(401); + const res = await channel.handleEvent(req); + expect(res.status).toBe(401); }); test("rejects 403 on team_id mismatch even with valid HMAC", async () => { const channel = new SlackHttpChannel(baseConfig); const req = makeReq({ bodyJson: { team_id: FOREIGN_TEAM_ID } }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(403); + const res = await channel.handleEvent(req); + expect(res.status).toBe(403); + expect(processEventCalls).toHaveLength(0); }); - test("accepts a url_verification challenge body (the one shape with no team_id)", async () => { + test("returns 200 with the challenge string for a url_verification body", async () => { const channel = new SlackHttpChannel(baseConfig); - const req = makeReq({ bodyJson: { type: "url_verification", challenge: "abc" } }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(0); + const req = makeReq({ bodyJson: { type: "url_verification", challenge: "abc-challenge-123" } }); + const res = await channel.handleEvent(req); + expect(res.status).toBe(200); + expect(await res.text()).toBe("abc-challenge-123"); + // url_verification short-circuits before Bolt; the App pipeline is + // not invoked. + expect(processEventCalls).toHaveLength(0); }); test("rejects 403 when body has no team_id and is not url_verification", async () => { const channel = new SlackHttpChannel(baseConfig); const req = makeReq({ bodyJson: { type: "event_callback", event: { type: "app_mention" } } }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(403); - }); - - test("rejects 403 when body is malformed JSON (defense in depth)", async () => { - const channel = new SlackHttpChannel(baseConfig); - // Bypass JSON.stringify by passing a pre-built malformed string. - const req = makeReq({ bodyJson: "{not-json" }); - const res = await callGuard(channel, "/slack/events", req); - expect(res.statusCode).toBe(403); + const res = await channel.handleEvent(req); + expect(res.status).toBe(403); + }); + + test("returns 400 when JSON body is malformed (defense in depth)", async () => { + const channel = new SlackHttpChannel(baseConfig); + // Sign over the same malformed bytes the request carries so the HMAC + // guard passes; the parse step is what fails. + const malformed = "{not-json"; + const fwd = Date.now(); + const sig = signGatewayRequest({ + forwardedAt: fwd, + eventId: "Ev1", + rawBody: Buffer.from(malformed), + secret: SECRET, + }); + const req = new Request("http://localhost:3100/slack/events", { + method: "POST", + headers: { + "content-type": "application/json", + "x-phantom-signature": sig, + "x-phantom-forwarded-at": String(fwd), + "x-phantom-slack-event-id": "Ev1", + }, + body: malformed, + }); + const res = await channel.handleEvent(req); + // Malformed JSON fails team_id extraction first (no team_id, no + // url_verification shape), so we 403 before the parse step. The + // regression we guard against is "this returned 200 and + // dispatched", which the !ok / processEventCalls check pins. + expect(res.status).toBe(403); + expect(processEventCalls).toHaveLength(0); + }); + + test("returns 403 on a corrupted interactivity payload (no team_id reachable)", async () => { + const channel = new SlackHttpChannel(baseConfig); + const malformedPayload = "payload=%7Bnot-json"; + const fwd = Date.now(); + const sig = signGatewayRequest({ + forwardedAt: fwd, + eventId: "EvCorrupt", + rawBody: Buffer.from(malformedPayload), + secret: SECRET, + }); + const req = new Request("http://localhost:3100/slack/interactivity", { + method: "POST", + headers: { + "content-type": "application/x-www-form-urlencoded", + "x-phantom-signature": sig, + "x-phantom-forwarded-at": String(fwd), + "x-phantom-slack-event-id": "EvCorrupt", + }, + body: malformedPayload, + }); + const res = await channel.handleInteractivity(req); + // urlencoded body with a malformed `payload` field trips the + // team_id guard (extractTeamId returns undefined and the body is + // not url_verification), so we 403 fail-closed without ever + // invoking processEvent. The regression we pin: a corrupted + // forward must NOT silently dispatch. + expect(res.status).toBe(403); + expect(processEventCalls).toHaveLength(0); }); test("accepts an interactivity payload (urlencoded form) when team.id matches", async () => { @@ -336,26 +340,88 @@ describe("guard middleware (HMAC + replay window + team_id)", () => { const req = makeReq({ bodyJson: { type: "block_actions", team: { id: TEAM_ID, domain: "acme" } }, contentType: "application/x-www-form-urlencoded", + path: "/slack/interactivity", }); - const res = await callGuard(channel, "/slack/interactivity", req); - expect(res.statusCode).toBe(0); + const res = await channel.handleInteractivity(req); + expect(res.status).toBe(200); + expect(processEventCalls).toHaveLength(1); + // urlencoded interactivity bodies are unwrapped: the inner JSON is + // what Bolt sees, not the {payload: "..."} envelope. + const dispatched = processEventCalls[0]?.body as Record; + expect(dispatched.type).toBe("block_actions"); + const team = dispatched.team as Record; + expect(team.id).toBe(TEAM_ID); }); - test("rejects 403 on interactivity payload with foreign team.id", async () => { + test("rejects 403 on interactivity payload with a foreign team.id", async () => { const channel = new SlackHttpChannel(baseConfig); const req = makeReq({ bodyJson: { type: "block_actions", team: { id: FOREIGN_TEAM_ID, domain: "evil" } }, contentType: "application/x-www-form-urlencoded", + path: "/slack/interactivity", + }); + const res = await channel.handleInteractivity(req); + expect(res.status).toBe(403); + }); + + test("handleCommand accepts a slash-command form and dispatches", async () => { + const channel = new SlackHttpChannel(baseConfig); + // Slack slash commands send urlencoded fields including team_id at + // the top level (no `payload` envelope). + const formBody = `team_id=${TEAM_ID}&command=%2Fphantom&text=hello`; + const fwd = Date.now(); + const sig = signGatewayRequest({ + forwardedAt: fwd, + eventId: "EvCmd1", + rawBody: Buffer.from(formBody), + secret: SECRET, + }); + const req = new Request("http://localhost:3100/slack/commands", { + method: "POST", + headers: { + "content-type": "application/x-www-form-urlencoded", + "x-phantom-signature": sig, + "x-phantom-forwarded-at": String(fwd), + "x-phantom-slack-event-id": "EvCmd1", + }, + body: formBody, + }); + const res = await channel.handleCommand(req); + expect(res.status).toBe(200); + expect(processEventCalls).toHaveLength(1); + const body = processEventCalls[0]?.body as Record; + expect(body.team_id).toBe(TEAM_ID); + expect(body.command).toBe("/phantom"); + }); + + test("forwards retry headers to Bolt's ReceiverEvent", async () => { + const channel = new SlackHttpChannel(baseConfig); + const ctype = "application/json"; + const bodyText = JSON.stringify({ team_id: TEAM_ID, event: { type: "app_mention" } }); + const fwd = Date.now(); + const sig = signGatewayRequest({ + forwardedAt: fwd, + eventId: "EvRetry1", + rawBody: Buffer.from(bodyText), + secret: SECRET, + }); + const req = new Request("http://localhost:3100/slack/events", { + method: "POST", + headers: { + "content-type": ctype, + "x-phantom-signature": sig, + "x-phantom-forwarded-at": String(fwd), + "x-phantom-slack-event-id": "EvRetry1", + "x-slack-retry-num": "2", + "x-slack-retry-reason": "http_timeout", + }, + body: bodyText, }); - const res = await callGuard(channel, "/slack/interactivity", req); - expect(res.statusCode).toBe(403); - }); - - test("rehydrates JSON body for downstream Bolt parsers", async () => { - const channel = new SlackHttpChannel(baseConfig); - const req = makeReq({ bodyJson: { team_id: TEAM_ID, foo: "bar" } }); - await callGuard(channel, "/slack/events", req); - expect(req.body).toEqual({ team_id: TEAM_ID, foo: "bar" }); + await channel.handleEvent(req); + expect(processEventCalls).toHaveLength(1); + const dispatched = processEventCalls[0] as unknown as Record; + expect(dispatched.retryNum).toBe(2); + expect(dispatched.retryReason).toBe("http_timeout"); }); }); @@ -550,12 +616,18 @@ describe("lifecycle and bot user discovery", () => { expect(mockAuthTest).toHaveBeenCalledTimes(1); }); - test("connect transitions to connected state", async () => { + test("connect transitions to connected state without binding an HTTP server", async () => { const channel = new SlackHttpChannel(baseConfig); await channel.connect(); expect(channel.isConnected()).toBe(true); expect(channel.getConnectionState()).toBe("connected"); - expect(mockStart).toHaveBeenCalledTimes(1); + // The new receiver does NOT call any `start(port)` ever; that's the + // whole point of Bug A's fix. We pin this behaviour by asserting + // the receiver Bolt got is the BunReceiver shape (init/start/stop) + // and that no port-binding side effect happened. There is no global + // listener to inspect; the fact that the test process can run + // concurrently with itself is itself the regression guard. + expect(typeof initReceived.receiver?.init).toBe("function"); }); test("disconnect transitions back to disconnected", async () => { @@ -564,7 +636,6 @@ describe("lifecycle and bot user discovery", () => { await channel.disconnect(); expect(channel.isConnected()).toBe(false); expect(channel.getConnectionState()).toBe("disconnected"); - expect(mockStop).toHaveBeenCalledTimes(1); }); test("connect refuses to start when auth.test returns no user_id (revoked token)", async () => { @@ -593,10 +664,6 @@ describe("lifecycle and bot user discovery", () => { }); test("auth.test failure with a hostile token-bearing message redacts the token", async () => { - // Pin the redaction contract: even if a future Bolt SDK upgrade includes - // the offending token in `err.message`, the receiver must strip it - // before logging. The message text ("invalid_auth") still surfaces so - // operators can debug, but the token is replaced with a sentinel. mockAuthTest.mockImplementation(() => Promise.reject(new Error("invalid_auth: xoxb-test-token-leaked exposed in error")), ); @@ -619,8 +686,10 @@ describe("lifecycle and bot user discovery", () => { test("disconnect on a never-connected channel is a no-op", async () => { const channel = new SlackHttpChannel(baseConfig); + // No throw, no state change beyond what the never-connected channel + // already has. await channel.disconnect(); - expect(mockStop).toHaveBeenCalledTimes(0); + expect(channel.getConnectionState()).toBe("disconnected"); }); }); @@ -632,8 +701,6 @@ describe("synthetic first DM on connect", () => { channel.setPhantomName("Maple"); await channel.connect(); - // installerUserId is U_INSTALLER per baseConfig; conversations.open is - // called once for the introduction. The post then hits D_DM_OPEN. expect(mockConversationsOpen).toHaveBeenCalledWith({ users: "U_INSTALLER" }); const calls = mockPostMessage.mock.calls as unknown as Array<[{ channel?: string; text?: string }]>; const introCall = calls.find((c) => { @@ -667,16 +734,10 @@ describe("synthetic first DM on connect", () => { const channel = new SlackHttpChannel(baseConfig); await expect(channel.connect()).resolves.toBeUndefined(); expect(channel.getConnectionState()).toBe("connected"); - // Restore for subsequent tests. mockPostMessage.mockImplementation(() => Promise.resolve({ ts: "1234567890.123456" })); }); test("retries the introduction on a fresh connect after first send returned null", async () => { - // First connect: chat.postMessage returns no ts (rate-limit, archived - // channel). firstDmSent stays false; the caller's failed_first_dm - // path is what surfaces this externally. On a fresh connect (after - // the operator restarts the channel), the introduction should fire - // again so the user eventually gets their DM. mockPostMessage.mockImplementationOnce(() => Promise.resolve({ ts: "" } as { ts: string })); const channel = new SlackHttpChannel(baseConfig); await channel.connect(); @@ -684,7 +745,6 @@ describe("synthetic first DM on connect", () => { await channel.connect(); const calls = mockPostMessage.mock.calls as unknown as Array<[{ channel?: string }]>; const introCalls = calls.filter((c) => c[0].channel === "D_DM_OPEN").length; - // One failed attempt + one successful retry on the second connect. expect(introCalls).toBe(2); }); }); @@ -707,9 +767,6 @@ describe("send / outbound", () => { test("postToChannel chunks long messages but keeps one chat.postMessage per chunk", async () => { const channel = new SlackHttpChannel(baseConfig); await channel.connect(); - // connect() fires the synthetic introduction DM which hits - // chat.postMessage once. Clear the count here so this test - // asserts only the explicit postToChannel call. mockPostMessage.mockClear(); await channel.postToChannel("C1", "short"); expect(mockPostMessage).toHaveBeenCalledTimes(1); diff --git a/src/channels/slack-channel-factory.ts b/src/channels/slack-channel-factory.ts index 91a3c82..7a941ff 100644 --- a/src/channels/slack-channel-factory.ts +++ b/src/channels/slack-channel-factory.ts @@ -15,7 +15,6 @@ export type SlackTransportMode = "socket" | "http"; export type CreateSlackChannelInput = { transport: SlackTransportMode; channelsConfig: ChannelsConfig | null; - port: number; metadataBaseUrl?: string; // The identity and secrets fetchers are injectable so tests can substitute // mocks without going through real fetch. In production, omit them and the @@ -75,8 +74,6 @@ export async function createSlackChannel(input: CreateSlackChannelInput): Promis teamId: identity.slack.teamId, installerUserId: identity.slack.installerUserId, teamName: identity.slack.teamName, - listenPort: input.port, - listenPath: "/slack", }); } diff --git a/src/channels/slack-http-handlers.ts b/src/channels/slack-http-handlers.ts new file mode 100644 index 0000000..e650e54 --- /dev/null +++ b/src/channels/slack-http-handlers.ts @@ -0,0 +1,276 @@ +// HTTP-receiver handler glue for the in-VM Phantom. This file converts a +// Bun `Request` into a Bolt `ReceiverEvent` and dispatches it through the +// host channel. Pulling the conversion out of `slack-http-receiver.ts` +// keeps the channel class focused on lifecycle (auth.test, intro DM, +// state machine) and lets the receiver file stay under the 300-line +// budget enforced for the channels/ directory. +// +// Design note on "why a Bun handler instead of an ExpressReceiver": +// The Phantom process serves /health, /chat, /ui, /webhook, and now the +// Slack ingress on a single `Bun.serve` listener at config.port (3100). +// The previous code instantiated an `ExpressReceiver` and tried to +// `receiver.start(3100)`, which collided with the existing Bun.serve +// bind, failed with EADDRINUSE-equivalent semantics, and left +// `slackChannel.isConnected() === false`. That made `POST /slack/events` +// return 404 because the receiver routes never came up. One process, one +// listener, one socket; the Bun handler dispatches verified events +// straight into Bolt's `App.processEvent` via a tiny custom Receiver. + +import type { App, Receiver, ReceiverEvent } from "@slack/bolt"; +import { extractTeamId, verifyGatewaySignature } from "./slack-gateway-verifier.ts"; +import { isUrlVerificationBody } from "./slack-http-utils.ts"; + +const HANDLER_LOG_TAG = "slack-http"; + +// Three Slack ingress paths Phantom mounts on the shared Bun.serve. +// These are pinned: phantom-slack-events on the gateway side targets +// these exact suffixes via the forwarder's tenant_url_template. +export const SLACK_HTTP_PATHS = { + events: "/slack/events", + interactivity: "/slack/interactivity", + commands: "/slack/commands", +} as const; + +// HTTP transport never sends a Slack response body in the inbound ack; +// we 200 with an empty body for every accepted event. Bolt's listener +// timing semantics (auto-ack within 3s, processBeforeResponse=false) +// match this since the actual user-facing reply is sent via +// `chat.postMessage` from the agent runtime, not as the inbound HTTP +// response. +const EMPTY_OK = (): Response => new Response("", { status: 200 }); + +// Bolt requires the `App` constructor to receive a `Receiver` whose +// `init(app)` is called synchronously. We don't bind a server here; +// the Bun handler is the receiver. `start` and `stop` are no-ops so +// `await app.start()` / `await app.stop()` (if ever called by Bolt +// internals) resolve cleanly. The widening at the App constructor +// site casts our minimal shape into Bolt's Receiver type. +export class BunReceiver implements Receiver { + private bolt: App | null = null; + + init(app: App): void { + this.bolt = app; + } + + getBoltApp(): App | null { + return this.bolt; + } + + start(): Promise { + return Promise.resolve(); + } + + stop(): Promise { + return Promise.resolve(); + } +} + +// Verifier outcome: either the parsed body the dispatcher should hand +// to Bolt, or a Response the caller returns directly to the client. +type VerifyOutcome = + | { ok: true; body: Record; rawBody: Buffer; contentType: string } + | { ok: false; response: Response }; + +export type DispatchInput = { + req: Request; + gatewaySigningSecret: string; + expectedTeamId: string; +}; + +// Verify the gateway HMAC, the replay window, and the team_id binding, +// then parse the raw body into the shape Bolt expects (JSON object for +// events, the inner JSON of `payload` for interactivity, the URLSearch +// dictionary for commands). Returns a Response on every short-circuit +// path so the caller can pipe it straight back to Bun. +export async function verifyAndParse(input: DispatchInput): Promise { + const { req, gatewaySigningSecret, expectedTeamId } = input; + + let raw: Buffer; + try { + const buf = await req.arrayBuffer(); + raw = Buffer.from(buf); + } catch { + return { ok: false, response: new Response("bad request", { status: 400 }) }; + } + + const sigHeader = req.headers.get("x-phantom-signature"); + const fwdHeader = req.headers.get("x-phantom-forwarded-at"); + const eventId = req.headers.get("x-phantom-slack-event-id"); + const contentType = req.headers.get("content-type") ?? "application/json"; + + if (!verifyGatewaySignature({ sigHeader, fwdHeader, eventId, raw, secret: gatewaySigningSecret })) { + return { ok: false, response: new Response("unauthorized", { status: 401 }) }; + } + + // extractTeamId from slack-gateway-verifier.ts handles JSON events and + // the urlencoded-with-`payload` shape used by interactivity. Slash + // commands are urlencoded with top-level `team_id`; we extend the + // extractor here (NOT in the verifier file, which is intentionally + // kept pure for the events/interactivity contract) so the third + // Slack ingress path can pass team binding without a verifier change. + let eventTeamId = extractTeamId(raw, contentType); + if (eventTeamId === undefined && contentType.toLowerCase().includes("application/x-www-form-urlencoded")) { + const fromForm = new URLSearchParams(raw.toString("utf-8")).get("team_id"); + if (fromForm) eventTeamId = fromForm; + } + if (eventTeamId === undefined) { + // `url_verification` is the one legitimate body shape with no team_id. + // Anything else is foreign by contract: we reject defensively even + // though the gateway already verified Slack-side authenticity. + if (!isUrlVerificationBody(raw, contentType)) { + return { ok: false, response: new Response("forbidden", { status: 403 }) }; + } + } else if (eventTeamId !== expectedTeamId) { + return { ok: false, response: new Response("forbidden", { status: 403 }) }; + } + + const body = parseRequestBody(raw, contentType); + if (body === null) { + return { ok: false, response: new Response("bad request", { status: 400 }) }; + } + + return { ok: true, body, rawBody: raw, contentType }; +} + +// Mirror of Bolt's ExpressReceiver `parseRequestBody`: JSON for +// application/json, inner JSON for `payload=...` urlencoded interactivity +// payloads, querystring object for everything else (slash commands). +// Returns null when the body cannot be parsed at all. +function parseRequestBody(raw: Buffer, contentType: string): Record | null { + const text = raw.toString("utf-8"); + const lower = contentType.toLowerCase(); + if (lower.includes("application/x-www-form-urlencoded")) { + const params = new URLSearchParams(text); + const payload = params.get("payload"); + if (typeof payload === "string") { + try { + return JSON.parse(payload) as Record; + } catch { + return null; + } + } + const obj: Record = {}; + for (const [k, v] of params) obj[k] = v; + return obj; + } + try { + return JSON.parse(text) as Record; + } catch { + return null; + } +} + +// Build the AckFn the Bolt App calls when a listener finishes. The +// resulting Promise resolves to the Response we return to Bun. The +// hard timeout fallback (3s, matching Bolt's own +// unhandledRequestTimeoutMillis default) guarantees the Promise always +// settles even if a future Bolt regression were to drop the auto-ack; +// the operator-facing log makes the regression visible. +const ACK_TIMEOUT_MS = 3000; + +type AckResolver = { + ackFn: (response?: unknown) => Promise; + awaitAck: Promise; +}; + +function buildAckResolver(): AckResolver { + let resolve!: (resp: Response) => void; + const awaitAck = new Promise((r) => { + resolve = r; + }); + let acked = false; + const settle = (resp: Response): void => { + if (acked) return; + acked = true; + resolve(resp); + }; + const ackFn = async (response?: unknown): Promise => { + if (response instanceof Error) { + settle(new Response("", { status: 500 })); + return; + } + if (response === undefined || response === null || response === "") { + settle(EMPTY_OK()); + return; + } + if (typeof response === "string") { + settle(new Response(response, { status: 200 })); + return; + } + // Bolt action / view-submission listeners return a structured + // response; we serialize as JSON so the Slack client receives a + // well-formed body without a Content-Type sniff. + settle( + new Response(JSON.stringify(response), { + status: 200, + headers: { "content-type": "application/json" }, + }), + ); + }; + const timer = setTimeout(() => { + if (acked) return; + console.warn(`[${HANDLER_LOG_TAG}] ack timeout after ${ACK_TIMEOUT_MS}ms; returning empty 200`); + settle(EMPTY_OK()); + }, ACK_TIMEOUT_MS); + // Don't keep the event loop alive purely for the timeout; the + // awaitAck promise alone is what dispatchers wait on. + if (typeof timer === "object" && "unref" in timer && typeof timer.unref === "function") { + timer.unref(); + } + void awaitAck.then(() => clearTimeout(timer)); + return { ackFn, awaitAck }; +} + +export type DispatchToBoltInput = DispatchInput & { + app: App; + retryNumHeader?: string | null; + retryReasonHeader?: string | null; +}; + +// Run the verifier guard and dispatch the parsed body into the Bolt +// `App`. `processEvent` invokes the registered listener middleware and +// calls our AckFn when a listener acks; we return that response to +// Bun. URL-verification bodies short-circuit with the challenge string, +// matching the path Bolt's HTTPReceiver takes before constructing a +// ReceiverEvent. +export async function dispatchToBolt(input: DispatchToBoltInput): Promise { + const verified = await verifyAndParse(input); + if (!verified.ok) return verified.response; + + if (verified.body.type === "url_verification" && typeof verified.body.challenge === "string") { + return new Response(verified.body.challenge, { + status: 200, + headers: { "content-type": "text/plain" }, + }); + } + + const { ackFn, awaitAck } = buildAckResolver(); + const receiverEvent: ReceiverEvent = { + body: verified.body, + ack: ackFn, + retryNum: parseRetryNum(input.retryNumHeader), + retryReason: input.retryReasonHeader ?? undefined, + }; + + try { + await input.app.processEvent(receiverEvent); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`[${HANDLER_LOG_TAG}] processEvent threw: ${msg}`); + // processEvent already invokes ack via Bolt's auto-ack pathway in + // most cases, so the awaitAck promise has already resolved. We + // only fall back to a 500 when the ack never fired. + await ackFn(); + } + + // Bolt 3.x auto-acks within 3s (processBeforeResponse=false default); + // we wait that promise here so the HTTP response carries the + // listener's chosen body when one exists. + return awaitAck; +} + +function parseRetryNum(value: string | null | undefined): number | undefined { + if (!value) return undefined; + const n = Number.parseInt(value, 10); + return Number.isFinite(n) ? n : undefined; +} diff --git a/src/channels/slack-http-receiver.ts b/src/channels/slack-http-receiver.ts index 12a66ca..e910554 100644 --- a/src/channels/slack-http-receiver.ts +++ b/src/channels/slack-http-receiver.ts @@ -1,21 +1,29 @@ -// HTTP receiver mode for hosted, operator-managed deployments. Slack -// events are captured by a shared central gateway, verified against -// Slack's signing secret there, then forwarded over HTTPS to the -// per-deployment Phantom on this VM. Self-hosters keep the Socket Mode -// flow at `slack.ts`; SLACK_TRANSPORT=http opts a deployment into this -// class. +// HTTP receiver mode for hosted, operator-managed deployments. The Slack +// gateway (phantom-slack-events on the cpx22 host) verifies Slack-side +// HMAC and forwards each event to phantomd over mTLS gRPC; phantomd then +// signs the canonical request with the per-tenant gateway secret and +// POSTs to this in-VM Phantom on the host-side veth. // -// Three security layers operate at this boundary: -// 1. Caddy validates the gateway HMAC and strips inbound X-Phantom-* headers. -// 2. This class re-verifies the gateway HMAC via `slack-gateway-verifier`. -// 3. The parsed body's team_id MUST match the tenant's installer team_id. -// The one body shape allowed without team_id is `url_verification`. +// On the in-VM side three security layers operate at this boundary: +// 1. The host-side veth firewall (phantomd_tenants nft chain) only +// admits packets originating from the host network namespace. +// 2. `slack-gateway-verifier` re-verifies the gateway HMAC + replay +// window over the raw bytes the gateway signed. +// 3. The parsed body's team_id MUST match the tenant's installer +// team_id; the one body shape allowed without team_id is +// `url_verification`, which we short-circuit before Bolt sees it. // -// `signatureVerification: false` on ExpressReceiver skips Slack's signing -// secret because the gateway has already verified Slack's signature. - -import { App, ExpressReceiver, type LogLevel } from "@slack/bolt"; -import type { Request, RequestHandler, Response } from "express"; +// Why a Bun-native receiver instead of an ExpressReceiver: +// Phantom's `Bun.serve` already owns config.port (3100) for /health, +// /chat, /ui, /webhook, and /mcp. Spinning up a second HTTP listener on +// the same port is impossible (EADDRINUSE) and a separate port would +// require a second tenant ingress rule on every host. We mount the +// three Slack ingress paths on the existing Bun.serve via +// `tryHandleSlackHttp` (slack-http-routes.ts) and dispatch parsed +// events into Bolt's App via a tiny `BunReceiver` whose start/stop are +// no-ops. One process, one socket, no port collision. + +import { App, type LogLevel } from "@slack/bolt"; import type { SlackBlock } from "./feedback.ts"; import { registerSlackActions } from "./slack-actions.ts"; import { @@ -29,17 +37,9 @@ import { egressUpdateMessage, egressUpdateWithFeedback, } from "./slack-egress.ts"; -import { extractTeamId, verifyGatewaySignature } from "./slack-gateway-verifier.ts"; import { type EventDispatchHost, type ReactionFn, registerHttpEventHandlers } from "./slack-http-events.ts"; -import { - type RequestWithRawBody, - getContentType, - headerString, - isUrlVerificationBody, - readRequestBody, - redactTokens, - rehydrateBody, -} from "./slack-http-utils.ts"; +import { BunReceiver, dispatchToBolt } from "./slack-http-handlers.ts"; +import { redactTokens } from "./slack-http-utils.ts"; import { sendIntroductionDm } from "./slack-introduction.ts"; import type { Channel, ChannelCapabilities, InboundMessage, OutboundMessage, SentMessage } from "./types.ts"; @@ -49,8 +49,6 @@ export type SlackHttpChannelConfig = { teamId: string; installerUserId: string; teamName: string; - listenPort: number; - listenPath: string; }; type ConnectionState = "disconnected" | "connecting" | "connected" | "error"; @@ -70,13 +68,11 @@ export class SlackHttpChannel implements Channel, EventDispatchHost { }; private readonly app: App; - private readonly receiver: ExpressReceiver; + private readonly receiver: BunReceiver; private readonly teamId: string; private readonly installerUserId: string; private readonly teamName: string; private readonly gatewaySigningSecret: string; - private readonly listenPort: number; - private readonly listenPath: string; private messageHandler: ((message: InboundMessage) => Promise) | null = null; private reactionHandler: ReactionFn | null = null; @@ -98,23 +94,11 @@ export class SlackHttpChannel implements Channel, EventDispatchHost { this.installerUserId = config.installerUserId; this.teamName = config.teamName; this.gatewaySigningSecret = config.gatewaySigningSecret; - this.listenPort = config.listenPort; - this.listenPath = config.listenPath; - - this.receiver = new ExpressReceiver({ - signingSecret: "phase-5b-unused-gateway-verifies-instead", - signatureVerification: false, - endpoints: { - events: `${this.listenPath}/events`, - interactive: `${this.listenPath}/interactivity`, - commands: `${this.listenPath}/commands`, - }, - processBeforeResponse: false, - logLevel: "ERROR" as LogLevel, - }); - - this.installVerifier(); + // Bolt's `App` constructor calls `receiver.init(app)` synchronously + // to wire the App reference; we reuse that App for processEvent + // dispatches from the Bun handlers. + this.receiver = new BunReceiver(); this.app = new App({ token: config.botToken, receiver: this.receiver, @@ -161,54 +145,32 @@ export class SlackHttpChannel implements Channel, EventDispatchHost { return text.replace(/^<@[A-Z0-9]+>\s*/, ""); } - // Fails closed: missing headers, tampered body, stale/future timestamp -> 401; - // foreign or unknown team_id -> 403. Only `url_verification` may lack team_id. - private installVerifier(): void { - const expressApp = this.receiver.app; - const guard = this.makeGuardMiddleware(); - expressApp.use(`${this.listenPath}/events`, guard); - expressApp.use(`${this.listenPath}/interactivity`, guard); - expressApp.use(`${this.listenPath}/commands`, guard); + // Public entrypoints invoked by `mountSlackHttpRoutes` against the + // Bun.serve listener. Each runs the verifier guard, parses the body, + // and dispatches the event into Bolt. Failure modes resolve to a + // Response (401/403/400) rather than throwing so the Bun handler can + // pipe them straight to the client without surrounding try/catch. + async handleEvent(req: Request): Promise { + return this.dispatch(req); } - private makeGuardMiddleware(): RequestHandler { - const secret = this.gatewaySigningSecret; - const expectedTeamId = this.teamId; - - return async (req: Request, res: Response, next): Promise => { - let raw: Buffer; - try { - raw = await readRequestBody(req as RequestWithRawBody); - } catch { - res.status(400).end("bad request"); - return; - } - - (req as RequestWithRawBody).rawBody = raw; - - const sigHeader = headerString(req, "x-phantom-signature"); - const fwdHeader = headerString(req, "x-phantom-forwarded-at"); - const eventId = headerString(req, "x-phantom-slack-event-id"); - - if (!verifyGatewaySignature({ sigHeader, fwdHeader, eventId, raw, secret })) { - res.status(401).end("unauthorized"); - return; - } + async handleInteractivity(req: Request): Promise { + return this.dispatch(req); + } - const eventTeamId = extractTeamId(raw, getContentType(req)); - if (eventTeamId === undefined) { - if (!isUrlVerificationBody(raw, getContentType(req))) { - res.status(403).end("forbidden"); - return; - } - } else if (eventTeamId !== expectedTeamId) { - res.status(403).end("forbidden"); - return; - } + async handleCommand(req: Request): Promise { + return this.dispatch(req); + } - rehydrateBody(req, raw); - next(); - }; + private async dispatch(req: Request): Promise { + return dispatchToBolt({ + req, + app: this.app, + gatewaySigningSecret: this.gatewaySigningSecret, + expectedTeamId: this.teamId, + retryNumHeader: req.headers.get("x-slack-retry-num"), + retryReasonHeader: req.headers.get("x-slack-retry-reason"), + }); } async connect(): Promise { @@ -228,9 +190,8 @@ export class SlackHttpChannel implements Channel, EventDispatchHost { } console.log(`[${LOG_TAG}] Resolved bot user <@${this.botUserId}>`); - await this.receiver.start(this.listenPort); this.connectionState = "connected"; - console.log(`[${LOG_TAG}] Listening on :${this.listenPort}${this.listenPath}/{events,interactivity,commands}`); + console.log(`[${LOG_TAG}] Ready; routes mounted on the shared Bun.serve listener`); } catch (err: unknown) { this.connectionState = "error"; const rawMsg = err instanceof Error ? err.message : String(err); @@ -239,8 +200,8 @@ export class SlackHttpChannel implements Channel, EventDispatchHost { throw err; } - // Synthetic first DM. Fire after receiver.start so the channel is - // wired before the user can reply; gate on firstDmSent so a + // Synthetic first DM. Fire after the channel state flips to connected + // so the user can reply immediately; gate on firstDmSent so a // reconnect-after-drop does not re-introduce. if (!this.firstDmSent && this.installerUserId) { const result = await sendIntroductionDm({ @@ -257,12 +218,11 @@ export class SlackHttpChannel implements Channel, EventDispatchHost { async disconnect(): Promise { if (this.connectionState === "disconnected") return; - try { - await this.receiver.stop(); - } catch (err: unknown) { - const msg = err instanceof Error ? err.message : String(err); - console.warn(`[${LOG_TAG}] Error during disconnect: ${redactTokens(msg)}`); - } + // No HTTP listener to stop here; the routes are mounted on the shared + // Bun.serve owned by `core/server.ts`. We simply transition the + // state machine so the /health channel flag flips to false and + // downstream callers (router.disconnectAll, scheduler) see a + // disconnected channel. this.connectionState = "disconnected"; console.log(`[${LOG_TAG}] Disconnected`); } diff --git a/src/channels/slack-http-routes.ts b/src/channels/slack-http-routes.ts new file mode 100644 index 0000000..03fec42 --- /dev/null +++ b/src/channels/slack-http-routes.ts @@ -0,0 +1,57 @@ +// Slack ingress routes mounted on Phantom's shared Bun.serve listener. +// `core/server.ts` calls `tryHandleSlackHttp(req, channel)` before +// returning 404 for unknown paths. The channel is supplied via a +// provider (set in src/index.ts after the Slack channel is created) +// and may be null for self-hosted deployments running Socket Mode, +// which never mount these routes. + +import { SLACK_HTTP_PATHS } from "./slack-http-handlers.ts"; +import type { SlackHttpChannel } from "./slack-http-receiver.ts"; + +let slackHttpChannelProvider: (() => SlackHttpChannel | null) | null = null; + +export function setSlackHttpChannelProvider(provider: () => SlackHttpChannel | null): void { + slackHttpChannelProvider = provider; +} + +// Returns the Slack ingress Response for /slack/{events,interactivity,commands}, +// or `null` when the path is not a Slack route or no HTTP-mode channel is +// wired (Socket Mode self-hosters fall through to the 404 path). +export async function tryHandleSlackHttp(req: Request): Promise { + const url = new URL(req.url); + if (!isSlackHttpPath(url.pathname)) return null; + + if (req.method !== "POST") { + return new Response("method not allowed", { + status: 405, + headers: { Allow: "POST" }, + }); + } + + const channel = slackHttpChannelProvider?.(); + if (!channel) { + // Socket Mode tenants never mount these routes; an inbound POST is + // either a probe or a misconfigured forwarder. 503 makes the + // distinction loud while still being a retryable signal upstream. + return new Response("slack http channel not configured", { status: 503 }); + } + + switch (url.pathname) { + case SLACK_HTTP_PATHS.events: + return channel.handleEvent(req); + case SLACK_HTTP_PATHS.interactivity: + return channel.handleInteractivity(req); + case SLACK_HTTP_PATHS.commands: + return channel.handleCommand(req); + default: + return null; + } +} + +function isSlackHttpPath(pathname: string): boolean { + return ( + pathname === SLACK_HTTP_PATHS.events || + pathname === SLACK_HTTP_PATHS.interactivity || + pathname === SLACK_HTTP_PATHS.commands + ); +} diff --git a/src/core/__tests__/server-slack-routes.test.ts b/src/core/__tests__/server-slack-routes.test.ts new file mode 100644 index 0000000..d5a56f2 --- /dev/null +++ b/src/core/__tests__/server-slack-routes.test.ts @@ -0,0 +1,135 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import YAML from "yaml"; +import { setSlackHttpChannelProvider } from "../../channels/slack-http-routes.ts"; +import { hashTokenSync } from "../../mcp/config.ts"; +import type { McpConfig } from "../../mcp/types.ts"; +import { startServer } from "../server.ts"; + +/** + * Pin Bug A's port-collision fix end-to-end. Before this fix, an inbound + * `POST /slack/events` to the Phantom Bun.serve port returned 404 because + * the slack channel tried to bind a second HTTP server to the same port, + * failed, and never mounted the routes. After the fix, the routes are + * mounted on the existing Bun.serve and a stub channel handles them. + * + * The stub channel here is shaped exactly like the SlackHttpChannel + * surface that `tryHandleSlackHttp` calls into: handleEvent, + * handleInteractivity, handleCommand. We do NOT instantiate Bolt; the + * goal is to prove the route is alive and the handler is reachable. + */ +describe("server slack-http routing", () => { + const mcpConfigPath = "config/mcp.yaml"; + let originalMcpYaml: string | null = null; + let server: ReturnType; + let baseUrl: string; + + const calls: Array<{ method: string; path: string }> = []; + const stub = { + async handleEvent(req: Request): Promise { + calls.push({ method: req.method, path: new URL(req.url).pathname }); + return new Response("event-ok", { status: 200 }); + }, + async handleInteractivity(req: Request): Promise { + calls.push({ method: req.method, path: new URL(req.url).pathname }); + return new Response("interactivity-ok", { status: 200 }); + }, + async handleCommand(req: Request): Promise { + calls.push({ method: req.method, path: new URL(req.url).pathname }); + return new Response("command-ok", { status: 200 }); + }, + }; + + beforeAll(() => { + if (existsSync(mcpConfigPath)) { + originalMcpYaml = readFileSync(mcpConfigPath, "utf-8"); + } + const mcpConfig: McpConfig = { + tokens: [{ name: "admin", hash: hashTokenSync("test-admin"), scopes: ["read", "operator", "admin"] }], + rate_limit: { requests_per_minute: 60, burst: 10 }, + }; + mkdirSync("config", { recursive: true }); + writeFileSync(mcpConfigPath, YAML.stringify(mcpConfig), "utf-8"); + + // `setSlackHttpChannelProvider` accepts a SlackHttpChannel; the stub + // matches the structural shape `tryHandleSlackHttp` invokes and is + // cast via `as never` ONLY at the test boundary so the test can stay + // hermetic without spinning up a real Bolt App + auth.test. + setSlackHttpChannelProvider(() => stub as never); + + server = startServer({ name: "phantom", port: 0, role: "base" } as never, Date.now()); + baseUrl = `http://localhost:${server.port}`; + }); + + afterAll(() => { + server?.stop(true); + setSlackHttpChannelProvider(() => null); + if (originalMcpYaml !== null) { + writeFileSync(mcpConfigPath, originalMcpYaml, "utf-8"); + } + }); + + test("POST /slack/events reaches the channel handler", async () => { + calls.length = 0; + const res = await fetch(`${baseUrl}/slack/events`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: '{"team_id":"T1"}', + }); + expect(res.status).toBe(200); + expect(await res.text()).toBe("event-ok"); + expect(calls).toHaveLength(1); + expect(calls[0]?.path).toBe("/slack/events"); + }); + + test("POST /slack/interactivity reaches the channel handler", async () => { + calls.length = 0; + const res = await fetch(`${baseUrl}/slack/interactivity`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: "payload=%7B%7D", + }); + expect(res.status).toBe(200); + expect(await res.text()).toBe("interactivity-ok"); + expect(calls).toHaveLength(1); + expect(calls[0]?.path).toBe("/slack/interactivity"); + }); + + test("POST /slack/commands reaches the channel handler", async () => { + calls.length = 0; + const res = await fetch(`${baseUrl}/slack/commands`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: "team_id=T1&command=%2Fphantom", + }); + expect(res.status).toBe(200); + expect(await res.text()).toBe("command-ok"); + expect(calls).toHaveLength(1); + expect(calls[0]?.path).toBe("/slack/commands"); + }); + + test("GET /slack/events returns 405 (POST-only ingress)", async () => { + const res = await fetch(`${baseUrl}/slack/events`); + expect(res.status).toBe(405); + expect(res.headers.get("Allow")).toBe("POST"); + }); + + test("POST /slack/unknown is not a slack route and falls through to 404", async () => { + const res = await fetch(`${baseUrl}/slack/unknown`, { method: "POST" }); + expect(res.status).toBe(404); + }); + + test("POST /slack/events with no channel provider returns 503", async () => { + setSlackHttpChannelProvider(() => null); + try { + const res = await fetch(`${baseUrl}/slack/events`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + expect(res.status).toBe(503); + } finally { + setSlackHttpChannelProvider(() => stub as never); + } + }); +}); diff --git a/src/core/server.ts b/src/core/server.ts index c45f21a..c0be71a 100644 --- a/src/core/server.ts +++ b/src/core/server.ts @@ -1,5 +1,6 @@ import { resolve as pathResolve } from "node:path"; import type { AgentRuntime } from "../agent/runtime.ts"; +import { tryHandleSlackHttp } from "../channels/slack-http-routes.ts"; import type { SlackTransport } from "../channels/slack-transport.ts"; import { handleEmailLogin } from "../chat/email-login.ts"; import type { PhantomConfig } from "../config/types.ts"; @@ -168,6 +169,14 @@ export function startServer(config: PhantomConfig, startedAt: number): ReturnTyp return handleTrigger(req); } + // Slack HTTP-mode ingress: phantom-slack-events on the gateway side + // forwards verified Slack events here through phantomd. The channel + // holds the per-tenant gateway signing secret; the helper returns + // `null` when this is not a Slack path, so we fall through to the + // remaining routes without overlap. + const slackResponse = await tryHandleSlackHttp(req); + if (slackResponse) return slackResponse; + if (url.pathname === "/webhook") { if (!webhookHandler) { return Response.json({ status: "error", message: "Webhook channel not configured" }, { status: 503 }); diff --git a/src/index.ts b/src/index.ts index 8511b64..e4208a7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -12,6 +12,8 @@ import { createProgressStream } from "./channels/progress-stream.ts"; import { ChannelRouter } from "./channels/router.ts"; import { setActionFollowUpHandler } from "./channels/slack-actions.ts"; import { createSlackChannel, readSlackTransportFromEnv } from "./channels/slack-channel-factory.ts"; +import { SlackHttpChannel } from "./channels/slack-http-receiver.ts"; +import { setSlackHttpChannelProvider } from "./channels/slack-http-routes.ts"; import type { SlackTransport } from "./channels/slack-transport.ts"; import { createStatusReactionController } from "./channels/status-reactions.ts"; import { TelegramChannel } from "./channels/telegram.ts"; @@ -313,13 +315,24 @@ async function main(): Promise { const slackChannel: SlackTransport | null = await createSlackChannel({ transport: slackTransport, channelsConfig, - port: config.port, metadataBaseUrl: process.env.METADATA_BASE_URL, }); if (slackChannel) { slackChannel.setPhantomName(config.name); + // HTTP-mode tenants mount /slack/{events,interactivity,commands} on + // the existing Bun.serve listener. The provider lookup is lazy so + // `core/server.ts` can resolve the live channel reference per + // request, which keeps the route table compatible with a future + // hot-swap of the Slack identity without restarting the process. + // `instanceof` discriminates the SlackTransport union without an + // `as` cast even though slackTransport === "http" already implies + // the http variant by construction. + if (slackChannel instanceof SlackHttpChannel) { + setSlackHttpChannelProvider(() => slackChannel); + } + // Wire Slack reaction feedback to evolution slackChannel.onReaction((event) => { emitFeedback({ From 2c232c56117cd32a1c62df0360a8f5387bb25453 Mon Sep 17 00:00:00 2001 From: Muhammad Ahmed Cheema Date: Mon, 27 Apr 2026 23:50:50 -0700 Subject: [PATCH 2/3] channels: gate slack-http dispatch on channel.isConnected() (Codex P1) The provider is wired in src/index.ts during channel setup before router.connectAll() runs, so inbound POSTs arriving in the startup window dispatched into a half-initialized channel. The gate also covers the post-disconnect() path (state flips to "disconnected") and the auth-failure path (state flips to "error"). Both 503 paths return JSON. Slack retries 503 up to 3 times within 5 minutes per the inbound contract, so the gate resolves naturally. +3 tests pinning the not-yet-connected case, the post-disconnect sibling case (audit doc inline), and the same gate on /slack/interactivity. --- src/channels/slack-http-routes.ts | 25 ++++++- .../__tests__/server-slack-routes.test.ts | 74 ++++++++++++++++++- 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/src/channels/slack-http-routes.ts b/src/channels/slack-http-routes.ts index 03fec42..4e59fba 100644 --- a/src/channels/slack-http-routes.ts +++ b/src/channels/slack-http-routes.ts @@ -31,9 +31,21 @@ export async function tryHandleSlackHttp(req: Request): Promise const channel = slackHttpChannelProvider?.(); if (!channel) { // Socket Mode tenants never mount these routes; an inbound POST is - // either a probe or a misconfigured forwarder. 503 makes the - // distinction loud while still being a retryable signal upstream. - return new Response("slack http channel not configured", { status: 503 }); + // either a probe or a misconfigured forwarder. 503 stays loud but + // retryable so Slack's own retry schedule (up to 3 attempts within + // 5 minutes) resolves a transient deploy-window race. + return notReady("slack channel not configured"); + } + if (!channel.isConnected()) { + // The provider is wired in `src/index.ts` during channel setup + // BEFORE `router.connectAll()` runs, so an inbound POST in the + // startup window finds a channel object whose `connect()` (auth.test + // + state flip) has not completed. The same gate covers the + // post-`disconnect()` path (state flips back to "disconnected") and + // the auth-failure path (state flips to "error"); only "connected" + // passes. 503 lets Slack retry until the channel is ready instead + // of accepting events that no listener is wired to handle. + return notReady("slack channel not ready"); } switch (url.pathname) { @@ -55,3 +67,10 @@ function isSlackHttpPath(pathname: string): boolean { pathname === SLACK_HTTP_PATHS.commands ); } + +function notReady(reason: string): Response { + return new Response(JSON.stringify({ error: reason }), { + status: 503, + headers: { "content-type": "application/json" }, + }); +} diff --git a/src/core/__tests__/server-slack-routes.test.ts b/src/core/__tests__/server-slack-routes.test.ts index d5a56f2..2cf79f5 100644 --- a/src/core/__tests__/server-slack-routes.test.ts +++ b/src/core/__tests__/server-slack-routes.test.ts @@ -25,7 +25,9 @@ describe("server slack-http routing", () => { let baseUrl: string; const calls: Array<{ method: string; path: string }> = []; + let stubConnected = true; const stub = { + isConnected: () => stubConnected, async handleEvent(req: Request): Promise { calls.push({ method: req.method, path: new URL(req.url).pathname }); return new Response("event-ok", { status: 200 }); @@ -119,7 +121,7 @@ describe("server slack-http routing", () => { expect(res.status).toBe(404); }); - test("POST /slack/events with no channel provider returns 503", async () => { + test("POST /slack/events with no channel provider returns 503 with JSON body", async () => { setSlackHttpChannelProvider(() => null); try { const res = await fetch(`${baseUrl}/slack/events`, { @@ -128,8 +130,78 @@ describe("server slack-http routing", () => { body: "{}", }); expect(res.status).toBe(503); + expect(res.headers.get("content-type")).toContain("application/json"); + expect(await res.json()).toEqual({ error: "slack channel not configured" }); } finally { setSlackHttpChannelProvider(() => stub as never); } }); + + // Codex P1 fix: the channel provider is wired during channel setup in + // `src/index.ts` BEFORE `router.connectAll()` finishes running connect() + // on each channel. Inbound POSTs that arrive in the startup window must + // not dispatch into a half-initialized channel; tryHandleSlackHttp gates + // on `channel.isConnected()` and returns a retryable 503 so Slack's own + // retry schedule (up to 3 attempts within 5 minutes) resolves the race. + test("POST /slack/events when channel is not yet connected returns 503 with JSON body", async () => { + calls.length = 0; + stubConnected = false; + try { + const res = await fetch(`${baseUrl}/slack/events`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + expect(res.status).toBe(503); + expect(res.headers.get("content-type")).toContain("application/json"); + expect(await res.json()).toEqual({ error: "slack channel not ready" }); + expect(calls).toHaveLength(0); + } finally { + stubConnected = true; + } + }); + + // Sibling-bug audit: the same gate must also cover the post-`disconnect()` + // path. SlackHttpChannel.disconnect() flips connectionState back to + // "disconnected" (slack-http-receiver.ts), and the auth-failure path + // during connect() flips it to "error". `isConnected()` returns true only + // for "connected", so this single gate handles all three failure modes: + // startup race, post-disconnect ingress, and auth-error state. + test("POST /slack/events after channel disconnects returns 503 (same gate covers disconnect path)", async () => { + stubConnected = true; + const okRes = await fetch(`${baseUrl}/slack/events`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + expect(okRes.status).toBe(200); + + stubConnected = false; + try { + const res = await fetch(`${baseUrl}/slack/events`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + expect(res.status).toBe(503); + expect(await res.json()).toEqual({ error: "slack channel not ready" }); + } finally { + stubConnected = true; + } + }); + + test("POST /slack/interactivity respects the same isConnected gate", async () => { + stubConnected = false; + try { + const res = await fetch(`${baseUrl}/slack/interactivity`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: "payload=%7B%7D", + }); + expect(res.status).toBe(503); + expect(await res.json()).toEqual({ error: "slack channel not ready" }); + } finally { + stubConnected = true; + } + }); }); From ad01625e1990af2a8b9802c041aca438b8193d13 Mon Sep 17 00:00:00 2001 From: Muhammad Ahmed Cheema Date: Tue, 28 Apr 2026 00:09:50 -0700 Subject: [PATCH 3/3] channels: race processEvent against ack in dispatchToBolt (Codex round 2 P1+P2) Previously dispatchToBolt awaited input.app.processEvent before returning, but Bolt's processEvent resolves only when listener middleware finishes, not when ack() fires. Phantom's Slack listener calls runtime.handleMessage which routinely outlasts Slack's ~3s ack window, so /slack/events stayed open past the deadline and triggered Slack-side retries. The catch path also called ackFn() with no args, which resolves to 200 and silently suppressed listener failures. Replace the await + try/catch with Promise.race over three tagged outcomes: ack winner returns the listener's response immediately, processEvent rejecting before ack surfaces as 500 so Slack retries, and processEvent resolving without any ack falls back to a 200 with an operator warning (defense against a hypothetical Bolt regression). The async listener work continues in the background, matching the previous HTTPReceiver processBeforeResponse=false semantics. Tests: +3 covering long-running listener (handler returns 200 while listener continues), processEvent rejects before ack (handler 500), and processEvent resolves without ack (handler 200 + warning). Total 1971 pass, typecheck and lint clean. File at 294 lines (under 300 channels/ budget). --- .../__tests__/slack-http-receiver.test.ts | 134 +++++++++++++++++- src/channels/slack-http-handlers.ts | 43 ++++-- 2 files changed, 161 insertions(+), 16 deletions(-) diff --git a/src/channels/__tests__/slack-http-receiver.test.ts b/src/channels/__tests__/slack-http-receiver.test.ts index 40589e9..bc7672b 100644 --- a/src/channels/__tests__/slack-http-receiver.test.ts +++ b/src/channels/__tests__/slack-http-receiver.test.ts @@ -23,6 +23,14 @@ const eventHandlers = new Map(); const actionHandlers = new Map(); const processEventCalls: ProcessEventCall[] = []; +// Per-test override for the App.processEvent mock. Tests that exercise +// the ack-vs-processEvent race set this; the default behavior auto-acks +// to keep the existing verifier-path tests realistic. +let processEventOverride: ((event: ProcessEventCall) => Promise) | null = null; +// Most recent processEvent promise so the race tests can wait on the +// background listener after the handler has already returned. +let lastProcessEventReturn: Promise | null = null; + let initReceived: { receiver: { init?: (app: unknown) => void } | null } = { receiver: null }; const MockApp = mock((opts: { receiver?: { init?: (app: unknown) => void } }) => { @@ -34,9 +42,17 @@ const MockApp = mock((opts: { receiver?: { init?: (app: unknown) => void } }) => const key = pattern instanceof RegExp ? pattern.source : pattern; actionHandlers.set(key, handler); }, - processEvent: async (event: ProcessEventCall) => { + processEvent: (event: ProcessEventCall): Promise => { processEventCalls.push(event); - await event.ack(); + const p: Promise = (async () => { + if (processEventOverride) { + await processEventOverride(event); + return; + } + await event.ack(); + })(); + lastProcessEventReturn = p; + return p; }, client: { auth: { test: mockAuthTest }, @@ -123,6 +139,8 @@ beforeEach(() => { eventHandlers.clear(); actionHandlers.clear(); processEventCalls.length = 0; + processEventOverride = null; + lastProcessEventReturn = null; initReceived = { receiver: null }; mockAuthTest.mockClear(); mockPostMessage.mockClear(); @@ -912,3 +930,115 @@ describe("access control (HTTP mode = any user from this.teamId)", () => { expect(called).toBe(false); }); }); + +// ----- ack-vs-processEvent race (Codex round 2 P1 + P2) ------------------- +// +// Bolt's `processEvent` resolves only after listener middleware has +// finished, not when `ack()` fires. Phantom's listener calls +// `runtime.handleMessage`, which can outlast Slack's ~3s ack window. +// `dispatchToBolt` therefore races the ack-resolved promise against +// processEvent so: +// - ack winner (normal path) returns the listener's response immediately +// - processEvent rejecting before ack returns 500 (Slack retries) +// - processEvent resolving without any ack returns 200 + log warn +// (defense against a hypothetical Bolt regression) +// These three tests pin each branch. + +describe("dispatchToBolt: ack-vs-processEvent race", () => { + test("long-running listener: handler returns 200 immediately, listener continues in background", async () => { + let releaseListener!: () => void; + const longWork = new Promise((r) => { + releaseListener = r; + }); + let listenerFinished = false; + + processEventOverride = async (event) => { + await event.ack(); // listener acks within Slack's window + await longWork; // then runs work that outlasts the ack budget + listenerFinished = true; + }; + + const channel = new SlackHttpChannel(baseConfig); + const req = makeReq({ + bodyJson: { type: "event_callback", team_id: TEAM_ID, event: { type: "app_mention" } }, + }); + + const start = Date.now(); + const res = await channel.handleEvent(req); + const elapsed = Date.now() - start; + + expect(res.status).toBe(200); + // Handler returned promptly even though the listener is still pending. + // 250ms gives plenty of slack on slow CI without admitting any + // regression that would actually block on `processEvent`. + expect(elapsed).toBeLessThan(250); + expect(listenerFinished).toBe(false); + + // Release the listener and verify it eventually finishes its work. + releaseListener(); + await lastProcessEventReturn; + expect(listenerFinished).toBe(true); + }); + + test("processEvent rejects before ack: handler returns 500 with operator log", async () => { + processEventOverride = async () => { + // Listener middleware fails before any listener invokes ack. + throw new Error("listener middleware blew up"); + }; + + const errors: string[] = []; + const original = console.error; + console.error = (...args: unknown[]) => { + errors.push(args.map(String).join(" ")); + }; + let res: Response; + try { + const channel = new SlackHttpChannel(baseConfig); + const req = makeReq({ + bodyJson: { type: "block_actions", team: { id: TEAM_ID, domain: "acme" } }, + contentType: "application/x-www-form-urlencoded", + path: "/slack/interactivity", + }); + res = await channel.handleInteractivity(req); + } finally { + console.error = original; + } + + expect(res.status).toBe(500); + const all = errors.join("\n"); + expect(all).toContain("processEvent rejected before ack"); + expect(all).toContain("listener middleware blew up"); + // We must NEVER log Slack body content; the message we logged + // carries only the listener's error string. + expect(all).not.toContain("block_actions"); + expect(all).not.toContain("acme"); + }); + + test("processEvent resolves without ack: handler returns 200 with operator warning (Bolt-bug fallback)", async () => { + processEventOverride = async () => { + // Hypothetical Bolt regression: listener pipeline finishes + // without anyone calling ack. We log + 200; Slack retries. + }; + + const warnings: string[] = []; + const original = console.warn; + console.warn = (...args: unknown[]) => { + warnings.push(args.map(String).join(" ")); + }; + let res: Response; + try { + const channel = new SlackHttpChannel(baseConfig); + const req = makeReq({ + bodyJson: { type: "event_callback", team_id: TEAM_ID, event: { type: "app_mention" } }, + }); + res = await channel.handleEvent(req); + } finally { + console.warn = original; + } + + expect(res.status).toBe(200); + expect(await res.text()).toBe(""); + const all = warnings.join("\n"); + expect(all).toContain("processEvent resolved without ack"); + }); +}); diff --git a/src/channels/slack-http-handlers.ts b/src/channels/slack-http-handlers.ts index e650e54..d4d6034 100644 --- a/src/channels/slack-http-handlers.ts +++ b/src/channels/slack-http-handlers.ts @@ -227,6 +227,13 @@ export type DispatchToBoltInput = DispatchInput & { retryReasonHeader?: string | null; }; +// Outcome of the ack-vs-processEvent race in `dispatchToBolt`. `ack`: +// a listener invoked AckFn (normal path). `process-ok`: processEvent +// resolved without any ack (Bolt regression; log + 200). `process-err`: +// processEvent rejected before ack (return 500; log the error message, +// never the Slack body). +type RaceOutcome = { kind: "ack"; resp: Response } | { kind: "process-ok" } | { kind: "process-err"; err: Error }; + // Run the verifier guard and dispatch the parsed body into the Bolt // `App`. `processEvent` invokes the registered listener middleware and // calls our AckFn when a listener acks; we return that response to @@ -252,21 +259,29 @@ export async function dispatchToBolt(input: DispatchToBoltInput): Promise = awaitAck.then((resp) => ({ kind: "ack", resp })); + const processTagged: Promise = input.app.processEvent(receiverEvent).then( + (): RaceOutcome => ({ kind: "process-ok" }), + (err: unknown): RaceOutcome => ({ + kind: "process-err", + err: err instanceof Error ? err : new Error(String(err)), + }), + ); - // Bolt 3.x auto-acks within 3s (processBeforeResponse=false default); - // we wait that promise here so the HTTP response carries the - // listener's chosen body when one exists. - return awaitAck; + const outcome = await Promise.race([ackTagged, processTagged]); + if (outcome.kind === "ack") return outcome.resp; + if (outcome.kind === "process-err") { + console.error(`[${HANDLER_LOG_TAG}] processEvent rejected before ack: ${outcome.err.message}`); + return new Response("internal server error", { status: 500 }); + } + console.warn(`[${HANDLER_LOG_TAG}] processEvent resolved without ack; returning empty 200`); + return EMPTY_OK(); } function parseRetryNum(value: string | null | undefined): number | undefined {