fix(notifications): render Markdown in plain-text emails (#3219)

2026-07-21 17:37:54 -05:00 · 2026-07-18 14:39:32 +02:00
parent 4d8b7255e7
commit 6b9a879574
3 changed files with 275 additions and 18 deletions
@@ -17,11 +17,14 @@
 package notifications

 import (
+	"bufio"
 	"bytes"
 	"embed"
+	"html"
 	templatehtml "html/template"
 	"net/url"
 	"regexp"
+	"strconv"
 	"strings"
 	templatetext "text/template"

@@ -32,6 +35,9 @@ import (

 	"github.com/microcosm-cc/bluemonday"
 	"github.com/yuin/goldmark"
+	"github.com/yuin/goldmark/ast"
+	goldmarkhtml "github.com/yuin/goldmark/renderer/html"
+	"github.com/yuin/goldmark/text"
 )

 const mailTemplatePlain = `
@@ -290,12 +296,183 @@ func ensurePMargins(html string) string {
 	return rePTag.ReplaceAllString(html, "<p "+pMarginStyle+">")
 }

-// convertLinesToPlain converts mail lines to plain text, stripping HTML from lines marked as HTML.
+var markdownTextWriter = goldmarkhtml.NewWriter()
+
+func markdownToPlainText(markdown string) string {
+	source := []byte(markdown)
+	document := goldmark.DefaultParser().Parse(text.NewReader(source))
+	var plain strings.Builder
+	linkStarts := make(map[ast.Node]int)
+	listItemIndents := make([]int, 0)
+	listItemHasBlocks := make([]bool, 0)
+
+	_ = ast.Walk(document, func(node ast.Node, entering bool) (ast.WalkStatus, error) {
+		switch n := node.(type) {
+		case *ast.Text:
+			if !entering {
+				return ast.WalkContinue, nil
+			}
+			writeMarkdownText(&plain, n.Value(source), n.IsRaw())
+			if n.SoftLineBreak() || n.HardLineBreak() {
+				plain.WriteByte('\n')
+				if len(listItemIndents) > 0 {
+					plain.WriteString(strings.Repeat(" ", listItemIndents[len(listItemIndents)-1]))
+				}
+			}
+		case *ast.String:
+			if entering {
+				writeMarkdownText(&plain, n.Value, n.IsRaw() || n.IsCode())
+			}
+		case *ast.AutoLink:
+			if entering {
+				plain.Write(n.Label(source))
+			}
+		case *ast.Link:
+			if entering {
+				linkStarts[node] = plain.Len()
+				return ast.WalkContinue, nil
+			}
+			start := linkStarts[node]
+			label := plain.String()[start:]
+			var normalized strings.Builder
+			writeMarkdownText(&normalized, n.Destination, false)
+			destination := normalized.String()
+			if destination != "" && label != destination {
+				plain.WriteString(" (")
+				plain.WriteString(destination)
+				plain.WriteByte(')')
+			}
+			delete(linkStarts, node)
+		case *ast.Image:
+			if !entering {
+				if len(n.Destination) > 0 {
+					plain.WriteString(" (")
+					writeMarkdownText(&plain, n.Destination, false)
+					plain.WriteByte(')')
+				}
+			}
+		case *ast.ListItem:
+			if entering {
+				if len(listItemHasBlocks) > 0 {
+					listItemHasBlocks[len(listItemHasBlocks)-1] = true
+				}
+				listItemIndents = append(listItemIndents, writePlainListItem(&plain, n))
+				listItemHasBlocks = append(listItemHasBlocks, false)
+			} else {
+				listItemIndents = listItemIndents[:len(listItemIndents)-1]
+				listItemHasBlocks = listItemHasBlocks[:len(listItemHasBlocks)-1]
+				writePlainNewline(&plain)
+			}
+		case *ast.Paragraph, *ast.Heading:
+			if entering {
+				writePlainListBlockStart(&plain, listItemIndents, listItemHasBlocks)
+			} else {
+				writePlainNewline(&plain)
+			}
+		case *ast.CodeBlock, *ast.FencedCodeBlock:
+			if entering {
+				writePlainListBlockStart(&plain, listItemIndents, listItemHasBlocks)
+				writePlainBlock(&plain, node.Lines().Value(source), listItemIndents)
+				writePlainNewline(&plain)
+				return ast.WalkSkipChildren, nil
+			}
+		case *ast.ThematicBreak:
+			if entering {
+				writePlainListBlockStart(&plain, listItemIndents, listItemHasBlocks)
+				plain.WriteString("---\n")
+			}
+		case *ast.RawHTML, *ast.HTMLBlock:
+			if entering {
+				return ast.WalkSkipChildren, nil
+			}
+		}
+
+		return ast.WalkContinue, nil
+	})
+
+	return strings.TrimSpace(plain.String())
+}
+
+func writePlainListItem(plain *strings.Builder, item *ast.ListItem) int {
+	writePlainNewline(plain)
+	prefixStart := plain.Len()
+	list := item.Parent().(*ast.List)
+	depth := 0
+	for parent := list.Parent(); parent != nil; parent = parent.Parent() {
+		if _, nested := parent.(*ast.List); nested {
+			depth++
+		}
+	}
+	plain.WriteString(strings.Repeat("  ", depth))
+
+	if list.IsOrdered() {
+		position := list.Start
+		for sibling := item.PreviousSibling(); sibling != nil; sibling = sibling.PreviousSibling() {
+			position++
+		}
+		plain.WriteString(strconv.Itoa(position))
+		plain.WriteString(". ")
+	} else {
+		plain.WriteString("- ")
+	}
+
+	return plain.Len() - prefixStart
+}
+
+func writePlainListBlockStart(plain *strings.Builder, indents []int, hasBlocks []bool) {
+	if len(hasBlocks) == 0 {
+		return
+	}
+
+	current := len(hasBlocks) - 1
+	if hasBlocks[current] {
+		writePlainNewline(plain)
+		plain.WriteString(strings.Repeat(" ", indents[current]))
+	}
+	hasBlocks[current] = true
+}
+
+func writePlainBlock(plain *strings.Builder, value []byte, indents []int) {
+	indent := 0
+	if len(indents) > 0 {
+		indent = indents[len(indents)-1]
+	}
+
+	for i, char := range value {
+		plain.WriteByte(char)
+		if char == '\n' && i < len(value)-1 {
+			plain.WriteString(strings.Repeat(" ", indent))
+		}
+	}
+}
+
+func writeMarkdownText(plain *strings.Builder, value []byte, raw bool) {
+	if raw {
+		plain.Write(value)
+		return
+	}
+
+	var escaped bytes.Buffer
+	writer := bufio.NewWriter(&escaped)
+	markdownTextWriter.Write(writer, value)
+	_ = writer.Flush()
+	plain.WriteString(html.UnescapeString(escaped.String()))
+}
+
+func writePlainNewline(plain *strings.Builder) {
+	if plain.Len() == 0 || plain.String()[plain.Len()-1] != '\n' {
+		plain.WriteByte('\n')
+	}
+}
+
 func convertLinesToPlain(lines []*mailLine) []*mailLine {
 	plain := make([]*mailLine, 0, len(lines))
 	for _, line := range lines {
 		if !line.isHTML {
-			plain = append(plain, line)
+			text := markdownToPlainText(line.Text)
+			if text != "" {
+				plain = append(plain, &mailLine{Text: text})
+			}
 			continue
 		}

@@ -352,20 +529,15 @@ func RenderMail(m *Mail, lang string) (mailOpts *mail.Opts, err error) {
 	data := make(map[string]interface{})

 	data["Greeting"] = m.greeting
-	if m.conversational {
-		data["IntroLines"] = convertLinesToPlain(m.introLines)
-		data["OutroLines"] = convertLinesToPlain(m.outroLines)
-		if m.headerLine != nil {
-			plainHeaders := convertLinesToPlain([]*mailLine{m.headerLine})
-			if len(plainHeaders) > 0 {
-				data["HeaderLinePlain"] = plainHeaders[0].Text
-			}
+	data["IntroLines"] = convertLinesToPlain(m.introLines)
+	data["OutroLines"] = convertLinesToPlain(m.outroLines)
+	if m.conversational && m.headerLine != nil {
+		plainHeaders := convertLinesToPlain([]*mailLine{m.headerLine})
+		if len(plainHeaders) > 0 {
+			data["HeaderLinePlain"] = plainHeaders[0].Text
 		}
-	} else {
-		data["IntroLines"] = m.introLines
-		data["OutroLines"] = m.outroLines
 	}
-	data["FooterLines"] = m.footerLines
+	data["FooterLines"] = convertLinesToPlain(m.footerLines)
 	data["ActionText"] = m.actionText
 	data["ActionURL"] = m.actionURL
 	data["Boundary"] = boundary
@@ -20,6 +20,7 @@ import (
 	"strings"
 	"testing"

+	"code.vikunja.io/api/pkg/config"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -163,7 +164,7 @@ Hi there,

 This is a line

-This **line** contains [a link](https://vikunja.io)
+This line contains a link (https://vikunja.io)

 And another one

@@ -230,6 +231,17 @@ This is a footer line
 		// Verify no action button
 		assert.NotContains(t, mailopts.HTMLMessage, `class="email-button"`)
 	})
+	t.Run("with link to notification settings in footer", func(t *testing.T) {
+		originalPublicURL := config.ServicePublicURL.GetString()
+		t.Cleanup(func() { config.ServicePublicURL.Set(originalPublicURL) })
+		config.ServicePublicURL.Set("https://vikunja.example.com/")
+
+		mailopts, err := RenderMail(NewMail().IncludeLinkToSettings("en"), "en")
+		require.NoError(t, err)
+
+		assert.Contains(t, mailopts.Message, "here (https://vikunja.example.com/user/settings/general)")
+		assert.Contains(t, mailopts.HTMLMessage, `<a href="https://vikunja.example.com/user/settings/general" rel="nofollow">here</a>`)
+	})
 	t.Run("with footer and action", func(t *testing.T) {
 		mail := NewMail().
 			From("test@example.com").
@@ -254,7 +266,7 @@ Hi there,

 This is a line

-This **line** contains [a link](https://vikunja.io)
+This line contains a link (https://vikunja.io)

 And another one

@@ -314,6 +326,26 @@ This is a footer line
 		// &#34; is the correct HTML entity for the quote character and will render as " in the browser
 		assert.Contains(t, mailopts.HTMLMessage, `&#34;Fix structured data Value in property &#34;reviewCount&#34; must be positive&#34;`)
 	})
