From ac46f71f8d92da86905fc9ebd89eca6f1593fe72 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Wed, 15 Jul 2026 03:43:03 -0400 Subject: [PATCH 1/2] fix(visualize): escape labels, keep repo slugs injective, encode theme values Signed-off-by: Joshua Temple (cherry picked from commit 2353567d2c05b730e502616937ca4dc15adfdc92) --- internal/visualize/crossrepo.go | 18 +- internal/visualize/crossrepo_test.go | 8 +- internal/visualize/escaping_test.go | 164 ++++++++++++++++++ internal/visualize/mermaid.go | 47 ++++- internal/visualize/testdata/crossrepo.mmd | 24 +-- internal/visualize/testdata/env.mmd | 2 +- .../visualize/testdata/representative.mmd | 2 +- .../testdata/representative_bland.mmd | 2 +- internal/visualize/testdata/stages.mmd | 2 +- internal/visualize/theme.go | 19 +- internal/visualize/theme_test.go | 2 +- 11 files changed, 256 insertions(+), 34 deletions(-) create mode 100644 internal/visualize/escaping_test.go diff --git a/internal/visualize/crossrepo.go b/internal/visualize/crossrepo.go index 7aef48f..1215367 100644 --- a/internal/visualize/crossrepo.go +++ b/internal/visualize/crossrepo.go @@ -1,6 +1,8 @@ package visualize import ( + "crypto/sha256" + "encoding/hex" "fmt" "strings" @@ -94,18 +96,28 @@ func BuildCrossRepoViewModel(cfg *config.TrunkConfig) (ViewModel, error) { // repoSlug folds a repository reference (for example "org/cdk-infra") into a // Mermaid-safe identifier fragment by replacing every rune that is not a letter -// or digit with an underscore. The human-facing repo string is kept on the node -// or group label, so only the identity token is slugged. +// or digit with an underscore. Folding is lossy ("org/api-v2" and "org/api.v2" +// fold identically), so whenever any rune was folded a short content-hash +// suffix keeps the slug injective; without it two distinct repos would merge +// into one lane and one would silently disappear from the diagram. The +// human-facing repo string is kept on the node or group label, so only the +// identity token is slugged. func repoSlug(repo string) string { var b strings.Builder b.Grow(len(repo)) + folded := false for _, r := range repo { switch { case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': b.WriteRune(r) default: b.WriteByte('_') + folded = true } } - return b.String() + if !folded { + return b.String() + } + sum := sha256.Sum256([]byte(repo)) + return b.String() + "_" + hex.EncodeToString(sum[:4]) } diff --git a/internal/visualize/crossrepo_test.go b/internal/visualize/crossrepo_test.go index 3048da3..5b8b5bc 100644 --- a/internal/visualize/crossrepo_test.go +++ b/internal/visualize/crossrepo_test.go @@ -171,9 +171,11 @@ func TestMermaidEmitter_CrossRepoGolden(t *testing.T) { for _, want := range []string{ "flowchart TD", `subgraph primary["primary"]`, - `subgraph repo_org_cdk_infra["org/cdk-infra"]`, - `subgraph repo_org_web_edge["org/web-edge"]`, - `subgraph repo_org_platform["org/platform"]`, + // Slugged lane ids carry a short content-hash suffix so distinct repos + // that fold to the same alphanumeric text cannot merge into one lane. + `subgraph repo_org_cdk_infra_d8ebfd17["org/cdk-infra"]`, + `subgraph repo_org_web_edge_2d1cec96["org/web-edge"]`, + `subgraph repo_org_platform_e0d10933["org/platform"]`, "==>|cdk|", "==>|edge|", "-. notify .->", diff --git a/internal/visualize/escaping_test.go b/internal/visualize/escaping_test.go new file mode 100644 index 0000000..89c92af --- /dev/null +++ b/internal/visualize/escaping_test.go @@ -0,0 +1,164 @@ +package visualize + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/stablekernel/cascade/internal/config" +) + +// TestMermaidLabels_FoldNewlines proves a newline in a manifest-derived display +// name cannot split a node, state, or subgraph declaration across lines: a raw +// newline inside any label would terminate the Mermaid statement mid-declaration +// and the remainder would parse as diagram syntax. +func TestMermaidLabels_FoldNewlines(t *testing.T) { + t.Run("flowchart node label", func(t *testing.T) { + vm := ViewModel{ + Kind: DiagramFlowchart, + Nodes: []Node{{ID: "build-app", Label: "line one\nline two", Kind: NodeBuild}}, + } + out, err := NewMermaidEmitter().Emit(vm, Theme{}) + if err != nil { + t.Fatalf("Emit: %v", err) + } + if strings.Contains(out, "line one\nline two") { + t.Errorf("node label newline must be folded, got:\n%s", out) + } + if !strings.Contains(out, "line one line two") { + t.Errorf("folded label text missing, got:\n%s", out) + } + }) + + t.Run("subgraph label", func(t *testing.T) { + vm := ViewModel{ + Kind: DiagramFlowchart, + Nodes: []Node{{ID: "build-app", Label: "app", Kind: NodeBuild}}, + Groups: []Group{ + {ID: "lane", Label: "repo\nend\nevil --> injected", NodeIDs: []string{"build-app"}}, + }, + } + out, err := NewMermaidEmitter().Emit(vm, Theme{}) + if err != nil { + t.Fatalf("Emit: %v", err) + } + if strings.Contains(out, "repo\nend") { + t.Errorf("subgraph label newline must be folded, got:\n%s", out) + } + }) + + t.Run("state label", func(t *testing.T) { + vm := ViewModel{ + Kind: DiagramState, + Nodes: []Node{{ID: "dev", Label: "dev\nevil --> injected", Kind: NodeEnv}}, + } + out, err := NewMermaidEmitter().Emit(vm, Theme{}) + if err != nil { + t.Fatalf("Emit: %v", err) + } + if strings.Contains(out, "dev\nevil") { + t.Errorf("state label newline must be folded, got:\n%s", out) + } + }) +} + +// TestRepoSlug_DistinctReposStayDistinct proves two repos that fold to the same +// alphanumeric slug (org/api-v2 vs org/api.v2) still produce distinct lane and +// node identities, so neither lane is silently dropped or merged. +func TestRepoSlug_DistinctReposStayDistinct(t *testing.T) { + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Builds: []config.BuildConfig{ + {Name: "api", Workflow: ".github/workflows/build-api.yaml"}, + }, + External: []config.ExternalRepoConfig{ + {Repo: "org/api-v2", Deploys: []config.ExternalDeployConfig{ + {Name: "svc", Workflow: ".github/workflows/svc.yaml"}, + }}, + {Repo: "org/api.v2", Deploys: []config.ExternalDeployConfig{ + {Name: "svc", Workflow: ".github/workflows/svc.yaml"}, + }}, + }, + } + + vm, err := BuildCrossRepoViewModel(cfg) + if err != nil { + t.Fatalf("BuildCrossRepoViewModel: %v", err) + } + + groupIDs := make(map[string]bool) + for _, g := range vm.Groups { + if groupIDs[g.ID] { + t.Fatalf("duplicate group id %q: distinct repos must not collide", g.ID) + } + groupIDs[g.ID] = true + } + + nodeIDs := make(map[string]bool) + for _, n := range vm.Nodes { + if nodeIDs[n.ID] { + t.Fatalf("duplicate node id %q: distinct repos must not collide", n.ID) + } + nodeIDs[n.ID] = true + } +} + +// TestRepoSlug_PlainSlugStaysReadable proves a repo already made of safe runes +// keeps a readable slug with no hash suffix, so common diagrams stay clean. +func TestRepoSlug_PlainSlugStaysReadable(t *testing.T) { + if got := repoSlug("orgrepo"); got != "orgrepo" { + t.Errorf("repoSlug(orgrepo) = %q, want orgrepo", got) + } +} + +// TestThemeInit_ValuesAreJSONEncoded proves the init directive is real JSON: +// a theme value carrying a quote or backslash must be encoded, not spliced, +// or the directive breaks (and a crafted value could inject directive keys). +func TestThemeInit_ValuesAreJSONEncoded(t *testing.T) { + vm := ViewModel{ + Kind: DiagramFlowchart, + Nodes: []Node{{ID: "build-app", Label: "app", Kind: NodeBuild}}, + } + theme := Theme{Base: `ba"se`, LineColor: "#57606a\\"} + + out, err := NewMermaidEmitter().Emit(vm, theme) + if err != nil { + t.Fatalf("Emit: %v", err) + } + + start := strings.Index(out, "%%{init: ") + if start == -1 { + t.Fatalf("init directive missing:\n%s", out) + } + rest := out[start+len("%%{init: "):] + end := strings.Index(rest, "}%%") + if end == -1 { + t.Fatalf("init directive not closed:\n%s", out) + } + payload := rest[:end] + + var decoded map[string]any + if err := json.Unmarshal([]byte(payload), &decoded); err != nil { + t.Fatalf("init payload is not valid JSON: %v\npayload: %s", err, payload) + } + if decoded["theme"] != `ba"se` { + t.Errorf("theme value did not round-trip, got %v", decoded["theme"]) + } +} + +// TestClassDefBody_SanitizesUserThemeValues proves a user-theme color value +// cannot smuggle extra classDef attributes or statements: the comma, semicolon, +// and newline that would act as separators are stripped from the value. +func TestClassDefBody_SanitizesUserThemeValues(t *testing.T) { + body := classDefBody(NodeStyle{ + Fill: "red,stroke:#f00", + Stroke: "blue;classDef evil fill:#000", + Text: "white\nlinkStyle default stroke:#000", + }) + for _, forbidden := range []string{",stroke:#f00", ";", "\n", "\r"} { + if strings.Contains(body, forbidden) { + t.Errorf("classDef body must not contain %q, got %q", forbidden, body) + } + } +} diff --git a/internal/visualize/mermaid.go b/internal/visualize/mermaid.go index 87cf359..b346825 100644 --- a/internal/visualize/mermaid.go +++ b/internal/visualize/mermaid.go @@ -1,6 +1,7 @@ package visualize import ( + "encoding/json" "fmt" "strings" ) @@ -198,7 +199,9 @@ func emitState(b *strings.Builder, vm ViewModel, theme Theme) (string, error) { // writeThemeInit emits the Mermaid init directive carrying the theme's base and // edge color. It writes nothing when the theme sets neither, so an unstyled // theme produces no directive. The directive must follow any frontmatter block -// and precede the diagram keyword. +// and precede the diagram keyword. The payload is built with encoding/json, +// never string splicing: Go's %q emits Go escape syntax, not JSON, so a theme +// value with a quote or non-ASCII rune would produce an invalid directive. func writeThemeInit(b *strings.Builder, theme Theme) { if theme.Base == "" && theme.LineColor == "" { return @@ -207,11 +210,21 @@ func writeThemeInit(b *strings.Builder, theme Theme) { if base == "" { base = "base" } - fmt.Fprintf(b, "%%%%{init: {%q: %q", "theme", base) + + type initThemeVariables struct { + LineColor string `json:"lineColor"` + } + payload := struct { + Theme string `json:"theme"` + ThemeVariables *initThemeVariables `json:"themeVariables,omitempty"` + }{Theme: base} if theme.LineColor != "" { - fmt.Fprintf(b, ", %q: {%q: %q}", "themeVariables", "lineColor", theme.LineColor) + payload.ThemeVariables = &initThemeVariables{LineColor: theme.LineColor} } - b.WriteString("}}%%\n") + + // Marshaling a struct of strings cannot fail. + data, _ := json.Marshal(payload) + fmt.Fprintf(b, "%%%%{init: %s}%%%%\n", data) } // writeThemeClasses emits the theme's per-kind classDef lines followed by the @@ -276,9 +289,16 @@ func mermaidID(id string) string { // mermaidStateLabel renders a state's display name as a quoted Mermaid string // for the `state "name" as id` rename form, escaping any embedded quote so the -// alias declaration cannot be broken by punctuation in the name. +// alias declaration cannot be broken by punctuation in the name. Newlines are +// folded to spaces: a raw newline splits the declaration mid-statement and the +// remainder parses as diagram syntax. func mermaidStateLabel(label string) string { - return `"` + strings.ReplaceAll(label, `"`, """) + `"` + r := strings.NewReplacer( + `"`, """, + "\n", " ", + "\r", " ", + ) + return `"` + r.Replace(label) + `"` } // mermaidTransitionLabel sanitizes a transition caption. A colon would otherwise @@ -295,9 +315,16 @@ func mermaidTransitionLabel(label string) string { // mermaidSubgraphLabel renders a lane caption as a quoted Mermaid string for the // `subgraph id["label"]` form, escaping any embedded quote so a repo name with -// punctuation cannot break the subgraph header. +// punctuation cannot break the subgraph header. Newlines are folded to spaces: +// a raw newline splits the header mid-statement and the remainder parses as +// diagram syntax. func mermaidSubgraphLabel(label string) string { - return `"` + strings.ReplaceAll(label, `"`, """) + `"` + r := strings.NewReplacer( + `"`, """, + "\n", " ", + "\r", " ", + ) + return `"` + r.Replace(label) + `"` } // mermaidEdgeLabel sanitizes a flowchart edge caption. A pipe would close the @@ -318,6 +345,8 @@ func mermaidEdgeLabel(label string) string { // mermaidLabel escapes a display label for use inside a Mermaid node shape. // Brackets would otherwise close the shape early and quotes would confuse the // parser, so both are replaced with HTML entities Mermaid renders verbatim. +// Newlines are folded to spaces: a raw newline terminates the node statement +// mid-declaration and the remainder parses as diagram syntax. func mermaidLabel(label string) string { r := strings.NewReplacer( "[", "[", @@ -325,6 +354,8 @@ func mermaidLabel(label string) string { "(", "(", ")", ")", `"`, """, + "\n", " ", + "\r", " ", ) return r.Replace(label) } diff --git a/internal/visualize/testdata/crossrepo.mmd b/internal/visualize/testdata/crossrepo.mmd index ad21005..3c0ed90 100644 --- a/internal/visualize/testdata/crossrepo.mmd +++ b/internal/visualize/testdata/crossrepo.mmd @@ -1,7 +1,7 @@ --- title: "cross-repo" --- -%%{init: {"theme": "base", "themeVariables": {"lineColor": "#57606a"}}}%% +%%{init: {"theme":"base","themeVariables":{"lineColor":"#57606a"}}}%% flowchart TD subgraph primary["primary"] trunk(Trunk) @@ -10,25 +10,25 @@ flowchart TD promote(Promote) release(Release) end - subgraph repo_org_cdk_infra["org/cdk-infra"] - ext_org_cdk_infra_cdk[[cdk]] + subgraph repo_org_cdk_infra_d8ebfd17["org/cdk-infra"] + ext_org_cdk_infra_d8ebfd17_cdk[[cdk]] end - subgraph repo_org_web_edge["org/web-edge"] - ext_org_web_edge_edge[[edge]] + subgraph repo_org_web_edge_2d1cec96["org/web-edge"] + ext_org_web_edge_2d1cec96_edge[[edge]] end - subgraph repo_org_platform["org/platform"] - notify_org_platform[org/platform] + subgraph repo_org_platform_e0d10933["org/platform"] + notify_org_platform_e0d10933[org/platform] end trunk --> build build --> deploy deploy --> promote promote --> release - release ==>|cdk| ext_org_cdk_infra_cdk - release ==>|edge| ext_org_web_edge_edge - release -. notify .-> notify_org_platform + release ==>|cdk| ext_org_cdk_infra_d8ebfd17_cdk + release ==>|edge| ext_org_web_edge_2d1cec96_edge + release -. notify .-> notify_org_platform_e0d10933 classDef node_stage fill:#bf8700,stroke:#7d4e00,color:#ffffff classDef node_deploy fill:#8250df,stroke:#512a97,color:#ffffff classDef node_repo fill:#6e7781,stroke:#424a53,color:#ffffff class trunk,build,deploy,promote,release node_stage - class ext_org_cdk_infra_cdk,ext_org_web_edge_edge node_deploy - class notify_org_platform node_repo + class ext_org_cdk_infra_d8ebfd17_cdk,ext_org_web_edge_2d1cec96_edge node_deploy + class notify_org_platform_e0d10933 node_repo diff --git a/internal/visualize/testdata/env.mmd b/internal/visualize/testdata/env.mmd index 930382c..8bb0265 100644 --- a/internal/visualize/testdata/env.mmd +++ b/internal/visualize/testdata/env.mmd @@ -1,7 +1,7 @@ --- title: "environments" --- -%%{init: {"theme": "base", "themeVariables": {"lineColor": "#57606a"}}}%% +%%{init: {"theme":"base","themeVariables":{"lineColor":"#57606a"}}}%% stateDiagram-v2 state "hotfix/login" as staging_hotfix [*] --> dev diff --git a/internal/visualize/testdata/representative.mmd b/internal/visualize/testdata/representative.mmd index 337fff8..390886a 100644 --- a/internal/visualize/testdata/representative.mmd +++ b/internal/visualize/testdata/representative.mmd @@ -1,7 +1,7 @@ --- title: "pipeline" --- -%%{init: {"theme": "base", "themeVariables": {"lineColor": "#57606a"}}}%% +%%{init: {"theme":"base","themeVariables":{"lineColor":"#57606a"}}}%% flowchart TD validate([Validate (validate)]) build_api[Build (api)] diff --git a/internal/visualize/testdata/representative_bland.mmd b/internal/visualize/testdata/representative_bland.mmd index 61f7362..7eeed54 100644 --- a/internal/visualize/testdata/representative_bland.mmd +++ b/internal/visualize/testdata/representative_bland.mmd @@ -1,7 +1,7 @@ --- title: "pipeline" --- -%%{init: {"theme": "neutral", "themeVariables": {"lineColor": "#8c959f"}}}%% +%%{init: {"theme":"neutral","themeVariables":{"lineColor":"#8c959f"}}}%% flowchart TD validate([Validate (validate)]) build_api[Build (api)] diff --git a/internal/visualize/testdata/stages.mmd b/internal/visualize/testdata/stages.mmd index 0ec7963..a3efc11 100644 --- a/internal/visualize/testdata/stages.mmd +++ b/internal/visualize/testdata/stages.mmd @@ -1,7 +1,7 @@ --- title: "stages" --- -%%{init: {"theme": "base", "themeVariables": {"lineColor": "#57606a"}}}%% +%%{init: {"theme":"base","themeVariables":{"lineColor":"#57606a"}}}%% flowchart TD trunk(Trunk) build(Build) diff --git a/internal/visualize/theme.go b/internal/visualize/theme.go index c10888e..1870053 100644 --- a/internal/visualize/theme.go +++ b/internal/visualize/theme.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "os" + "strings" ) // NodeStyle holds the presentation attributes an emitter renders for one node @@ -129,20 +130,32 @@ func className(kind NodeKind) string { // classDefBody renders a NodeStyle as the attribute list of a Mermaid classDef, // emitting only the set fields in a fixed order so the output is deterministic. +// Values are sanitized before splicing: a user-theme color carrying a comma, +// semicolon, or newline would otherwise smuggle extra classDef attributes or +// whole statements into the diagram. func classDefBody(s NodeStyle) string { parts := make([]string, 0, 3) if s.Fill != "" { - parts = append(parts, "fill:"+s.Fill) + parts = append(parts, "fill:"+classDefValue(s.Fill)) } if s.Stroke != "" { - parts = append(parts, "stroke:"+s.Stroke) + parts = append(parts, "stroke:"+classDefValue(s.Stroke)) } if s.Text != "" { - parts = append(parts, "color:"+s.Text) + parts = append(parts, "color:"+classDefValue(s.Text)) } return joinComma(parts) } +// classDefValueReplacer strips the runes that act as classDef separators (comma +// between attributes, semicolon and newline between statements) from a style +// value, so a theme file controls only the one attribute it names. +var classDefValueReplacer = strings.NewReplacer(",", "", ";", "", "\n", "", "\r", "") + +func classDefValue(v string) string { + return classDefValueReplacer.Replace(v) +} + // joinComma joins parts with a comma. It exists so classDefBody stays free of an // import for a single call and keeps the attribute separator in one place. func joinComma(parts []string) string { diff --git a/internal/visualize/theme_test.go b/internal/visualize/theme_test.go index 872859a..987d60d 100644 --- a/internal/visualize/theme_test.go +++ b/internal/visualize/theme_test.go @@ -162,7 +162,7 @@ func TestLoadTheme_AppliesStyling(t *testing.T) { if !strings.Contains(out, "fill:#123456") { t.Errorf("custom fill not applied in:\n%s", out) } - if !strings.Contains(out, `"lineColor": "#abcdef"`) { + if !strings.Contains(out, `"lineColor":"#abcdef"`) { t.Errorf("custom line color not applied in:\n%s", out) } } From 96620da192c9081a13abf7fc6ff4d01c5ed658e6 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Wed, 15 Jul 2026 04:03:02 -0400 Subject: [PATCH 2/2] test(visualize): align graph and CLI assertions with encoded theme output Signed-off-by: Joshua Temple (cherry picked from commit a4dc64320e6f6eeac9ac6d6f7b17660f98cdaf95) --- cmd/cascade/main_test.go | 4 ++-- internal/graph/graph_test.go | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cmd/cascade/main_test.go b/cmd/cascade/main_test.go index 143affc..e2d5002 100644 --- a/cmd/cascade/main_test.go +++ b/cmd/cascade/main_test.go @@ -943,7 +943,7 @@ func TestGraphCommand_BlandThemeStylesNodes(t *testing.T) { // color, and grayscale node fills. These tokens are absent from the branded // cascade default, so their presence proves the bland theme reached the // emitter rather than the default palette. - for _, want := range []string{`"theme": "neutral"`, "#8c959f", "#f6f8fa"} { + for _, want := range []string{`"theme":"neutral"`, "#8c959f", "#f6f8fa"} { if !contains(stdout, want) { t.Errorf("expected bland token %q in output, got:\n%s", want, stdout) } @@ -983,7 +983,7 @@ func TestGraphCommand_CustomThemeFileStylesNodes(t *testing.T) { if payload.Theme != "midnight" { t.Errorf("expected theme name midnight, got %q", payload.Theme) } - for _, want := range []string{`"theme": "dark"`, "#abcdef", "fill:#123456"} { + for _, want := range []string{`"theme":"dark"`, "#abcdef", "fill:#123456"} { if !contains(payload.Diagram, want) { t.Errorf("expected custom theme token %q in diagram, got:\n%s", want, payload.Diagram) } diff --git a/internal/graph/graph_test.go b/internal/graph/graph_test.go index e787f74..e5cd81f 100644 --- a/internal/graph/graph_test.go +++ b/internal/graph/graph_test.go @@ -189,10 +189,12 @@ func TestRun_CrossRepoGranularity_EmitsLanesAndEdges(t *testing.T) { got := out.String() require.Contains(t, got, "flowchart TD") - // A lane per repository. + // A lane per repository. Slugged lane ids carry a short content-hash + // suffix so distinct repos that fold to the same alphanumeric text cannot + // merge into one lane. require.Contains(t, got, `subgraph primary["primary"]`) - require.Contains(t, got, `subgraph repo_org_cdk_infra["org/cdk-infra"]`) - require.Contains(t, got, `subgraph repo_org_platform["org/platform"]`) + require.Contains(t, got, `subgraph repo_org_cdk_infra_d8ebfd17["org/cdk-infra"]`) + require.Contains(t, got, `subgraph repo_org_platform_e0d10933["org/platform"]`) // Cross-repo edges in both directions: primary coordinates the dependent and // the local pipeline notifies its upstream primary. require.Contains(t, got, "==>|cdk|") @@ -249,7 +251,7 @@ func TestRun_BlandTheme_StylesOutput(t *testing.T) { got := out.String() require.Contains(t, got, "flowchart TD") // The bland theme sets a neutral Mermaid base and its own line color. - require.Contains(t, got, `"theme": "neutral"`) + require.Contains(t, got, `"theme":"neutral"`) require.Contains(t, got, "classDef node_validate") }