From 07f79af65b228bb560ac51cea7f97ae81223fe44 Mon Sep 17 00:00:00 2001 From: Jakub Domeracki Date: Mon, 10 Aug 2026 10:42:31 +0200 Subject: [PATCH] fix: markdown rendering improvements 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 --- coderd/notifications/dispatch/smtp.go | 2 +- .../notifications/dispatch/smtp/html.gotmpl | 10 +- .../dispatch/smtp_internal_test.go | 350 ++++++++++++++++++ coderd/notifications/notifier.go | 13 +- coderd/notifications/render/gotmpl.go | 64 ++++ coderd/notifications/render/sanitize_test.go | 333 +++++++++++++++++ .../smtp/TemplateTaskCompleted.html.golden | 4 +- .../smtp/TemplateTaskFailed.html.golden | 4 +- .../smtp/TemplateTaskIdle.html.golden | 4 +- .../smtp/TemplateTaskPaused.html.golden | 4 +- .../smtp/TemplateTaskResumed.html.golden | 4 +- .../smtp/TemplateTaskWorking.html.golden | 4 +- .../smtp/TemplateTemplateDeleted.html.golden | 4 +- .../TemplateTemplateDeprecated.html.golden | 4 +- .../TemplateUserAccountActivated.html.golden | 4 +- .../TemplateUserAccountCreated.html.golden | 4 +- .../TemplateUserAccountDeleted.html.golden | 4 +- .../TemplateUserAccountSuspended.html.golden | 4 +- ...teUserRequestedOneTimePasscode.html.golden | 8 +- .../TemplateWorkspaceAutoUpdated.html.golden | 5 +- ...mplateWorkspaceAutobuildFailed.html.golden | 4 +- ...plateWorkspaceAutostopReminder.html.golden | 4 +- .../smtp/TemplateWorkspaceCreated.html.golden | 4 +- .../smtp/TemplateWorkspaceDeleted.html.golden | 4 +- ...kspaceDeleted_CustomAppearance.html.golden | 4 +- .../smtp/TemplateWorkspaceDormant.html.golden | 4 +- ...eWorkspaceDormant_NoAutoDelete.html.golden | 4 +- ...lateWorkspaceManualBuildFailed.html.golden | 4 +- ...mplateWorkspaceManuallyUpdated.html.golden | 5 +- ...lateWorkspaceMarkedForDeletion.html.golden | 4 +- .../TemplateWorkspaceOutOfDisk.html.golden | 5 +- ...spaceOutOfDisk_MultipleVolumes.html.golden | 5 +- .../TemplateWorkspaceOutOfMemory.html.golden | 5 +- .../TemplateYourAccountActivated.html.golden | 4 +- .../TemplateYourAccountSuspended.html.golden | 4 +- coderd/render/markdown.go | 23 +- coderd/render/markdown_test.go | 112 ++++++ 37 files changed, 962 insertions(+), 70 deletions(-) create mode 100644 coderd/notifications/render/sanitize_test.go diff --git a/coderd/notifications/dispatch/smtp.go b/coderd/notifications/dispatch/smtp.go index 5dfcc43851..7f86857fe3 100644 --- a/coderd/notifications/dispatch/smtp.go +++ b/coderd/notifications/dispatch/smtp.go @@ -66,7 +66,7 @@ func (s *SMTPHandler) Dispatcher(payload types.MessagePayload, titleTmpl, bodyTm return nil, xerrors.Errorf("render subject: %w", err) } - htmlBody := markdown.HTMLFromMarkdown(bodyTmpl) + htmlBody := markdown.HTMLFromMarkdownSafe(bodyTmpl) plainBody, err := markdown.PlaintextFromMarkdown(bodyTmpl) if err != nil { return nil, xerrors.Errorf("render plaintext body: %w", err) diff --git a/coderd/notifications/dispatch/smtp/html.gotmpl b/coderd/notifications/dispatch/smtp/html.gotmpl index cecba560af..770696e870 100644 --- a/coderd/notifications/dispatch/smtp/html.gotmpl +++ b/coderd/notifications/dispatch/smtp/html.gotmpl @@ -3,7 +3,7 @@ - {{ .Labels._subject }} + {{ .Labels._subject | html }}
@@ -11,16 +11,16 @@ {{ app_name | html }} Logo

- {{ .Labels._subject }} + {{ .Labels._subject | html }}

-

Hi {{ .UserName }},

+

Hi {{ .UserName | html }},

{{ .Labels._body }}
{{ range $action := .Actions }} - - {{ $action.Label }} + + {{ $action.Label | html }} {{ end }}
diff --git a/coderd/notifications/dispatch/smtp_internal_test.go b/coderd/notifications/dispatch/smtp_internal_test.go index 2e7dff8cbe..5aee2385c2 100644 --- a/coderd/notifications/dispatch/smtp_internal_test.go +++ b/coderd/notifications/dispatch/smtp_internal_test.go @@ -7,6 +7,8 @@ import ( "github.com/stretchr/testify/require" + markdown "github.com/coder/coder/v2/coderd/render" + "github.com/coder/coder/v2/coderd/notifications/render" "github.com/coder/coder/v2/coderd/notifications/types" ) @@ -43,6 +45,354 @@ func TestSMTPHTMLTemplateEscapesAppearanceHelpers(t *testing.T) { require.False(t, strings.Contains(got, logoURL), "raw logo URL must not be rendered") } +// TestSMTPHTMLTemplateMarkdownInjection is an end-to-end regression test that +// exercises the full notification email rendering pipeline with malicious input. +// It simulates the complete flow: SanitizePayload -> GoTemplate (body) -> +// HTMLFromMarkdownSafe -> GoTemplate (outer HTML template) and asserts that the +// final HTML email output contains no attacker-controlled links, headings, images, +// scripts, or other injected content. +// +// If this test fails, it likely means a change to the rendering pipeline has +// introduced a Markdown or HTML injection vulnerability in notification emails. +func TestSMTPHTMLTemplateMarkdownInjection(t *testing.T) { + t.Parallel() + + helpers := map[string]any{ + "base_url": func() string { return "https://coder.example.com" }, + "current_year": func() string { return "2026" }, + "logo_url": func() string { return "https://coder.example.com/logo.png" }, + "app_name": func() string { return "Coder" }, + } + + tests := []struct { + name string + // payload is the unsanitized notification payload (as it would arrive + // from the database before SanitizePayload runs). + payload types.MessagePayload + // bodyTemplate is the notification body template (stored in the DB). + bodyTemplate string + // absentInHTML lists substrings that must NOT appear in the final HTML + // email output. + absentInHTML []string + // presentInHTML lists substrings that MUST appear in the final HTML. + presentInHTML []string + }{ + { + name: "markdown link injection via display name", + payload: types.MessagePayload{ + NotificationTemplateID: "00000000-0000-0000-0000-000000000000", + UserName: "Eve\n[Re-authenticate](https://evil.example/login)", + Labels: map[string]string{ + "created_account_user_name": "Eve\n[Re-authenticate](https://evil.example/login)", + "initiator": "admin", + }, + }, + bodyTemplate: `Account created for **{{.Labels.created_account_user_name}}** by **{{.Labels.initiator}}**.`, + absentInHTML: []string{ + `admin", + }, + }, + { + name: "heading injection via display name", + payload: types.MessagePayload{ + NotificationTemplateID: "00000000-0000-0000-0000-000000000000", + UserName: "Eve\n## URGENT SECURITY ALERT", + Labels: map[string]string{ + "suspended_account_user_name": "Eve\n## URGENT SECURITY ALERT", + "initiator": "admin", + }, + }, + bodyTemplate: `Account **{{.Labels.suspended_account_user_name}}** suspended by **{{.Labels.initiator}}**.`, + absentInHTML: []string{ + "

", + "

", + "

", + }, + presentInHTML: []string{ + "## URGENT SECURITY ALERT", // rendered as literal text + }, + }, + { + name: "image tracking pixel injection", + payload: types.MessagePayload{ + NotificationTemplateID: "00000000-0000-0000-0000-000000000000", + UserName: "![](https://evil.example/track.gif)", + Labels: map[string]string{ + "name": "![](https://evil.example/track.gif)", + }, + }, + bodyTemplate: `Workspace **{{.Labels.name}}** deleted.`, + absentInHTML: []string{ + `src="https://evil.example`, // no attacker-controlled image source + }, + presentInHTML: []string{ + "![]", // rendered as literal text + }, + }, + { + name: "javascript URI via markdown link", + payload: types.MessagePayload{ + NotificationTemplateID: "00000000-0000-0000-0000-000000000000", + UserName: "normal", + Labels: map[string]string{ + "name": "[xss](javascript:alert(document.cookie))", + }, + }, + bodyTemplate: `Workspace **{{.Labels.name}}** updated.`, + absentInHTML: []string{ + `href="javascript:`, // no javascript link + }, + }, + { + name: "raw HTML injection in display name", + payload: types.MessagePayload{ + NotificationTemplateID: "00000000-0000-0000-0000-000000000000", + UserName: ``, + Labels: map[string]string{}, + }, + bodyTemplate: `Hello.`, + absentInHTML: []string{ + ")", + absent: []string{"data:", "bold", "italic"}, + }, + { + name: "raw HTML stripped", + input: ``, + absent: []string{"