diff --git a/go.mod b/go.mod index df8616d..6b34b3b 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.26 require ( github.com/santhosh-tekuri/jsonschema/v6 v6.0.1 github.com/spf13/cobra v1.10.2 + github.com/yuin/goldmark v1.8.4 golang.org/x/text v0.37.0 sigs.k8s.io/yaml v1.4.0 ) diff --git a/go.sum b/go.sum index 2b79d06..452d866 100644 --- a/go.sum +++ b/go.sum @@ -12,6 +12,8 @@ github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= +github.com/yuin/goldmark v1.8.4 h1:oat/nd3U6NeQqFEL3xpEJq7d7c86NI+DbSNGAs4xnjA= +github.com/yuin/goldmark v1.8.4/go.mod h1:ip/1k0VRfGynBgxOz0yCqHrbZXhcjxyuS66Brc7iBKg= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index 1cb787f..f242c3e 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -11,6 +11,10 @@ import ( "strings" "unicode" + "github.com/yuin/goldmark/ast" + "github.com/yuin/goldmark/parser" + "github.com/yuin/goldmark/text" + "github.com/stacklok/modelith/internal/model" "github.com/stacklok/modelith/internal/render/mermaid" ) @@ -44,10 +48,10 @@ func Render(m *model.Model, sourceDir, outDir string) string { if title == "" { title = "Domain Model" } - fmt.Fprintf(&b, "# %s\n\n", oneLine(title)) + fmt.Fprintf(&b, "# %s\n\n", proseLine(title)) if desc := strings.TrimSpace(m.Description); desc != "" { - b.WriteString(desc) + b.WriteString(proseBlock(desc)) b.WriteString("\n\n") } @@ -72,7 +76,7 @@ func renderInvariants(b *strings.Builder, m *model.Model) { } b.WriteString("## Invariants\n\n") for _, inv := range m.Invariants { - fmt.Fprintf(b, "- **%s** — %s\n", inv.ID, inv.Statement) + fmt.Fprintf(b, "- %s\n", invariantLine(inv.ID, inv.Statement)) } b.WriteString("\n") } @@ -188,7 +192,7 @@ func renderGlossary(b *strings.Builder, m *model.Model) { } sort.Strings(terms) for _, t := range terms { - fmt.Fprintf(b, "- **`%s`** — %s\n", t, oneLine(m.Glossary[t])) + fmt.Fprintf(b, "- %s\n", assembledLine("**"+codeSpan(t)+"**", oneLine(m.Glossary[t]))) } b.WriteString("\n") } @@ -208,13 +212,13 @@ func renderEnums(b *strings.Builder, m *model.Model) { en := m.Enums[name] fmt.Fprintf(b, "### `%s`\n\n", name) if d := strings.TrimSpace(en.Description); d != "" { - b.WriteString(d) + b.WriteString(proseBlock(d)) b.WriteString("\n\n") } b.WriteString("| Value | Definition |\n") b.WriteString("| --- | --- |\n") for _, v := range en.Values { - fmt.Fprintf(b, "| `%s` | %s |\n", v.Name, mdCell(v.Definition)) + fmt.Fprintf(b, "| %s | %s |\n", codeCell(v.Name), mdCell(v.Definition)) } b.WriteString("\n") } @@ -240,7 +244,7 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string fmt.Fprintf(b, "### `%s`\n\n", name) if def := strings.TrimSpace(ent.Definition); def != "" { - b.WriteString(def) + b.WriteString(proseBlock(def)) b.WriteString("\n\n") } @@ -258,7 +262,7 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string if ent.Derived { d := "**Derived.** Computed on demand from other state; never persisted." if derivation := strings.TrimSpace(ent.Derivation); derivation != "" { - d = fmt.Sprintf("**Derived:** %s", derivation) + d = fmt.Sprintf("**Derived:** %s", proseLine(derivation)) } b.WriteString(d) b.WriteString("\n\n") @@ -275,12 +279,12 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string parts = append(parts, rel.Ownership) } if rel.Role != "" { - parts = append(parts, rel.Role) + parts = append(parts, oneLine(rel.Role)) } if rel.Note != "" { - parts = append(parts, rel.Note) + parts = append(parts, oneLine(rel.Note)) } - fmt.Fprintf(b, "- %s\n", strings.Join(parts, " — ")) + fmt.Fprintf(b, "- %s\n", assembledLine(parts...)) } b.WriteString("\n") } @@ -302,7 +306,7 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string desc = d } } - fmt.Fprintf(b, "| `%s` | %s | %s |\n", attr.Name, typeCell(attr.Type, importTargets), mdCell(desc)) + fmt.Fprintf(b, "| %s | %s | %s |\n", codeCell(attr.Name), typeCell(attr.Type, importTargets), mdCell(desc)) } b.WriteString("\n") } @@ -312,7 +316,7 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string if len(ent.Invariants) > 0 { b.WriteString("**Invariants**\n\n") for _, inv := range ent.Invariants { - fmt.Fprintf(b, "- **%s** — %s\n", inv.ID, inv.Statement) + fmt.Fprintf(b, "- %s\n", invariantLine(inv.ID, inv.Statement)) } b.WriteString("\n") } @@ -336,9 +340,9 @@ func renderActions(b *strings.Builder, actions []model.Action) { if !structured { quoted := make([]string, len(actions)) for i, a := range actions { - quoted[i] = "`" + a.Name + "`" + quoted[i] = codeSpan(a.Name) } - fmt.Fprintf(b, "**Actions:** %s\n\n", strings.Join(quoted, ", ")) + fmt.Fprintf(b, "%s\n\n", escapeProse("**Actions:** "+strings.Join(quoted, ", "), false)) return } @@ -346,19 +350,23 @@ func renderActions(b *strings.Builder, actions []model.Action) { for _, a := range actions { var detail []string if a.Actor != "" { - detail = append(detail, fmt.Sprintf("actor `%s`", a.Actor)) + detail = append(detail, "actor "+codeSpan(a.Actor)) } if len(a.Preserves) > 0 { - detail = append(detail, "preserves "+strings.Join(a.Preserves, ", ")) + preserves := make([]string, len(a.Preserves)) + for i, id := range a.Preserves { + preserves[i] = oneLine(id) + } + detail = append(detail, "preserves "+strings.Join(preserves, ", ")) } - line := "`" + a.Name + "`" + parts := []string{codeSpan(a.Name)} if len(detail) > 0 { - line += " — " + strings.Join(detail, "; ") + parts = append(parts, strings.Join(detail, "; ")) } if a.Description != "" { - line += " — " + oneLine(a.Description) + parts = append(parts, oneLine(a.Description)) } - fmt.Fprintf(b, "- %s\n", line) + fmt.Fprintf(b, "- %s\n", assembledLine(parts...)) } b.WriteString("\n") } @@ -392,21 +400,25 @@ func renderScenarios(b *strings.Builder, m *model.Model) { b.WriteString("## Scenarios\n\n") for _, sc := range m.Scenarios { - fmt.Fprintf(b, "### %s\n\n", oneLine(sc.Name)) + fmt.Fprintf(b, "### %s\n\n", proseLine(sc.Name)) if desc := strings.TrimSpace(sc.Description); desc != "" { - b.WriteString(desc) + b.WriteString(proseBlock(desc)) b.WriteString("\n\n") } if len(sc.Actors) > 0 { - fmt.Fprintf(b, "**Actors:** %s\n\n", strings.Join(sc.Actors, ", ")) + actors := make([]string, len(sc.Actors)) + for i, a := range sc.Actors { + actors[i] = oneLine(a) + } + fmt.Fprintf(b, "%s\n\n", escapeProse("**Actors:** "+strings.Join(actors, ", "), false)) } if len(sc.Steps) > 0 { b.WriteString("**Steps**\n\n") for i, step := range sc.Steps { - fmt.Fprintf(b, "%d. %s\n", i+1, step) + fmt.Fprintf(b, "%d. %s\n", i+1, proseLine(step)) } b.WriteString("\n") } @@ -415,9 +427,9 @@ func renderScenarios(b *strings.Builder, m *model.Model) { b.WriteString("**Invariants touched**\n\n") for _, id := range sc.InvariantsTouched { if s, ok := statement[id]; ok { - fmt.Fprintf(b, "- **%s** — %s\n", id, s) + fmt.Fprintf(b, "- %s\n", invariantLine(id, s)) } else { - fmt.Fprintf(b, "- **%s**\n", id) + fmt.Fprintf(b, "- %s\n", escapeProse("**"+oneLine(id)+"**", false)) } } b.WriteString("\n") @@ -425,9 +437,223 @@ func renderScenarios(b *strings.Builder, m *model.Model) { } } -// mdCell escapes a value for use inside a Markdown table cell. +// assembledLine joins the pieces of one rendered line with the em-dash +// separator and escapes the result once, as the finished line. +// +// Escaping the pieces one at a time asked the wrong question. Whether a backtick +// opens a code span, and so whether the tag behind it is literal, depends on +// what else is on the line — so a stray backtick in a `role:` paired with the +// backticks of the `note:` beside it and left the note's contents outside every +// span, live (#37). Each piece is collapsed to a line by its caller; only the +// assembled line is parsed, because only the assembled line is what the reader's +// parser sees. +func assembledLine(parts ...string) string { + return escapeProse(strings.Join(parts, " — "), false) +} + +// invariantLine renders an invariant's id and statement as the one line they +// share. Both are author-written, so they are escaped together; see +// assembledLine. +func invariantLine(id, statement string) string { + return assembledLine("**"+oneLine(id)+"**", oneLine(statement)) +} + +// mdCell escapes an author-written prose value for use inside a Markdown table +// cell. A cell is escaped on its own because GFM splits a row into cells before +// it parses any of them, so nothing in one cell can reach into the next. func mdCell(s string) string { - return strings.ReplaceAll(oneLine(s), "|", "\\|") + return cellEscape(proseLine(s)) +} + +// codeCell renders a value as a code span inside a Markdown table cell. GFM +// recognizes a backslash-escaped pipe inside a code span, which is the only way +// a cell can carry one. +func codeCell(s string) string { + return cellEscape(codeSpan(s)) +} + +func cellEscape(s string) string { + return strings.ReplaceAll(s, "|", "\\|") +} + +// mdParser is a CommonMark parser used only to locate the literal regions of a +// prose string. Nothing is ever rendered through goldmark: its renderer +// normalizes and reflows text, which would rewrite prose the author wrote and +// churn every committed .md, so the escaping writes back into the original +// bytes at the offsets the parse reports (ADR-0014). +// +// The parser is built from the CommonMark defaults rather than through +// goldmark.New so the HTML renderer never enters the binary. parser.Parse is +// safe for concurrent use: its one-time setup is guarded by a sync.Once and +// each call carries its own context. +var mdParser = parser.NewParser( + parser.WithBlockParsers(parser.DefaultBlockParsers()...), + parser.WithInlineParsers(parser.DefaultInlineParsers()...), + parser.WithParagraphTransformers(parser.DefaultParagraphTransformers()...), +) + +// byteRange is a half-open [start, stop) region of a prose string. +type byteRange struct{ start, stop int } + +// linePrefix stands in, during the parse only, for text a line-context value +// follows on its rendered line — the "# " of a heading, the "| " of a table +// cell, the "**Bold** — " of a bullet that opens with generated markup. It +// holds column zero so the value is read as the inline content it is there. +// The offsets come back shifted by its length and are shifted off again. +const linePrefix = "x " + +// proseBlock escapes an author-written string that is emitted as its own block +// — a model, enum or scenario description, an entity definition. Fenced and +// indented code blocks are real there, so their contents are text and stay +// verbatim. +func proseBlock(s string) string { return escapeProse(s, true) } + +// proseLine escapes an author-written string that lands inside a line — a +// heading, a list item, a table cell — collapsing it to one line first. +// +// A value sharing its line with other author-written values is assembled first +// and escaped once, by the call site, rather than passed here piece by piece: +// what pairs with what is a property of the finished line (#37). +func proseLine(s string) string { return escapeProse(oneLine(s), false) } + +// escapeProse escapes the raw HTML in an author-written string and leaves every +// other byte alone. A `definition:` is Markdown — models backtick their entity +// names throughout and authors use emphasis — but it is not HTML: an angle +// bracket is text the author typed, not a tag to interpret (ADR-0014). +// +// The rule is an allowlist of what to escape, not of what to leave: exactly the +// bytes a parser reported as markup are escaped, and everything else is copied +// through. Escaping by exclusion was the same idea inverted, and it escaped +// Markdown structure — a ">" that opened a blockquote, the "<...>" around a link +// destination — which changed how the text parsed and so invalidated the very +// offsets it was writing at (#37). +func escapeProse(s string, blockContext bool) string { + ranges := markupRanges(s, blockContext) + if len(ranges) == 0 { + return s + } + var b strings.Builder + i := 0 + for _, r := range ranges { + b.WriteString(s[i:r.start]) + escapeMarkup(&b, s[r.start:r.stop]) + i = r.stop + } + b.WriteString(s[i:]) + return b.String() +} + +// markupRanges returns the regions of s that a CommonMark parser reads as +// markup rather than as text, sorted and non-overlapping: the tags of every +// raw-HTML node, and a code fence's info string. +// +// The parse is what decides. A hand-rolled backtick scanner has 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 call raw +// HTML literal that a real parser never would (#37). +// +// A code fence's info string is markup by the same test: it reaches the page as +// a class attribute rather than as text, so escaping it costs nothing and keeps +// an unclosed fence from carrying a tag on its opening line. It is also inert to +// block structure, so escaping it cannot disturb the parse it came from. +// +// A line-context value is read twice, and everything either reading calls markup +// is escaped. The two readings are both real: a value that opens a list item +// — a scenario step, rendered as "1. " and then the step — begins a block, where +// "" further along the line is +// the live tag instead. Neither reading alone covers the other, and the parse is +// too cheap to be worth guessing which applies. +func markupRanges(s string, blockContext bool) []byteRange { + out := parseMarkup(s, "") + if !blockContext { + out = append(out, parseMarkup(s, linePrefix)...) + } + + sort.Slice(out, func(i, j int) bool { return out[i].start < out[j].start }) + merged := out[:0] + for _, r := range out { + if n := len(merged); n > 0 && r.start <= merged[n-1].stop { + merged[n-1].stop = max(merged[n-1].stop, r.stop) + continue + } + merged = append(merged, r) + } + return merged +} + +// parseMarkup parses s behind prefix and returns the markup it finds, as ranges +// in s. The ranges are neither sorted nor merged; markupRanges does both, and +// two readings of the same string can overlap. +// +// The trailing newline is a workaround, not a nicety. goldmark decides whether +// a line is blank by comparing an indent measured in *columns* against the +// line's length in *bytes*, so a final line short enough for a tab to outrun it +// — "> \t`" — is called blank, and the fenced-code-block parser then indexes it +// at the -1 that marks one. That panics `render` on a model that lints clean. +// A line ending keeps the comparison honest, costs nothing (a document is +// defined to end with one) and, appended at the end, moves no offset in s. +// Upstream yuin/goldmark#556 fixed one path into the same -1 and v1.8.4 carries +// that fix; this input reaches it by another, and #556's own repro still panics +// on v1.8.4. TestRender_UnnormalisedProseDoesNotPanic guards it. +func parseMarkup(s, prefix string) []byteRange { + src := prefix + s + if !strings.HasSuffix(src, "\n") { + src += "\n" + } + shift := len(prefix) + + var out []byteRange + add := func(start, stop int) { + start, stop = max(start-shift, 0), min(stop-shift, len(s)) + if start < stop { + out = append(out, byteRange{start, stop}) + } + } + addSegments := func(segs *text.Segments) { + for i := 0; i < segs.Len(); i++ { + seg := segs.At(i) + add(seg.Start, seg.Stop) + } + } + // Walk never returns an error: the callback below returns none. + _ = ast.Walk(mdParser.Parse(text.NewReader([]byte(src))), func(n ast.Node, entering bool) (ast.WalkStatus, error) { + if !entering { + return ast.WalkContinue, nil + } + switch node := n.(type) { + case *ast.RawHTML: + addSegments(node.Segments) + case *ast.HTMLBlock: + addSegments(node.Lines()) + if node.HasClosure() { + add(node.ClosureLine.Start, node.ClosureLine.Stop) + } + case *ast.FencedCodeBlock: + if node.Info != nil { + add(node.Info.Segment.Start, node.Info.Segment.Stop) + } + } + return ast.WalkContinue, nil + }) + return out +} + +// escapeMarkup writes s as the text it should have been. A character reference +// is text to every Markdown parser, so nothing here can become a tag again. +func escapeMarkup(b *strings.Builder, s string) { + for i := 0; i < len(s); i++ { + switch c := s[i]; c { + case '<': + b.WriteString("<") + case '>': + b.WriteString(">") + case '&': + b.WriteString("&") + default: + b.WriteByte(c) + } + } } // oneLine collapses a value onto a single line for use in a heading, a list diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index f646bc7..8f4ebb2 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -3,12 +3,177 @@ package markdown import ( "fmt" "os" + "slices" "strings" "testing" + "github.com/yuin/goldmark" + "github.com/yuin/goldmark/ast" + "github.com/yuin/goldmark/extension" + extast "github.com/yuin/goldmark/extension/ast" + "github.com/yuin/goldmark/text" + "github.com/stacklok/modelith/internal/model" ) +// gfm parses rendered output the way a reader's tooling will: CommonMark plus +// the GFM extensions, tables above all, since most of what this renderer emits +// is a table. +// +// Every assertion about escaping in this file goes through it rather than +// through a hand-written scan of the output. The renderer used to carry its own +// backtick matcher, and the test that guarded it reimplemented the same +// matching — so the guard shared the implementation's model of Markdown and +// agreed with it about an escaped "\`" and about a span running across a +// paragraph break, both wrong, both shipping live HTML (#37). A test that +// reuses the implementation's assumptions proves nothing; this one asks a +// second implementation. +var gfm = goldmark.New(goldmark.WithExtensions(extension.GFM)) + +func parseRendered(md string) (ast.Node, []byte) { + src := []byte(md) + return gfm.Parser().Parse(text.NewReader(src)), src +} + +// rawHTML returns every raw-HTML fragment a real parser finds in md. ADR-0014's +// rule is that a rendered model holds none: an angle bracket the author typed +// reaches the page as text. +func rawHTML(t *testing.T, md string) []string { + t.Helper() + doc, src := parseRendered(md) + var out []string + segments := func(segs *text.Segments) { + for i := range segs.Len() { + s := segs.At(i) + out = append(out, string(src[s.Start:s.Stop])) + } + } + walk(t, doc, func(n ast.Node) ast.WalkStatus { + switch node := n.(type) { + case *ast.RawHTML: + segments(node.Segments) + case *ast.HTMLBlock: + segments(node.Lines()) + if node.HasClosure() { + out = append(out, string(src[node.ClosureLine.Start:node.ClosureLine.Stop])) + } + } + return ast.WalkContinue + }) + return out +} + +// textOutsideCode returns the document's text with every code span and code +// block skipped, walking the parse tree rather than rescanning backticks. A +// payload that appears here escaped the span that was supposed to hold it. +// +// A link destination is not part of it. A destination is percent-encoded by +// linkTarget, not escaped for prose, so an encoded payload would read as a +// false positive here; the destinations are asserted exactly, by hand, at the +// call sites that produce them. +func textOutsideCode(t *testing.T, md string) string { + t.Helper() + doc, src := parseRendered(md) + var b strings.Builder + walk(t, doc, func(n ast.Node) ast.WalkStatus { + switch node := n.(type) { + case *ast.CodeSpan, *ast.FencedCodeBlock, *ast.CodeBlock: + return ast.WalkSkipChildren + case *ast.Text: + b.Write(node.Segment.Value(src)) + b.WriteString("\n") + case *ast.AutoLink: + b.Write(node.URL(src)) + b.WriteString("\n") + } + return ast.WalkContinue + }) + return b.String() +} + +// links counts the link nodes in md. A destination that closed early and opened +// a second link changes the count, whatever the label says. +func links(t *testing.T, md string) int { + t.Helper() + doc, _ := parseRendered(md) + n := 0 + walk(t, doc, func(node ast.Node) ast.WalkStatus { + switch node.(type) { + case *ast.Link, *ast.AutoLink, *ast.Image: + n++ + } + return ast.WalkContinue + }) + return n +} + +// headings returns the text of every heading in md, in document order. A value +// that broke out of its line and opened a section shows up as an extra entry. +func headings(t *testing.T, md string) []string { + t.Helper() + doc, src := parseRendered(md) + var out []string + walk(t, doc, func(n ast.Node) ast.WalkStatus { + if h, ok := n.(*ast.Heading); ok { + out = append(out, nodeText(h, src)) + return ast.WalkSkipChildren + } + return ast.WalkContinue + }) + return out +} + +// nodeText concatenates the source text of n's inline descendants, code spans +// included. ast.Node.Text is deprecated, and it would drop the span that every +// entity heading is written in. +func nodeText(n ast.Node, src []byte) string { + var b strings.Builder + for c := n.FirstChild(); c != nil; c = c.NextSibling() { + if t, ok := c.(*ast.Text); ok { + b.Write(t.Segment.Value(src)) + continue + } + b.WriteString(nodeText(c, src)) + } + return b.String() +} + +// tableRows returns the cell count of every row of every table in md, header +// row first. A pipe that escaped its cell changes a count; counting pipes in +// the raw line cannot tell an escaped one from a structural one. +func tableRows(t *testing.T, md string) []int { + t.Helper() + doc, _ := parseRendered(md) + var out []int + walk(t, doc, func(n ast.Node) ast.WalkStatus { + switch n.(type) { + case *extast.TableHeader, *extast.TableRow: + cells := 0 + for c := n.FirstChild(); c != nil; c = c.NextSibling() { + if _, ok := c.(*extast.TableCell); ok { + cells++ + } + } + out = append(out, cells) + } + return ast.WalkContinue + }) + return out +} + +func walk(t *testing.T, doc ast.Node, visit func(ast.Node) ast.WalkStatus) { + t.Helper() + err := ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) { + if !entering { + return ast.WalkContinue, nil + } + return visit(n), nil + }) + if err != nil { + t.Fatalf("walking the parsed document: %v", err) + } +} + // render calls Render with sourceDir == outDir, the default beside-the-source // case every test in this file exercises except the ones specifically about // relativizing an import link against a different output directory (see @@ -377,69 +542,32 @@ func TestRenderImports_HostilePathCannotEscapeItsMarkup(t *testing.T) { } } - // Every breakout token that survives is inside a code span, where it is - // text. A bullet's only variable parts are the two spans and the - // percent-encoded destination, so removing the spans must leave the fixed - // skeleton and nothing else. - for _, line := range strings.Split(got, "\n") { - if !strings.HasPrefix(line, "- **") { - continue - } - bare := stripCodeSpans(line) - for _, tok := range []string{" 0 { + t.Errorf("a path reached the page as live HTML %q:\n%s", html, got) } - for _, line := range strings.Split(got, "\n") { - if strings.HasPrefix(line, "#") && strings.Contains(line, "Injected") { - t.Errorf("a path injected the heading %q:\n%s", line, got) + bare := textOutsideCode(t, got) + for _, tok := range []string{"= 0 && close+len(fence) < len(rest) && rest[close+len(fence)] == '`' { - // A longer run is not this fence's closer; keep looking. - next := strings.Index(rest[close+1:], fence) - if next < 0 { - close = -1 - break - } - close += 1 + next - } - if close < 0 { - b.WriteString(fence) // unclosed: the fence is literal text - continue - } - i += close + len(fence) - } - return b.String() -} - // TestCodeSpan_FencesAroundBackticks checks the CommonMark rule the imports // section leans on: a span's fence is longer than any backtick run inside it, so // a value holding backticks stays intact instead of closing the span early. @@ -478,3 +606,426 @@ func TestRenderEntity_SubtypeHierarchy(t *testing.T) { t.Errorf("expected parent to list its subtypes:\n%s", got) } } + +// TestProse_EscapesHTMLOutsideCodeSpans pins the escaping rule ADR-0014 +// records, case by case: angle brackets become character references outside a +// literal region and are left alone inside one, an ampersand is escaped only +// where it introduces a character reference, and Markdown is never touched. +// +// The "block" cases are the fields emitted as their own block — a description, +// a definition — where a code fence or an indented block is literal. Everything +// else lands inside a line, where it is not. +func TestProse_EscapesHTMLOutsideCodeSpans(t *testing.T) { + t.Parallel() + + cases := []struct { + name, in, want string + block bool + }{ + {name: "plain text", in: "A parking ticket.", want: "A parking ticket."}, + {name: "tag", in: "an tag", want: "an <img src=q onerror=alert(1)> tag"}, + {name: "markdown survives", in: "a `Ticket` with *emphasis*", want: "a `Ticket` with *emphasis*"}, + {name: "inside a code span", in: "a `` span", want: "a `` span"}, + {name: "in and out", in: " `` ", want: "<a> `` <c>"}, + {name: "unclosed span is text", in: "a ` tail", want: "a ` <b> tail"}, + {name: "double fence", in: "``a `` c`` ", want: "``a `` c`` <d>"}, + // An "&" is markup to nobody, so every one of these is left as the + // author typed it. A character reference renders as the character it + // names, which is what the author asked for; it can never become a tag, + // because a parser decodes it to text and re-escapes it on output. + {name: "bare ampersand", in: "R&D and a & b", want: "R&D and a & b"}, + {name: "named reference", in: "<pre>", want: "<pre>"}, + {name: "decimal reference", in: "<", want: "<"}, + {name: "hex reference", in: "<", want: "<"}, + {name: "reference-shaped but unterminated", in: "< and &", want: "< and &"}, + {name: "reference inside a code span", in: "`<`", want: "`<`"}, + // An angle bracket that opens no tag is not markup and is not touched. + // A Markdown parser renders it as the character it is. + {name: "comparison", in: "1 < 2 && 3 > 2", want: "1 < 2 && 3 > 2"}, + {name: "conceptual type", in: "map", want: "map"}, + {name: "multibyte passes through", in: "café — naïve ", want: "café — naïve <x>"}, + // An "&" inside a tag is escaped with it, so the whole tag reaches the + // page as the bytes the author typed. + {name: "ampersand inside a tag", in: ``, want: `<a href="?a=1&amp;b=2">`}, + + // Markdown structure is never escaped. Escaping it was the previous + // rule's undoing: a ">" turned into ">" stops opening a blockquote, + // so the indented code block inside collapsed into a paragraph and + // shipped its contents live (#37 H2). + { + name: "blockquote marker survives", + block: true, + in: "Quoted from the migration notes:\n\n> \n\nNothing after is affected.", + want: "Quoted from the migration notes:\n\n> \n\nNothing after is affected.", + }, + {name: "angle-bracket link destination", in: "[click]()", want: "[click]()"}, + {name: "autolink", in: "see ok", want: "see ok"}, + + // An HTML block opens on a tag that is not a complete one, and a value + // on a line can still begin a block: a scenario step is rendered as + // "1. " and then the step, so the step opens the list item's first + // block. Read only as inline content, "x", want: "<pre>x"}, + + // A backslash-escaped backtick is a literal character and opens + // nothing. The scanner this replaced read every backtick as a + // delimiter, so the tag between two of them shipped live (#37 F1). + { + name: "escaped backtick opens no span", + in: `Escaped tick: \` + "`" + ` then bold then \` + "`" + `.`, + want: `Escaped tick: \` + "`" + ` then <b>bold</b> then \` + "`" + `.`, + }, + // A code span cannot cross a paragraph break, so neither backtick + // opens one and the tag between them is ordinary inline HTML (#37 F2). + { + name: "span cannot cross a paragraph break", + block: true, + in: "A trailing ` marks it.\n\nLegacy importers emit .\n\nA second ` closes nothing.", + want: "A trailing ` marks it.\n\nLegacy importers emit <img src=x onerror=alert(1)>.\n\nA second ` closes nothing.", + }, + // Block-level code is literal in a block field: escaping there puts a + // visible "<" on the page, the fidelity bug ADR-0014 exists to + // avoid. A backtick fence survived the old scanner only by accident + // (#37 F3). + { + name: "tilde fence is verbatim", + block: true, + in: "Shape:\n\n~~~\n\n~~~", + want: "Shape:\n\n~~~\n\n~~~", + }, + { + name: "indented code block is verbatim", + block: true, + in: "Shape:\n\n ", + want: "Shape:\n\n ", + }, + { + name: "backtick fence is verbatim", + block: true, + in: "Shape:\n\n```\n\n```", + want: "Shape:\n\n```\n\n```", + }, + // Inside a line there is no block context to open, so what looks like + // a fence is text and the tag behind it is escaped. + {name: "fence opener inside a line", in: "```", want: "```<b>"}, + {name: "code fence info is not text", block: true, in: "```\nx\n```", want: "```<b>\nx\n```"}, + } + for _, tc := range cases { + got := escapeProse(tc.in, tc.block) + if got != tc.want { + t.Errorf("%s: escapeProse(%q, %t) = %q, want %q", tc.name, tc.in, tc.block, got, tc.want) + } + } +} + +// TestADR_0014_NoRawHTMLSurvivesRender is the whole-document form of the rule: +// take a model whose every prose field carries a payload that defeated the +// hand-rolled scanner, render it, and ask an independent CommonMark parser +// whether any raw HTML is left. This is the guard the old one should have been +// — it shared the scanner's model of Markdown, so it agreed with the bug. +func TestADR_0014_NoRawHTMLSurvivesRender(t *testing.T) { + t.Parallel() + + const tag = "" + esc := "`" + cases := []struct{ name, payload string }{ + {"plain tag", "Legacy importers emit " + tag + " in the notes."}, + // #37 F1: the escaped backtick opened a span the parser never sees. + {"escaped backtick", `A trailing \` + esc + ` marks it, then ` + tag + `, then \` + esc + `.`}, + // #37 F2: two bare backticks separated by a paragraph break. + {"paragraph break", "A trailing " + esc + " marks it.\n\n" + tag + " in the notes.\n\nA second " + esc + " here."}, + {"unclosed span", "A trailing " + esc + " then " + tag + "."}, + {"reference-encoded tag", "<img src=x onerror=alert(1)>"}, + {"html block", "
\ntext\n
"}, + // An HTML block opens on a tag that never completes, and a scenario step + // begins its list item's first block — so this is a block opener in one + // position and inline text in another. + {"unterminated closing tag", " 0 { + t.Errorf("raw HTML survived the render: %q\n%s", html, got) + } + }) + } +} + +// TestADR_0014_BlockCodeStaysVerbatim is the other half of ADR-0014: a code +// block in a block-level field must reach the page as the author wrote it. The +// old pass escaped inside a "~~~" fence and an indented block, putting the +// visible "<" on the page the rule exists to avoid (#37 F3). +func TestADR_0014_BlockCodeStaysVerbatim(t *testing.T) { + t.Parallel() + + const payload = "Shape:\n\n~~~\n\n~~~\n\nAnd indented:\n\n " + m := &model.Model{ + Entities: map[string]model.Entity{"Ticket": {Definition: payload}}, + } + got := render(m) + + if strings.Contains(got, "<policy") { + t.Errorf("escaped inside a code block, so the reader sees the escape:\n%s", got) + } + if !strings.Contains(got, "") || !strings.Contains(got, "") { + t.Errorf("a code block did not survive verbatim:\n%s", got) + } + // Verbatim is only safe because a real parser agrees it is code. + if html := rawHTML(t, got); len(html) > 0 { + t.Errorf("the code block reached the page as live HTML %q:\n%s", html, got) + } +} + +// TestADR_0014_ProseRendersHTMLAsText walks every prose-bearing field through a +// render and checks that raw HTML lands as visible text rather than as markup, +// while the Markdown those fields are written in still works. Before the fix +// each of these strings reached the document as live HTML. +func TestADR_0014_ProseRendersHTMLAsText(t *testing.T) { + t.Parallel() + + const tag = "" + m := &model.Model{ + Title: "Title " + tag, + Description: "Description " + tag, + Glossary: map[string]string{"Attendant": "Glossary " + tag}, + Enums: map[string]model.Enum{ + "Status": { + Description: "Enum " + tag, + Values: []model.EnumValue{{Name: "active", Definition: "Value " + tag}}, + }, + }, + Entities: map[string]model.Entity{ + "Ticket": { + Definition: "Definition " + tag + " with a `code ` and *emphasis*", + Derived: true, + Derivation: "Derivation " + tag, + Relationships: []model.Relationship{ + {Entity: "Policy", Cardinality: "1:n", Role: "Role " + tag, Note: "Note " + tag}, + }, + Attributes: []model.Attribute{ + {Name: "paid", Type: "map", Description: "Attribute " + tag}, + }, + Actions: []model.Action{ + {Name: "pay", Actor: "Attendant", Preserves: []string{"pre-" + tag}, Description: "Action " + tag}, + }, + Invariants: []model.Invariant{{ID: "ticket-paid", Statement: "Invariant " + tag}}, + }, + "Policy": {Definition: "A policy."}, + }, + Invariants: []model.Invariant{{ID: "model-rule", Statement: "Model invariant " + tag}}, + Scenarios: []model.Scenario{{ + Name: "Scenario " + tag, + Description: "Scenario description " + tag, + Actors: []string{"Actor " + tag}, + Steps: []string{"Step " + tag}, + InvariantsTouched: []string{"ticket-paid", "touched-" + tag}, + }}, + } + got := render(m) + + const escaped = "<img src=q onerror=alert(1)>" + // One occurrence per field above, plus the two invariant statements repeated + // under "Invariants touched" — every one of them escaped. + for _, field := range []string{ + "Title", "Description", "Glossary", "Enum", "Value", "Definition", + "Derivation", "Role", "Note", "Attribute", "Action", "Invariant", + "Model invariant", "Scenario", "Scenario description", "Actor", "Step", + "pre-", "touched-", + } { + if !strings.Contains(got, field+" "+escaped) && !strings.Contains(got, field+escaped) { + t.Errorf("%s field did not render its tag as text:\n%s", field, got) + } + } + if strings.Contains(got, tag) { + t.Errorf("a raw tag survived into the document:\n%s", got) + } + // The type column is prose too, and a conceptual type may hold angle + // brackets legitimately. " |") { + t.Errorf("expected the conceptual type verbatim:\n%s", got) + } + if html := rawHTML(t, got); len(html) > 0 { + t.Errorf("raw HTML survived the render: %q\n%s", html, got) + } + // Markdown in prose is the point of not escaping everything: the code span + // keeps its angle brackets literal and the emphasis still renders. + if !strings.Contains(got, "a `code ` and *emphasis*") { + t.Errorf("expected Markdown and its code span to survive intact:\n%s", got) + } +} + +// TestRender_UnnormalisedProseDoesNotPanic pins the guard in parseSource. +// goldmark measures a line's indent in columns and its length in bytes, so a +// final line a tab can outrun is taken for a blank one and the fenced-code-block +// parser indexes it at the -1 that marks one. Every input here lints clean and +// crashed `modelith render` with a Go stack trace before the trailing newline +// went in; the first is upstream yuin/goldmark#556's own repro, which v1.8.4 +// still panics on despite carrying the fix for it. +// +// Rendering has to stay sane, not merely survive: the payloads come back as the +// author typed them, and no raw HTML rides in on the workaround. +func TestRender_UnnormalisedProseDoesNotPanic(t *testing.T) { + t.Parallel() + + cases := []struct{ name, payload string }{ + {"upstream 556 repro", "*\n\t* \t~"}, + {"blockquote tab backtick", "A ticket.\n\n> \t`"}, + {"bare blockquote tab backtick", "> \t`"}, + {"blockquote tab tilde", "> \t~"}, + {"tab before a fence", "Shape:\n\n> \t```"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := &model.Model{ + Description: tc.payload, + Entities: map[string]model.Entity{ + "Ticket": {Definition: tc.payload}, + }, + Scenarios: []model.Scenario{{Name: "Pay", Description: tc.payload}}, + } + got := render(m) + if !strings.Contains(got, tc.payload) { + t.Errorf("payload %q did not reach the page verbatim:\n%s", tc.payload, got) + } + if html := rawHTML(t, got); len(html) > 0 { + t.Errorf("raw HTML survived the render: %q\n%s", html, got) + } + }) + } +} + +// TestADR_0014_AssembledLineEscapesAsOneLine is the regression for the +// cross-field pairing in #37: a `role:` and a `note:` are separate schema +// fields that share one rendered line, so escaping them separately let a stray +// backtick in the role pair with the note's backticks and leave the note's tag +// outside every code span, live. Whether a span is open is a property of the +// finished line, so the line is what gets escaped. +func TestADR_0014_AssembledLineEscapesAsOneLine(t *testing.T) { + t.Parallel() + + const tag = "" + m := &model.Model{ + Glossary: map[string]string{"Term": "a ` stray then `" + tag + "` here"}, + Entities: map[string]model.Entity{ + "Team": { + Definition: "A team.", + Relationships: []model.Relationship{ + {Entity: "Member", Cardinality: "n:n", Role: "`Owner or `Member`", Note: "Rendered as `" + tag + "` in the UI"}, + }, + Actions: []model.Action{ + {Name: "add", Preserves: []string{"a ` stray"}, Description: "Shown as `" + tag + "` there"}, + }, + Invariants: []model.Invariant{{ID: "one ` tick", Statement: "Holds for `" + tag + "` rows"}}, + }, + "Member": {Definition: "A member."}, + }, + Scenarios: []model.Scenario{{ + Name: "Join", + Actors: []string{"a ` stray", "the `" + tag + "` operator"}, + }}, + } + got := render(m) + + if html := rawHTML(t, got); len(html) > 0 { + t.Errorf("a field paired with its neighbour and shipped live HTML %q:\n%s", html, got) + } + if bare := textOutsideCode(t, got); strings.Contains(bare, " 0 { + t.Errorf("a name reached the page as live HTML %q:\n%s", html, got) + } + bare := textOutsideCode(t, got) + for _, tok := range []string{"" become HTML character references, in that order so an +// ampersand the author wrote is not read as part of a reference this function +// introduced. Mermaid builds its labels as HTML: a raw "" is parsed as a +// tag and disappears from the rendered diagram with no diagnostic, while the +// reference form is decoded back to the literal character. This is an encoding, +// not a visible escape — the reader sees exactly what the author typed. func sanitize(s string) string { s = strings.ReplaceAll(s, "`", "") s = strings.ReplaceAll(s, "\"", "'") s = strings.ReplaceAll(s, "\\", "") s = strings.ReplaceAll(s, "[", "(") s = strings.ReplaceAll(s, "]", ")") + s = percentRunRE.ReplaceAllString(s, "%") + s = strings.ReplaceAll(s, "&", "&") + s = strings.ReplaceAll(s, "<", "<") + s = strings.ReplaceAll(s, ">", ">") s = strings.ReplaceAll(s, "\n", " ") s = strings.ReplaceAll(s, "\r", " ") s = strings.ReplaceAll(s, "\t", " ") diff --git a/internal/render/mermaid/mermaid_test.go b/internal/render/mermaid/mermaid_test.go index 193b4c4..210b35a 100644 --- a/internal/render/mermaid/mermaid_test.go +++ b/internal/render/mermaid/mermaid_test.go @@ -600,22 +600,22 @@ func TestERDedupesSemanticallyEqualInverses(t *testing.T) { } } -// TestERRole_HostileCharacters pins current output for a role that packs -// every hostile character sanitize knows about (backticks, quotes, brackets, a -// literal newline) alongside characters it does NOT neutralize: '<', '>', and -// '%%'. This golden pins the CURRENT (buggy) behavior on purpose, not the -// desired one: fixing issue #29 must update this golden as part of that fix, -// not treat the diff as a regression. This test pins only the generated `.mmd` -// source text produced by ER() — it does not verify how a Mermaid renderer -// interprets that text. +// TestERRole_HostileCharacters pins the output for a role that packs every +// hostile character sanitize knows about — backticks, quotes, brackets, a +// literal newline, '<', '>', and '%%'. // -// The role also embeds a `%%{init: ...}%%`-shaped substring. Separately, a -// real render of that shape through mmdc was confirmed to parse successfully -// while silently changing the whole diagram's theme and dropping this edge's -// label text — a more severe manifestation of #29 than plain character -// passthrough into the label. This golden pins that the substring reaches the -// generated source unescaped; it does not itself exercise or assert the -// renderer-level effect, which was only checked by hand outside this test. +// This golden previously pinned the pre-#29 behavior on purpose, with '<', '>' +// and '%%' reaching the diagram source untouched. The fix for #29 replaced it: +// the angle brackets are now character references and the doubled percent signs +// are collapsed to single ones, which is what the diff against the old golden +// shows. +// +// The test asserts the generated `.mmd` source only. What a Mermaid renderer +// does with that source was checked separately against mmdc 11: with the +// doubled signs, the `%%{init: ...}%%` substring parsed as a configuration +// directive, restyled the whole diagram and swallowed the label; with single +// signs the diagram keeps its default theme and the text stays in the label. +// `task mermaid-check` feeds this file to the real parser. func TestERRole_HostileCharacters(t *testing.T) { t.Parallel() role := "he said \"hi\" [bracket] `tick` {brace} #hash %%pct %%{init: {'theme':'forest'}}%%\nsecond line" @@ -673,3 +673,50 @@ func TestER_UndeclaredRelationshipTarget(t *testing.T) { }} assertGolden(t, m, "testdata/undeclared_target.golden.mmd") } + +// TestSanitize_NeutralizesMermaidMetacharacters is the regression for issue +// #29, case by case. A directive needs doubled percent signs and a tag needs a +// raw angle bracket, so neutralizing exactly those is what closes the gap while +// leaving an ordinary role — including a lone "%" and a lone "&" — readable. +func TestSanitize_NeutralizesMermaidMetacharacters(t *testing.T) { + t.Parallel() + + cases := []struct{ name, in, want string }{ + {"ordinary role", "owns", "owns"}, + {"directive", "%%{init:{'theme':'dark'}}%%", "%{init:{'theme':'dark'}}%"}, + {"comment", "before %% after", "before % after"}, + {"longer run", "%%%%x", "%x"}, + {"lone percent survives", "50% full", "50% full"}, + {"angle brackets", "", "<angle>"}, + {"tag", "", "<img src=q onerror=alert(1)>"}, + {"ampersand encoded", "R&D", "R&D"}, + // The ampersand is escaped before the angle brackets, so a reference the + // author wrote is not fused with one this function introduced: mermaid + // decodes it back to the four characters "<". + {"pre-escaped reference", "<x>", "&lt;x&gt;"}, + {"backticks and quotes still go", "`a` \"b\" [c]", "a 'b' (c)"}, + {"newlines still collapse", "a\nb\tc", "a b c"}, + } + for _, tc := range cases { + if got := sanitize(tc.in); got != tc.want { + t.Errorf("%s: sanitize(%q) = %q, want %q", tc.name, tc.in, got, tc.want) + } + } +} + +// TestERSelfRow_HostileRole guards that a self-referential relationship, which +// renders as a row inside the entity block rather than as an edge, goes through +// the same sanitizing. It is a second call site (selfComment), and #29's gap +// was equally reachable from it. +func TestERSelfRow_HostileRole(t *testing.T) { + t.Parallel() + m := &model.Model{Entities: map[string]model.Entity{ + "Node": {Definition: "n", Relationships: []model.Relationship{ + {Entity: "Node", Cardinality: "n:n", Role: "%%{init:{'theme':'dark'}}%% "}, + }}, + }} + got := ER(m) + if want := `Node self "n:n — %{init:{'theme':'dark'}}% <angle>"`; !strings.Contains(got, want) { + t.Errorf("expected %q in:\n%s", want, got) + } +} diff --git a/internal/render/mermaid/testdata/hostile_role.golden.mmd b/internal/render/mermaid/testdata/hostile_role.golden.mmd index e9e86b5..bcf0322 100644 --- a/internal/render/mermaid/testdata/hostile_role.golden.mmd +++ b/internal/render/mermaid/testdata/hostile_role.golden.mmd @@ -1,4 +1,4 @@ erDiagram A {} B {} - A ||--o{ B : "he said 'hi' (bracket) tick {brace} #hash %%pct %%{init: {'theme':'forest'}}%% second line" + A ||--o{ B : "he said 'hi' (bracket) tick {brace} <angle> #hash %pct %{init: {'theme':'forest'}}% second line" diff --git a/project-docs/adr/0014-prose-is-markdown-not-html.md b/project-docs/adr/0014-prose-is-markdown-not-html.md new file mode 100644 index 0000000..9ec897b --- /dev/null +++ b/project-docs/adr/0014-prose-is-markdown-not-html.md @@ -0,0 +1,209 @@ +# A prose field is Markdown, not HTML + +A `definition:`, `description:`, `role:`, `note:`, invariant `statement:` or +scenario step is rendered as Markdown, and its raw HTML is rendered as visible +text rather than interpreted. An angle bracket the author typed appears on the +page as an angle bracket. Markdown — emphasis, and the backticked entity names +these fields are written in throughout — keeps working. + +## Context + +The renderer wrote prose fields into the Markdown document verbatim. Markdown +allows inline HTML, so `` in a `definition:` became a +tag in the published page, and no lint severity flagged it. + +Passing Markdown through is deliberate and has to stay. `lint`'s backticked-term +check expects prose to name entities in code spans, the schema tells authors to +backtick entity names in a role and an invariant statement, and the worked +example relies on both. So the choice was never "escape prose or don't" — it was +where inside prose the line falls. + +Today the hole is cosmetic: you wrote the model you render. ADR-0010's vendoring +slice renders models copied from repositories this one does not control into +local docs and a published site. At that point the author of the string needs +commit access to *their* repo, not yours, and this becomes a trust boundary. +ADR-0011 keeps `render` offline, so nothing else stands between a vendored file +and the page. + +## Decision + +**Escape the bytes a parser calls markup, and leave every other byte alone.** +The rule is an allowlist of what to escape rather than of what to spare: the +byte ranges of the raw-HTML nodes — `ast.RawHTML`, `ast.HTMLBlock` — have their +`<`, `>` and `&` replaced by character references, and nothing else in the +string is touched. A character reference is text to every Markdown parser; it +cannot become a tag. + +**The inverse rule was tried first, and it corrupted the document.** Collecting +the literal regions and escaping everything outside them meant escaping Markdown +*structure*. A `>` is a blockquote marker, so `>` stopped opening one: the +indented code block inside the quote — which the same parse had just called +literal — collapsed into a paragraph and shipped its contents live. Escaping +changed how the text parsed, which invalidated the offsets it was writing at. It +also killed the `<...>` around a link destination, leaving a dead href. Escaping +only what a parser recognised as markup has no such feedback: it removes angle +brackets and never adds one, so it cannot disturb the parse it came from. + +**`github.com/yuin/goldmark` decides what is markup.** The first attempt tracked +code spans with a character scanner, and a scanner is not a parser: it read a +backslash-escaped `` \` `` as a delimiter and let a span run across a paragraph +break, calling raw HTML literal in two ways CommonMark never would. Each corner +was found by someone looking for it. The next one would have shipped live. + +**Only the parse is borrowed, never the rendering.** goldmark locates the markup +and the escaping writes back into the original bytes at those offsets. Running +goldmark's renderer would normalize and reflow prose the author wrote and +rewrite every committed `.md`; the parser is also assembled directly from +`parser.NewParser`, so the HTML renderer never enters the binary. + +**A code fence's info string counts as markup.** It reaches the page as a class +attribute rather than as text, and it is inert to block structure, so escaping +it cannot disturb the parse and it keeps an unclosed fence from carrying a tag +on its opening line. + +**A rendered line is escaped once, as the whole line.** Whether a backtick opens +a code span is a property of the finished line, not of the field it came from. A +`role:` and the `note:` beside it are separate schema fields that share one +bullet, and escaping them separately let a stray backtick in the role pair with +the note's backticks and leave the note's tag outside every span, live. So the +line is assembled first, generated markup included, and escaped as the reader's +parser will see it. Table cells stay separate, because GFM splits a row into +cells before it parses any of them. + +**Where a value lands decides how it is parsed.** A description emitted as its +own block can hold a code block. A value on a line always follows something — +`- `, `# `, `| ` — so a leading four-space indent or a ` ``` ` run cannot open a +code block there. A line-context value is therefore parsed behind a stand-in for +that text: parsed alone it would look like a code block, and a parser reports +nothing inside one as HTML. + +**An `&` outside a tag is left alone.** Escaping the ones that introduced a +character reference was fidelity, not safety — a reference decodes to a +character and is re-escaped on output, so it can never produce markup — and it +is not worth a rule that reaches beyond the markup. `R&D`, `a & b` and a query +string pass into the committed Markdown byte-for-byte. Inside a tag the `&` is +escaped along with the brackets, so the tag reaches the page as it was typed. + +The Mermaid renderer reaches the same contract by a different encoding. Mermaid +builds labels as HTML and decodes references back to characters, so `sanitize` +escapes *every* `&` along with `<` and `>` — a round trip that never surfaces, +rather than a visible escape. Both renderers answer the same question the same +way: the reader sees the characters the author typed. + +## Considered and rejected + +- **Harden the scanner.** Teach it backslash escapes, then blank lines, then + fences. That is writing a CommonMark parser one bug report at a time, and the + failure mode of getting it wrong is a live tag on a published page. +- **Escape everything outside the literal regions.** The parse says what is + literal, so the complement looked like a safe default. It is not: the + complement contains the document's structure, and escaping structure changes + the parse the offsets came from. See the Decision above. +- **Escape every `<` and `>`, code spans included.** Simple and safe, and it + puts a visible `<` in every model that backticks a generic type. The + fidelity loss is the thing this decision is about. +- **Render through goldmark and emit its output.** It would be correct by + construction and it would rewrite the author's prose — reflowed paragraphs, + normalized emphasis, churn in every committed `.md` on every upgrade of the + library. +- **Vendor goldmark's table transformer to parse as GFM.** The parser + configuration here is CommonMark; every reader's is GFM. GitHub and Docusaurus + both apply the GFM extensions, and GFM's table transformer can dissolve a + paragraph into a table, so a CommonMark code span that spanned a newline never + forms and its contents land live. `goldmark/extension` cannot be added for its + parsers alone: every file in it imports the root `goldmark` package for the + `Extend` signature, whose eager `defaultMarkdown` global drags `renderer` and + `renderer/html` into the binary — measured, 27 renderer symbols against zero + today. Closing it means vendoring the transformer into a renderer-free + package. + + Rejected on cost against threat. Reaching this needs a model author who is + hostile *and* already has commit access to the repository being rendered — + who could edit the generated `.md` directly instead. Carrying a vendored copy + of someone else's parser internals, with its own licence and upgrade burden, + is not worth closing a hole that only opens when the author is a stranger. + + **That changes when vendoring lands.** ADR-0010's slice 2 renders models from + repositories this one does not control, which is the first time the author of + a prose string is not already trusted. The decision there is to say so rather + than to armour the renderer: `modelith deps import` and `deps update` warn + that vendoring carries an injection risk and that models should only be + vendored from sources you trust. In practice vendoring is expected to be + between projects that already trust each other. If that assumption stops + holding, revisit this rejection before adding more escaping. + +## Consequences + +- **modelith takes its first parsing dependency.** It ships as a single static + binary, so this is not free: goldmark is pure Go (no `import "C"`, builds + under `CGO_ENABLED=0`), only its `ast`, `parser`, `text` and `util` packages + link in, and the binary grows from 6.26 MB to 6.89 MB. Its `net` imports are + `net/url` and, through it, `net/netip` — parsers that perform no I/O, which + `TestADR_0011_OfflinePackages` allows by name. The offline boundary is + unchanged. +- **Markdown structure survives, because none of it is escaped.** A blockquote + stays a blockquote, `[click]()` keeps its href, and + `` stays an autolink. +- **A code block in a block-level field reaches the page verbatim.** A `~~~` + fence or an indented block in a `description:` or `definition:` is code, and + is left alone. In a table cell or a list item the same text is not, because it + could not open a block there. +- **An angle bracket that opens no tag is not markup and is not escaped.** + `map` and `1 < 2 && 3 > 2` reach the committed `.md` as typed and + render as themselves. +- **An author who wrote `<` now sees `<`.** The reference is decoded on the + page rather than shown. This is the fidelity the `&` rule used to buy, given + up to keep the escaping inside the markup. +- **An entity `derivation:` is collapsed onto one line.** It renders after + `**Derived:** `, so it is a line, not a block, and is escaped as one. +- **An author cannot embed raw HTML in a model.** No model did, and a domain + model is not a place to hand-write markup. Someone who needs a construct + Markdown lacks has to ask for it, which is the right conversation to have. +- **goldmark is handed a trailing newline it did not get from the author.** It + compares an indent measured in columns against a line's length in bytes, so a + final line a tab can outrun is taken for a blank one and the fenced-code-block + parser indexes it at the -1 that marks one — a panic on a model that lints + clean. Upstream yuin/goldmark#556 is the same -1 by another path; v1.8.4 + carries that fix and still panics on #556's own repro, so there is no release + to upgrade to. Appending the line ending a document is defined to have moves + no offset inside the string. `TestRender_UnnormalisedProseDoesNotPanic` pins + it. +- **No committed golden changed.** Nothing in `examples/` or + `docs/05-parking-garage/` holds an angle bracket or an ampersand in a prose + field, so this landed as a pure behavior change with no output churn — which + also means the goldens do not demonstrate it. + `TestADR_0014_ProseRendersHTMLAsText` walks every prose-bearing field through + a render; `TestADR_0014_NoRawHTMLSurvivesRender` asks a second parser whether + anything survived; `TestADR_0014_BlockCodeStaysVerbatim` pins the other + direction; `TestADR_0014_AssembledLineEscapesAsOneLine` pins the shared line. + +## Not decided here + +Each of these is a different class from raw HTML. Naming them is the point: the +rule above is about what a parser calls a tag, and none of these is one. + +- **A link's scheme.** `[click](javascript:alert(1))` and the autolink + `` both pass through, because a destination is Markdown + and not raw HTML. The allowlist made the autolink form survive where the + earlier rule had incidentally broken it, so this is now the live question + rather than a latent one. Constraining schemes is its own decision, with its + own list of what to allow, and it is not taken here. +- **MDX.** The docs are built by Docusaurus, whose MDX parser reads `<` followed + by a name character as a JSX tag. `map` and `` + both fail an `@mdx-js/mdx@3` compile, while `1 < 2 && 3 > 2` and an escaped + `<img src=q>` both pass. A `{` in prose still reaches the build as an + expression. This is unchanged from before the escaping existed — the same + bytes reach the page as they did — but the escaping does not close it either. +- **A prose block that leaves a code fence open.** A `definition:` of + ` ``` ` and then `` parses, on its own, as an unclosed + fenced block whose contents are code — so nothing in it is markup. In the + assembled document that fence pairs with the next one, which is the Mermaid + diagram's, and the tag lands outside any block and goes live. The field's own + parse is only authoritative while the field is self-contained, and an open + fence is exactly the case where it is not. The bytes are unchanged from before + the escaping existed. Refusing an unbalanced fence reads as a lint rule about + a malformed prose block rather than as a renderer escaping decision, and which + it should be is not settled here. +The parser-configuration gap — CommonMark here, GFM in every reader — *was* +open when this ADR was first written. It is now decided: see "Vendor goldmark's +table transformer to parse as GFM" under Considered and rejected.