From 09f293e3c15a8a19d0211315ac2730a9b1ef6124 Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Tue, 19 May 2026 17:24:01 -0700 Subject: [PATCH] notifications(P4g): strip URLs from user-controlled render inputs Third-pass audit finding. renderShell auto-linkifies any https?:// in body lines so the manageUrl line becomes a clickable link in the HTML email. The same regex would happily turn a malicious URL injected into a user-controlled field (member display_name, equipment / class / form title, submitter label, space name) into a clickable link. In Phase 2 dues emails this was self-only: the recipient is the same member whose display_name appears, so self-phishing is silly. Phase 4 introduces the first email type where one user's display_name lands in a different user's inbox: form_submission_admin embeds the submitter's display_name (or, when the form was submitted anonymously and an email match attributed it to a member, that member's display_name) in an email read by every forms.manage holder. An authenticated member can set display_name to "Click https://evil.com for free stuff", submit a form, and the admin's inbox now has a clickable phishing URL. body.email is Zod-validated against email format so the typed-email path is closed, but display_name (z.string().min(1).max(100)) is a free string. No format restriction will catch a determined attacker. Mitigation: stripUrls(s) replaces https?://\S+ with [link]. Applied to every user-controlled input across all four renderers (dues, booking, class, form) including spaceName, memberName / recipientName, equipment / class / form title, location, and submitterLabel. manageUrl, which buildManageUrl constructs from NEXT_PUBLIC_APP_URL or a Stripe- authenticated webhook host, stays untouched and remains the only linkified URL in the rendered email. 5 new tests cover the new attack vector (form_submission_admin) and defense-in-depth across booking, class, and dues renderers. Suite 572, build clean. --- __tests__/notifications-logic.test.ts | 84 +++++++++++++++++++++++++++ lib/notifications-logic.ts | 42 +++++++++----- 2 files changed, 112 insertions(+), 14 deletions(-) diff --git a/__tests__/notifications-logic.test.ts b/__tests__/notifications-logic.test.ts index 16d0228..b817711 100644 --- a/__tests__/notifications-logic.test.ts +++ b/__tests__/notifications-logic.test.ts @@ -383,3 +383,87 @@ describe('renderFormEmail', () => { expect(r.html).toContain('<img>') }) }) + +describe('URL stripping in user-controlled fields (phishing defense)', () => { + // renderShell auto-linkifies https?:// in body lines so the manageUrl line + // becomes clickable. That same regex would happily turn a malicious URL in + // an injected user-controlled field into a clickable link, which is a + // phishing vector wherever those fields land in an email read by someone + // OTHER than the field's owner (notably form_submission_admin, which + // embeds the submitter's display_name in an admin's inbox). The fix is to + // strip URLs from user-controlled inputs before render; manageUrl stays + // untouched and remains the only linkified URL. + + it('form_submission_admin scrubs a URL embedded in the submitter label', () => { + const r = renderFormEmail({ + type: 'form_submission_admin', + spaceName: 'Acme', + recipientName: 'Boss', + formTitle: 'Liability Waiver', + manageUrl: 'https://x.test/forms/abc/results', + submitterLabel: 'Click https://evil.com for free stuff', + }) + // The malicious URL is stripped, replaced with [link]. + expect(r.text).not.toContain('https://evil.com') + expect(r.html).not.toContain('href="https://evil.com"') + expect(r.text).toContain('[link]') + // The legitimate results URL is still linkified. + expect(r.html).toContain('href="https://x.test/forms/abc/results"') + }) + + it('form_submission_admin scrubs a URL embedded in the form title', () => { + const r = renderFormEmail({ + type: 'form_submission_admin', + spaceName: 'Acme', + formTitle: 'Visit https://evil.com', + manageUrl: 'https://x.test/forms/abc/results', + submitterLabel: 'Ada', + }) + expect(r.html).not.toContain('href="https://evil.com"') + expect(r.text).toContain('[link]') + }) + + it('renderBookingEmail scrubs URLs in member name, equipment name, and location (defense in depth)', () => { + const r = renderBookingEmail({ + type: 'booking_confirmed', + spaceName: 'Visit https://x.evil', + memberName: 'Look https://x.evil', + equipmentName: 'Buy https://x.evil', + location: 'Bay https://x.evil', + startsAt: '2026-05-22T15:00:00Z', + endsAt: '2026-05-22T17:00:00Z', + manageUrl: 'https://x.test/me', + }) + expect(r.html).not.toContain('href="https://x.evil"') + expect(r.html).toContain('href="https://x.test/me"') + }) + + it('renderClassEmail scrubs URLs in class title and location', () => { + const r = renderClassEmail({ + type: 'class_signup_registered', + spaceName: 'Acme', + memberName: 'Ada', + className: 'Welding https://evil.test', + location: 'Bay 1', + startsAt: '2026-05-22T15:00:00Z', + endsAt: '2026-05-22T17:00:00Z', + manageUrl: 'https://x.test/me', + }) + expect(r.html).not.toContain('href="https://evil.test"') + expect(r.html).toContain('href="https://x.test/me"') + }) + + it('renderDuesEmail scrubs URLs in member name and space name (defense in depth)', () => { + const r = renderDuesEmail({ + type: 'dues_renewed', + spaceName: 'Acme https://evil.test', + memberName: 'Bad https://evil.test', + amount: 25, + currency: 'USD', + periodEnd: '2026-06-15', + manageUrl: 'https://x.test/me', + }) + expect(r.html).not.toContain('href="https://evil.test"') + expect(r.html).toContain('href="https://x.test/me"') + }) +}) diff --git a/lib/notifications-logic.ts b/lib/notifications-logic.ts index 6afebe9..2b96df0 100644 --- a/lib/notifications-logic.ts +++ b/lib/notifications-logic.ts @@ -70,6 +70,20 @@ function escapeHtml(s: string): string { .replace(/'/g, ''') } +// renderShell auto-linkifies any https?:// in the body so the manageUrl line +// becomes a clickable link. That same regex turns ANY URL embedded in a +// user-controlled input (member display_name, equipment/class title, form +// title, submitter label, space name) into a clickable link too, opening a +// phishing vector wherever those fields land in an email to a different user +// (notably form_submission_admin, which surfaces a submitter's display_name +// to an admin). Scrub URL patterns from every user-controlled input before +// rendering. Only manageUrl, which is server-built from NEXT_PUBLIC_APP_URL, +// is allowed to reach the linkify regex. +function stripUrls(s: string | null | undefined): string { + if (!s) return '' + return s.replace(/https?:\/\/\S+/gi, '[link]') +} + const CURRENCY_SYMBOL: Record = { USD: '$', CAD: '$', EUR: '€', GBP: '£' } // Stripe zero-decimal currencies: the smallest unit IS the major unit, so @@ -164,8 +178,8 @@ export type RenderedEmail = { subject: string; html: string; text: string } // is injected, never hard-coded. Returns both an HTML and a plain-text body // (Resend will also auto-generate text, but we send an explicit one). export function renderDuesEmail(input: DuesEmailInput): RenderedEmail { - const space = input.spaceName || 'your hackerspace' - const name = input.memberName?.trim() || null + const space = stripUrls(input.spaceName) || 'your hackerspace' + const name = stripUrls(input.memberName).trim() || null const greeting = name ? `Hi ${name},` : 'Hi,' const money = formatMoney(input.amount, input.currency) const renews = formatDate(input.periodEnd) @@ -235,12 +249,12 @@ export type ClassEmailInput = { } export function renderClassEmail(input: ClassEmailInput): RenderedEmail { - const space = input.spaceName || 'your hackerspace' - const name = input.memberName?.trim() || null + const space = stripUrls(input.spaceName) || 'your hackerspace' + const name = stripUrls(input.memberName).trim() || null const greeting = name ? `Hi ${name},` : 'Hi,' - const cls = input.className || 'the class' + const cls = stripUrls(input.className) || 'the class' const range = formatRange(input.startsAt, input.endsAt) - const location = (input.location || '').trim() + const location = stripUrls(input.location).trim() let subject: string const lines: string[] = [greeting, ''] @@ -308,10 +322,10 @@ export type FormEmailInput = { } export function renderFormEmail(input: FormEmailInput): RenderedEmail { - const space = input.spaceName || 'your hackerspace' - const name = input.recipientName?.trim() || null + const space = stripUrls(input.spaceName) || 'your hackerspace' + const name = stripUrls(input.recipientName).trim() || null const greeting = name ? `Hi ${name},` : 'Hi,' - const form = input.formTitle || 'the form' + const form = stripUrls(input.formTitle) || 'the form' let subject: string const lines: string[] = [greeting, ''] @@ -325,7 +339,7 @@ export function renderFormEmail(input: FormEmailInput): RenderedEmail { ) } else { subject = `New submission: ${form} at ${space}` - const who = (input.submitterLabel || '').trim() || 'someone' + const who = stripUrls(input.submitterLabel).trim() || 'someone' lines.push( `${who} submitted ${form} at ${space}.`, '', @@ -363,12 +377,12 @@ export type BookingEmailInput = { } export function renderBookingEmail(input: BookingEmailInput): RenderedEmail { - const space = input.spaceName || 'your hackerspace' - const name = input.memberName?.trim() || null + const space = stripUrls(input.spaceName) || 'your hackerspace' + const name = stripUrls(input.memberName).trim() || null const greeting = name ? `Hi ${name},` : 'Hi,' - const equipment = input.equipmentName || 'the equipment' + const equipment = stripUrls(input.equipmentName) || 'the equipment' const range = formatRange(input.startsAt, input.endsAt) - const location = (input.location || '').trim() + const location = stripUrls(input.location).trim() let subject: string const lines: string[] = [greeting, '']