diff --git a/.changeset/channel-privacy-fail-closed.md b/.changeset/channel-privacy-fail-closed.md
new file mode 100644
index 0000000000..4d2b7171cf
--- /dev/null
+++ b/.changeset/channel-privacy-fail-closed.md
@@ -0,0 +1,46 @@
+---
+---
+
+**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'` — 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
+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/.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/routes/admin/settings.ts b/server/src/routes/admin/settings.ts
index 7da5cfff7c..64ec89c456 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 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
+ 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();
@@ -91,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,
@@ -126,17 +180,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 +229,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 +276,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 +319,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 +362,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 +405,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 +448,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/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/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..0938bd3205
--- /dev/null
+++ b/server/tests/unit/admin-settings-privacy-gate.test.ts
@@ -0,0 +1,265 @@
+/**
+ * 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,
+ mockGetSlackChannels,
+} = vi.hoisted(() => ({
+ mockVerifyPrivacy: vi.fn(),
+ mockIsSlackConfigured: vi.fn(),
+ mockSetBillingChannel: vi.fn(),
+ 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: (...args: unknown[]) => mockGetSlackChannels(...args),
+}));
+
+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();
+ });
+});
+
+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\(/);
+ }
+ });
+});
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', () => {
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();
+ });
});