{{ message }}
fix(coderd/notifications): HTML-escape the email template values (#28397) - #28646
Merged
Conversation
) 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 (cherry picked from commit 2236710)
mtojek
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

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.34failed: https://github.com/coder/coder/actions/runs/32985803559/job/98231735640Why 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/deleteconflicts (below). That alone is not fatal — the same happened forrelease/2.36andrelease/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:
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. Therelease/2.35job (#28644) failed the same way.Manual resolution: 9 golden files dropped
release/2.34already carries #28606 (the 2.34 backport of #28340), so thenotifier.gohunk applies cleanly. What remains is a genuinemodify/deleteconflict on nine golden files: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/onrelease/2.34contains no reference toAIBudget,BudgetLimitReached,BudgetWarning,ServiceAccount,AccountType,AutostopReminderorNoAutoDelete, 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.goldendoes exist on 2.34 and is updated normally by this PR. Only the_NoAutoDeletevariant 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.gosits one line higher on this branch, so the hunk header reads@@ -252instead of@@ -253. The escaping changes tohtml.gotmpl,notifier.goandsmtp_internal_test.goare 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/...— cleango test ./coderd/notifications/dispatch/...— pass, including the three newTestSMTPHTMLTemplateEscapes*testsgo test ./coderd/notifications/ -run TestNotificationTemplates_Golden— pass across all affected goldensRelated backports
release/2.37release/2.36release/2.35release/2.29(still needs manual resolution)