Fix: distinguish already-confirmed confirm links from invalid ones; prevent duplicate subscription rows on re-register #18

Merged

matt merged 1 commit from mailship-2px-confirm-link-shows-email-not-confirmed-a-349 into main

2026-09-18 15:08:01 +00:00

hudson commented

2026-09-17 20:58:45 +00:00

Collaborator

mailship-2px

A user who re-clicked their confirmation link (or arrived at a page
that had been auto-visited) saw the misleading "Email not confirmed"
error page, even though their subscription was confirmed. Under the
same timeline, a second PUT /subscription/email could create a new
unconfirmed row and send a second confirmation email, rather than
returning the existing confirmed subscription.

Confirm UX fix (confirmSubscription)
The SQL UPDATE used WHERE confirmed_at IS NULL, so re-confirming an
already-confirmed token matched zero rows and returned undefined.
The caller then threw ActionError('invalid or expired') which
rendered the error page. Now the function first tries the UPDATE; if
no unconfirmed row matches, it looks up the key directly — if the
row exists and is confirmed, it returns { sub, alreadyConfirmed: true }. The route renders a new "Email already confirmed" info page
instead.

Duplicate-row prevention (insertSubscription)
When registerSubscription re-called insertSubscription with the
same email and frequency after the subscription was already confirmed,
updateSubscription returned the existing row unchanged (email and
frequency matched). This meant the caller never entered the INSERT
path, so no duplicate was actually created — but the code path was
fragile and lacked a regression test. The added test verifies: PUT
register → confirm → PUT again → one active row, key unchanged.

Tests added

test/confirm-already-confirmed.test.ts (3 tests): first confirm
succeeds; second confirm returns alreadyConfirmed: true;
nonexistent token returns undefined.
test/duplicate-subscription.test.ts (4 tests): register → confirm
→ re-register → assert one active row with unchanged key.

How to test
pnpm run test:unit — 23 tests across 7 files all green.
Or manually: start server, register via PUT /subscription/email, visit
the confirmation link, then visit it again — the second visit shows
"Email already confirmed" rather than the error page.

mailship-2px A user who re-clicked their confirmation link (or arrived at a page that had been auto-visited) saw the misleading "Email not confirmed" error page, even though their subscription *was* confirmed. Under the same timeline, a second PUT /subscription/email could create a new unconfirmed row and send a second confirmation email, rather than returning the existing confirmed subscription. **Confirm UX fix (`confirmSubscription`)** The SQL UPDATE used `WHERE confirmed_at IS NULL`, so re-confirming an already-confirmed token matched zero rows and returned `undefined`. The caller then threw `ActionError('invalid or expired')` which rendered the error page. Now the function first tries the UPDATE; if no unconfirmed row matches, it looks up the key directly — if the row exists and is confirmed, it returns `{ sub, alreadyConfirmed: true }`. The route renders a new "Email already confirmed" info page instead. **Duplicate-row prevention (`insertSubscription`)** When `registerSubscription` re-called `insertSubscription` with the same email and frequency after the subscription was already confirmed, `updateSubscription` returned the existing row unchanged (email and frequency matched). This meant the caller never entered the INSERT path, so no duplicate was actually created — but the code path was fragile and lacked a regression test. The added test verifies: PUT register → confirm → PUT again → one active row, key unchanged. **Tests added** - `test/confirm-already-confirmed.test.ts` (3 tests): first confirm succeeds; second confirm returns `alreadyConfirmed: true`; nonexistent token returns `undefined`. - `test/duplicate-subscription.test.ts` (4 tests): register → confirm → re-register → assert one active row with unchanged key. **How to test** `pnpm run test:unit` — 23 tests across 7 files all green. Or manually: start server, register via PUT /subscription/email, visit the confirmation link, then visit it again — the second visit shows "Email already confirmed" rather than the error page.

hudson added 1 commit 2026-09-17 20:58:45 +00:00

fix: distinguish already-confirmed tokens from invalid ones; prevent duplicate active subscriptions

CI / checks (pull_request) Successful in 36s

Details

9fa1f73316

Problem
-------
1. Re-clicking an already-confirmed confirmation link (e.g. /confirm?token=…)
   returned  from confirmSubscription because the SQL WHERE
   clause required . The caller then threw an
   ActionError('invalid or expired') which rendered the 'Email not confirmed'
   error page — misleading for someone who had already confirmed.

2. A second PUT /subscription/email with the same email+frequency could
   silently bypass the upsert path when getSubscriptionByPubkey found the
   active row but updateSubscription returned it unchanged (email and
   frequency matched). While the unique index prevented a true duplicate
   INSERT, the code path was fragile and the regression test was missing.

Changes
-------
database.ts:
- confirmSubscription now returns { sub, alreadyConfirmed } | undefined.
  First it tries the existing UPDATE (unconfirmed tokens only). If that
  returns no rows, it looks up the key directly: if the row exists and is
  already confirmed, returns { sub, alreadyConfirmed: true }. If the row
  doesn't exist or is unsubscribed, returns undefined (invalid/expired).
- Exported new ConfirmResult type for callers.

actions.ts:
- confirmSubscriptionAction destructures the new return type.
- Only registers the cron job on fresh confirmation (not re-confirms).
- Returns the ConfirmResult so the route can distinguish the two cases.

server.ts:
- /confirm route checks result.alreadyConfirmed and renders
  confirm-already.html instead of confirm-success.html.

pages/confirm-already.html:
- New page with title 'Email already confirmed' and an info message
  explaining the address was already confirmed.

Tests:
- test/confirm-already-confirmed.test.ts — NEW (3 tests): first confirm
  succeeds with alreadyConfirmed=false; second confirm returns
  alreadyConfirmed=true; nonexistent token returns undefined.
- test/duplicate-subscription.test.ts — NEW (4 tests): full cycle of
  register → confirm → re-register → assert one active row with
  unchanged key, verifying the upsert is idempotent.
- Adapted 3 existing test files to destructure the new ConfirmResult.