+	t.Run("renders markdown as plain text", func(t *testing.T) {
+		title := EscapeMarkdown(`Test - xyz - 123|456@789! Keep \!`)
+		mail := NewMail().
+			From("test@example.com").
+			To("test@otherdomain.com").
+			Subject("Testmail").
+			Greeting("Hi there,").
+			Line(`A **friendly** reminder for "` + title + `". See [the task](https://vikunja.io/tasks/1).`).
+			Line(`* [` + title + `](https://vikunja.io/tasks/1)`)
+
+		mailopts, err := RenderMail(mail, "en")
+		require.NoError(t, err)
+
+		assert.Contains(t, mailopts.Message, `A friendly reminder for "Test - xyz - 123|456@789! Keep \!". See the task (https://vikunja.io/tasks/1).`)
+		assert.NotContains(t, mailopts.Message, `\-`)
+		assert.NotContains(t, mailopts.Message, `**`)
+		assert.Contains(t, mailopts.Message, `- Test - xyz - 123|456@789! Keep \! (https://vikunja.io/tasks/1)`)
+		assert.Contains(t, mailopts.HTMLMessage, `<strong>friendly</strong>`)
+		assert.Contains(t, mailopts.HTMLMessage, `Test - xyz - 123|456@789! Keep \!`)
+	})
 	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().
