diff --git a/act/exprparser/interpreter.go b/act/exprparser/interpreter.go index dedb55fd..4bed3e85 100644 --- a/act/exprparser/interpreter.go +++ b/act/exprparser/interpreter.go @@ -85,37 +85,26 @@ func NewInterpeter(env *EvaluationEnvironment, config Config) Interpreter { } } +// Evaluate evaluates one expression. An empty input asks defaultStatusCheck on its own, which is +// what a value that carries no expression of its own runs under. func (impl *interperterImpl) Evaluate(input string, defaultStatusCheck DefaultStatusCheck) (any, error) { input = strings.TrimPrefix(input, "${{") - if defaultStatusCheck != DefaultStatusCheckNone && input == "" { - input = "success()" + + if input == "" && defaultStatusCheck != DefaultStatusCheckNone { + return impl.evaluateNode(statusCheckNode(defaultStatusCheck)) } + parser := actionlint.NewExprParser() exprNode, err := parser.Parse(actionlint.NewExprLexer(input + "}}")) if err != nil { return nil, fmt.Errorf("Failed to parse: %s", err.Message) } - if defaultStatusCheck != DefaultStatusCheckNone { - hasStatusCheckFunction := false - actionlint.VisitExprNode(exprNode, func(node, _ actionlint.ExprNode, entering bool) { - if funcCallNode, ok := node.(*actionlint.FuncCallNode); entering && ok { - switch strings.ToLower(funcCallNode.Callee) { - case "success", "always", "cancelled", "failure": - hasStatusCheckFunction = true - } - } - }) - - if !hasStatusCheckFunction { - exprNode = &actionlint.LogicalOpNode{ - Kind: actionlint.LogicalOpNodeKindAnd, - Left: &actionlint.FuncCallNode{ - Callee: defaultStatusCheck.String(), - Args: []actionlint.ExprNode{}, - }, - Right: exprNode, - } + if defaultStatusCheck != DefaultStatusCheckNone && !CallsStatusFunction(exprNode) { + exprNode = &actionlint.LogicalOpNode{ + Kind: actionlint.LogicalOpNodeKindAnd, + Left: statusCheckNode(defaultStatusCheck), + Right: exprNode, } } @@ -124,6 +113,25 @@ func (impl *interperterImpl) Evaluate(input string, defaultStatusCheck DefaultSt return result, err2 } +func statusCheckNode(defaultStatusCheck DefaultStatusCheck) *actionlint.FuncCallNode { + return &actionlint.FuncCallNode{Callee: defaultStatusCheck.String(), Args: []actionlint.ExprNode{}} +} + +// CallsStatusFunction reports whether the expression calls a status function, which counts as the +// expression asking its own status question instead of the default one. +func CallsStatusFunction(exprNode actionlint.ExprNode) bool { + found := false + actionlint.VisitExprNode(exprNode, func(node, _ actionlint.ExprNode, entering bool) { + if funcCallNode, ok := node.(*actionlint.FuncCallNode); entering && ok { + switch strings.ToLower(funcCallNode.Callee) { + case "success", "always", "cancelled", "failure": + found = true + } + } + }) + return found +} + func (impl *interperterImpl) evaluateNode(exprNode actionlint.ExprNode) (any, error) { switch node := exprNode.(type) { case *actionlint.VariableNode: diff --git a/act/exprparser/interpreter_test.go b/act/exprparser/interpreter_test.go index e1e94283..4d25dcc1 100644 --- a/act/exprparser/interpreter_test.go +++ b/act/exprparser/interpreter_test.go @@ -682,3 +682,10 @@ func TestCoerceToString(t *testing.T) { }) } } + +func TestEvaluateEmptyInputAsksItsOwnStatusCheck(t *testing.T) { + // always() needs no job or step context, so it shows which function an empty input asks for + output, err := NewInterpeter(&EvaluationEnvironment{}, Config{}).Evaluate("", DefaultStatusCheckAlways) + require.NoError(t, err) + assert.Equal(t, true, output) +} diff --git a/act/runner/expression.go b/act/runner/expression.go index 6a2c94a9..c382ea15 100644 --- a/act/runner/expression.go +++ b/act/runner/expression.go @@ -7,6 +7,7 @@ package runner import ( "bytes" "context" + "errors" "fmt" "maps" "path" @@ -22,12 +23,14 @@ import ( _ "embed" + "github.com/rhysd/actionlint" "go.yaml.in/yaml/v4" ) // ExpressionEvaluator is the interface for evaluating expressions type ExpressionEvaluator interface { evaluate(context.Context, string, exprparser.DefaultStatusCheck) (any, error) + interpolate(context.Context, string) (string, error) EvaluateYamlNode(context.Context, *yaml.Node) error Interpolate(context.Context, string) string } @@ -240,8 +243,7 @@ func (ee expressionEvaluator) evaluateScalarYamlNode(ctx context.Context, node * if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") { return nil, nil //nolint:nilnil // pre-existing issue from nektos/act } - expr, _ := rewriteSubExpression(ctx, in, false) - res, err := ee.evaluate(ctx, expr, exprparser.DefaultStatusCheckNone) + res, err := ee.evaluateScalar(ctx, in) if err != nil { return nil, err } @@ -367,105 +369,146 @@ func (ee expressionEvaluator) EvaluateYamlNode(ctx context.Context, node *yaml.N } func (ee expressionEvaluator) Interpolate(ctx context.Context, in string) string { - if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") { - return in - } - - expr, _ := rewriteSubExpression(ctx, in, true) - evaluated, err := ee.evaluate(ctx, expr, exprparser.DefaultStatusCheckNone) + out, err := ee.interpolate(ctx, in) if err != nil { - common.Logger(ctx).Errorf("Unable to interpolate expression '%s': %s", expr, err) + common.Logger(ctx).Errorf("Unable to interpolate expression '%s': %s", in, err) return "" } - - value, ok := evaluated.(string) - if !ok { - panic(fmt.Sprintf("Expression %s did not evaluate to a string", expr)) - } - - return value + return out } -// EvalBool evaluates an expression against given evaluator -func EvalBool(ctx context.Context, evaluator ExpressionEvaluator, expr string, defaultStatusCheck exprparser.DefaultStatusCheck) (bool, error) { - nextExpr, _ := rewriteSubExpression(ctx, expr, false) +// interpolate evaluates every part on its own, so a malformed one cannot restructure its neighbours +func (ee expressionEvaluator) interpolate(ctx context.Context, in string) (string, error) { + parts, err := splitSubExpressions(in) + if err != nil { + return "", err + } + if len(parts) == 1 && !parts[0].isExpr { + return in, nil + } + var out strings.Builder + out.Grow(len(in)) + for _, part := range parts { + if !part.isExpr { + out.WriteString(part.text) + continue + } + evaluated, err := ee.evaluate(ctx, part.text, exprparser.DefaultStatusCheckNone) + if err != nil { + return "", err + } + out.WriteString(exprparser.CoerceToString(evaluated)) + } + return out.String(), nil +} - evaluated, err := evaluator.evaluate(ctx, nextExpr, defaultStatusCheck) +// evaluateScalar keeps the type of a lone expression, so `${{ fromJSON('[1,2]') }}` stays an array +func (ee expressionEvaluator) evaluateScalar(ctx context.Context, in string) (any, error) { + parts, err := splitSubExpressions(in) + if err != nil { + return nil, err + } + if len(parts) == 1 && parts[0].isExpr { + return ee.evaluate(ctx, parts[0].text, exprparser.DefaultStatusCheckNone) + } + return ee.interpolate(ctx, in) +} + +// EvalBool evaluates an expression against given evaluator. An `if:` is an expression even without +// `${{ }}`, while literal text around one makes the whole value a string. +func EvalBool(ctx context.Context, evaluator ExpressionEvaluator, expr string, defaultStatusCheck exprparser.DefaultStatusCheck) (bool, error) { + parts, err := splitSubExpressions(expr) if err != nil { return false, err } - - return exprparser.IsTruthy(evaluated), nil -} - -func escapeFormatString(in string) string { - return strings.ReplaceAll(strings.ReplaceAll(in, "{", "{{"), "}", "}}") -} - -func rewriteSubExpression(ctx context.Context, in string, forceFormat bool) (string, error) { //nolint:unparam // pre-existing issue from nektos/act - if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") { - return in, nil + if len(parts) == 1 { + evaluated, err := evaluator.evaluate(ctx, parts[0].text, defaultStatusCheck) + if err != nil { + return false, err + } + return exprparser.IsTruthy(evaluated), nil } - strPattern := regexp.MustCompile("(?:''|[^'])*'") - pos := 0 - exprStart := -1 - strStart := -1 - var results []string - var formatOut strings.Builder - for pos < len(in) { - if strStart > -1 { - matches := strPattern.FindStringIndex(in[pos:]) - if matches == nil { - panic("unclosed string.") - } - - strStart = -1 - pos += matches[1] - } else if exprStart > -1 { - exprEnd := strings.Index(in[pos:], "}}") - strStart = strings.Index(in[pos:], "'") - - if exprEnd > -1 && strStart > -1 { - if exprEnd < strStart { - strStart = -1 - } else { - exprEnd = -1 - } - } - - if exprEnd > -1 { - fmt.Fprintf(&formatOut, "{%d}", len(results)) - results = append(results, strings.TrimSpace(in[exprStart:pos+exprEnd])) - pos += exprEnd + 2 - exprStart = -1 - } else if strStart > -1 { - pos += strStart + 1 - } else { - panic("unclosed expression.") - } - } else { - exprStart = strings.Index(in[pos:], "${{") - if exprStart != -1 { - formatOut.WriteString(escapeFormatString(in[pos : pos+exprStart])) - exprStart = pos + exprStart + 3 - pos = exprStart - } else { - formatOut.WriteString(escapeFormatString(in[pos:])) - pos = len(in) - } + // mixed content is a string, so the status check applies to it separately + if defaultStatusCheck != exprparser.DefaultStatusCheckNone && !callsStatusFunction(parts) { + status, err := evaluator.evaluate(ctx, "", defaultStatusCheck) + if err != nil { + return false, err + } + if !exprparser.IsTruthy(status) { + return false, nil } } + interpolated, err := evaluator.interpolate(ctx, expr) + if err != nil { + return false, err + } + return exprparser.IsTruthy(interpolated), nil +} - if len(results) == 1 && formatOut.String() == "{0}" && !forceFormat { - return in, nil +// callsStatusFunction reports whether any part calls a status function. A part that does not parse +// counts as one, so the evaluation reports it against the real values. +func callsStatusFunction(parts []exprPart) bool { + for _, part := range parts { + if !part.isExpr { + continue + } + // The lexer needs the closing `}}` that the scanner strips. + exprNode, err := actionlint.NewExprParser().Parse(actionlint.NewExprLexer(part.text + "}}")) + if err != nil || exprparser.CallsStatusFunction(exprNode) { + return true + } + } + return false +} + +type exprPart struct { + text string + isExpr bool +} + +// splitSubExpressions splits in the way GitHub's template reader does, leaving a value without a +// complete expression literal. +func splitSubExpressions(in string) ([]exprPart, error) { + if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") { + return []exprPart{{text: in}}, nil } - out := fmt.Sprintf("format('%s', %s)", strings.ReplaceAll(formatOut.String(), "'", "''"), strings.Join(results, ", ")) - if in != out { - common.Logger(ctx).Debugf("expression '%s' rewritten to '%s'", in, out) + parts := make([]exprPart, 0, 2*strings.Count(in, "${{")+1) + for { + start := strings.Index(in, "${{") + if start < 0 { + if in != "" { + parts = append(parts, exprPart{text: in}) + } + return parts, nil + } + if start > 0 { + parts = append(parts, exprPart{text: in[:start]}) + } + rest := in[start+len("${{"):] + end := indexExprEnd(rest) + if end < 0 { + return nil, errors.New("unclosed expression") + } + parts = append(parts, exprPart{text: strings.TrimSpace(rest[:end]), isExpr: true}) + in = rest[end+len("}}"):] } - return out, nil +} + +// indexExprEnd returns the offset of the `}}` ending an expression, or -1. A quote toggles string +// state, so a `}}` inside a string does not end it. +func indexExprEnd(in string) int { + inString := false + for i := range len(in) { + switch { + case in[i] == '\'': + inString = !inString + case !inString && in[i] == '}' && i+1 < len(in) && in[i+1] == '}': + return i + } + } + return -1 } func getEvaluatorInputs(ctx context.Context, rc *RunContext, step step, ghc *model.GithubContext) map[string]any { diff --git a/act/runner/expression_test.go b/act/runner/expression_test.go index e988e086..13719e4c 100644 --- a/act/runner/expression_test.go +++ b/act/runner/expression_test.go @@ -259,6 +259,15 @@ func TestInterpolate(t *testing.T) { {"${{ env.SOMETHING_FALSE || false }}", "false"}, {"${{ env.SOMETHING_FALSE }} && ${{ env.SOMETHING_TRUE }}", "false && true"}, {"${{ fromJSON('{}') < 2 }}", "false"}, + {"${{ 1 }}", "1"}, + {"${{ 1.0 }}", "1"}, + {"${{ null }}", ""}, + {"${{ fromJSON('[1,2]') }}", "Array"}, + {"${{ fromJSON('{\"a\":1}') }}", "Object"}, + // a malformed part must not restructure its neighbours, and it interpolates to nothing + {"${{ 1) && (2 }}", ""}, + {"run ${{ 1) && (2 }} now", ""}, + {"${{ 1", "${{ 1"}, } for _, table := range tables { @@ -270,57 +279,38 @@ func TestInterpolate(t *testing.T) { } } -func TestRewriteSubExpression(t *testing.T) { - table := []struct { - in string - out string +func TestSplitSubExpressions(t *testing.T) { + expr := func(text string) exprPart { return exprPart{text: text, isExpr: true} } + literal := func(text string) exprPart { return exprPart{text: text} } + + for _, tt := range []struct { + in string + want []exprPart }{ - {in: "Hello World", out: "Hello World"}, - {in: "${{ true }}", out: "${{ true }}"}, - {in: "${{ true }} ${{ true }}", out: "format('{0} {1}', true, true)"}, - {in: "${{ true || false }} ${{ true && true }}", out: "format('{0} {1}', true || false, true && true)"}, - {in: "${{ '}}' }}", out: "${{ '}}' }}"}, - {in: "${{ '''}}''' }}", out: "${{ '''}}''' }}"}, - {in: "${{ '''' }}", out: "${{ '''' }}"}, - {in: `${{ fromJSON('"}}"') }}`, out: `${{ fromJSON('"}}"') }}`}, - {in: `${{ fromJSON('"\"}}\""') }}`, out: `${{ fromJSON('"\"}}\""') }}`}, - {in: `${{ fromJSON('"''}}"') }}`, out: `${{ fromJSON('"''}}"') }}`}, - {in: "Hello ${{ 'World' }}", out: "format('Hello {0}', 'World')"}, + {"Hello World", []exprPart{literal("Hello World")}}, + {"${{ true }}", []exprPart{expr("true")}}, + {"${{ true }} ${{ false }}", []exprPart{expr("true"), literal(" "), expr("false")}}, + {"Hello ${{ 'World' }}", []exprPart{literal("Hello "), expr("'World'")}}, + // a quote toggles string state, so a `}}` inside a string does not end the expression + {"${{ '}}' }}", []exprPart{expr("'}}'")}}, + {"${{ '''}}''' }}", []exprPart{expr("'''}}'''")}}, + {"${{ '''' }}", []exprPart{expr("''''")}}, + {`${{ fromJSON('"}}"') }}`, []exprPart{expr(`fromJSON('"}}"')`)}}, + {`${{ fromJSON('"\"}}\""') }}`, []exprPart{expr(`fromJSON('"\"}}\""')`)}}, + {`${{ fromJSON('"''}}"') }}`, []exprPart{expr(`fromJSON('"''}}"')`)}}, + // without a complete literal the value stays text, as GitHub's template reader leaves it + {"${{ 1", []exprPart{literal("${{ 1")}}, + // a malformed part stays one part, so it cannot restructure its neighbours + {"${{ 1) && (2 }}", []exprPart{expr("1) && (2")}}, + } { + got, err := splitSubExpressions(tt.in) + require.NoError(t, err, tt.in) + assert.Equal(t, tt.want, got, tt.in) } - for _, table := range table { - t.Run("TestRewriteSubExpression", func(t *testing.T) { - assertObject := assert.New(t) - out, err := rewriteSubExpression(context.Background(), table.in, false) - if err != nil { - t.Fatal(err) - } - assertObject.Equal(table.out, out, table.in) - }) - } -} - -func TestRewriteSubExpressionForceFormat(t *testing.T) { - table := []struct { - in string - out string - }{ - {in: "Hello World", out: "Hello World"}, - {in: "${{ true }}", out: "format('{0}', true)"}, - {in: "${{ '}}' }}", out: "format('{0}', '}}')"}, - {in: `${{ fromJSON('"}}"') }}`, out: `format('{0}', fromJSON('"}}"'))`}, - {in: "Hello ${{ 'World' }}", out: "format('Hello {0}', 'World')"}, - } - - for _, table := range table { - t.Run("TestRewriteSubExpressionForceFormat", func(t *testing.T) { - assertObject := assert.New(t) - out, err := rewriteSubExpression(context.Background(), table.in, true) - if err != nil { - t.Fatal(err) - } - assertObject.Equal(table.out, out, table.in) - }) + for _, in := range []string{"${{ 'a' }} ${{ b", "${{ 'a }}"} { + _, err := splitSubExpressions(in) + assert.ErrorContains(t, err, "unclosed expression", in) } }