20 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 05-web-push-notifications | 2026-06-09T00:00:00Z | standard | 20 |
|
|
issues_found |
Phase 5: Code Review Report
Reviewed: 2026-06-09 Depth: standard Files Reviewed: 20+ Status: issues_found
Summary
Phase 5 delivers Web Push across three trigger paths (NOTIF-01/02/03) plus the iOS reliability infrastructure. The architecture is sound: VAPID signing is delegated to web-push, the D-03 actor-suppression filter is applied both at the DB layer and in application code, event.waitUntil() is used correctly in the SW, and the D-13 boundary (no tsdav in notification code) is respected.
Four blockers were found. Two are correctness bugs: (1) the sentReminders set in reminderScheduler.ts grows without bound and is never pruned — it will leak memory and eventually begin skipping due reminders after a process restart populates a new set without stale guard entries; (2) the uniq_push_endpoint unique constraint is defined on a text column in the migration SQL, which MariaDB/InnoDB silently truncates to 767 bytes when creating the index — a subscription with a very long endpoint can collide with or fail to match a different subscription, causing phantom prune-without-delete or missed 410 cleanup. Two are security: (3) VAPID_PRIVATE_KEY is passed via docker-compose.yml environment block from the host .env but with no fallback (${VAPID_PRIVATE_KEY} with no :- default) — Docker Compose will error at startup if the variable is unset rather than leaving push non-functional, which is correct, but the VAPID private key has no runtime check that it is well-formed before setVapidDetails is called, and invalid keys are only warned, not blocked; and (4) the notificationclick handler in sw.ts uses client.url === url for exact-URL matching, which means a focused-window match will never fire for notifications with query-string deep-links whose current window URL does not exactly match (e.g. /calendar?date=2026-06-09&event=uid vs /calendar) — this is a functional regression, not a security issue, but is reclassified as a correctness blocker because the D-14 deep-link requirement is unmet for the most common case.
Critical Issues
CR-01: sentReminders Set Grows Without Bound (Memory Leak + Correctness)
File: apps/api/src/broker/reminderScheduler.ts:38
Issue: sentReminders is a module-level Set<string> that accumulates one entry per ${uid}:${minuteBucket} every minute for every shared timed event. Entries are never removed. For an app running continuously with N shared events, the set grows by N entries/minute indefinitely. After a long uptime the set consumes significant memory. More critically: the deduplication key includes minuteBucket = floor(ms / 60000), so after a process restart the set is empty and an event that fired in the previous minute (same wall-clock minute bucket) will fire again — the dedup logic is only effective within a single process lifetime. This is documented as "acceptable data loss on restart" for the double-fire case, but the unbounded growth is not documented and constitutes a bug.
Fix: Prune entries for past minute buckets after each scan. After the dispatch loop completes, remove any set entry whose minuteBucket is strictly less than currentBucket - 1:
// After the dispatch loop in runReminderCheck():
const staleBucket = minuteBucket - 1
for (const key of sentReminders) {
const [, bucketStr] = key.split(':')
if (Number(bucketStr) < staleBucket) {
sentReminders.delete(key)
}
}
CR-02: uniq_push_endpoint Unique Constraint on text Column Will Silently Truncate in MariaDB/InnoDB
File: apps/api/src/db/migrations/0003_same_xavin.sql:10 and apps/api/src/db/schema.ts:243-244
Issue: endpoint is declared text NOT NULL in both the schema and migration, and CONSTRAINT uniq_push_endpoint UNIQUE(endpoint) is applied to it. MariaDB/InnoDB cannot index an unbounded text column directly — it requires a prefix length. The behavior differs by server version and innodb_large_prefix setting: some versions silently create a prefix index (767 bytes by default, 3072 bytes with ROW_FORMAT=DYNAMIC), others fail. In either case a silent prefix match means two endpoints that share the same first 767 characters — which is possible for push endpoints from the same push service domain — are treated as duplicates. This makes the upsert in push.ts's onDuplicateKeyUpdate unreliable: a second distinct subscription may silently collide, or the migration may fail entirely on strict-mode MariaDB.
The correct fix is to define endpoint as varchar(2048) (matching the Zod validation bound of z.string().url().max(2048) in routes/push.ts:61) in both the schema and migration.
Fix:
-- Migration: change endpoint column type
ALTER TABLE `push_subscriptions`
MODIFY COLUMN `endpoint` varchar(2048) NOT NULL;
-- Schema: change text() to varchar
endpoint: varchar('endpoint', { length: 2048 }).notNull(),
The Zod schema in push.ts already enforces max(2048), so server-validated inputs fit within the column. Update p256dh similarly — it is also text but a varchar(512) would match the Zod bound.
CR-03: notificationclick Deep-Link Window Focus Uses Exact URL Match — D-14 Never Fires for Query-String Deep-Links
File: apps/pwa/src/sw.ts:154
Issue: The notificationclick handler matches client.url === url to find an existing window. Push deep-link URLs for event notifications are of the form /calendar?event=<uid> or /calendar?date=<date>&event=<uid>. When the user has the PWA open on /calendar (the root path, no query string), client.url will be https://host/calendar but url will be https://host/calendar?event=uid — the exact comparison fails, so the handler falls through to openWindow and opens a second PWA window instead of focusing the existing one. On iOS standalone mode, openWindow does NOT open a second window; it navigates the existing window, so the net behavior is correct on iOS. On Android Chrome it opens a second window. The deeper problem: the handler never navigates the focused window to the deep-link URL; it only calls focus() without navigate() — so even when the match succeeds (notification for the currently-open URL), the window is focused at the current route, not the notification target.
Fix: Use clients.matchAll to find any window on the same origin, focus it, and call client.navigate(url):
self.addEventListener('notificationclick', (event: NotificationEvent) => {
event.notification.close()
const url: string =
typeof event.notification.data?.url === 'string'
? event.notification.data.url
: '/'
event.waitUntil(
self.clients
.matchAll({ type: 'window', includeUncontrolled: true })
.then((clientList) => {
// Focus any existing window on this origin and navigate it
for (const client of clientList) {
if ('focus' in client) {
return (client as WindowClient).focus().then(() =>
(client as WindowClient).navigate(url)
)
}
}
if (self.clients.openWindow) {
return self.clients.openWindow(url)
}
}),
)
})
CR-04: subscribe() in usePushSubscription Calls fetchVapidKey() (an async await) Before pushManager.subscribe() — iOS Gesture Gate May Be Broken on Cache Miss
File: apps/pwa/src/hooks/usePushSubscription.ts:197-206
Issue: The subscribe() function unconditionally awaits fetchVapidKey() before calling registration.pushManager.subscribe(). The comment says "Fetch VAPID key (from cache if already prefetched)" and the VAPID key is pre-fetched in a useEffect by PushPermissionPrompt. However:
- The
useEffectpre-fetch is fire-and-forget (void prefetchVapidKey()). There is no guarantee it completes before the user taps "Enable Notifications" — especially on a slow connection on first install. - If
sessionStorageis unavailable (private mode, quota exceeded) or the pre-fetch failed silently,fetchVapidKey()will make a network round-trip insidesubscribe(). - iOS requires
pushManager.subscribe()to be called synchronously within the user gesture's call stack. Anawait fetch(...)beforepushManager.subscribe()breaks the gesture gate — the iOS browser will throwNotAllowedErrorwith the message "The operation was not permitted." The existing code has thisawaiton the very first line ofsubscribe().
The setEnabled(true) path in the same hook has the same structure (await fetchVapidKey() before pushManager.subscribe()), but setEnabled(true) is only called when Notification.permission === 'granted' (OS already approved), so no OS dialog is shown — the gesture gate for permission is not relevant there. For the subscribe() path used from PushPermissionPrompt and SettingsSheet, the gate IS relevant when Notification.permission === 'default'.
Fix: Pre-fetch the VAPID key synchronously into a variable before the tap, and pass it as an argument to subscribe(). Remove the fetchVapidKey() call from inside subscribe():
// In PushPermissionPrompt: pre-fetch before the button renders
const [vapidKey, setVapidKey] = useState<string | null>(null)
useEffect(() => {
fetchVapidKey().then(setVapidKey).catch(() => {})
}, [])
// subscribe() signature change:
const subscribe = async (
registration: ServiceWorkerRegistration,
vapidKey: string,
): Promise<void> => {
// No await before this line — iOS gesture gate preserved
const sub = await registration.pushManager.subscribe({
userVisibleOnly: true,
applicationServerKey: urlBase64ToUint8Array(vapidKey),
})
// ... POST to server
}
If a pre-pass API approach is not desirable, the minimum safe fix is to ensure fetchVapidKey() always resolves from sessionStorage (never from the network) when called from within a tap handler, by making the pre-fetch mandatory and blocking the button render until the key is ready:
// Disable the "Enable Notifications" button until vapidKey is loaded
<button disabled={!vapidKey} onClick={handleEnableClick}>
Warnings
WR-01: sentReminders Dedup Key Survives Process Restart — Same-Minute Double-Fire Is Possible
File: apps/api/src/broker/reminderScheduler.ts:133-134
Issue: The dedup comment documents "Acceptable data loss on process restart for a two-person household." This is a reasonable MVP trade-off, but the dedup fires AFTER marking the key in the set (sentReminders.add(key) before dispatchPush). If dispatchPush throws after the key is added (a non-410/404 failure), the event is marked as sent but no notification was delivered, and the set prevents retry in the same minute bucket. Combined with a process restart before the next bucket, no notification is ever delivered for that event.
Fix: Move sentReminders.add(key) to after the fan-out loop succeeds (or accept that at-least-once delivery requires not pre-marking):
// Only mark sent after all dispatches have been attempted
for (const sub of event.subs) {
try { await dispatchPush(sub, notification) } catch { /* log */ }
}
sentReminders.add(key) // moved here
WR-02: listChangeDispatcher Unused Import and
File: apps/api/src/lib/listChangeDispatcher.ts:20
Issue: and is imported from drizzle-orm but never used in the file. inArray and eq are used; and is dead.
Fix:
import { eq, inArray } from 'drizzle-orm'
WR-03: push.ts POST /subscription Catches and Logs the Full Error Object (Potential Key Leakage)
File: apps/api/src/routes/push.ts:116-118
Issue: The catch block logs err directly via console.error('[push/POST /subscription] DB operation failed:', err). If the DB error includes the failing query parameters in its message (e.g. MariaDB's ER_DUP_ENTRY sometimes includes the duplicate value), the p256dh or auth values from the request body could appear in server logs. The threat model (T-05-03) explicitly says "never logs subscription keys."
Fix: Log only err instanceof Error ? err.message : String(err) — do not pass the raw error object:
} catch (err) {
console.error(
'[push/POST /subscription] DB operation failed:',
err instanceof Error ? err.message : String(err),
)
return c.json({ error: 'Service unavailable' }, 503)
}
Apply the same fix to the DELETE handler's catch at line 136.
WR-04: sync.ts Emits delete Change Events Before the DB DELETE — Race Condition in Fan-Out
File: apps/api/src/broker/sync.ts:242-259
Issue: Deleted UIDs are added to changes (lines 252-258) before the db.delete() call that removes them from the cache (lines 262-268). The onChanges callback is fired at line 272-274 after the delete, but within onChanges (in poller.ts and outboxWorker.ts) dispatchEventChange is called as fire-and-forget with .catch(). There is no await on the notification dispatch before syncCalendar returns. The sequence is:
changes.push({ operation: 'delete', uid })— collecteddb.delete(calendarEvents)— DB row removedonChanges(changes)— firesdispatchEventChangefire-and-forgetsyncCalendarreturns
If a rapid second poll cycle begins at step 4 (before the fire-and-forget dispatch at step 3 completes), the deleted uid may be re-fetched from Fastmail (if it was not actually deleted on the server — false delete detection from a transient REPORT response) and re-inserted, then dispatchEventChange fires with operation: 'delete' for an event that now exists again in the cache. This is an edge case in the two-person household but worth documenting: the delete change detection and DB delete should be atomic, or the changes collection for deletes should happen after the DB delete.
Fix: Collect delete changes AFTER the db.delete():
// 5. Delete pruned rows from DB first
if (seenUids.length > 0) {
await db.delete(calendarEvents)
.where(and(eq(calendarEvents.calendarId, cal.id), notInArray(calendarEvents.uid, seenUids)))
} else {
await db.delete(calendarEvents).where(eq(calendarEvents.calendarId, cal.id))
}
// THEN collect delete changes for onChanges
if (onChanges && cachedRows) {
for (const row of cachedRows) {
changes.push({ uid: row.uid, title: row.title ?? null, operation: 'delete' })
}
}
WR-05: Health-Check Re-Subscribe in usePushSubscription Silently Re-Subscribes Without Checking Server-Side Record
File: apps/pwa/src/hooks/usePushSubscription.ts:166-178
Issue: The health-check useEffect (lines 152-183) detects a missing browser-side subscription and silently re-subscribes. The POST to /api/push/subscription will upsert (due to onDuplicateKeyUpdate on endpoint). However, if pushManager.getSubscription() returns a subscription object (line 157: setIsSubscribed(true); return) but the server-side record was deleted (e.g. pruned by a 410), the isSubscribed state shows true but notifications silently fail because the server has no record to dispatch to. The health-check does not verify the server-side record exists.
This is a silent failure mode: the user sees "notifications on" (isSubscribed=true), the app sends no notification, and the user never knows. Reminders and alerts are missed with no indication.
Fix: After existingSub is found in the browser, POST it to the server as a health confirmation (the upsert is idempotent):
if (existingSub) {
// Re-confirm server-side record (handles 410-prune recovery)
await fetch('/api/push/subscription', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
credentials: 'include',
body: JSON.stringify(existingSub.toJSON()),
}).catch(() => {}) // non-blocking
setIsSubscribed(true)
return
}
Info
IN-01: eventChangeDispatcher.ts Does Not Name the Actor in Notification Copy (D-02/D-03 Partial Implementation)
File: apps/api/src/lib/eventChangeDispatcher.ts:83
Issue: The buildCopy function uses generic copy ('New calendar event', 'Calendar event updated') rather than naming the actor per D-02/D-03 spec ("Lucas added…", "Wife moved Dentist → Wed 3pm"). The comment at line 81-82 acknowledges this: "actorName resolution (querying users table) is a future D-02 enhancement." The acceptance criteria spec requires actor attribution. For the two-person household this means the notification body only shows the event title, not who made the change, which is less useful and technically incomplete against D-03's "name the actor" requirement.
Fix: Query users.displayName for actorUserId at the top of dispatchEventChange and pass it to buildCopy:
const actorRow = await db.select({ displayName: users.displayName })
.from(users).where(eq(users.id, actorUserId)).limit(1)
const actorName = actorRow[0]?.displayName ?? 'A family member'
// Pass actorName into buildCopy
IN-02: docker-compose.yml VAPID Variables Have No Fallback — Compose Startup Fails if Unset
File: docker-compose.yml:29-31
Issue: VAPID_PUBLIC_KEY: ${VAPID_PUBLIC_KEY}, VAPID_PRIVATE_KEY: ${VAPID_PRIVATE_KEY}, and VAPID_SUBJECT: ${VAPID_SUBJECT} have no :- fallbacks. If any of these are unset in the operator's .env, Docker Compose fails to start the API service entirely (variable is not set error) rather than starting with push disabled. The index.ts startup guard (lines 118-128) already handles missing VAPID vars gracefully with a console.warn, but that logic is never reached because Compose errors first.
This is an operator experience issue, not a security issue (keeping VAPID_PRIVATE_KEY out of source control is correct). Adding empty-string fallbacks would allow the container to start and push to fail gracefully:
VAPID_PUBLIC_KEY: ${VAPID_PUBLIC_KEY:-}
VAPID_PRIVATE_KEY: ${VAPID_PRIVATE_KEY:-}
VAPID_SUBJECT: ${VAPID_SUBJECT:-}
IN-03: PushPermissionPrompt Uses Math.random() for headingId — Stable Between Renders But Unstable Across Mounts
File: apps/pwa/src/components/PushPermissionPrompt.tsx:63
Issue: const headingId = useRef(push-prompt-heading-${Math.random().toString(36).slice(2)}) generates a random ID at component mount. This is stable within a mount lifecycle but changes each time the component unmounts and remounts (e.g. App.tsx renders <PushPermissionPrompt /> at the root, and the component may remount on navigation). The aria-labelledby/id pairing is still valid within a mount, so there is no accessibility regression per se, but React's Strict Mode double-invocation in dev will produce two different IDs for what is logically the same element. Use useId() (React 18+) instead.
Fix:
import { useId } from 'react'
// inside the component:
const headingId = useId()
Reviewed: 2026-06-09 Reviewer: Claude (gsd-code-reviewer) Depth: standard