@@ -326,8 +358,7 @@ This is a footer line
 		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, `&#34;`)
+		assert.Contains(t, mailopts.Message, `"already escaped"`)

 		// HTML should properly handle the pre-escaped entity without double-escaping
 		// The entity should remain as &#34; (not become &amp;#34;)
@@ -99,3 +99,57 @@ func TestEscapeMarkdown_RoundTripThroughGoldmark(t *testing.T) {
 		})
 	}
 }
+
+func TestMarkdownToPlainText_RoundTripsEscapedText(t *testing.T) {
+	inputs := []string{
+		"",
+		"plain title",
+		`Test - xyz - 123|456@789!`,
+		`a\b`,
+		"`code`",
+		"*bold*",
+		"test](https://evil.com) [Click to verify",
+		"![](https://evil.com/track.png)",
+		"<https://evil.com>",
+		`mixed \ and - and | and !`,
+		"#+.{}()[]<>~_",
+	}
+
+	for _, input := range inputs {
+		t.Run(input, func(t *testing.T) {
+			assert.Equal(t, input, markdownToPlainText(EscapeMarkdown(input)))
+		})
+	}
+}
+
+func TestMarkdownToPlainText_NormalizesDestinations(t *testing.T) {
+	assert.Equal(t,
+		`site (https://e.test/a(b)?x=1&y=2)`,
+		markdownToPlainText(`[site](https://e.test/a\(b\)?x=1&amp;y=2)`),
+	)
+	assert.Equal(t,
+		`chart (https://e.test/a(b)?x=1&y=2)`,
+		markdownToPlainText(`![chart](https://e.test/a\(b\)?x=1&amp;y=2)`),
+	)
+}
+
+func TestMarkdownToPlainText_PreservesListNumberingAndNesting(t *testing.T) {
+	markdown := "3. Third\n   - Nested one\n   - Nested two\n4. Fourth"
+	want := "3. Third\n  - Nested one\n  - Nested two\n4. Fourth"
+
+	assert.Equal(t, want, markdownToPlainText(markdown))
+}
+
+func TestMarkdownToPlainText_AlignsListContinuationLines(t *testing.T) {
+	markdown := "3. Parent\n   - [Nested first\n     nested second](https://example.com)\n4. Fourth"
+	want := "3. Parent\n  - Nested first\n    nested second (https://example.com)\n4. Fourth"
+
+	assert.Equal(t, want, markdownToPlainText(markdown))
+}
+
+func TestMarkdownToPlainText_AlignsListContinuationBlocks(t *testing.T) {
+	markdown := "- first\n\n  second\n\n  ```\n  code one\n  code two\n  ```\n- next"
+	want := "- first\n  second\n  code one\n  code two\n- next"
+
+	assert.Equal(t, want, markdownToPlainText(markdown))
+}