From f7557d43996d577ecf7bc0e488f9d337dd5fbc26 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 4 Oct 2026 10:13:13 -0400 Subject: [PATCH] fix(api): mask email addresses in mail failure logs and audit entries A failed send logged the recipient in plaintext, and SMTP servers echo the rejected address back in their error, so the whole log line is now passed through redactEmails. The SEND_EMAIL audit entry, which every admin can read and download, stored the plaintext recipient too; it now stores the masked form. Addresses are masked to their first character and full domain (bob.smith@example.org -> b***@example.org) rather than hashed, because an unsalted hash of an address is reversed by hashing a candidate list. The delivery result returned to the caller still carries the full address, which the clinician needs for the manual-send fallback. Closes #1547 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/assignments.controller.spec.ts | 20 ++++++++++-- .../src/assignments/assignments.controller.ts | 4 ++- .../src/mail/__tests__/mail.service.spec.ts | 13 ++++++++ .../api/src/mail/__tests__/mail.utils.spec.ts | 31 +++++++++++++++++++ apps/api/src/mail/mail.service.ts | 3 +- apps/api/src/mail/mail.utils.ts | 14 +++++++++ testing/src/specs/mail.spec.ts | 21 +++++++++++++ testing/src/support/api-client.ts | 30 ++++++++++++++++++ 8 files changed, 131 insertions(+), 5 deletions(-) diff --git a/apps/api/src/assignments/__tests__/assignments.controller.spec.ts b/apps/api/src/assignments/__tests__/assignments.controller.spec.ts index ab6199445..b2e27813b 100644 --- a/apps/api/src/assignments/__tests__/assignments.controller.spec.ts +++ b/apps/api/src/assignments/__tests__/assignments.controller.spec.ts @@ -172,13 +172,14 @@ describe('AssignmentsController', () => { }); }); - it('records who sent what to whom, with the outcome, in the audit entry', async () => { + // Every admin reads the audit log, so it records the domain the link went to, not the address. + it('records who sent what to which masked address, with the outcome, in the audit entry', async () => { assignmentsService.findById.mockResolvedValueOnce(assignment); groupsService.findById.mockResolvedValueOnce({ emailTemplates: [] }); - await sendEmail({ language: 'en', recipient: 'p@x.org' }); + await sendEmail({ language: 'en', recipient: 'participant@x.org' }); expect(auditLogger.log).toHaveBeenCalledWith('SEND_EMAIL', 'ASSIGNMENT', { groupId: 'group-1', - metadata: { assignmentId: 'assignment-1', recipient: 'p@x.org', status: 'SENT' }, + metadata: { assignmentId: 'assignment-1', recipient: 'p***@x.org', status: 'SENT' }, userId: 'user-1' }); }); @@ -217,5 +218,18 @@ describe('AssignmentsController', () => { }); await expect(sendEmail({ language: 'en', recipient: 'p@x.org' })).resolves.toMatchObject({ status: 'SENT' }); }); + + // The clinician who typed the address needs it back to send the message by hand. + it('returns the full recipient to the caller even though the audit entry masks it', async () => { + assignmentsService.findById.mockResolvedValueOnce({ ...assignment, groupId: null }); + mailService.sendAssignmentEmail.mockResolvedValueOnce({ + message: 'rendered', + recipient: 'participant@x.org', + status: 'SENT' + }); + await expect(sendEmail({ language: 'en', recipient: 'participant@x.org' })).resolves.toMatchObject({ + recipient: 'participant@x.org' + }); + }); }); }); diff --git a/apps/api/src/assignments/assignments.controller.ts b/apps/api/src/assignments/assignments.controller.ts index 5b19af140..2bddc6880 100644 --- a/apps/api/src/assignments/assignments.controller.ts +++ b/apps/api/src/assignments/assignments.controller.ts @@ -34,6 +34,7 @@ import { ASSIGNMENT_EMAIL_THROTTLER_LIMIT, ASSIGNMENT_EMAIL_THROTTLER_TTL } from import { RouteAccess } from '@/core/decorators/route-access.decorator'; import { GroupsService } from '@/groups/groups.service'; import { MailService } from '@/mail/mail.service'; +import { redactEmails } from '@/mail/mail.utils'; import { AssignmentsService } from './assignments.service'; @@ -140,10 +141,11 @@ export class AssignmentsController { }); // The entry must mean the instance's mail identity actually carried this credential toward // this address: nothing goes outbound for DISABLED or NO_RECIPIENT, so nothing is recorded. + // Every admin reads the audit log, so it keeps only the masked address. if (result.status === 'SENT' || result.status === 'FAILED') { await this.auditLogger.log('SEND_EMAIL', 'ASSIGNMENT', { groupId: assignment.groupId ?? null, - metadata: { assignmentId: id, recipient, status: result.status }, + metadata: { assignmentId: id, recipient: redactEmails(recipient), status: result.status }, userId: currentUser.id }); } diff --git a/apps/api/src/mail/__tests__/mail.service.spec.ts b/apps/api/src/mail/__tests__/mail.service.spec.ts index 83547849e..d8835bdb5 100644 --- a/apps/api/src/mail/__tests__/mail.service.spec.ts +++ b/apps/api/src/mail/__tests__/mail.service.spec.ts @@ -47,6 +47,7 @@ const newUserArgs = { }; describe('MailService', () => { + let loggingService: MockedInstance; let mailService: MailService; let setupStateModel: MockedInstance>; let transporter: { sendMail: Mock; verify: Mock }; @@ -70,6 +71,7 @@ describe('MailService', () => { ] }).compile(); moduleRef.get>(ConfigService).getOrThrow.mockReturnValue(SECRET_KEY); + loggingService = moduleRef.get(LoggingService); setupStateModel = moduleRef.get(getModelToken('SetupState')); mailService = moduleRef.get(MailService); transporter = { sendMail: vi.fn().mockResolvedValue({}), verify: vi.fn().mockResolvedValue(true) }; @@ -334,6 +336,17 @@ describe('MailService', () => { expect(result.error).toBe('AUTHENTICATION_FAILED'); }); + // SMTP servers echo the rejected address back, so masking only the recipient would still log it. + it('logs a failed delivery with the recipient masked, including where the SMTP error echoes it', async () => { + setupStateModel.findFirst.mockResolvedValue({ mailConfig: stored(validConfig) }); + transporter.sendMail.mockRejectedValueOnce( + new Error('550 5.1.1 : Recipient address rejected') + ); + await mailService.sendAssignmentEmail({ ...assignmentArgs, recipient: 'participant@x.org' }); + expect(loggingService.error).toHaveBeenCalledWith(expect.stringContaining('')); + expect(loggingService.error).not.toHaveBeenCalledWith(expect.stringContaining('participant@x.org')); + }); + it('substitutes url/expiresAt and sends when enabled', async () => { setupStateModel.findFirst.mockResolvedValue({ mailConfig: stored(validConfig) }); const result = await mailService.sendAssignmentEmail({ diff --git a/apps/api/src/mail/__tests__/mail.utils.spec.ts b/apps/api/src/mail/__tests__/mail.utils.spec.ts index 556183423..355a4bca3 100644 --- a/apps/api/src/mail/__tests__/mail.utils.spec.ts +++ b/apps/api/src/mail/__tests__/mail.utils.spec.ts @@ -6,6 +6,7 @@ import { formatExpiryDate, formatSender, pickLocale, + redactEmails, renderTemplate } from '../mail.utils'; @@ -25,6 +26,36 @@ describe('renderTemplate', () => { }); }); +// Log lines and audit entries keep the domain mail went to, never who received it. +describe('redactEmails', () => { + it('masks an address down to its first character and domain', () => { + expect(redactEmails('bob.smith@example.org')).toBe('b***@example.org'); + }); + + it('masks every local part to the same length, so the mask does not reveal it', () => { + expect(redactEmails('a@example.org')).toBe(redactEmails('abcdefgh@example.org')); + }); + + it('masks an address an SMTP server echoes inside its error', () => { + expect(redactEmails('550 5.1.1 : Recipient address rejected')).toBe( + '550 5.1.1 : Recipient address rejected' + ); + }); + + it('masks every address in the text', () => { + expect(redactEmails('to alice@a.org, bob@b.org')).toBe('to a***@a.org, b***@b.org'); + }); + + it('masks the whole local part when it holds an apostrophe or non-ASCII letters', () => { + expect(redactEmails("o'brien@example.org")).toBe('o***@example.org'); + expect(redactEmails('jösé@example.org')).toBe('j***@example.org'); + }); + + it('leaves text without an address unchanged', () => { + expect(redactEmails('getaddrinfo ENOTFOUND smtp.invalid.test')).toBe('getaddrinfo ENOTFOUND smtp.invalid.test'); + }); +}); + describe('pickLocale', () => { it('returns the requested language', () => { expect(pickLocale({ en: 'Hello', fr: 'Bonjour' }, 'fr')).toBe('Bonjour'); diff --git a/apps/api/src/mail/mail.service.ts b/apps/api/src/mail/mail.service.ts index eeac042f5..db81b9cce 100644 --- a/apps/api/src/mail/mail.service.ts +++ b/apps/api/src/mail/mail.service.ts @@ -36,6 +36,7 @@ import { formatExpiryDate, formatSender, pickLocale, + redactEmails, renderTemplate } from './mail.utils'; @@ -261,7 +262,7 @@ export class MailService { }); return { message, recipient, status: 'SENT' }; } catch (err) { - this.loggingService.error(`Failed to send "${subject}" to ${recipient}: ${String(err)}`); + this.loggingService.error(redactEmails(`Failed to send "${subject}" to ${recipient}: ${String(err)}`)); return { error: describeMailError(err), message, recipient, status: 'FAILED' }; } } diff --git a/apps/api/src/mail/mail.utils.ts b/apps/api/src/mail/mail.utils.ts index 9a6864765..271bf32df 100644 --- a/apps/api/src/mail/mail.utils.ts +++ b/apps/api/src/mail/mail.utils.ts @@ -38,6 +38,20 @@ export function formatExpiryDate(expiresAt: Date | number | string, language: La }).format(new Date(expiresAt)); } +/** + * Mask every email address in `text` down to the first character of its local part and its full + * domain — `bob.smith@example.org` becomes `b***@example.org` — so a log line or audit entry still + * shows which domain mail went to without recording who it went to. Masked rather than hashed: + * hashing every address on a candidate list reverses an unsalted hash. + * + * Applied to whole lines, not just the recipient, because SMTP servers echo the rejected address + * back in their error (`550 5.1.1 : Recipient address rejected`). + */ +export function redactEmails(text: string): string { + // A local part is any run of characters other than whitespace and the RFC 5322 specials. + return text.replace(/([^\s"(),:;<>@[\]])[^\s"(),:;<>@[\]]*@/gu, '$1***@'); +} + /** Pick a language from a localized string, falling back to any available language when the requested one is empty. */ export function pickLocale(localized: LocalizedString, language: Language): string { // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- empty strings mean "no translation"; fall back intentionally diff --git a/testing/src/specs/mail.spec.ts b/testing/src/specs/mail.spec.ts index 40c1dafb5..c9083f7cf 100644 --- a/testing/src/specs/mail.spec.ts +++ b/testing/src/specs/mail.spec.ts @@ -260,5 +260,26 @@ test.describe('mail delivery', () => { await expect(feedback).toBeVisible({ timeout: 40_000 }); await expect(feedback).not.toContainText('ENOTFOUND'); }); + + // Every admin reads the audit log and can download it whole, so it records the domain an + // assignment link went to, never the participant's address. + test('should record only a masked recipient in the audit log', async ({ api, uniqueId }) => { + const recipient = `participant${uniqueId}@example.org`; + const group = await api.createGroup(); + const assignment = await api.createAssignment({ + expiresAt: new Date(Date.now() + 86_400_000), + groupId: group.id, + instrumentId: await api.findInstrumentId('FORM'), + subjectId: await api.createSubject(group.id) + }); + + const result = await api.sendAssignmentEmail(assignment.id, { language: 'en', recipient }); + expect(result.status).toBe('FAILED'); + + const entries = await api.findAuditLogs('SEND_EMAIL'); + const entry = entries.find(({ metadata }) => metadata?.assignmentId === assignment.id); + expect(entry?.metadata?.recipient).toBe('p***@example.org'); + expect(JSON.stringify(entries)).not.toContain(recipient); + }); }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index 147f4de52..a09b8aa98 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,9 +1,13 @@ import type { $CreateAssignmentData, Assignment } from '@opendatacapture/schemas/assignment'; +import { AUDIT_LOGS_MAX_PAGE_SIZE } from '@opendatacapture/schemas/audit'; +import type { $AuditLog, $AuditLogAction, $AuditLogsPage } from '@opendatacapture/schemas/audit'; import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import type { Permissions } from '@opendatacapture/schemas/core'; import type { $CreateGroupData, Group } from '@opendatacapture/schemas/group'; import type { InstrumentInfo } from '@opendatacapture/schemas/instrument'; import type { UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; +import { MAIL_CLIENT_TIMEOUT } from '@opendatacapture/schemas/mail'; +import type { $SendAssignmentEmailData, EmailDeliveryResult } from '@opendatacapture/schemas/mail'; import type { $CreateSessionData, Session } from '@opendatacapture/schemas/session'; import type { $CreateSubjectData } from '@opendatacapture/schemas/subject'; import type { $CreateUserData, $UpdateUserData, User } from '@opendatacapture/schemas/user'; @@ -120,6 +124,19 @@ export class ApiClient { ); } + /** The newest audit entries for one action, as the audit log page and its JSON download read them. */ + async findAuditLogs(action: $AuditLogAction): Promise<$AuditLog[]> { + const { data } = await this.expectJson<$AuditLogsPage>( + this.request.get(`${API}/audit/logs`, { + headers: this.authHeaders, + params: { action, limit: AUDIT_LOGS_MAX_PAGE_SIZE } + }), + 200, + `find the '${action}' audit logs` + ); + return data; + } + /** The ids the start-session form suggests to a user with no group: custom ids scoped to the default group. */ async findDefaultGroupSubjectCustomIds(): Promise { return this.expectJson( @@ -180,6 +197,19 @@ export class ApiClient { ); } + /** Emails an assignment's link through the route the web app's send form uses, waiting out the SMTP budget. */ + async sendAssignmentEmail(id: string, data: $SendAssignmentEmailData): Promise { + return this.expectJson( + this.request.post(`${API}/assignments/${id}/email`, { + data, + headers: this.authHeaders, + timeout: MAIL_CLIENT_TIMEOUT + }), + 201, + `email assignment '${id}'` + ); + } + /** * Switch outgoing mail on or off instance-wide, (re)seeding {@link E2E_MAIL_CONFIG}. Every * caller must switch it back off — `isMailEnabled` is global, and leaving it on changes the UI