From bd8f22a3ffb52edd117b241bbaa39092d253861a Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Tue, 19 May 2026 16:12:03 -0700 Subject: [PATCH] notifications(P4f): audit fixes, lock down 047 + best-effort enqueue paths Two findings from an ultrathink audit of the Phase 4 work. (1) members_with_permission was callable by any PostgREST client. Postgres grants EXECUTE on new functions to PUBLIC by default, and this codebase has no existing REVOKE pattern. The function returns a SET of member_ids for any (space, permission) pair, so without a lockdown ANY authenticated caller could enumerate "who has permission X in space Y" for spaces they are not even members of, learning member_ids and the permission distribution. user_has_permission has a narrower exposure (per-uid boolean) and is needed by anon/authenticated for RLS-policy evaluation, so it stays callable; the new function is locked to service_role since the only legitimate caller (the form-submission fan-out) goes through the admin client. Migration 047 + schema.sql now REVOKE EXECUTE FROM PUBLIC and GRANT TO service_role. (2) The Phase 4 enqueue blocks in equipment.ts (reserve, cancel) and classes.ts (signUp, cancelMySignup waitlist promotion, updateSession session-cancellation fan-out) used the shared helpers, but the helpers themselves were not internally best-effort. The Stripe webhook's original enqueueDues wrapped its entire body in try/catch so a thrown member-lookup error could never bubble into the money path; my refactor removed that inner wrap and relied on enqueueNotification's best-effort. resolveMemberContact and getSpaceName could still throw on a transient supabase network error, which would surface to the action return value after the underlying mutation already committed: the client would see an error even though their booking, signup, or session-cancellation succeeded. Fixed at both layers: the helpers now swallow throws internally (returning null / empty), and each call site in equipment/classes is wrapped in try/catch around the lookup chain so secondary action-specific queries (equipment name+location lookup, promoted-signup session lookup, session-cancellation signup fan-out) also cannot leak. forms.submitForm already had the wrap from P4d. 2 new helper tests pin the best-effort behavior; suite 567, build clean. --- __tests__/notifications-enqueue.test.ts | 23 ++- lib/actions/classes.ts | 240 +++++++++++++----------- lib/actions/equipment.ts | 117 +++++++----- lib/notifications/enqueue.ts | 42 +++-- scripts/047_members_with_permission.sql | 11 ++ scripts/schema.sql | 8 + 6 files changed, 260 insertions(+), 181 deletions(-) diff --git a/__tests__/notifications-enqueue.test.ts b/__tests__/notifications-enqueue.test.ts index 14a8fb6..85d691e 100644 --- a/__tests__/notifications-enqueue.test.ts +++ b/__tests__/notifications-enqueue.test.ts @@ -15,6 +15,7 @@ function mockAdmin(opts: { selectResult?: { data: Row | null; error?: { message: string } | null } upsertResult?: { error: { message: string } | null } throwOnUpsert?: boolean + throwOnMaybeSingle?: boolean }) { const upsert = vi.fn(async () => { if (opts.throwOnUpsert) throw new Error('boom') @@ -24,9 +25,10 @@ function mockAdmin(opts: { const builder = { select: vi.fn().mockReturnThis(), eq: vi.fn().mockReturnThis(), - maybeSingle: vi.fn(async () => - opts.selectResult ?? { data: null, error: null }, - ), + maybeSingle: vi.fn(async () => { + if (opts.throwOnMaybeSingle) throw new Error('network down') + return opts.selectResult ?? { data: null, error: null } + }), upsert, } const from = vi.fn(() => builder) @@ -62,6 +64,14 @@ describe('resolveMemberContact', () => { const r = await resolveMemberContact(admin as never, 'space-1', 'member-1') expect(r).toEqual({ email: null, displayName: null }) }) + + it('returns null and never throws when the lookup itself throws (best-effort)', async () => { + const admin = mockAdmin({ throwOnMaybeSingle: true }) + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}) + await expect(resolveMemberContact(admin as never, 'space-1', 'member-1')).resolves.toBeNull() + expect(spy).toHaveBeenCalled() + spy.mockRestore() + }) }) describe('enqueueNotification', () => { @@ -155,4 +165,11 @@ describe('getSpaceName', () => { const admin = mockAdmin({ selectResult: { data: null, error: null } }) expect(await getSpaceName(admin as never, 'ghost')).toBe('') }) + it('returns empty string when the lookup itself throws (best-effort)', async () => { + const admin = mockAdmin({ throwOnMaybeSingle: true }) + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}) + await expect(getSpaceName(admin as never, 'space-1')).resolves.toBe('') + expect(spy).toHaveBeenCalled() + spy.mockRestore() + }) }) diff --git a/lib/actions/classes.ts b/lib/actions/classes.ts index 7e13363..c612e2e 100644 --- a/lib/actions/classes.ts +++ b/lib/actions/classes.ts @@ -318,59 +318,65 @@ export async function updateSession(input: unknown) { // to 'cancelled', every signed-up member needs to know (the only other // place that lives is /me, and they probably aren't checking it). One // outbox row per (session, member); dedupe collapses a re-cancel. + // Wrapped: the session update is committed; transient DB errors in the + // fan-out must never surface to the action's result. if (u.status === 'cancelled') { - const admin = createAdminClient() - const { data: sessRow } = await admin - .from('class_sessions') - .select('starts_at, ends_at, location, classes(title)') - .eq('id', u.sessionId) - .eq('space_id', member.space_id) - .maybeSingle() - const sessShape = sessRow as { - starts_at: string | null - ends_at: string | null - location: string | null - classes?: { title: string | null } | null - } | null - const { data: signups } = await admin - .from('class_signups') - .select('member_id') - .eq('space_id', member.space_id) - .eq('session_id', u.sessionId) - .neq('status', 'cancelled') - const memberIds = Array.from( - new Set(((signups ?? []) as Array<{ member_id: string }>).map(s => s.member_id)), - ) - if (memberIds.length > 0) { - const spaceName = await getSpaceName(admin, member.space_id) - const manageUrl = buildManageUrl(null) - for (const affectedMemberId of memberIds) { - const contact = await resolveMemberContact(admin, member.space_id, affectedMemberId) - if (!contact?.email) continue - const { subject, html, text } = renderClassEmail({ - type: 'class_session_cancelled', - spaceName, - memberName: contact.displayName, - className: sessShape?.classes?.title ?? '', - location: sessShape?.location ?? null, - startsAt: sessShape?.starts_at ?? null, - endsAt: sessShape?.ends_at ?? null, - manageUrl, - }) - await enqueueNotification(admin, { - spaceId: member.space_id, - memberId: affectedMemberId, - type: 'class_session_cancelled', - recipient: contact.email, - subject, - bodyHtml: html, - bodyText: text, - dedupeKey: classDedupeKey('class_session_cancelled', { - sessionId: u.sessionId, + try { + const admin = createAdminClient() + const { data: sessRow } = await admin + .from('class_sessions') + .select('starts_at, ends_at, location, classes(title)') + .eq('id', u.sessionId) + .eq('space_id', member.space_id) + .maybeSingle() + const sessShape = sessRow as { + starts_at: string | null + ends_at: string | null + location: string | null + classes?: { title: string | null } | null + } | null + const { data: signups } = await admin + .from('class_signups') + .select('member_id') + .eq('space_id', member.space_id) + .eq('session_id', u.sessionId) + .neq('status', 'cancelled') + const memberIds = Array.from( + new Set(((signups ?? []) as Array<{ member_id: string }>).map(s => s.member_id)), + ) + if (memberIds.length > 0) { + const spaceName = await getSpaceName(admin, member.space_id) + const manageUrl = buildManageUrl(null) + for (const affectedMemberId of memberIds) { + const contact = await resolveMemberContact(admin, member.space_id, affectedMemberId) + if (!contact?.email) continue + const { subject, html, text } = renderClassEmail({ + type: 'class_session_cancelled', + spaceName, + memberName: contact.displayName, + className: sessShape?.classes?.title ?? '', + location: sessShape?.location ?? null, + startsAt: sessShape?.starts_at ?? null, + endsAt: sessShape?.ends_at ?? null, + manageUrl, + }) + await enqueueNotification(admin, { + spaceId: member.space_id, memberId: affectedMemberId, - }), - }) + type: 'class_session_cancelled', + recipient: contact.email, + subject, + bodyHtml: html, + bodyText: text, + dedupeKey: classDedupeKey('class_session_cancelled', { + sessionId: u.sessionId, + memberId: affectedMemberId, + }), + }) + } } + } catch (e) { + console.error('[updateSession] class_session_cancelled fan-out failed:', e instanceof Error ? e.message : e) } } @@ -603,7 +609,8 @@ export async function signUpForClass(input: unknown) { // Signup confirmation to the affected member (target, not actor): a manager // signing someone else up still emails the booked-for member. Registered // and waitlisted use different copy; the type derives from the RPC's - // signup_status. Best-effort: never throws into this action. + // signup_status. Wrapped: the signup is already written by the RPC, so the + // email path must never surface an error to the action's result. const signupType = status === 'registered' ? ('class_signup_registered' as const) @@ -611,28 +618,32 @@ export async function signUpForClass(input: unknown) { ? ('class_signup_waitlisted' as const) : null if (signupType) { - const contact = await resolveMemberContact(admin, member.space_id, targetMemberId) - if (contact?.email) { - const { subject, html, text } = renderClassEmail({ - type: signupType, - spaceName: await getSpaceName(admin, member.space_id), - memberName: contact.displayName, - className: cls?.title ?? '', - location: (session.location as string | null) ?? null, - startsAt: session.starts_at as string, - endsAt: (session.ends_at as string | null) ?? null, - manageUrl: buildManageUrl(null), - }) - await enqueueNotification(admin, { - spaceId: member.space_id, - memberId: targetMemberId, - type: signupType, - recipient: contact.email, - subject, - bodyHtml: html, - bodyText: text, - dedupeKey: classDedupeKey(signupType, { signupId: row.signup_id }), - }) + try { + const contact = await resolveMemberContact(admin, member.space_id, targetMemberId) + if (contact?.email) { + const { subject, html, text } = renderClassEmail({ + type: signupType, + spaceName: await getSpaceName(admin, member.space_id), + memberName: contact.displayName, + className: cls?.title ?? '', + location: (session.location as string | null) ?? null, + startsAt: session.starts_at as string, + endsAt: (session.ends_at as string | null) ?? null, + manageUrl: buildManageUrl(null), + }) + await enqueueNotification(admin, { + spaceId: member.space_id, + memberId: targetMemberId, + type: signupType, + recipient: contact.email, + subject, + bodyHtml: html, + bodyText: text, + dedupeKey: classDedupeKey(signupType, { signupId: row.signup_id }), + }) + } + } catch (e) { + console.error(`[signUpForClass] ${signupType} enqueue failed:`, e instanceof Error ? e.message : e) } } @@ -673,47 +684,52 @@ export async function cancelMySignup(input: unknown) { // Waitlist promotion: someone else was bumped from waitlist into the // session by this cancel. Tell them, since the only place that information - // lives otherwise is /me. Best-effort: never throws into this action. + // lives otherwise is /me. Wrapped: cancel + promotion are already committed + // by class_cancel_tx; the email path must never surface an error. if (row.promoted_id) { - const { data: promo } = await admin - .from('class_signups') - .select( - 'id, member_id, class_sessions(starts_at, ends_at, location, classes(title))', - ) - .eq('id', row.promoted_id) - .eq('space_id', member.space_id) - .maybeSingle() - const promotedMemberId = (promo?.member_id as string | null) ?? null - const promoSession = (promo as { class_sessions?: { - starts_at: string | null - ends_at: string | null - location: string | null - classes?: { title: string | null } | null - } | null } | null)?.class_sessions - if (promotedMemberId) { - const contact = await resolveMemberContact(admin, member.space_id, promotedMemberId) - if (contact?.email) { - const { subject, html, text } = renderClassEmail({ - type: 'class_signup_promoted', - spaceName: await getSpaceName(admin, member.space_id), - memberName: contact.displayName, - className: promoSession?.classes?.title ?? '', - location: promoSession?.location ?? null, - startsAt: promoSession?.starts_at ?? null, - endsAt: promoSession?.ends_at ?? null, - manageUrl: buildManageUrl(null), - }) - await enqueueNotification(admin, { - spaceId: member.space_id, - memberId: promotedMemberId, - type: 'class_signup_promoted', - recipient: contact.email, - subject, - bodyHtml: html, - bodyText: text, - dedupeKey: classDedupeKey('class_signup_promoted', { signupId: row.promoted_id }), - }) + try { + const { data: promo } = await admin + .from('class_signups') + .select( + 'id, member_id, class_sessions(starts_at, ends_at, location, classes(title))', + ) + .eq('id', row.promoted_id) + .eq('space_id', member.space_id) + .maybeSingle() + const promotedMemberId = (promo?.member_id as string | null) ?? null + const promoSession = (promo as { class_sessions?: { + starts_at: string | null + ends_at: string | null + location: string | null + classes?: { title: string | null } | null + } | null } | null)?.class_sessions + if (promotedMemberId) { + const contact = await resolveMemberContact(admin, member.space_id, promotedMemberId) + if (contact?.email) { + const { subject, html, text } = renderClassEmail({ + type: 'class_signup_promoted', + spaceName: await getSpaceName(admin, member.space_id), + memberName: contact.displayName, + className: promoSession?.classes?.title ?? '', + location: promoSession?.location ?? null, + startsAt: promoSession?.starts_at ?? null, + endsAt: promoSession?.ends_at ?? null, + manageUrl: buildManageUrl(null), + }) + await enqueueNotification(admin, { + spaceId: member.space_id, + memberId: promotedMemberId, + type: 'class_signup_promoted', + recipient: contact.email, + subject, + bodyHtml: html, + bodyText: text, + dedupeKey: classDedupeKey('class_signup_promoted', { signupId: row.promoted_id }), + }) + } } + } catch (e) { + console.error('[cancelMySignup] class_signup_promoted enqueue failed:', e instanceof Error ? e.message : e) } } diff --git a/lib/actions/equipment.ts b/lib/actions/equipment.ts index 228e1f0..9e1a0ed 100644 --- a/lib/actions/equipment.ts +++ b/lib/actions/equipment.ts @@ -311,29 +311,36 @@ export async function reserveEquipment(input: unknown) { // Booking confirmation goes to the affected member (target), not the actor. // A manager booking on behalf of someone else still emails the booked-for - // member. Best-effort: the enqueue helper never throws into this action. - const contact = await resolveMemberContact(admin, member.space_id, targetMemberId) - if (contact?.email) { - const { subject, html, text } = renderBookingEmail({ - type: 'booking_confirmed', - spaceName: await getSpaceName(admin, member.space_id), - memberName: contact.displayName, - equipmentName: (equip.name as string | null) ?? 'equipment', - location: (equip.location as string | null) ?? null, - startsAt: r.starts_at, - endsAt: r.ends_at, - manageUrl: buildManageUrl(null), - }) - await enqueueNotification(admin, { - spaceId: member.space_id, - memberId: targetMemberId, - type: 'booking_confirmed', - recipient: contact.email, - subject, - bodyHtml: html, - bodyText: text, - dedupeKey: bookingDedupeKey('booking_confirmed', data.id as string), - }) + // member. Wrapped: the reservation row is already committed, so a transient + // DB error in the email path must NOT surface as an action error to the + // client. enqueueNotification is internally best-effort; the wrap covers + // resolveMemberContact, getSpaceName, and any other lookup that could throw. + try { + const contact = await resolveMemberContact(admin, member.space_id, targetMemberId) + if (contact?.email) { + const { subject, html, text } = renderBookingEmail({ + type: 'booking_confirmed', + spaceName: await getSpaceName(admin, member.space_id), + memberName: contact.displayName, + equipmentName: (equip.name as string | null) ?? 'equipment', + location: (equip.location as string | null) ?? null, + startsAt: r.starts_at, + endsAt: r.ends_at, + manageUrl: buildManageUrl(null), + }) + await enqueueNotification(admin, { + spaceId: member.space_id, + memberId: targetMemberId, + type: 'booking_confirmed', + recipient: contact.email, + subject, + bodyHtml: html, + bodyText: text, + dedupeKey: bookingDedupeKey('booking_confirmed', data.id as string), + }) + } + } catch (e) { + console.error('[reserveEquipment] booking_confirmed enqueue failed:', e instanceof Error ? e.message : e) } revalidatePath('/equipment') @@ -382,36 +389,42 @@ export async function cancelReservation(input: unknown) { // Cancel emails fire ONLY when someone other than the affected member // cancelled (a manager cancelling on the member's behalf). A self-cancel is // silent: the actor already saw the UI confirm and an email would be noise. + // Wrapped: cancel is committed; transient errors in the email path must not + // surface to the client. if (!ownIt) { - const affectedMemberId = res.member_id as string - const contact = await resolveMemberContact(admin, member.space_id, affectedMemberId) - if (contact?.email) { - const { data: equip } = await admin - .from('equipment') - .select('name, location') - .eq('id', res.equipment_id as string) - .eq('space_id', member.space_id) - .maybeSingle() - const { subject, html, text } = renderBookingEmail({ - type: 'booking_cancelled', - spaceName: await getSpaceName(admin, member.space_id), - memberName: contact.displayName, - equipmentName: (equip?.name as string | null) ?? 'equipment', - location: (equip?.location as string | null) ?? null, - startsAt: res.starts_at as string, - endsAt: res.ends_at as string, - manageUrl: buildManageUrl(null), - }) - await enqueueNotification(admin, { - spaceId: member.space_id, - memberId: affectedMemberId, - type: 'booking_cancelled', - recipient: contact.email, - subject, - bodyHtml: html, - bodyText: text, - dedupeKey: bookingDedupeKey('booking_cancelled', v.data.reservationId), - }) + try { + const affectedMemberId = res.member_id as string + const contact = await resolveMemberContact(admin, member.space_id, affectedMemberId) + if (contact?.email) { + const { data: equip } = await admin + .from('equipment') + .select('name, location') + .eq('id', res.equipment_id as string) + .eq('space_id', member.space_id) + .maybeSingle() + const { subject, html, text } = renderBookingEmail({ + type: 'booking_cancelled', + spaceName: await getSpaceName(admin, member.space_id), + memberName: contact.displayName, + equipmentName: (equip?.name as string | null) ?? 'equipment', + location: (equip?.location as string | null) ?? null, + startsAt: res.starts_at as string, + endsAt: res.ends_at as string, + manageUrl: buildManageUrl(null), + }) + await enqueueNotification(admin, { + spaceId: member.space_id, + memberId: affectedMemberId, + type: 'booking_cancelled', + recipient: contact.email, + subject, + bodyHtml: html, + bodyText: text, + dedupeKey: bookingDedupeKey('booking_cancelled', v.data.reservationId), + }) + } + } catch (e) { + console.error('[cancelReservation] booking_cancelled enqueue failed:', e instanceof Error ? e.message : e) } } diff --git a/lib/notifications/enqueue.ts b/lib/notifications/enqueue.ts index 0f21dd8..b3bd4b2 100644 --- a/lib/notifications/enqueue.ts +++ b/lib/notifications/enqueue.ts @@ -22,22 +22,29 @@ export type MemberContact = { // Look up a member's email + display_name, scoped to the given space. Used // by every enqueue site to derive a recipient from a member id. Returns null // if the member is not in that space (cross-tenant safety: the (id, space_id) -// filter pins the lookup). +// filter pins the lookup) OR if the lookup itself fails (best-effort: never +// throws into the calling action, so a transient DB error cannot turn a +// successful domain mutation into a client-visible error). export async function resolveMemberContact( admin: AdminClient, spaceId: string, memberId: string, ): Promise { - const { data } = await admin - .from('space_members') - .select('email, display_name') - .eq('id', memberId) - .eq('space_id', spaceId) - .maybeSingle() - if (!data) return null - return { - email: (data.email as string | null) ?? null, - displayName: (data.display_name as string | null) ?? null, + try { + const { data } = await admin + .from('space_members') + .select('email, display_name') + .eq('id', memberId) + .eq('space_id', spaceId) + .maybeSingle() + if (!data) return null + return { + email: (data.email as string | null) ?? null, + displayName: (data.display_name as string | null) ?? null, + } + } catch (e) { + console.error('[notifications] resolveMemberContact threw:', e instanceof Error ? e.message : e) + return null } } @@ -91,10 +98,17 @@ export async function enqueueNotification( // Read the space's name once per caller. Used for brand-neutral copy that // injects the space name (this is a generic multi-space platform; never -// hard-code a tenant). Returns empty string on miss so renders still work. +// hard-code a tenant). Returns empty string on miss or on lookup failure +// (best-effort, same rationale as resolveMemberContact); the renderers all +// fall back to "your hackerspace" so the email still composes. export async function getSpaceName(admin: AdminClient, spaceId: string): Promise { - const { data } = await admin.from('spaces').select('name').eq('id', spaceId).maybeSingle() - return (data?.name as string | null) ?? '' + try { + const { data } = await admin.from('spaces').select('name').eq('id', spaceId).maybeSingle() + return (data?.name as string | null) ?? '' + } catch (e) { + console.error('[notifications] getSpaceName threw:', e instanceof Error ? e.message : e) + return '' + } } // Build the member portal URL the way every enqueue site does. Header host diff --git a/scripts/047_members_with_permission.sql b/scripts/047_members_with_permission.sql index 80edf52..9f65952 100644 --- a/scripts/047_members_with_permission.sql +++ b/scripts/047_members_with_permission.sql @@ -35,3 +35,14 @@ AS $$ ) ); $$; + +-- LOCKDOWN: unlike user_has_permission (a per-user boolean that is hard to +-- enumerate without already knowing target uids), members_with_permission +-- returns a SET of member_ids for any (space, permission) pair. By default +-- Postgres grants EXECUTE on a new function to PUBLIC, so without an explicit +-- REVOKE this function would let any authenticated PostgREST caller enumerate +-- "who has permission X in space Y" across spaces they are not even members +-- of. Lock it to service_role: the notifications fan-out goes through the +-- admin (service-role) client; no client should call this directly. +REVOKE EXECUTE ON FUNCTION public.members_with_permission(uuid, text) FROM PUBLIC; +GRANT EXECUTE ON FUNCTION public.members_with_permission(uuid, text) TO service_role; diff --git a/scripts/schema.sql b/scripts/schema.sql index 5701fff..4981370 100644 --- a/scripts/schema.sql +++ b/scripts/schema.sql @@ -1775,6 +1775,12 @@ $$; -- user_effective_roles fallback. Used by notification fan-outs (e.g. the -- forms.manage admin alert) where N user_has_permission calls would be N -- round-trips. Equivalent to scripts/047_members_with_permission.sql. +-- LOCKDOWN: unlike user_has_permission (a per-user boolean that is hard to +-- enumerate without already knowing target uids), this function returns a SET +-- of member_ids for any (space, permission) pair. Postgres grants EXECUTE on +-- new functions to PUBLIC by default, so an explicit REVOKE is required to +-- prevent any authenticated PostgREST caller from enumerating permission +-- holders across spaces they are not members of. Locked to service_role. CREATE OR REPLACE FUNCTION public.members_with_permission(sid uuid, perm text) RETURNS TABLE(member_id uuid) LANGUAGE sql STABLE SECURITY DEFINER SET search_path = public AS $$ @@ -1791,6 +1797,8 @@ AS $$ ) ); $$; +REVOKE EXECUTE ON FUNCTION public.members_with_permission(uuid, text) FROM PUBLIC; +GRANT EXECUTE ON FUNCTION public.members_with_permission(uuid, text) TO service_role; ALTER TABLE public.space_role_permissions ENABLE ROW LEVEL SECURITY; ALTER TABLE public.ops_acl ENABLE ROW LEVEL SECURITY;