fix: prevent markdown injection in notifications (#28340) by github-actions[bot] · Pull Request #28609 · coder/coder · GitHub
Skip to content

fix: prevent markdown injection in notifications (#28340) - #28609

Merged
mtojek merged 1 commit into
release/2.36from
backport/28340-to-2.36
Aug 26, 2026
Merged

fix: prevent markdown injection in notifications (#28340)#28609
mtojek merged 1 commit into
release/2.36from
backport/28340-to-2.36

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Backport of #28340

Original PR: #28340 — fix: prevent markdown injection in notifications
Merge commit: 9e8075d
Requested by: @BobbyHo

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.

(cherry picked from commit 9e8075d)
@github-actions github-actions Bot added the backport/v2.36 Backport PR targeting release/2.36 label Aug 25, 2026
@github-actions
github-actions Bot requested a review from BobbyHo August 25, 2026 22:02
@mtojek
mtojek self-requested a review August 26, 2026 07:33
@mtojek
mtojek merged commit 7bb00a1 into release/2.36 Aug 26, 2026
25 checks passed
@mtojek
mtojek deleted the backport/28340-to-2.36 branch August 26, 2026 07:33
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/v2.36 Backport PR targeting release/2.36

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants