From c86b460643e0e22a9c822a4419317584985ba0e0 Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Thu, 24 Sep 2026 14:17:01 -0400 Subject: [PATCH 1/4] docs(key-backup): generate the full JSON keyring in the quick path A raw base64 key as PII_KEYRING fails parseKeyRing and turns every webhook call into a 500. --- docs/key-backup.md | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/docs/key-backup.md b/docs/key-backup.md index 8ad7800..9391b0d 100644 --- a/docs/key-backup.md +++ b/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 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":""}}`), 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`. From d7581f37d5e1fb335be9ecc059b967a84d18a1da Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Thu, 24 Sep 2026 14:17:01 -0400 Subject: [PATCH 2/4] fix(http): log safe config error reasons and accept non-object bodies Composition failures now log a fixed, non-sensitive reason for config errors (PII_KEYRING, BOT_INFO shape) instead of only the error name. A valid-JSON but non-object webhook body is logged as MalformedUpdate and answered 200 instead of crashing into a 500 retry loop. --- src/adapters/crypto/key-ring.ts | 20 ++-- src/adapters/log/safe-logger.ts | 3 +- src/composition.ts | 29 ++++- src/config-error.ts | 7 ++ src/domain/ports.ts | 3 + src/index.ts | 13 ++- test/adapters/crypto/aes-gcm-cipher.test.ts | 40 +++++++ test/adapters/log/safe-logger.test.ts | 20 ++++ test/http/webhook-e2e.test.ts | 112 ++++++++++++++++++++ 9 files changed, 236 insertions(+), 11 deletions(-) create mode 100644 src/config-error.ts diff --git a/src/adapters/crypto/key-ring.ts b/src/adapters/crypto/key-ring.ts index 9e06fb2..844d598 100644 --- a/src/adapters/crypto/key-ring.ts +++ b/src/adapters/crypto/key-ring.ts @@ -1,7 +1,11 @@ // Parses the `PII_KEYRING` secret (design.md "Key ring"): // {"active":,"keys":{"":""}} // 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":""}}', ); } @@ -43,13 +47,13 @@ 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}`, ); } @@ -57,7 +61,7 @@ export function parseKeyRing(raw: string | undefined): KeyRing { } if (!keys.has(active)) { - throw new Error( + throw new ConfigError( `PII_KEYRING secret "active" version ${active} has no matching key entry`, ); } diff --git a/src/adapters/log/safe-logger.ts b/src/adapters/log/safe-logger.ts index 6938580..c871dd9 100644 --- a/src/adapters/log/safe-logger.ts +++ b/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)); }, diff --git a/src/composition.ts b/src/composition.ts index 8d9b341..168ae34 100644 --- a/src/composition.ts +++ b/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, { diff --git a/src/config-error.ts b/src/config-error.ts new file mode 100644 index 0000000..d7324c0 --- /dev/null +++ b/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"; +} diff --git a/src/domain/ports.ts b/src/domain/ports.ts index 2d03f7f..d5c0057 100644 --- a/src/domain/ports.ts +++ b/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 { diff --git a/src/index.ts b/src/index.ts index f4c5693..65928d0 100644 --- a/src/index.ts +++ b/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); diff --git a/test/adapters/crypto/aes-gcm-cipher.test.ts b/test/adapters/crypto/aes-gcm-cipher.test.ts index 94c7c1c..7fd9c00 100644 --- a/test/adapters/crypto/aes-gcm-cipher.test.ts +++ b/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":""}}`). @@ -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); + }); +}); diff --git a/test/adapters/log/safe-logger.test.ts b/test/adapters/log/safe-logger.test.ts index 194da2c..2ff787b 100644 --- a/test/adapters/log/safe-logger.test.ts +++ b/test/adapters/log/safe-logger.test.ts @@ -38,6 +38,26 @@ describe("createSafeLogger", () => { spy.mockRestore(); }); + it("logs the allowlisted `reason` field alongside errorCode", () => { + const spy = vi.spyOn(console, "log").mockImplementation(() => {}); + const logger = createSafeLogger(); + + logger.log({ + event: "composition", + outcome: "error", + errorCode: "ConfigError", + reason: "PII_KEYRING secret is not valid JSON", + }); + + expect(JSON.parse(spy.mock.calls[0]?.[0] as string)).toEqual({ + event: "composition", + outcome: "error", + errorCode: "ConfigError", + reason: "PII_KEYRING secret is not valid JSON", + }); + spy.mockRestore(); + }); + it("never logs PII even if an extra property is attached to the entry via an unsafe cast", () => { const spy = vi.spyOn(console, "log").mockImplementation(() => {}); const logger = createSafeLogger(); diff --git a/test/http/webhook-e2e.test.ts b/test/http/webhook-e2e.test.ts index a415277..effe0b0 100644 --- a/test/http/webhook-e2e.test.ts +++ b/test/http/webhook-e2e.test.ts @@ -119,6 +119,92 @@ describe("POST /telegram/webhook — end-to-end through real composition (REL-00 consoleSpy.mockRestore(); }); + + it("logs a safe, diagnosable reason (never the secret) when PII_KEYRING is a raw key instead of the JSON keyring", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + const rawKey = Buffer.alloc(32, 3).toString("base64"); + + const res = await post({ update_id: nextUpdateId++ }, { ...env, PII_KEYRING: rawKey }); + + expect(res.status).toBe(500); + const entry = logs.map((l) => JSON.parse(l)).find((e) => e.event === "composition"); + expect(entry).toEqual({ + event: "composition", + outcome: "error", + errorCode: "ConfigError", + reason: "PII_KEYRING secret is not valid JSON", + }); + expect(logs.join("\n")).not.toContain(rawKey); + + consoleSpy.mockRestore(); + }); + + it("logs a safe reason (never the raw value) when BOT_INFO is not valid JSON", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + + const res = await post( + { update_id: nextUpdateId++ }, + { ...env, BOT_INFO: "{bot-info-leak-marker" }, + ); + + expect(res.status).toBe(500); + const logged = logs.join("\n"); + expect(logged).toMatch(/"reason":"BOT_INFO var is not valid JSON"/); + expect(logged).not.toContain("bot-info-leak-marker"); + + consoleSpy.mockRestore(); + }); + + it.each([ + ["JSON null", "null"], + ["a JSON number", "8808"], + ["a JSON array", JSON.stringify([{ id: 1, username: "bot-info-leak-marker" }])], + ["an empty object", "{}"], + ["a non-numeric id", JSON.stringify({ id: "bot-info-leak-marker", username: "test_bot" })], + ["a missing username", JSON.stringify({ id: 8808, first_name: "bot-info-leak-marker" })], + ])("logs a safe reason (never the raw value) when BOT_INFO has the wrong shape (%s)", async (_label, botInfo) => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + + const res = await post({ update_id: nextUpdateId++ }, { ...env, BOT_INFO: botInfo }); + + expect(res.status).toBe(500); + const entry = logs.map((l) => JSON.parse(l)).find((e) => e.event === "composition"); + expect(entry).toEqual({ + event: "composition", + outcome: "error", + errorCode: "ConfigError", + reason: "BOT_INFO var must be a getMe object with a numeric id and a string username", + }); + expect(logs.join("\n")).not.toContain("bot-info-leak-marker"); + + consoleSpy.mockRestore(); + }); + + it("never logs the message of a non-config composition error (it may echo input)", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + + // An empty BOT_TOKEN makes grammY's own Bot constructor throw a plain + // Error — not a ConfigError, so only its name may be logged. + const res = await post({ update_id: nextUpdateId++ }, { ...env, BOT_TOKEN: "" }); + + expect(res.status).toBe(500); + const entry = logs.map((l) => JSON.parse(l)).find((e) => e.event === "composition"); + expect(entry).toEqual({ event: "composition", outcome: "error", errorCode: "Error" }); + + consoleSpy.mockRestore(); + }); }); describe("POST /telegram/webhook — response policy (RES-001 / RES-002)", () => { @@ -148,6 +234,32 @@ describe("POST /telegram/webhook — response policy (RES-001 / RES-002)", () => consoleSpy.mockRestore(); }); + it.each([ + ["JSON null", "null"], + ["a JSON array", "[]"], + ["a JSON string", JSON.stringify("hello")], + ["a JSON number", "42"], + ["an empty object", "{}"], + ["an empty body", ""], + ["a non-numeric update_id", JSON.stringify({ update_id: "x" })], + ["a message missing chat and from", JSON.stringify({ update_id: 7, message: { message_id: 1, date: 0, text: "/setup" } })], + ["a callback query missing data and message", JSON.stringify({ update_id: 8, callback_query: { id: "cb", from: { id: 1, is_bot: false, first_name: "U" }, chat_instance: "c" } })], + ])("a malformed update body (%s) never replies to Telegram and is not answered with 500", async (_label, body) => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + const calls = stubTelegramApi(); + + const res = await post(body); + + expect(res.status).toBe(200); + expect(calls.filter((c) => c.method === "sendMessage")).toHaveLength(0); + for (const line of logs) expect(line).not.toContain(WEBHOOK_SECRET); + + consoleSpy.mockRestore(); + }); + it("an injected infra failure (D1 unavailable) is logged (no PII) and answered with 500 so Telegram retries", async () => { const logs: string[] = []; const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { From e7d73a9a73246bff9bdf7bc05bfa64a6ba2a4810 Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Thu, 24 Sep 2026 14:17:01 -0400 Subject: [PATCH 3/4] fix(telegram): clarify /setup refusals and validate picker team ids /setup in a DM asks to run it inside the group, and anonymous admins are told to disable 'Remain anonymous'. Team picker callback data goes through parseTeamId instead of an unchecked cast. --- src/adapters/telegram/commands.ts | 26 ++++++++- src/adapters/telegram/team-picker.ts | 11 ++-- src/domain/ids.ts | 13 +++++ test/adapters/telegram/commands.test.ts | 76 +++++++++++++++++++++++++ test/domain/ids.test.ts | 29 ++++++++++ 5 files changed, 148 insertions(+), 7 deletions(-) create mode 100644 test/domain/ids.test.ts diff --git a/src/adapters/telegram/commands.ts b/src/adapters/telegram/commands.ts index 3572105..e13a95d 100644 --- a/src/adapters/telegram/commands.ts +++ b/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", diff --git a/src/adapters/telegram/team-picker.ts b/src/adapters/telegram/team-picker.ts index c611890..210d876 100644 --- a/src/adapters/telegram/team-picker.ts +++ b/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 — diff --git a/src/domain/ids.ts b/src/domain/ids.ts index 6a35063..5297937 100644 --- a/src/domain/ids.ts +++ b/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; } diff --git a/test/adapters/telegram/commands.test.ts b/test/adapters/telegram/commands.test.ts index 6c5b48d..60bd43f 100644 --- a/test/adapters/telegram/commands.test.ts +++ b/test/adapters/telegram/commands.test.ts @@ -158,6 +158,45 @@ describe("registerCommands — /setup (team-registration spec)", () => { expect(deps.teamRepo.rows).toHaveLength(0); expect(replies[0]?.text).toMatch(/admin/i); }); + + it("tells the caller to run /setup inside the group when run in a private chat (DM)", async () => { + const { bot, replies, deps } = makeBot([{ chatId: 30, userId: 30 }]); + const adminCheck = vi.spyOn(deps.chatAdminChecker, "isAdmin"); + const logSpy = vi.spyOn(deps.logger, "log"); + + await bot.handleUpdate(commandUpdate("setup", 30, 30, { chatType: "private" })); + + expect(deps.teamRepo.rows).toHaveLength(0); + expect(adminCheck).not.toHaveBeenCalled(); + expect(replies[0]?.text).toMatch(/inside the group/i); + expect(replies[0]?.text).not.toMatch(/Only a Telegram group admin/i); + expect(logSpy).toHaveBeenCalledWith( + expect.objectContaining({ event: "setup-team", outcome: "refused", errorCode: "PrivateChat" }), + ); + }); + + it("tells an anonymous group admin to turn off anonymity instead of refusing as a non-admin", async () => { + // Telegram delivers an anonymous admin's message with `from` set to + // the GroupAnonymousBot and `sender_chat` set to the group itself. + const { bot, replies, deps } = makeBot(); + const adminCheck = vi.spyOn(deps.chatAdminChecker, "isAdmin"); + const logSpy = vi.spyOn(deps.logger, "log"); + const base = commandUpdate("setup", -100_40, 1_087_968_824); + const update = { + ...base, + message: { ...base.message, sender_chat: { id: -100_40, type: "supergroup", title: "Test group" } }, + } as Update; + + await bot.handleUpdate(update); + + expect(deps.teamRepo.rows).toHaveLength(0); + expect(adminCheck).not.toHaveBeenCalled(); + expect(replies[0]?.text).toMatch(/anonymous/i); + expect(replies[0]?.text).not.toMatch(/Only a Telegram group admin/i); + expect(logSpy).toHaveBeenCalledWith( + expect.objectContaining({ event: "setup-team", outcome: "refused", errorCode: "AnonymousAdmin" }), + ); + }); }); describe("registerCommands — /join (team-membership spec)", () => { @@ -264,6 +303,43 @@ describe("registerCommands — DM team selection (team-membership spec)", () => await expect(bot.handleUpdate(callbackUpdate(50, 1, `sel:${teamId}`))).rejects.toMatchObject({ error: failure }); }); + + it.each([ + ["empty team id", "sel:"], + ["path traversal", "sel:../team"], + ["SQL-ish", "sel:1' OR '1'='1"], + ["oversized team id", `sel:${"a".repeat(65)}`], + ["unknown prefix", "pick:id-1"], + ["no prefix", "id-1"], + ["embedded newline", "sel:id-1\nsel:id-2"], + ])("ignores malformed callback data (%s) without persisting or replying", async (_label, data) => { + const { bot, replies, deps } = makeBot([{ chatId: 10, userId: 1 }]); + await bot.handleUpdate(commandUpdate("setup", 10, 1)); + + await expect(bot.handleUpdate(callbackUpdate(50, 1, data))).resolves.toBeUndefined(); + + expect(deps.dmSelectionRepo.rows).toHaveLength(0); + expect(replies).toHaveLength(1); // only the /setup reply + }); + + it("ignores a well-formed selection callback that arrives from a group chat", async () => { + const { bot, replies, deps } = makeBot([{ chatId: 10, userId: 1 }]); + await bot.handleUpdate(commandUpdate("setup", 10, 1)); + const teamId = deps.teamRepo.rows[0]!.id; + const base = callbackUpdate(10, 1, `sel:${teamId}`); + const update = { + ...base, + callback_query: { + ...base.callback_query, + message: { message_id: 1, date: 0, chat: { id: 10, type: "supergroup", title: "Test group" } }, + }, + } as Update; + + await bot.handleUpdate(update); + + expect(deps.dmSelectionRepo.rows).toHaveLength(0); + expect(replies).toHaveLength(1); + }); }); describe("registerCommands — /profile data-channel gating", () => { diff --git a/test/domain/ids.test.ts b/test/domain/ids.test.ts new file mode 100644 index 0000000..a1f66f6 --- /dev/null +++ b/test/domain/ids.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from "vitest"; +import { parseTeamId } from "../../src/domain/ids"; + +describe("parseTeamId (validated TeamId construction for untrusted input)", () => { + it("accepts a UUID-shaped team id", () => { + expect(parseTeamId("00000000-0000-4000-8000-000000000001")).toBe( + "00000000-0000-4000-8000-000000000001", + ); + }); + + it.each([ + ["1 char (lower bound)", "a"], + ["64 chars (upper bound)", "a".repeat(64)], + ])("accepts a boundary-length id (%s)", (_label, raw) => { + expect(parseTeamId(raw)).toBe(raw); + }); + + it.each([ + ["empty", ""], + ["whitespace", " "], + ["path traversal", "../team"], + ["SQL-ish", "1' OR '1'='1"], + ["too long (65 chars)", "a".repeat(65)], + ["unicode", "tëam"], + ["embedded newline", "team\n1"], + ])("rejects a malformed id (%s)", (_label, raw) => { + expect(parseTeamId(raw)).toBeNull(); + }); +}); From 552a6c22cbc698f30fc5e61b9d050613040b059d Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Thu, 24 Sep 2026 14:17:02 -0400 Subject: [PATCH 4/4] chore(wrangler): make workers_dev and preview_urls explicit --- wrangler.jsonc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/wrangler.jsonc b/wrangler.jsonc index 021cbcb..68cd681 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -4,6 +4,11 @@ "main": "src/index.ts", "compatibility_date": "2026-08-22", "compatibility_flags": ["nodejs_compat"], + // Explicit copies of what deploys already resolved to by default: the + // webhook is served at hack-bot..workers.dev, and per-version + // Preview URLs are enabled. + "workers_dev": true, + "preview_urls": true, "observability": { "enabled": true, "traces": {