From a9d3de658e9e7f5afd5c0165738a1e7c4d981e5d Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Sat, 6 Jun 2026 22:32:10 -0400 Subject: [PATCH] fix(03): correct event-write timezone + per-user calendar identity (Gate 2 Part D) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG A — timed events written 4h off: EventForm sent a naive local wall-clock string with no offset; the UTC API container parsed it via new Date() as UTC, so 09:00 America/Toronto serialized to DTSTART:...090000Z. Fix: new apps/pwa/src/lib/eventDateTime.ts serializes timed events to an unambiguous UTC instant in the browser (where the operator's zone is known); all-day stays a DATE string. No backend change. BUG B — created events attached to the wrong user's calendar + duplicate calendar rows per poll: calendars had no unique key on url, and poller/sync matched calendars by url alone — so under the shared single Fastmail account (D-16) one member's collection resolved to the other member's row. Fix: composite unique(user_id, url); scope poller lookup + sync select to (userId, url); hand migration 0001 (dedup + add key), applied to the live DB. Regression tests fail against the buggy url-only predicate. API 98/98, PWA 140/140, tsc clean both packages. --- apps/api/src/broker/poller.ts | 10 ++- apps/api/src/broker/sync.ts | 13 +++- .../0001_calendars_user_url_unique.sql | 41 +++++++++++++ apps/api/src/db/schema.ts | 11 +++- apps/api/tests/broker/poller.test.ts | 38 ++++++++++++ apps/api/tests/broker/sync.test.ts | 46 ++++++++++++++ apps/pwa/src/components/EventForm.tsx | 17 +++++- apps/pwa/src/lib/eventDateTime.test.ts | 53 ++++++++++++++++ apps/pwa/src/lib/eventDateTime.ts | 61 +++++++++++++++++++ 9 files changed, 281 insertions(+), 9 deletions(-) create mode 100644 apps/api/src/db/migrations/0001_calendars_user_url_unique.sql create mode 100644 apps/pwa/src/lib/eventDateTime.test.ts create mode 100644 apps/pwa/src/lib/eventDateTime.ts diff --git a/apps/api/src/broker/poller.ts b/apps/api/src/broker/poller.ts index fe32f63..7953abc 100644 --- a/apps/api/src/broker/poller.ts +++ b/apps/api/src/broker/poller.ts @@ -16,7 +16,7 @@ */ import { schedule } from 'node-cron' -import { eq } from 'drizzle-orm' +import { and, eq } from 'drizzle-orm' import { db } from '../db/client.js' import { memberCredentials, calendars } from '../db/schema.js' import { decryptPassword } from './crypto.js' @@ -44,11 +44,15 @@ export async function runPoll(): Promise { const davCalendars = await client.fetchCalendars() for (const davCal of davCalendars) { - // Look up the stored calendar row to get the known ctag (D-13) + // Look up the stored calendar row to get the known ctag (D-13). + // BUG B: scope by (userId, url). The two members share one Fastmail account + // (D-16), so the same collection URL exists for both. A url-only predicate + // matched the OTHER member's row → wrong ctag compared, and syncCalendar + // wrote events under the wrong calendar. Match on this member's row only. const [stored] = await db .select() .from(calendars) - .where(eq(calendars.url, davCal.url)) + .where(and(eq(calendars.userId, cred.userId), eq(calendars.url, davCal.url))) .limit(1) // ctag/syncToken: defensive null handling (Pitfall #6) diff --git a/apps/api/src/broker/sync.ts b/apps/api/src/broker/sync.ts index f4e6c15..b793e61 100644 --- a/apps/api/src/broker/sync.ts +++ b/apps/api/src/broker/sync.ts @@ -18,7 +18,7 @@ import type { DAVCalendar } from 'tsdav' import type { FastmailClient } from './client.js' import ICAL from 'ical.js' -import { eq } from 'drizzle-orm' +import { and, eq } from 'drizzle-orm' import { db } from '../db/client.js' import { calendars, calendarEvents } from '../db/schema.js' @@ -56,10 +56,17 @@ export async function syncCalendar( }) // 2. Select the calendar row to get its DB id (insertId is unreliable on ON DUPLICATE KEY UPDATE). - const [cal] = await db.select().from(calendars).where(eq(calendars.url, davCal.url)).limit(1) + // BUG B: scope by (userId, url) — the same collection URL exists for both members + // (shared Fastmail account, D-16). A url-only lookup returned the OTHER member's + // row (lowest id), so events were cached under the wrong calendarId. + const [cal] = await db + .select() + .from(calendars) + .where(and(eq(calendars.userId, userId), eq(calendars.url, davCal.url))) + .limit(1) if (!cal) { // Should never happen — we just upserted it - throw new Error(`syncCalendar: could not find calendar row for url=${davCal.url}`) + throw new Error(`syncCalendar: could not find calendar row for userId=${userId} url=${davCal.url}`) } // 3. Fetch all calendar objects (REPORT calendar-query). diff --git a/apps/api/src/db/migrations/0001_calendars_user_url_unique.sql b/apps/api/src/db/migrations/0001_calendars_user_url_unique.sql new file mode 100644 index 0000000..5ec3d93 --- /dev/null +++ b/apps/api/src/db/migrations/0001_calendars_user_url_unique.sql @@ -0,0 +1,41 @@ +-- BUG B — calendars: add composite unique key (user_id, url). +-- +-- Context (D-16): the two household members share ONE Fastmail account, so the +-- SAME collection URL is polled by both credentials. The calendar upsert keyed on +-- url alone never triggered onDuplicateKeyUpdate (no unique key on url), so every +-- poll inserted a fresh calendar row; the url-only lookup then resolved to the +-- other member's row, caching events under the wrong calendarId. +-- +-- This migration is hand-written (not drizzle-kit generated) because drizzle-kit +-- push is unsafe on populated MariaDB (false destructive diffs) and there is no +-- migrations baseline. Apply it directly to the live DB before/with the image +-- rebuild that ships the schema + broker fixes. +-- +-- Order matters: duplicate (user_id, url) rows must be collapsed BEFORE the unique +-- key is added, or ADD UNIQUE fails. We keep the LOWEST id per (user_id, url), +-- repoint any cached events from the loser rows onto the keeper, then delete losers. + +-- 1. Repoint calendar_events from duplicate calendar rows onto the keeper +-- (lowest id) for each (user_id, url) group. +UPDATE calendar_events ce +JOIN calendars dup ON dup.id = ce.calendar_id +JOIN ( + SELECT user_id, url, MIN(id) AS keep_id + FROM calendars + GROUP BY user_id, url +) keeper ON keeper.user_id = dup.user_id AND keeper.url = dup.url +SET ce.calendar_id = keeper.keep_id +WHERE ce.calendar_id <> keeper.keep_id; + +-- 2. Delete the duplicate (loser) calendar rows, keeping the lowest id per group. +DELETE c FROM calendars c +JOIN ( + SELECT user_id, url, MIN(id) AS keep_id + FROM calendars + GROUP BY user_id, url +) keeper ON keeper.user_id = c.user_id AND keeper.url = c.url +WHERE c.id <> keeper.keep_id; + +-- 3. Add the composite unique key that makes the upsert idempotent per (user_id, url). +ALTER TABLE calendars + ADD CONSTRAINT uniq_calendar_user_url UNIQUE (user_id, url); diff --git a/apps/api/src/db/schema.ts b/apps/api/src/db/schema.ts index 7c24883..8740eae 100644 --- a/apps/api/src/db/schema.ts +++ b/apps/api/src/db/schema.ts @@ -73,7 +73,16 @@ export const calendars = mysqlTable( lastSyncedAt: timestamp('last_synced_at'), isShared: boolean('is_shared').default(false).notNull(), }, - (t) => [index('idx_calendars_user_id').on(t.userId)], + (t) => [ + index('idx_calendars_user_id').on(t.userId), + // BUG B: calendar identity is (userId, url), not url alone. The two household + // members share one Fastmail account (D-16), so the SAME collection URL is + // polled by both credentials. Without this unique key the calendar upsert's + // onDuplicateKeyUpdate never fired → a new row per poll, and the url-only + // lookup resolved to the other member's row → events cached under the wrong + // calendarId. Keying on (userId, url) makes the upsert idempotent per member. + unique('uniq_calendar_user_url').on(t.userId, t.url), + ], ) /** diff --git a/apps/api/tests/broker/poller.test.ts b/apps/api/tests/broker/poller.test.ts index fc8891d..7a7a7f8 100644 --- a/apps/api/tests/broker/poller.test.ts +++ b/apps/api/tests/broker/poller.test.ts @@ -194,6 +194,44 @@ describe('broker poller — runPoll', () => { expect(mockSyncCalendar).not.toHaveBeenCalled() }) + it('BUG B: scopes the stored-calendar lookup to (userId, url), not url alone', async () => { + // Capture the predicate passed to db.select().from(calendars).where(...). + // The buggy code passed eq(url) only; the fix passes and(eq(userId), eq(url)). + // We serialize the predicate and assert it references the member's user_id column. + const capturedWhere: unknown[] = [] + mockSelectWhere.mockImplementation((pred: unknown) => { + capturedWhere.push(pred) + return { limit: mockSelectLimit } + }) + + const { runPoll } = await import('../../src/broker/poller.js') + + mockCredentialsSelectResult.push({ + id: 7, + userId: 42, + fastmailEmail: 'lucas@fastmail.com', + encryptedPassword: 'enc', + }) + + mockFetchCalendars.mockResolvedValue([ + { url: 'https://caldav.fastmail.com/cal/', displayName: 'Calendar', ctag: 'c', syncToken: null }, + ]) + + await runPoll() + + expect(capturedWhere.length).toBeGreaterThan(0) + // A composite and(...) predicate exposes multiple queryChunks; a single eq does not + // contain a nested SQL referencing the user_id column. Serialize and inspect. + const pred = capturedWhere[0] as { queryChunks?: unknown[] } + const serialized = JSON.stringify(pred, (_k, v) => + typeof v === 'object' && v !== null && 'name' in (v as Record) + ? (v as { name?: unknown }).name + : v, + ) + expect(serialized).toContain('user_id') + expect(serialized).toContain('url') + }) + it('processes all member credentials in a poll cycle', async () => { const { runPoll } = await import('../../src/broker/poller.js') diff --git a/apps/api/tests/broker/sync.test.ts b/apps/api/tests/broker/sync.test.ts index 3bfc666..dd98af3 100644 --- a/apps/api/tests/broker/sync.test.ts +++ b/apps/api/tests/broker/sync.test.ts @@ -247,6 +247,52 @@ describe('syncCalendar', () => { expect(eventUpdateArg.set).toHaveProperty('hasRrule', true) }) + it('BUG B: scopes the calendar-row select to (userId, url), not url alone', async () => { + // Capture the predicate passed to db.select().from(calendars).where(...).limit(1). + const capturedWhere: unknown[] = [] + mockWhere.mockImplementation((pred: unknown) => { + capturedWhere.push(pred) + return { limit: mockLimit } + }) + + const { syncCalendar } = await import('../../src/broker/sync.js') + const mockClient = { fetchCalendarObjects: vi.fn().mockResolvedValue([]) } + const mockDavCal = { + url: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/', + displayName: 'Test', + ctag: 'v1', + syncToken: null, + } + + await syncCalendar(mockClient as never, mockDavCal as never, 99) + + expect(capturedWhere.length).toBeGreaterThan(0) + const serialized = JSON.stringify(capturedWhere[0], (_k, v) => + typeof v === 'object' && v !== null && 'name' in (v as Record) + ? (v as { name?: unknown }).name + : v, + ) + expect(serialized).toContain('user_id') + expect(serialized).toContain('url') + }) + + it('BUG B: calendar upsert is idempotent — onDuplicateKeyUpdate fires for the calendar row', async () => { + const { syncCalendar } = await import('../../src/broker/sync.js') + const mockClient = { fetchCalendarObjects: vi.fn().mockResolvedValue([]) } + const mockDavCal = { + url: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/', + displayName: 'Test', + ctag: 'v1', + syncToken: null, + } + await syncCalendar(mockClient as never, mockDavCal as never, 1) + // The calendar insert (call 0) must use onDuplicateKeyUpdate so the (userId,url) + // unique key makes re-polls update-in-place instead of inserting duplicate rows. + expect(mockOnDuplicateKeyUpdate).toHaveBeenCalled() + const calUpdateArg = mockOnDuplicateKeyUpdate.mock.calls[0][0] + expect(calUpdateArg.set).toHaveProperty('ctag') + }) + it('updates the calendar ctag/syncToken after a successful sync', async () => { const { syncCalendar } = await import('../../src/broker/sync.js') diff --git a/apps/pwa/src/components/EventForm.tsx b/apps/pwa/src/components/EventForm.tsx index 6b19d93..706b0c4 100644 --- a/apps/pwa/src/components/EventForm.tsx +++ b/apps/pwa/src/components/EventForm.tsx @@ -40,6 +40,7 @@ import { type RecurrencePreset, } from '../api/client.js' import type { CalendarOccurrence } from '../api/client.js' +import { serializeEventDateTime } from '../lib/eventDateTime.js' // ── Constants ───────────────────────────────────────────────────────────────── @@ -262,11 +263,23 @@ export function EventForm() { const handleSubmit = () => { if (!validate()) return + // BUG A fix: serialize timed events to an unambiguous UTC instant here in + // the browser (operator's zone is known) instead of sending a naive local + // wall-clock string. The API container is UTC; a naive string was being read + // as UTC, shifting 09:00 local to 09:00Z (4h off). See lib/eventDateTime.ts. + const { start: serializedStart, end: serializedEnd } = serializeEventDateTime( + allDay, + startDate, + startTime, + endDate, + endTime, + ) + const payload: CreateEventPayload = { title: title.trim(), allDay, - start: allDay ? startDate : `${startDate}T${startTime}:00`, - end: allDay ? endDate : `${endDate}T${endTime}:00`, + start: serializedStart, + end: serializedEnd, recurrence, ...(location.trim() ? { location: location.trim() } : {}), ...(description.trim() ? { description: description.trim() } : {}), diff --git a/apps/pwa/src/lib/eventDateTime.test.ts b/apps/pwa/src/lib/eventDateTime.test.ts new file mode 100644 index 0000000..32cb997 --- /dev/null +++ b/apps/pwa/src/lib/eventDateTime.test.ts @@ -0,0 +1,53 @@ +/** + * BUG A regression — write-path timezone serialization. + * + * Verifies that timed events are serialized to an unambiguous UTC instant + * (so the operator's wall-clock time round-trips correctly regardless of the + * API container's timezone), while all-day events keep their DATE strings. + * + * The PWA vitest harness runs with TZ=UTC, so the assertions are computed + * relative to the local zone (whatever it is) rather than hard-coding an offset. + * The core guarantee under test: the serialized timed value is a UTC instant + * (ends in 'Z') derived from the LOCAL wall clock — never the naive wall-clock + * string passed through verbatim, and never an instant that loses the local hour. + */ + +import { describe, it, expect } from 'vitest' +import { serializeEventDateTime, localWallClockToUtcIso } from './eventDateTime.js' + +describe('serializeEventDateTime (BUG A — write-path TZ)', () => { + it('serializes a timed start to a UTC instant (ends in Z)', () => { + const { start } = serializeEventDateTime(false, '2026-06-07', '09:00', '2026-06-07', '10:00') + expect(start.endsWith('Z')).toBe(true) + // It must NOT be the naive wall-clock string (the original bug shape). + expect(start).not.toBe('2026-06-07T09:00:00') + }) + + it('the serialized instant round-trips back to the SAME local wall clock', () => { + // This is the heart of BUG A: 09:00 in → 09:00 back out in the operator's zone. + const { start, end } = serializeEventDateTime(false, '2026-06-07', '09:00', '2026-06-07', '10:30') + const startBack = new Date(start) + expect(startBack.getHours()).toBe(9) + expect(startBack.getMinutes()).toBe(0) + const endBack = new Date(end) + expect(endBack.getHours()).toBe(10) + expect(endBack.getMinutes()).toBe(30) + }) + + it('equals the instant new Date(local parts) produces — not a passthrough', () => { + const { start } = serializeEventDateTime(false, '2026-06-07', '09:00', '2026-06-07', '10:00') + expect(start).toBe(new Date('2026-06-07T09:00:00').toISOString()) + }) + + it('leaves all-day events as DATE strings (no time, no Z) — D-13 contract', () => { + const { start, end } = serializeEventDateTime(true, '2026-06-07', '09:00', '2026-06-09', '10:00') + expect(start).toBe('2026-06-07') + expect(end).toBe('2026-06-09') + }) + + it('localWallClockToUtcIso round-trips a local wall clock to a UTC instant', () => { + const iso = localWallClockToUtcIso('2026-06-07', '09:00') + expect(iso.endsWith('Z')).toBe(true) + expect(new Date(iso).getHours()).toBe(9) + }) +}) diff --git a/apps/pwa/src/lib/eventDateTime.ts b/apps/pwa/src/lib/eventDateTime.ts new file mode 100644 index 0000000..d499be3 --- /dev/null +++ b/apps/pwa/src/lib/eventDateTime.ts @@ -0,0 +1,61 @@ +/** + * Event date/time serialization for the write path (BUG A fix). + * + * The bug: EventForm previously sent a NAIVE local wall-clock string + * (`${date}T${time}:00`, e.g. "2026-06-07T09:00:00") with NO timezone offset. + * The API outbox worker then did `new Date(thatString)`, which Node parses in + * the SERVER container's local timezone (UTC in Docker). So "09:00" Toronto + * became 09:00 UTC, and buildVeventString serialized it as `090000Z`, which + * displays back as 05:00 EDT — a 4-hour error. + * + * The fix: serialize timed events to an UNAMBIGUOUS instant here in the browser, + * where the operator's timezone IS known. `new Date(date + 'T' + time)` is parsed + * in the browser's local zone (the operator's wall clock), and `.toISOString()` + * converts that instant to a correct UTC `...Z` string. The worker's `new Date()` + * then parses an unambiguous UTC instant regardless of container timezone, and + * the round-trip preserves the operator's wall-clock time. + * + * All-day events carry no time component and remain `YYYY-MM-DD` DATE strings + * (D-13) — they are timezone-independent by contract and must NOT be converted. + */ + +/** + * Serialize the start/end of an event for the create/update payload. + * + * @param allDay when true, returns the date strings unchanged (DATE contract, D-13) + * @param startDate 'YYYY-MM-DD' + * @param startTime 'HH:MM' (ignored when allDay) + * @param endDate 'YYYY-MM-DD' + * @param endTime 'HH:MM' (ignored when allDay) + * @returns { start, end } — for timed events, ISO-8601 UTC instants ('...Z'); + * for all-day events, the raw 'YYYY-MM-DD' date strings. + */ +export function serializeEventDateTime( + allDay: boolean, + startDate: string, + startTime: string, + endDate: string, + endTime: string, +): { start: string; end: string } { + if (allDay) { + // DATE contract (D-13): no time component, timezone-independent. + return { start: startDate, end: endDate } + } + + // Timed: build the instant from local wall-clock parts (browser is in the + // operator's zone) and serialize to a UTC instant so the wire value is + // unambiguous and container-timezone-independent. + return { + start: localWallClockToUtcIso(startDate, startTime), + end: localWallClockToUtcIso(endDate, endTime), + } +} + +/** + * Convert a local wall-clock date+time to a UTC ISO-8601 instant. + * `new Date('YYYY-MM-DDTHH:MM:00')` (no offset) is parsed in the browser's + * local timezone per ECMAScript, giving the correct instant for the operator. + */ +export function localWallClockToUtcIso(date: string, time: string): string { + return new Date(`${date}T${time}:00`).toISOString() +}