memories and such

agents/security-reviewer/review-FRE-580.md

@@ -0,0 +1,157 @@
+## Security Review — FRE-580 Email Marketing Sequences
+
+**Reviewer:** Security Reviewer
+**Scope:** All 8 files in the email marketing implementation (services, tRPC routers, webhooks, templates, scheduler)
+**Verdict:** **2 P1, 4 P2, 4 P3** findings — assign back to Senior Engineer for fixes
+
+---
+
+### P1 — Critical (2 findings)
+
+**P1#1 Webhook Signature Validation Bypass** (`server/services/email-webhooks.ts:99-121`)
+
+When `RESEND_WEBHOOK_SECRET` is unset (common in dev/staging) OR the `x-signature` header is missing, the handler falls through to lines 117-121 which parse and process the payload with **zero signature verification**:
+
+```ts
+const sigHeader = req.headers.get("x-signature");
+if (sigHeader && process.env.RESEND_WEBHOOK_SECRET) {
+  // ...signature validation...
+}
+// FALLTHROUGH: no validation when secret is missing
+const payload = await req.json();
+```
+
+**Impact:** Any POST to the webhook endpoint is accepted. Attackers can forge delivery/open/click events to manipulate analytics, or forge `unsubscribed`/`bounced` events to alter send-log state. In production with the secret set, only missing-header attacks apply.
+
+**Fix:** Always validate signature. If secret is missing, return `503 Service Unavailable` rather than falling through to unvalidated processing.
+
+---
+
+**P1#2 `sendTriggered` Open to All Authenticated Users with Unbounded Input** (`server/trpc/routers/email-marketing.ts:139-151`)
+
+```ts
+sendTriggered: baseProcedure
+  .use(requireAuth)          // any authenticated user
+  .input(z.object({
+    templateKey: z.string(), // any string, not enum-constrained
+    variables: z.record(z.string()).optional(), // arbitrary key-value pairs
+  }))
+```
+
+Any authenticated user can trigger **any** template in the registry with arbitrary variables. This means:
+- A free-tier user can fire `conversion:trial_ending` with `price: "$0.01"` and receive a misleading upgrade email
+- Variables like `{{price}}`, `{{feature_name}}`, `{{winback_code}}` render directly into HTML without escaping — stored XSS vector if email HTML is later displayed in admin UI
+
+**Impact:** Unbounded email sending (Resend quota exhaustion), HTML injection via template variables, template abuse (users triggering internal/Pro-only templates).
+
+**Fix:** Either (a) add `requireAdmin` middleware, or (b) constrain `templateKey` to an enum of user-allowed templates and add server-side variable allowlists per template.
+
+---
+
+### P2 — High (4 findings)
+
+**P2#3 HTML Injection via Template Variables** (`server/services/email-templates.ts` + `email-marketing.ts:146`)
+
+Template variables `{{price}}`, `{{feature_name}}`, `{{trial_price}}`, `{{winback_code}}` are interpolated directly into HTML bodies. Combined with P1#2 (arbitrary user-supplied variables), this creates a stored XSS vector:
+
+```html
+<p>Only <strong>{{price}}/month</strong></p>
+```
+
+If `price` = `<script>alert(1)</script>`, the rendered HTML contains executable script. If email content is stored and later rendered in an admin dashboard or analytics view, this becomes persistent XSS.
+
+**Fix:** HTML-escape all user-supplied template variables before interpolation, or use a templating library with auto-escaping (e.g., Handlebars).
+
+---
+
+**P2#4 Empty Email Enrollment Still Possible** (`server/trpc/routers/email-marketing.ts:113-117`)
+
+```ts
+const [user] = await db.select({ email: users.email }).from(users).where(eq(users.id, userId));
+const email = user?.email || ""; // fallback to empty string
+await emailSequenceService.enrollUser(userId, input.sequenceKey, email);
+```
+
+When `userId` parses correctly but no user row exists (e.g., deleted user, race condition), `email` is `""`. The schema allows `text("email").notNull()` — empty string passes validation. The enrollment is created with an empty email, causing silent send failures.
+
+**Fix:** Return error when user not found, or re-fetch email from `users` table at send time (don't cache in enrollment).
+
+---
+
+**P2#5 Analytics Memory Exhaustion** (`server/services/email-sequence-service.ts:473`)
+
+```ts
+const allEmails = await db.select().from(emailSendLog).where(buildWhere());
+for (const email of allEmails) { ... }
+```
+
+The `getAnalytics` query loads **all** email send log records into memory to compute `bySequence` breakdowns. With no row limit or pagination, a large send log (100K+ rows) can exhaust process memory.
+
+**Fix:** Use SQL `GROUP BY` aggregation instead of in-memory iteration, or add a `LIMIT` clause with a warning when truncated.
+
+---
+
+**P2#6 `getOptInField` Undefined Cast on Unknown Keys** (`server/services/email-sequence-service.ts:543-553`)
+
+```ts
+const map: Record<SequenceKey, string> = {
+  welcome: "welcomeOptIn",
+  // ...
+  transactional: "marketingOptIn",
+};
+return map[sequenceKey] as keyof typeof emailPreferences.$inferSelect;
+```
+
+If a new `SequenceKey` is added to the type but forgotten in the map, `map[sequenceKey]` returns `undefined`, which is cast to a valid key. The subsequent access `prefs[0][undefined]` returns `undefined`, which is falsy, causing silent opt-in suppression for all enrollments in that sequence.
+
+**Fix:** Add runtime assertion: `const field = map[sequenceKey]; if (!field) throw new Error(...); return field as ...`
+
+---
+
+### P3 — Medium/Low (4 findings)
+
+**P3#7 In-Memory Lock Fragility** (`server/services/email-sequence-service.ts:74-82`)
+
+The `sequenceLocks` Map is process-local. In a multi-instance deployment (or after process restart), concurrent runs can process the same sequence simultaneously.
+
+**Fix:** Use database-level advisory locks (SQLite `BEGIN IMMEDIATE`) or a distributed lock (Redis) for production deployments.
+
+---
+
+**P3#8 No Rate Limiting on tRPC Endpoints** (`server/trpc/routers/email-marketing.ts`)
+
+`sendTriggered` and `enrollSequence` can be called in rapid succession without throttling. A single user can exhaust Resend API quotas.
+
+**Fix:** Add per-user rate limiting (e.g., 5 triggered emails/hour) using a sliding window counter.
+
+---
+
+**P3#9 Scheduler Interval Validation** (`server/services/email-scheduler.ts:9`)
+
+```ts
+const SCHEDULE_INTERVAL_MS = Number(process.env.EMAIL_SCHEDULE_INTERVAL_MS) || 5 * 60 * 1000;
+```
+
+If `EMAIL_SCHEDULE_INTERVAL_MS=-1`, `Number("-1")` is `-1` (truthy), causing `setInterval` to fire on every event loop tick.
+
+**Fix:** Validate: `Math.max(Number(...), 1000) || 5 * 60 * 1000`
+
+---
+
+**P3#10 Webhook Missing Content-Type Validation** (`server/services/email-webhooks.ts:117`)
+
+`req.json()` is called without verifying `Content-Type: application/json`. Malformed request bodies cause unhandled `JSON.parse` exceptions.
+
+**Fix:** Check `req.headers.get("content-type")` before parsing; return `415 Unsupported Media Type` for non-JSON.
+
+---
+
+### Summary
+
+| Severity | Count | Status |
+|----------|-------|--------|
+| P1 (Critical) | 2 | **Blocking** — webhook bypass + unbounded sendTriggered |
+| P2 (High) | 4 | **Should fix** — HTML injection, empty email, memory, undefined cast |
+| P3 (Medium) | 4 | **Nice to have** — lock, rate limit, interval, content-type |
+
+**Disposition:** Assign back to Senior Engineer with `in_progress` status for P1/P2 fixes. P3 items can be tracked as child issues or addressed in a follow-up.