Follows #28340, now merged.
`smtp.go` renders the notification title through
`PlaintextFromMarkdown`, which strips Markdown **and decodes HTML
entities**, then stores the result in `Labels["_subject"]`.
`html.gotmpl` interpolated that raw into `<title>` and `<h1>`, so an
entity-encoded payload in a user-controlled label arrived as live
markup:
```
template_display_name = <a href="https://attacker.example/login">Re-authenticate now</a>
-> <title>Template "<a href="https://attacker.example/login">Re-authenticate now</a>" deleted</title>
```
Markdown escaping cannot reach this. `&` is not backslash-escapable in
either renderer, and this path never enters gomarkdown, so neither
`html.SkipHTML` nor the `Safelink` added in #28340 sees the string. `{{
.UserName }}` was interpolated raw at the same template, straight from
the unescaped payload the dispatcher receives.
This PR adds `| html` to seven values across eleven positions in
`html.gotmpl`:
| Value | Positions | Why |
|---|---|---|
| `.Labels._subject` | 2 | the injection above, in `<title>` and `<h1>`
|
| `.UserName` | 1 | reaches the template straight from the unescaped
payload |
| `$action.URL` | 1 | rendered from user data at `enqueuer.go:201`;
`EscapedForMarkdown` does not touch `Actions` |
| `$action.Label` | 1 | static today, escaped so it stays safe if that
changes |
| `base_url` | 4 | `--access-url` is scheme-checked only, so a `"`
closes the `href` |
| `current_year` | 1 | cannot carry markup, escaped so the rule has no
exceptions |
| `.NotificationTemplateID` | 1 | same |
`logo_url` and `app_name` were already escaped in #28340. `{{
.Labels._body }}` stays unescaped: it is intentionally gomarkdown
output, and it is the only value in the file that is not escaped.
The action and `base_url` values are defense in depth rather than open
holes. A `"` in an action URL fails closed at enqueue, because the
rendered actions JSON is unmarshalled before use and the quote breaks
that parse; `<`, `>`, `&` and `'` survive but are inert inside a
double-quoted attribute. `base_url` requires an operator to set a
hostile `--access-url`.
Every value is guarded by a test. Removing `| html` from any of the nine
escaped values now fails a named test, verified by removing each pipe in
turn:
- `TestSMTPHTMLTemplateEscapesUntrustedValues` covers `_subject`,
`UserName` and both action values.
- `TestSMTPHTMLTemplateEscapesTrustedValues` covers `base_url`,
`current_year` and `.NotificationTemplateID`, none of which can carry
markup in production, so no golden file would catch their regression.
- `TestSMTPHTMLTemplateEscapesAppearanceHelpers` covers `logo_url` and
`app_name`.
That last point is why the trusted values needed tests rather than
goldens: escaping them costs zero golden churn, so nothing already in
the tree fails when it is removed. Before this PR the same was true of
`$action.Label`, whose escaping could be deleted with no golden diff and
no failing test at all.
The 36 golden files change by entity encoding only, mostly `"` to
`"` and `'` to `'` in subjects. Verified by quoted-printable
decoding every file before and after and confirming the two are
identical once HTML entities are decoded: 36/36 with no semantic
difference. Escaping `base_url`, `current_year` and
`.NotificationTemplateID` added no further churn.
**NOTE**: `$action.URL | html` turns the `&` in the one-time passcode
reset link into `&`. That is the correct encoding of a literal `&`
in an attribute value, and every conformant client decodes it before
navigating, so the request the server receives is unchanged. It is the
only golden change with behavior attached.
Migrating this template to `html/template` was considered and declined;
the reasoning and the conditions that would reverse it are on the CRF-3
review thread.
Refs
https://linear.app/codercom/issue/PLAT-273/markdown-link-injection-into-admin-notification-emails-sec-93
This reverts commit 07f79af65b ("fix:
markdown rendering improvements").
The revert was applied cleanly with `git revert` and restores the
notification rendering pipeline to its prior state, including:
- `coderd/notifications/dispatch/smtp.go` and `smtp/html.gotmpl`
- `coderd/notifications/notifier.go` and `render/gotmpl.go`
- Removal of `coderd/notifications/render/sanitize_test.go` and
`smtp_internal_test.go` additions
- Regenerated SMTP golden templates under
`coderd/notifications/testdata/`
`go build ./coderd/notifications/...` passes on the reverted tree.
---
_This PR was generated by Coder Agents on behalf of @jdomeracki-coder._
Improvements to markdown rendering in notification emails:
- More consistent escaping of values interpolated into notification templates
- Stricter link handling in the notification email renderer, scoped to the notification rendering path
- HTML escaping of values interpolated into the outer email template
- Expanded unit and end-to-end coverage of the notification rendering pipeline
- `make gen` run to regenerate golden files for SMTP and webhook notification templates
This PR aimes to [fix this
issue](https://github.com/coder/internal/issues/448) -
The main idea is to remove greetings from templates stored in the DB -
and instead push it into the template for require methods - for now
SMTP.
This PR aims to close#14253
We keep the default behavior using the Coder logo if there's no logo
set.
Otherwise we want to use the logo based on the URL set in appearance.
---------
Co-authored-by: defelmnq <yvincent@coder.com>
Bobby Ho