docs(todos): add round-6 review findings 103-115 for PR #2024

Author: koala73

todos/103-pending-p1-debug-block-leaks-api-keys-in-validateapikey.md

@@ -0,0 +1,69 @@
+---
+status: pending
+priority: p1
+issue_id: "103"
+tags: [code-review, security, payments]
+dependencies: []
+---
+
+# `_debug` Object in `validateApiKey` Leaks All Valid API Keys
+
+## Problem Statement
+
+`api/_api-key.js` returns a `_debug` object on invalid key attempts that contains `envVarRaw` (the raw `WORLDMONITOR_VALID_KEYS` value) and `parsedKeys` (every valid key as a parsed array). Although the gateway currently only surfaces `keyCheck.error` (a string) in the HTTP response, the full `_debug` object is held in `keyCheck` in memory. Any future logging middleware, error reporter, or Sentry breadcrumb that serializes `keyCheck` would leak ALL valid API keys to external observers.
+
+This is one refactor away from a full key disclosure incident.
+
+## Findings
+
+- `api/_api-key.js:59-65` — `_debug` object contains `envVarRaw`, `parsedKeys`, `receivedKey`, `receivedKeyLen`, `envVarLen`
+- `server/gateway.ts` currently only propagates `keyCheck.error` (string) to HTTP response — leakage is latent, not immediate
+- Identified by: security-sentinel (round-6 review)
+
+## Proposed Solutions
+
+### Option 1: Delete the `_debug` block entirely
+
+**Approach:** Remove lines 59-65 from `api/_api-key.js`. The function returns only `{ valid, required, error }`.
+
+**Pros:** Eliminates risk entirely. Simple change.
+**Cons:** Loses diagnostic data (not meaningful — logs should not contain secrets anyway).
+**Effort:** 5 minutes
+**Risk:** None
+
+### Option 2: Gate `_debug` behind `ENABLE_KEY_DEBUG=true` env var
+
+**Approach:** Only attach `_debug` when `process.env.ENABLE_KEY_DEBUG === 'true'`. Never set this in production.
+
+**Pros:** Preserves debug capability for local development.
+**Cons:** Env var must never be set in production; adds a conditional that could be mistakenly enabled.
+**Effort:** 10 minutes
+**Risk:** Low (if the env var discipline holds)
+
+## Recommended Action
+
+Option 1. Delete the `_debug` block. No diagnostic value justifies storing all valid API keys in a runtime object.
+
+## Technical Details
+
+**Affected files:**
+- `api/_api-key.js:59-65` — remove `_debug` block
+
+## Resources
+
+- **PR:** koala73/worldmonitor#2024
+- **Identified by:** security-sentinel, round-6 review 2026-03-31
+
+## Acceptance Criteria
+
+- [ ] `_debug` block removed from `validateApiKey` return value
+- [ ] `validateApiKey` still returns `{ valid, required, error }` for invalid key cases
+- [ ] `tsc --noEmit` + `vitest run` pass
+
+## Work Log
+
+### 2026-03-31 - Identified
+
+**By:** security-sentinel (round-6 /ce-review)
+
+- Latent high — not in HTTP response today but one logging-middleware addition away from full disclosure

todos/104-pending-p1-jwt-missing-algorithms-allowlist-alg-none-bypass.md

