From 7729a3dcad2a512304dfd270a4b4092a5321a649 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 15 Nov 2025 21:37:09 +0100 Subject: [PATCH] fix: HTML entity double-escaping in email notifications (#1829) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: kolaente <13721712+kolaente@users.noreply.github.com> --- pkg/notifications/mail_render.go | 2 +- pkg/notifications/mail_test.go | 186 +++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 1 deletion(-) diff --git a/pkg/notifications/mail_render.go b/pkg/notifications/mail_render.go index 0c483f0e2..c356e60e7 100644 --- a/pkg/notifications/mail_render.go +++ b/pkg/notifications/mail_render.go @@ -116,7 +116,7 @@ func convertLinesToHTML(lines []*mailLine) (linesHTML []templatehtml.HTML, err e continue } - md := []byte(templatehtml.HTMLEscapeString(line.Text)) + md := []byte(line.Text) var buf bytes.Buffer err = goldmark.Convert(md, &buf) if err != nil { diff --git a/pkg/notifications/mail_test.go b/pkg/notifications/mail_test.go index d6578f023..9858d7a8d 100644 --- a/pkg/notifications/mail_test.go +++ b/pkg/notifications/mail_test.go @@ -422,4 +422,190 @@ This is a footer line assert.Equal(t, mail.to, mailopts.To) assert.Equal(t, "", mailopts.ThreadID) }) + t.Run("with special characters in task title", func(t *testing.T) { + mail := NewMail(). + From("test@example.com"). + To("test@otherdomain.com"). + Subject("Testmail"). + Greeting("Hi there,"). + Line(`This is a friendly reminder of the task "Fix structured data Value in property "reviewCount" must be positive" (My Project).`) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + assert.Equal(t, mail.from, mailopts.From) + assert.Equal(t, mail.to, mailopts.To) + + // Plain text should keep quotes as-is + assert.Contains(t, mailopts.Message, `"Fix structured data Value in property "reviewCount" must be positive"`) + + // HTML should have proper HTML entities for quotes + // " is the correct HTML entity for the quote character and will render as " in the browser + assert.Contains(t, mailopts.HTMLMessage, `"Fix structured data Value in property "reviewCount" must be positive"`) + }) + t.Run("with pre-escaped HTML entities", func(t *testing.T) { + // This tests the fix for issue #1664 where HTML entities were being double-escaped + mail := NewMail(). + From("test@example.com"). + To("test@otherdomain.com"). + Subject("Testmail"). + Greeting("Hi there,"). + Line(`Task with entity: "already escaped" should render correctly`) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // Plain text should contain the HTML entity as-is (it will be interpreted by email client) + assert.Contains(t, mailopts.Message, `"`) + + // HTML should properly handle the pre-escaped entity without double-escaping + // The entity should remain as " (not become &#34;) + assert.Contains(t, mailopts.HTMLMessage, `"already escaped"`) + // Should NOT double-escape to &#34; which would display as literal " + assert.NotContains(t, mailopts.HTMLMessage, `&#34;`) + }) + t.Run("with XSS attempt via script tag", func(t *testing.T) { + mail := NewMail(). + From("test@example.com"). + To("test@otherdomain.com"). + Subject("Testmail"). + Greeting("Hi there,"). + Line(`Task: `) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // Script tags should be stripped by bluemonday sanitization + assert.NotContains(t, mailopts.HTMLMessage, ``) + assert.NotContains(t, mailopts.HTMLMessage, `alert('XSS')`) + // The text should be present but sanitized + assert.Contains(t, mailopts.HTMLMessage, `Task:`) + }) + t.Run("with XSS attempt via img onerror", func(t *testing.T) { + mail := NewMail(). + From("test@example.com"). + To("test@otherdomain.com"). + Subject("Testmail"). + Greeting("Hi there,"). + Line(`Task: `) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // The dangerous HTML should be escaped, not rendered as actual HTML + // This makes it safe - it will display as text, not execute + assert.Contains(t, mailopts.HTMLMessage, `<img`) + assert.Contains(t, mailopts.HTMLMessage, `>`) + // Verify it's not an actual executable img tag + assert.NotContains(t, mailopts.HTMLMessage, `Click me`) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // JavaScript protocol should be stripped + assert.NotContains(t, mailopts.HTMLMessage, `javascript:alert`) + assert.NotContains(t, mailopts.HTMLMessage, `href="javascript:`) + // Text content should remain + assert.Contains(t, mailopts.HTMLMessage, `Task:`) + }) + t.Run("with XSS attempt via iframe", func(t *testing.T) { + mail := NewMail(). + From("test@example.com"). + To("test@otherdomain.com"). + Subject("Testmail"). + Greeting("Hi there,"). + Line(`Task: `) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // Iframes should be completely stripped by bluemonday + assert.NotContains(t, mailopts.HTMLMessage, `Dangerous`) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // onclick handler should be stripped + assert.NotContains(t, mailopts.HTMLMessage, `onclick=`) + assert.NotContains(t, mailopts.HTMLMessage, `onclick="alert`) + // Text content may remain but without the dangerous attributes + assert.Contains(t, mailopts.HTMLMessage, `Task:`) + }) + t.Run("with XSS attempt via data URI", func(t *testing.T) { + mail := NewMail(). + From("test@example.com"). + To("test@otherdomain.com"). + Subject("Testmail"). + Greeting("Hi there,"). + Line(`Task: `) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // Script tags should not appear in final HTML + assert.NotContains(t, mailopts.HTMLMessage, ``) + assert.NotContains(t, mailopts.HTMLMessage, ` priority & needs **attention**`) + + mailopts, err := RenderMail(mail, "en") + require.NoError(t, err) + + // Malicious content should be stripped + assert.NotContains(t, mailopts.HTMLMessage, `