fix(comment): prevent markdown injection from IaC tag values (#22)

* fix(comment): prevent markdown injection from IaC tag values

escapeAndFormatCode wrapped values in backticks without escaping. Tag
keys/values and attribute names come from user-controlled IaC and are
rendered as Markdown in PR comments, so a crafted value (e.g. a backtick
followed by a [link](...)) could close the code span and inject live
Markdown. All 29 tag/governance render sites route through this helper
(directly or via joinEscapedCode), so make it emit an injection-safe
code span: fence longer than any backtick run, newlines stripped. Output
is unchanged for ordinary values.

* fix(comment): escape user-controlled template leaf values

text/template applies no escaping, so template interpolations of
user-controlled values bypassed escapeAndFormatCode entirely: project
name/module path/workspace (cost table), project error name/output, and
the fixed-issues tag label were emitted raw into the HTML-flavoured
Markdown comment.

Escape them at the template boundary: values inside raw-HTML blocks
(<td>, <b>, <pre>) use the built-in `html` function (Markdown is not
processed there, so HTML escaping is the correct and sufficient guard);
the fixed-issues label, shown as inline code in a Markdown context, uses
the new mdCode template function (escapeAndFormatCode). Error output
stays in its <pre> code block with escaped content.

Author: liamg

pkg/vcs/comment/escape_test.go

@@ -0,0 +1,40 @@
+package comment
+
+import "testing"
+
+func TestEscapeAndFormatCode(t *testing.T) {
+	tests := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{name: "ordinary value", in: "environment", want: "`environment`"},
+		{name: "ordinary tag value", in: "prod", want: "`prod`"},
+		{name: "empty", in: "", want: "``"},
+		{
+			// A crafted tag value must stay inside the code span rather than
+			// closing it and injecting a link into the PR comment.
+			name: "injection payload is contained",
+			in:   "` - go [here](https://evil.example) to fix`",
+			want: "`` ` - go [here](https://evil.example) to fix` ``",
+		},
+		{name: "internal backtick widens the fence", in: "a`b", want: "``a`b``"},
+		{name: "newline collapsed to space", in: "a\nb", want: "`a b`"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := escapeAndFormatCode(tt.in); got != tt.want {
+				t.Errorf("escapeAndFormatCode(%q) = %q, want %q", tt.in, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestJoinEscapedCode(t *testing.T) {
+	got := joinEscapedCode([]string{"a", "b`c"})
+	want := "`a`, ``b`c``"
+	if got != want {
+		t.Errorf("joinEscapedCode = %q, want %q", got, want)
+	}
+}

pkg/vcs/comment/governance.go

@@ -720,9 +720,40 @@ func formatProjectNamesLabel(names []string) string {
 	return fmt.Sprintf("%s %s", noun, strings.Join(escaped, ", "))
 }
 
-// escapeAndFormatCode wraps a string in backticks for inline code display.
+// escapeAndFormatCode renders s as an inline code span that is safe against
+// Markdown injection. Values passed here originate from user-controlled IaC
+// (tag keys and values, attribute names, etc.) and are rendered into PR
+// comments, so a crafted value must not be able to break out of the code span
+// and inject links, images, or other live Markdown.
+//
+// It picks a backtick fence one longer than the longest run of backticks in s
+// and strips newlines (a code span cannot span lines). For ordinary values
+// with no backticks the output is the usual "`" + s + "`".
 func escapeAndFormatCode(s string) string {
-	return "`" + s + "`"
+	// Code spans cannot contain line endings.
+	s = strings.NewReplacer("\r\n", " ", "\n", " ", "\r", " ").Replace(s)
+
+	longest, current := 0, 0
+	for _, r := range s {
+		if r == '`' {
+			current++
+			if current > longest {
+				longest = current
+			}
+		} else {
+			current = 0
+		}
+	}
+	fence := strings.Repeat("`", longest+1)
+
+	// If the content starts or ends with a backtick, pad with a space so the
+	// boundary is not read as part of the fence; the renderer strips a single
+	// leading/trailing space.
+	if strings.HasPrefix(s, "`") || strings.HasSuffix(s, "`") {
+		s = " " + s + " "
+	}
+
+	return fence + s + fence
 }
 
 const (

pkg/vcs/comment/template.go

@@ -43,7 +43,15 @@ var templateFS embed.FS
 // or replace with their own. It renders HTML-flavoured Markdown suitable for
 // most platforms (GitHub, GitLab, Azure Repos).
 var DefaultTemplate = template.Must(
-	template.New("comment.tmpl").ParseFS(templateFS, "templates/*.tmpl"),
+	template.New("comment.tmpl").
+		Funcs(template.FuncMap{
+			// mdCode renders a user-controlled value as an injection-safe inline
+			// code span, for values shown as code in a Markdown context. Values
+			// rendered inside raw-HTML blocks (<td>, <pre>, <b>) use the built-in
+			// `html` function instead, which is the correct escaping there.
+			"mdCode": escapeAndFormatCode,
+		}).
+		ParseFS(templateFS, "templates/*.tmpl"),
 )
 
 // truncationBuffer is reserved inside maxCommentSize to cover both the

pkg/vcs/comment/templates/errors.tmpl

@@ -5,8 +5,8 @@
 
 <ul>
 {{- range .ProjectErrors }}
-<li><b>{{ .Name }}</b></li>
-<pre>{{ .Error }}</pre>
+<li><b>{{ .Name | html }}</b></li>
+<pre>{{ .Error | html }}</pre>
 {{- end }}
 </ul>
 <hr/>

pkg/vcs/comment/templates/fixed_issues.tmpl

@@ -2,7 +2,7 @@
 <details><summary>🟢 {{ .FixedIssuesSentence }}</summary>
 
 {{ range .FixedIssueCounts -}}
-- {{ .PolicyName }} `{{ .FixedIssuesLabel }}`
+- {{ .PolicyName | html }} {{ .FixedIssuesLabel | mdCode }}
 {{ end -}}
 {{ if .FixedIssuesTruncated -}}
 - ...and {{ .FixedIssuesTruncated }} more {{ if eq .FixedIssuesTruncated 1 }}issue{{ else }}issues{{ end }}

pkg/vcs/comment/templates/project_costs.tmpl

@@ -18,12 +18,12 @@
   <tbody>
   {{- range .CostTableEntries }}
     <tr>
-      <td>{{ .ProjectName }}</td>
+      <td>{{ .ProjectName | html }}</td>
       {{- if $.ShowModulePath }}
-      <td>{{ .ModulePath }}</td>
+      <td>{{ .ModulePath | html }}</td>
       {{- end }}
       {{- if $.ShowWorkspace }}
-      <td>{{ .Workspace }}</td>
+      <td>{{ .Workspace | html }}</td>
       {{- end }}
       <td align="right">{{ .BaselineCostChange }}</td>
       <td align="right">{{ .UsageCostChange }}</td>