fix(admin): fail-closed on write-time channel privacy check (#3003)

.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.

.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.

server/public/admin-settings.html

@@ -903,11 +903,6 @@ <h2>Recent changes</h2>
     }
 
     // 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;

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<string | null> {
   try {
@@ -435,10 +435,9 @@ export async function resolveEditorialChannel(): Promise<string | null> {
       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<TriggerResult> {
@@ -447,7 +446,7 @@ export async function runAnnouncementTriggerJob(): Promise<TriggerResult> {
   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;
   }

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<Response | null> {
+  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) {

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<void> {
 
   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);