feat(mail): add outgoing email, group email templates, and assignment emails#1439
feat(mail): add outgoing email, group email templates, and assignment emails#1439thomasbeaudry wants to merge 4 commits into
Conversation
… emails Adds a mail module to the API backed by nodemailer, with admin-configurable SMTP settings stored on the setup state and never returned to clients. The public setup route exposes only a derived `isMailEnabled` flag so the client can hide all email UI when mail is off. Group managers can author named, categorized email templates (remote assignment / information) per group, with one active template per category. Remote assignment links can be emailed to a participant, and creating a user sends a welcome email whose rendered text is offered for manual copying when delivery fails. Adds an admin mail settings page, a group email templates page, and the supporting queries and mutations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The create-assignment dialog now uses a native `<Button>` rather than a libui
`Form`, so its submit control's accessible name comes from its text content.
`getByLabel('Submit')` no longer matches it — matching the pattern already used
for native submit buttons in the instrument render page object — which timed out
the "create a remote assignment and display the link" spec.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Updated with latest What was red: the Cause: this PR replaces the libui Fix: the page object now uses Also merged |
The Amazon SES endpoint host is built by interpolating the configured AWS
region (`email.{awsRegion}.amazonaws.com`). Because the region originates from
admin-supplied mail settings, a crafted value such as `evil.example/` would
redirect the outgoing request to an attacker-controlled host — a server-side
request forgery flagged by CodeQL (js/request-forgery, critical).
Constrain the region to the AWS region character set (lowercase letters,
digits, hyphens) both at the schema boundary and at the point the host is
built, so a value that could break out of the host is rejected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Also fixed the CodeQL failure (separate from the e2e one above). What it flagged: 2 critical Fix: constrain the region to the AWS region character set ( CodeQL now passes with 0 open critical alerts on the PR; full unit suite 405 passed / 1 skipped. |
gdevenyi
left a comment
There was a problem hiding this comment.
Review — outgoing mail
A lot of careful work here: the transport abstraction is clean, the SigV4 signing is hand-rolled rather than pulling in the AWS SDK, describeMailError/describeHttpMailError deliberately never leak raw errors, and the ~840 lines of unit tests around mail.utils/mail.service are genuinely good — including the SSRF guard on the SES region. The hasPassword/hasApiKey DTO split is the right shape.
The concerns below are mostly about two definitions of the same thing drifting apart, plus a few client/server mismatches.
Blocking
-
isMailEnabledis false for every HTTP-transport provider.setup.service.tsderives it frommailConfig.enabled && mailConfig.host, buthostis''on thehttptransport. So an instance configured with Mailgun/SendGrid/SES/Postmark hasMailService.isEnabled() === true(mail sends fine) while the public flag says false — which hides the Mail nav item, makesAssignmentEmailFormreturnnull, and hides the welcome-email language picker. The whole HTTP path is unreachable through the UI. These two predicates need to be one function. -
AssignmentEmailFormresolves templates from the wrong group. The component queries/v1/groups/{currentGroup.id}from the app store, but the server resolves the template fromassignment.groupId. On/datahub/$subjectId/assignmentsthose differ whenever the assignment belongs to another group: the dropdown lists the wrong group's templates, and thetemplateIdposted isn't found server-side, so it silently falls back to the built-in default. The user sees a template name and gets different content. -
SMTP password and provider secrets are stored in plaintext.
MailConfig.password/apiKey(the latter being the AWS secret access key for SES) sit unencrypted inSetupStateModel. Anyone with a mongodump, a backup, or read access to the DB has the instance's outbound-mail identity. The code comment says "never returned to clients", which is true and good, but that isn't the exposure that matters here. At minimum this needs an explicit decision recorded; encrypting with a key from$Env(or sourcing the secret from env entirely) would be better.
Should fix
-
The language set is now defined in five unrelated places —
MAIL_LANGUAGE,$LocalizedString's keys, Prisma'sLocalizedStringtype,ALL_LANGUAGES(typed{ [key: string]: string }), and$SetupState.activeLanguages: z.array(z.string()). Nothing makes them agree, andactiveLanguageswill happily accept a code no template can hold. AGENTS.md: one source of truth; everything derived and no loose records where a closed key set is known. -
useUpdateGroupMutationlost its success toast for every caller. TheonSuccessnotification was deleted from the hook and re-added at one call site ingroup/manage.tsx. Any other caller now saves silently.useUpdateSetupStateMutationalready models the right pattern — an optionalsuccessNotificationon the hook. -
Lost-update race on
emailTemplates.persist()sends the entire array from the client's cached copy and the service replaces it withset. Two group managers editing concurrently means one set of edits vanishes with no error. -
remote-assignment.tsxreplaces the libui<Form>and date picker with a free-text<Input type="text" placeholder="YYYY-MM-DD">. That drops the picker, drops libui's validation/error rendering, and parses withnew Date(string)— which treats2026-08-01as UTC midnight but2026/08/01as local. This is also the file that conflicts with #1441. -
No e2e coverage. AGENTS.md requires new e2e tests in
testing/alongside unit tests. Nothing here exercises the mail admin page, the templates page, or sending an assignment email. The unit tests are excellent, so this is the one gap.
Coordination with the other open PRs
The stack described in the PR body is stale — #1438 is closed and was replaced by #1442/#1443, which target main rather than being stacked on this branch. The practical consequence is that this branch and #1441/#1442 now overlap:
| Conflict | Files |
|---|---|
| #1439 × #1441 | apps/web/src/routes/_app/session/remote-assignment.tsx (real content conflict — this PR deletes the <Form> that #1441 modifies; both add the same document.querySelector autofocus effect) |
| #1439 × #1442 | apps/web/src/components/SaveStatus.tsx (add/add) |
| — | apps/web/src/utils/languages.ts, activeLanguages in schema.prisma, setup.service.ts, $SetupState are all duplicated verbatim between this PR and #1442 |
GitHub shows all four as MERGEABLE only because it compares each against main in isolation. Worth deciding an order and rebasing, or moving the shared pieces (SaveStatus, ALL_LANGUAGES, activeLanguages) into whichever lands first.
One correction to the PR body's rationale: declaring es in LanguageOptions does not make es a required key. libui types the argument as { [L in Language]?: string } (TranslateFunction in libui/dist/i18n/types.d.ts) and t() falls back to defaultLanguage at runtime. So the ordering constraint that justified splitting the Spanish strings out doesn't actually exist — which is worth knowing, because it also means nothing will ever force the 380-odd inline t({ en, fr }) calls to gain Spanish.
| isGatewayEnabled: this.configService.get('GATEWAY_ENABLED'), | ||
| // Non-secret flag so the client can hide email UI when mail is off. The SMTP | ||
| // configuration itself is never exposed here (this is a public route). | ||
| isMailEnabled: Boolean(savedOptions?.mailConfig?.enabled && savedOptions.mailConfig.host), |
There was a problem hiding this comment.
host is only populated on the smtp transport. With transport: 'http' (Mailgun/SendGrid/SES/Postmark) this is '', so isMailEnabled is false even though MailService.isEnabled() returns true and mail delivers successfully.
The client gates every piece of email UI on this flag — the Mail and Email Templates nav items, AssignmentEmailForm (returns null), and the welcome-email language picker in users/create.tsx — so the entire HTTP-provider path is unreachable through the UI.
Two predicates for "is mail on" will keep drifting. Suggest exporting the check from MailService and calling it here, e.g. isMailEnabled: await this.mailService.isEnabled(), or lifting the predicate into schemas/mail so both sides derive it from the same function.
| enabled Boolean | ||
| encryption String | ||
| host String | ||
| password String |
There was a problem hiding this comment.
password and apiKey are persisted in plaintext. For SES, apiKey holds the AWS secret access key, so a database backup or a read-only mongo credential leaks credentials that can send mail as the institution — and, for a broadly-scoped IAM key, potentially more.
The comment above correctly notes these are never returned to clients, but that isn't the exposure that matters; the DB is.
Options, roughly in order of effort: encrypt at rest with a key from $Env (envelope-encrypt just these two fields), or read the secret from an env var and store only non-secret config here. If plaintext is a deliberate accepted risk for this deployment model, please say so explicitly in the comment so the next reader doesn't have to re-derive the decision.
| const groupQuery = useQuery({ | ||
| enabled: Boolean(groupId && setupStateQuery.data.isMailEnabled), | ||
| queryFn: async () => $Group.parseAsync((await axios.get(`/v1/groups/${groupId}`)).data), | ||
| queryKey: ['group', groupId] |
There was a problem hiding this comment.
This queries the group from useAppStore().currentGroup, but the server resolves the template from assignment.groupId (assignments.controller.ts). On /datahub/$subjectId/assignments an assignment can belong to a group other than the currently-selected one, and then:
- the dropdown lists templates the server will never consider, and
- the
templateIdposted isn't found ingroup.emailTemplates, sochosenisundefinedand the controller silently falls back toDEFAULT_ASSIGNMENT_EMAIL_TEMPLATE.
The user picks "Follow-up reminder" and the participant receives the built-in default, with no error.
Fix: drive this off the assignment's own group. Assignment already carries groupId, so useQuery(['group', assignment.groupId]) would keep client and server in agreement — and would let the component be given the assignment rather than just its id.
Separately, this inline useQuery + axios.get + $Group.parseAsync is duplicated verbatim in GroupEmailTemplates.tsx. Worth a useGroupQuery(groupId) hook alongside the other hooks in @/hooks.
| const currentGroup = useAppStore((store) => store.currentGroup); | ||
| const activeLanguages = setupStateQuery.data.activeLanguages ?? ['en', 'fr']; | ||
| const [recipient, setRecipient] = useState(''); | ||
| const [language, setLanguage] = useState<string>( |
There was a problem hiding this comment.
language is initialised once, but the option list it has to belong to (languageDropdownOptions, derived from the selected template's populated languages ∩ instrumentLanguages ∩ activeLanguages) is recomputed on every render.
Switching to a template that is only authored in French while language === 'en' leaves the Select holding a value with no matching Select.Item — Radix renders an empty trigger, and sendEmail still posts 'en', which pickLocale then resolves by falling back to whatever language the template does have. The visible state and the sent state disagree.
Clamping it keeps the invalid state unrepresentable:
const language = languageDropdownOptions.includes(languageChoice)
? languageChoice
: (languageDropdownOptions[0] ?? 'en');with languageChoice as the raw state — same pattern already used for templateChoice ?? activeValue just above.
| setName(''); | ||
| setSubject({ [firstLang]: '' }); | ||
| setBody({ [firstLang]: '' }); | ||
| document.querySelector('.overflow-y-scroll')?.scrollTo({ behavior: 'smooth', top: 0 }); |
There was a problem hiding this comment.
Selecting the scroll container by Tailwind utility class reaches outside the component into whatever ancestor happens to carry overflow-y-scroll today, and breaks silently the moment that layout changes to overflow-y-auto or the class moves. It will also grab the first such element in the document, which may not be this page's scroller.
A ref on the form (or the page container) and ref.current?.scrollIntoView({ behavior: 'smooth' }) is both local and unbreakable.
| import { z } from 'zod/v4'; | ||
|
|
||
| import { $BaseModel, $RegexString } from '../core/core.js'; | ||
| import { $LocalizedString } from '../mail/mail.js'; |
There was a problem hiding this comment.
group importing from mail for a generic localized-string type inverts the layering: LocalizedString has nothing to do with mail, and this makes the group schema depend on the mail schema for all time.
It belongs in ../core/core.js next to $RegexString, where $BrandingConfig's instanceName/instanceTagline/instanceDetails and the resourceLinks[].label objects could also use it — those are hand-rolled { en, fr } objects today, which is exactly what forces the six as { [key: string]: string } casts in #1442. Consolidating on one $LocalizedString in core would fix both PRs' problem in one place.
| @ApiOperation({ summary: 'Email Assignment Link' }) | ||
| @Post(':id/email') | ||
| @RouteAccess({ action: 'update', subject: 'Assignment' }) | ||
| async sendEmail( |
There was a problem hiding this comment.
Two things on this endpoint:
Recipient is unconstrained. RouteAccess({ action: 'update', subject: 'Assignment' }) means any user who can update an assignment can make the instance's SMTP identity deliver to an arbitrary address, unthrottled. The content is template-constrained so it isn't a general open relay, but it is a free "send mail from <institution>" primitive, and the assignment URL it carries is a live credential. A rate limit (per user and per recipient) would be cheap insurance.
expiresAt is formatted in UTC. new Date(assignment.expiresAt).toISOString().slice(0, 10) renders the date in UTC, so an assignment expiring at 2026-08-01T02:00Z reads as "expires 2026-08-01" to a recipient in Montréal for whom it already expired on July 31 local. Given the participant acts on this date, formatting in the instance's timezone (or including the time) would avoid off-by-one-day support tickets.
| } | ||
|
|
||
| /** Send a message using the currently saved configuration and its active transport. */ | ||
| private async sendMail(options: SendOptions): Promise<void> { |
There was a problem hiding this comment.
sendMail re-reads the config, but every caller has already read it moments earlier: sendAssignmentEmail calls isEnabled() → getConfig() → findFirst(), then sendMail() → getConfig() → findFirst() again. getSettings() does the same via getConfig() + getNewUserEmailTemplate().
Beyond the extra round-trips, it's a TOCTOU seam: the config can change between the enabled-check and the send, so a message can go out through a configuration that was just disabled.
Reading once at the top of the public method and threading config down removes both.
| }; | ||
|
|
||
| // Autosave the SMTP configuration whenever the form settles on a valid state. | ||
| useAutosave(JSON.stringify(values), () => { |
There was a problem hiding this comment.
Autosaving on JSON.stringify(values) means partially-typed secrets get persisted. Typing a new SMTP password pauses for 1.2s mid-word → buildConfig(true) returns a valid payload (everything else is already filled in) → the truncated password is written to the DB and enabled stays true. The instance is now configured with a credential the admin never intended, and the only signal is a "saved" pill.
Excluding the secret fields from the autosave snapshot and committing them on blur (or behind an explicit action) would keep the convenience without that failure mode.
Related: values is lazily initialised from config and never resynced, so after useUpdateMailSettingsMutation invalidates and the query refetches, any server-side normalisation is invisible and the local plaintext secret keeps being re-sent on every subsequent autosave.
| <Label htmlFor="expires-at">{t({ en: 'Expires At', fr: "Date d'expiration" })}</Label> | ||
| <Input | ||
| id="expires-at" | ||
| placeholder="YYYY-MM-DD" |
There was a problem hiding this comment.
Replacing the libui <Form> with a free-text field is a real UX step back: no date picker, no locale-aware input, and no libui validation rendering — a clinician now types YYYY-MM-DD by hand.
It's also ambiguous to parse. new Date('2026-08-01') is UTC midnight, whereas new Date('2026/08/01') is local midnight; anything the user types that isn't exactly ISO silently shifts by up to a day west of UTC. The old kind: 'date' field handed you a Date and z.coerce.date().min(new Date()) did the checking.
Was there a specific problem with the <Form> here? If it was just to control the submit button's label/pending state, submitBtnLabel + suspendWhileSubmitting cover that.
Note this hunk also conflicts with #1441, which keeps the <Form> and seeds expiresAt from the new instance-wide default setting. Those two changes can't both land as written.
Splits #1434 into three PRs. This is PR 2 of 3 — merges after #1440.
Merge order
split/uxmainsplit/mailermainsplit/languagesplit/mailerThis branch is independent of #1440 and auto-merges with it cleanly. #1438 is stacked on top of this one, because Spanish has to be added to the strings introduced here — see below.
All user-facing strings here are
en/frDeclaring
esinLanguageOptionsmakesesa required key in everyt({ ... })call inapps/web, so it can't be declared until the last PR in the stack. The mailer's new strings are therefore written without Spanish here, and #1438 adds the 117 Spanish lines for them as part of its own diff. Nothing is lost and nothing is left dangling — once #1438 lands, the mail admin page, group email templates, and assignment email form are fully translated.What's here
nodemailer, with admin-configurable SMTP settings stored on the setup state and never returned to clients. The public setup route exposes only a derivedisMailEnabledflag so the client hides all email UI when mail is off.One note on
?langAssignment URLs are built here as
${assignment.url}?lang=${language}, but the gateway code that reads that param lives in #1438. Between this merging and #1438 merging, an emailed link opens the instrument in the gateway's default language rather than the recipient's. The email body itself is already correctly localized — only the linked page is affected, and it resolves as soon as #1438 lands.Lockfile
The 13k-line lockfile diff on #1434 was almost entirely missing prettier formatting, not dependency changes. This branch regenerates from
main's lockfile and re-runs prettier, leaving a 6-line diff:nodemailerand@types/nodemailer.Verification
tscandeslintclean forschemas,web,api;pnpm test382 passed / 1 skipped. Four redundant type assertions inGroupEmailTemplateswere removed to satisfyno-unnecessary-type-assertion.🤖 Generated with Claude Code