@@ -0,0 +1,59 @@
+---
+status: pending
+priority: p1
+issue_id: "104"
+tags: [code-review, security, auth]
+dependencies: []
+---
+
+# JWT Verification Missing `algorithms: ['RS256']` — `alg:none` Bypass Risk
+
+## Problem Statement
+
+`server/auth-session.ts` calls `jwtVerify(token, jwks, { issuer, audience })` without an `algorithms` field. Clerk issues RS256 tokens. Without pinning the algorithm, some versions of `jose` will accept tokens with `alg: none` or `alg: HS256`, bypassing signature verification entirely. This is in the direct path for Vercel edge gateway entitlement enforcement — a bypass here means an attacker can craft a JWT with any `sub` claim and access any tier-gated endpoint.
+
+## Findings
+
+- `server/auth-session.ts:35` — `jwtVerify` has no `algorithms` option
+- Identified in existing todo #035 (pending P1)
+- The entitlement enforcement path in `server/gateway.ts` → `resolveSessionUserId` → `verifyClerkToken` → `jwtVerify` is the critical path
+
+## Proposed Solutions
+
+### Option 1: Add `algorithms: ['RS256']` to `jwtVerify` options
+
+```ts
+const { payload } = await jwtVerify(token, jwks, {
+  issuer: issuerDomain,
+  audience: undefined,
+  algorithms: ['RS256'],
+});
+```
+
+**Effort:** 2 minutes
+**Risk:** None — Clerk always issues RS256
+
+## Recommended Action
+
+Option 1. One-line fix. Zero risk.
+
+## Technical Details
+
+**Affected files:**
+- `server/auth-session.ts` — add `algorithms: ['RS256']` to `jwtVerify` options
+
+## Resources
+
+- **PR:** koala73/worldmonitor#2024
+- **Prior todo:** `todos/035-pending-p1-jwtverify-missing-algorithms-allowlist.md`
+- **Identified by:** learnings-researcher + security-sentinel
+
+## Acceptance Criteria
+
+- [ ] `jwtVerify` call includes `algorithms: ['RS256']`
+- [ ] Tokens with `alg: none` are rejected
+- [ ] `tsc --noEmit` + `vitest run` pass
+
+## Work Log
+
+### 2026-03-31 - Identified (round-6 review)

todos/105-pending-p1-auth-init-deadlock-isprousuer-guard-blocks-clerk.md

@@ -0,0 +1,57 @@
+---
+status: pending
+priority: p1
+issue_id: "105"
+tags: [code-review, auth, payments]
+dependencies: []
+---
+
+# Auth Init Deadlock — `initAuthState()` Gated Behind `isProUser()` Blocks New Users
+
+## Problem Statement
+
+`initAuthState()` in `src/App.ts` is wrapped inside an `if (isProUser())` guard (or similar premium check). `isProUser()` returns false for all new users (no `wm-pro-key`/`wm-widget-key` in localStorage). This creates a bootstrapping deadlock: Clerk never loads, the sign-in button never appears, and new users can never sign in to migrate from anonymous to authenticated identity. The entire anonymous-purchase migration flow in this PR (claimSubscription) is unreachable for new users.
+
+## Findings
+
+- `src/App.ts` — `initAuthState()` / `setupAuthWidget()` guarded by premium check
+- `isProUser()` reads from localStorage, starts false for every new visitor
+- Identified in existing todo #034 (pending P1) and learnings-researcher
+- Direct blocker for the anon-to-Clerk migration path added in this PR
+
+## Proposed Solutions
+
+### Option 1: Remove the `isProUser()` guard from auth init
+
+**Approach:** Call `initAuthState()` unconditionally on web (gate only on `!isDesktopRuntime()`). `initAuthState()` is cheap when no session exists — it checks for a token and returns quickly.
+
+**Pros:** Fixes the deadlock entirely. New users see sign-in button. Migration path works.
+**Cons:** Clerk SDK loads for all web users (already bundled, negligible cost).
+**Effort:** 5 minutes
+**Risk:** Low
+
+## Recommended Action
+
+Option 1. Remove the `isProUser()` gate from `initAuthState()`.
+
+## Technical Details
+
+**Affected files:**
+- `src/App.ts` — remove `isProUser()` guard around `initAuthState()` / Clerk setup
+
+## Resources
+
+- **PR:** koala73/worldmonitor#2024
+- **Prior todo:** `todos/034-pending-p1-auth-init-gated-behind-isProUser-deadlock.md`
+- **Identified by:** learnings-researcher
+
+## Acceptance Criteria
+
+- [ ] New user (no localStorage keys) sees sign-in button on page load
+- [ ] `initAuthState()` runs unconditionally for web runtime
+- [ ] Clerk sign-in modal opens successfully for new unauthenticated users
+- [ ] `vitest run` passes
+
+## Work Log
+
+### 2026-03-31 - Identified (round-6 review)

