From 3242cf92e468ac16b60047ae74b0ba83f3ecc83a Mon Sep 17 00:00:00 2001 From: threebeats Date: Fri, 31 Jul 2026 13:28:02 -0400 Subject: [PATCH 1/2] fix: atomic promo redemption duplicate guard (fixes #92) Close the TOCTOU between quotePromo()'s duplicate check and recordPromoRedemption()'s insert by making the redemption row itself the single atomic authority: INSERT OR IGNORE + rowsAffected check. Returns whether the redemption was newly recorded; skips the uses increment when the row already exists. Also keeps the capped increment (uses < max_uses) so over-redemption is impossible even if the quote check races. Tests: new recordPromoRedemption suite (records + bumps, uppercase normalization, duplicate no-op, blank code). --- apps/web/lib/entitlements.test.ts | 45 +++++++++++++++++++++++++++---- apps/web/lib/entitlements.ts | 31 ++++++++++++++------- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/apps/web/lib/entitlements.test.ts b/apps/web/lib/entitlements.test.ts index b26c361..a0a476e 100644 --- a/apps/web/lib/entitlements.test.ts +++ b/apps/web/lib/entitlements.test.ts @@ -1,16 +1,15 @@ -import { describe, expect, it, vi } from "vitest"; +import { describe, expect, it, vi, beforeEach } from "vitest"; vi.mock("server-only", () => ({})); +const execute = vi.fn(); vi.mock("./db", () => ({ - sqlClient: { - execute: vi.fn(), - }, + sqlClient: { execute }, })); vi.mock("@aiornot/db", () => ({ newId: () => "key_test", })); -import { normalizeApiKeyLabel } from "./entitlements"; +import { normalizeApiKeyLabel, recordPromoRedemption } from "./entitlements"; describe("normalizeApiKeyLabel", () => { it("trims and collapses labels before storing them", () => { @@ -26,3 +25,39 @@ describe("normalizeApiKeyLabel", () => { expect(normalizeApiKeyLabel("a".repeat(80))).toHaveLength(60); }); }); + +describe("recordPromoRedemption (atomic duplicate guard, #92)", () => { + beforeEach(() => { + execute.mockReset(); + }); + + it("records a new redemption and bumps uses", async () => { + execute.mockResolvedValueOnce({ rows: [], rowsAffected: 1 }); // INSERT OR IGNORE + execute.mockResolvedValueOnce({ rows: [], rowsAffected: 1 }); // UPDATE uses + const ok = await recordPromoRedemption("summer25", "user_1"); + expect(ok).toBe(true); + expect(execute).toHaveBeenCalledTimes(2); + expect(String(execute.mock.calls[0][0].sql)).toContain("INSERT OR IGNORE"); + expect(String(execute.mock.calls[1][0].sql)).toContain("uses < max_uses"); + }); + + it("normalizes the code to uppercase", async () => { + execute.mockResolvedValueOnce({ rows: [], rowsAffected: 1 }); + execute.mockResolvedValueOnce({ rows: [], rowsAffected: 1 }); + await recordPromoRedemption(" summer25 ", "user_1"); + expect(execute.mock.calls[0][0].args).toEqual(["SUMMER25", "user_1"]); + }); + + it("returns false and skips the increment when the row already exists", async () => { + execute.mockResolvedValueOnce({ rows: [], rowsAffected: 0 }); // duplicate — INSERT OR IGNORE no-op + const ok = await recordPromoRedemption("SUMMER25", "user_1"); + expect(ok).toBe(false); + expect(execute).toHaveBeenCalledTimes(1); // no UPDATE issued + }); + + it("returns false for a blank code", async () => { + const ok = await recordPromoRedemption(" ", "user_1"); + expect(ok).toBe(false); + expect(execute).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/lib/entitlements.ts b/apps/web/lib/entitlements.ts index a8fc135..b02c40e 100644 --- a/apps/web/lib/entitlements.ts +++ b/apps/web/lib/entitlements.ts @@ -140,16 +140,29 @@ export async function quotePromo( return { ok: true, code, percentOff, baseUsd, finalUsd, free: finalUsd <= 0 }; } -/** Record a promo redemption (idempotent). Call once the discount is actually granted. */ -export async function recordPromoRedemption(codeRaw: string, userId: string): Promise { +/** + * Record a promo redemption. Returns true if this call newly recorded the + * redemption, false if it was already recorded for this user+code. + * + * Uses INSERT OR IGNORE + a rowsAffected check so the duplicate guard is a + * single atomic statement instead of the separate check-then-record pair in + * quotePromo()/recordPromoRedemption() (fixes #92, TOCTOU). The composite PK + * (code, user_id) makes the INSERT itself the source of truth. + */ +export async function recordPromoRedemption(codeRaw: string, userId: string): Promise { const code = codeRaw.trim().toUpperCase(); - if (!code) return; - try { - await sqlClient.execute({ sql: "INSERT INTO promo_redemptions (code, user_id) VALUES (?, ?)", args: [code, userId] }); - await sqlClient.execute({ sql: "UPDATE promo_codes SET uses = uses + 1 WHERE code = ?", args: [code] }); - } catch { - /* already recorded — composite PK makes this a no-op */ - } + if (!code) return false; + const res = await sqlClient.execute({ + sql: "INSERT OR IGNORE INTO promo_redemptions (code, user_id) VALUES (?, ?)", + args: [code, userId], + }); + if (Number(res.rowsAffected) !== 1) return false; // already recorded + // Atomic capped increment (also covered by #106): never over-redeem past max_uses. + await sqlClient.execute({ + sql: "UPDATE promo_codes SET uses = uses + 1 WHERE code = ? AND (max_uses IS NULL OR uses < max_uses)", + args: [code], + }); + return true; } /** Apply the entitlement for a purpose directly (used for 100%-off comps). */ From ce8e975152909ff7168271480a1988304efbe4f8 Mon Sep 17 00:00:00 2001 From: threebeats Date: Fri, 31 Jul 2026 14:08:01 -0400 Subject: [PATCH 2/2] test: fix vi.mock hoisting in entitlements test Same TDZ fix as the rate-limit test: create the mocked execute via vi.hoisted() so the hoisted vi.mock factory doesn't reference it before initialization. --- apps/web/lib/entitlements.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/web/lib/entitlements.test.ts b/apps/web/lib/entitlements.test.ts index a0a476e..6e65896 100644 --- a/apps/web/lib/entitlements.test.ts +++ b/apps/web/lib/entitlements.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; vi.mock("server-only", () => ({})); -const execute = vi.fn(); +// vi.mock factories are hoisted above top-level consts — create the mock fn +// via vi.hoisted() to avoid a temporal-dead-zone ReferenceError. +const { execute } = vi.hoisted(() => ({ execute: vi.fn() })); vi.mock("./db", () => ({ sqlClient: { execute }, }));