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

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
02-bot-skeleton-initdata-security	2026-10-22T19:59:00Z	standard	14	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	critical warning info total 0 3 5 8	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:

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:

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 puts 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:

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:

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