todos/106-pending-p1-any-ctx-type-getentitlementshandler-convex.md

@@ -0,0 +1,60 @@
+---
+status: pending
+priority: p1
+issue_id: "106"
+tags: [code-review, typescript, payments]
+dependencies: []
+---
+
+# `any` ctx Type in `getEntitlementsHandler` Breaks Convex Type Safety
+
+## Problem Statement
+
+`convex/entitlements.ts` defines `getEntitlementsHandler(ctx: { db: any }, userId: string)` with `db: any`. This bypasses all Convex-generated type checking for DB operations inside this function — TypeScript cannot catch typos in index names, field names, or return types. For a security-critical billing module this is unacceptable.
+
+## Findings
+
+- `convex/entitlements.ts:25-26` — `ctx: { db: any }` parameter type
+- The `(q: any)` cast on the index callback at line 31 compounds this
+- This function is called from both `query` and `internalQuery` handlers — the correct type is `QueryCtx`
+- Identified by: kieran-typescript-reviewer (round-6 review)
+
+## Proposed Solutions
+
+### Option 1: Use `QueryCtx` from `_generated/server`
+
+```ts
+import type { QueryCtx } from "./_generated/server";
+
+async function getEntitlementsHandler(ctx: QueryCtx, userId: string) {
+```
+
+Remove the `(q: any)` cast — the index callback will be fully typed once `ctx` is `QueryCtx`.
+
+**Effort:** 5 minutes
+**Risk:** None — tightens types, no behavior change
+
+## Recommended Action
+
+Option 1. Import `QueryCtx` and replace `{ db: any }`.
+
+## Technical Details
+
+**Affected files:**
+- `convex/entitlements.ts:25-26` — change parameter type
+- `convex/entitlements.ts:31` — remove `(q: any)` cast
+
+## Resources
+
+- **PR:** koala73/worldmonitor#2024
+- **Identified by:** kieran-typescript-reviewer
+
+## Acceptance Criteria
+
+- [ ] `getEntitlementsHandler` uses `QueryCtx` type from `_generated/server`
+- [ ] No `any` types in the function signature or body
+- [ ] `tsc --noEmit` produces zero errors
+
+## Work Log
+
+### 2026-03-31 - Identified (round-6 review)

todos/107-pending-p1-concurrent-webhooks-duplicate-entitlement-rows.md

