Weekly Security Review: Jun 13 – Jun 21, 2026

[timothyfroehlich]

PRs reviewed: #1509, #1539, #1540, #1541, #1544, #1548, #1550, #1554, #1562, #1563, #1564, #1566, #1567, #1568, #1569, #1572

PRs skipped (deps/CI/docs only): #1537, #1538, #1542, #1543, #1545, #1546, #1551, #1553, #1556, #1557, #1558, #1559, #1560, #1561, #1565, #1570, #1573

Verdict: 1 medium finding needs attention (cross-user comment content leak via unscoped idempotency key); 1 minor observation; multiple positive security improvements landed this week.

---

Non-Negotiable Checklist

Status	Rule	Notes
✅	CORE-SEC-001 — Auth checks	All new endpoints/actions authenticate before serving data. addCommentAction has no checkPermission() beyond authentication (pre-existing, not introduced this week).
✅	CORE-SEC-002 — Input validation	All user inputs validated with Zod in reviewed PRs. Idempotency key uses either uuidish regex or z.string().uuid() (both correct; minor inconsistency, not a security issue).
✅	CORE-SEC-003/004 — CSP / security headers	#1572 correctly adds https://*.public.blob.vercel-storage.com to img-src. No regressions.
✅	CORE-SEC-005 — No hardcoded hostnames	Old hardcoded prod project ref in scripts/db-reset-preview.sh was deleted in #1564.
✅	CORE-SEC-006 — Minimal data at server→client boundary	All reviewed PRs project to minimal shapes before passing to client components. New collection view (#1509), profile page (#1572), and hover card API all correctly exclude emails, full ORM objects, and internal fields.
✅	CORE-SEC-007 — Email privacy	Profile page, hover card endpoint, and collection view all explicitly exclude user emails. getProfileById selects only safe columns; integration test asserts email is absent from the API response.
✅	CORE-SEC-008 — localhost not 127.0.0.1	No violations observed.
✅	CORE-SSR-001/002 — Supabase SSR patterns	All new auth checks follow createClient() → auth.getUser() immediately. Hover card API route (#1572) and collection issues tab (#1509) both comply.
✅	CORE-SSR-003 — Middleware present	No changes to middleware.
✅	CORE-ARCH-008 — Permissions matrix	New machines.pinballmap.link permission added to matrix in #1569. Tournament notes removed cleanly in #1554 (no orphaned matrix entries). No drift observed.
✅	CORE-ARCH-011 — No side effects in transactions	PR #1548 significantly hardened this: removed the createNotification(tx) footgun (which could dispatch email/Discord before commit), added assertNotInTransaction() to isDiscordIntegrationEnabled, and updated both static ESLint rule and runtime tripwire documentation.
✅	CORE-PBM-001 — PinballMap API conduct	Both PBM PRs (#1562, #1569) route all calls through the PinballMapClient seam with descriptive User-Agent, 429 backoff, token-store-and-reuse design, no crawling, and no live PBM calls from tests (mocked at seam boundary).
✅	CORE-TEST-006 — Test what we own	No live external services reached from tests this week. PBM client correctly mocked.

---

Broader Analysis

❌ Finding 1 — Medium: Cross-user comment content leak via unscoped idempotency key (src/services/issues.ts)

PR: #1550
File: src/services/issues.ts, approximately lines 708–718 and 742–755

The comment deduplication lookup uses WHERE idempotency_key = $1 with no AND author_id = $userId constraint:

const existing = await tx.query.issueComments.findFirst({
  where: eq(issueComments.idempotencyKey, idempotencyKey),
});
if (existing) {
  return { comment: existing, deliveryPlan: { deliveries: [] } };
}

If User A knows (or can guess) the UUID that User B used for a previous comment, A can submit a comment to any issue using that UUID. The server finds B's comment and returns it to A — leaking comment content — without creating a row, without any auth check that A can read B's comment, and without any check that the returned comment belongs to the target issue.

Realistic attack vectors: UUID v4 has 122 bits of entropy so random guessing is infeasible. The risk materialises if: (a) keys are leaked or predictable (e.g. test fixtures or a misconfigured client), or (b) a user submits to issue X with a key that was used on issue Y — the unscoped lookup will return Y's comment across issue boundaries.

The machine-timeline path (createMachineComment) uses ON CONFLICT DO NOTHING and never returns the existing row, so only addIssueComment is affected.

Recommended fix — scope both dedup lookups to the caller's user ID and the target issue:

const existing = await tx.query.issueComments.findFirst({
  where: and(
    eq(issueComments.idempotencyKey, idempotencyKey),
    eq(issueComments.authorId, userId),   // required — prevents cross-user leak
    eq(issueComments.issueId, issueId),   // defense in depth — prevents cross-issue leak
  ),
});

The same fix applies to both the pre-insert dedup check and the race-winner path.

---

⬜ Finding 2 — Minor: client-mock.ts missing import "server-only" guard

PR: #1562
File: src/lib/pinballmap/client-mock.ts

client-live.ts and client.ts both carry import "server-only" at line 1, which produces a build error if accidentally imported from a client bundle. client-mock.ts does not have this guard. In production getPinballMapMode() returns "live" so the mock never runs, but a client component accidentally importing the mock would not get a build-time error. Low risk today; easy to fix.

Recommended fix: add import "server-only"; as the first line of src/lib/pinballmap/client-mock.ts.

---

Positive security work landed this week

Several PRs this week represent meaningful security and data-integrity hardening — worth calling out:

• fix(db): disable prepared statements on the transaction pooler (PP-d8l8) #1567 (PP-d8l8) — Root-cause fix for the 2026-06-18 production silent COMMIT loss. Setting prepare: false on the runtime DB client (src/server/db/index.ts) eliminates the Supavisor transaction pooler hazard that caused write transactions to silently roll back.
• chore(arch): harden CORE-ARCH-011 side-effect guards (PP-lbqh) #1548 (PP-lbqh) — Removed the createNotification(tx) overload that allowed dispatching email/Discord from inside a DB transaction (the "Doodle Bug" class). Two-layer enforcement (ESLint static + runtime assertNotInTransaction) is now documented and consistent.
• fix(issues): add post-commit read-back guard to createIssue (PP-qk7s) #1563 (PP-qk7s) — Stop-gap read-back guard in createIssue: verifies the row is visible post-commit and throws IssueCommitVerificationError (with no PII in the message) rather than silently returning a 200 on commit loss.
• fix(settings): no redirect loop on missing profile (PP-etip) #1540 — Fixed an infinite auth redirect loop on settings page when user_profiles row is missing; uses notFound() + Sentry report instead of redirect(loginUrl).
• feat(ci): /audit-override PR-comment command to bypass the pnpm audit gate (PP-vsdo) #1566 — /audit-override escape hatch is commit-bound (drops on new push), authorization-gated (write access only), and adds an explicit warning to CI output — thoughtfully designed to avoid masking real vulnerabilities.
• fix(db): harden prod migration connection path (PP-xhqt) #1568 (PP-xhqt) — Migration script now requires POSTGRES_URL_NON_POOLING in production instead of silently falling back to the transaction pooler for DDL.
• Pooler/connection cleanup: shared script helper, prepare:false, terminology, admin key fallback (PP-z428, PP-54eu) #1564 — Deleted a shell script containing a hardcoded production project ref. Unified all DB scripts onto a shared helper with prepare: false. .env.example now correctly documents the session pooler endpoint.

---

Recommendations

1. [Medium — feat(idempotency): retry dedup for issue & machine comments (PP-e5th) #1550] File a follow-up bead to scope both idempotency dedup lookups in addIssueComment (src/services/issues.ts ~line 708 and ~line 742) to AND author_id = $userId AND issue_id = $issueId. Add a regression test that verifies User A cannot retrieve User B's comment via UUID collision.

2. [Minor — feat(pinballmap): integration design + client foundation (PP-o355.1) #1562] Add import "server-only"; to src/lib/pinballmap/client-mock.ts (one-line fix, can fold into the next PBM bead or a standalone cleanup).

3. [Informational — feat(profiles): redesigned user profile page + hover card #1572] The machine info tab passes machine.owner.id directly to PersonHoverCard without checking whether the owner is an invited user (who has no userProfiles row). This results in a 404 hover-card link for invited owners, not a security issue. The issue timeline already has the correct guard (userId: null for invited reporters) — the machine info tab should follow the same pattern.

---

Reviewed by scheduled Claude security routine. 33 PRs total this week; 16 reviewed in depth, 17 skipped (deps bumps, CI plumbing, docs).