mirror of
https://github.com/coder/coder.git
synced 2026-10-31 02:43:21 +08:00
First of two PRs. Notification title and body templates are Markdown authored by Coder, but the label values interpolated into them are user-controlled and were substituted through `text/template`, which does no escaping. Those values arrive from user profile fields and from OIDC/GitHub name claims. This PR: - Neutralizes Markdown structure in label, data, and `UserName` values before they reach the template. Applied in `notifier.prepare`, so already-queued messages are covered and the stored payload keeps its original values for webhook consumers. Nested `.Data` map keys are escaped too: one shipped template prints a key, and those keys are Terraform resource addresses. - Narrows the notification Markdown grammar to what templates actually use. `CommonExtensions` enabled Tables, DefinitionLists and MathJax, each openable from a value and used by no template. Autolink stays off so a URL in a value cannot become an anchor. - Enables `html.Safelink`, restricting generated hrefs to safe schemes, and guards the panic it exposes: `parser.IsSafeURL` slices a destination before bounds-checking it, so `[docs]()` crashed both renderers. - Folds line breaks out of the `Subject:` header and encodes it, fixing a pre-existing RFC 2047 violation for non-ASCII subjects, a forged encoded-word that let a value choose the displayed subject, and headers running past RFC 5322's 998-octet line limit. Escaping is narrow on purpose, split by where each character carries meaning: - `` \[]()!<` `` everywhere. Backtick is in this group because a fenced block's info string is an HTML sink: gomarkdown writes it into `class="language-..."` unescaped, and `SkipHTML` does not apply to a `CodeBlock` node, so a value that closes the attribute and the tag injects live markup. - `#-+.>|` only in leading position, so values like `bobby-workspace` and `1.5` are untouched. - `=`, `~` and `:` are not escapable by both renderers, so the preceding line break is folded instead. `:` opens a definition list and a GFM table delimiter row that escaping `|` cannot reach. A value's *first* line has no preceding break to fold, so a real tilde fence or `===` underline is escaped there instead, accepting a visible backslash: an unterminated `~~~` at the start of a title otherwise renders the Subject, `<title>` and heading empty. - Leading indentation is truncated to three spaces. Four open an indented code block and a space has no escape. - Emphasis characters are left alone. Escaping `_` corrupts label values such as `user_override` that body templates compare with `eq`, which silently drops content from the rendered email. One golden file changes: the resource replacements `body_markdown` now reads `docker_container\[0\]`, from the map-key escaping above. Every other golden is byte-identical. **One known residual**, pinned by a test that fails if it closes: CommonMark does not process escapes inside a code span, so where a template wraps a value in one, as the workspace out-of-disk body does, the escaper's own backslashes reach the reader. That depends on where the value lands rather than what it contains, which a pre-render escaper cannot see. This narrows the class rather than closing it. **#28397 completes the fix and is stacked on this branch. This PR should not merge without it.** Escaping here cannot reach the SMTP HTML template's sinks, by design rather than by oversight: the subject is produced by `PlaintextFromMarkdown`, which strips exactly the backslashes added here, and `html.gotmpl` then interpolated the result through `text/template`. On this branch alone, a label value still reaches the Subject, `<title>` and `<h1>` as live markup. #28397 escapes at those sinks with `| html`, which is the only place the information needed to escape correctly exists.
314 lines
12 KiB
Go
314 lines
12 KiB
Go
package render
|
|
|
|
import (
|
|
"fmt"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// asciiPunctuation is every ASCII punctuation character, the set CommonMark
|
|
// declares escapable.
|
|
const asciiPunctuation = "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
|
|
|
|
// TestEscapableSet pins the escapes each renderer honors, which is what
|
|
// EscapeMarkdown's character classes are derived from.
|
|
func TestEscapableSet(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Measured against gomarkdown and glamour as vendored today.
|
|
const (
|
|
wantHTML = "!#$&()*+-.:<>[\\]^_`{|}~"
|
|
wantPlain = "!#()*+-.<>[\\]_`{|}"
|
|
)
|
|
|
|
var gotHTML, gotPlain strings.Builder
|
|
for _, r := range asciiPunctuation {
|
|
escaped := `X\` + string(r) + `Y`
|
|
|
|
// HTML escapes markup characters, so compare the entity form.
|
|
wantLiteral := "X" + string(r) + "Y"
|
|
switch r {
|
|
case '<':
|
|
wantLiteral = "X<Y"
|
|
case '>':
|
|
wantLiteral = "X>Y"
|
|
case '&':
|
|
wantLiteral = "X&Y"
|
|
case '"':
|
|
wantLiteral = "X"Y"
|
|
}
|
|
html := strings.TrimSuffix(strings.TrimPrefix(HTMLFromMarkdown(escaped), "<p>"), "</p>")
|
|
if html == wantLiteral {
|
|
_, _ = gotHTML.WriteRune(r)
|
|
}
|
|
|
|
plain, err := PlaintextFromMarkdown(escaped)
|
|
require.NoError(t, err)
|
|
if plain == "X"+string(r)+"Y" {
|
|
_, _ = gotPlain.WriteRune(r)
|
|
}
|
|
}
|
|
|
|
require.Equal(t, wantHTML, gotHTML.String(),
|
|
"the set of characters gomarkdown honors as escapes has changed; re-derive EscapeMarkdown's character classes")
|
|
require.Equal(t, wantPlain, gotPlain.String(),
|
|
"the set of characters glamour honors as escapes has changed; re-derive EscapeMarkdown's character classes")
|
|
|
|
// Every character EscapeMarkdown escapes must be honored by both renderers.
|
|
for _, r := range inlineCritical + blockStart + leadingEmphasis {
|
|
assert.Contains(t, wantHTML, string(r), "gomarkdown does not honor \\%s", string(r))
|
|
assert.Contains(t, wantPlain, string(r), "glamour does not honor \\%s", string(r))
|
|
}
|
|
// foldStart characters are folded precisely because they are not escapable.
|
|
for _, r := range foldStart {
|
|
assert.NotContains(t, wantPlain, string(r),
|
|
"glamour now honors \\%s, so it could be escaped instead of folded", string(r))
|
|
}
|
|
}
|
|
|
|
// TestEscapeMarkdownControlValues guards the label values body templates compare
|
|
// with `eq`. Escaping one silently changes template control flow.
|
|
func TestEscapeMarkdownControlValues(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, v := range []string{
|
|
"user_override", // migrations/000553, {{if eq .Labels.limit_source "user_override"}}
|
|
"service", // migrations/000568, {{if eq .Labels.account_type "service"}}
|
|
"0", // migrations/000480, {{if eq .Data.retention_days "0"}}
|
|
"autobuild",
|
|
"initiator",
|
|
"user-override",
|
|
"1.5",
|
|
"10.0.0.1",
|
|
"bobby-workspace",
|
|
} {
|
|
require.Equal(t, v, EscapeMarkdown(v), "escaping changed a control value")
|
|
}
|
|
}
|
|
|
|
func TestEscapeMarkdown(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Emphasis and code tags are accepted residuals: no destination.
|
|
structuralTags := []string{
|
|
"<a ", "<img ", "<h1", "<h2", "<h3", "<h4", "<h5", "<h6",
|
|
"<ul", "<ol", "<hr", "<blockquote", "<table",
|
|
}
|
|
|
|
t.Run("NeutralisesStructure", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
type structureCase struct {
|
|
name string
|
|
value string
|
|
// inertRaw marks a value neutralized by something other than marker
|
|
// escaping. Leaving it unset on one fails the liveness check below.
|
|
inertRaw bool
|
|
}
|
|
|
|
for _, tc := range []structureCase{
|
|
{name: "DisclosurePayload", value: "Eve\n## URGENT: SSO certificate expiring\n[Re-authenticate now](https://coder-sso.attacker.example/login)"},
|
|
{name: "InlineLink", value: "[Re-authenticate now](https://attacker.example/login)"},
|
|
// A link reference definition is not recognized mid-paragraph.
|
|
{name: "ReferenceLink", value: "[Re-auth][1]\n\n[1]: https://attacker.example", inertRaw: true},
|
|
{name: "Image", value: ""},
|
|
{name: "AngleAutolink", value: "Eve <https://attacker.example>"},
|
|
// Neutralized by autolinking being off. See
|
|
// TestEscapeMarkdownNoAutolink.
|
|
{name: "BareURL", value: "Eve https://attacker.example/login", inertRaw: true},
|
|
{name: "Mailto", value: "Eve mailto:eve@attacker.example", inertRaw: true},
|
|
{name: "ATXHeading", value: "Eve\n## URGENT"},
|
|
{name: "SetextH1", value: "URGENT: re-auth required\n===\nx"},
|
|
{name: "SetextH1Spaced", value: "Eve\n=== \n#### x"},
|
|
{name: "SetextH1Repeated", value: "Eve\n===\n===\nx"},
|
|
{name: "SetextH2", value: "Eve\n---\nx"},
|
|
{name: "ThematicBreak", value: "Eve\n----\nx"},
|
|
{name: "ThematicBreakStars", value: "Eve\n***\nnext"},
|
|
{name: "ThematicBreakUnderscores", value: "Eve\n___\nnext"},
|
|
{name: "ThematicBreakSpacedStars", value: "Eve\n* * *\nnext"},
|
|
{name: "ThematicBreakSpacedUnderscores", value: "Eve\n_ _ _\nnext"},
|
|
{name: "BulletList", value: "Eve\n- one\n- two"},
|
|
{name: "BulletListStar", value: "Eve\n* one\n* two"},
|
|
{name: "BulletListStarIndented", value: "Eve\n * one\n * two"},
|
|
{name: "OrderedList", value: "Eve\n1. one\n2. two"},
|
|
{name: "OrderedListMultiDigit", value: "Eve\n99. one\n100. two"},
|
|
{name: "OrderedListParen", value: "Eve\n1) one\n2) two"},
|
|
{name: "Blockquote", value: "Eve\n> quoted"},
|
|
// Neutralized by the Tables extension being off; the escaper's own
|
|
// handling is covered by TestEscapeMarkdownColon.
|
|
{name: "Table", value: "a | b\n--- | ---\nc | d", inertRaw: true},
|
|
// Neutralized by the safelink policy. See
|
|
// TestEscapeMarkdownNoAutolink/SafelinkRejectsUnsafeSchemes.
|
|
{name: "JavascriptScheme", value: "[click](javascript:alert(1))", inertRaw: true},
|
|
// Asserts escaping does not re-enable the value's own backslashes.
|
|
{name: "EscapeForging", value: `Eve \[Re-auth\](https://attacker.example)`, inertRaw: true},
|
|
} {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Rendered twice, the second with line breaks doubled: only a
|
|
// blank line lets a block construct interrupt a paragraph.
|
|
rawProducedTag := false
|
|
for _, value := range []string{tc.value, strings.ReplaceAll(tc.value, "\n", "\n\n")} {
|
|
escaped := EscapeMarkdown(value)
|
|
html := HTMLFromNotificationMarkdown(suspendedBody(escaped))
|
|
plain, err := PlaintextFromMarkdown(suspendedBody(escaped))
|
|
require.NoError(t, err)
|
|
raw := HTMLFromNotificationMarkdown(suspendedBody(value))
|
|
|
|
for _, tag := range structuralTags {
|
|
assert.NotContains(t, html, tag, "value %q rendered HTML: %s", value, html)
|
|
rawProducedTag = rawProducedTag || strings.Contains(raw, tag)
|
|
}
|
|
// A backslash the value did not contain means a character the
|
|
// renderer does not honor was escaped.
|
|
if !strings.Contains(value, `\`) {
|
|
assert.NotContains(t, html, `\`, "value %q leaked a literal backslash into HTML", value)
|
|
assert.NotContains(t, plain, `\`, "value %q leaked a literal backslash into plaintext", value)
|
|
}
|
|
}
|
|
|
|
// Without this, a row whose value can never reach a line-start
|
|
// position passes whether or not EscapeMarkdown runs.
|
|
if !tc.inertRaw {
|
|
assert.True(t, rawProducedTag,
|
|
"vacuous row: %q produces no structural tag even unescaped, so the assertions above guard nothing; fix the value or set inertRaw with a reason",
|
|
tc.value)
|
|
}
|
|
})
|
|
}
|
|
})
|
|
|
|
t.Run("PreservesBenignValues", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Also why this change leaves the notification golden files untouched.
|
|
for _, value := range []string{
|
|
"William Tables",
|
|
"bobby-workspace",
|
|
"Bobby's Template",
|
|
"O'Brien-Smith (Eng) 100%",
|
|
"autodeleted due to dormancy (autobuild)",
|
|
"José Müller 日本語",
|
|
// Documented multi-line custom notification, see
|
|
// docs/admin/monitoring/notifications/index.md.
|
|
"Test results:\n • ✅ success",
|
|
"Test results:\n • ❌ failed (3 tests failed)",
|
|
} {
|
|
t.Run(value, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
escaped := EscapeMarkdown(value)
|
|
require.Equal(t, HTMLFromNotificationMarkdown(suspendedBody(value)), HTMLFromNotificationMarkdown(suspendedBody(escaped)))
|
|
|
|
wantPlain, err := PlaintextFromMarkdown(suspendedBody(value))
|
|
require.NoError(t, err)
|
|
gotPlain, err := PlaintextFromMarkdown(suspendedBody(escaped))
|
|
require.NoError(t, err)
|
|
require.Equal(t, wantPlain, gotPlain)
|
|
})
|
|
}
|
|
})
|
|
|
|
t.Run("ControlCharacters", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, tc := range []struct {
|
|
name string
|
|
value string
|
|
want string
|
|
}{
|
|
{"KeepsNewlines", "a\nb", "a\nb"},
|
|
{"FoldsCarriageReturn", "a\r\nb", "a \nb"},
|
|
{"FoldsTab", "a\tb", "a b"},
|
|
{"FoldsVerticalTab", "a\vb", "a b"},
|
|
{"DropsNul", "a\x00b", "ab"},
|
|
{"DropsBell", "a\x07b", "ab"},
|
|
{"DropsDelete", "a\x7fb", "ab"},
|
|
{"Empty", "", ""},
|
|
} {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
require.Equal(t, tc.want, EscapeMarkdown(tc.value))
|
|
})
|
|
}
|
|
})
|
|
|
|
t.Run("AngleBracketsAreNeutralised", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// "Ops <ops@example.com>" used to render as a mailto anchor; turning it
|
|
// into text is a deliberate behavior change.
|
|
html := HTMLFromNotificationMarkdown(suspendedBody(EscapeMarkdown("Ops <ops@example.com>")))
|
|
require.NotContains(t, html, "<a ")
|
|
require.Contains(t, html, "<ops@example.com>")
|
|
})
|
|
|
|
t.Run("EmphasisIsNotEscaped", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Mid-line "*" and "_" carry no destination and escaping "_" corrupts
|
|
// control values. Backtick reaches the info-string sink, so it is escaped.
|
|
require.Equal(t, "Eve *_\\`~", EscapeMarkdown("Eve *_`~"))
|
|
|
|
// Leading "*" and "_" open a list or thematic break. "~" is denied the
|
|
// line-start position by the fold, glamour not honoring "\~".
|
|
require.Equal(t, "\\*_\\`~", EscapeMarkdown("*_`~"))
|
|
require.Equal(t, "\\_*\\`~", EscapeMarkdown("_*`~"))
|
|
})
|
|
}
|
|
|
|
// TestEscapeMarkdownNoAutolink: a URL in an untrusted value must not become an
|
|
// anchor, while links in the trusted template markdown keep working.
|
|
func TestEscapeMarkdownNoAutolink(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
t.Run("UntrustedValueProducesNoAnchor", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, value := range []string{
|
|
"Eve https://attacker.example/login",
|
|
"Eve [Re-auth](https://attacker.example/login)",
|
|
"Eve <https://attacker.example>",
|
|
"Eve mailto:eve@attacker.example",
|
|
"Eve http://attacker.example",
|
|
} {
|
|
html := HTMLFromNotificationMarkdown("Account **" + EscapeMarkdown(value) + "** suspended.")
|
|
assert.NotContains(t, html, "<a ", "value %q produced an anchor", value)
|
|
}
|
|
})
|
|
|
|
t.Run("TrustedTemplateLinksStillRender", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Shapes taken from shipped notification body templates.
|
|
for _, markdown := range []string{
|
|
"marked as [**dormant**](https://coder.com/docs/templates/schedule#dormancy-threshold-enterprise) because of x",
|
|
"See [the docs](https://coder.com/docs/admin/templates/troubleshooting).",
|
|
} {
|
|
html := HTMLFromNotificationMarkdown(markdown)
|
|
assert.Contains(t, html, `<a href="https://coder.com/docs/`, "trusted link did not render: %s", html)
|
|
}
|
|
})
|
|
|
|
t.Run("HTMLFromMarkdownStillAutolinks", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// The shared renderer keeps Autolink, so the OIDC signups-disabled page
|
|
// still linkifies. Safelink does now apply; see TestHTMLFromMarkdownSafelink.
|
|
require.Contains(t, HTMLFromMarkdown("see https://coder.com/docs"), `<a href="https://coder.com/docs"`)
|
|
})
|
|
|
|
t.Run("SafelinkRejectsUnsafeSchemes", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, dest := range []string{"javascript:alert(1)", "data:text/html;base64,PHNjcmlwdD4="} {
|
|
html := HTMLFromNotificationMarkdown(fmt.Sprintf("[click](%s)", dest))
|
|
assert.NotContains(t, html, "<a ", "unsafe scheme %q was linked", dest)
|
|
}
|
|
})
|
|
}
|