@@ -0,0 +1,66 @@
+---
+status: pending
+priority: p1
+issue_id: "107"
+tags: [code-review, architecture, payments, data-integrity]
+dependencies: []
+---
+
+# Concurrent Webhook Events Can Produce Duplicate `entitlements` Rows
+
+## Problem Statement
+
+`upsertEntitlements` in `convex/payments/subscriptionHelpers.ts` performs a read-check-then-write. Two concurrent Convex mutations operating on different `subscriptions` documents for the same userId can both observe zero entitlement rows and both `ctx.db.insert()`. Convex has no uniqueness constraint on the `entitlements.by_userId` index — duplicate rows are possible.
+
+`getEntitlementsForUser` uses `.first()` (not `.unique()`) so it won't throw, but which row wins is non-deterministic. A paying user's active plan might be shadowed by a stale duplicate row.
+
+## Findings
+
+- `convex/payments/subscriptionHelpers.ts:63-107` — read-then-write without uniqueness guarantee
+- `convex/schema.ts:129-142` — no unique index annotation on `entitlements` table
+- Narrow window but structurally present: `subscription.active` + `payment.succeeded` for same user can both call `upsertEntitlements` concurrently
+- Test `convex/__tests__/entitlements.test.ts:134` confirms query survives duplicates but does not assert which row wins
+- Identified by: architecture-strategist (round-6 review)
+
+## Proposed Solutions
+
+### Option 1: Application-level deduplication guard in `upsertEntitlements`
+
+Before inserting, check again inside the mutation using `.first()`. If a row already exists by the time we reach the insert branch, patch it instead. This is safe because Convex mutations are serialized — the second one will see the first one's write.
+
+The current code already does this check, but the race window is between the initial `.first()` returning null and the subsequent `insert`. Since Convex mutations are serialized *per document*, two mutations on different documents can race. The fix is to use a single Convex mutation that handles both cases atomically with `.withIndex("by_userId").first()` re-checked at write time (Convex's OCC will retry on conflict).
+
+**Effort:** Small
+**Risk:** Low
+
+### Option 2: Post-insert deduplication cleanup job
+
+Schedule a cleanup action that periodically merges duplicate `entitlements` rows, keeping the one with the highest tier/latest `validUntil`.
+
+**Effort:** Medium
+**Risk:** Low (can run alongside the fix from Option 1)
+
+## Recommended Action
+
+Option 1: Add a second `.first()` check immediately before insert in `upsertEntitlements`, and add a comment documenting the race window and Convex's OCC protection.
+
+## Technical Details
+
+**Affected files:**
+- `convex/payments/subscriptionHelpers.ts:63-107` — add post-read re-check before insert
+- `convex/schema.ts` — add comment that Convex doesn't support unique indexes natively
+
+## Resources
+
+- **PR:** koala73/worldmonitor#2024
+- **Identified by:** architecture-strategist
+
+## Acceptance Criteria
+
+- [ ] `upsertEntitlements` re-checks for existing row immediately before inserting
+- [ ] Concurrent duplicate webhook events for same userId result in exactly one entitlement row
+- [ ] Test added: seed duplicate rows, process `subscription.active`, verify single row result
+
+## Work Log
+
+### 2026-03-31 - Identified (round-6 review)

todos/108-pending-p2-dispute-lost-uses-date-now-instead-of-eventtimestamp.md

@@ -0,0 +1,62 @@
+---
+status: pending
+priority: p2
+issue_id: "108"
+tags: [code-review, payments, data-integrity]
+dependencies: []
+---
+
+# `dispute.lost` Revocation Uses `Date.now()` Instead of `eventTimestamp`
+
+## Problem Statement
+
+In `handleDisputeEvent`, when `dispute.lost` fires and revokes the user's entitlement, the code calls `Date.now()` three separate times for `validUntil`, `updatedAt`, and the Redis cache sync args. Every other handler in `subscriptionHelpers.ts` consistently uses `eventTimestamp` (the webhook timestamp) for these fields — specifically to support the out-of-order event guard `isNewerEvent`. Using `Date.now()` in the dispute.lost block breaks this protection: a later-replayed older event with a smaller `timestamp` will pass the `isNewerEvent` check and overwrite the revocation with restored entitlements. For a chargeback scenario, this is a meaningful correctness gap — paid access could be unintentionally restored.
+
+## Findings
+
+- `convex/payments/subscriptionHelpers.ts:638-649` — three `Date.now()` calls in `dispute.lost` block
+- All other handlers use `eventTimestamp` parameter passed to `handleDisputeEvent`
+- Identified by: kieran-typescript-reviewer + architecture-strategist (round-6)
+
+## Proposed Solutions
+
+### Option 1: Capture `eventTimestamp` once, use consistently
+
+```ts
+const now = eventTimestamp; // consistent with all other handlers
+await ctx.db.patch(existing._id, {
+  planKey: "free",
+  features: getFeaturesForPlan("free"),
+  validUntil: now,
+  updatedAt: now,
+});
+// ...scheduler...
+  validUntil: now,
+```
+
+**Effort:** 2 minutes
+**Risk:** None
+
+## Recommended Action
+
+Option 1. Replace all three `Date.now()` calls with `eventTimestamp`.
+
+## Technical Details
+
+**Affected files:**
+- `convex/payments/subscriptionHelpers.ts:638,639,649` — replace `Date.now()` with `eventTimestamp`
+
+## Resources
+
+- **PR:** koala73/worldmonitor#2024
+- **Identified by:** kieran-typescript-reviewer, architecture-strategist
+
+## Acceptance Criteria
+
+- [ ] All `Date.now()` calls in `dispute.lost` block replaced with `eventTimestamp`
+- [ ] A replayed older event after `dispute.lost` does NOT restore entitlements
+- [ ] `vitest run` passes
+
+## Work Log
+
+### 2026-03-31 - Identified (round-6 review)