fix: backlog cleanup after team-foundation

docs/key-backup.md

@@ -6,8 +6,16 @@ running `wrangler secret put`, or the data becomes permanently unreadable.
 
 ## Quick path
 
-1. Generate a keyring offline: `openssl rand -base64 32` (see [Generate keyring](#generate-keyring)).
-2. Build the `PII_KEYRING` JSON and back it up with two custodians in a password manager (see [Back up](#back-up-two-custodians)) — do this before step 3.
+1. Generate the full `PII_KEYRING` value offline, on a trusted machine:
+   ```sh
+   printf '{"active":1,"keys":{"1":"%s"}}\n' "$(openssl rand -base64 32)"
+   ```
+   It prints the whole JSON keyring, e.g. `{"active":1,"keys":{"1":"q2x...8Q="}}`.
+   That whole line is the secret (see [Generate keyring](#generate-keyring)).
+   > **Warning:** the secret must be this JSON, not the bare base64 key.
+   > A raw `openssl rand -base64 32` output fails `parseKeyRing` ("not valid
+   > JSON"), and every webhook request returns `500`.
+2. Back up that exact JSON with two custodians in a password manager (see [Back up](#back-up-two-custodians)) — do this before step 3.
 3. `wrangler secret put BOT_TOKEN`, `WEBHOOK_SECRET`, `PII_KEYRING` (see [Install secrets](#install-secrets)).
 4. Register the webhook with `secret_token` and verify with `getWebhookInfo` (see [Register webhook](#register-webhook)).
 5. Record who the two custodians are and which password-manager entry holds the secrets, somewhere your team can find during an incident (not in this repo).
@@ -28,19 +36,19 @@ The required shape is:
   fails closed (`buildBot` throws, the webhook returns 500) — it never falls
   back to plaintext or a wrong key.
 
-Generate one 32-byte key offline, on a machine you trust, never in a shared
-chat or logged shell history:
+Generate the full secret value offline, on a machine you trust, never in a
+shared chat. The key only exists inside the command's output, never as a
+literal in the command line or shell history:
 
 ```sh
-openssl rand -base64 32
+printf '{"active":1,"keys":{"1":"%s"}}\n' "$(openssl rand -base64 32)"
 ```
 
-Then assemble the full secret value by hand (do not paste the key into any
-online JSON tool):
-
-```json
-{"active":1,"keys":{"1":"<paste the openssl output here>"}}
-```
+Paste that whole JSON line (not only the key inside it) when
+`wrangler secret put PII_KEYRING` prompts. Do not paste the key into any
+online JSON tool. A bare base64 key is rejected: the Worker fails closed and
+every webhook request returns `500`, with `"reason":"PII_KEYRING secret is not
+valid JSON"` in the Worker logs.
 
 ## Back up (two custodians)
 
@@ -172,7 +180,7 @@ recovered** — there is no backdoor or master key.
 
 ## Final checklist
 
-- [ ] Keyring generated offline with `openssl rand -base64 32` per key version.
+- [ ] Keyring generated offline as the full JSON (`{"active":1,"keys":{"1":"<base64 32-byte key>"}}`), not a bare base64 key.
 - [ ] `PII_KEYRING` JSON backed up in a password manager shared by two custodians, before `wrangler secret put`.
 - [ ] `BOT_TOKEN` and `WEBHOOK_SECRET` also backed up with the two custodians.
 - [ ] `wrangler secret put` run for `BOT_TOKEN`, `WEBHOOK_SECRET`, `PII_KEYRING`.

src/adapters/crypto/key-ring.ts

@@ -1,7 +1,11 @@
 // Parses the `PII_KEYRING` secret (design.md "Key ring"):
 //   {"active":<version>,"keys":{"<version>":"<base64 32-byte key>"}}
 // Fails closed (throws) on anything missing or malformed — a broken keyring
-// must never silently fall back to plaintext or a wrong key.
+// must never silently fall back to plaintext or a wrong key. Every failure
+// is a ConfigError whose message is logged as-is, so messages only ever
+// interpolate already-validated numbers, never raw secret content.
+
+import { ConfigError } from "../../config-error";
 
 const KEY_BYTES = 32;
 
@@ -12,14 +16,14 @@ export interface KeyRing {
 
 export function parseKeyRing(raw: string | undefined): KeyRing {
   if (!raw || raw.trim() === "") {
-    throw new Error("PII_KEYRING secret is missing");
+    throw new ConfigError("PII_KEYRING secret is missing");
   }
 
   let parsed: unknown;
   try {
     parsed = JSON.parse(raw);
   } catch {
-    throw new Error("PII_KEYRING secret is not valid JSON");
+    throw new ConfigError("PII_KEYRING secret is not valid JSON");
   }
 
   if (
@@ -29,7 +33,7 @@ export function parseKeyRing(raw: string | undefined): KeyRing {
     typeof (parsed as { keys?: unknown }).keys !== "object" ||
     (parsed as { keys?: unknown }).keys === null
   ) {
-    throw new Error(
+    throw new ConfigError(
       'PII_KEYRING secret must have the shape {"active":N,"keys":{"N":"<base64 32B>"}}',
     );
   }
@@ -43,21 +47,21 @@ export function parseKeyRing(raw: string | undefined): KeyRing {
   for (const [versionText, base64Key] of Object.entries(rawKeys)) {
     const version = Number(versionText);
     if (!Number.isInteger(version) || typeof base64Key !== "string") {
-      throw new Error(
-        `PII_KEYRING secret has an invalid key entry for version "${versionText}"`,
+      throw new ConfigError(
+        "PII_KEYRING secret has an invalid key entry (version must be an integer, key a base64 string)",
       );
     }
     const bytes = Buffer.from(base64Key, "base64");
     if (bytes.length !== KEY_BYTES) {
-      throw new Error(
+      throw new ConfigError(
         `PII_KEYRING secret key version ${version} must decode to ${KEY_BYTES} bytes, got ${bytes.length}`,
       );
     }
     keys.set(version, new Uint8Array(bytes));
   }
 
   if (!keys.has(active)) {
-    throw new Error(
+    throw new ConfigError(
       `PII_KEYRING secret "active" version ${active} has no matching key entry`,
     );
   }

src/adapters/log/safe-logger.ts

@@ -1,7 +1,7 @@
 import type { LogEvent, Logger } from "../../domain/ports";
 
 // design.md "Logging": an allowlisted field set only (event, teamId,
-// membershipId, field, outcome, errorCode). Update text and values are
+// membershipId, field, outcome, errorCode, reason). Update text and values are
 // NEVER logged. Fields are copied explicitly, one by one — never spread
 // from the caller's object — so an accidental extra property on a
 // LogEvent-shaped value (e.g. via an unsafe cast) cannot leak into a log
@@ -18,6 +18,7 @@ export function createSafeLogger(): Logger {
           : {}),
         ...(entry.field !== undefined ? { field: entry.field } : {}),
         ...(entry.errorCode !== undefined ? { errorCode: entry.errorCode } : {}),
+        ...(entry.reason !== undefined ? { reason: entry.reason } : {}),
       };
       console.log(JSON.stringify(safe));
     },

src/adapters/telegram/commands.ts

@@ -1,4 +1,4 @@
-import type { Bot } from "grammy";
+import type { Bot, Context } from "grammy";
 import { bindDataChannel } from "../../domain/usecases/bind-data-channel";
 import { joinTeam } from "../../domain/usecases/join-team";
 import { setupTeam } from "../../domain/usecases/setup-team";
@@ -151,11 +151,35 @@ function profileDirectoryReply(memberships: Membership[], fields: ProfileField[]
   return `Team ${teamId}\n${memberships.map((m) => formatMemberBlock(m, fields)).join("\n\n")}`;
 }
 
+// An anonymous group admin's message arrives with `sender_chat` set to the
+// group itself (and `from` set to the GroupAnonymousBot placeholder).
+function isAnonymousGroupAdmin(ctx: Context): boolean {
+  const senderChatId = ctx.message?.sender_chat?.id;
+  return senderChatId !== undefined && senderChatId === ctx.chat?.id;
+}
+
 export function registerCommands(bot: Bot, deps: CommandDeps): void {
   registerTeamPicker(bot, deps);
   bot.command("setup", async (ctx) => {
     const loc = callerLocation(ctx);
     if (!loc) return;
+    // Telegram-specific pre-checks, answered before any admin lookup: a DM
+    // has no group to register, and an anonymous admin's `from` is the
+    // GroupAnonymousBot, which getChatMember never reports as an admin —
+    // both would otherwise fall through to a misleading "not an admin"
+    // refusal.
+    if (isPrivateChat(ctx)) {
+      deps.logger.log({ event: "setup-team", outcome: "refused", errorCode: "PrivateChat" });
+      await ctx.reply("Run /setup inside the group you want to register as a team, not in a private chat.");
+      return;
+    }
+    if (isAnonymousGroupAdmin(ctx)) {
+      deps.logger.log({ event: "setup-team", outcome: "refused", errorCode: "AnonymousAdmin" });
+      await ctx.reply(
+        'You are posting as an anonymous admin, so your admin status cannot be verified. Turn off "Remain anonymous" in your admin rights and run /setup again.',
+      );
+      return;
+    }
     await runCommand(
       {
         event: "setup-team",

src/adapters/telegram/team-picker.ts

@@ -1,9 +1,10 @@
 import type { Bot, Context } from "grammy";
 import { UnauthorizedError } from "../../domain/errors";
+import { parseTeamId, TEAM_ID_PATTERN_SOURCE } from "../../domain/ids";
 import { selectDmTeam } from "../../domain/usecases/dm-team-selection";
 import type { Clock, DmSelectionRepo, Logger, MembershipRepo } from "../../domain/ports";
 
-const SELECTION_PATTERN = /^sel:([a-z0-9-]{1,64})$/i;
+const SELECTION_PATTERN = new RegExp(`^sel:(${TEAM_ID_PATTERN_SOURCE})$`, "i");
 
 const EVENT = "dm-team-selection";
 
@@ -51,13 +52,11 @@ async function safeReply(ctx: Context, deps: TeamPickerDeps, text: string): Prom
 export function registerTeamPicker(bot: Bot, deps: TeamPickerDeps): void {
   bot.callbackQuery(SELECTION_PATTERN, async (ctx) => {
     const match = SELECTION_PATTERN.exec(ctx.callbackQuery.data);
-    if (!match || !isPrivateChat(ctx) || !ctx.from) return;
+    const teamId = match?.[1] !== undefined ? parseTeamId(match[1]) : null;
+    if (!teamId || !isPrivateChat(ctx) || !ctx.from) return;
 
     try {
-      await selectDmTeam(
-        { telegramUserId: ctx.from.id, teamId: match[1] as never },
-        deps,
-      );
+      await selectDmTeam({ telegramUserId: ctx.from.id, teamId }, deps);
     } catch (error) {
       if (!(error instanceof UnauthorizedError)) {
         // Unexpected/transient failure (e.g. D1) persisting the selection —

src/composition.ts

@@ -9,7 +9,9 @@ import { createSafeLogger } from "./adapters/log/safe-logger";
 import { createBot } from "./adapters/telegram/bot";
 import { createChatAdminChecker } from "./adapters/telegram/chat-admin-checker";
 import { registerCommands } from "./adapters/telegram/commands";
+import { ConfigError } from "./config-error";
 import type { FieldCipher } from "./domain/ports";
+import type { UserFromGetMe } from "grammy/types";
 import type { Env } from "./env";
 
 // Composition root: wires env bindings -> adapters -> use cases for one
@@ -39,6 +41,31 @@ function getOrBuildCipher(rawKeyRing: string): FieldCipher {
 const idGen = { newId: () => crypto.randomUUID() };
 const clock = { now: () => Date.now() };
 
+// A raw JSON.parse SyntaxError can quote the input, so it is replaced by a
+// ConfigError with a fixed message that is safe to log. The shape check
+// covers the fields grammY reads from its cached getMe result (the bot's
+// id, and its username for command matching).
+function parseBotInfo(raw: string): UserFromGetMe {
+  let parsed: unknown;
+  try {
+    parsed = JSON.parse(raw);
+  } catch {
+    throw new ConfigError("BOT_INFO var is not valid JSON");
+  }
+  if (
+    typeof parsed !== "object" ||
+    parsed === null ||
+    Array.isArray(parsed) ||
+    typeof (parsed as { id?: unknown }).id !== "number" ||
+    typeof (parsed as { username?: unknown }).username !== "string"
+  ) {
+    throw new ConfigError(
+      "BOT_INFO var must be a getMe object with a numeric id and a string username",
+    );
+  }
+  return parsed as UserFromGetMe;
+}
+
 export function buildBot(env: Env) {
   const cipher = getOrBuildCipher(env.PII_KEYRING);
   const logger = createSafeLogger();
@@ -49,7 +76,7 @@ export function buildBot(env: Env) {
   const profileRepo = createD1ProfileRepo(env.DB, idGen, clock, cipher);
   const dmSelectionRepo = createD1DmSelectionRepo(env.DB);
 
-  const bot = createBot(env.BOT_TOKEN, JSON.parse(env.BOT_INFO));
+  const bot = createBot(env.BOT_TOKEN, parseBotInfo(env.BOT_INFO));
   const chatAdminChecker = createChatAdminChecker(bot.api);
 
   registerCommands(bot, {

src/config-error.ts

@@ -0,0 +1,7 @@
+// A missing or malformed Worker binding (secret or var). Its `message` is
+// logged verbatim as the composition failure `reason` (src/index.ts), so it
+// MUST be a fixed, non-sensitive description — never interpolate secret
+// values, key material, tokens, or any part of the raw binding.
+export class ConfigError extends Error {
+  override name = "ConfigError";
+}

src/domain/ids.ts

@@ -6,10 +6,23 @@ export type TeamId = string & { readonly __brand: "TeamId" };
 export type MemberId = string & { readonly __brand: "MemberId" };
 export type MembershipId = string & { readonly __brand: "MembershipId" };
 
+// Trusted construction only (ids we generated or read back from D1).
 export function asTeamId(id: string): TeamId {
   return id as TeamId;
 }
 
+// Same character set and length the ids we generate (crypto.randomUUID)
+// fit in. Used for untrusted input (e.g. callback data) so a TeamId is
+// never built from an unchecked string. The unanchored source is exported
+// so adapters that embed a team id in a larger format (e.g. callback data)
+// reuse this single rule instead of restating it.
+export const TEAM_ID_PATTERN_SOURCE = "[a-z0-9-]{1,64}";
+const TEAM_ID_PATTERN = new RegExp(`^${TEAM_ID_PATTERN_SOURCE}$`, "i");
+
+export function parseTeamId(raw: string): TeamId | null {
+  return TEAM_ID_PATTERN.test(raw) ? (raw as TeamId) : null;
+}
+
 export function asMemberId(id: string): MemberId {
   return id as MemberId;
 }

src/domain/ports.ts

@@ -140,6 +140,9 @@ export interface LogEvent {
   field?: string;
   outcome: "ok" | "refused" | "error";
   errorCode?: string;
+  // A fixed, non-sensitive failure description (e.g. a ConfigError
+  // message). Never a raw error message, input value, or secret.
+  reason?: string;
 }
 
 export interface Logger {

src/index.ts

@@ -2,6 +2,7 @@ import { Hono } from "hono";
 import type { Update } from "grammy/types";
 import { createSafeLogger } from "./adapters/log/safe-logger";
 import { buildBot } from "./composition";
+import { ConfigError } from "./config-error";
 import type { Env } from "./env";
 
 export type { Env } from "./env";
@@ -32,11 +33,15 @@ app.post("/telegram/webhook", async (c) => {
     bot = buildBot(c.env);
   } catch (err) {
     // Fail closed on a broken PII_KEYRING (or any other composition
-    // failure) — never log the secret or any error detail beyond a code.
+    // failure) — never log the secret. Only a ConfigError's message is
+    // logged (as `reason`), because it is a fixed, non-sensitive string by
+    // contract (src/config-error.ts); any other error's message may echo
+    // input, so only its name is logged.
     logger.log({
       event: "composition",
       outcome: "error",
       errorCode: err instanceof Error ? err.name : "UnknownError",
+      ...(err instanceof ConfigError ? { reason: err.message } : {}),
     });
     return c.text("Internal Server Error", 500);
   }
@@ -66,6 +71,12 @@ app.post("/telegram/webhook", async (c) => {
     });
     return c.text("ok", 200);
   }
+  // Valid JSON that is not an object (e.g. `null`) makes grammY throw a
+  // TypeError, which would otherwise be answered 500 and retried forever.
+  if (typeof update !== "object" || update === null || Array.isArray(update)) {
+    logger.log({ event: "webhook", outcome: "error", errorCode: "MalformedUpdate" });
+    return c.text("ok", 200);
+  }
 
   try {
     await bot.handleUpdate(update as Update);

test/adapters/crypto/aes-gcm-cipher.test.ts

@@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest";
 import { FieldUnreadableError } from "../../../src/domain/errors";
 import { createAesGcmCipher } from "../../../src/adapters/crypto/aes-gcm-cipher";
 import { parseKeyRing } from "../../../src/adapters/crypto/key-ring";
+import { ConfigError } from "../../../src/config-error";
 
 // 32 raw bytes, base64-encoded — matches PII_KEYRING's documented format
 // (.dev.vars.example: `{"active":1,"keys":{"1":"<base64 32B key>"}}`).
@@ -198,3 +199,42 @@ describe("parseKeyRing (PII_KEYRING secret parsing, fail-closed)", () => {
     expect(keyRing.keys.size).toBe(2);
   });
 });
+
+// Malformed-input coverage at the PII_KEYRING trust boundary. Every failure
+// MUST be a ConfigError (the only error type whose message the webhook
+// boundary logs, see src/index.ts), and that message MUST NOT echo key
+// material or any other part of the raw secret.
+describe("parseKeyRing — malformed PII_KEYRING input", () => {
+  const LEAK_MARKER = "leak-marker-SECRET";
+
+  it.each([
+    ["a raw base64 key instead of the JSON keyring", KEY_V1],
+    ["whitespace only", "   "],
+    ["JSON null", "null"],
+    ["a JSON array", JSON.stringify([KEY_V1])],
+    ["a JSON number", "42"],
+    ["a JSON string", JSON.stringify(KEY_V1)],
+    ["missing active", JSON.stringify({ keys: { "1": KEY_V1 } })],
+    ["active as a string", JSON.stringify({ active: "1", keys: { "1": KEY_V1 } })],
+    ["keys null", JSON.stringify({ active: 1, keys: null })],
+    ["keys as a string", JSON.stringify({ active: 1, keys: KEY_V1 })],
+    ["a non-integer version key", JSON.stringify({ active: 1, keys: { "1": KEY_V1, [LEAK_MARKER]: KEY_V2 } })],
+    ["a non-string key value", JSON.stringify({ active: 1, keys: { "1": 12345 } })],
+    ["a key that is not 32 bytes", JSON.stringify({ active: 1, keys: { "1": Buffer.alloc(31, 1).toString("base64") } })],
+    ["an active version with no key entry", JSON.stringify({ active: 2, keys: { "1": KEY_V1 } })],
+  ])("throws a ConfigError that does not echo the secret for %s", (_label, raw) => {
+    let thrown: unknown;
+    try {
+      parseKeyRing(raw);
+    } catch (err) {
+      thrown = err;
+    }
+
+    expect(thrown).toBeInstanceOf(ConfigError);
+    const message = (thrown as Error).message;
+    expect(message).toMatch(/PII_KEYRING/);
+    expect(message).not.toContain(KEY_V1);
+    expect(message).not.toContain(KEY_V2);
+    expect(message).not.toContain(LEAK_MARKER);
+  });
+});