From 9978e200e662c1f6b0deb1a69733a481767d5c85 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 10 Oct 2026 12:35:58 -0400 Subject: [PATCH] Read Recent updates afresh per opening and guard their URLs by origin Address the review of PR #176 (Feature 21, task 96e2dc3f): - MQ-176-01: useUpdates drops its list as soon as the sheet closes (gcTime 0), so each opening reads GET /api/v1/updates afresh and only marks seen through the newest of that read, never a cached list. - MQ-176-02: parseUpdate checks each url with appPath from push/notification.ts, by parsed origin, instead of a prefix check that tabs and newlines got past; the url kept is the path as parsed. - MQ-176-03: TestMarkUpdatesSeen asserts updated_at is kept and settings subscribers are not told. Co-Authored-By: Claude Opus 5.5 --- internal/home/home_test.go | 23 +++++++++++-- web/src/home/UpdatesSheet.test.tsx | 54 ++++++++++++++++++++++++++++-- web/src/home/api.test.ts | 39 +++++++++++++++++++++ web/src/home/api.ts | 32 ++++++++++-------- 4 files changed, 128 insertions(+), 20 deletions(-) create mode 100644 web/src/home/api.test.ts diff --git a/internal/home/home_test.go b/internal/home/home_test.go index e0235ca..fb53d7e 100644 --- a/internal/home/home_test.go +++ b/internal/home/home_test.go @@ -45,6 +45,7 @@ var weekDates = []string{"2026-10-12", "2026-10-13", "2026-10-14", "2026-10-15", type env struct { svc *home.Service st *store.Store + settings *household.SettingsService notifier *notify.Service loc *time.Location } @@ -116,7 +117,8 @@ func newEnv(t *testing.T) *env { if err != nil { t.Fatal(err) } - svc, err := home.NewService(household.NewSettingsService(st), plans, suggestSvc, shop, notifier) + settings := household.NewSettingsService(st) + svc, err := home.NewService(settings, plans, suggestSvc, shop, notifier) if err != nil { t.Fatal(err) } @@ -124,7 +126,7 @@ func newEnv(t *testing.T) *env { if err != nil { t.Fatal(err) } - return &env{svc: svc, st: st, notifier: notifier, loc: loc} + return &env{svc: svc, st: st, settings: settings, notifier: notifier, loc: loc} } // at reads the Home screen with the clock at the household-local wall @@ -480,6 +482,15 @@ func TestMarkUpdatesSeen(t *testing.T) { if !unread() { t.Fatal("not unread with an event never seen") } + // The settings were last edited well before: marking updates seen must + // keep that time and tell no settings subscriber. + edited := (store.Timestamp{Time: time.Date(2026, 10, 1, 9, 0, 0, 0, time.UTC)}).String() + if _, err := e.st.Writer.ExecContext(ctx, `UPDATE household_settings SET updated_at = ?`, edited); err != nil { + t.Fatal(err) + } + var notified int + unsubscribe := e.settings.Subscribe(func(household.Settings) { notified++ }) + defer unsubscribe() if err := e.svc.MarkUpdatesSeen(ctx, first); err != nil { t.Fatal(err) } @@ -509,7 +520,7 @@ func TestMarkUpdatesSeen(t *testing.T) { if err := e.svc.MarkUpdatesSeen(ctx, first); err != nil { t.Fatal(err) } - settings, err := household.NewSettingsService(e.st).Get(ctx) + settings, err := e.settings.Get(ctx) if err != nil { t.Fatal(err) } @@ -519,6 +530,12 @@ func TestMarkUpdatesSeen(t *testing.T) { if settings.UpdatedBy != nil { t.Errorf("updated_by = %q, want unset: marking updates seen is not a settings edit", *settings.UpdatedBy) } + if settings.UpdatedAt != edited { + t.Errorf("updated_at = %s, want %s: marking updates seen is not a settings edit", settings.UpdatedAt, edited) + } + if notified != 0 { + t.Errorf("settings subscribers told %d times, want none: marking updates seen is not a settings edit", notified) + } // The clock reads 17:00 UTC. if err := e.svc.MarkUpdatesSeen(ctx, first.Add(2*time.Hour)); !errors.Is(err, home.ErrSeenInFuture) { diff --git a/web/src/home/UpdatesSheet.test.tsx b/web/src/home/UpdatesSheet.test.tsx index b5de4f2..e825ca9 100644 --- a/web/src/home/UpdatesSheet.test.tsx +++ b/web/src/home/UpdatesSheet.test.tsx @@ -6,7 +6,7 @@ import { ApiProvider, createApiClient, createQueryClient, createVerifiedSession import { createNetworkStatus } from '../pwa/networkStatus.ts' import { HOME } from '../test/ApiProviders.tsx' import { errorResponse, fakeLocation, jsonResponse, ME } from '../test/fetchStub.ts' -import { HOME_PATH, UPDATES_PATH, UPDATES_SEEN_PATH } from './api.ts' +import { HOME_KEY, HOME_PATH, UPDATES_PATH, UPDATES_SEEN_PATH } from './api.ts' import { HomePage } from './HomePage.tsx' afterEach(() => { @@ -90,7 +90,8 @@ function Where() { return

