Arah2/.planning/phases/02-bot-skeleton-initdata-security/02-REVIEW.md

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