From a796297cb9b236ac1fa82a5c04d8c31be710239c Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 18:07:31 -0700 Subject: [PATCH 01/10] fix(mermaid): neutralize directives and angle brackets in labels (#29) `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 `` 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 --- internal/render/mermaid/mermaid.go | 36 +++++++-- internal/render/mermaid/mermaid_test.go | 77 +++++++++++++++---- .../mermaid/testdata/hostile_role.golden.mmd | 2 +- 3 files changed, 93 insertions(+), 22 deletions(-) diff --git a/internal/render/mermaid/mermaid.go b/internal/render/mermaid/mermaid.go index c0a605e..46f23ec 100644 --- a/internal/render/mermaid/mermaid.go +++ b/internal/render/mermaid/mermaid.go @@ -5,6 +5,7 @@ package mermaid import ( "fmt" + "regexp" "strconv" "strings" @@ -240,19 +241,42 @@ func relationshipLabel(rel model.Relationship) string { return sanitize(rel.Role) } +// percentRunRE finds the doubled percent signs a Mermaid directive or comment +// is built from; see sanitize. +var percentRunRE = regexp.MustCompile(`%{2,}`) + // sanitize strips or replaces characters that would break a quoted Mermaid -// label. Square brackets are replaced with parentheses because Mermaid uses -// them for node/attribute syntax; backticks, quotes and backslashes are -// neutralized and newlines collapsed. Dropping the backslash also keeps the %q -// the callers emit with from doubling it into a visible "\\". Entity names -// interpolated elsewhere are constrained to PascalCase by the schema, so they -// need no escaping. +// label, so a role reaches the diagram as the text its author wrote. Entity +// names interpolated elsewhere are constrained to PascalCase by the schema, so +// they need no escaping. +// +// Square brackets are replaced with parentheses because Mermaid uses them for +// node/attribute syntax; backticks, quotes and backslashes are neutralized and +// newlines collapsed. Dropping the backslash also keeps the %q the callers emit +// with from doubling it into a visible "\\". +// +// A run of two or more percent signs collapses to one. Mermaid reads "%%{...}%%" +// anywhere in the source as a configuration directive, not as label text: a role +// carrying one restyles the whole diagram and loses its own label (issue #29). +// A directive needs the doubled sign, so a single "%" is inert and an ordinary +// "50% full" survives. +// +// "&", "<" and ">" 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" From 02e3f4f7233422b4a7e7009eb25656e17f4e22bf Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 18:07:45 -0700 Subject: [PATCH 02/10] fix(markdown): fence hostile code spans and render prose HTML as text (#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 `` 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 --- internal/render/markdown/markdown.go | 142 ++++++++++++++--- internal/render/markdown/markdown_test.go | 177 ++++++++++++++++++++++ 2 files changed, 294 insertions(+), 25 deletions(-) diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index 1cb787f..9a97f27 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -26,6 +26,10 @@ var ( qualifiedTypeRE = regexp.MustCompile(`^(` + model.ScopeSlug + `)\.([A-Z][A-Za-z0-9]*)$`) // backtickRunRE finds the backtick runs a code span's fence has to clear. backtickRunRE = regexp.MustCompile("`+") + // entityRefRE matches, anchored at an "&", the character references a + // Markdown parser decodes. Anything else beginning with "&" is an ordinary + // ampersand and is left alone; see prose. + entityRefRE = regexp.MustCompile(`^&(#[0-9]+|#[xX][0-9a-fA-F]+|[A-Za-z][A-Za-z0-9]*);`) ) // Render produces the full Markdown document for the model. sourceDir is the @@ -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", prose(oneLine(title))) if desc := strings.TrimSpace(m.Description); desc != "" { - b.WriteString(desc) + b.WriteString(prose(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** — %s\n", inv.ID, prose(oneLine(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`** — %s\n", t, prose(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(prose(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(prose(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", prose(derivation)) } b.WriteString(d) b.WriteString("\n\n") @@ -275,10 +279,10 @@ 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, prose(oneLine(rel.Role))) } if rel.Note != "" { - parts = append(parts, rel.Note) + parts = append(parts, prose(oneLine(rel.Note))) } fmt.Fprintf(b, "- %s\n", strings.Join(parts, " — ")) } @@ -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** — %s\n", inv.ID, prose(oneLine(inv.Statement))) } b.WriteString("\n") } @@ -336,7 +340,7 @@ 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, ", ")) return @@ -346,17 +350,21 @@ 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] = prose(oneLine(id)) + } + detail = append(detail, "preserves "+strings.Join(preserves, ", ")) } - line := "`" + a.Name + "`" + line := codeSpan(a.Name) if len(detail) > 0 { line += " — " + strings.Join(detail, "; ") } if a.Description != "" { - line += " — " + oneLine(a.Description) + line += " — " + prose(oneLine(a.Description)) } fmt.Fprintf(b, "- %s\n", line) } @@ -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", prose(oneLine(sc.Name))) if desc := strings.TrimSpace(sc.Description); desc != "" { - b.WriteString(desc) + b.WriteString(prose(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] = prose(oneLine(a)) + } + fmt.Fprintf(b, "**Actors:** %s\n\n", strings.Join(actors, ", ")) } 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, prose(oneLine(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** — %s\n", prose(oneLine(id)), prose(oneLine(s))) } else { - fmt.Fprintf(b, "- **%s**\n", id) + fmt.Fprintf(b, "- **%s**\n", prose(oneLine(id))) } } b.WriteString("\n") @@ -425,9 +437,89 @@ func renderScenarios(b *strings.Builder, m *model.Model) { } } -// mdCell escapes a value for use inside a Markdown table cell. +// mdCell escapes an author-written prose value for use inside a Markdown table +// cell. func mdCell(s string) string { - return strings.ReplaceAll(oneLine(s), "|", "\\|") + return cellEscape(prose(oneLine(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, "|", "\\|") +} + +// prose escapes raw HTML in an author-written string while leaving its Markdown +// intact. 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). +// +// Outside a code span, "<" and ">" become character references so they render +// as themselves. Inside one they are already literal, and escaping them would +// put a visible "<" on the page, so a code span is copied through whole. +// +// An "&" is escaped only where it introduces a character reference. Character +// references cannot produce markup — a Markdown parser decodes one to a +// character and re-escapes it on output — so this is fidelity, not safety: an +// author who wrote "<" sees "<", the same rule as one who wrote "<". A +// bare ampersand ("R&D", "a & b") already renders as itself and passes through +// untouched, which keeps the committed Markdown readable. +func prose(s string) string { + var b strings.Builder + for i := 0; i < len(s); { + switch c := s[i]; { + case c == '`': + end := codeSpanEnd(s, i) + b.WriteString(s[i:end]) + i = end + case c == '<': + b.WriteString("<") + i++ + case c == '>': + b.WriteString(">") + i++ + case c == '&' && entityRefRE.MatchString(s[i:]): + b.WriteString("&") + i++ + default: + b.WriteByte(c) + i++ + } + } + return b.String() +} + +// codeSpanEnd returns the index just past the code span opening with the +// backtick run at i. Per CommonMark a span closes at the next run of exactly +// the same length; with no such run the backticks are ordinary text, and the +// run alone is consumed. +func codeSpanEnd(s string, i int) int { + n := backtickRun(s, i) + for j := i + n; j < len(s); { + if s[j] != '`' { + j++ + continue + } + r := backtickRun(s, j) + if r == n { + return j + n + } + j += r + } + return i + n +} + +func backtickRun(s string, i int) int { + n := 0 + for i+n < len(s) && s[i+n] == '`' { + n++ + } + return n } // 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..db86a95 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -478,3 +478,180 @@ 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 +// code span and are left alone inside one, an ampersand is escaped only where +// it introduces a character reference, and Markdown is never touched. +func TestProse_EscapesHTMLOutsideCodeSpans(t *testing.T) { + t.Parallel() + + cases := []struct{ name, in, want string }{ + {"plain text", "A parking ticket.", "A parking ticket."}, + {"tag", "an tag", "an <img src=q onerror=alert(1)> tag"}, + {"markdown survives", "a `Ticket` with *emphasis*", "a `Ticket` with *emphasis*"}, + {"inside a code span", "a `` span", "a `` span"}, + {"in and out", " `` ", "<a> `` <c>"}, + {"unclosed span is text", "a ` tail", "a ` <b> tail"}, + {"double fence", "``a `` c`` ", "``a `` c`` <d>"}, + {"bare ampersand", "R&D and a & b", "R&D and a & b"}, + {"named reference", "<pre>", "&lt;pre&gt;"}, + {"decimal reference", "<", "&#60;"}, + {"hex reference", "<", "&#x3c;"}, + {"reference-shaped but unterminated", "< and &", "< and &"}, + {"reference inside a code span", "`<`", "`<`"}, + {"comparison", "1 < 2 && 3 > 2", "1 < 2 && 3 > 2"}, + {"multibyte passes through", "café — naïve ", "café — naïve <x>"}, + } + for _, tc := range cases { + if got := prose(tc.in); got != tc.want { + t.Errorf("%s: prose(%q) = %q, want %q", tc.name, tc.in, got, tc.want) + } + } +} + +// 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. + if !strings.Contains(got, "| map<string, int> |") { + t.Errorf("expected an escaped attribute type:\n%s", 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) + } +} + +// TestRenderCodeSpans_HostileNamesStayInsideTheirSpan is the regression for +// issue #35: a backtick in an unconstrained field closed its code span early +// and let the remainder land as live Markdown. Every one of these fields is a +// bare string in the schema, so only the renderer can hold the line. +func TestRenderCodeSpans_HostileNamesStayInsideTheirSpan(t *testing.T) { + t.Parallel() + + const breakout = "a ` `" + m := &model.Model{ + Enums: map[string]model.Enum{ + "Status": {Values: []model.EnumValue{{Name: breakout}, {Name: "pipe|value"}}}, + }, + Entities: map[string]model.Entity{ + "Ticket": { + Definition: "A stub.", + Attributes: []model.Attribute{{Name: breakout, Type: "string"}, {Name: "pipe|name", Type: "string"}}, + Actions: []model.Action{ + {Name: breakout}, + {Name: "structured " + breakout, Actor: "actor " + breakout}, + }, + }, + }, + } + got := render(m) + + // The fence widens to two backticks and the value is padded, so the whole + // string stays inside the span (CommonMark strips the padding on render). + const span = "`` a ` ` ``" + for _, want := range []string{ + "| " + span + " |", // enum value name + "| " + span + " | string |", // attribute name + "- `` structured a ` Date: Sun, 26 Jul 2026 18:08:21 -0700 Subject: [PATCH 03/10] docs(adr): a prose field is Markdown, not HTML (ADR-0014) 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 --- .../adr/0014-prose-is-markdown-not-html.md | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 project-docs/adr/0014-prose-is-markdown-not-html.md 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..fd03200 --- /dev/null +++ b/project-docs/adr/0014-prose-is-markdown-not-html.md @@ -0,0 +1,68 @@ +# 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 + +**`<` and `>` outside a code span become `<` and `>`.** That is the whole +security rule. A character reference is text to every Markdown parser; it cannot +become a tag. + +**Inside a code span nothing is escaped.** Markdown already treats a code span's +contents literally, so escaping there would put a visible `<x>` on the +page — the fidelity bug this decision exists to avoid, in the other direction. +The pass tracks code spans by CommonMark's rule: a run of *n* backticks closes at +the next run of exactly *n*, and an unclosed run is ordinary text. + +**An `&` is escaped only where it introduces a character reference** — +`&name;`, `<`, `<`. This is fidelity, not safety: a reference can never +produce markup, because a Markdown parser decodes one to a character and +re-escapes it on output. Escaping it makes an author who wrote `<` see +`<`, the same rule as one who wrote `<`. Leaving a bare `&` alone means +`R&D`, `a & b` and a query string render as themselves and pass into the +committed Markdown byte-for-byte, which keeps the generated file readable. + +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. + +## Consequences + +- **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. +- **An autolink written as `` renders as text.** Bare URLs + still autolink under GFM, so the loss is the pointy-bracket form only. +- **MDX expression syntax is untouched.** A `{` in prose still reaches the + Docusaurus build as an expression. That is a separate class from raw HTML and + is not decided here. +- **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` + does, walking every prose-bearing field through a render. From 1e45e499afb5d6f777d4afbf99bf7b0823404de6 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 18:42:38 -0700 Subject: [PATCH 04/10] fix(markdown): let a real parser decide what prose is literal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- go.mod | 1 + go.sum | 2 + internal/render/markdown/markdown.go | 202 ++++++---- internal/render/markdown/markdown_test.go | 430 +++++++++++++++++----- 4 files changed, 479 insertions(+), 156 deletions(-) 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 9a97f27..c256ad4 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" ) @@ -28,7 +32,7 @@ var ( backtickRunRE = regexp.MustCompile("`+") // entityRefRE matches, anchored at an "&", the character references a // Markdown parser decodes. Anything else beginning with "&" is an ordinary - // ampersand and is left alone; see prose. + // ampersand and is left alone; see escapeProse. entityRefRE = regexp.MustCompile(`^&(#[0-9]+|#[xX][0-9a-fA-F]+|[A-Za-z][A-Za-z0-9]*);`) ) @@ -48,10 +52,10 @@ func Render(m *model.Model, sourceDir, outDir string) string { if title == "" { title = "Domain Model" } - fmt.Fprintf(&b, "# %s\n\n", prose(oneLine(title))) + fmt.Fprintf(&b, "# %s\n\n", proseLine(title)) if desc := strings.TrimSpace(m.Description); desc != "" { - b.WriteString(prose(desc)) + b.WriteString(proseBlock(desc)) b.WriteString("\n\n") } @@ -76,7 +80,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, prose(oneLine(inv.Statement))) + fmt.Fprintf(b, "- **%s** — %s\n", inv.ID, proseLine(inv.Statement)) } b.WriteString("\n") } @@ -192,7 +196,7 @@ func renderGlossary(b *strings.Builder, m *model.Model) { } sort.Strings(terms) for _, t := range terms { - fmt.Fprintf(b, "- **`%s`** — %s\n", t, prose(oneLine(m.Glossary[t]))) + fmt.Fprintf(b, "- **`%s`** — %s\n", t, proseLine(m.Glossary[t])) } b.WriteString("\n") } @@ -212,7 +216,7 @@ 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(prose(d)) + b.WriteString(proseBlock(d)) b.WriteString("\n\n") } b.WriteString("| Value | Definition |\n") @@ -244,7 +248,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(prose(def)) + b.WriteString(proseBlock(def)) b.WriteString("\n\n") } @@ -262,7 +266,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", prose(derivation)) + d = fmt.Sprintf("**Derived:** %s", proseLine(derivation)) } b.WriteString(d) b.WriteString("\n\n") @@ -279,10 +283,10 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string parts = append(parts, rel.Ownership) } if rel.Role != "" { - parts = append(parts, prose(oneLine(rel.Role))) + parts = append(parts, proseLine(rel.Role)) } if rel.Note != "" { - parts = append(parts, prose(oneLine(rel.Note))) + parts = append(parts, proseLine(rel.Note)) } fmt.Fprintf(b, "- %s\n", strings.Join(parts, " — ")) } @@ -316,7 +320,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, prose(oneLine(inv.Statement))) + fmt.Fprintf(b, "- **%s** — %s\n", inv.ID, proseLine(inv.Statement)) } b.WriteString("\n") } @@ -355,7 +359,7 @@ func renderActions(b *strings.Builder, actions []model.Action) { if len(a.Preserves) > 0 { preserves := make([]string, len(a.Preserves)) for i, id := range a.Preserves { - preserves[i] = prose(oneLine(id)) + preserves[i] = proseLine(id) } detail = append(detail, "preserves "+strings.Join(preserves, ", ")) } @@ -364,7 +368,7 @@ func renderActions(b *strings.Builder, actions []model.Action) { line += " — " + strings.Join(detail, "; ") } if a.Description != "" { - line += " — " + prose(oneLine(a.Description)) + line += " — " + proseLine(a.Description) } fmt.Fprintf(b, "- %s\n", line) } @@ -400,17 +404,17 @@ func renderScenarios(b *strings.Builder, m *model.Model) { b.WriteString("## Scenarios\n\n") for _, sc := range m.Scenarios { - fmt.Fprintf(b, "### %s\n\n", prose(oneLine(sc.Name))) + fmt.Fprintf(b, "### %s\n\n", proseLine(sc.Name)) if desc := strings.TrimSpace(sc.Description); desc != "" { - b.WriteString(prose(desc)) + b.WriteString(proseBlock(desc)) b.WriteString("\n\n") } if len(sc.Actors) > 0 { actors := make([]string, len(sc.Actors)) for i, a := range sc.Actors { - actors[i] = prose(oneLine(a)) + actors[i] = proseLine(a) } fmt.Fprintf(b, "**Actors:** %s\n\n", strings.Join(actors, ", ")) } @@ -418,7 +422,7 @@ func renderScenarios(b *strings.Builder, m *model.Model) { if len(sc.Steps) > 0 { b.WriteString("**Steps**\n\n") for i, step := range sc.Steps { - fmt.Fprintf(b, "%d. %s\n", i+1, prose(oneLine(step))) + fmt.Fprintf(b, "%d. %s\n", i+1, proseLine(step)) } b.WriteString("\n") } @@ -427,9 +431,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", prose(oneLine(id)), prose(oneLine(s))) + fmt.Fprintf(b, "- **%s** — %s\n", proseLine(id), proseLine(s)) } else { - fmt.Fprintf(b, "- **%s**\n", prose(oneLine(id))) + fmt.Fprintf(b, "- **%s**\n", proseLine(id)) } } b.WriteString("\n") @@ -440,7 +444,7 @@ func renderScenarios(b *strings.Builder, m *model.Model) { // mdCell escapes an author-written prose value for use inside a Markdown table // cell. func mdCell(s string) string { - return cellEscape(prose(oneLine(s))) + return cellEscape(proseLine(s)) } // codeCell renders a value as a code span inside a Markdown table cell. GFM @@ -454,14 +458,48 @@ func cellEscape(s string) string { return strings.ReplaceAll(s, "|", "\\|") } -// prose escapes raw HTML in an author-written string while leaving its Markdown -// intact. 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). +// 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). // -// Outside a code span, "<" and ">" become character references so they render -// as themselves. Inside one they are already literal, and escaping them would -// put a visible "<" on the page, so a code span is copied through whole. +// 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 } + +// 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 literal there, so they are left 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. +// +// Only code spans are literal in that position. A leading "```" or four-space +// indent cannot open a code block partway through a line, so this string, +// parsed alone, may look like a code block where the document has ordinary +// text; treating it as one would leave the HTML behind it live. +func proseLine(s string) string { return escapeProse(oneLine(s), false) } + +// escapeProse escapes raw HTML in an author-written string while leaving its +// Markdown intact. 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). +// +// Outside a literal region, "<" and ">" become character references so they +// render as themselves. Inside one they are already literal, and escaping them +// would put a visible "<" on the page, so the region is copied through +// whole. // // An "&" is escaped only where it introduces a character reference. Character // references cannot produce markup — a Markdown parser decodes one to a @@ -469,57 +507,89 @@ func cellEscape(s string) string { // author who wrote "<" sees "<", the same rule as one who wrote "<". A // bare ampersand ("R&D", "a & b") already renders as itself and passes through // untouched, which keeps the committed Markdown readable. -func prose(s string) string { +func escapeProse(s string, blockContext bool) string { var b strings.Builder - for i := 0; i < len(s); { + i := 0 + for _, r := range literalRanges(s, blockContext) { + escapeMarkup(&b, s[i:r.start]) + b.WriteString(s[r.start:r.stop]) + i = r.stop + } + escapeMarkup(&b, s[i:]) + return b.String() +} + +// literalRanges returns the regions of s a CommonMark parser treats as literal +// text: the contents of code spans, and — in block context only — of fenced and +// indented code blocks. The ranges are sorted and do not overlap. +// +// The parse is what decides. A hand-rolled backtick scanner has no model of +// block structure or of backslash escapes, so it reads an escaped "\`" as a +// delimiter and lets a span run across a paragraph break — two ways to declare +// raw HTML literal that a real parser never would (#37). +// +// A code fence's info string is not returned: it is an attribute of the block +// rather than text on the page, so escaping it costs nothing and keeps an +// unclosed fence from carrying a tag on its opening line. +func literalRanges(s string, blockContext bool) []byteRange { + src := []byte(s) + var out []byteRange + appendSegments := func(segs *text.Segments) { + for i := 0; i < segs.Len(); i++ { + seg := segs.At(i) + out = append(out, byteRange{seg.Start, seg.Stop}) + } + } + // Walk never returns an error: the callback below returns none. + _ = ast.Walk(mdParser.Parse(text.NewReader(src)), func(n ast.Node, entering bool) (ast.WalkStatus, error) { + if !entering { + return ast.WalkContinue, nil + } + switch node := n.(type) { + case *ast.CodeSpan: + for c := node.FirstChild(); c != nil; c = c.NextSibling() { + if t, ok := c.(*ast.Text); ok { + out = append(out, byteRange{t.Segment.Start, t.Segment.Stop}) + } + } + case *ast.FencedCodeBlock: + if blockContext { + appendSegments(node.Lines()) + } + case *ast.CodeBlock: + if blockContext { + appendSegments(node.Lines()) + } + } + return ast.WalkContinue, nil + }) + + sort.Slice(out, func(i, j int) bool { return out[i].start < out[j].start }) + merged := out[:0] + prev := 0 + for _, r := range out { + if r.start < prev { + continue + } + merged = append(merged, r) + prev = r.stop + } + return merged +} + +func escapeMarkup(b *strings.Builder, s string) { + for i := 0; i < len(s); i++ { switch c := s[i]; { - case c == '`': - end := codeSpanEnd(s, i) - b.WriteString(s[i:end]) - i = end case c == '<': b.WriteString("<") - i++ case c == '>': b.WriteString(">") - i++ case c == '&' && entityRefRE.MatchString(s[i:]): b.WriteString("&") - i++ default: b.WriteByte(c) - i++ - } - } - return b.String() -} - -// codeSpanEnd returns the index just past the code span opening with the -// backtick run at i. Per CommonMark a span closes at the next run of exactly -// the same length; with no such run the backticks are ordinary text, and the -// run alone is consumed. -func codeSpanEnd(s string, i int) int { - n := backtickRun(s, i) - for j := i + n; j < len(s); { - if s[j] != '`' { - j++ - continue } - r := backtickRun(s, j) - if r == n { - return j + n - } - j += r - } - return i + n -} - -func backtickRun(s string, i int) int { - n := 0 - for i+n < len(s) && s[i+n] == '`' { - n++ } - return n } // 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 db86a95..dde0661 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. @@ -481,35 +609,160 @@ func TestRenderEntity_SubtypeHierarchy(t *testing.T) { // TestProse_EscapesHTMLOutsideCodeSpans pins the escaping rule ADR-0014 // records, case by case: angle brackets become character references outside a -// code span and are left alone inside one, an ampersand is escaped only where -// it introduces a character reference, and Markdown is never touched. +// 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 }{ - {"plain text", "A parking ticket.", "A parking ticket."}, - {"tag", "an tag", "an <img src=q onerror=alert(1)> tag"}, - {"markdown survives", "a `Ticket` with *emphasis*", "a `Ticket` with *emphasis*"}, - {"inside a code span", "a `` span", "a `` span"}, - {"in and out", " `` ", "<a> `` <c>"}, - {"unclosed span is text", "a ` tail", "a ` <b> tail"}, - {"double fence", "``a `` c`` ", "``a `` c`` <d>"}, - {"bare ampersand", "R&D and a & b", "R&D and a & b"}, - {"named reference", "<pre>", "&lt;pre&gt;"}, - {"decimal reference", "<", "&#60;"}, - {"hex reference", "<", "&#x3c;"}, - {"reference-shaped but unterminated", "< and &", "< and &"}, - {"reference inside a code span", "`<`", "`<`"}, - {"comparison", "1 < 2 && 3 > 2", "1 < 2 && 3 > 2"}, - {"multibyte passes through", "café — naïve ", "café — naïve <x>"}, + 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>"}, + {name: "bare ampersand", in: "R&D and a & b", want: "R&D and a & b"}, + {name: "named reference", in: "<pre>", want: "&lt;pre&gt;"}, + {name: "decimal reference", in: "<", want: "&#60;"}, + {name: "hex reference", in: "<", want: "&#x3c;"}, + {name: "reference-shaped but unterminated", in: "< and &", want: "< and &"}, + {name: "reference inside a code span", in: "`<`", want: "`<`"}, + {name: "comparison", in: "1 < 2 && 3 > 2", want: "1 < 2 && 3 > 2"}, + {name: "multibyte passes through", in: "café — naïve ", want: "café — naïve <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 { - if got := prose(tc.in); got != tc.want { - t.Errorf("%s: prose(%q) = %q, want %q", tc.name, tc.in, got, tc.want) + 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
"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := &model.Model{ + Description: tc.payload, + Glossary: map[string]string{"Term": tc.payload}, + Enums: map[string]model.Enum{ + "Status": {Description: tc.payload, Values: []model.EnumValue{{Name: "active", Definition: tc.payload}}}, + }, + Entities: map[string]model.Entity{ + "Ticket": { + Definition: tc.payload, + Derived: true, + Derivation: tc.payload, + Attributes: []model.Attribute{{Name: "rate", Type: "string", Description: tc.payload}}, + Actions: []model.Action{{Name: "pay", Description: tc.payload}}, + Invariants: []model.Invariant{{ID: "paid", Statement: tc.payload}}, + }, + }, + Scenarios: []model.Scenario{{Name: tc.payload, Description: tc.payload, Steps: []string{tc.payload}}}, + } + got := render(m) + if html := rawHTML(t, got); len(html) > 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 @@ -635,23 +888,20 @@ func TestRenderCodeSpans_HostileNamesStayInsideTheirSpan(t *testing.T) { t.Errorf("expected %q in:\n%s", want, inline) } - // Nothing hostile survives outside a code span. Strip every span and no - // fragment of the payload may remain. - for _, line := range strings.Split(got, "\n") { - bare := stripCodeSpans(line) - for _, tok := range []string{" 0 { + t.Errorf("a name reached the page as live HTML %q:\n%s", html, got) } - // A table row's cell count is what a broken-out pipe would change. - for _, line := range strings.Split(got, "\n") { - if !strings.HasPrefix(line, "| ") || strings.HasPrefix(line, "| ---") { - continue - } - if n := strings.Count(stripCodeSpans(line), "|"); n != 3 && n != 4 { - t.Errorf("row has %d unescaped pipes, want the header's count: %q", n, line) + bare := textOutsideCode(t, got) + for _, tok := range []string{" Date: Sun, 26 Jul 2026 18:44:17 -0700 Subject: [PATCH 05/10] docs(adr): re-derive ADR-0014 under goldmark MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../adr/0014-prose-is-markdown-not-html.md | 74 ++++++++++++++++--- 1 file changed, 63 insertions(+), 11 deletions(-) diff --git a/project-docs/adr/0014-prose-is-markdown-not-html.md b/project-docs/adr/0014-prose-is-markdown-not-html.md index fd03200..23cc005 100644 --- a/project-docs/adr/0014-prose-is-markdown-not-html.md +++ b/project-docs/adr/0014-prose-is-markdown-not-html.md @@ -27,15 +27,33 @@ and the page. ## Decision -**`<` and `>` outside a code span become `<` and `>`.** That is the whole -security rule. A character reference is text to every Markdown parser; it cannot -become a tag. +**`<` and `>` outside a literal region become `<` and `>`.** That is the +whole security rule. A character reference is text to every Markdown parser; it +cannot become a tag. -**Inside a code span nothing is escaped.** Markdown already treats a code span's -contents literally, so escaping there would put a visible `<x>` on the -page — the fidelity bug this decision exists to avoid, in the other direction. -The pass tracks code spans by CommonMark's rule: a run of *n* backticks closes at -the next run of exactly *n*, and an unclosed run is ordinary text. +**Inside a literal region nothing is escaped.** Markdown already treats a code +span's contents and a code block's lines literally, so escaping there would put +a visible `<x>` on the page — the fidelity bug this decision exists to +avoid, in the other direction. + +**`github.com/yuin/goldmark` decides which regions those are.** 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, declaring raw HTML literal in two ways CommonMark +never would, and it escaped inside `~~~` fences and indented blocks. 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 +literal regions 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. + +**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 — treating it as code would leave +the HTML behind it live. **An `&` is escaped only where it introduces a character reference** — `&name;`, `<`, `<`. This is fidelity, not safety: a reference can never @@ -51,18 +69,52 @@ 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 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. + ## 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 only `net` import + is `net/url`, which `TestADR_0011_OfflinePackages` allows as a parser that + performs no I/O; the offline boundary is unchanged. +- **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 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. - **An autolink written as `` renders as text.** Bare URLs - still autolink under GFM, so the loss is the pointy-bracket form only. + still autolink under GFM, so the loss is the pointy-bracket form only — + and with it goes ``, which CommonMark would otherwise + turn into a live link. +- **A Markdown link is still a Markdown link.** `[click](javascript:alert(1))` + in prose passes through, because it is Markdown and this decision is about + raw HTML. Whether a destination's scheme should be constrained is a separate + question and is not decided here. - **MDX expression syntax is untouched.** A `{` in prose still reaches the Docusaurus build as an expression. That is a separate class from raw HTML and is not decided here. - **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` - does, walking every prose-bearing field through a render. + 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 CommonMark + implementation whether anything survived; `TestADR_0014_BlockCodeStaysVerbatim` + pins the other direction. From d35ccc34a28aaeb77aac1a3ed06bde45d50a837e Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 19:32:04 -0700 Subject: [PATCH 06/10] fix(markdown): stop goldmark panicking on a prose field it calls blank MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- internal/render/markdown/markdown.go | 25 +++++++++++++++-- internal/render/markdown/markdown_test.go | 34 +++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index c256ad4..1274b2f 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -532,12 +532,14 @@ func escapeProse(s string, blockContext bool) string { // rather than text on the page, so escaping it costs nothing and keeps an // unclosed fence from carrying a tag on its opening line. func literalRanges(s string, blockContext bool) []byteRange { - src := []byte(s) + src := parseSource(s) var out []byteRange appendSegments := func(segs *text.Segments) { for i := 0; i < segs.Len(); i++ { seg := segs.At(i) - out = append(out, byteRange{seg.Start, seg.Stop}) + if stop := min(seg.Stop, len(s)); seg.Start < stop { + out = append(out, byteRange{seg.Start, stop}) + } } } // Walk never returns an error: the callback below returns none. @@ -577,6 +579,25 @@ func literalRanges(s string, blockContext bool) []byteRange { return merged } +// parseSource returns the bytes to hand the parser for s. 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 `modelith render` on a model that lints clean. +// +// A line ending keeps the comparison honest and costs nothing: a document is +// defined to end with one, and appending it moves no offset inside 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 parseSource(s string) []byte { + if !strings.HasSuffix(s, "\n") { + s += "\n" + } + return []byte(s) +} + func escapeMarkup(b *strings.Builder, s string) { for i := 0; i < len(s); i++ { switch c := s[i]; { diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index dde0661..09b2c7a 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -838,6 +838,40 @@ func TestADR_0014_ProseRendersHTMLAsText(t *testing.T) { } } +// 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. +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}}, + } + if html := rawHTML(t, render(m)); len(html) > 0 { + t.Errorf("raw HTML survived the render: %q", html) + } + }) + } +} + // TestRenderCodeSpans_HostileNamesStayInsideTheirSpan is the regression for // issue #35: a backtick in an unconstrained field closed its code span early // and let the remainder land as live Markdown. Every one of these fields is a From 36c10e959303701f0bca85e641bfe782a82b63a6 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 19:32:31 -0700 Subject: [PATCH 07/10] fix(markdown): escape what a parser calls HTML, not what it doesn't MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/render/markdown/markdown.go | 210 +++++++++++++--------- internal/render/markdown/markdown_test.go | 96 +++++++++- 2 files changed, 211 insertions(+), 95 deletions(-) diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index 1274b2f..8849ef1 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -30,10 +30,6 @@ var ( qualifiedTypeRE = regexp.MustCompile(`^(` + model.ScopeSlug + `)\.([A-Z][A-Za-z0-9]*)$`) // backtickRunRE finds the backtick runs a code span's fence has to clear. backtickRunRE = regexp.MustCompile("`+") - // entityRefRE matches, anchored at an "&", the character references a - // Markdown parser decodes. Anything else beginning with "&" is an ordinary - // ampersand and is left alone; see escapeProse. - entityRefRE = regexp.MustCompile(`^&(#[0-9]+|#[xX][0-9a-fA-F]+|[A-Za-z][A-Za-z0-9]*);`) ) // Render produces the full Markdown document for the model. sourceDir is the @@ -80,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, proseLine(inv.Statement)) + fmt.Fprintf(b, "- %s\n", invariantLine(inv.ID, inv.Statement)) } b.WriteString("\n") } @@ -196,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, proseLine(m.Glossary[t])) + fmt.Fprintf(b, "- %s\n", assembledLine("**"+codeSpan(t)+"**", oneLine(m.Glossary[t]))) } b.WriteString("\n") } @@ -283,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, proseLine(rel.Role)) + parts = append(parts, oneLine(rel.Role)) } if rel.Note != "" { - parts = append(parts, proseLine(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") } @@ -320,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, proseLine(inv.Statement)) + fmt.Fprintf(b, "- %s\n", invariantLine(inv.ID, inv.Statement)) } b.WriteString("\n") } @@ -346,7 +342,7 @@ func renderActions(b *strings.Builder, actions []model.Action) { for i, a := range actions { 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 } @@ -359,18 +355,18 @@ func renderActions(b *strings.Builder, actions []model.Action) { if len(a.Preserves) > 0 { preserves := make([]string, len(a.Preserves)) for i, id := range a.Preserves { - preserves[i] = proseLine(id) + preserves[i] = oneLine(id) } detail = append(detail, "preserves "+strings.Join(preserves, ", ")) } - line := codeSpan(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 += " — " + proseLine(a.Description) + parts = append(parts, oneLine(a.Description)) } - fmt.Fprintf(b, "- %s\n", line) + fmt.Fprintf(b, "- %s\n", assembledLine(parts...)) } b.WriteString("\n") } @@ -414,9 +410,9 @@ func renderScenarios(b *strings.Builder, m *model.Model) { if len(sc.Actors) > 0 { actors := make([]string, len(sc.Actors)) for i, a := range sc.Actors { - actors[i] = proseLine(a) + actors[i] = oneLine(a) } - fmt.Fprintf(b, "**Actors:** %s\n\n", strings.Join(actors, ", ")) + fmt.Fprintf(b, "%s\n\n", escapeProse("**Actors:** "+strings.Join(actors, ", "), false)) } if len(sc.Steps) > 0 { @@ -431,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", proseLine(id), proseLine(s)) + fmt.Fprintf(b, "- %s\n", invariantLine(id, s)) } else { - fmt.Fprintf(b, "- **%s**\n", proseLine(id)) + fmt.Fprintf(b, "- %s\n", escapeProse("**"+oneLine(id)+"**", false)) } } b.WriteString("\n") @@ -441,8 +437,30 @@ func renderScenarios(b *strings.Builder, m *model.Model) { } } +// 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. +// 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 cellEscape(proseLine(s)) } @@ -477,69 +495,83 @@ var mdParser = parser.NewParser( // 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 the text a line-context +// value follows on its rendered line: the "- ", "# ", "| " or "**Bold** — " +// that always precedes it. Those bytes hold column zero, where a four-space +// indent or a "```" run would otherwise open a code block. Parsed on its own +// the same value can look like a code block, and a parser reports nothing +// inside one as HTML — which would leave a tag live on a line that has no code +// block in it. 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 literal there, so they are left verbatim. +// 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. // -// Only code spans are literal in that position. A leading "```" or four-space -// indent cannot open a code block partway through a line, so this string, -// parsed alone, may look like a code block where the document has ordinary -// text; treating it as one would leave the HTML behind it live. +// 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 raw HTML in an author-written string while leaving its -// Markdown intact. A `definition:` is Markdown — models backtick their entity +// 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). // -// Outside a literal region, "<" and ">" become character references so they -// render as themselves. Inside one they are already literal, and escaping them -// would put a visible "<" on the page, so the region is copied through -// whole. -// -// An "&" is escaped only where it introduces a character reference. Character -// references cannot produce markup — a Markdown parser decodes one to a -// character and re-escapes it on output — so this is fidelity, not safety: an -// author who wrote "<" sees "<", the same rule as one who wrote "<". A -// bare ampersand ("R&D", "a & b") already renders as itself and passes through -// untouched, which keeps the committed Markdown readable. +// 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 literalRanges(s, blockContext) { - escapeMarkup(&b, s[i:r.start]) - b.WriteString(s[r.start:r.stop]) + for _, r := range ranges { + b.WriteString(s[i:r.start]) + escapeMarkup(&b, s[r.start:r.stop]) i = r.stop } - escapeMarkup(&b, s[i:]) + b.WriteString(s[i:]) return b.String() } -// literalRanges returns the regions of s a CommonMark parser treats as literal -// text: the contents of code spans, and — in block context only — of fenced and -// indented code blocks. The ranges are sorted and do not overlap. +// 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 reads an escaped "\`" as a -// delimiter and lets a span run across a paragraph break — two ways to declare -// raw HTML literal that a real parser never would (#37). +// 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 not returned: it is an attribute of the block -// rather than text on the page, so escaping it costs nothing and keeps an -// unclosed fence from carrying a tag on its opening line. -func literalRanges(s string, blockContext bool) []byteRange { - src := parseSource(s) +// 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. +func markupRanges(s string, blockContext bool) []byteRange { + src, shift := parseSource(s, blockContext) var out []byteRange - appendSegments := func(segs *text.Segments) { + 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) - if stop := min(seg.Stop, len(s)); seg.Start < stop { - out = append(out, byteRange{seg.Start, stop}) - } + add(seg.Start, seg.Stop) } } // Walk never returns an error: the callback below returns none. @@ -548,19 +580,16 @@ func literalRanges(s string, blockContext bool) []byteRange { return ast.WalkContinue, nil } switch node := n.(type) { - case *ast.CodeSpan: - for c := node.FirstChild(); c != nil; c = c.NextSibling() { - if t, ok := c.(*ast.Text); ok { - out = append(out, byteRange{t.Segment.Start, t.Segment.Stop}) - } + 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 blockContext { - appendSegments(node.Lines()) - } - case *ast.CodeBlock: - if blockContext { - appendSegments(node.Lines()) + if node.Info != nil { + add(node.Info.Segment.Start, node.Info.Segment.Stop) } } return ast.WalkContinue, nil @@ -579,33 +608,42 @@ func literalRanges(s string, blockContext bool) []byteRange { return merged } -// parseSource returns the bytes to hand the parser for s. 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 `modelith render` on a model that lints clean. +// parseSource returns the bytes to hand the parser for s and the offset its +// reported positions carry as a result, so a range in the parse maps back to +// s[start-shift : stop-shift]. // -// A line ending keeps the comparison honest and costs nothing: a document is -// defined to end with one, and appending it moves no offset inside 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 parseSource(s string) []byte { +// 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 parseSource(s string, blockContext bool) ([]byte, int) { + shift := 0 + if !blockContext { + s = linePrefix + s + shift = len(linePrefix) + } if !strings.HasSuffix(s, "\n") { s += "\n" } - return []byte(s) + return []byte(s), shift } +// 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]; { - case c == '<': + switch c := s[i]; c { + case '<': b.WriteString("<") - case c == '>': + case '>': b.WriteString(">") - case c == '&' && entityRefRE.MatchString(s[i:]): + case '&': b.WriteString("&") default: b.WriteByte(c) diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index 09b2c7a..6d85ea1 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -629,14 +629,37 @@ func TestProse_EscapesHTMLOutsideCodeSpans(t *testing.T) { {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: "&lt;pre&gt;"}, - {name: "decimal reference", in: "<", want: "&#60;"}, - {name: "hex reference", in: "<", want: "&#x3c;"}, + {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: "`<`"}, - {name: "comparison", in: "1 < 2 && 3 > 2", want: "1 < 2 && 3 > 2"}, + // 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"}, // A backslash-escaped backtick is a literal character and opens // nothing. The scanner this replaced read every backtick as a @@ -827,9 +850,14 @@ func TestADR_0014_ProseRendersHTMLAsText(t *testing.T) { 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. - if !strings.Contains(got, "| map<string, int> |") { - t.Errorf("expected an escaped attribute type:\n%s", got) + // 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. @@ -845,6 +873,9 @@ func TestADR_0014_ProseRendersHTMLAsText(t *testing.T) { // 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() @@ -865,13 +896,60 @@ func TestRender_UnnormalisedProseDoesNotPanic(t *testing.T) { }, Scenarios: []model.Scenario{{Name: "Pay", Description: tc.payload}}, } - if html := rawHTML(t, render(m)); len(html) > 0 { - t.Errorf("raw HTML survived the render: %q", html) + 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, " Date: Sun, 26 Jul 2026 19:35:45 -0700 Subject: [PATCH 08/10] docs(adr): re-derive ADR-0014 under the allowlist 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 ``, 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 --- .../adr/0014-prose-is-markdown-not-html.md | 156 ++++++++++++------ 1 file changed, 108 insertions(+), 48 deletions(-) diff --git a/project-docs/adr/0014-prose-is-markdown-not-html.md b/project-docs/adr/0014-prose-is-markdown-not-html.md index 23cc005..a4ae0ec 100644 --- a/project-docs/adr/0014-prose-is-markdown-not-html.md +++ b/project-docs/adr/0014-prose-is-markdown-not-html.md @@ -27,41 +27,62 @@ and the page. ## Decision -**`<` and `>` outside a literal region become `<` and `>`.** That is the -whole security rule. A character reference is text to every Markdown parser; it +**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. -**Inside a literal region nothing is escaped.** Markdown already treats a code -span's contents and a code block's lines literally, so escaping there would put -a visible `<x>` on the page — the fidelity bug this decision exists to -avoid, in the other direction. - -**`github.com/yuin/goldmark` decides which regions those are.** 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, declaring raw HTML literal in two ways CommonMark -never would, and it escaped inside `~~~` fences and indented blocks. Each corner +**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 -literal regions 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. - -**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 — treating it as code would leave -the HTML behind it live. - -**An `&` is escaped only where it introduces a character reference** — -`&name;`, `<`, `<`. This is fidelity, not safety: a reference can never -produce markup, because a Markdown parser decodes one to a character and -re-escapes it on output. Escaping it makes an author who wrote `<` see -`<`, the same rule as one who wrote `<`. Leaving a bare `&` alone means -`R&D`, `a & b` and a query string render as themselves and pass into the -committed Markdown byte-for-byte, which keeps the generated file readable. +**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` @@ -74,6 +95,10 @@ way: the reader sees the characters the author typed. - **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. @@ -87,34 +112,69 @@ way: the reader sees the characters the author typed. - **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 only `net` import - is `net/url`, which `TestADR_0011_OfflinePackages` allows as a parser that - performs no I/O; the offline boundary is unchanged. + 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. -- **An autolink written as `` renders as text.** Bare URLs - still autolink under GFM, so the loss is the pointy-bracket form only — - and with it goes ``, which CommonMark would otherwise - turn into a live link. -- **A Markdown link is still a Markdown link.** `[click](javascript:alert(1))` - in prose passes through, because it is Markdown and this decision is about - raw HTML. Whether a destination's scheme should be constrained is a separate - question and is not decided here. -- **MDX expression syntax is untouched.** A `{` in prose still reaches the - Docusaurus build as an expression. That is a separate class from raw HTML and - is not decided here. +- **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 CommonMark - implementation whether anything survived; `TestADR_0014_BlockCodeStaysVerbatim` - pins the other direction. + 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. +- **The parser configuration 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 this means vendoring the table transformer into a + renderer-free package, which is a dependency-surface decision of its own. From 494d86381d2064430a2d0fc65f24aacb7ddb761d Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 19:51:10 -0700 Subject: [PATCH 09/10] fix(markdown): read a line-context value both ways it can land MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 "