Skip to content

Commit f0f9f53

Browse files
committed
Refactor: Replace YamlHelper utility with YamlScalarAccessor trait
Migrated all security recipes from static YamlHelper utility methods to the YamlScalarAccessor trait pattern. This improves code organization and follows OpenRewrite best practices for LST manipulation. Changes: - Created YamlScalarAccessor trait with 6 default methods - Migrated 7 recipes to implement the trait - Deleted deprecated YamlHelper utility class - All tests pass
1 parent 52cd6c7 commit f0f9f53

16 files changed

+4122
-117
lines changed
Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,238 @@
1+
/*
2+
* MIGRATION EXAMPLE: TemplateInjectionRecipe using YamlScalarAccessor trait
3+
*
4+
* This file demonstrates what TemplateInjectionRecipe would look like after
5+
* migrating from YamlHelper utility class to YamlScalarAccessor trait.
6+
*
7+
* Key changes:
8+
* 1. Add "implements YamlScalarAccessor" to visitor class
9+
* 2. Replace "YamlHelper.getScalarValue(x)" with "getScalarValue(x)"
10+
* 3. No import changes needed (trait is an interface)
11+
*
12+
* Lines changed: 4 (lines 125, 146, 164 in original)
13+
* Import changes: 0
14+
* Behavior changes: 0 (identical functionality)
15+
*/
16+
17+
package org.openrewrite.github.security;
18+
19+
import lombok.EqualsAndHashCode;
20+
import lombok.Value;
21+
import org.jspecify.annotations.Nullable;
22+
import org.openrewrite.*;
23+
import org.openrewrite.marker.SearchResult;
24+
import org.openrewrite.yaml.JsonPathMatcher;
25+
import org.openrewrite.yaml.YamlIsoVisitor;
26+
import org.openrewrite.yaml.tree.Yaml;
27+
import org.openrewrite.github.traits.YamlScalarAccessor; // NEW IMPORT
28+
29+
import java.util.Arrays;
30+
import java.util.HashSet;
31+
import java.util.Set;
32+
import java.util.regex.Matcher;
33+
import java.util.regex.Pattern;
34+
35+
@Value
36+
@EqualsAndHashCode(callSuper = false)
37+
public class TemplateInjectionRecipe extends Recipe {
38+
39+
// User-controllable contexts that can lead to injection vulnerabilities
40+
private static final Set<String> DANGEROUS_CONTEXTS = new HashSet<>(Arrays.asList(
41+
"github.event.pull_request.title",
42+
"github.event.pull_request.body",
43+
"github.event.pull_request.head.ref",
44+
"github.event.pull_request.head.label",
45+
"github.event.pull_request.head.repo.default_branch",
46+
"github.event.pull_request.base.ref",
47+
"github.event.issue.title",
48+
"github.event.issue.body",
49+
"github.event.comment.body",
50+
"github.event.review.body",
51+
"github.event.pages[0].page_name",
52+
"github.event.commits[0].message",
53+
"github.event.head_commit.message",
54+
"github.event.commits[0].author.name",
55+
"github.event.commits[0].author.email",
56+
"github.head_ref"
57+
));
58+
59+
// Contexts that reference user-controllable step outputs
60+
private static final Pattern STEPS_OUTPUT_PATTERN = Pattern.compile(
61+
"steps\\.[^.]+\\.outputs\\.[^\\s}]+"
62+
);
63+
64+
// Actions known to have code injection sinks
65+
private static final Set<String> CODE_INJECTION_ACTIONS = new HashSet<>(Arrays.asList(
66+
"actions/github-script",
67+
"amadevus/pwsh-script",
68+
"jannekem/run-python-script-action",
69+
"cardinalby/js-eval-action"
70+
));
71+
72+
// Expression pattern to find GitHub Actions expressions
73+
private static final Pattern EXPRESSION_PATTERN = Pattern.compile(
74+
"\\$\\{\\{([^}]+)\\}\\}"
75+
);
76+
77+
@Override
78+
public String getDisplayName() {
79+
return "Find template injection vulnerabilities";
80+
}
81+
82+
@Override
83+
public String getDescription() {
84+
return "Find GitHub Actions workflows vulnerable to template injection attacks. These occur when user-controllable " +
85+
"input (like pull request titles, issue bodies, or commit messages) is used directly in `run` commands or " +
86+
"`script` inputs without proper escaping. Attackers can exploit this to execute arbitrary code. " +
87+
"Based on [zizmor's `template-injection` audit](https://github.com/woodruffw/zizmor/blob/main/crates/zizmor/src/audit/template_injection.rs).";
88+
}
89+
90+
@Override
91+
public TreeVisitor<?, ExecutionContext> getVisitor() {
92+
return Preconditions.check(
93+
new FindSourceFiles(".github/workflows/*.yml"),
94+
new TemplateInjectionVisitor()
95+
);
96+
}
97+
98+
// CHANGE: Added "implements YamlScalarAccessor"
99+
private static class TemplateInjectionVisitor extends YamlIsoVisitor<ExecutionContext>
100+
implements YamlScalarAccessor {
101+
102+
private static final JsonPathMatcher STEP_RUN_MATCHER = new JsonPathMatcher("$..steps[*].run");
103+
private static final JsonPathMatcher STEP_USES_MATCHER = new JsonPathMatcher("$..steps[*].uses");
104+
private static final JsonPathMatcher STEP_SCRIPT_MATCHER = new JsonPathMatcher("$..steps[*].with.script");
105+
106+
@Override
107+
public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionContext ctx) {
108+
Yaml.Mapping.Entry mappingEntry = super.visitMappingEntry(entry, ctx);
109+
110+
// Check run commands for injection vulnerabilities
111+
if (STEP_RUN_MATCHER.matches(getCursor())) {
112+
return checkRunEntry(mappingEntry);
113+
}
114+
115+
// Check uses entries for code injection actions
116+
if (STEP_USES_MATCHER.matches(getCursor())) {
117+
return checkUsesEntry(mappingEntry);
118+
}
119+
120+
// Check script inputs for code injection actions
121+
if (STEP_SCRIPT_MATCHER.matches(getCursor())) {
122+
return checkScriptEntry(mappingEntry);
123+
}
124+
125+
return mappingEntry;
126+
}
127+
128+
private Yaml.Mapping.Entry checkRunEntry(Yaml.Mapping.Entry entry) {
129+
// CHANGE: Removed "YamlHelper." prefix - now using trait method
130+
String runCommand = getScalarValue(entry.getValue());
131+
if (runCommand == null) {
132+
return entry;
133+
}
134+
135+
// Check for dangerous template expressions in run commands
136+
String vulnerableContext = findVulnerableContext(runCommand);
137+
if (vulnerableContext != null) {
138+
String message;
139+
if (vulnerableContext.startsWith("User-controlled input")) {
140+
message = "Potential template injection vulnerability. " + vulnerableContext + " used in run command without proper escaping.";
141+
} else {
142+
message = "Potential template injection vulnerability. User-controlled input '" + vulnerableContext + "' used in run command without proper escaping.";
143+
}
144+
return SearchResult.found(entry, message);
145+
}
146+
147+
return entry;
148+
}
149+
150+
private Yaml.Mapping.Entry checkUsesEntry(Yaml.Mapping.Entry entry) {
151+
// CHANGE: Removed "YamlHelper." prefix - now using trait method
152+
String usesValue = getScalarValue(entry.getValue());
153+
if (usesValue == null) {
154+
return entry;
155+
}
156+
157+
// Check if this is a code injection action
158+
for (String dangerousAction : CODE_INJECTION_ACTIONS) {
159+
if (usesValue.startsWith(dangerousAction)) {
160+
// Flag the use of a code injection action as potentially dangerous
161+
return SearchResult.found(entry,
162+
"Potential code injection in script input. User-controlled content in script execution context.");
163+
}
164+
}
165+
166+
return entry;
167+
}
168+
169+
private Yaml.Mapping.Entry checkScriptEntry(Yaml.Mapping.Entry entry) {
170+
// CHANGE: Removed "YamlHelper." prefix - now using trait method
171+
String scriptContent = getScalarValue(entry.getValue());
172+
if (scriptContent == null) {
173+
return entry;
174+
}
175+
176+
// Check for vulnerable contexts in the script content
177+
String vulnerableContext = findVulnerableContext(scriptContent);
178+
if (vulnerableContext != null) {
179+
String message;
180+
if (vulnerableContext.startsWith("User-controlled input")) {
181+
message = "Potential code injection in script. " + vulnerableContext + " used in script without proper escaping.";
182+
} else {
183+
message = "Potential code injection in script. User-controlled input '" + vulnerableContext +
184+
"' used in script without proper escaping.";
185+
}
186+
return SearchResult.found(entry, message);
187+
}
188+
189+
return entry;
190+
}
191+
192+
private @Nullable String findVulnerableContext(String content) {
193+
// Find all expressions in the content
194+
Matcher matcher = EXPRESSION_PATTERN.matcher(content);
195+
196+
while (matcher.find()) {
197+
String expression = matcher.group(1).trim();
198+
199+
// Check for complex expressions containing dangerous contexts first
200+
if (isComplexExpression(expression) && containsDangerousContextInExpression(expression)) {
201+
return "User-controlled input in complex expression";
202+
}
203+
204+
// Check for directly dangerous contexts
205+
for (String dangerousContext : DANGEROUS_CONTEXTS) {
206+
if (expression.contains(dangerousContext)) {
207+
return dangerousContext;
208+
}
209+
}
210+
211+
// Check for steps outputs (which may contain user input)
212+
Matcher stepsMatcher = STEPS_OUTPUT_PATTERN.matcher(expression);
213+
if (stepsMatcher.find()) {
214+
return stepsMatcher.group();
215+
}
216+
}
217+
218+
return null;
219+
}
220+
221+
private boolean isComplexExpression(String expression) {
222+
// Consider it complex if it contains function calls or multiple contexts
223+
return expression.contains("(") && expression.contains(")");
224+
}
225+
226+
private boolean containsDangerousContextInExpression(String expression) {
227+
// Check for function calls or complex expressions that might contain dangerous contexts
228+
for (String dangerousContext : DANGEROUS_CONTEXTS) {
229+
if (expression.contains(dangerousContext)) {
230+
return true;
231+
}
232+
}
233+
234+
return false;
235+
}
236+
237+
}
238+
}

0 commit comments

Comments
 (0)