fix(render): harden both renderers against hostile model strings (#35, #29) - #37
Conversation
`sanitize` passed `<`, `>` and `%%` through untouched. A role carrying a
`%%{init: ...}%%` directive was interpreted rather than drawn: confirmed
against mmdc 11, it restyled the whole diagram and swallowed its own label.
A role containing `<angle>` was parsed as a tag and vanished from the
rendered diagram with no diagnostic.
Runs of two or more percent signs collapse to one, which a directive cannot
be built from while a lone "50% full" still reads. `&`, `<` and `>` become
character references, in that order so an ampersand the author wrote is not
fused with a reference this function introduced; Mermaid decodes them back,
so the reader sees exactly what the author typed.
testdata/hostile_role.golden.mmd deliberately pinned the pre-fix behavior --
its comment said fixing #29 must replace it. This does, and the test comment
now describes the fixed contract instead.
Fixes #29
Signed-off-by: Joe Beda <joe@stacklok.com>
…#35) Two gaps in the same class, both reachable from any unconstrained string. A backtick in a code-spanned field closed its span early and dropped the remainder into the document as live Markdown/HTML (#35). The `codeSpan` helper from #34 already fences past backtick runs, so this applies it at the call sites that wrap a schema-unconstrained value: attribute names, enum value names, action names, and an action's actor. Entity, enum and glossary keys, `subtypeOf`, a relationship's target and an invariant id are pinned by schema patterns that exclude a backtick, and `render` validates structurally before it renders, so they stay as they are. Table cells route through a new `codeCell`, since a pipe in a name ended the cell. GFM recognizes a backslash-escaped pipe inside a code span. Separately, prose fields (`definition`, `description`, a role, a note, an invariant statement, a scenario step) emitted raw HTML. They stay Markdown -- models backtick their entity names throughout -- but an angle bracket in one is now a character reference, so a tag renders as the text the author typed. The pass is code-span aware: inside a span `<x>` is already literal and escaping it would put a visible "<x>" on the page. An ampersand is escaped only where it introduces a character reference. ADR-0014 records the contract and the reasoning. No committed golden changed: no example holds a backtick or an angle bracket in any affected field. Fixes #35 Signed-off-by: Joe Beda <joe@stacklok.com>
Records the rendering-contract change behind the #35 prose fix: what a definition: means when rendered, why Markdown stays and raw HTML does not, and why the ampersand rule differs between the two renderers. Ties it to ADR-0010's vendoring boundary and ADR-0011's offline render. Signed-off-by: Joe Beda <joe@stacklok.com>
Review loop — round 1 recordSource: real Two HIGH findings that defeat the security rule ADR-0014 states, two LOW. Both HIGHs verified end to end with the real binary and a real Markdown parser ( F1 (HIGH) —
|
The escaping pass tracked code spans with a character scanner. It had no model of block structure or of backslash escapes, so it read an escaped "\`" as a delimiter and let a span run across a paragraph break — two ways to declare raw HTML literal that CommonMark never would, both shipping a live tag into the published page. It also escaped inside "~~~" fences and indented code blocks, putting the visible "<" on the page ADR-0014 exists to avoid; a backtick fence survived only because the scanner happened to match it as a span. Delegate the semantics to goldmark. The parse locates the literal regions and the escaping writes back into the original bytes at those offsets — goldmark's renderer is never run, because re-rendering would reflow the author's prose and churn every committed .md. The parser is assembled from parser.NewParser rather than goldmark.New so the HTML renderer never enters the binary; no committed .md changes. Where a value lands decides what is literal there. A description emitted as its own block can hold a code block; a table cell, a heading or a list item cannot open one partway through a line, so a value that looks like a fence when parsed alone is ordinary text in the document, and treating it as code would leave the HTML behind it live. Hence proseBlock and proseLine, and an entity derivation — emitted after "**Derived:** " — takes the line form. The old guard reimplemented the scanner's own matching, so it shared the bug and passed on both inputs. Assert against goldmark instead: parse the rendered document with a second implementation and require that no raw HTML node survives, that no payload reaches the text outside a code span, and that heading, link and table-cell counts are what the renderer meant to emit. Signed-off-by: Joe Beda <joe@stacklok.com>
The record described the hand-rolled scanner and the consequences that followed from it. Rewrite it in place — it is unmerged, so there is no shipped decision to supersede — around what the code now does, and fold in the dependency call: why a parser rather than a better scanner, what it costs a single-binary tool, and what was rejected. Every accepted consequence was re-checked against the binary rather than carried forward. The autolink loss and the untouched MDX brace still hold; the goldens still do not churn. Three are new: the dependency's measured cost and its clean bill under ADR-0011, a code block in a block-level field now reaching the page verbatim, and an entity derivation collapsing onto one line. Signed-off-by: Joe Beda <joe@stacklok.com>
Review loop — round 1 fixes, independently verifiedAll four findings fixed in 2 signed-off commits (
Earlier prose rules still hold, re-checked: The renderer is never run. Only F4 was teeth-checked. The fix agent temporarily reverted Gates
Judgment calls for @jbeda to validate1. A block-vs-line distinction the brief did not specify. Parsing a one-line value standalone makes Consequence: a multi-paragraph I think this is right rather than a loss — a GFM table cell cannot contain a paragraph break under any implementation, so the alternative is a broken table. No golden moved. 2. Residual, unfixed and pre-existing: an unclosed fence in block prose swallows the rest of the document. Verified byte-identical between a Round 2 next: an approximation, since |
Review loop — round 2 recordSource: an APPROXIMATION of
Every finding below was reproduced independently by the orchestrator before being recorded. Two are regressions this PR introduces. Findings
One root cause behind H2, M1, L1 and L2
The fix is to invert it to an allowlist, which is what the implementation brief originally specified:
Escaping only nodes that are raw HTML never touches structure, so parse-then-write-back stays sound. It is also the more defensible rule: "escape exactly what we recognised as dangerous" rather than "escape everything we did not recognise as safe." Verified cleanOffset arithmetic (200k-case fuzz), UTF-8 handling, idempotence, determinism, node-kind coverage for the ranges it does collect, and the field-routing table — every field that reaches output unescaped is still schema-constrained. goldmark is pure Go; Where this leaves the PRWorse than it looked after round 1. It currently introduces a crash and a live-HTML regression, neither of which CI catches. A fix round follows. That will exhaust the three-round cap, so the result wants a real For @jbeda to decide, flagged not guessedUnder an allowlist, a |
`modelith render` crashed with a Go stack trace on a model that lints
clean. An entity definition ending "> \t`" exits 2 with
panic: runtime error: index out of range [-1]
goldmark/parser.(*fencedCodeBlockParser).Open
goldmark measures a line's indent in columns and its length in bytes,
then calls the line blank when the indent reaches the length. A tab
expands to four columns, so a short final line with one in it outruns
itself, gets marked blank with the -1 that means "no offset", and the
fenced-code-block parser — dispatched because the byte after the indent
is a backtick — indexes the line at -1.
Upstream yuin/goldmark#556 is the same -1 reached by a different path.
It is fixed, and v1.8.4 carries the fix, but the fix only swapped
BlockIndent() for BlockOffset() and both return -1 from the same
mismatched comparison: v1.8.4 still panics on #556's own reported repro.
There is no release to upgrade to.
So normalise the input instead of guarding the library. A document is
defined to end with a line ending, and appending one keeps the column
count from outrunning the byte count. It moves no offset inside the
string, so the escaping still writes back at the positions the parse
reports. 7.4 million fuzz executions over the normalised input found no
remaining panic; the raw input crashes in seconds.
Signed-off-by: Joe Beda <joe@stacklok.com>
The escaping was an allowlist of what to leave alone: a parse collected the code spans and code blocks, and every byte outside them had "<", ">" and an entity "&" escaped. That escaped Markdown structure. A ">" is a blockquote marker, and turning it into ">" stopped it opening one, so the indented code block inside the quote — which the parse had just reported as literal — collapsed into a paragraph and shipped its contents live. The rule invalidated the very offsets it wrote at. It also killed the "<...>" around a link destination, leaving a dead href, and the pointy-bracket autolink form. Invert it. Walk the AST and escape the byte ranges of the raw-HTML nodes — ast.RawHTML, ast.HTMLBlock — and a code fence's info string, which reaches the page as an attribute rather than as text and is inert to block structure. Leave every other byte as the author typed it. Escaping exactly what a parser called markup cannot disturb the parse it came from: it only ever removes angle brackets, never adds one. An "&" is no longer escaped outside a tag. That was fidelity rather than safety — a character reference is text to every parser and can never become a tag — and it is not worth a rule reaching beyond the markup. Inside a tag it is escaped along with the brackets, so the tag reaches the page byte-for-byte. A line assembled from several author-written fields is now escaped once, as the finished line. Whether a backtick opens a code span depends on what else is on the line, so escaping a `role:` and the `note:` beside it separately let a stray backtick in the role pair with the note's backticks and leave the note's tag outside every span, live. That was not a regression — it renders byte-identically on main — but ADR-0014 claims raw HTML does not survive a render, and it did. A line-context value is parsed behind a two-byte stand-in for the "- ", "# " or "| " it always follows, so a leading four-space indent or "```" run cannot pretend to open a code block the rendered line has no room for. No committed golden changes. Signed-off-by: Joe Beda <joe@stacklok.com>
The decision section described the rule the renderer no longer has. It claimed everything outside a literal region was escaped, which is what broke blockquotes and link destinations; it claimed the pointy-bracket autolink form was lost, and with it `<javascript:alert(1)>`, which is now the opposite of true. Restate the rule as the allowlist it became, record what that preserves that the old rule destroyed, and add the shared-line rule and the fence-info case. Correct the goldmark import claim: the parser pulls `net/netip` through `net/url`, and the offline test allows both by name. Add a "Not decided here" section, because an ADR whose whole claim is that raw HTML does not survive a render has to say where its rule stops. Three things sit outside it, all measured rather than asserted: a link scheme, which the allowlist made a live question by letting the autolink form survive; MDX, which reads a bare `<` as a JSX tag and fails a compile on a conceptual type that is unchanged from before the escaping existed; and the CommonMark/GFM parser mismatch, which stays open because `goldmark/extension` cannot be taken parser-only. Signed-off-by: Joe Beda <joe@stacklok.com>
A value on a line was parsed behind a stand-in for the text it follows, so it was always read as inline content. That is right for a heading and a table cell and wrong for a list item: a scenario step renders as "1. " and then the step, and the step opens the list item's first block. An HTML block opens there on a tag that never completes, so a step of "</div " was read as text, escaped nothing, and reached the page as an open HTML block. Read it both ways and escape what either reading calls markup. Neither covers the other — dropping the stand-in instead would let a heading of "> <script>" be read as a blockquote holding an indented code block, which is code in a list item and live text in a heading. Over-escaping costs a visible "<" in a case nobody writes; under-escaping ships the tag. Merging the two readings means ranges can now overlap, so the merge extends an interval instead of discarding the one that starts inside its predecessor, which would have dropped the tail of the longer range. Found by fuzzing escapeProse and the whole render against a GFM parser; 4.5 million executions are now clean outside the two classes ADR-0014 records as open. One of those is newly written down: a prose block that leaves a code fence open parses as code on its own and pairs with the Mermaid fence once it is in the document. Both are unchanged from main rather than introduced here. Signed-off-by: Joe Beda <joe@stacklok.com>
Review loop — round 2 fixes, independently verified. Loop cap reached with one HIGH still open.Four signed-off commits (
Earlier prose rules re-checked and still holding: H1: the upstream fix exists and does not workgoldmark#556 is the same H4 is open, and deliberately soAligning the implementation to GFM cannot be done parser-only: every So the implementation still parses CommonMark while GitHub and Docusaurus parse GFM, and GFM's table transformer can dissolve a CommonMark code span and ship a live tag. Pre-existing, not a regression — but it is a real open HIGH, and the three-round cap is now spent. Also open, recorded not fixed
Judgment calls for @jbeda to validate
RecommendationRun a real |
The renderer parses CommonMark while every reader parses GFM, so GFM's table transformer can dissolve a code span and let a tag through. Closing it means vendoring goldmark's table transformer into a renderer-free package: every extension file imports the root package, whose eager global drags the HTML renderer in — 27 symbols against zero today. Not worth it. Reaching the gap needs an author who is hostile and already has commit access to the repository being rendered, who could edit the generated Markdown directly instead. Vendoring changes that, since it renders models from repositories this one does not control. The answer there is to say so rather than to armour the renderer: deps import and deps update will warn that vendoring carries an injection risk and that models should only be vendored from sources you trust. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joe Beda <joe@stacklok.com>
1 participant
Hardens both renderers before ADR-0010's vendoring slice makes their inputs a trust boundary. Three parts, one PR.
Fixes #35
Fixes #29
1.
codeSpanat the unconstrained call sites (#35)A backtick in a code-spanned field closed its span early and dropped the remainder into the document as live Markdown/HTML.
codeSpan(added in #34, fences past backtick runs) now wraps every schema-unconstrained value: attribute names, enum value names, action names, and an action's actor.The audit found one call site the issue did not list —
enumValue.namein the Enums table — and two of the four it did list are stale:attributes[].typeandrelationships[].roleare not code-spanned on currentmain. Their raw-HTML hole is closed by part 3 instead. Fields with a schemapattern(entity/enum/glossary keys,subtypeOf, a relationship's target, an invariant id) are left alone;renderruns full structural validation before rendering, so those patterns genuinely hold.Also fixed alongside: a pipe in a name ended its table cell. New
codeCellescapes it, which GFM recognizes inside a code span.2. Mermaid
sanitize(#29)Confirmed against real mmdc 11, base vs. change:
%%{init:{'theme':'forest'}}%%fill:#cde498), swallowed its own label<angle><angle>Runs of 2+
%collapse to one (a directive can't be built from a single one;50% fullstill reads).&,<,>become character references, in that order so an author-written&isn't fused with one this function introduced — Mermaid decodes them back, so the reader sees what the author typed.testdata/hostile_role.golden.mmddeliberately pinned the pre-fix behaviour; its own comment said fixing #29 must replace it. This does.3. Prose fields render HTML as text — via a real parser (ADR-0014)
definition,description, a role, a note, an invariant statement, a scenario step emitted raw HTML. They stay Markdown — models backtick their entity names throughout andlint's backticked-term check depends on it — but a tag now renders as the text the author typed.This part was rewritten twice under review, and the shape it landed on matters:
goldmark decides what is a tag. A hand-rolled scanner shipped two HIGH bugs past CI (a backslash-escaped backtick opened a fake span; a span was allowed to cross a paragraph break). Both are cases where a character scanner and a Markdown parser disagree, so the scanner is gone.
internal/render/markdownnow parses the prose with goldmark and escapes the byte ranges ofast.RawHTMLandast.HTMLBlocknodes, leaving every other byte unchanged.It escapes what a parser calls HTML, not what it fails to call safe. The first goldmark attempt inverted this — it collected "literal" ranges and escaped the complement — which escaped the document's structure, and escaping structure changes the parse the offsets came from. That flattened blockquotes (turning a safe code block into a live tag), broke angle-bracket link destinations, and mangled autolinks. The allowlist has none of those failure modes.
goldmark's renderer is never run: only
goldmark/ast,goldmark/parserandgoldmark/textare imported, and the parser is built withparser.NewParserrather thangoldmark.New, so the HTML renderer never enters the binary. Verified withgo tool nm— 0 renderer, html or extension symbols.On
&: an author-written&passes through unchanged, including when it introduces a character reference. So an author who writes<sees<on the page, not<. That is a deliberate fidelity loss that follows from "leave every other byte unchanged"; a character reference cannot produce markup, so nothing unsafe follows from it. The Mermaid path still escapes every&— different consumer, different encoding. ADR-0014 records both.Dependency
goldmark v1.8.4 — modelith's first real parsing dependency. Pure Go, no cgo, +617 KiB. Adds only
net/urlandnet/netipfrom thenettree, both already allowed byTestADR_0011_OfflinePackages, which passes..goreleaser.yaml, the SBOM step andaction.ymlneed no changes.One local workaround: goldmark panics with
index out of range [-1]on certain unnormalised prose. Upstream #556 is the same bug and v1.8.4 already carries its fix, but that fix does not work — it swappedBlockIndent()forBlockOffset(), both of which return -1, comparing indent columns against bytes. Worked around by appending the trailing newline a document is defined to have, which moves no offset. To be reported upstream.Golden churn
None in any committed
.md. No prose field inexamples/ordocs/05-parking-garage/holds<,>or&, and no affected identifier holds a backtick. The only goldens that changed are the Mermaid testdata file, deliberately, and ADR-0014 itself.Knowingly not fixed
Decided on cost against threat, not overlooked. Reaching any of these needs a model author who is hostile and already has commit access to the repository being rendered — who could edit the generated
.mddirectly instead. That changes when ADR-0010's vendoring slice lands; the plan there is to warn ondeps import/deps updatethat vendoring carries an injection risk and that models should only be vendored from sources you trust.extensionfile imports the root package, whose eager global drags in the HTML renderer (27 symbols vs 0 today). Rejected in ADR-0014 under Considered and rejected.[click](javascript:alert(1))passes through — a destination is Markdown, not raw HTML. Its own decision, not taken here.map<string, int>and<https://example.com>fail an@mdx-js/mdx@3compile. Unchanged from before the escaping existed — the same bytes reach the page — but not closed by it either.Review
Three rounds. Round 1 was a real
/code-review high; rounds 2 and 3 were approximations (reviewer subagents plus an adversarial binary pass) run while the repo owner was away, and are labelled as such in each record. Every finding was reproduced independently before being recorded — including one published repro that did not reproduce and was corrected.Round records: r1 · r1 fixes · r2 · r2 fixes