ambek .planning

2026-10-23 22:37:55 +08:00
parent e512104d5e
commit 275879a18b
54 changed files with 9921 additions and 0 deletions
@@ -0,0 +1,153 @@
+---
+phase: 02-bot-skeleton-initdata-security
+reviewed: 2026-10-22T19:59:00Z
+depth: standard
+files_reviewed: 14
+files_reviewed_list:
+  - bot/package.json
+  - bot/wrangler.jsonc
+  - bot/tsconfig.json
+  - bot/.gitignore
+  - bot/.dev.vars.example
+  - bot/src/config.ts
+  - bot/src/webhook.ts
+  - bot/src/security/initdata.ts
+  - bot/src/kv/sessions.ts
+  - bot/src/i18n/strings.ts
+  - bot/src/bot/bot.ts
+  - bot/src/bot/commands/start.ts
+  - bot/src/bot/commands/help.ts
+  - bot/src/bot/commands/privacy.ts
+  - bot/scripts/register-commands.mjs
+  - bot/scripts/set-webhook.mjs
+  - bot/test/runner.mjs
+  - bot/test/harness.mjs
+  - bot/test/initdata.test.mjs
+  - bot/test/commands.test.mjs
+  - bot/test/privacy.test.mjs
+  - bot/test/sessions.test.mjs
+  - package.json
+findings:
+  critical: 0
+  warning: 3
+  info: 5
+  total: 8
+status: issues_found
+---
+
+# Phase 2: Code Review Report
+
+**Reviewed:** 2026-10-22T19:59:00Z
+**Depth:** standard
+**Files Reviewed:** 22 (14 entry list + hidden config/test files)
+**Status:** issues_found
+
+## Summary
+
+The security centerpiece holds up under scrutiny. The HMAC initData validator implements the official algorithm correctly (key = `HMAC-SHA256(key="WebAppData", msg=botToken)`, message = key-sorted data-check-string, comparison in constant time, freshness check after signature — D-13 precedence as specified). Re-verified against grammY's shipped typings: the `webhookCallback(bot, "cloudflare-mod", onTimeout, timeoutMs, secretToken)` positional form used in webhook.ts matches the 5-arg overload in `grammy/out/convenience/webhook.d.ts`. KV session layer honors TTL-only on every write path; `.dev.vars` is correctly gitignored via `bot/.gitignore` and untracked in git (verified with `git check-ignore` / `git ls-files`). Full suite green (27/27) and `tsc --noEmit` clean at review time.
+
+Three warnings remain, none of them in the validator's crypto path itself: (1) the only public unauthenticated endpoint accepts an unbounded request body straight into memory, (2) a one-secret-missing misconfiguration silently *disables* the webhook secret check instead of failing closed (confirmed in grammY's runtime source, `webhook.js:24-27`), and (3) the D-07 rotate-on-new invariant is only best-effort under KV's non-atomic read-delete-put, while the code comments call it "structurally closed".
+
+Not flagged (locked decisions per D-01..D-17): opaque UUID tokens instead of JWTs, no replay-nonce store (D-07 rotation is the accepted replay answer), 900 s TTL envelope, per-request bot instantiation.
+
+## Warnings
+
+### WR-01: `/api/session` reads an unbounded request body into memory (DoS surface)
+
+**File:** `bot/src/webhook.ts:49`
+**Issue:** `const raw = await req.text()` has no length cap before `parseFields` splits it. This is the one *unauthenticated, public* endpoint on the Worker: an attacker can POST up to Cloudflare's ingress body limit (tens of MB) of `&` or long values. `raw.split("&")` on a multi-MB body allocates millions of array elements plus Map entries — enough to blow the Worker isolate's 128 MB memory or CPU budget → 500s and isolate churn on a free-tier Worker. Telegram initData is a few hundred bytes; anything more is abuse.
+**Fix:**
+```ts
+async function handleSession(req: Request, env: Env): Promise<Response> {
+  if (req.method !== "POST") return new Response("Method not allowed", { status: 405 });
+  const raw = await req.text();
+  if (raw.length > 4096) {
+    // register in D-14 vocabulary: still category-only (no payload echo)
+    console.log("[initdata] rejected: malformed");
+    return new Response(JSON.stringify({ error: "malformed" }), { status: 413, headers: { "content-type": "application/json" } });
+  }
+  const result = await validateInitData(raw, env.BOT_TOKEN);
+  ...
+```
+(4 KB is generous — known-good real initData fixtures are ≪ 1 KB.)
+
+### WR-02: Missing `WEBHOOK_SECRET` silently disables webhook authentication (fail-open)
+
+**File:** `bot/src/webhook.ts:33-36`
+**Issue:** `webhookCallback(bot, "cloudflare-mod", undefined, undefined, env.WEBHOOK_SECRET)` passes the secret through, but grammY's check is opt-in per call: `compareSecretToken` in `node_modules/grammy/out/convenience/webhook.js:24-27` returns `true` for **every** request when `secretToken === undefined`. If `BOT_TOKEN` is armed as a wrangler secret but `WEBHOOK_SECRET` is forgotten (a plausible prod path — no deploy script runs `wrangler secret put`), Telegram still sends the header against a `setWebhook`-registered secret, yet the Worker accepts *any* unsigned POST to `/telegram/webhook` as auth: anyone can inject fabricated updates/spam, and this is the identity-carrying path Phase 4's Mini App flow will build on. Today's phase fails-anything-but-500 for missing secrets elsewhere; the one auth gate must not be the exception.
+**Fix:**
+```ts
+if (url.pathname === "/telegram/webhook") {
+  if (!env.WEBHOOK_SECRET) {
+    // fail closed: an unarmed secret must not silently arm the endpoint
+    return new Response("Server misconfigured", { status: 500 });
+  }
+  const bot = createBot(env);
+  ...
+}
+```
+
+### WR-03: D-07 rotate-on-new is not atomic — interleaved mints leave an orphan live token
+
+**File:** `bot/src/kv/sessions.ts:49-57`
+**Issue:** `mintSession` is get-pointer → delete-old-session → put-new-session → put-pointer. Cloudflare KV has no transactions and is eventually consistent, so two `POST `/api/session`` replays of the same valid initData can interleave: both read the same pointer, both mint, the loser's `session:<token>` record is never deleted, and KV replication may serve a stale pointer across colos — both tokens are live for up to 900 s. The module comments claim the invariant is "structurally closed" (line 48: "one live token per chat") and `SR2` asserts "exactly one live record" — true only sequentially. Impact is bounded (any party able to mint within the 15-min freshness window already holds a valid identity by D-13's design), so this is not a privilege escalation, but the invariant the tests/docs pin is stronger than what KV delivers.
+**Fix (pick one and make the choice explicit in docs/tests):**
+- Reconcile after write: re-read `chat:<chat_id>` after both `put`s and `kv.delete` the pointer target if it is neither the new token nor already deleted.
+- Or downgrade the stated invariant truthfully: "rotation is best-effort under KV eventual consistency; every token dies within 900 s regardless", and note it in `sessions.ts`/`02-CONTEXT.md` so Phase 4 does not build a nonce-store assumption on top.
+
+## Info
+
+### IN-01: Dead code — unused `ParsedInitData` export, unreachable branch condition, unused read APIs
+
+**File:** `bot/src/security/initdata.ts:53-60` / `bot/src/kv/sessions.ts:52` / `bot/src/kv/sessions.ts:61-86`
+**Issue:** (a) `ParsedInitData` is exported and never imported anywhere — `validateInitData` never emits it (the file's own header comment doesn't use the shape either). (b) `previousToken !== token` at sessions.ts:52 can never be false — `token` is a fresh `crypto.randomUUID()` minted two lines above, so the guard is unreachable code dressed as a check. (c) `getTokenForChat` and `getClaimByToken` have no production caller in this phase; acceptable as the Phase 4 access-layer API, but only if that intent is stated.
+**Fix:** Delete `ParsedInitData` and the dead `previousToken !== token` half-branch (keep `if (previousToken !== null) { ... }`). Add a one-line comment over the two read helpers: "consumed by the Phase 4 Mini App flow" — or their first real consumer from that phase.
+
+### IN-02: Stale wrangler.jsonc comment contradicts the shipped namespace id
+
+**File:** `bot/wrangler.jsonc:2` (vs `:9`)
+**Issue:** Line 2 still says the KV id is "filled in plan 03 task 1 after `npx wrangler kv namespace create`", but line 9 already carries the real id. A future reader trusts the comment and tries to re-create/replace an existing namespace — the exact setup churn the comment was guarding against.
+**Fix:** Replace the comment with the post-fact record: `// chat_sessions namespace created via wrangler kv namespace create (id below is live).`
+
+### IN-03: Claimed 900 s freshness boundary is not actually tested
+
+**File:** `bot/test/initdata.test.mjs:133-150` / `bot/src/security/initdata.ts:185`
+**Issue:** Both the implementation comment ("899 ok / 900 ok / 901 stale") and the T4 plan-header claim the exact-900 case, but the suite only signs 899 s and 901 s fixtures. For a security-critical boundary (D-13), the middle point is the one that pins the comparator direction (`>` vs `>=`); the code's `>` matches the stated claim today, but nothing would catch a `>=` regression.
+**Fix:**
+```js
+test("T4 freshness boundary: age 900 s → ok:true (boundary is inclusive)", async () => {
+  const validateInitData = await loadValidator();
+  const now = Math.floor(Date.now() / 1000);
+  const res = await validateInitData(signedUser(now - 900), TEST_TOKEN);
+  assert.equal(res.ok, true, `expected ok:true at exactly 900 s, got ${JSON.stringify(res)}`);
+});
+```
+
+### IN-04: `.dev.vars` parser diverges from wrangler's dotenv semantics — and is duplicated in both scripts
+
+**File:** `bot/scripts/register-commands.mjs:29-46` / `bot/scripts/set-webhook.mjs:24-41`
+**Issue:** The hand-rolled parser keeps quotes and inline comments as part of the value (`BOT_TOKEN="123:ABC"` sends a token wrapped in literal quotes; `WEBHOOK_URL=... # comment)` mangles the URL), whereas wrangler itself parses `.dev.vars` dotenv-style with quotes stripped. A user following dotenv habits gets opaque 401s from `npm run bot:commands` while local `wrangler dev` works — the two runtimes disagree about the same file. Additionally the identical 18-line `readDevVars` exists in both scripts — the repo's first copy-paste of a secrets-handling helper, the worst place for implementations to drift.
+**Fix:** Strip surrounding quotes and trailing inline comments in one shared `bot/scripts/lib/dev-vars.mjs`, imported by both scripts; or better, call out the assumed format in `.dev.vars.example` (`unquoted KEY=value`, no inline comments).
+
+### IN-05: `handleSession` has no error handling around the mint — KV failure surfaces as an opaque 500 with no category log
+
+**File:** `bot/src/webhook.ts:47-64`
+**Issue:** `validateInitData` itself never throws (all parse paths are caught), but `mintSession` unhooks that guarantee: a KV write failure during `/api/session` propagates as an unhandled Worker exception — generic 500 page, no D-14 category entry, nothing distinguishing "storage down" from a crash in review/Tail logs. Not a correctness hazard (fail-closed), just an observability gap in the phase that defined the category vocabulary.
+**Fix:**
+```ts
+try {
+  const token = await mintSession(env.chat_sessions, { /* claim */ });
+  return new Response(JSON.stringify({ token }), { status: 200, ... });
+} catch {
+  console.log("[session] mint failed: storage"); // category-only (D-14)
+  return new Response(JSON.stringify({ error: "storage" }), { status: 503, headers: { "content-type": "application/json" } });
+}
+```
+
+---
+
+_Verified during review (not findings): validation vectors T1–T7 green; `tsc --noEmit` clean; `bot/.dev.vars` ignored and untracked; HMAC key derivation matches Telegram's official algorithm and the RFC-4231-pinned independent node:crypto path; every `kv.put` carries `expirationTtl`; the single `console.log` is category-only (D-14); no secrets or payload fields echoed in scripts or responses._
+
+_Reviewed: 2026-10-22T19:59:00Z_
+_Reviewer: OpenCode (gsd-code-reviewer)_
+_Depth: standard_