From 16477da2c741e1cbba30fe0476785006f80fc236 Mon Sep 17 00:00:00 2001 From: Alban Mouton Date: Mon, 18 May 2026 15:19:32 +0200 Subject: [PATCH] chore: harden mail send pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caller-supplied content (events htmlBody, contact-form text, …) flowed through microTemplate → mjml2html with no escaping or sanitization, so arbitrary HTML/JS rendered in recipients' mail clients. Tighten the trust boundary and the /api/mails origin/auth check. - /api/mails: switch to assertReqInternalSecret (internal-origin + secret in one helper). Accepts the modern x-secret-key header while keeping the legacy ?key= query-param fallback for graceful migration. - /api/mails: caller-supplied html runs through sanitize-html with a conservative allow-list, http/https/mailto hrefs only; text fallback is HTML-escaped (textToSafeHtml). - /api/mails/contact: user-typed text reaches htmlMsg only via textToSafeHtml (escape + \n→
); the contact-form schema admits text only. - sendMailI18n: reject non-http(s) params.link before deriving the {host}/{origin}/{link} substitutions used as button hrefs. - New api/src/mails/escape.ts holds both helpers as thin wrappers over sanitize-html, so the entity-escape and sanitize policies live in one library. - New docs/architecture/emails.md captures the send-pipeline trust model and invariants; AGENTS.md references it alongside the existing email-trust-and-site-isolation doc. - IGNORE_ASSERT_REQ_INTERNAL=1 added to the api dev script and the in-process test harness so existing tests through nginx still exercise the route; production has no env-var bypass. - test-env DELETE also wipes sd-rate-limiter-contact, matching the existing sd-rate-limiter-auth cleanup. - mails.api.spec.ts: 4 new cases — plain-text escape, html sanitization, javascript: href stripping, contact-form escape. Co-Authored-By: Claude Opus 4.7 (1M context) --- AGENTS.md | 1 + api/package.json | 3 +- api/src/mails/escape.ts | 28 +++++ api/src/mails/router.ts | 23 ++-- api/src/mails/service.ts | 3 + api/src/test-env.ts | 1 + docs/architecture/emails.md | 183 +++++++++++++++++++++++++++++ package-lock.json | 116 +++++++++++++++++- package.json | 1 + tests/features/mails.api.spec.ts | 82 ++++++++++++- tests/support/in-process-server.ts | 1 + 11 files changed, 426 insertions(+), 16 deletions(-) create mode 100644 api/src/mails/escape.ts create mode 100644 docs/architecture/emails.md diff --git a/AGENTS.md b/AGENTS.md index e93970d6..cb34e1df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,3 +62,4 @@ npm run quality # Full quality check (lint + types + tests) Read before changing anything in the corresponding area: - [`docs/architecture/email-trust-and-site-isolation.md`](docs/architecture/email-trust-and-site-isolation.md) -- how SSO email claims are verified and how site-level SSO trust is confined so a compromised site config cannot escalate to superadmin or cross-site takeover. Required reading for changes to auth providers, `cleanUser`, `authProviderLoginCallback`, `adminMode`, or the change-host flow. +- [`docs/architecture/emails.md`](docs/architecture/emails.md) -- the outbound email pipeline: the two `/api/mails*` endpoints, sanitization/escape at the trust boundary, MJML template substitution, and `sendMailI18n`. Required reading for changes under `api/src/mails/`, the MJML templates, the mail schemas, or any caller that posts to `/api/mails`. diff --git a/api/package.json b/api/package.json index ded41299..b169cdca 100644 --- a/api/package.json +++ b/api/package.json @@ -4,7 +4,7 @@ "type": "module", "license": "MIT", "scripts": { - "dev": "mkdir -p ../dev/logs && NODE_ENV=development ENABLE_TEST_API=1 DEBUG=upgrade* node --watch --experimental-strip-types index.ts 2>&1 | tee ../dev/logs/dev-api.log" + "dev": "mkdir -p ../dev/logs && NODE_ENV=development ENABLE_TEST_API=1 IGNORE_ASSERT_REQ_INTERNAL=1 DEBUG=upgrade* node --watch --experimental-strip-types index.ts 2>&1 | tee ../dev/logs/dev-api.log" }, "imports": { "#config": "./src/config.ts", @@ -53,6 +53,7 @@ "prom-client": "^15.1.3", "qrcode": "^1.5.4", "rate-limiter-flexible": "^5.0.5", + "sanitize-html": "^2.13.0", "@authenio/samlify-node-xmllint": "^2.0.0", "samlify": "^2.11.0", "seedrandom": "^3.0.5", diff --git a/api/src/mails/escape.ts b/api/src/mails/escape.ts new file mode 100644 index 00000000..e8bc91c7 --- /dev/null +++ b/api/src/mails/escape.ts @@ -0,0 +1,28 @@ +import sanitizeHtml from 'sanitize-html' + +// HTML-escape policy: every tag is escaped to entities. Used by textToSafeHtml +// to turn caller-supplied plain text into safe-to-substitute HTML. +const textToSafeHtmlOptions: sanitizeHtml.IOptions = { + allowedTags: [], + allowedAttributes: {}, + disallowedTagsMode: 'escape' +} + +// HTML sanitization policy: conservative allow-list, http/https/mailto hrefs +// only. Used by sanitizeMailHtml on caller-supplied html for /api/mails. +const sanitizeMailHtmlOptions: sanitizeHtml.IOptions = { + allowedTags: ['a', 'b', 'i', 'em', 'strong', 'u', 'br', 'p', 'ul', 'ol', 'li', 'code', 'pre', 'blockquote', 'span', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6'], + allowedAttributes: { + a: ['href'], + '*': ['style'] + }, + allowedSchemes: ['http', 'https', 'mailto'], + allowedSchemesAppliedToAttributes: ['href'], + allowProtocolRelative: false +} + +export const textToSafeHtml = (text: string) => + sanitizeHtml(text, textToSafeHtmlOptions).replace(/\r?\n/g, '
') + +export const sanitizeMailHtml = (html: string) => + sanitizeHtml(html, sanitizeMailHtmlOptions) diff --git a/api/src/mails/router.ts b/api/src/mails/router.ts index 0b341087..5aa54b76 100644 --- a/api/src/mails/router.ts +++ b/api/src/mails/router.ts @@ -1,7 +1,6 @@ import config from '#config' import { Router } from 'express' -import { reqSiteUrl, reqIp, reqUser, session, httpError, reqIsInternal } from '@data-fair/lib-express' -import { internalError } from '@data-fair/lib-node/observer.js' +import { reqSiteUrl, reqIp, reqUser, session, httpError, assertReqInternalSecret } from '@data-fair/lib-express' import storages from '#storages' import mongo from '#mongo' import { RateLimiterMongo } from 'rate-limiter-flexible' @@ -9,6 +8,7 @@ import emailValidator from 'email-validator' import multer from 'multer' import { reqI18n } from '#i18n' import { sendMail } from './service.ts' +import { textToSafeHtml, sanitizeMailHtml } from './escape.ts' import type { FindMembersParams } from '../storages/interface.ts' import { reqSite } from '#services' @@ -18,15 +18,8 @@ export default router const upload = multer({ storage: multer.diskStorage({}) }) router.post('/', async (req, res, next) => { - const key = req.query.key - if (!config.secretKeys.sendMails || config.secretKeys.sendMails !== key) { - throw httpError(403, 'Bad secret in "key" parameter') - } - if (!reqIsInternal(req)) { - internalError('mails-send', 'Trying to send mails from an external request') - // TODO: make this blocking in a coming release - // throw httpError(403, 'Forbidden') - } + if (!config.secretKeys.sendMails) throw httpError(403, 'sendMails secret is not configured') + assertReqInternalSecret(req, config.secretKeys.sendMails) next() }, upload.any(), async (req, res) => { const mailBody = (await import('#types/mail/index.ts')).returnValid(typeof req.body.body === 'string' ? JSON.parse(req.body.body) : req.body) @@ -76,13 +69,17 @@ router.post('/', async (req, res, next) => { } } + const htmlMsg = mailBody.html + ? sanitizeMailHtml(mailBody.html) + : textToSafeHtml(mailBody.text ?? '') + results.push(await sendMail([...to].join(', '), { replyTo: mailBody.replyTo, host, path, subject: mailBody.subject, text: mailBody.text ?? '', - htmlMsg: mailBody.html ?? mailBody.text ?? '', + htmlMsg, htmlCaption: '' }, attachments)) } @@ -138,7 +135,7 @@ router.post('/contact', async (req, res) => { path: site?.path, subject: req.body.subject, text, - htmlMsg: text, + htmlMsg: textToSafeHtml(text), htmlCaption: '' }) res.send(req.body) diff --git a/api/src/mails/service.ts b/api/src/mails/service.ts index b5cccc3c..4d50a041 100644 --- a/api/src/mails/service.ts +++ b/api/src/mails/service.ts @@ -75,6 +75,9 @@ export const getI18NParams = (key: string, messages: any, params: SendMailI18nPa export const sendMailI18n = async (key: string, messages: any, to: string, params: SendMailI18nParams) => { if (params.link) { const linkUrl = new URL(params.link) + if (linkUrl.protocol !== 'http:' && linkUrl.protocol !== 'https:') { + throw new Error(`refusing to send mail with non-http(s) link: ${linkUrl.protocol}`) + } params.host = linkUrl.host params.origin = linkUrl.origin params.path = linkUrl.pathname diff --git a/api/src/test-env.ts b/api/src/test-env.ts index f67a6d95..8bf84255 100644 --- a/api/src/test-env.ts +++ b/api/src/test-env.ts @@ -33,6 +33,7 @@ router.delete('/', async (req, res) => { } await mongo.passwordLists.deleteMany() await mongo.db.collection('sd-rate-limiter-auth').deleteMany() + await mongo.db.collection('sd-rate-limiter-contact').deleteMany() const { getSiteByHost } = await import('./sites/service.ts') getSiteByHost.clear() // Force a fresh SAML cert mint on the next request — exercises createCert end-to-end diff --git a/docs/architecture/emails.md b/docs/architecture/emails.md new file mode 100644 index 00000000..263b3f53 --- /dev/null +++ b/docs/architecture/emails.md @@ -0,0 +1,183 @@ +# Email send pipeline + +How simple-directory accepts, sanitizes, templates and dispatches outbound +email. Required reading before changing anything under `api/src/mails/`, +either of the two `/api/mails*` routes, the MJML templates, or +`sendMailI18n`. + +See also [`./email-trust-and-site-isolation.md`](./email-trust-and-site-isolation.md) +for how SSO email *claims* are verified — orthogonal to this doc, which +covers the SMTP-send path only. + +## Why this surface is sensitive + +The mail pipeline takes content from three populations of callers: + +- **Internal services** (events, future others) post arbitrary `html` to + `POST /api/mails` over a shared secret. +- **Anonymous web visitors** post arbitrary `text` to + `POST /api/mails/contact` through any portal's contact form. +- **simple-directory itself** sends i18n-templated mail via `sendMailI18n` + (signup, login, invitations, planned-deletion, …). + +All three feed a single MJML pipeline in `api/src/mails/service.ts` that +does plain `String.replace`-style substitution before MJML parses. Any value +substituted into a template is HTML in the recipient's mail client; values +that reach `href=` / `src=` attributes are URLs in the recipient's mail +client. The trust boundary therefore lives at the **values**, not at the +template. + +## Pipeline + +``` +caller payload + │ + ▼ +endpoint ─ schema validation (api/types/mail/schema.js, api/contract/contact-mail.ts) + │ ─ auth / rate-limit (assertReqInternal + secret, or session+IP-rate) + ▼ +escape / sanitize at boundary (api/src/mails/escape.ts) + │ + ▼ +sendMailI18n (optional layer 1) microTemplate(messages.mails[key][k], i18nParams) ─ api/src/mails/service.ts + │ fills {host}, {origin}, {link}, {contact} into the i18n strings + ▼ +sendMail (layer 2, always) microTemplate(template, tmplParams) ─ api/src/mails/service.ts:114 + │ fills {htmlMsg}, {htmlCaption}, {htmlButton}, {htmlAlternativeLink}, {link}, {logo}, theme.* into the MJML + ▼ +mjml2html (MJML → HTML) + │ + ▼ +nodemailer transport (api/src/mails/transport.ts) +``` + +`microTemplate` is `@data-fair/lib-utils/micro-template.js`, a literal +`String.replace` over `{key}` patterns — **no escaping**. The escape/ +sanitize step in front of it is the only thing that keeps caller-controlled +HTML from reaching the recipient. + +## Endpoints + +### `POST /api/mails` — internal-only, secret-key gated + +- File: `api/src/mails/router.ts:20-90`. +- Body: `api/types/mail/schema.js` — `to`, `subject`, `text?`, `html?`, + `replyTo?`, `sender?`. +- Auth: `assertReqInternalSecret(req, config.secretKeys.sendMails)` (from + `@data-fair/lib-express`). That helper enforces both the internal-origin + check **and** the secret in one call, accepting the secret via the + `x-secret-key` header (preferred) or the legacy `?key=` query param + (kept as a deprecated fallback that logs a warning, so existing callers + keep working while they migrate). The internal-origin check is bypassed + in dev/test by `IGNORE_ASSERT_REQ_INTERNAL=1` (set by `npm -w api run dev` + and `tests/support/in-process-server.ts`); in production it is + unconditional. +- Sanitization of the `htmlMsg` value substituted into the MJML template: + - `body.html` present → `sanitizeMailHtml(body.html)` — allow-list of + safe tags, `href` restricted to `http` / `https` / `mailto`. + - `body.html` absent → `textToSafeHtml(body.text)` — HTML-escape + `\n`→`
`. +- The plain-text part sent to nodemailer is `body.text` unmodified + (recipients on text-only clients see what the caller composed). + +### `POST /api/mails/contact` — anonymous, rate-limited + +- File: `api/src/mails/router.ts:94-145`. +- Body: `api/contract/contact-mail.ts` — `from` (email), `subject`, + `text`. **No `html` field by schema.** +- Auth: enabled only if `config.anonymousContactForm`; requires + `req.body.token` (an anonymous-action token, used as a present-on-page + proof) plus IP-based rate limit (1 req / 60s, mongo-backed via + `RateLimiterMongo`). +- The `text` is wrapped with a "Message transmitted by the contact form of + …" prefix and sent both as plain-text and as + `textToSafeHtml(...)` → `htmlMsg`. The schema accepts text only, so the + caller never gets to inject HTML. + +### Direct `sendMailI18n` (internal, no HTTP) + +- File: `api/src/mails/service.ts:75-83`. +- Callers: `api/src/auth/router.ts`, `api/src/users/router.ts`, + `api/src/users/worker.ts`, `api/src/invitations/router.ts`, + `api/src/organizations/router.ts`. +- The i18n templates (`api/i18n/{en,fr,…}.js`, `mails:` section) are + authored alongside the service and contain HTML (``, + …). The substituted values are derived from a validated URL: + `sendMailI18n` asserts `params.link` is `http(s):` before deriving + `{host}`, `{origin}`, `{path}`. + +## Known external callers + +| Service | Endpoint | Caller (file:line) | What reaches the template | +|---------|----------|---------------------|---------------------------| +| portals | `POST /api/mails/contact` | `portal/app/components/page-element/basic/page-element-contact.vue:354` | `text` (the form body — schema rejects html, server escapes) | +| events | `POST /api/mails` | `events/api/src/notifications/service.ts:65` | `html` (notification `htmlBody`, third-party-supplied) — sanitized by `sanitizeMailHtml` | +| sd internal | `sendMailI18n` direct | auth / users / invitations / organizations routers, users worker | i18n template HTML; substituted params come from validated URLs and trusted config | + +## Templates + +- `api/src/mails/generic-mail.mjml` — used when `params.htmlButton` is set. + Placeholders inside `` (text context): `{htmlMsg}`, + `{htmlAlternativeLink}`, `{link}`, `{htmlCaption}`. Inside attributes + (URL context): `{logo}` (`src`), `{link}` (`href`), + `{theme.colors.primary}` (`background-color`). +- `api/src/mails/generic-mail-nobutton.mjml` — same shape without the + button block. +- `api/src/mails/{mail,mail-nobutton}.mjml` — optional operator-supplied + overrides for the main site (loaded from disk at startup). +- A legacy `/webapp/server/mails/mail.mjml` path is still read for back- + compat with a `console.error` to nag operators to migrate. + +No template uses ``. Substitution happens *before* MJML parses, +so a value containing `` would still be +honoured by mjml — that is why sanitization sits at the value boundary +above, not at the template. + +## Operator-trusted inputs (bypass sanitization by design) + +These values flow into the MJML pipeline unfiltered. They are part of the +same trust model as the rest of operator-supplied config: + +- `config.mails.extraParams` (`api/src/mails/service.ts:109`) — spread last + into `tmplParams`, so an operator can override anything. +- `config.theme.*`, `site.theme.*` — colours and `logo` URL. +- `config.contact`, `site.mails.contact`, `config.mails.from`, + `site.mails.from` — addresses substituted as `{contact}` and used as + `From:` / `replyTo`. +- Operator-supplied templates `mail.mjml` / `mail-nobutton.mjml`. + +This matches the broader "main-site / operator config is fully trusted" +posture in [`./email-trust-and-site-isolation.md`](./email-trust-and-site-isolation.md). + +## Invariants + +1. `POST /api/mails` requires both a valid `sendMails` secret **and** + `assertReqInternal(req)` — enforced by `assertReqInternalSecret`. The + internal-origin check is unconditional in production (no env-var bypass). +2. Caller-supplied `html` to `POST /api/mails` is run through + `sanitizeMailHtml` (a strict tag allow-list, `href` schemes restricted + to `http`/`https`/`mailto`) before reaching the MJML substitution. +3. Caller-supplied `text` to either endpoint reaches `htmlMsg` only via + `textToSafeHtml` (HTML-escape + `\n`→`
`). +4. The contact-form schema (`api/contract/contact-mail.ts`) admits `text` + only; the server never reads an html field from the contact-form caller. +5. `sendMailI18n` rejects a `params.link` whose protocol is not `http:` or + `https:`, so `{link}` (button `href`) and the derived `{origin}` / + `{host}` cannot carry a `javascript:` / `data:` payload into an i18n + template. +6. The MJML templates contain no `` and are not writable at + runtime; placeholder substitution is the only injection surface, and + it is gated by invariants 2–5. + +Violations re-open the C-class injection paths flagged in the +2026-05 portals review around `microTemplate` + `mjml2html`. + +## References + +- `api/src/mails/router.ts` — both endpoints +- `api/src/mails/service.ts` — `sendMail`, `sendMailI18n`, MJML rendering +- `api/src/mails/escape.ts` — `textToSafeHtml`, `sanitizeMailHtml` +- `api/src/mails/generic-mail.mjml`, `generic-mail-nobutton.mjml` +- `api/types/mail/schema.js` — `/api/mails` body schema +- `api/contract/contact-mail.ts` — `/api/mails/contact` body schema +- `api/i18n/{en,fr,es,it,pt,de}.js` — i18n mail strings under `mails:` +- `tests/features/mails.api.spec.ts` — endpoint, sanitization, escape tests diff --git a/package-lock.json b/package-lock.json index 8fea720f..9d627b07 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,6 +42,7 @@ "@types/node-cron": "^3.0.11", "@types/nodemailer": "^6.4.23", "@types/qrcode": "^1.5.6", + "@types/sanitize-html": "^2.13.0", "@types/seedrandom": "^3.0.8", "@types/serialize-javascript": "^5.0.4", "@types/simple-oauth2": "^5.0.8", @@ -100,6 +101,7 @@ "qrcode": "^1.5.4", "rate-limiter-flexible": "^5.0.5", "samlify": "^2.11.0", + "sanitize-html": "^2.13.0", "seedrandom": "^3.0.5", "serialize-javascript": "^7.0.4", "simple-oauth2": "^5.1.0", @@ -2930,6 +2932,49 @@ "version": "1.2.7", "license": "MIT" }, + "node_modules/@types/sanitize-html": { + "version": "2.16.1", + "resolved": "https://registry.npmjs.org/@types/sanitize-html/-/sanitize-html-2.16.1.tgz", + "integrity": "sha512-n9wjs8bCOTyN/ynwD8s/nTcTreIHB1vf31vhLMGqUPNHaweKC4/fAl4Dj+hUlCTKYgm4P3k83fmiFfzkZ6sgMA==", + "dev": true, + "license": "MIT", + "dependencies": { + "htmlparser2": "^10.1" + } + }, + "node_modules/@types/sanitize-html/node_modules/entities": { + "version": "7.0.1", + "resolved": "https://registry.npmjs.org/entities/-/entities-7.0.1.tgz", + "integrity": "sha512-TWrgLOFUQTH994YUyl1yT4uyavY5nNB5muff+RtWaqNVCAK408b5ZnnbNAUEWLTCpum9w6arT70i1XdQ4UeOPA==", + "dev": true, + "license": "BSD-2-Clause", + "engines": { + "node": ">=0.12" + }, + "funding": { + "url": "https://github.com/fb55/entities?sponsor=1" + } + }, + "node_modules/@types/sanitize-html/node_modules/htmlparser2": { + "version": "10.1.0", + "resolved": "https://registry.npmjs.org/htmlparser2/-/htmlparser2-10.1.0.tgz", + "integrity": "sha512-VTZkM9GWRAtEpveh7MSF6SjjrpNVNNVJfFup7xTY3UpFtm67foy9HDVXneLtFVt4pMz5kZtgNcvCniNFb1hlEQ==", + "dev": true, + "funding": [ + "https://github.com/fb55/htmlparser2?sponsor=1", + { + "type": "github", + "url": "https://github.com/sponsors/fb55" + } + ], + "license": "MIT", + "dependencies": { + "domelementtype": "^2.3.0", + "domhandler": "^5.0.3", + "domutils": "^3.2.2", + "entities": "^7.0.1" + } + }, "node_modules/@types/seedrandom": { "version": "3.0.8", "dev": true, @@ -4861,6 +4906,15 @@ "version": "0.1.4", "license": "MIT" }, + "node_modules/deepmerge": { + "version": "4.3.1", + "resolved": "https://registry.npmjs.org/deepmerge/-/deepmerge-4.3.1.tgz", + "integrity": "sha512-3sUqbMEc77XqpdNO7FRyRog+eW3ph+GYCbj+rK+uYyRMuwsVy0rMiVtPn+QJlKFvWP/1PYpapqYn0Me2knFn+A==", + "license": "MIT", + "engines": { + "node": ">=0.10.0" + } + }, "node_modules/define-data-property": { "version": "1.1.4", "license": "MIT", @@ -7146,7 +7200,6 @@ }, "node_modules/is-plain-object": { "version": "5.0.0", - "dev": true, "license": "MIT", "engines": { "node": ">=0.10.0" @@ -7716,6 +7769,15 @@ "json-buffer": "3.0.1" } }, + "node_modules/launder": { + "version": "1.7.1", + "resolved": "https://registry.npmjs.org/launder/-/launder-1.7.1.tgz", + "integrity": "sha512-mU6WRz5EusL9ZZuiZ5SO4Y6C0P9PAUR9iwdb6bzj4KDihm28DiHFw+/yk9DBH4f+Pv1wuzQ4e2jV3oQ7mkIqvw==", + "license": "MIT", + "dependencies": { + "dayjs": "^1.11.7" + } + }, "node_modules/ldapjs": { "version": "3.0.7", "license": "MIT", @@ -9486,6 +9548,12 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/parse-srcset": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/parse-srcset/-/parse-srcset-1.0.2.tgz", + "integrity": "sha512-/2qh0lav6CmI15FzA3i/2Bzk2zCgQhGMkvhOhKNcBVQ1ldgpbfiNTVslmooUmWJcADi1f1kIeynbDRVzNlfR6Q==", + "license": "MIT" + }, "node_modules/parse5": { "version": "7.3.0", "resolved": "https://registry.npmjs.org/parse5/-/parse5-7.3.0.tgz", @@ -10366,6 +10434,52 @@ "node": ">=0.6.0" } }, + "node_modules/sanitize-html": { + "version": "2.17.4", + "resolved": "https://registry.npmjs.org/sanitize-html/-/sanitize-html-2.17.4.tgz", + "integrity": "sha512-2HW7v2ol/uAM7sX4hbD8Z59OGWmAPrvjL8E71UWlBcj6m+kcF6ilQBLny+cIgY214QJeJT5tQuxKKqX0SQqjGQ==", + "license": "MIT", + "dependencies": { + "deepmerge": "^4.2.2", + "escape-string-regexp": "^4.0.0", + "htmlparser2": "^10.1.0", + "is-plain-object": "^5.0.0", + "launder": "^1.7.1", + "parse-srcset": "^1.0.2", + "postcss": "^8.3.11" + } + }, + "node_modules/sanitize-html/node_modules/entities": { + "version": "7.0.1", + "resolved": "https://registry.npmjs.org/entities/-/entities-7.0.1.tgz", + "integrity": "sha512-TWrgLOFUQTH994YUyl1yT4uyavY5nNB5muff+RtWaqNVCAK408b5ZnnbNAUEWLTCpum9w6arT70i1XdQ4UeOPA==", + "license": "BSD-2-Clause", + "engines": { + "node": ">=0.12" + }, + "funding": { + "url": "https://github.com/fb55/entities?sponsor=1" + } + }, + "node_modules/sanitize-html/node_modules/htmlparser2": { + "version": "10.1.0", + "resolved": "https://registry.npmjs.org/htmlparser2/-/htmlparser2-10.1.0.tgz", + "integrity": "sha512-VTZkM9GWRAtEpveh7MSF6SjjrpNVNNVJfFup7xTY3UpFtm67foy9HDVXneLtFVt4pMz5kZtgNcvCniNFb1hlEQ==", + "funding": [ + "https://github.com/fb55/htmlparser2?sponsor=1", + { + "type": "github", + "url": "https://github.com/sponsors/fb55" + } + ], + "license": "MIT", + "dependencies": { + "domelementtype": "^2.3.0", + "domhandler": "^5.0.3", + "domutils": "^3.2.2", + "entities": "^7.0.1" + } + }, "node_modules/sass": { "version": "1.98.0", "resolved": "https://registry.npmjs.org/sass/-/sass-1.98.0.tgz", diff --git a/package.json b/package.json index 125fe3ae..4cd0826b 100644 --- a/package.json +++ b/package.json @@ -59,6 +59,7 @@ "@types/node-cron": "^3.0.11", "@types/nodemailer": "^6.4.23", "@types/qrcode": "^1.5.6", + "@types/sanitize-html": "^2.13.0", "@types/seedrandom": "^3.0.8", "@types/serialize-javascript": "^5.0.4", "@types/simple-oauth2": "^5.0.8", diff --git a/tests/features/mails.api.spec.ts b/tests/features/mails.api.spec.ts index 268fb014..ecd12d35 100644 --- a/tests/features/mails.api.spec.ts +++ b/tests/features/mails.api.spec.ts @@ -4,6 +4,12 @@ import { test } from '@playwright/test' import { axios, testEnvAx, maildevAx, deleteAllEmails } from '../support/axios.ts' import FormData from 'form-data' +const findEmail = async (subject: string) => { + await new Promise(resolve => setTimeout(resolve, 50)) + const emails: any[] = (await maildevAx.get('/email')).data + return emails.find(m => m.subject === subject) +} + test.describe('mails', () => { test.beforeEach(async () => { await deleteAllEmails() @@ -13,7 +19,7 @@ test.describe('mails', () => { test('Try to send mail whithout the secret', async () => { const ax = await axios() - await assert.rejects(ax.post('/api/mails', {}), (res: any) => res.status === 403) + await assert.rejects(ax.post('/api/mails', {}), (res: any) => res.status === 401) }) test('Send email to a user', async () => { @@ -64,6 +70,80 @@ test.describe('mails', () => { assert.equal(email.envelope.to.length, 2) }) + test('Plain text body is HTML-escaped in the rendered email', async () => { + const ax = await axios() + const res = await ax.post('/api/mails', { + to: ['injection-text@test.com'], + subject: 'injection-text', + text: 'hello \nnew line' + }, { params: { key: 'testkey' } }) + assert.equal(res.status, 200) + const email = await findEmail('injection-text') + assert.ok(email) + // plain-text part stays as-is so the recipient sees the original text in a text client + assert.equal(email.text, 'hello \nnew line') + // html part has the script escaped and newlines turned into
+ assert.ok(email.html.includes('<script>alert(1)</script>'), + 'html should contain the escaped script') + assert.ok(!email.html.includes(''), + 'html must not contain a raw kept' + }, { params: { key: 'testkey' } }) + assert.equal(res.status, 200) + const email = await findEmail('injection-html') + assert.ok(email) + assert.ok(email.html.includes('

hello

'), 'safe tags should survive') + assert.ok(email.html.includes('kept'), 'safe tags should survive') + assert.ok(!email.html.includes(' should be stripped') + assert.ok(!email.html.includes('onerror'), 'event handlers should be stripped') + // MJML compiles the template logo to an ; the attacker should + // not appear — check by its unique src + assert.ok(!email.html.match(/]*src=["']?x["']?/), 'attacker image should be stripped') + }) + + test('javascript: hrefs are stripped from caller html', async () => { + const ax = await axios() + const res = await ax.post('/api/mails', { + to: ['injection-href@test.com'], + subject: 'injection-href', + html: 'click ok' + }, { params: { key: 'testkey' } }) + assert.equal(res.status, 200) + const email = await findEmail('injection-href') + assert.ok(email) + assert.ok(!email.html.includes('javascript:'), 'javascript: scheme must be dropped') + assert.ok(email.html.includes('https://example.com/ok'), 'http(s) href should survive') + }) + + test('Contact form: user text is HTML-escaped in the html part', async () => { + await testEnvAx.patch('/config', { anonymousContactForm: true }) + const ax = await axios() + const token = (await ax.get('/api/auth/anonymous-action')).data + const res = await ax.post('/api/mails/contact', { + token, + from: 'visitor@test.com', + subject: 'contact-injection', + text: 'hello \nsecond line' + }) + assert.equal(res.status, 200) + const email = await findEmail('contact-injection') + assert.ok(email) + assert.ok(email.html.includes('<script>alert(1)</script>'), + 'html should contain escaped '), + 'html must not contain a raw