{location.pathname + location.search}

} -// renderHome loads /me (for the CSRF token the POST needs) and renders Home. +// renderHome loads /me (for the CSRF token the POST needs) and renders Home, +// answering its query client. async function renderHome() { const api = createApiClient(fakeLocation('/'), createNetworkStatus(window), createVerifiedSession(localStorage)) await api.loadMe() @@ -101,13 +102,15 @@ async function renderHome() { ], { initialEntries: ['/'] }, ) + const queryClient = createQueryClient() render( - + , ) + return queryClient } // openSheet taps the unread bell and answers the open sheet. @@ -153,6 +156,51 @@ describe('Recent updates sheet', () => { expect(screen.queryByTestId('unread-dot')).not.toBeInTheDocument() }) + it('reads the updates afresh on each opening, so one stored since the last opening is listed and marked seen', async () => { + // The server holds the stored events, newest first, and says Home is + // unread while the newest is after the time marked seen. + const stored = [...UPDATES.updates] + let seenThrough = '' + const fetchMock = stubServer({ + [`GET ${HOME_PATH}`]: () => jsonResponse(200, { ...HOME, recentUpdates: { unread: stored[0]!.created_at > seenThrough } }), + [`GET ${UPDATES_PATH}`]: () => jsonResponse(200, { updates: stored }), + [`POST ${UPDATES_SEEN_PATH}`]: (init) => { + seenThrough = (JSON.parse(String(init?.body)) as { through: string }).through + return new Response(null, { status: 204 }) + }, + }) + const queryClient = await renderHome() + await rows(await openSheet()) + await waitFor(() => expect(posts(fetchMock)).toHaveLength(1)) + await waitFor(() => expect(homeReads(fetchMock)).toBe(2)) + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }) + + // A new event is stored a moment later, well within the query client's + // staleTime, and Home is read again, as on coming back to it. + const reminder = { + type: 'day_of_reminder', + title: 'Tacos tonight', + body: 'Dinner at 6:00.', + url: '/plan', + created_at: new Date().toISOString(), + } + stored.unshift(reminder) + await queryClient.invalidateQueries({ queryKey: HOME_KEY }) + expect(await screen.findByTestId('unread-dot')).toBeInTheDocument() + + const listed = await rows(await openSheet()) + expect(listed).toHaveLength(4) + expect(listed[0]).toHaveTextContent('Tacos tonight') + await waitFor(() => + expect(posts(fetchMock)).toEqual([ + JSON.stringify({ through: UPDATES.updates[0]!.created_at }), + JSON.stringify({ through: reminder.created_at }), + ]), + ) + await waitFor(() => expect(homeReads(fetchMock)).toBe(4)) + expect(screen.queryByTestId('unread-dot')).not.toBeInTheDocument() + }) + it.each([ [0, '/plan'], [1, '/discover'], diff --git a/web/src/home/api.test.ts b/web/src/home/api.test.ts new file mode 100644 index 0000000..d4321c7 --- /dev/null +++ b/web/src/home/api.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest' +import { parseUpdates } from './api.ts' + +const origin = 'https://mealquest.example' +const where = 'GET /api/v1/updates' + +// body is a GET /api/v1/updates body holding one update that opens url. +function body(url: string) { + return { + updates: [{ type: 'plan_ready', title: 'Your week is drafted', body: '', url, created_at: '2026-10-10T16:00:00.000Z' }], + } +} + +describe('parseUpdates', () => { + it('keeps a same-origin path with its query and fragment, as parsed', () => { + expect(parseUpdates(where, body('/plan/shopping?week=2026-10-05#tuesday'), origin)[0]!.url).toBe( + '/plan/shopping?week=2026-10-05#tuesday', + ) + expect(parseUpdates(where, body('/plan/../discover'), origin)[0]!.url).toBe('/discover') + }) + + // Each of these would open another site, or run script, once parsed: URL + // parsing strips tabs and newlines and reads a backslash as a slash. + it.each([ + ['a tab', '/\t/evil.example/x'], + ['a newline', '/\n/evil.example/x'], + ['a carriage return', '/\r/evil.example/x'], + ['a backslash', '/\\evil.example/x'], + ['dot segments', '/..//evil.example/x'], + ['a protocol-relative URL', '//evil.example/x'], + ['an absolute URL', 'https://evil.example/x'], + ['javascript:', 'javascript:alert(1)'], + ['a relative path', 'plan'], + ])('refuses an update url with %s', (_, url) => { + expect(() => parseUpdates(where, body(url), origin)).toThrow( + `${where}: update url ${JSON.stringify(url)} is not a path on this app`, + ) + }) +}) diff --git a/web/src/home/api.ts b/web/src/home/api.ts index c9936ac..19f60a1 100644 --- a/web/src/home/api.ts +++ b/web/src/home/api.ts @@ -3,6 +3,7 @@ import { useApi } from '../api/index.ts' import { isRecord } from '../api/errors.ts' import { oneOf, photoId } from '../dinners/api.ts' import { weekLabel } from '../plan/week.ts' +import { appPath } from '../push/notification.ts' export const HOME_PATH = '/api/v1/home' export const HOME_KEY = ['home'] as const @@ -108,13 +109,10 @@ export type UpdateType = (typeof UPDATE_TYPES)[number] // takes back. export type Update = { type: UpdateType; title: string; body: string; url: string; created_at: string } -// isAppPath says whether url is a path on the app's own origin, as the -// server only ever stores. -function isAppPath(url: string): boolean { - return url.startsWith('/') && !url.startsWith('//') && !url.includes('\\') -} - -function parseUpdate(where: string, body: unknown): Update { +// parseUpdate checks one update. Its url must stay on origin once parsed, as +// the server only ever stores a path on the app's own origin; url is that +// path as parsed. +function parseUpdate(where: string, body: unknown, origin: string): Update { if ( !isRecord(body) || typeof body.title !== 'string' || @@ -124,7 +122,8 @@ function parseUpdate(where: string, body: unknown): Update { ) { throw new Error(`${where}: an update is not a notification event`) } - if (!isAppPath(body.url)) throw new Error(`${where}: update url "${body.url}" is not a path on this app`) + const url = appPath(body.url, origin) + if (url === null) throw new Error(`${where}: update url ${JSON.stringify(body.url)} is not a path on this app`) if (Number.isNaN(Date.parse(body.created_at))) { throw new Error(`${where}: update created_at "${body.created_at}" is not a time`) } @@ -132,24 +131,29 @@ function parseUpdate(where: string, body: unknown): Update { type: oneOf(where, 'type', UPDATE_TYPES, body.type), title: body.title, body: body.body, - url: body.url, + url, created_at: body.created_at, } } -// parseUpdates checks a GET /api/v1/updates body: the updates, newest first. -export function parseUpdates(where: string, body: unknown): Update[] { +// parseUpdates checks a GET /api/v1/updates body, from the app at origin: +// the updates, newest first. +export function parseUpdates(where: string, body: unknown, origin: string): Update[] { if (!isRecord(body) || !Array.isArray(body.updates)) throw new Error(`${where}: response is not the updates`) - return body.updates.map((update) => parseUpdate(where, update)) + return body.updates.map((update) => parseUpdate(where, update, origin)) } // useUpdates is the Recent updates (GET /api/v1/updates): the last ten -// notification events, newest first. +// notification events, newest first. The list is dropped as soon as the +// sheet showing it closes (gcTime 0), so each opening reads it afresh and +// never shows, or marks seen, a list read before an event stored since. export function useUpdates() { const { apiFetch } = useApi() return useQuery({ queryKey: UPDATES_KEY, - queryFn: async () => parseUpdates(`GET ${UPDATES_PATH}`, await apiFetch(UPDATES_PATH)), + queryFn: async () => + parseUpdates(`GET ${UPDATES_PATH}`, await apiFetch(UPDATES_PATH), window.location.origin), + gcTime: 0, }) }