" path, and
+// a relative path in an email is simply a broken image — mail clients have no
+// origin to resolve it against.
+//
+// Anything that is neither absolute-http(s) nor site-relative returns "",
+// which the templates render as "no logo" rather than as a broken image. That
+// also means a `javascript:` or `data:` value stored by a hostile admin can
+// never reach an ![]()
.
+func (f *TemplateFormatter) absoluteURL(value any) string {
+ raw := strings.TrimSpace(stringify(value))
+ if raw == "" {
+ return ""
+ }
+
+ if strings.HasPrefix(raw, "https://") || strings.HasPrefix(raw, "http://") {
+ return raw
}
- return &TemplateFormatter{
- funcMap: funcMap,
- }, nil
+ base := f.base()
+ if !strings.HasPrefix(raw, "/") || base == "" {
+ return ""
+ }
+
+ return base + raw
+}
+
+// stringify renders the handful of shapes a view-model value can arrive as
+// (string, *string, nil) as a plain string. Anything else yields "".
+func stringify(value any) string {
+ switch typed := value.(type) {
+ case nil:
+ return ""
+ case string:
+ return typed
+ case *string:
+ if typed == nil {
+ return ""
+ }
+
+ return *typed
+ case fmt.Stringer:
+ return typed.String()
+ default:
+ return ""
+ }
+}
+
+// field looks a key up on a view model that may be either a map or a struct,
+// returning nil when it is absent.
+//
+// This exists to dodge the trap documented in supportreply.go: html/template
+// ERRORS on a missing struct field (while a missing map key is merely nil), and
+// the repo's view models are a mix of map[string]any and structs
+// (uptimereport.Data). A new `{{.OrgLogoURL}}` in base.html would therefore
+// break every struct-backed template until the field was added to each one —
+// silently, at send time. Every branding key base.html reads goes through this
+// helper instead, so a view model that does not carry it renders empty.
+func field(data any, name string) any {
+ if data == nil {
+ return nil
+ }
+
+ value := reflect.ValueOf(data)
+ for value.Kind() == reflect.Pointer || value.Kind() == reflect.Interface {
+ if value.IsNil() {
+ return nil
+ }
+
+ value = value.Elem()
+ }
+
+ // An if-chain rather than a switch on reflect.Kind: only two of the ~27
+ // kinds are meaningful here, and enumerating the rest to satisfy an
+ // exhaustiveness check would be noise.
+ if value.Kind() == reflect.Map {
+ if value.Type().Key().Kind() != reflect.String {
+ return nil
+ }
+
+ found := value.MapIndex(reflect.ValueOf(name).Convert(value.Type().Key()))
+ if !found.IsValid() {
+ return nil
+ }
+
+ return found.Interface()
+ }
+
+ if value.Kind() == reflect.Struct {
+ found := value.FieldByName(name)
+ if !found.IsValid() || !found.CanInterface() {
+ return nil
+ }
+
+ return found.Interface()
+ }
+
+ return nil
}
// dict builds a map[string]any from an alternating key/value argument list,
@@ -92,6 +260,40 @@ func (f *TemplateFormatter) parseTemplate(templateName string) (*template.Templa
return tmpl, nil
}
+// parseTextTemplate parses the same two files as parseTemplate, but with
+// text/template rather than html/template.
+//
+// The subject and the plaintext alternative are NOT HTML, and rendering them
+// through html/template escapes every value it interpolates: a check named
+// "Search & Discovery" arrives as "Search & Discovery" — in the SUBJECT
+// LINE, where the reader sees it before opening anything, and again in the
+// plaintext part that text-only clients and screen readers display. Literal
+// template text is untouched by the escaper, which is why this went unnoticed:
+// it only corrupts values, and only those carrying & < > " or '.
+func (f *TemplateFormatter) parseTextTemplate(templateName string) (*texttemplate.Template, error) {
+ baseContent, err := templateFS.ReadFile("templates/base.html")
+ if err != nil {
+ return nil, fmt.Errorf("reading base template: %w", err)
+ }
+
+ templateContent, err := templateFS.ReadFile("templates/" + templateName)
+ if err != nil {
+ return nil, fmt.Errorf("reading template %s: %w", templateName, err)
+ }
+
+ tmpl, err := texttemplate.New("base.html").Funcs(f.funcMap).Parse(string(baseContent))
+ if err != nil {
+ return nil, fmt.Errorf("parsing base template: %w", err)
+ }
+
+ tmpl, err = tmpl.New(templateName).Parse(string(templateContent))
+ if err != nil {
+ return nil, fmt.Errorf("parsing template %s: %w", templateName, err)
+ }
+
+ return tmpl, nil
+}
+
// Format renders a template with the given data and returns the rendered
// subject (from a {{define "subject"}} block, or "" when the template has
// none), the HTML body with inlined CSS, and a plaintext alternative (from a
@@ -103,12 +305,17 @@ func (f *TemplateFormatter) Format(templateName string, data any) (string, strin
return "", "", "", fmt.Errorf("parsing template %s: %w", templateName, err)
}
- subject, err := f.renderSubject(tmpl, templateName, data)
+ textTmpl, err := f.parseTextTemplate(templateName)
+ if err != nil {
+ return "", "", "", fmt.Errorf("parsing template %s: %w", templateName, err)
+ }
+
+ subject, err := f.renderSubject(textTmpl, templateName, data)
if err != nil {
return "", "", "", err
}
- text, err := f.renderText(tmpl, templateName, data)
+ text, err := f.renderText(textTmpl, templateName, data)
if err != nil {
return "", "", "", err
}
@@ -135,7 +342,7 @@ func (f *TemplateFormatter) Format(templateName string, data any) (string, strin
// Returns "" when no subject block is defined — callers may then fall back
// to a static subject.
func (f *TemplateFormatter) renderSubject(
- tmpl *template.Template, templateName string, data any,
+ tmpl *texttemplate.Template, templateName string, data any,
) (string, error) {
subjTmpl := tmpl.Lookup("subject")
if subjTmpl == nil {
@@ -154,7 +361,7 @@ func (f *TemplateFormatter) renderSubject(
// Returns "" when no text block is defined — callers then send HTML-only,
// exactly as before this block existed.
func (f *TemplateFormatter) renderText(
- tmpl *template.Template, templateName string, data any,
+ tmpl *texttemplate.Template, templateName string, data any,
) (string, error) {
textTmpl := tmpl.Lookup("text")
if textTmpl == nil {
diff --git a/server/internal/email/formatter_test.go b/server/internal/email/formatter_test.go
index fa7b72755..ecbe9aaca 100644
--- a/server/internal/email/formatter_test.go
+++ b/server/internal/email/formatter_test.go
@@ -91,7 +91,12 @@ func TestFormatter_IncidentNumberOmittedWhenZero(t *testing.T) {
r.NoError(err)
r.NotContains(subject, "#0")
r.NotContains(subject, "#")
- r.NotContains(html, "#0")
+ // The HTML assertion is anchored on ">#0" — i.e. "#0" rendered as element
+ // TEXT — rather than a bare "#0", which the branded palette's hex colors
+ // (#0f1a24, #0072d5, #0e8f2a) now match inside inlined style attributes.
+ // The number only ever appears as the content of a or an , so this
+ // still fails the moment a "#0" is rendered to the reader.
+ r.NotContains(html, ">#0")
r.NotContains(text, "Incident: #")
}
@@ -624,3 +629,41 @@ func TestDict(t *testing.T) {
_, err = dict(1, "a")
r.ErrorIs(err, errDictKeyNotString)
}
+
+// TestSubjectAndTextAreNotHTMLEscaped pins the split between the three parts
+// Format returns: only the HTML body is HTML.
+//
+// The whole template set is parsed by html/template, whose escaper rewrites
+// every INTERPOLATED value (literal template text is left alone, which is why
+// this stayed invisible for so long). A check named "Search & Discovery"
+// therefore reached the reader as "Search & Discovery" — in the subject
+// line, before they even opened the mail, and again in the plaintext part.
+// Subject and text now render through text/template.
+//
+// The last assertion is the one that matters most: the HTML body must STILL be
+// escaped. Rendering everything as text would fix the subject and quietly turn
+// every user-controlled name in the HTML part into markup injection.
+func TestSubjectAndTextAreNotHTMLEscaped(t *testing.T) {
+ t.Parallel()
+
+ r := require.New(t)
+
+ formatter, err := NewFormatter()
+ r.NoError(err)
+
+ const hostile = `Search & Discovery "prod" `
+
+ viewModel := incidentViewModel()
+ viewModel["CheckName"] = hostile
+
+ subject, htmlBody, text, err := formatter.Format("incident-created.html", viewModel)
+ r.NoError(err)
+
+ r.Contains(subject, hostile, "the subject line is a mail header, not HTML")
+ r.NotContains(subject, "&")
+ r.Contains(text, hostile, "the plaintext alternative is not HTML either")
+ r.NotContains(text, "&")
+
+ r.NotContains(htmlBody, "", "the HTML body must keep escaping interpolated values")
+ r.Contains(htmlBody, "&")
+}
diff --git a/server/internal/email/supportreply.go b/server/internal/email/supportreply.go
index 120ce58a8..77b38c62c 100644
--- a/server/internal/email/supportreply.go
+++ b/server/internal/email/supportreply.go
@@ -2,6 +2,7 @@ package email
import (
"io/fs"
+ "sort"
"strings"
)
@@ -126,6 +127,17 @@ func classifiedTemplates() map[string]bool {
// templates directory, excluding the base wrapper (which is never rendered on
// its own and therefore has no classification of its own).
func shippedTemplateNames() ([]string, error) {
+ return ShippedTemplateNames()
+}
+
+// ShippedTemplateNames lists every template file shipped in the embedded
+// templates directory, sorted, excluding base.html.
+//
+// Exported so other packages enumerate the real directory instead of keeping a
+// hand-maintained copy of the list: the email preview package's fixture-coverage
+// test reads this, which is what makes "a new template ships without a preview
+// fixture" a failing test rather than a silent gap.
+func ShippedTemplateNames() ([]string, error) {
entries, err := fs.ReadDir(templateFS, "templates")
if err != nil {
return nil, err
@@ -141,6 +153,8 @@ func shippedTemplateNames() ([]string, error) {
names = append(names, entry.Name())
}
+ sort.Strings(names)
+
return names, nil
}
diff --git a/server/internal/email/templates/base.html b/server/internal/email/templates/base.html
index 2e64b7b22..7c37c3f02 100644
--- a/server/internal/email/templates/base.html
+++ b/server/internal/email/templates/base.html
@@ -3,73 +3,221 @@
+
+
+
+
+
+
+
SolidPing
+ {{- /* Preheader: the grey line the inbox shows next to the subject. Without
+ one, clients scrape the first visible text — which for an alert is the
+ header wordmark, and for a CTA mail the fallback URL. The trailing
+ zero-width-joiner run stops the client pulling footer text in behind a
+ short preheader. Hidden with the standard belt-and-braces set (a real
+ inline style, not a class, so it survives clients that drop |