fix(actions): evaluate each ${{ }} part on its own (#38754)

Every `${{ }}` part was spliced as raw text into a synthesized
`format('...', <raw>)` call and re-parsed, so unbalanced parentheses
restructured the whole expression:

```yaml
run-name: ${{ 1) && (2 }}     # panicked, aborting workflow parsing for the push
if: x ${{ 1) && (2 }} y       # silently evaluated to 2
```

One scanner shaped like GitHub's template reader now splits every value
and each part is evaluated on its own, so nothing builds an expression
out of text. A part that fails is an error instead of an empty string,
making a `run-name`, `runs-on` or job name that GitHub rejects invalid
here too.

Replaces https://github.com/go-gitea/gitea/pull/38736 as the actual root-cause fix.
Signed-off-by: silverwind <me@silverwind.io>

modules/actions/jobparser/evaluator.go

@@ -6,7 +6,10 @@ package jobparser
 import (
 	"errors"
 	"fmt"
+	"math"
+	"reflect"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"gitea.com/gitea/runner/act/exprparser"
@@ -23,12 +26,6 @@ func NewExpressionEvaluator(interpreter exprparser.Interpreter) *ExpressionEvalu
 	return &ExpressionEvaluator{interpreter: interpreter}
 }
 
-func (ee ExpressionEvaluator) evaluate(in string, defaultStatusCheck exprparser.DefaultStatusCheck) (any, error) {
-	evaluated, err := ee.interpreter.Evaluate(in, defaultStatusCheck)
-
-	return evaluated, err
-}
-
 func (ee ExpressionEvaluator) evaluateScalarYamlNode(node *yaml.Node) error {
 	var in string
 	if err := node.Decode(&in); err != nil {
@@ -37,17 +34,17 @@ func (ee ExpressionEvaluator) evaluateScalarYamlNode(node *yaml.Node) error {
 	if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") {
 		return nil
 	}
-	expr, _ := rewriteSubExpression(in, false)
-	res, err := ee.evaluate(expr, exprparser.DefaultStatusCheckNone)
+	res, err := ee.evaluateScalar(in)
 	if err != nil {
 		return err
 	}
 	return node.Encode(res)
 }
 
+// GitHub has this undocumented feature to merge maps, called insert directive
+var insertDirective = regexp.MustCompile(`\${{\s*insert\s*}}`)
+
 func (ee ExpressionEvaluator) evaluateMappingYamlNode(node *yaml.Node) error {
-	// GitHub has this undocumented feature to merge maps, called insert directive
-	insertDirective := regexp.MustCompile(`\${{\s*insert\s*}}`)
 	for i := 0; i < len(node.Content)/2; {
 		k := node.Content[i*2]
 		v := node.Content[i*2+1]
@@ -102,88 +99,170 @@ func (ee ExpressionEvaluator) EvaluateYamlNode(node *yaml.Node) error {
 	}
 }
 
-func (ee ExpressionEvaluator) Interpolate(in string) string {
-	if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") {
-		return in
-	}
-
-	expr, _ := rewriteSubExpression(in, true)
-	evaluated, err := ee.evaluate(expr, exprparser.DefaultStatusCheckNone)
+// interpolate evaluates every part on its own, so a malformed one cannot restructure its neighbours
+func (ee ExpressionEvaluator) interpolate(in string) (string, error) {
+	parts, err := splitSubExpressions(in)
 	if err != nil {
-		return ""
+		return "", err
 	}
-
-	value, ok := evaluated.(string)
-	if !ok {
-		panic(fmt.Sprintf("Expression %s did not evaluate to a string", expr))
-	}
-
-	return value
-}
-
-func escapeFormatString(in string) string {
-	return strings.ReplaceAll(strings.ReplaceAll(in, "{", "{{"), "}", "}}")
-}
-
-func rewriteSubExpression(in string, forceFormat bool) (string, error) {
-	if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") {
+	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.interpreter.Evaluate(part.text, exprparser.DefaultStatusCheckNone)
+		if err != nil {
+			return "", err
+		}
+		out.WriteString(coerceToString(evaluated))
+	}
+	return out.String(), 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 {
-				return "", errors.New("unclosed string")
-			}
+// evaluateScalar keeps the type of a lone expression, so `${{ fromJSON('[1,2]') }}` stays an array
+func (ee ExpressionEvaluator) evaluateScalar(in string) (any, error) {
+	parts, err := splitSubExpressions(in)
+	if err != nil {
+		return nil, err
+	}
+	if len(parts) == 1 && parts[0].isExpr {
+		return ee.interpreter.Evaluate(parts[0].text, exprparser.DefaultStatusCheckNone)
+	}
+	return ee.interpolate(in)
+}
 
-			strStart = -1
-			pos += matches[1]
-		} else if exprStart > -1 {
-			exprEnd := strings.Index(in[pos:], "}}")
-			strStart = strings.Index(in[pos:], "'")
+// evaluateCondition evaluates an `if:`, an expression even without `${{ }}`. Mixed content
+// interpolates to a string, so the success() default applies to it separately.
+func (ee ExpressionEvaluator) evaluateCondition(in string) (bool, error) {
+	parts, err := splitSubExpressions(in)
+	if err != nil {
+		return false, err
+	}
+	if len(parts) == 1 {
+		evaluated, err := ee.interpreter.Evaluate(parts[0].text, exprparser.DefaultStatusCheckSuccess)
+		if err != nil {
+			return false, err
+		}
+		return exprparser.IsTruthy(evaluated), nil
+	}
 
-			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 success() default applies to it separately
+	if !expressionCallsFunction(in, "success", "always", "failure", "cancelled") {
+		status, err := ee.interpreter.Evaluate("success()", exprparser.DefaultStatusCheckNone)
+		if err != nil {
+			return false, err
+		}
+		if !exprparser.IsTruthy(status) {
+			return false, nil
 		}
 	}
+	interpolated, err := ee.interpolate(in)
+	if err != nil {
+		return false, err
+	}
+	return exprparser.IsTruthy(interpolated), nil
+}
 
-	if len(results) == 1 && formatOut.String() == "{0}" && !forceFormat {
-		return in, nil
+// coerceToString converts an evaluated expression value to a string the way GitHub does,
+// see https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#operators
+// An already reflected value is accepted as-is, since Interface() would panic on an invalid one.
+func coerceToString(v any) string {
+	value, ok := v.(reflect.Value)
+	if !ok {
+		value = reflect.ValueOf(v)
 	}
 
-	out := fmt.Sprintf("format('%s', %s)", strings.ReplaceAll(formatOut.String(), "'", "''"), strings.Join(results, ", "))
-	return out, nil
+	switch value.Kind() {
+	case reflect.Invalid:
+		return ""
+
+	case reflect.Bool:
+		return strconv.FormatBool(value.Bool())
+
+	case reflect.String:
+		return value.String()
+
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		return strconv.FormatInt(value.Int(), 10)
+
+	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
+		return strconv.FormatUint(value.Uint(), 10)
+
+	case reflect.Float32, reflect.Float64:
+		if math.IsInf(value.Float(), 1) {
+			return "Infinity"
+		} else if math.IsInf(value.Float(), -1) {
+			return "-Infinity"
+		}
+		return fmt.Sprintf("%.15G", value.Float())
+
+	case reflect.Slice, reflect.Array:
+		return "Array"
+
+	// contexts such as `github` are pointers to structs, so they stringify as objects too
+	case reflect.Map, reflect.Struct:
+		return "Object"
+
+	case reflect.Interface, reflect.Pointer:
+		if value.IsNil() {
+			return ""
+		}
+		return coerceToString(value.Elem())
+	}
+
+	return fmt.Sprintf("%v", value)
+}
+
+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
+	}
+
+	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("}}"):]
+	}
+}
+
+// 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
 }

