fix(coderd/notifications): HTML-escape the email template values (#28397) by BobbyHo · Pull Request #28646 · coder/coder · GitHub
Skip to content

fix(coderd/notifications): HTML-escape the email template values (#28397) - #28646

Merged
mtojek merged 1 commit into
release/2.34from
backport/28397-to-2.34
Aug 26, 2026
Merged

fix(coderd/notifications): HTML-escape the email template values (#28397)#28646
mtojek merged 1 commit into
release/2.34from
backport/28397-to-2.34

Conversation

@BobbyHo

@BobbyHo BobbyHo commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Backport of #28397

Original PR: #28397 — fix(coderd/notifications): HTML-escape the email template values
Merge commit: 2236710
Requested by: @BobbyHo

Opened manually because the Backport workflow run for release/2.34 failed: https://github.com/coder/coder/actions/runs/32985803559/job/98231735640

Why the automatic backport failed

Two separate things went wrong in that run, and only one of them is about conflicts.

The cherry-pick hit nine modify/delete conflicts (below). That alone is not fatal — the same happened for release/2.36 and release/2.37, where the job still pushed a placeholder branch and opened a PR for manual resolution.

What actually failed the job was the push:

! [remote rejected] backport/28397-to-2.34 -> backport/28397-to-2.34
  (Unable to determine if workflow can be created or updated due to timeout; `workflows` scope may be required.)

This cherry-pick touches no files under .github/workflows/, so this is GitHub timing out while determining scope rather than a genuine permission gap. The push was retried by hand for this PR and succeeded unchanged. The net effect of the failure was that no branch and no PR were created for 2.34 at all, so this one is opened from scratch rather than fixed up in place. The release/2.35 job (#28644) failed the same way.

Manual resolution: 9 golden files dropped

release/2.34 already carries #28606 (the 2.34 backport of #28340), so the notifier.go hunk applies cleanly. What remains is a genuine modify/delete conflict on nine golden files:

TemplateAIBudgetLimitReachedUser.html.golden
TemplateAIBudgetWarningUser.html.golden
TemplateUserAccountActivatedServiceAccount.html.golden
TemplateUserAccountCreatedServiceAccount.html.golden
TemplateUserAccountCreatedWithoutAccountType.html.golden
TemplateUserAccountDeletedServiceAccount.html.golden
TemplateUserAccountSuspendedServiceAccount.html.golden
TemplateWorkspaceAutostopReminder.html.golden
TemplateWorkspaceDormant_NoAutoDelete.html.golden

These are fixtures for notification templates and test cases that postdate the 2.34 branch point — AI budgets, service accounts, the autostop reminder, and the no-auto-delete variant of the dormancy notice. coderd/notifications/ on release/2.34 contains no reference to AIBudget, BudgetLimitReached, BudgetWarning, ServiceAccount, AccountType, AutostopReminder or NoAutoDelete, so there is no template to render them and no test case that reads them. All nine were removed rather than added; carrying them over would leave orphan fixtures.

Note that TemplateWorkspaceDormant.html.golden does exist on 2.34 and is updated normally by this PR. Only the _NoAutoDelete variant of that test case is dropped.

This is the only deviation from the original PR. Verified with a diff-of-diffs: excluding those nine paths, this commit matches #28397 line for line. The only textual differences are blob hashes and hunk line offsets — notifier.go sits one line higher on this branch, so the hunk header reads @@ -252 instead of @@ -253. The escaping changes to html.gotmpl, notifier.go and smtp_internal_test.go are fully intact.

Net: 30 files, +226/−71 (upstream: 39 files, +244/−89 — the delta is exactly the nine goldens). The smtp golden directory holds 36 files before and after, so nothing was added or lost.

Verification

  • go vet ./coderd/notifications/... — clean
  • go test ./coderd/notifications/dispatch/... — pass, including the three new TestSMTPHTMLTemplateEscapes* tests
  • go test ./coderd/notifications/ -run TestNotificationTemplates_Golden — pass across all affected goldens

Related backports

)

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 = &lt;a href="https://attacker.example/login"&gt;Re-authenticate now&lt;/a&gt;

  -> <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
`&#34;` and `'` to `&#39;` 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 `&amp;`. 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

(cherry picked from commit 2236710)
@BobbyHo BobbyHo added the backport/v2.34 Backport PR targeting release/2.34 label Aug 26, 2026
@BobbyHo BobbyHo self-assigned this Aug 26, 2026
@mtojek
mtojek merged commit e70e9b1 into release/2.34 Aug 26, 2026
56 checks passed
@mtojek
mtojek deleted the backport/28397-to-2.34 branch August 26, 2026 19:32
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/v2.34 Backport PR targeting release/2.34

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants