diff --git a/internal/workflowfile/testdata/mixed_refs.yml b/internal/workflowfile/testdata/mixed_refs.yml index 5a86a329..7077d318 100644 --- a/internal/workflowfile/testdata/mixed_refs.yml +++ b/internal/workflowfile/testdata/mixed_refs.yml @@ -8,7 +8,6 @@ jobs: - uses: ./local-action - uses: docker://alpine:3.18 - uses: actions/cache/save@v4 - - uses: ${{ matrix.action }} - uses: owner/repo/.github/workflows/called.yml@v1 - uses: owner/repo/.github/workflows/called.yaml@main - uses: actions/setup-node@v4 diff --git a/internal/workflowfile/workflowfile.go b/internal/workflowfile/workflowfile.go index ea4c8cf5..cda5aeb9 100644 --- a/internal/workflowfile/workflowfile.go +++ b/internal/workflowfile/workflowfile.go @@ -48,9 +48,34 @@ func Parse(path string, content []byte) (*File, error) { return nil, fmt.Errorf("parsing workflow YAML: %w", err) } + var invalid []string + walkUses(&f.root, func(value string, _ bool) { + if isUsesExpression(value) { + invalid = append(invalid, strings.TrimSpace(value)) + } + }) + if len(invalid) > 0 { + return nil, usesExpressionErr(invalid) + } + return f, nil } +// isUsesExpression reports whether a `uses:` value contains an expression. +// Malformed `$/…@ref` values are excluded so the more specific self +// repository diagnostic still wins. +func isUsesExpression(value string) bool { + value = strings.TrimSpace(value) + return strings.Contains(value, "${") && !SelfRepositoryRefHasVersion(value) +} + +// usesExpressionErr reports an expression in `uses:`. GitHub Actions rejects +// the workflow outright at both step and job level (no context is in scope +// there), so parsing fails rather than skipping the line. +func usesExpressionErr(values []string) error { + return fmt.Errorf("`uses:` can't contain an expression: %s", strings.Join(values, ", ")) +} + // RefScan is the classified result of walking a workflow's `uses:` values. type RefScan struct { // Refs are remote repository action references (owner/repo[/path]@ref). @@ -88,10 +113,6 @@ func (f *File) ExtractActionRefs() RefScan { } return } - if strings.Contains(value, "${") { - scan.Warnings = append(scan.Warnings, fmt.Sprintf("skipping unparseable uses: value %q (expressions are not supported)", value)) - return - } if IsSelfRepositoryAction(value) { // Bare `$/…` is a legal self repository reference at both step level // (action dir) and job level (reusable-workflow file): "this repo @@ -212,8 +233,6 @@ func ScanSelfRepositoryActions(workflowPath string, actionRefs []string) SelfRep seenInvalid[use] = true scan.SelfRepositoryRefErrs = append(scan.SelfRepositoryRefErrs, use) } - case strings.Contains(use, "${"): - scan.Warnings = append(scan.Warnings, fmt.Sprintf("skipping unparseable uses: value %q in %s (expressions are not supported)", use, current.ref)) case IsSelfRepositoryAction(use): if !seenSelf[use] { seenSelf[use] = true @@ -375,37 +394,81 @@ func ExtractLocalCompositeRefs(workflowPath string, localPaths []string) ([]pars return refs, warnings } +// walkUses visits `uses:` values at the only places GitHub Actions honors +// them: `jobs..uses`, `jobs..steps[*].uses` in a workflow, and +// `runs.steps[*].uses` in a composite action definition. A `uses` key +// anywhere else (an env var, a `with:` input) is ordinary data, not a +// reference. func walkUses(node *yaml.Node, fn func(value string, stepLevel bool)) { - walkUsesDepth(node, false, fn, 0) + root := resolveAlias(documentRoot(node)) + walkStepUses(mapValue(root, "runs"), fn) + + jobs := mapValue(root, "jobs") + if jobs == nil || jobs.Kind != yaml.MappingNode { + return + } + for i := 1; i < len(jobs.Content); i += 2 { + job := resolveAlias(jobs.Content[i]) + if job == nil || job.Kind != yaml.MappingNode { + continue + } + if use := scalarValue(mapValue(job, "uses")); use != "" { + fn(use, false) + } + walkStepUses(job, fn) + } } -func walkUsesDepth(node *yaml.Node, inStep bool, fn func(value string, stepLevel bool), depth int) { - if node == nil || depth > maxYAMLWalkDepth { +// walkStepUses visits the `uses:` of each step in owner's `steps:` sequence. +func walkStepUses(owner *yaml.Node, fn func(value string, stepLevel bool)) { + steps := mapValue(owner, "steps") + if steps == nil || steps.Kind != yaml.SequenceNode { return } - switch node.Kind { - case yaml.DocumentNode, yaml.SequenceNode: - for _, child := range node.Content { - walkUsesDepth(child, inStep, fn, depth+1) + for _, stepNode := range steps.Content { + step := resolveAlias(stepNode) + if step == nil || step.Kind != yaml.MappingNode { + continue } - case yaml.MappingNode: - for i := 0; i < len(node.Content)-1; i += 2 { - key := node.Content[i] - value := node.Content[i+1] - childInStep := inStep - if key.Kind == yaml.ScalarNode { - switch key.Value { - case "uses": - if value.Kind == yaml.ScalarNode { - fn(value.Value, inStep) - } - case "steps": - childInStep = true - } - } - walkUsesDepth(value, childInStep, fn, depth+1) + if use := scalarValue(mapValue(step, "uses")); use != "" { + fn(use, true) + } + } +} + +func documentRoot(node *yaml.Node) *yaml.Node { + if node != nil && node.Kind == yaml.DocumentNode && len(node.Content) > 0 { + return node.Content[0] + } + return node +} + +// resolveAlias dereferences YAML anchors so an aliased job or step is walked +// like the node it points at. +func resolveAlias(node *yaml.Node) *yaml.Node { + for i := 0; node != nil && node.Kind == yaml.AliasNode && i < maxYAMLWalkDepth; i++ { + node = node.Alias + } + return node +} + +func mapValue(node *yaml.Node, key string) *yaml.Node { + if node == nil || node.Kind != yaml.MappingNode { + return nil + } + for i := 0; i < len(node.Content)-1; i += 2 { + if node.Content[i].Kind == yaml.ScalarNode && node.Content[i].Value == key { + return resolveAlias(node.Content[i+1]) } } + return nil +} + +func scalarValue(node *yaml.Node) string { + if node == nil || node.Kind != yaml.ScalarNode { + return "" + } + return node.Value } // maxYAMLWalkDepth bounds recursion in walkYAMLNodes so a hostile or @@ -517,11 +580,19 @@ func parseActionYAMLForUses(content []byte) ([]string, error) { } var uses []string + var invalid []string for _, step := range action.Runs.Steps { - if step.Uses != "" { + switch { + case step.Uses == "": + case isUsesExpression(step.Uses): + invalid = append(invalid, strings.TrimSpace(step.Uses)) + default: uses = append(uses, step.Uses) } } + if len(invalid) > 0 { + return nil, usesExpressionErr(invalid) + } return uses, nil } diff --git a/internal/workflowfile/workflowfile_test.go b/internal/workflowfile/workflowfile_test.go index 4597e6b1..e0b2a9b4 100644 --- a/internal/workflowfile/workflowfile_test.go +++ b/internal/workflowfile/workflowfile_test.go @@ -41,9 +41,32 @@ func TestExtractActionRefsMixed(t *testing.T) { assert.Len(t, localPaths, 1) assert.Equal(t, "./local-action", localPaths[0]) + assert.Empty(t, warnings) +} - assert.Len(t, warnings, 1) - assert.Contains(t, warnings[0], "unparseable uses:") +func TestParseRejectsUsesExpression(t *testing.T) { + for name, content := range map[string]string{ + "step level": ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: ${{ format('actions/checkout@v4') }} +`, + "job level": ` +on: push +jobs: + call: + uses: owner/repo/.github/workflows/x.yml@${{ github.ref_name }} +`, + } { + t.Run(name, func(t *testing.T) { + _, err := Parse("wf.yml", []byte(content)) + require.Error(t, err) + assert.Contains(t, err.Error(), "`uses:` can't contain an expression") + }) + } } func TestExtractActionRefs_SelfRepositoryClassification(t *testing.T) { @@ -382,3 +405,52 @@ func TestExtractLocalCompositeRefs_RejectsPathTraversal(t *testing.T) { } assert.True(t, sawRefusal, "expected refusal warning, got: %#v", warnings) } + +func TestParseIgnoresNonUsesKeys(t *testing.T) { + f, err := Parse("wf.yml", []byte(` +on: push +env: + uses: ${{ github.repository }} +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + uses: ${{ github.ref }} +`)) + require.NoError(t, err) + assert.Len(t, f.ExtractActionRefs().Refs, 1) +} + +func TestParseRejectsAliasedUsesExpression(t *testing.T) { + _, err := Parse("wf.yml", []byte(` +on: push +x: &step + uses: ${{ matrix.action }} +jobs: + build: + runs-on: ubuntu-latest + steps: + - *step +`)) + require.Error(t, err) + assert.Contains(t, err.Error(), "`uses:` can't contain an expression") +} + +func TestScanSelfRepositoryActionsRejectsUsesExpression(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(root, ".git"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(root, "actions", "compo"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(root, "actions", "compo", "action.yml"), []byte(` +runs: + using: composite + steps: + - uses: actions/checkout@${{ inputs.ref }} +`), 0o644)) + + scan := ScanSelfRepositoryActions(filepath.Join(root, ".github", "workflows", "ci.yml"), []string{"$/actions/compo"}) + require.Len(t, scan.Errors, 1) + assert.Contains(t, scan.Errors[0], "`uses:` can't contain an expression") + assert.Empty(t, scan.Refs) +} diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index 886bfe0a..1238875b 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -414,8 +414,9 @@ scenarios: - name: expression_in_uses category: workflow_parsing - description: "Expression-based uses: (${{ }}) — unparseable, skipped with warning" - needs_token: true + description: "Expression-based uses: (${{ }}) — illegal in Actions, fails to parse" + needs_stub: true + tags: [stub] fixtures: workflows: ci.yml: @@ -429,8 +430,8 @@ scenarios: - uses: ${{ matrix.action }} - uses: actions/checkout@v4 expect: - exit: 0 - lockfile_deps_cover_direct: true + exit: 2 + output_contains: ["can't contain an expression"] - name: local_action_skipped category: workflow_parsing