fix: prevent markdown injection in notifications - #28340
Conversation
Notification title and body templates are Markdown authored by Coder, but
the values interpolated into them are user-controlled and were substituted
through text/template, which does no escaping. A display name that any
member can set via PUT /users/{user}/profile, or that arrives unvalidated
from an OIDC or GitHub name claim on self-signup, could inject Markdown
structure into notifications delivered to every site Owner and User Admin.
A name such as "Eve\n## URGENT\n[Re-authenticate now](https://evil)"
rendered a live anchor and heading in the email body. The same string also
renders as a clickable link in the dashboard notification popover, which
displays inbox content as Markdown. html.SkipHTML does not help here: the
anchor is generated from Markdown syntax, not present as raw HTML in the
input.
Neutralize Markdown structure in label, data, and UserName values before
they reach the template. This happens in notifier.prepare so that
already-queued messages are covered and the stored payload keeps its
original values for webhook consumers. The character classes are narrow on
purpose:
- "\[]()!<" are escaped everywhere, as they can carry a destination.
- "#-+.>|" are escaped only in leading position, so mid-token occurrences
in values like "bobby-workspace" are untouched.
- "=~" are not escapable in either renderer, so the preceding line break
is folded instead. That denies them the line-start position a Setext
heading or tilde fence requires.
- Emphasis characters are left alone. They cannot carry a destination, and
escaping "_" corrupts values such as "user_override" that body templates
compare with eq, which silently dropped a paragraph from the AI budget
notifications.
Also drop Autolink for notification bodies, so neither a bare URL in a
value nor the URL inside escaped link syntax becomes an anchor; enable
html.Safelink to restrict generated hrefs to safe schemes; and fold line
breaks out of the Subject header while Q-encoding it, so a rendered value
cannot terminate the header and inject another.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 8 findings (2 P1, 1 P2, 1 P3, 3 Nit, 1 Note), COMMENT. Review Finding inventoryFinding inventory - PR #28340Findings
Law analysisRound 2. Effective LOC 1540 (+643 since round 1 baseline of 897), head Slices (from
Mandate is on independent risk domains, not size. A reviewer signing off on Markdown escaping is not evaluating RFC 2047 encoded-word semantics or RFC 5322 folding; bundling forces one sign-off for both. Round logRound 1Netero-only. 1 P1, 1 Nit. Reviewed against 676b9bb..9594a14. LOC 897 effective (no Law). Netero P1 gate: no panel, review posted with COMMENT event, panel will review after Netero findings addressed. Round 2Infrastructure-only (Netero + Law), no panel. Churn guard PROCEED (CRF-1, CRF-2 addressed). Law verdict Split-Mandatory: extract slice B (SMTP subject encoding) as a separate PR. Netero adds 1 P1, 1 P2, 1 P3, 1 Note, 2 Nits. Reviewed against c029869..a671d84. Posted with REQUEST_CHANGES. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review only: this is Netero's mechanical scan, the full review panel has not yet looked at this PR. The panel will review after the findings below are addressed.
Nice attributes of the change: the character-class comments justify each split cleanly (inlineCritical vs blockStart vs foldStart); TestEscapableSet pins the renderer honor tables so a dependency bump cannot silently drift; and the control-value guard (user_override, service, 0, bobby-workspace, 1.5) is the right cross-check for a design that deliberately keeps _ unescaped.
Netero flagged 1 P1 and 1 Nit. Verification against HEAD reproduces Netero's <ol> case and finds three siblings from the same root cause (bullet lists with *, and thematic breaks with *** or ___); they are folded into CRF-1 as one class rather than filed separately.
None of them appears in a control label value such as
user_override.
(Netero, describing the character-class rationale that CRF-1 revises.)
🤖 This review was automatically generated with Coder Agents.
EscapeMarkdown treated block-level meaning as "the line's first character is in blockStart", which missed two constructs. An ordered list opens with a digit run followed by ".", and the digit cleared the leading flag before the "." was reached. A bullet list and a thematic break open with "*" or "_", which were left unescaped as emphasis. A display name of "Eve\n\n1. malicious" therefore rendered <ol> into a notification body, and "***", "___", "* * *" or "_ _ _" rendered <hr>. Those are the same structural tags the package already asserts against. Escape "*" and "_" in leading position, and escape the "." that closes an ordered-list marker when a leading digit run precedes it and a space or the line end follows it. That following-space condition is CommonMark's rule for a list marker, and it keeps "1.5" and "10.0.0.1" out of the escaped set: body templates compare numeric label values with `eq`. Both renderers honor these escapes, so the rendered text and the notification golden files are unchanged. NeutralisesStructure now renders every value twice, once with its line breaks doubled. The existing OrderedList row passed only because a list cannot interrupt a paragraph without a blank line, so it exercised a shape that could never fire.
NeutralisesStructure rows are regression guards only if their value produces document structure without escaping. "Eve\n1. one" did not: a list cannot interrupt a paragraph without a blank line, so the digit-dot never reached a line-start position and the row passed whether or not EscapeMarkdown ran. Render each value a second time with escaping removed and require a structural tag, so a row that guards nothing fails at authoring time. Five values are inert by construction and carry inertRaw with the reason: two rely on the renderer having autolinking disabled, one on the safelink policy, one on a link reference definition not being recognized mid-paragraph, and one on its own backslashes. Reverting the doubled-line-break shape now fails six list rows instead of passing silently.
|
Implementing our own markdown escaping raises a red flag. Is there a way we can approach this where we either avoid applying the markdown->html transformation on the user-supplied values and/or use a popular library to do the escaping? In the past I've paired gomarkdown with bluemonday, though the goal there was stripping content rather than escaping. Alternatively we could inject random sentinel placeholder values that we then replace after the markdown has been rendered, though that's likely a bigger change to our rendering pipeline. |
Thanks @jscottmiller for the suggestion. I agree that implementing our own Markdown escaping isn’t ideal. I went looking for holes in the current escaper and found a few. A simple example is a display name like Eve\n\n hidden, which produces: I took a quick look at Bluemonday, but I don’t think it helps here. It operates on the rendered HTML, so by then x is already an and there’s no way to distinguish it from links intentionally produced by our templates. Stripping all links would break those templates. It also wouldn’t cover the plaintext output, webhook body, or inbox, none of which render HTML. I also looked for a well-established off-the-shelf Markdown escaper, including html-to-markdown/escape, but I couldn’t find one that fits this use case. The Markdown libraries I found focus on rendering rather than escaping untrusted interpolated values. If you know of one, I’d be happy to take a look. I do like the sentinel idea. I tried a quick prototype and it looks promising: A random 32-character token survives both renderers unchanged in bold text, headings, link URLs, table cells, and list items. Glamour uses WithWordWrap(0), so it isn’t split. As you mentioned, though, this would be a broader rendering-pipeline change. We’d need a resolve pass in six places, including the inbox path where rendered Markdown is stored in the DB and rendered again on read. It would also affect body_markdown in the webhook payload, which is documented and consumed outside this repo. I’d rather not make that design decision as part of this security fix, so I’ll open a separate ticket to get broader input. For this PR, I’m thinking we strengthen the custom escaper enough to address the security issue, then consider moving to sentinels as a follow-up if we agree that’s the better long-term approach. What do you think? |
Review: six items, three of them reproduced end to endI ran a multi-perspective review of this branch and then validated the significant findings against the real render pipeline rather than by inspection. Items 1-3 are confirmed with output; 4-6 are read from the code and the stdlib behavior. The design here is good, and I want to be specific about that before the findings: the Items 1-3 are cases where the escaper reasons about markdown markers while the renderer has a sink the marker analysis doesn't cover. 1. BLOCKER: backtick is escaped nowhere, and the fence info string is a live HTML sink
Interpolating an untrusted label into a normal body template produces: <pre><code class="language-">2\<a/href="https://phish.example">Click here to reactivate your account
</code></pre>
The fence needs block position, so the vector requires a value containing a blank line. That is satisfied by the ingress the PR description names. Reachability, most severe path
That is member privilege to HTML injection in an admin's mail client. Two more paths, both template-admin privilege but much wider blast radius: The OIDC and GitHub claim paths bypass Fix: add 2. BLOCKER:
|
The rendered-template goldens embed the body of every stored notification template, and one of those bodies carries an emdash predating this check (migration 000324, "workspace startup...even when claiming a prebuilt environment"). The text lives in an applied migration, so it cannot be edited in place, and any change that regenerates the goldens fails the check on a character it did not introduce. Same rationale as the existing cli/testdata exclusions: generated files whose content comes from somewhere the author does not control.
Five gaps found by re-probing the escaper against the real render pipeline.
Backtick moves to inlineCritical. A fenced block's info string is an HTML
sink: gomarkdown writes it into class="language-..." unescaped, and
html.SkipHTML does not apply because the node is a CodeBlock rather than an
HTMLBlock. A display name that closes the attribute and the tag put a live
anchor in mail to every user admin, reachable by any member via
PUT /users/me/profile.
Colon joins foldStart. It opens a definition list, and also a GFM table
delimiter row such as ":-- | --:" that escaping "|" cannot reach, because a
delimiter row's pipes are mid-line. Folded rather than escaped because
glamour does not honor "\:" and would leak a backslash into the plaintext
part.
Leading indentation is capped at three spaces. Four open an indented code
block and a space has no escape, so the run is truncated instead.
escapeValue now escapes nested map keys. It walked a decoded JSON value and
escaped its string leaves, copying keys verbatim, but a key is content
whenever a template ranges over the map with two variables:
{{range $resource, $paths := .Data.replacements}}
Those keys are Terraform resource addresses from provisioner output, so an
unescaped one rendered a live anchor to every template admin.
renderHTML guards Safelink. parser.IsSafeURL slices a destination to each
candidate prefix length before checking the destination is that long, so
"[docs]()" panicked. That reached HTMLFromMarkdown as well, making it live
outside notifications via OIDCConfig.SignupsDisabledText. A length-safe
override plus a recover, mirroring InnerTextFromMarkdown.
encodeHeaderValue handles two cases mime.WordEncoder cannot. It only encodes
a value holding a byte outside printable ASCII, and an RFC 2047 encoded-word
is nothing but printable ASCII, so a forged one reached the recipient's mail
client and was decoded there. It also joins words with a space rather than
folding, leaving long headers past RFC 5322's 998-octet limit.
notificationExtensions becomes an allowlist of the grammar templates use
rather than CommonExtensions minus Autolink. That drops Tables,
DefinitionLists and MathJax, each reachable from an untrusted value and used
by no shipped template, with no change to any template's rendering.
Three residuals stay open and are pinned by tests that fail if they close:
escapes are inert inside a template-supplied code span, a value's own first
line cannot be folded, and a title beginning "~~~" still renders an empty
subject. All three depend on where the value lands rather than what it
contains, which is what a pre-render escaper cannot see. They close under
placeholder substitution, not under another character class.
One golden moves, contradicting the current PR description: the resource
replacements body_markdown now reads docker_container\[0\].
…r.prepare The comment claimed the dispatcher "escapes at its own sinks". It does not. smtp/html.gotmpl renders through text/template, and its _subject, UserName, _body and action URL interpolations have no escaping, while logo_url and app_name do. _subject has also been through PlaintextFromMarkdown by then, which strips the escaping applied here back out. State the real reason the dispatcher gets the unescaped payload, which is that the webhook contract surfaces enqueued values verbatim, and name the sinks that are still bare so nobody reads this as a guarantee.
BobbyHo
changed the title
The fold that handles "=" and "~" can only remove a line break EscapeMarkdown itself emitted, so it never reaches the value's first line, where the template decides the position. Two consequences: A title template beginning with a label puts the value at the start of the document. A display name of "~~~" made the whole title an unterminated tilde fence whose info string was the trusted text, and glamour renders an empty code block, so the Subject, <title> and heading all came out blank. Reachable by any member via PUT /users/me/profile. A body template placing a value at a line start beneath a text line let "===" underline the trusted line into an <h1>. No shipped template does this, but nothing recorded the dependency either. Escape the character instead of folding it. Neither renderer honors "\=" or "\~", so the backslash reaches the reader, which is why isLeadingFoldConstruct is exact where opensFoldConstruct is approximate: folding a line that was not going to open anything is free, escaping one is not. "=> next" and a display name of "~tilde" keep rendering clean; "~~~" does not. A visible "\~~~" beats a Subject line that renders empty. ":" stays out of it. It is in foldStart for the definition list and table cases, both of which need a preceding line that a first line does not have.
|
All six items are addressed, plus the
Your Your open item is settled on the escaper side: an unescaped key renders a live Two corrections.
Also closed from a parallel sweep: MathJax spans, and a first line that is a tilde fence or a A repro note on item 1, since it cost me two attempts: the payload needs a line after the closing fence. Otherwise the template's trailing text lands on that line, the fence stops being a fence, and it degrades to an inert inline One residual ships documented, pinned by a test that fails if it closes: escapes are inert inside a template-supplied code span. That one is unfixable before rendering by construction, since whether an escape is honored is decided by a context the escaper never sees. Your original objection stands. Three of the sixteen findings resisted escaping outright, and each needed a different non-escaping mechanism to close: truncation for indentation, escaping-with-a-visible-artifact for the first line, and nothing at all for the code span. Placeholder substitution closes all three with one mechanism. I would ship this as the short-term fix and file that as a tracked follow-up, but I am open on the sequencing. |
Around a hundred comment lines across the escaper, the renderer and the SMTP header work restated what the code already says or re-explained CommonMark. Cut those, along with the placeholder-substitution asides that belong in the design doc rather than repeated across five function comments. Kept what a reader cannot derive from the code: that gomarkdown writes a fence info string into class="language-..." unescaped and SkipHTML does not apply to a CodeBlock node; that glamour honors neither "\:" nor "\=" nor "\~", which is why those fold instead of escaping; that parser.IsSafeURL slices before bounds-checking; that mime.WordEncoder passes printable ASCII through untouched; and why opensFoldConstruct may be approximate where isLeadingFoldConstruct has to be exact. Test comments keep their notes on how a case could go vacuous, since that is what stops someone deleting a row that looks redundant.
Enabling Safelink also stopped fragment and bare relative destinations from linking, so [a](#x) and [a](docs/x.md) now render without an anchor while /path, ./path, mailto: and http(s):// keep working. No shipped template is affected, but a future template author gets silent link loss with no error. Pinned by two rows in TestEscapeMarkdownEmptyLinkDestination so the comment cannot drift away from the behavior.
There was a problem hiding this comment.
Round 2. CRF-1 (P1) and CRF-2 (Nit) both landed; verified against the current code, thanks. This round is Netero + Law only, no panel: Law's mandatory-split verdict gates panel review, so this is a COMMENT with changes recommended before the panel opens.
Netero: 1 P1, 1 P2, 1 P3, 1 Note, 2 Nits.
The P1 (CRF-3) is a deferral without a ticket. Dispatcher runs PlaintextFromMarkdown on the rendered title, which honors exactly the backslashes the escaper just added, and then assigns the plaintext into payload.Labels["_subject"] on the unescaped payload; smtp/html.gotmpl interpolates {{ .Labels._subject }} twice with no | html. The notifier.prepare comment now names this bare sink honestly, but that is not the same as fixing it. Per accepted-gap rules this needs an explicit human decision: file the tracking ticket for the follow-up work, or state the acceptance explicitly here. A follow-up PR completes the fix is a promise, not a deferral.
Law, effective 1540 LOC (+643 since round 1): Split, Mandatory. Extract slice B (SMTP Subject encoding, smtp.go + smtp_internal_test.go + smtp_test.go, ~65 prod / ~233 test) as a separate PR. Slice A (emdash exclusion) is already isolated as commit cf118ad4d2. Slice C (escape + wiring + grammar allowlist) stays whole. Mandate is on independent risk domains, not size: RFC 2047 encoded-word / RFC 5322 folding is a different failure surface (mangled subject lines in production mail) than Markdown escaping (backslash leaks, unescaped values). Bundled, one sign-off covers two review questions that do not overlap.
Extracting slice B naturally moves CRF-4 (P2) into its own PR where the RFC-correctness review can focus on it. CRF-3 needs to close in slice C regardless of the split.
Good in the diff: TestNotificationExtensionsDropUnusedGrammar pinning the exact reason each extension is dropped; TestEscapeMarkdownResiduals locking in what the escaper cannot fix pre-render; the liveness guard in checkNeutralised that fails any row that would have passed the vacuous check.
"a follow-up PR completes the fix" is a promise, not a deferral.
(Netero, on the Subject sink.)
🤖 This review was automatically generated with Coder Agents.
The gate measured the raw value, but Q-encoding expands a non-ASCII rune to three characters per byte, so 200 accented characters (400 bytes) cleared the 900-byte gate and still emitted a single 1459-octet header line, past RFC 5322's 998. Measure the encoded form instead. [CRF-4] Also from the same review: - The shared renderer is not unchanged, as a test comment claimed. It routes through renderHTML, so Safelink applies to HTMLFromMarkdown as well, and OIDCConfig.SignupsDisabledText silently stopped linking unsafe schemes, fragments and bare relative destinations. Correct the comment and pin both the changed behavior and what that caller actually needs. [CRF-5] - Split renderHTML's panic guard into recoverToEscapedSource so it has a test. Driving it through renderHTML would not have reached it: safeURL closed the only input known to panic, so such a test would pass without exercising the recovery at all. [CRF-6] - Drop a duplicated body closure in favor of suspendedBody, and hoist a twice-declared permissive const to package level. [CRF-7, CRF-8]
Fold the four restatements of the eq-control-value rationale into one, drop the doc comments that paraphrase the code they sit above, and cut edit history from the test comments. Keeps the facts a reader cannot recover from the code: the fence info-string sink, why =~: are folded rather than escaped, the IsSafeURL bounds bug, and the code-span residual. Comments only, no behavior change.
Cut every test doc comment to one or two lines and trim the inline comments to match. What was dropped is already stated where it acts: the character class rationale on the consts in escape.go, and the re-derive instruction in the assertion failure messages. Comments only, no behavior change.
BobbyHo
added
backport
backport/v2.35
2 participants
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:
UserNamevalues before they reach the template. Applied innotifier.prepare, so already-queued messages are covered and the stored payload keeps its original values for webhook consumers. Nested.Datamap keys are escaped too: one shipped template prints a key, and those keys are Terraform resource addresses.CommonExtensionsenabled 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.html.Safelink, restricting generated hrefs to safe schemes, and guards the panic it exposes:parser.IsSafeURLslices a destination before bounds-checking it, so[docs]()crashed both renderers.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 intoclass="language-..."unescaped, andSkipHTMLdoes not apply to aCodeBlocknode, so a value that closes the attribute and the tag injects live markup.#-+.>|only in leading position, so values likebobby-workspaceand1.5are 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._corrupts label values such asuser_overridethat body templates compare witheq, which silently drops content from the rendered email.One golden file changes: the resource replacements
body_markdownnow readsdocker_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, andhtml.gotmplthen interpolated the result throughtext/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.