Skip to content

feat(mail): add outgoing email, group email templates, and assignment emails#1439

Open
thomasbeaudry wants to merge 4 commits into
mainfrom
split/mailer
Open

feat(mail): add outgoing email, group email templates, and assignment emails#1439
thomasbeaudry wants to merge 4 commits into
mainfrom
split/mailer

Conversation

@thomasbeaudry

@thomasbeaudry thomasbeaudry commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Splits #1434 into three PRs. This is PR 2 of 3 — merges after #1440.

Merge order

Order PR Branch Base
1st #1440 — UX fixes split/ux main
2nd this PR — mailer split/mailer main
3rd #1438 — Spanish + active languages split/language split/mailer

This 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/fr

Declaring es in LanguageOptions makes es a required key in every t({ ... }) call in apps/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

  • A mail module in 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 hides all email UI when mail is off.
  • Group-manager-authored, categorized email templates per group (remote assignment / information), one active per category.
  • Emailing a remote assignment link; creating a user sends a welcome email, offering the rendered text for manual copying when delivery fails.
  • Admin mail settings page, group email templates page, plus supporting queries and mutations.

One note on ?lang

Assignment 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: nodemailer and @types/nodemailer.

Verification

tsc and eslint clean for schemas, web, api; pnpm test 382 passed / 1 skipped. Four redundant type assertions in GroupEmailTemplates were removed to satisfy no-unnecessary-type-assertion.

🤖 Generated with Claude Code

… 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>
Comment thread apps/api/src/mail/mail.service.ts Fixed
Comment thread apps/api/src/mail/mail.service.ts Fixed
thomasbeaudry and others added 2 commits July 23, 2026 14:39
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>
@thomasbeaudry

Copy link
Copy Markdown
Collaborator Author

Updated with latest main (merge commit) and fixed the failing CI.

What was red: the lint-and-test job's End-to-End Tests step — one spec, remote-assignment › should create a remote assignment and display the link, timed out (30s) waiting for the submit button.

Cause: this PR replaces the libui Form in the create-assignment dialog with a native <Button>Submit</Button>. A native button's accessible name comes from its text content, which getByLabel('Submit') does not match — so the page object's submitAssignmentForm() never found it. (The unit suite couldn't catch this; it's an e2e-only locator break.)

Fix: the page object now uses getByRole('button', { name: 'Submit' }), matching the pattern already used for native submit buttons in the instrument-render page object. Verified the locator behavior directly (role matches the native button, label matches 0 — exactly the observed timeout), and confirmed the rest of the spec still holds: the AssignmentEmailForm returns null when mail is disabled (the test default), so the Assignment Link and read-only URL input assertions are unaffected.

Also merged main, which brought in #1440's useNavItems changes — the auto-merge kept both the audit-logs reorder and this PR's Mail nav items. Full unit suite: 403 passed / 1 skipped.

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>
@thomasbeaudry

Copy link
Copy Markdown
Collaborator Author

Also fixed the CodeQL failure (separate from the e2e one above).

What it flagged: 2 critical js/request-forgery (SSRF) alerts in mail.service.ts — the outgoing fetch URL depended on admin-supplied config. The vector was the SES branch: the endpoint host is email.{awsRegion}.amazonaws.com, and awsRegion comes from the mail settings, so a crafted value like evil.example/ would repoint the request to an attacker host. (Mailgun's host is a fixed enum-selected pair and its domain is encodeURIComponent'd into the path, so it wasn't exploitable.)

Fix: constrain the region to the AWS region character set (^[a-z0-9-]+$/) in two places — at the schema boundary ($MailConfig) and at the point the host is built in mail.utils.ts, which throws on a bad value. Added unit tests covering both buildSendRequest and buildVerifyRequest rejecting a host-injecting region.

CodeQL now passes with 0 open critical alerts on the PR; full unit suite 405 passed / 1 skipped.

@gdevenyi gdevenyi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. isMailEnabled is false for every HTTP-transport provider. setup.service.ts derives it from mailConfig.enabled && mailConfig.host, but host is '' on the http transport. So an instance configured with Mailgun/SendGrid/SES/Postmark has MailService.isEnabled() === true (mail sends fine) while the public flag says false — which hides the Mail nav item, makes AssignmentEmailForm return null, and hides the welcome-email language picker. The whole HTTP path is unreachable through the UI. These two predicates need to be one function.

  2. AssignmentEmailForm resolves templates from the wrong group. The component queries /v1/groups/{currentGroup.id} from the app store, but the server resolves the template from assignment.groupId. On /datahub/$subjectId/assignments those differ whenever the assignment belongs to another group: the dropdown lists the wrong group's templates, and the templateId posted 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.

  3. SMTP password and provider secrets are stored in plaintext. MailConfig.password / apiKey (the latter being the AWS secret access key for SES) sit unencrypted in SetupStateModel. 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

  1. The language set is now defined in five unrelated placesMAIL_LANGUAGE, $LocalizedString's keys, Prisma's LocalizedString type, ALL_LANGUAGES (typed { [key: string]: string }), and $SetupState.activeLanguages: z.array(z.string()). Nothing makes them agree, and activeLanguages will 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.

  2. useUpdateGroupMutation lost its success toast for every caller. The onSuccess notification was deleted from the hook and re-added at one call site in group/manage.tsx. Any other caller now saves silently. useUpdateSetupStateMutation already models the right pattern — an optional successNotification on the hook.

  3. Lost-update race on emailTemplates. persist() sends the entire array from the client's cached copy and the service replaces it with set. Two group managers editing concurrently means one set of edits vanishes with no error.

  4. remote-assignment.tsx replaces 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 with new Date(string) — which treats 2026-08-01 as UTC midnight but 2026/08/01 as local. This is also the file that conflicts with #1441.

  5. 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),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 templateId posted isn't found in group.emailTemplates, so chosen is undefined and the controller silently falls back to DEFAULT_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>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

language is initialised once, but the option list it has to belong to (languageDropdownOptions, derived from the selected template's populated languages ∩ instrumentLanguagesactiveLanguages) 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 });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants