Skip to content

Refactor: Replace YamlHelper utility with YamlScalarAccessor trait #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Refactor: Replace YamlHelper utility with YamlScalarAccessor trait #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47ff257
Refactor: Replace YamlHelper utility with YamlScalarAccessor trait
sjungling Oct 15, 2025
797700b
Rename traits package to util
sjungling Oct 15, 2025
741211c
refactor: explore usages of traits
sjungling Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/main/java/org/openrewrite/github/security/ArtifactSecurityRecipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import lombok.Value;
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.github.util.YamlScalarAccessor;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.yaml.JsonPathMatcher;
import org.openrewrite.yaml.YamlIsoVisitor;
Expand Down Expand Up @@ -78,7 +79,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
);
}

private static class ArtifactSecurityVisitor extends YamlIsoVisitor<ExecutionContext> {
private static class ArtifactSecurityVisitor extends YamlIsoVisitor<ExecutionContext> implements YamlScalarAccessor {

private static final JsonPathMatcher STEP_USES_MATCHER = new JsonPathMatcher("$..steps[*].uses");

Expand Down Expand Up @@ -108,7 +109,7 @@ public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionC


private Yaml.Mapping.Entry checkUsesEntry(Yaml.Mapping.Entry entry) {
String usesValue = YamlHelper.getScalarValue(entry.getValue());
String usesValue = getScalarValue(entry.getValue());
if (usesValue == null) {
return entry;
}
Expand All @@ -133,7 +134,7 @@ private Yaml.Mapping.Entry checkCheckoutAction(Yaml.Mapping.Entry entry) {
return entry;
}

String persistCredentials = YamlHelper.findNestedScalarValue(stepMapping, "with", "persist-credentials");
String persistCredentials = findNestedScalarValue(stepMapping, "with", "persist-credentials");

if (persistCredentials == null) {
// No 'with' section or no persist-credentials means default behavior (persist-credentials: true)
Expand All @@ -159,7 +160,7 @@ private Yaml.Mapping.Entry checkUploadArtifactAction(Yaml.Mapping.Entry entry) {
return entry;
}

String pathValue = YamlHelper.findNestedScalarValue(stepMapping, "with", "path");
String pathValue = findNestedScalarValue(stepMapping, "with", "path");
if (pathValue != null && hasDangerousArtifactPaths(pathValue)) {
return SearchResult.found(entry,
"Uploading potentially sensitive paths that may contain credentials or configuration files.");
Expand Down
15 changes: 8 additions & 7 deletions src/main/java/org/openrewrite/github/security/CachePoisoningRecipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.openrewrite.ExecutionContext;
import org.openrewrite.Recipe;
import org.openrewrite.TreeVisitor;
import org.openrewrite.github.util.YamlScalarAccessor;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.yaml.YamlIsoVisitor;
import org.openrewrite.yaml.tree.Yaml;
Expand Down Expand Up @@ -90,7 +91,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
return new CachePoisoningVisitor();
}

private static class CachePoisoningVisitor extends YamlIsoVisitor<ExecutionContext> {
private static class CachePoisoningVisitor extends YamlIsoVisitor<ExecutionContext> implements YamlScalarAccessor {

private boolean isPublishingWorkflow = false;
private boolean hasPublisherAction = false;
Expand Down Expand Up @@ -131,14 +132,14 @@ private void analyzeWorkflow(Yaml.Document document) {
}

private boolean isPublishingTrigger(Yaml.Block onValue) {
String scalarTrigger = YamlHelper.getScalarValue(onValue);
String scalarTrigger = getScalarValue(onValue);
if (scalarTrigger != null) {
return "release".equals(scalarTrigger);
}
if (onValue instanceof Yaml.Sequence) {
Yaml.Sequence sequence = (Yaml.Sequence) onValue;
for (Yaml.Sequence.Entry seqEntry : sequence.getEntries()) {
String trigger = YamlHelper.getScalarValue(seqEntry.getBlock());
String trigger = getScalarValue(seqEntry.getBlock());
if ("release".equals(trigger)) {
return true;
}
Expand Down Expand Up @@ -184,7 +185,7 @@ private boolean hasReleaseBranches(Yaml.Block branchesValue) {
if (branchesValue instanceof Yaml.Sequence) {
Yaml.Sequence sequence = (Yaml.Sequence) branchesValue;
for (Yaml.Sequence.Entry entry : sequence.getEntries()) {
String branch = YamlHelper.getScalarValue(entry.getBlock());
String branch = getScalarValue(entry.getBlock());
if (branch != null && RELEASE_BRANCH_PATTERN.matcher(branch).matches()) {
return true;
}
Expand Down Expand Up @@ -230,7 +231,7 @@ private boolean jobHasPublisherAction(Yaml.Mapping jobMapping) {
private boolean stepUsesPublisherAction(Yaml.Mapping stepMapping) {
for (Yaml.Mapping.Entry entry : stepMapping.getEntries()) {
if ("uses".equals(entry.getKey().getValue())) {
String uses = YamlHelper.getScalarValue(entry.getValue());
String uses = getScalarValue(entry.getValue());
if (uses != null) {
String actionName = extractActionName(uses);
return PUBLISHER_ACTIONS.contains(actionName);
Expand Down Expand Up @@ -262,7 +263,7 @@ private boolean isCacheAwareActionStep(Yaml.Mapping.Entry entry) {
return false;
}

String uses = YamlHelper.getScalarValue(entry.getValue());
String uses = getScalarValue(entry.getValue());
if (uses == null) {
return false;
}
Expand All @@ -272,7 +273,7 @@ private boolean isCacheAwareActionStep(Yaml.Mapping.Entry entry) {
}

private String getActionName(Yaml.Mapping.Entry entry) {
String uses = YamlHelper.getScalarValue(entry.getValue());
String uses = getScalarValue(entry.getValue());
return uses != null ? extractActionName(uses) : "unknown";
}

Expand Down
7 changes: 4 additions & 3 deletions src/main/java/org/openrewrite/github/security/ExcessivePermissionsRecipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import lombok.EqualsAndHashCode;
import lombok.Value;
import org.openrewrite.*;
import org.openrewrite.github.util.YamlScalarAccessor;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.yaml.YamlIsoVisitor;
import org.openrewrite.yaml.tree.Yaml;
Expand Down Expand Up @@ -58,7 +59,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
);
}

private static class ExcessivePermissionsVisitor extends YamlIsoVisitor<ExecutionContext> {
private static class ExcessivePermissionsVisitor extends YamlIsoVisitor<ExecutionContext> implements YamlScalarAccessor {

@Override
public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionContext ctx) {
Expand All @@ -76,7 +77,7 @@ private boolean isPermissionsEntry(Yaml.Mapping.Entry entry) {
}

private Yaml.Mapping.Entry checkPermissions(Yaml.Mapping.Entry entry) {
String scalarPermissionValue = YamlHelper.getScalarValue(entry.getValue());
String scalarPermissionValue = getScalarValue(entry.getValue());
if (scalarPermissionValue != null) {
return checkScalarPermissions(entry, scalarPermissionValue);
}
Expand Down Expand Up @@ -107,7 +108,7 @@ private Yaml.Mapping.Entry checkMappingPermissions(Yaml.Mapping.Entry entry, Yam

for (Yaml.Mapping.Entry permEntry : permissionsMapping.getEntries()) {
String permissionName = permEntry.getKey().getValue();
String permissionValue = YamlHelper.getScalarValue(permEntry.getValue());
String permissionValue = getScalarValue(permEntry.getValue());
if (permissionName != null && permissionValue != null) {

if ("write".equals(permissionValue)) {
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/org/openrewrite/github/security/GitHubEnvRecipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.openrewrite.ExecutionContext;
import org.openrewrite.Recipe;
import org.openrewrite.TreeVisitor;
import org.openrewrite.github.util.YamlScalarAccessor;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.yaml.YamlIsoVisitor;
import org.openrewrite.yaml.tree.Yaml;
Expand Down Expand Up @@ -75,7 +76,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
return new GitHubEnvVisitor();
}

private static class GitHubEnvVisitor extends YamlIsoVisitor<ExecutionContext> {
private static class GitHubEnvVisitor extends YamlIsoVisitor<ExecutionContext> implements YamlScalarAccessor {

private boolean hasDangerousTriggers = false;

Expand Down Expand Up @@ -109,14 +110,14 @@ private void analyzeTriggers(Yaml.Document document) {
}

private boolean checkForDangerousTriggers(Yaml.Block onValue) {
String scalarTrigger = YamlHelper.getScalarValue(onValue);
String scalarTrigger = getScalarValue(onValue);
if (scalarTrigger != null) {
return DANGEROUS_TRIGGERS.contains(scalarTrigger);
}
if (onValue instanceof Yaml.Sequence) {
Yaml.Sequence sequence = (Yaml.Sequence) onValue;
for (Yaml.Sequence.Entry seqEntry : sequence.getEntries()) {
String trigger = YamlHelper.getScalarValue(seqEntry.getBlock());
String trigger = getScalarValue(seqEntry.getBlock());
if (trigger != null && DANGEROUS_TRIGGERS.contains(trigger)) {
return true;
}
Expand Down Expand Up @@ -163,7 +164,7 @@ private boolean isRunStepEntry(Yaml.Mapping.Entry entry) {
}

private String getRunContent(Yaml.Mapping.Entry entry) {
return YamlHelper.getScalarValue(entry.getValue());
return getScalarValue(entry.getValue());
}

private boolean usesGitHubEnv(String runContent) {
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/org/openrewrite/github/security/SelfHostedRunnerRecipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import lombok.EqualsAndHashCode;
import lombok.Value;
import org.openrewrite.*;
import org.openrewrite.github.util.YamlScalarAccessor;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.yaml.YamlIsoVisitor;
import org.openrewrite.yaml.tree.Yaml;
Expand Down Expand Up @@ -47,7 +48,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
);
}

private static class SelfHostedRunnerVisitor extends YamlIsoVisitor<ExecutionContext> {
private static class SelfHostedRunnerVisitor extends YamlIsoVisitor<ExecutionContext> implements YamlScalarAccessor {

@Override
public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionContext ctx) {
Expand Down Expand Up @@ -87,7 +88,7 @@ private Yaml.Mapping.Entry checkRunsOnValue(Yaml.Mapping.Entry entry, String run
}

private Yaml.Mapping.Entry checkRunsOnSequence(Yaml.Mapping.Entry entry, Yaml.Sequence sequence) {
String firstValue = YamlHelper.getScalarValue(sequence.getEntries().get(0).getBlock());
String firstValue = getScalarValue(sequence.getEntries().get(0).getBlock());
if ("self-hosted".equals(firstValue)) {
return SearchResult.found(entry,
"Uses self-hosted runner which may have security implications in public repositories. " +
Expand Down Expand Up @@ -137,7 +138,7 @@ private boolean containsSelfHostedInMatrixValues(Yaml.Mapping matrixMapping) {
if (matrixEntry.getValue() instanceof Yaml.Sequence) {
Yaml.Sequence sequence = (Yaml.Sequence) matrixEntry.getValue();
for (Yaml.Sequence.Entry seqEntry : sequence.getEntries()) {
String value = YamlHelper.getScalarValue(seqEntry.getBlock());
String value = getScalarValue(seqEntry.getBlock());
if ("self-hosted".equals(value)) {
return true;
}
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/org/openrewrite/github/security/TemplateInjectionRecipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import lombok.Value;
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.github.util.YamlScalarAccessor;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.yaml.JsonPathMatcher;
import org.openrewrite.yaml.YamlIsoVisitor;
Expand Down Expand Up @@ -93,7 +94,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
);
}

private static class TemplateInjectionVisitor extends YamlIsoVisitor<ExecutionContext> {
private static class TemplateInjectionVisitor extends YamlIsoVisitor<ExecutionContext> implements YamlScalarAccessor {

private static final JsonPathMatcher STEP_RUN_MATCHER = new JsonPathMatcher("$..steps[*].run");
private static final JsonPathMatcher STEP_USES_MATCHER = new JsonPathMatcher("$..steps[*].uses");
Expand Down Expand Up @@ -122,7 +123,7 @@ public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionC
}

private Yaml.Mapping.Entry checkRunEntry(Yaml.Mapping.Entry entry) {
String runCommand = YamlHelper.getScalarValue(entry.getValue());
String runCommand = getScalarValue(entry.getValue());
if (runCommand == null) {
return entry;
}
Expand All @@ -143,7 +144,7 @@ private Yaml.Mapping.Entry checkRunEntry(Yaml.Mapping.Entry entry) {
}

private Yaml.Mapping.Entry checkUsesEntry(Yaml.Mapping.Entry entry) {
String usesValue = YamlHelper.getScalarValue(entry.getValue());
String usesValue = getScalarValue(entry.getValue());
if (usesValue == null) {
return entry;
}
Expand All @@ -161,7 +162,7 @@ private Yaml.Mapping.Entry checkUsesEntry(Yaml.Mapping.Entry entry) {
}

private Yaml.Mapping.Entry checkScriptEntry(Yaml.Mapping.Entry entry) {
String scriptContent = YamlHelper.getScalarValue(entry.getValue());
String scriptContent = getScalarValue(entry.getValue());
if (scriptContent == null) {
return entry;
}
Expand Down
88 changes: 32 additions & 56 deletions src/main/java/org/openrewrite/github/security/UnpinnedActionsRecipe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@
import lombok.EqualsAndHashCode;
import lombok.Value;
import org.openrewrite.*;
import org.openrewrite.github.util.ActionStep;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.yaml.YamlIsoVisitor;
import org.openrewrite.yaml.tree.Yaml;

import java.util.regex.Pattern;

Expand All @@ -32,8 +31,6 @@ public class UnpinnedActionsRecipe extends Recipe {
"^([^/@]+/[^/@]+)(@(main|master|HEAD|latest|v?\\d+(\\.\\d+)*(\\.\\d+)*))??$"
);

private static final Pattern SHA_PATTERN = Pattern.compile("^[a-f0-9]{40}$");

@Override
public String getDisplayName() {
return "Pin GitHub Actions to specific commits";
Expand All @@ -51,68 +48,47 @@ public String getDescription() {
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(
new FindSourceFiles(".github/workflows/*.yml"),
new UnpinnedActionsVisitor()
new ActionStep.Matcher().asVisitor((actionStep, ctx) -> {
if (isUnpinned(actionStep)) {
String actionRef = actionStep.getActionRef();
return SearchResult.found(actionStep.getTree(),
"Action '" + actionRef + "' is not pinned to a commit SHA. " +
"Consider pinning to a specific commit for security and reproducibility.");
}
return actionStep.getTree();
})
);
}

private static class UnpinnedActionsVisitor extends YamlIsoVisitor<ExecutionContext> {

@Override
public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionContext ctx) {
Yaml.Mapping.Entry mappingEntry = super.visitMappingEntry(entry, ctx);

if (isUsesEntry(mappingEntry)) {
String usesValue = getUsesValue(mappingEntry);
if (usesValue != null && isUnpinned(usesValue)) {
return SearchResult.found(mappingEntry,
"Action '" + usesValue + "' is not pinned to a commit SHA. " +
"Consider pinning to a specific commit for security and reproducibility.");
}
}

return mappingEntry;
private static boolean isUnpinned(ActionStep actionStep) {
String actionRef = actionStep.getActionRef();
if (actionRef == null) {
return false;
}

private boolean isUsesEntry(Yaml.Mapping.Entry entry) {
// Broader approach - match any "uses" entry and let the logic handle context validation
return entry.getKey() instanceof Yaml.Scalar &&
"uses".equals(((Yaml.Scalar) entry.getKey()).getValue());
// Skip local actions (start with ./)
if (actionRef.startsWith("./")) {
return false;
}

private String getUsesValue(Yaml.Mapping.Entry entry) {
if (entry.getValue() instanceof Yaml.Scalar) {
return ((Yaml.Scalar) entry.getValue()).getValue();
}
return null;
// Skip Docker actions (start with docker://)
if (actionRef.startsWith("docker://")) {
return false;
}

private boolean isUnpinned(String usesValue) {
// Skip local actions (start with ./)
if (usesValue.startsWith("./")) {
return false;
}

// Skip Docker actions (start with docker://)
if (usesValue.startsWith("docker://")) {
return false;
}

// Check if it's a repository action
String[] parts = usesValue.split("@", 2);
if (parts.length < 2) {
// No @ symbol means no version specified at all
return true;
}

String version = parts[1];

// If it's already a SHA, it's pinned
if (SHA_PATTERN.matcher(version).matches()) {
return false;
}
// If it's already pinned to a SHA, it's not unpinned
if (actionStep.isVersionPinned()) {
return false;
}

// If it matches unpinned patterns (main, master, HEAD, latest, or version tags), it's unpinned
return UNPINNED_ACTION_PATTERN.matcher(usesValue).matches();
// Check if version exists
String version = actionStep.getActionVersion();
if (version == null) {
// No @ symbol means no version specified at all
return true;
}

// If it matches unpinned patterns (main, master, HEAD, latest, or version tags), it's unpinned
return UNPINNED_ACTION_PATTERN.matcher(actionRef).matches();
}
}
Loading