modules/actions/jobparser/jobparser.go

@@ -72,9 +72,14 @@ func ExpressionReadsMatrix(ifValue string) bool {
 // the status functions that run a job whatever its needs did rather than under the implicit success().
 // Keep in sync with act's exprparser, which owns the same list for the evaluation itself.
 func ExpressionIgnoresNeedResults(ifValue string) bool {
-	return expressionsMatch(asIfExpression(ifValue), func(node actionlint.ExprNode) bool {
+	return expressionCallsFunction(asIfExpression(ifValue), "always", "failure", "cancelled")
+}
+
+// expressionCallsFunction reports whether any ${{ }} expression in value calls one of the functions.
+func expressionCallsFunction(value string, names ...string) bool {
+	return expressionsMatch(value, func(node actionlint.ExprNode) bool {
 		call, ok := node.(*actionlint.FuncCallNode)
-		return ok && slices.Contains([]string{"always", "failure", "cancelled"}, strings.ToLower(call.Callee))
+		return ok && slices.Contains(names, strings.ToLower(call.Callee))
 	})
 }
 
@@ -98,14 +103,16 @@ func expressionReadsContext(value, contextName string) bool {
 
 // expressionsMatch reports whether any ${{ }} expression in value holds a node the predicate accepts.
 func expressionsMatch(value string, match func(node actionlint.ExprNode) bool) bool {
-	for rest := value; ; {
-		_, after, found := strings.Cut(rest, "${{")
-		if !found {
-			return false
+	parts, err := splitSubExpressions(value)
+	if err != nil {
+		return true // unparseable here, let the expansion report it against the real values
+	}
+	for _, part := range parts {
+		if !part.isExpr {
+			continue
 		}
-		rest = after
-		// The lexer ends the expression at its closing `}}`, so it can be handed the whole remainder.
-		expr, err := actionlint.NewExprParser().Parse(actionlint.NewExprLexer(rest))
+		// The lexer needs the closing `}}` that the scanner strips.
+		expr, err := actionlint.NewExprParser().Parse(actionlint.NewExprLexer(part.text + "}}"))
 		if err != nil {
 			return true // unparseable here, let the expansion report it against the real values
 		}
@@ -119,6 +126,7 @@ func expressionsMatch(value string, match func(node actionlint.ExprNode) bool) b
 			return true
 		}
 	}
+	return false
 }
 
 func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) {
@@ -155,7 +163,9 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) {
 	}
 
 	evaluator := NewExpressionEvaluator(exprparser.NewInterpeter(&exprparser.EvaluationEnvironment{Github: pc.gitContext, Vars: pc.vars, Inputs: pc.inputs}, exprparser.Config{}))
-	workflow.RunName = evaluator.Interpolate(workflow.RunName)
+	if workflow.RunName, err = evaluator.interpolate(workflow.RunName); err != nil {
+		return nil, fmt.Errorf("interpolate run-name: %w", err)
+	}
 
 	for i, id := range ids {
 		job := jobs[i]
@@ -289,6 +299,7 @@ func validateMatrixFilters(job *model.Job) error {
 func buildMatrixCombos(jobID string, src *Job, matrixes []map[string]any, actJob *model.Job, gitCtx *model.GithubContext, results map[string]*JobResult, vars map[string]string, inputs map[string]any) ([]*Job, error) {
 	srcRunsOn := src.RunsOn()
 	combos := make([]*Job, 0, len(matrixes))
+	var err error
 	for _, matrix := range matrixes {
 		combo := src.Clone()
 		if combo.Name == "" {
@@ -296,10 +307,14 @@ func buildMatrixCombos(jobID string, src *Job, matrixes []map[string]any, actJob
 		}
 		combo.Strategy.RawMatrix = encodeMatrix(matrix)
 		evaluator := NewExpressionEvaluator(NewInterpeter(jobID, actJob, matrix, gitCtx, results, vars, inputs))
-		combo.Name = nameWithMatrix(combo.Name, matrix, evaluator)
+		if combo.Name, err = nameWithMatrix(combo.Name, matrix, evaluator); err != nil {
+			return nil, fmt.Errorf("interpolate name for job %q: %w", jobID, err)
+		}
 		runsOn := slices.Clone(srcRunsOn)
 		for i := range runsOn {
-			runsOn[i] = evaluator.Interpolate(runsOn[i])
+			if runsOn[i], err = evaluator.interpolate(runsOn[i]); err != nil {
+				return nil, fmt.Errorf("interpolate runs-on for job %q: %w", jobID, err)
+			}
 		}
 		combo.RawRunsOn = encodeRunsOn(runsOn)
 		if err := evaluator.EvaluateYamlNode(&combo.RawContinueOnError); err != nil {
@@ -371,16 +386,16 @@ func encodeRunsOn(runsOn []string) yaml.Node {
 	return node
 }
 
-func nameWithMatrix(name string, m map[string]any, evaluator *ExpressionEvaluator) string {
+func nameWithMatrix(name string, m map[string]any, evaluator *ExpressionEvaluator) (string, error) {
 	if len(m) == 0 {
-		return name
+		return name, nil
 	}
 
 	if !strings.Contains(name, "${{") || !strings.Contains(name, "}}") {
-		return name + " " + matrixName(m)
+		return name + " " + matrixName(m), nil
 	}
 
-	return evaluator.Interpolate(name)
+	return evaluator.interpolate(name)
 }
 
 func matrixName(m map[string]any) string {

modules/actions/jobparser/jobparser_test.go

@@ -160,6 +160,45 @@ jobs:
 	}
 }
 
+func TestParseInterpolatesRunName(t *testing.T) {
+	workflow := func(runName string) []byte {
+		return []byte("name: t\nrun-name: \"" + runName + "\"\non: push\njobs:\n  build:\n    runs-on: ubuntu-latest\n    steps: [{run: echo}]\n")
+	}
+
+	for _, tt := range []struct{ name, runName, want string }{
+		{"bool", "${{ true }}", "true"},
+		{"int", "${{ 1 }}", "1"},
+		{"float", "${{ 1.0 }}", "1"},
+		{"null", "${{ null }}", ""},
+		{"object", `${{ fromJSON('{\"a\":1}') }}`, "Object"},
+		{"array", "${{ fromJSON('[1,2]') }}", "Array"},
+		{"context", "${{ github }}", "Object"},
+		{"surrounding literals", "run ${{ 1 }} now", "run 1 now"},
+		{"two expressions", "${{ 1 }}-${{ true }}", "1-true"},
+		{"closing brace inside a string", "${{ 'a}}b' }}", "a}}b"},
+		{"incomplete expression stays literal", "${{ 1", "${{ 1"},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := Parse(workflow(tt.runName), WithGitContext(&model.GithubContext{EventName: "push"}))
+			require.NoError(t, err)
+			require.Len(t, result, 1)
+			assert.Equal(t, tt.want, result[0].RunName)
+		})
+	}
+
+	// a malformed part must not restructure the surrounding expression
+	for _, runName := range []string{"${{ 1) && (2 }}", "run ${{ 1) && (2 }} now", "${{ 'a' }} ${{ b", "${{ 'a }}"} {
+		_, err := Parse(workflow(runName), WithGitContext(&model.GithubContext{EventName: "push"}))
+		assert.ErrorContains(t, err, "interpolate run-name")
+	}
+
+	// callers such as commit status parse without a git context, leaving `github` a nil pointer
+	result, err := Parse(workflow("${{ github }}"))
+	require.NoError(t, err)
+	require.Len(t, result, 1)
+	assert.Empty(t, result[0].RunName)
+}
+
 func TestExpandMatrixWithNeeds(t *testing.T) {
 	// matrixYAML is the YAML value of the `matrix:` key, so a case can replace the whole node.
 	expandMax := func(t *testing.T, matrixYAML string, maxCombinations int) ([]*Job, error) {

modules/actions/jobparser/model.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 
-	"gitea.com/gitea/runner/act/exprparser"
 	"gitea.com/gitea/runner/act/model"
 	"go.yaml.in/yaml/v4"
 )
@@ -526,15 +525,7 @@ func EvaluateJobIfExpression(jobID string, job *Job, gitCtx map[string]any, resu
 		}
 	}
 	evaluator := NewExpressionEvaluator(NewInterpeter(jobID, actJob, matrix, toGitContext(gitCtx), results, vars, inputs))
-	expr, err := rewriteSubExpression(job.If.Value, false)
-	if err != nil {
-		return false, err
-	}
-	result, err := evaluator.evaluate(expr, exprparser.DefaultStatusCheckSuccess)
-	if err != nil {
-		return false, err
-	}
-	return exprparser.IsTruthy(result), nil
+	return evaluator.evaluateCondition(job.If.Value)
 }
 
 // parseMappingNode parse a mapping node and preserve order.

modules/actions/jobparser/model_test.go

@@ -527,6 +527,10 @@ func TestEvaluateJobIfExpression(t *testing.T) {
 		{name: "cancelled", ifCond: "${{ cancelled() }}", needResult: "success", expected: false},
 		{name: "not cancelled or failure", ifCond: "${{ !(cancelled() || failure()) }}", needResult: "success", expected: true},
 		{name: "not cancelled or failure, need failed", ifCond: "${{ !(cancelled() || failure()) }}", needResult: "failure", expected: false},
+		// a condition is an expression with or without `${{ }}`, literal text around one makes it a string
+		{name: "bare expression", ifCond: "always()", needResult: "failure", expected: true},
+		{name: "literal text keeps the success() default", ifCond: "x ${{ 1 }}", needResult: "failure", expected: false},
+		{name: "literal text around a status function drops it", ifCond: "x ${{ always() }}", needResult: "failure", expected: true},
 	}
 	for _, kase := range kases {
 		t.Run(kase.name, func(t *testing.T) {

modules/actions/jobparser/workflow_call.go

@@ -260,7 +260,7 @@ func MatchCallerInputsAgainstSpec(spec *WorkflowCallSpec, evaluated map[string]a
 func parseWorkflowCallInput(name string, typ InputType, v any) (any, error) {
 	switch typ {
 	case InputTypeString:
-		return toString(v), nil
+		return coerceToString(v), nil
 	case InputTypeBoolean:
 		// strict type matching: a boolean input only accepts a native bool, not a "true"/"false" string
 		if b, ok := v.(bool); ok {
@@ -361,11 +361,11 @@ func EvaluateWorkflowCallOutputs(spec *WorkflowCallSpec, gitCtx *model.GithubCon
 		Vars:   vars,
 		Inputs: inputs,
 	}
-	interpreter := exprparser.NewInterpeter(env, exprparser.Config{})
+	evaluator := NewExpressionEvaluator(exprparser.NewInterpeter(env, exprparser.Config{}))
 
 	out := make(map[string]string, len(spec.Outputs))
 	for name, o := range spec.Outputs {
-		v, err := evaluateWorkflowCallOutputValue(interpreter, o.Value)
+		v, err := evaluator.interpolate(o.Value)
 		if err != nil {
 			return nil, fmt.Errorf("workflow_call output %q: %w", name, err)
 		}
@@ -373,29 +373,3 @@ func EvaluateWorkflowCallOutputs(spec *WorkflowCallSpec, gitCtx *model.GithubCon
 	}
 	return out, nil
 }
-
-func evaluateWorkflowCallOutputValue(interpreter exprparser.Interpreter, value string) (string, error) {
-	if !strings.Contains(value, "${{") || !strings.Contains(value, "}}") {
-		return value, nil
-	}
-	expr, err := rewriteSubExpression(value, true)
-	if err != nil {
-		return "", err
-	}
-	evaluated, err := interpreter.Evaluate(expr, exprparser.DefaultStatusCheckNone)
-	if err != nil {
-		return "", err
-	}
-	return toString(evaluated), nil
-}
-
-func toString(v any) string {
-	switch s := v.(type) {
-	case string:
-		return s
-	case nil:
-		return ""
-	default:
-		return fmt.Sprintf("%v", s)
-	}
-}