From 561c14c17cdada419ff14e0972997ff8a04f5cd4 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 24 Apr 2026 10:53:38 -0400 Subject: [PATCH 1/3] fix(admin): fail-closed on write-time channel privacy check (#3003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the pre-existing security gap flagged during PR #3000 review. Triaged by Claude Code in #3003; security-reviewer pre-cleared the approach. Before: all seven admin-settings PUT endpoints guarded is_private with `if (channelInfo && !channelInfo.is_private)`. When getChannelInfo returned null (bot not a member, Slack 5xx, throttle, archived channel, wrong scope), the && short-circuit silently skipped the check and the write was accepted. For the six private-required endpoints (billing/escalation/admin/ prospect/error/editorial), the downstream sendChannelMessage's `requirePrivate: true` gate at send time covered most exposure. The announcement endpoint inverts (requires public) and has no downstream gate, so a null return silently accepted a private channel id that would then never receive the public post. After: - New verifyChannelPrivacyForWrite(channelId, expected) in slack/client.ts returns {ok:true} | {reason:'wrong_privacy', actual, expected} | {reason:'cannot_verify'}. - All seven endpoints go through a shared requireChannelPrivacy helper in settings.ts that surfaces distinct error messages for each branch — "Could not verify … invite the bot" vs "Only private/public channels are allowed …". - Local-dev behavior unchanged: isSlackConfigured() short-circuits before the helper runs. Also hardens the announcement-backlog test to tolerate cross-file vi.mock state bleed under thread-pool parallelism: a default mockResolvedValue({rows: []}) prevents a prior-test leak from leaving mockQuery returning undefined on an uncovered call. Tests: 5 new verifyChannelPrivacyForWrite cases + 8 supertest route tests covering billing (private-required) and announcement (public- required) endpoints across all three outcomes + the Slack-not- configured short-circuit. 820/820 pass under the precommit runner. Co-Authored-By: Claude Opus 4.7 --- .changeset/channel-privacy-fail-closed.md | 45 ++++ server/src/routes/admin/settings.ts | 139 ++++++------- server/src/slack/client.ts | 56 +++++ .../unit/admin-settings-privacy-gate.test.ts | 196 ++++++++++++++++++ .../tests/unit/slack-channel-privacy.test.ts | 62 ++++++ .../announcement/announcement-backlog.test.ts | 5 + 6 files changed, 425 insertions(+), 78 deletions(-) create mode 100644 .changeset/channel-privacy-fail-closed.md create mode 100644 server/tests/unit/admin-settings-privacy-gate.test.ts diff --git a/.changeset/channel-privacy-fail-closed.md b/.changeset/channel-privacy-fail-closed.md new file mode 100644 index 0000000000..00d84eef8f --- /dev/null +++ b/.changeset/channel-privacy-fail-closed.md @@ -0,0 +1,45 @@ +--- +--- + +**fix(admin): fail-closed on write-time channel privacy check (#3003)** + +Closes the pre-existing security gap flagged during PR #3000 review. + +**Before:** all seven admin-settings PUT endpoints guarded their +`is_private` requirement with `if (channelInfo && !channelInfo.is_private)`. +When `getChannelInfo` returned `null` (bot not a member, Slack 5xx, +throttle, archived channel, wrong scope), the `channelInfo && ...` +short-circuit silently skipped the check and the write was accepted. + +- For the six private-required endpoints (billing, escalation, admin, + prospect, error, editorial), the downstream `sendChannelMessage(..., + { requirePrivate: true })` gate at send time covered most exposure. +- The announcement endpoint inverts the check (requires public) and + has no downstream gate, so a `null` return accepted a private + channel id that would silently never receive the public post. + +**After:** new `verifyChannelPrivacyForWrite(channelId, expected)` +helper in `slack/client.ts` returns a discriminated result: + +- `{ ok: true }` — proceed +- `{ ok: false, reason: 'wrong_privacy', actual, expected }` — + channel confirmed wrong kind; pick another channel +- `{ ok: false, reason: 'cannot_verify' }` — Slack can't describe + this channel; invite the bot and retry + +All seven endpoints now go through a `requireChannelPrivacy` wrapper in +`settings.ts` that surfaces distinct error messages for each branch so +the admin knows whether to pick another channel or retry after inviting +the bot. Local-dev behavior (no ADDIE_BOT_TOKEN) is unchanged — +`isSlackConfigured()` short-circuits. + +Closes #3003. + +**Tests:** five new `verifyChannelPrivacyForWrite` cases in +`slack-channel-privacy.test.ts` (ok both directions, wrong_privacy +both directions, cannot_verify both directions) plus eight +supertest route tests in the new +`admin-settings-privacy-gate.test.ts` covering the billing (private- +required) and announcement (public-required) endpoints across all +three outcomes + the Slack-not-configured short-circuit. Full +announcement + privacy suite 206/206 pass; server typecheck clean. diff --git a/server/src/routes/admin/settings.ts b/server/src/routes/admin/settings.ts index 7da5cfff7c..adb668845e 100644 --- a/server/src/routes/admin/settings.ts +++ b/server/src/routes/admin/settings.ts @@ -29,10 +29,54 @@ import { setAnnouncementChannel, getSettingAuditHistory, } from '../../db/system-settings-db.js'; -import { getSlackChannels, getChannelInfo, isSlackConfigured } from '../../slack/client.js'; +import { + getSlackChannels, + isSlackConfigured, + verifyChannelPrivacyForWrite, + type ChannelPrivacyCheckResult, +} from '../../slack/client.js'; const logger = createLogger('admin-settings'); +/** + * Write-time privacy check wrapper for the admin-settings PUT routes. + * Centralizes the "fail-closed on cannot_verify, emit a distinct + * message vs wrong_privacy" shape so each endpoint can stay one line. + * + * Skips the check entirely when Slack isn't configured (local dev + * without ADDIE_BOT_TOKEN) — matches the prior behavior. + * + * Returns `null` to mean "proceed with the write"; returns the Response + * directly (and sends it) when the check fails. Caller just returns. + */ +async function requireChannelPrivacy( + res: Response, + channelId: string, + expected: 'private' | 'public', + contextNoun: string, +): Promise { + if (!isSlackConfigured()) return null; + const check: ChannelPrivacyCheckResult = await verifyChannelPrivacyForWrite( + channelId, + expected, + ); + if (check.ok) return null; + if (check.reason === 'cannot_verify') { + return res.status(400).json({ + error: 'Could not verify channel', + message: `Could not verify the channel's privacy for ${contextNoun}. Invite Addie to the channel (and confirm the bot has channels:read scope) and try again.`, + }); + } + // wrong_privacy + return res.status(400).json({ + error: 'Invalid channel', + message: + expected === 'private' + ? `Only private channels are allowed for ${contextNoun}` + : `Announcement channel must be public — announcements are meant for broad visibility`, + }); +} + export function createAdminSettingsRouter(): Router { const router = Router(); @@ -126,17 +170,8 @@ export function createAdminSettingsRouter(): Router { return; } - // Verify the channel is private (billing info should not go to public channels) - if (isSlackConfigured()) { - const channelInfo = await getChannelInfo(channel_id); - if (channelInfo && !channelInfo.is_private) { - res.status(400).json({ - error: 'Invalid channel', - message: 'Only private channels are allowed for billing notifications', - }); - return; - } - } + const privacyErr = await requireChannelPrivacy(res, channel_id, 'private', 'billing notifications'); + if (privacyErr) return; } // Validate channel name if provided @@ -184,17 +219,8 @@ export function createAdminSettingsRouter(): Router { return; } - // Verify the channel is private (escalation info may contain sensitive user data) - if (isSlackConfigured()) { - const channelInfo = await getChannelInfo(channel_id); - if (channelInfo && !channelInfo.is_private) { - res.status(400).json({ - error: 'Invalid channel', - message: 'Only private channels are allowed for escalation notifications', - }); - return; - } - } + const privacyErr = await requireChannelPrivacy(res, channel_id, 'private', 'escalation notifications'); + if (privacyErr) return; } // Validate channel name if provided @@ -240,16 +266,8 @@ export function createAdminSettingsRouter(): Router { return; } - if (isSlackConfigured()) { - const channelInfo = await getChannelInfo(channel_id); - if (channelInfo && !channelInfo.is_private) { - res.status(400).json({ - error: 'Invalid channel', - message: 'Only private channels are allowed for admin notifications', - }); - return; - } - } + const privacyErr = await requireChannelPrivacy(res, channel_id, 'private', 'admin notifications'); + if (privacyErr) return; } if (channel_name !== null && channel_name !== undefined) { @@ -291,16 +309,8 @@ export function createAdminSettingsRouter(): Router { return; } - if (isSlackConfigured()) { - const channelInfo = await getChannelInfo(channel_id); - if (channelInfo && !channelInfo.is_private) { - res.status(400).json({ - error: 'Invalid channel', - message: 'Only private channels are allowed for prospect notifications', - }); - return; - } - } + const privacyErr = await requireChannelPrivacy(res, channel_id, 'private', 'prospect notifications'); + if (privacyErr) return; } if (channel_name !== null && channel_name !== undefined) { @@ -342,16 +352,8 @@ export function createAdminSettingsRouter(): Router { return; } - if (isSlackConfigured()) { - const channelInfo = await getChannelInfo(channel_id); - if (channelInfo && !channelInfo.is_private) { - res.status(400).json({ - error: 'Invalid channel', - message: 'Only private channels are allowed for error notifications', - }); - return; - } - } + const privacyErr = await requireChannelPrivacy(res, channel_id, 'private', 'error notifications'); + if (privacyErr) return; } if (channel_name !== null && channel_name !== undefined) { @@ -393,16 +395,8 @@ export function createAdminSettingsRouter(): Router { return; } - if (isSlackConfigured()) { - const channelInfo = await getChannelInfo(channel_id); - if (channelInfo && !channelInfo.is_private) { - res.status(400).json({ - error: 'Invalid channel', - message: 'Only private channels are allowed for editorial notifications', - }); - return; - } - } + const privacyErr = await requireChannelPrivacy(res, channel_id, 'private', 'editorial notifications'); + if (privacyErr) return; } if (channel_name !== null && channel_name !== undefined) { @@ -444,21 +438,10 @@ export function createAdminSettingsRouter(): Router { return; } - // The announcement channel is intentionally public — a private channel - // here would defeat the point of the welcome post. Reject private so - // nobody configures `#admin-editorial-review` as the announcement - // destination by mistake and floods a reviewer-only channel with - // member-facing posts. - if (isSlackConfigured()) { - const channelInfo = await getChannelInfo(channel_id); - if (channelInfo && channelInfo.is_private) { - res.status(400).json({ - error: 'Invalid channel', - message: 'Announcement channel must be public — announcements are meant for broad visibility', - }); - return; - } - } + // The announcement channel is intentionally public — a private + // channel here would defeat the point of the welcome post. + const privacyErr = await requireChannelPrivacy(res, channel_id, 'public', 'announcements'); + if (privacyErr) return; } if (channel_name !== null && channel_name !== undefined) { diff --git a/server/src/slack/client.ts b/server/src/slack/client.ts index fd44b8eaff..cca8da0b3b 100644 --- a/server/src/slack/client.ts +++ b/server/src/slack/client.ts @@ -453,6 +453,62 @@ export async function verifyChannelStillPrivate( return 'private'; } +/** + * Result of `verifyChannelPrivacyForWrite` — the write-time gate that + * admin settings endpoints use before persisting a channel id. + * + * - `{ ok: true }` — channel exists, privacy matches expectation. + * - `{ ok: false, reason: 'wrong_privacy', actual, expected }` — + * channel confirmed to be the wrong kind (private when public was + * required, or vice versa). Admin should pick a different channel. + * - `{ ok: false, reason: 'cannot_verify' }` — Slack returned nothing + * for this id (bot not a member, missing scope, transient 5xx, + * archived, or genuinely not found). The write is refused rather + * than accepting an unverifiable channel; admin should invite the + * bot to the channel and retry. + */ +export type ChannelPrivacyCheckResult = + | { ok: true } + | { + ok: false; + reason: 'wrong_privacy'; + actual: 'private' | 'public'; + expected: 'private' | 'public'; + } + | { ok: false; reason: 'cannot_verify' }; + +/** + * Verify a Slack channel's privacy matches `expected` *at write time*. + * Fail-closed on a null `getChannelInfo` so the write is refused with + * a "cannot verify" reason instead of silently accepting a channel id + * that Slack can't describe. Admin-settings PUT endpoints call this + * before persisting. + * + * Distinct from `verifyChannelStillPrivate`, which is the runtime + * pre-send check and logs a `channel_privacy_drift` event when a + * previously-private channel turns public (because drift is the + * interesting thing at send time; at write time it's not drift, it's + * an admin picking the wrong channel). + */ +export async function verifyChannelPrivacyForWrite( + channelId: string, + expected: 'private' | 'public', +): Promise { + const info = await getChannelInfo(channelId); + if (!info) { + logger.warn( + { channelId, expected, event: 'channel_privacy_verify_write_null' }, + 'Could not verify Slack channel privacy at write time — refusing with cannot_verify', + ); + return { ok: false, reason: 'cannot_verify' }; + } + const actual: 'private' | 'public' = info.is_private === true ? 'private' : 'public'; + if (actual !== expected) { + return { ok: false, reason: 'wrong_privacy', actual, expected }; + } + return { ok: true }; +} + /** * Reasons a `sendChannelMessage` call may refuse to post. Left as a * discriminated union so future skip reasons (archived, bot kicked, diff --git a/server/tests/unit/admin-settings-privacy-gate.test.ts b/server/tests/unit/admin-settings-privacy-gate.test.ts new file mode 100644 index 0000000000..497f3384fe --- /dev/null +++ b/server/tests/unit/admin-settings-privacy-gate.test.ts @@ -0,0 +1,196 @@ +/** + * Integration tests for the write-time privacy gate on admin-settings + * PUT routes (#3003). + * + * Exercises the 400 response shape for each of the three branches: + * - cannot_verify → distinct message telling admin to invite the bot + * - wrong_privacy (private required, channel is public) + * - wrong_privacy (public required, channel is private) + * + * Previously the cannot_verify branch was silently accepted. We only + * test two representative endpoints (billing = private-required, + * announcement = public-required) because all seven routes share the + * same `requireChannelPrivacy` helper — adding coverage for the other + * five would duplicate the helper test in `slack-channel-privacy.test.ts`. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import express from 'express'; +import request from 'supertest'; + +process.env.WORKOS_API_KEY = process.env.WORKOS_API_KEY ?? 'test'; +process.env.WORKOS_CLIENT_ID = process.env.WORKOS_CLIENT_ID ?? 'client_test'; + +const { + mockVerifyPrivacy, + mockIsSlackConfigured, + mockSetBillingChannel, + mockSetAnnouncementChannel, + mockGetBillingChannel, + mockGetAnnouncementChannel, +} = vi.hoisted(() => ({ + mockVerifyPrivacy: vi.fn(), + mockIsSlackConfigured: vi.fn(), + mockSetBillingChannel: vi.fn(), + mockSetAnnouncementChannel: vi.fn(), + mockGetBillingChannel: vi.fn(), + mockGetAnnouncementChannel: vi.fn(), +})); + +vi.mock('../../src/slack/client.js', () => ({ + isSlackConfigured: (...args: unknown[]) => mockIsSlackConfigured(...args), + verifyChannelPrivacyForWrite: (...args: unknown[]) => mockVerifyPrivacy(...args), + getSlackChannels: vi.fn(), +})); + +vi.mock('../../src/middleware/auth.js', () => ({ + requireAuth: (req: any, _res: any, next: any) => { + req.user = { id: 'user_admin_01', email: 'admin@test', is_admin: true }; + next(); + }, + requireAdmin: (_req: any, _res: any, next: any) => next(), +})); + +vi.mock('../../src/db/system-settings-db.js', () => ({ + getAllSettings: vi.fn().mockResolvedValue([]), + getBillingChannel: (...args: unknown[]) => mockGetBillingChannel(...args), + setBillingChannel: (...args: unknown[]) => mockSetBillingChannel(...args), + getEscalationChannel: vi.fn().mockResolvedValue({ channel_id: null, channel_name: null }), + setEscalationChannel: vi.fn(), + getAdminChannel: vi.fn().mockResolvedValue({ channel_id: null, channel_name: null }), + setAdminChannel: vi.fn(), + getProspectChannel: vi.fn().mockResolvedValue({ channel_id: null, channel_name: null }), + setProspectChannel: vi.fn(), + getProspectTriageEnabled: vi.fn().mockResolvedValue(true), + setProspectTriageEnabled: vi.fn(), + getErrorChannel: vi.fn().mockResolvedValue({ channel_id: null, channel_name: null }), + setErrorChannel: vi.fn(), + getEditorialChannel: vi.fn().mockResolvedValue({ channel_id: null, channel_name: null }), + setEditorialChannel: vi.fn(), + getAnnouncementChannel: (...args: unknown[]) => mockGetAnnouncementChannel(...args), + setAnnouncementChannel: (...args: unknown[]) => mockSetAnnouncementChannel(...args), +})); + +async function buildApp() { + const { createAdminSettingsRouter } = await import('../../src/routes/admin/settings.js'); + const app = express(); + app.use(express.json()); + app.use('/api/admin/settings', createAdminSettingsRouter()); + return app; +} + +beforeEach(() => { + vi.clearAllMocks(); + mockIsSlackConfigured.mockReturnValue(true); + mockSetBillingChannel.mockResolvedValue(undefined); + mockSetAnnouncementChannel.mockResolvedValue(undefined); + mockGetBillingChannel.mockResolvedValue({ + channel_id: 'C_BILLING', + channel_name: 'billing', + }); + mockGetAnnouncementChannel.mockResolvedValue({ + channel_id: 'C_ANNOUNCE', + channel_name: 'all-agentic-ads', + }); +}); + +describe('PUT /api/admin/settings/billing-channel — privacy gate (#3003)', () => { + it('accepts when channel is confirmed private', async () => { + mockVerifyPrivacy.mockResolvedValueOnce({ ok: true }); + const app = await buildApp(); + const res = await request(app) + .put('/api/admin/settings/billing-channel') + .send({ channel_id: 'C1234567', channel_name: 'billing' }); + expect(res.status).toBe(200); + expect(mockSetBillingChannel).toHaveBeenCalled(); + expect(mockVerifyPrivacy).toHaveBeenCalledWith('C1234567', 'private'); + }); + + it('rejects on cannot_verify with a distinct "invite the bot" message', async () => { + // Previously: `channelInfo && !channelInfo.is_private` short- + // circuited to `false` on null → write accepted silently. + mockVerifyPrivacy.mockResolvedValueOnce({ ok: false, reason: 'cannot_verify' }); + const app = await buildApp(); + const res = await request(app) + .put('/api/admin/settings/billing-channel') + .send({ channel_id: 'C1234567', channel_name: 'billing' }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Could not verify channel'); + expect(res.body.message).toMatch(/Invite Addie/i); + expect(mockSetBillingChannel).not.toHaveBeenCalled(); + }); + + it('rejects on wrong_privacy (public channel for private-required endpoint)', async () => { + mockVerifyPrivacy.mockResolvedValueOnce({ + ok: false, + reason: 'wrong_privacy', + actual: 'public', + expected: 'private', + }); + const app = await buildApp(); + const res = await request(app) + .put('/api/admin/settings/billing-channel') + .send({ channel_id: 'C1234567', channel_name: 'billing' }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Invalid channel'); + expect(res.body.message).toMatch(/Only private channels.*billing/i); + expect(mockSetBillingChannel).not.toHaveBeenCalled(); + }); + + it('skips the check when Slack is not configured (local dev)', async () => { + mockIsSlackConfigured.mockReturnValue(false); + const app = await buildApp(); + const res = await request(app) + .put('/api/admin/settings/billing-channel') + .send({ channel_id: 'C1234567', channel_name: 'billing' }); + expect(res.status).toBe(200); + expect(mockVerifyPrivacy).not.toHaveBeenCalled(); + expect(mockSetBillingChannel).toHaveBeenCalled(); + }); +}); + +describe('PUT /api/admin/settings/announcement-channel — privacy gate (#3003)', () => { + it('accepts when channel is confirmed public', async () => { + mockVerifyPrivacy.mockResolvedValueOnce({ ok: true }); + const app = await buildApp(); + const res = await request(app) + .put('/api/admin/settings/announcement-channel') + .send({ channel_id: 'C1234567', channel_name: 'all-agentic-ads' }); + expect(res.status).toBe(200); + expect(mockVerifyPrivacy).toHaveBeenCalledWith('C1234567', 'public'); + expect(mockSetAnnouncementChannel).toHaveBeenCalled(); + }); + + it('rejects on cannot_verify — the inverted-direction endpoint had no downstream gate', async () => { + // Private-required endpoints had a send-time backstop + // (`requirePrivate: true` on `sendChannelMessage`). The + // announcement endpoint inverts: it requires public. A null + // getChannelInfo used to let a *private* channel id through + // here, which would silently never post. This test pins the + // write-time rejection. + mockVerifyPrivacy.mockResolvedValueOnce({ ok: false, reason: 'cannot_verify' }); + const app = await buildApp(); + const res = await request(app) + .put('/api/admin/settings/announcement-channel') + .send({ channel_id: 'C1234567', channel_name: 'announce' }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Could not verify channel'); + expect(mockSetAnnouncementChannel).not.toHaveBeenCalled(); + }); + + it('rejects on wrong_privacy (private channel for public-required endpoint)', async () => { + mockVerifyPrivacy.mockResolvedValueOnce({ + ok: false, + reason: 'wrong_privacy', + actual: 'private', + expected: 'public', + }); + const app = await buildApp(); + const res = await request(app) + .put('/api/admin/settings/announcement-channel') + .send({ channel_id: 'C1234567', channel_name: 'announce' }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Invalid channel'); + expect(res.body.message).toMatch(/must be public/i); + expect(mockSetAnnouncementChannel).not.toHaveBeenCalled(); + }); +}); diff --git a/server/tests/unit/slack-channel-privacy.test.ts b/server/tests/unit/slack-channel-privacy.test.ts index a8f345d66e..025dd840c6 100644 --- a/server/tests/unit/slack-channel-privacy.test.ts +++ b/server/tests/unit/slack-channel-privacy.test.ts @@ -10,6 +10,7 @@ vi.hoisted(() => { import { sendChannelMessage, verifyChannelStillPrivate, + verifyChannelPrivacyForWrite, __resetChannelCacheForTests, } from '../../src/slack/client.js'; @@ -187,3 +188,64 @@ describe('sendChannelMessage({ requirePrivate: "strict-public-only" })', () => { expect(postedMessages).toHaveLength(0); }); }); + +/** + * #3003 — write-time privacy check. The 7 admin settings PUT routes + * previously had a silent fail-open on `getChannelInfo` null: if Slack + * couldn't describe the channel (bot not a member, missing scope, + * transient 5xx), the check was skipped and the write was accepted. + * For private-required endpoints the downstream send-time gate + * covered most cases, but the announcement endpoint (inverted: + * require public) had no downstream gate and would silently misroute. + * + * `verifyChannelPrivacyForWrite` now fails closed on null with a + * distinct `cannot_verify` reason so the caller can render a distinct + * message ("invite the bot, retry") vs `wrong_privacy` ("pick + * another channel"). + */ +describe('verifyChannelPrivacyForWrite', () => { + it('expected=private, channel is private → ok', async () => { + channelInfoResponses.set('C_p', { id: 'C_p', name: 'priv', is_private: true }); + expect(await verifyChannelPrivacyForWrite('C_p', 'private')).toEqual({ ok: true }); + }); + + it('expected=public, channel is public → ok', async () => { + channelInfoResponses.set('C_pub', { id: 'C_pub', name: 'pub', is_private: false }); + expect(await verifyChannelPrivacyForWrite('C_pub', 'public')).toEqual({ ok: true }); + }); + + it('expected=private, channel is public → wrong_privacy with actual/expected', async () => { + channelInfoResponses.set('C_wrong', { id: 'C_wrong', name: 'wrong', is_private: false }); + expect(await verifyChannelPrivacyForWrite('C_wrong', 'private')).toEqual({ + ok: false, + reason: 'wrong_privacy', + actual: 'public', + expected: 'private', + }); + }); + + it('expected=public, channel is private → wrong_privacy with actual/expected', async () => { + channelInfoResponses.set('C_wrong2', { id: 'C_wrong2', name: 'wrong2', is_private: true }); + expect(await verifyChannelPrivacyForWrite('C_wrong2', 'public')).toEqual({ + ok: false, + reason: 'wrong_privacy', + actual: 'private', + expected: 'public', + }); + }); + + it('cannot resolve the channel → cannot_verify (fail-closed)', async () => { + // Previously this path silently accepted the write. The new + // contract is: refuse with cannot_verify so the admin knows to + // invite the bot and retry rather than saving an unverifiable + // channel id that might misroute at send time. + expect(await verifyChannelPrivacyForWrite('C_not_found', 'private')).toEqual({ + ok: false, + reason: 'cannot_verify', + }); + expect(await verifyChannelPrivacyForWrite('C_not_found', 'public')).toEqual({ + ok: false, + reason: 'cannot_verify', + }); + }); +}); diff --git a/tests/announcement/announcement-backlog.test.ts b/tests/announcement/announcement-backlog.test.ts index 06dce21c85..7e7e0fb224 100644 --- a/tests/announcement/announcement-backlog.test.ts +++ b/tests/announcement/announcement-backlog.test.ts @@ -22,6 +22,11 @@ vi.mock('../../server/src/db/client.js', () => ({ beforeEach(() => { vi.clearAllMocks(); mockQuery.mockReset(); + // Safer default — without this a `mockResolvedValueOnce` consumed + // by a prior test's overlap would leave a subsequent call returning + // `undefined` (→ TypeError or timeout). Tests that care always + // stack their own mockResolvedValueOnce on top. + mockQuery.mockResolvedValue({ rows: [] }); }); describe('loadAnnouncementBacklog', () => { From 788f25bff779f7aba5e0b7ffbfa740ccafaebbe4 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 24 Apr 2026 11:36:21 -0400 Subject: [PATCH 2/3] chore(workflow-b): drop SLACK_EDITORIAL_REVIEW_CHANNEL env fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prod has the editorial_slack_channel system setting configured via the admin UI that shipped in PR #3000, so the env-var fallback path in resolveEditorialChannel() is dead weight. Keeping a stale env-var fallback would silently mask a misconfigured DB value on future deploys — the right failure mode is "log + skip" so SRE gets a signal, not "quietly use a leftover env setting". - Drop the env read from resolveEditorialChannel(). - On DB read failure: log at error level and return null (instead of falling back to env). - Backfill script header and usage string no longer mention the env var; the pre-run error message points to /admin/settings. Tests rewritten: the env-fallback cases that previously existed are replaced with "env var is ignored" assertions so a stale SLACK_EDITORIAL_REVIEW_CHANNEL in a test environment does not affect the resolver's return value. 169/169 announcement tests pass. Co-Authored-By: Claude Opus 4.7 --- .changeset/channel-privacy-fail-closed.md | 15 +-- .changeset/triage-ship-more.md | 2 +- server/public/admin-settings.html | 5 - server/src/addie/jobs/announcement-trigger.ts | 23 ++-- .../scripts/backfill-member-announcements.ts | 16 ++- .../announcement-channel-resolver.test.ts | 103 +++++++----------- 6 files changed, 70 insertions(+), 94 deletions(-) diff --git a/.changeset/channel-privacy-fail-closed.md b/.changeset/channel-privacy-fail-closed.md index 00d84eef8f..4d2b7171cf 100644 --- a/.changeset/channel-privacy-fail-closed.md +++ b/.changeset/channel-privacy-fail-closed.md @@ -12,8 +12,9 @@ throttle, archived channel, wrong scope), the `channelInfo && ...` short-circuit silently skipped the check and the write was accepted. - For the six private-required endpoints (billing, escalation, admin, - prospect, error, editorial), the downstream `sendChannelMessage(..., - { requirePrivate: true })` gate at send time covered most exposure. + prospect, error, editorial), the downstream + `sendChannelMessage(..., requirePrivate: true)` gate at send time + covered most exposure. - The announcement endpoint inverts the check (requires public) and has no downstream gate, so a `null` return accepted a private channel id that would silently never receive the public post. @@ -21,11 +22,11 @@ short-circuit silently skipped the check and the write was accepted. **After:** new `verifyChannelPrivacyForWrite(channelId, expected)` helper in `slack/client.ts` returns a discriminated result: -- `{ ok: true }` — proceed -- `{ ok: false, reason: 'wrong_privacy', actual, expected }` — - channel confirmed wrong kind; pick another channel -- `{ ok: false, reason: 'cannot_verify' }` — Slack can't describe - this channel; invite the bot and retry +- `ok: true` — proceed +- `ok: false, reason: 'wrong_privacy'` — channel confirmed wrong kind; + pick another channel (response includes actual and expected) +- `ok: false, reason: 'cannot_verify'` — Slack can't describe this + channel; invite the bot and retry All seven endpoints now go through a `requireChannelPrivacy` wrapper in `settings.ts` that surfaces distinct error messages for each branch so diff --git a/.changeset/triage-ship-more.md b/.changeset/triage-ship-more.md index 2b6ba3a2c3..7cddbb3ee3 100644 --- a/.changeset/triage-ship-more.md +++ b/.changeset/triage-ship-more.md @@ -1,4 +1,4 @@ --- --- -Flip the triage routine's default from "flag unless obviously a small bug" to "execute unless the change is breaking, ambiguous, or high-risk." Drops the <150-line scope cap, the classification-only-Bug-or-Doc gate, and the blanket prohibition on `static/schemas/source/**` edits. Adds a crisp non-breaking-vs-breaking definition as the primary Execute/Flag binary, adds evergreen content as a first-class always-PR-able bucket, and guards `infra/agents` bucket as always-Flag (self-modification risk). CODEOWNERS + human review still gate every merge. +Flip the triage routine's default from "flag unless obviously a small bug" to "execute unless the change is breaking, ambiguous, or high-risk." Drops the under-150-line scope cap, the classification-only-Bug-or-Doc gate, and the blanket prohibition on `static/schemas/source/**` edits. Adds a crisp non-breaking-vs-breaking definition as the primary Execute/Flag binary, adds evergreen content as a first-class always-PR-able bucket, and guards `infra/agents` bucket as always-Flag (self-modification risk). CODEOWNERS + human review still gate every merge. diff --git a/server/public/admin-settings.html b/server/public/admin-settings.html index af4a21f05e..8791cd974e 100644 --- a/server/public/admin-settings.html +++ b/server/public/admin-settings.html @@ -903,11 +903,6 @@

Recent changes

} // Update the current editorial channel display. - // NB: we don't claim the feature is "disabled" when unset — the - // server can still fall back to SLACK_EDITORIAL_REVIEW_CHANNEL env - // var, and we don't know from the client whether that's set. "Not - // configured" is the honest label; the admin can reason about - // what that means for their deployment. function updateCurrentEditorialChannelDisplay() { const el = document.getElementById('currentEditorialChannel'); const channel = currentSettings?.editorial_channel; diff --git a/server/src/addie/jobs/announcement-trigger.ts b/server/src/addie/jobs/announcement-trigger.ts index 476aedad6d..07ebd259dd 100644 --- a/server/src/addie/jobs/announcement-trigger.ts +++ b/server/src/addie/jobs/announcement-trigger.ts @@ -418,15 +418,15 @@ async function processAnnounceCandidate( } /** - * Resolve the editorial review channel. Prefers the admin-UI DB setting - * (`editorial_slack_channel`), falls back to the legacy - * `SLACK_EDITORIAL_REVIEW_CHANNEL` env var for safe rollout. Both null - * returns `null`; callers should skip the run and log. + * Resolve the editorial review channel from the admin-UI DB setting + * (`editorial_slack_channel`). Returns `null` when the setting is + * unset or when the DB read fails; callers skip the run and log. * - * Logs at error level when the DB read fails — the env fallback keeps - * the job running for transient blips, but a persistent outage where - * the admin's DB value is stale (or wrong) needs SRE attention, not a - * buried warn. + * The `SLACK_EDITORIAL_REVIEW_CHANNEL` env var used to be a fallback + * path for the safe-rollout window after the env→DB migration landed + * (PR #3000). Prod now has the DB value set, so the env fallback is + * dropped — a stale env var in a future deploy would otherwise mask + * a misconfigured DB value rather than failing loudly. */ export async function resolveEditorialChannel(): Promise { try { @@ -435,10 +435,9 @@ export async function resolveEditorialChannel(): Promise { return setting.channel_id.trim(); } } catch (err) { - logger.error({ err }, 'resolveEditorialChannel: DB read failed, falling back to env'); + logger.error({ err }, 'resolveEditorialChannel: DB read failed — editorial channel unavailable'); } - const env = process.env.SLACK_EDITORIAL_REVIEW_CHANNEL; - return env && env.trim() ? env.trim() : null; + return null; } export async function runAnnouncementTriggerJob(): Promise { @@ -447,7 +446,7 @@ export async function runAnnouncementTriggerJob(): Promise { const reviewChannel = await resolveEditorialChannel(); if (!reviewChannel) { logger.warn( - 'Editorial channel not configured — set it at /admin/settings (or SLACK_EDITORIAL_REVIEW_CHANNEL env) — skipping run', + 'Editorial channel not configured — set it at /admin/settings — skipping run', ); return result; } diff --git a/server/src/scripts/backfill-member-announcements.ts b/server/src/scripts/backfill-member-announcements.ts index 8875e8d7b5..05c7f9a0cf 100644 --- a/server/src/scripts/backfill-member-announcements.ts +++ b/server/src/scripts/backfill-member-announcements.ts @@ -31,11 +31,14 @@ * ## Env * * DATABASE_URL required - * SLACK_EDITORIAL_REVIEW_CHANNEL required unless --dry-run * APP_URL optional, used in profile links * ADDIE_BOT_TOKEN required for non-dry-run posts * ANTHROPIC_API_KEY required for the drafter * + * The editorial review channel is read from the `editorial_slack_channel` + * system setting (set via /admin/settings). A non-dry-run exits with + * an error if that setting is unconfigured. + * * Prod-admin-only: whoever can run this has shell access, the Addie * bot token, and Anthropic billing. No finer-grained authz in-band. */ @@ -105,9 +108,11 @@ Usage: npx tsx server/src/scripts/backfill-member-announcements.ts [options] Env: DATABASE_URL (required) - SLACK_EDITORIAL_REVIEW_CHANNEL (required unless --dry-run) ADDIE_BOT_TOKEN (required unless --dry-run) ANTHROPIC_API_KEY (required unless --dry-run) + +The editorial review channel is read from the editorial_slack_channel +system setting. Configure it at /admin/settings. `; function formatPreviewRow(r: BackfillPreviewRow): string { @@ -149,12 +154,13 @@ async function main(): Promise { initializeDatabase(dbConfig); - // DB first (admin UI), env fallback. Only required on live runs — - // dry-run just wants to enumerate candidates. + // Resolve from the admin-UI `editorial_slack_channel` setting. + // Dry-run doesn't post anywhere, so it tolerates an unconfigured + // channel and just enumerates candidates. const reviewChannel = (await resolveEditorialChannel()) ?? ''; if (!args.dryRun && !reviewChannel) { console.error( - 'Editorial channel not configured. Set it at /admin/settings (or SLACK_EDITORIAL_REVIEW_CHANNEL env).', + 'Editorial channel not configured. Set it at /admin/settings.', ); await closeDatabase(); process.exit(1); diff --git a/tests/announcement/announcement-channel-resolver.test.ts b/tests/announcement/announcement-channel-resolver.test.ts index 1a0fc78b37..5cc941a206 100644 --- a/tests/announcement/announcement-channel-resolver.test.ts +++ b/tests/announcement/announcement-channel-resolver.test.ts @@ -1,15 +1,14 @@ /** - * Tests for `resolveEditorialChannel`: DB-first with env var fallback. + * Tests for `resolveEditorialChannel`: admin-UI DB setting only. * - * The resolver is the narrow seam that lets Stage 1 + backfill switch - * from the legacy SLACK_EDITORIAL_REVIEW_CHANNEL env var to the admin-UI - * `editorial_slack_channel` system setting without breaking existing - * prod config. Behavior: - * - DB setting populated → use it - * - DB setting null → fall back to env - * - DB setting null AND env unset/empty → return null; caller skips - * - DB read throws → fall back to env (don't block the job on a - * transient DB read) + * - DB setting populated → return the trimmed channel id + * - DB setting null/empty/whitespace → return null (caller skips) + * - DB read throws → return null (log at error level) + * + * The `SLACK_EDITORIAL_REVIEW_CHANNEL` env var used to be a fallback + * path during the env→DB migration window (PR #3000). Prod now has + * the DB value set, so the env fallback was dropped. Setting the env + * var in a test here must NOT affect the resolver's return value. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; @@ -21,9 +20,8 @@ vi.mock('../../server/src/db/system-settings-db.js', () => ({ getEditorialChannel: (...args: unknown[]) => mockGetEditorialChannel(...args), })); -// The module-under-test imports slack/client + visual + drafter + DB -// client transitively. We stub everything we don't need so module init -// is a no-op. +// Module-under-test imports slack/client + visual + drafter + DB client +// transitively. Stub everything we don't need so module init is a no-op. vi.mock('../../server/src/db/client.js', () => ({ query: vi.fn(), getPool: () => ({ connect: async () => ({ query: vi.fn(), release: () => {} }) }), @@ -54,83 +52,51 @@ afterEach(() => { }); describe('resolveEditorialChannel', () => { - it('returns the DB setting when configured (preferred over env)', async () => { + it('returns the DB setting when configured', async () => { mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: 'C0FROMDB01', channel_name: 'admin-editorial-review', }); - process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = 'C0FROMENV01'; - - const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); - const resolved = await resolveEditorialChannel(); - expect(resolved).toBe('C0FROMDB01'); - }); - - it('falls back to env var when the DB setting is null', async () => { - mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: null, channel_name: null }); - process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = 'C0FROMENV01'; const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); - expect(await resolveEditorialChannel()).toBe('C0FROMENV01'); + expect(await resolveEditorialChannel()).toBe('C0FROMDB01'); }); - it('returns null when both DB and env are unset', async () => { - mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: null, channel_name: null }); - delete process.env.SLACK_EDITORIAL_REVIEW_CHANNEL; + it('env var is ignored when DB setting is populated', async () => { + // The env fallback was dropped; prod now relies on the admin UI. + mockGetEditorialChannel.mockResolvedValueOnce({ + channel_id: 'C0FROMDB01', + channel_name: 'admin-editorial-review', + }); + process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = 'C0STALEENVCHANNEL'; const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); - expect(await resolveEditorialChannel()).toBeNull(); + expect(await resolveEditorialChannel()).toBe('C0FROMDB01'); }); - it('treats empty string env as unset', async () => { + it('returns null when DB setting is unset (env var is not a fallback)', async () => { mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: null, channel_name: null }); - process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = ''; + // Previously this would fall back to the env var. After #3000 rollout + // completed, a stale env var would otherwise mask a misconfigured DB. + process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = 'C0STALEENVCHANNEL'; const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); expect(await resolveEditorialChannel()).toBeNull(); }); - it('treats whitespace-only env as unset', async () => { - mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: null, channel_name: null }); - process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = ' '; - + it('treats empty-string DB channel_id as unset', async () => { + mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: '', channel_name: null }); const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); expect(await resolveEditorialChannel()).toBeNull(); }); - it('trims whitespace from env fallback', async () => { - mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: null, channel_name: null }); - process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = ' C0FROMENV01 '; - - const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); - expect(await resolveEditorialChannel()).toBe('C0FROMENV01'); - }); - - it('falls back to env when the DB read throws (transient failures should not block the job)', async () => { - mockGetEditorialChannel.mockRejectedValueOnce(new Error('db connection reset')); - process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = 'C0FROMENV01'; - - const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); - expect(await resolveEditorialChannel()).toBe('C0FROMENV01'); - }); - - it('returns null when DB throws and env unset (safe no-op for the caller)', async () => { - mockGetEditorialChannel.mockRejectedValueOnce(new Error('db connection reset')); - delete process.env.SLACK_EDITORIAL_REVIEW_CHANNEL; - - const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); - expect(await resolveEditorialChannel()).toBeNull(); - }); - - it('treats whitespace-only DB channel_id as unset (symmetric with env)', async () => { + it('treats whitespace-only DB channel_id as unset', async () => { mockGetEditorialChannel.mockResolvedValueOnce({ channel_id: ' ', channel_name: 'whatever', }); - process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = 'C0FROMENV01'; - const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); - expect(await resolveEditorialChannel()).toBe('C0FROMENV01'); + expect(await resolveEditorialChannel()).toBeNull(); }); it('trims whitespace from DB channel_id', async () => { @@ -138,8 +104,17 @@ describe('resolveEditorialChannel', () => { channel_id: ' C0FROMDB01 ', channel_name: 'editorial', }); - const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); expect(await resolveEditorialChannel()).toBe('C0FROMDB01'); }); + + it('returns null when DB read throws (logs at error level)', async () => { + mockGetEditorialChannel.mockRejectedValueOnce(new Error('db connection reset')); + // Even with env set, a DB failure no longer silently activates a + // fallback — the job skips and logs instead. + process.env.SLACK_EDITORIAL_REVIEW_CHANNEL = 'C0STALEENVCHANNEL'; + + const { resolveEditorialChannel } = await import('../../server/src/addie/jobs/announcement-trigger.js'); + expect(await resolveEditorialChannel()).toBeNull(); + }); }); From f5220a67163ef77442b1a1aeded71feb6c1d4017 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 24 Apr 2026 12:12:24 -0400 Subject: [PATCH 3/3] harden(admin-channel-privacy): DX + testing review follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies the three worth-now findings from the DX + testing-expert pass on PR #3039. Picker pre-filter (dx-expert: #3003 UX gap): - GET /api/admin/settings/slack-channels now drops channels where is_member is false. Before: public-channel picker would list every public channel in the workspace, including ones the bot isn't in — a "valid" pick would then hit cannot_verify at save time with the new write-side gate. Shifting the filter to the read side keeps the dropdown honest. - Missing is_member treated as still-a-member (conservative default, correct for private-channel listings where Slack's conversations.list omits the field because it only surfaces bot-member privates). Error-copy rewrite (dx-expert): - "channels:read scope" jargon in the cannot_verify message replaced with plain-English: "Invite @Addie to the channel in Slack and save again. If that doesn't work, an AAO engineer may need to re-grant the bot's channel permissions." AAO admins who didn't install the app don't know Slack scope names. Invariant lint test (nodejs-testing-expert): - New test asserts every channel-setting PUT handler in admin/settings.ts that destructures { channel_id, channel_name } also calls requireChannelPrivacy. A future eighth channel endpoint that forgets the gate would be a silent regression of #3003 — per-endpoint supertest duplication for all seven is low-signal, but one grep-style test catches the omission. Also fixes the existing cannot_verify test regex (Invite Addie → Invite @Addie) to match the new copy. 10/10 privacy-gate tests pass. Server typecheck clean. Deferred from the review (genuine follow-ups, not this PR): - Pool isolation for tests/announcement/ (larger vitest config change; backlog test's mockResolvedValue default stays as a band-aid for now) - Break-glass runbook for DB-only channel rotation during incidents (adtech-product-expert flagged; docs/ops work) - Slack deep-link / bot-ID copy button in the admin UI error state (dx-expert suggested; UX work) Co-Authored-By: Claude Opus 4.7 --- server/src/routes/admin/settings.ts | 14 +++- .../unit/admin-settings-privacy-gate.test.ts | 73 ++++++++++++++++++- 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/server/src/routes/admin/settings.ts b/server/src/routes/admin/settings.ts index adb668845e..64ec89c456 100644 --- a/server/src/routes/admin/settings.ts +++ b/server/src/routes/admin/settings.ts @@ -64,7 +64,7 @@ async function requireChannelPrivacy( if (check.reason === 'cannot_verify') { return res.status(400).json({ error: 'Could not verify channel', - message: `Could not verify the channel's privacy for ${contextNoun}. Invite Addie to the channel (and confirm the bot has channels:read scope) and try again.`, + message: `Could not verify the channel for ${contextNoun}. Invite @Addie to the channel in Slack and save again. If that doesn't work, an AAO engineer may need to re-grant the bot's channel permissions.`, }); } // wrong_privacy @@ -135,8 +135,18 @@ export function createAdminSettingsRouter(): Router { exclude_archived: true, }); + // Pre-filter to channels the bot is actually a member of. For + // private types Slack already only returns bot-member channels, + // so this matters mostly for public — without it the announcement + // picker would list every public channel in the workspace and + // picking a non-member would hit `verifyChannelPrivacyForWrite` + // cannot_verify at save time. Match the write-side gate at the + // read-side so the only save-time errors are genuine drift, not + // "you picked something you shouldn't have been offered." + const memberOnly = channels.filter((c) => c.is_member !== false); + // Sort by name and return minimal info - const sorted = channels + const sorted = memberOnly .map(c => ({ id: c.id, name: c.name, diff --git a/server/tests/unit/admin-settings-privacy-gate.test.ts b/server/tests/unit/admin-settings-privacy-gate.test.ts index 497f3384fe..0938bd3205 100644 --- a/server/tests/unit/admin-settings-privacy-gate.test.ts +++ b/server/tests/unit/admin-settings-privacy-gate.test.ts @@ -27,6 +27,7 @@ const { mockSetAnnouncementChannel, mockGetBillingChannel, mockGetAnnouncementChannel, + mockGetSlackChannels, } = vi.hoisted(() => ({ mockVerifyPrivacy: vi.fn(), mockIsSlackConfigured: vi.fn(), @@ -34,12 +35,13 @@ const { mockSetAnnouncementChannel: vi.fn(), mockGetBillingChannel: vi.fn(), mockGetAnnouncementChannel: vi.fn(), + mockGetSlackChannels: vi.fn(), })); vi.mock('../../src/slack/client.js', () => ({ isSlackConfigured: (...args: unknown[]) => mockIsSlackConfigured(...args), verifyChannelPrivacyForWrite: (...args: unknown[]) => mockVerifyPrivacy(...args), - getSlackChannels: vi.fn(), + getSlackChannels: (...args: unknown[]) => mockGetSlackChannels(...args), })); vi.mock('../../src/middleware/auth.js', () => ({ @@ -115,7 +117,7 @@ describe('PUT /api/admin/settings/billing-channel — privacy gate (#3003)', () .send({ channel_id: 'C1234567', channel_name: 'billing' }); expect(res.status).toBe(400); expect(res.body.error).toBe('Could not verify channel'); - expect(res.body.message).toMatch(/Invite Addie/i); + expect(res.body.message).toMatch(/Invite @Addie/i); expect(mockSetBillingChannel).not.toHaveBeenCalled(); }); @@ -194,3 +196,70 @@ describe('PUT /api/admin/settings/announcement-channel — privacy gate (#3003)' expect(mockSetAnnouncementChannel).not.toHaveBeenCalled(); }); }); + +describe('GET /api/admin/settings/slack-channels — is_member pre-filter', () => { + // Without this filter, the public-channel picker (used by the + // announcement setting) would list every public channel in the + // workspace — including ones the bot isn't a member of — and a + // "valid" pick would then hit cannot_verify at save time. Shifting + // the filter to the read side keeps the dropdown honest. + it('drops channels where is_member is false', async () => { + mockGetSlackChannels.mockResolvedValueOnce([ + { id: 'C_IN', name: 'all-agentic-ads', is_private: false, is_member: true, num_members: 42 }, + { id: 'C_OUT', name: 'random-public', is_private: false, is_member: false, num_members: 99 }, + ]); + const app = await buildApp(); + const res = await request(app).get('/api/admin/settings/slack-channels?visibility=public'); + expect(res.status).toBe(200); + expect(res.body.channels).toEqual([ + expect.objectContaining({ id: 'C_IN', name: 'all-agentic-ads' }), + ]); + expect(res.body.channels).not.toContainEqual(expect.objectContaining({ id: 'C_OUT' })); + }); + + it('treats missing is_member as still-a-member (conservative default)', async () => { + // Private-channel listings don't return is_member at all because + // conversations.list only surfaces private channels the bot is + // already in. Don't filter those out based on an absent field. + mockGetSlackChannels.mockResolvedValueOnce([ + { id: 'C_PRIV', name: 'admin-editorial-review', is_private: true }, + ]); + const app = await buildApp(); + const res = await request(app).get('/api/admin/settings/slack-channels'); + expect(res.body.channels).toEqual([ + expect.objectContaining({ id: 'C_PRIV' }), + ]); + }); +}); + +/** + * Invariant lint: every PUT handler on admin/settings.ts that accepts + * a `channel_id` MUST go through `requireChannelPrivacy`. A new + * channel-setting endpoint that forgets the gate would be a silent + * regression of #3003 — adding per-endpoint supertest coverage for + * all seven routes is low-signal duplication, but pinning the + * invariant in one grep-style test catches a future omission. + */ +describe('invariant: channel-setting PUT handlers call requireChannelPrivacy', () => { + it('every PUT handler that reads channel_id also goes through the privacy gate', async () => { + const { readFileSync } = await import('node:fs'); + const source = readFileSync( + new URL('../../src/routes/admin/settings.ts', import.meta.url), + 'utf8', + ); + // Split on `router.put(` — each section is one handler body. The + // prospect-triage-enabled handler also uses `router.put` but + // doesn't take a channel_id; we detect channel handlers by a + // `{ channel_id, channel_name }` destructure and require that + // `requireChannelPrivacy` appears in the same block. + const handlers = source.split(/router\.put\(/).slice(1); + const channelHandlers = handlers.filter((h) => + h.slice(0, 2000).includes('{ channel_id, channel_name }'), + ); + expect(channelHandlers.length).toBe(7); + for (const body of channelHandlers) { + // Expected shape: `const privacyErr = await requireChannelPrivacy(...)` + expect(body.slice(0, 2000)).toMatch(/requireChannelPrivacy\(/); + } + }); +});