From 47366f8f34169c6b577ac3b748100b318146647c Mon Sep 17 00:00:00 2001 From: silverwind Date: Sun, 2 Aug 2026 22:18:20 +0000 Subject: [PATCH] fix: coerce every expression value kind to a string and drop the `format()` panic (#1135) `coerceToString` covered only a few kinds and returned the unconverted `reflect.Value` for the rest, so callers rendered contexts as `<*model.GithubContext Value>` and mangled sized integers and floats the same way. It now returns a string, which makes that placeholder unrepresentable, and is exported as `CoerceToString` so Gitea can drop its own diverging copy. It also takes an already reflected value, so internal callers need no conversion and no guard against the zero `Value`. `format()` panicked whenever a lone `}` was followed by anything other than another `}`. Only a trailing `}` reached the existing unmatched-brace check, so an expression such as `format('a}b')`, which any workflow can write, took the process down instead. It now returns that same error. Reviewed-on: https://gitea.com/gitea/runner/pulls/1135 Reviewed-by: Lunny Xiao Co-authored-by: silverwind --- act/exprparser/functions.go | 24 ++++----- act/exprparser/functions_test.go | 3 ++ act/exprparser/interpreter.go | 80 ++++++++++++++++++------------ act/exprparser/interpreter_test.go | 49 ++++++++++++++++++ 4 files changed, 111 insertions(+), 45 deletions(-) diff --git a/act/exprparser/functions.go b/act/exprparser/functions.go index a4853bfb..7987273b 100644 --- a/act/exprparser/functions.go +++ b/act/exprparser/functions.go @@ -28,8 +28,8 @@ func (impl *interperterImpl) contains(search, item reflect.Value) (bool, error) switch search.Kind() { case reflect.String, reflect.Int, reflect.Float64, reflect.Bool, reflect.Invalid: return strings.Contains( - strings.ToLower(impl.coerceToString(search).String()), - strings.ToLower(impl.coerceToString(item).String()), + strings.ToLower(CoerceToString(search)), + strings.ToLower(CoerceToString(item)), ), nil case reflect.Slice: @@ -51,15 +51,15 @@ func (impl *interperterImpl) contains(search, item reflect.Value) (bool, error) func (impl *interperterImpl) startsWith(searchString, searchValue reflect.Value) (bool, error) { //nolint:unparam // pre-existing issue from nektos/act return strings.HasPrefix( - strings.ToLower(impl.coerceToString(searchString).String()), - strings.ToLower(impl.coerceToString(searchValue).String()), + strings.ToLower(CoerceToString(searchString)), + strings.ToLower(CoerceToString(searchValue)), ), nil } func (impl *interperterImpl) endsWith(searchString, searchValue reflect.Value) (bool, error) { //nolint:unparam // pre-existing issue from nektos/act return strings.HasSuffix( - strings.ToLower(impl.coerceToString(searchString).String()), - strings.ToLower(impl.coerceToString(searchValue).String()), + strings.ToLower(CoerceToString(searchString)), + strings.ToLower(CoerceToString(searchValue)), ), nil } @@ -70,7 +70,7 @@ const ( ) func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.Value) (string, error) { - input := impl.coerceToString(str).String() + input := CoerceToString(str) var output strings.Builder replacementIndex := "" @@ -108,7 +108,7 @@ func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.V return "", fmt.Errorf("The following format string references more arguments than were supplied: '%s'", input) } - output.WriteString(impl.coerceToString(replaceValue[index]).String()) + output.WriteString(CoerceToString(replaceValue[index])) state = passThrough @@ -124,7 +124,7 @@ func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.V state = passThrough default: - panic("Invalid format parser state") + return "", fmt.Errorf("Closing bracket without opening one. The following format string is invalid: '%s'", input) } } } @@ -143,17 +143,17 @@ func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.V } func (impl *interperterImpl) join(array, sep reflect.Value) (string, error) { //nolint:unparam // pre-existing issue from nektos/act - separator := impl.coerceToString(sep).String() + separator := CoerceToString(sep) switch array.Kind() { case reflect.Slice: var items []string for i := 0; i < array.Len(); i++ { - items = append(items, impl.coerceToString(array.Index(i).Elem()).String()) + items = append(items, CoerceToString(array.Index(i))) } return strings.Join(items, separator), nil default: - return strings.Join([]string{impl.coerceToString(array).String()}, separator), nil + return strings.Join([]string{CoerceToString(array)}, separator), nil } } diff --git a/act/exprparser/functions_test.go b/act/exprparser/functions_test.go index 707bf6ab..b380f586 100644 --- a/act/exprparser/functions_test.go +++ b/act/exprparser/functions_test.go @@ -121,6 +121,7 @@ func TestFunctionJoin(t *testing.T) { {"join(fromJSON('[\"a\", \"b\", null]'), null)", "ab", "join-number"}, {"join(fromJSON('[\"a\", \"b\"]'))", "a,b", "join-number"}, {"join(fromJSON('[\"a\", \"b\", null]'), 1)", "a1b1", "join-number"}, + {"join(fromJSON('[1, true, null]'), '-')", "1-true-", "join-mixed-types"}, } env := &EvaluationEnvironment{} @@ -230,8 +231,10 @@ func TestFunctionFormat(t *testing.T) { {`format('Hello "{0}" {1} {2} {3} {4}', null, true, -3.14, NaN, Infinity)`, `Hello "" true -3.14 NaN Infinity`, nil, "format-with-primitives"}, {`format('Hello "{0}" {1} {2}', fromJSON('[0, true, "abc"]'), fromJSON('[{"a":1}]'), fromJSON('{"a":{"b":1}}'))`, `Hello "Array" Array Object`, nil, "format-with-complex-types"}, {"format(true)", "true", nil, "format-with-primitive-args"}, + {"format('{0}', github)", "Object", nil, "format-with-context"}, {"format('echo Hello {0} ${{Test}}', github.undefined_property)", "echo Hello ${Test}", nil, "format-with-undefined-value"}, {"format('{0}}', '{1}', 'World')", nil, "Closing bracket without opening one. The following format string is invalid: '{0}}'", "format-invalid-format-string"}, + {"format('a}b')", nil, "Closing bracket without opening one. The following format string is invalid: 'a}b'", "format-unmatched-closing-brace"}, {"format('{0', '{1}', 'World')", nil, "Unclosed brackets. The following format string is invalid: '{0'", "format-invalid-format-string"}, {"format('{2}', '{1}', 'World')", "", "The following format string references more arguments than were supplied: '{2}'", "format-invalid-replacement-reference"}, {"format('{2147483648}')", "", "The following format string is invalid: '{2147483648}'", "format-invalid-replacement-reference"}, diff --git a/act/exprparser/interpreter.go b/act/exprparser/interpreter.go index 6de554eb..dedb55fd 100644 --- a/act/exprparser/interpreter.go +++ b/act/exprparser/interpreter.go @@ -10,6 +10,7 @@ import ( "fmt" "math" "reflect" + "strconv" "strings" "gitea.com/gitea/runner/act/model" @@ -429,41 +430,54 @@ func (impl *interperterImpl) coerceToNumber(value reflect.Value) reflect.Value { return reflect.ValueOf(math.NaN()) } -func (impl *interperterImpl) coerceToString(value reflect.Value) reflect.Value { - switch value.Kind() { - case reflect.Invalid: - return reflect.ValueOf("") - - case reflect.Bool: - switch value.Bool() { - case true: - return reflect.ValueOf("true") - case false: - return reflect.ValueOf("false") - } - - case reflect.String: - return value - - case reflect.Int: - return reflect.ValueOf(fmt.Sprint(value)) - - case reflect.Float64: - if math.IsInf(value.Float(), 1) { - return reflect.ValueOf("Infinity") - } else if math.IsInf(value.Float(), -1) { - return reflect.ValueOf("-Infinity") - } - return reflect.ValueOf(fmt.Sprintf("%.15G", value.Float())) - - case reflect.Slice: - return reflect.ValueOf("Array") - - case reflect.Map: - return reflect.ValueOf("Object") +// 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) } - return value + 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) } func (impl *interperterImpl) compareString(left, right string, kind actionlint.CompareOpNodeKind) (bool, error) { diff --git a/act/exprparser/interpreter_test.go b/act/exprparser/interpreter_test.go index b1484e11..e1e94283 100644 --- a/act/exprparser/interpreter_test.go +++ b/act/exprparser/interpreter_test.go @@ -6,6 +6,7 @@ package exprparser import ( "math" + "reflect" "testing" "gitea.com/gitea/runner/act/model" @@ -633,3 +634,51 @@ func TestContexts(t *testing.T) { }) } } + +func TestCoerceToString(t *testing.T) { + type object struct{ Name string } + obj := object{Name: "x"} + var nilPointer *object + var nilMap map[string]any + var nilSlice []any + + table := []struct { + input any + expected string + name string + }{ + {nil, "", "null"}, + {true, "true", "true"}, + {false, "false", "false"}, + {"foo", "foo", "string"}, + {"", "", "empty-string"}, + {123, "123", "int"}, + {int64(-9), "-9", "int64"}, + {uint8(7), "7", "uint8"}, + {1.0, "1", "float-integral"}, + {-9.7, "-9.7", "float"}, + {2.99e-2, "0.0299", "float-exponential"}, + {1e21, "1E+21", "float-large"}, + {float32(1.5), "1.5", "float32"}, + {math.NaN(), "NaN", "nan"}, + {math.Inf(1), "Infinity", "positive-infinity"}, + {math.Inf(-1), "-Infinity", "negative-infinity"}, + {[]any{1, 2}, "Array", "slice"}, + {nilSlice, "Array", "nil-slice"}, + {[2]int{1, 2}, "Array", "fixed-size-array"}, + {map[string]any{"a": 1}, "Object", "map"}, + {nilMap, "Object", "nil-map"}, + {obj, "Object", "struct"}, + {&obj, "Object", "pointer-to-struct"}, + {nilPointer, "", "nil-pointer"}, + {&model.GithubContext{Action: "push"}, "Object", "github-context"}, + {reflect.ValueOf(42), "42", "reflected-value"}, + {reflect.Value{}, "", "invalid-reflected-value"}, + } + + for _, tt := range table { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, CoerceToString(tt.input)) + }) + } +}