diff --git a/modules/actions/jobparser/jobparser.go b/modules/actions/jobparser/jobparser.go index 1f843c125b6..faa660441e3 100644 --- a/modules/actions/jobparser/jobparser.go +++ b/modules/actions/jobparser/jobparser.go @@ -125,6 +125,9 @@ type parseContext struct { type ParseOption func(c *parseContext) func getMatrixes(job *model.Job) ([]map[string]any, error) { + if err := validateMatrixFilters(job); err != nil { + return nil, err + } ret, err := job.GetMatrixes() if err != nil { return nil, fmt.Errorf("GetMatrixes: %w", err) @@ -135,6 +138,35 @@ func getMatrixes(job *model.Job) ([]map[string]any, error) { return ret, nil } +// validateMatrixFilters rejects an `include`/`exclude` that is not a list of mappings, so that the +// usual way to get there, an unevaluated ${{ }} expression that is still a scalar, is named as such +// instead of panicking inside the expansion. +func validateMatrixFilters(job *model.Job) error { + if job.Strategy == nil || job.Strategy.RawMatrix.Kind != yaml.MappingNode { + return nil + } + content := job.Strategy.RawMatrix.Content + for i := 0; i+1 < len(content); i += 2 { + name, value := content[i].Value, content[i+1] + if name != "include" && name != "exclude" { + continue + } + entries := []*yaml.Node{value} + if value.Kind == yaml.SequenceNode { + entries = value.Content + } + for _, entry := range entries { + if entry.Kind == yaml.AliasNode { + entry = entry.Alias + } + if entry.Kind != yaml.MappingNode { + return fmt.Errorf("matrix %s must be a list of mappings", name) + } + } + } + return nil +} + func encodeMatrix(matrix map[string]any) yaml.Node { if len(matrix) == 0 { return yaml.Node{} diff --git a/modules/actions/jobparser/jobparser_test.go b/modules/actions/jobparser/jobparser_test.go index ca8493ef1cb..737ae2455f7 100644 --- a/modules/actions/jobparser/jobparser_test.go +++ b/modules/actions/jobparser/jobparser_test.go @@ -4,6 +4,7 @@ package jobparser import ( + "fmt" "strings" "testing" @@ -147,3 +148,47 @@ func TestParseInterpolatesRunName(t *testing.T) { require.Len(t, result, 1) assert.Empty(t, result[0].RunName) } + +func TestRejectsUnevaluatedMatrixFilters(t *testing.T) { + // An unevaluated ${{ }} expression is still a scalar, which act cannot apply as a filter: it used + // to reach the expansion and panic there on an unchecked type assertion, taking down the file view + // and the push_update queue. + const workflow = ` +name: t +on: push +jobs: + setup: + runs-on: ubuntu-latest + outputs: + m: ${{ steps.s.outputs.m }} + steps: [{id: s, run: echo}] + build: + needs: setup + runs-on: ubuntu-latest + strategy: + matrix: + %s + steps: [{run: echo}] +` + for _, tt := range []struct { + name string + matrix string + }{ + {name: "include expression", matrix: "include: ${{ fromJson(needs.setup.outputs.m) }}"}, + {name: "exclude expression", matrix: "os: [a, b]\n exclude: ${{ fromJson(vars.MATRIX) }}"}, + {name: "include scalar", matrix: "include: whatever"}, + {name: "include list of scalars", matrix: "include: [a, b]"}, + } { + t.Run(tt.name, func(t *testing.T) { + require.NotPanics(t, func() { + _, err := Parse(fmt.Appendf(nil, workflow, tt.matrix)) + require.ErrorContains(t, err, "must be a list of mappings") + }) + }) + } + + // a well-formed include/exclude keeps expanding + planned, err := Parse(fmt.Appendf(nil, workflow, "os: [a, b]\n include:\n - os: c\n exclude:\n - os: b")) + require.NoError(t, err) + assert.Len(t, planned, 3) // setup, plus build for os a and c +}