From 12dc9d26a25a35b4664be331c6defa99f6760ce8 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 27 Aug 2026 09:36:27 +0000 Subject: [PATCH] fix: improve behaviour across masking, commands and status (#1194) Fixes 51 bugs discovered via comparison with `actions/runner`. Every fix has test coverage. ### Secrets - A short secret registered no shifted-base64 form, so `base64("user:$TOKEN")` printed in the clear - Encoded forms came only from the whole trimmed value, missing padded and per-line spellings - Masks split only on `\n`, so `::add-mask::a%0Db` registered neither half - Adds XML, expression-string and quote-trimming encoders ### Workflow commands - Split at the last `::` or `]` rather than the first, so `::add-mask::a::b` registered no mask - A command on the last line without a newline was ignored, and `::ADD-MASK::` did nothing - `##[...]` did not decode `%3B`/`%5D`, properties lost anything after a second `=` - `$GITHUB_ENV` and `::set-env::` now refuse `NODE_OPTIONS` ### Status - `continue-on-error` reported failed, a cancelled job reported success, an `if:` error reported cancelled - File commands ran after `continue-on-error`, failing the job while the step stayed green - A bad job output aborted the whole run instead of that job ### Steps and actions - `${{ matrix.* }}` and `${{ strategy.* }}` were empty inside composite actions - Composite inputs leaked into nested actions as `INPUT_*`, `with:` matched case-sensitively, `pre` failures were dropped - Docker actions dropped `runs.env` when the caller passed `with: args:`, and caller `args`/`entrypoint` beat the manifest - An implicit shell ran with `pipefail`, a `shell:` without `{0}` passed without running - `container.env` overrode job env and every `$GITHUB_ENV` write, heredocs lost leading blank lines, `$GITHUB_PATH` was not BOM-decoded Written by Claude Opus 5. Reviewed-on: https://gitea.com/gitea/runner/pulls/1194 Reviewed-by: bircni Co-authored-by: silverwind --- act/container/docker_run.go | 8 ++ act/container/docker_run_test.go | 11 +++ act/container/host_environment.go | 7 +- act/container/host_environment_test.go | 5 ++ act/container/parse_env_file.go | 9 +- act/container/parse_env_file_test.go | 5 ++ act/runner/action.go | 30 ++++--- act/runner/action_composite.go | 25 +++++- act/runner/action_composite_test.go | 44 ++++++++++ act/runner/action_test.go | 71 +++++++++++----- act/runner/cancellation_test.go | 2 + act/runner/command.go | 55 ++++++++++--- act/runner/command_test.go | 27 +++++- act/runner/job_executor.go | 25 ++++-- act/runner/job_executor_test.go | 30 +++++-- act/runner/job_hooks.go | 10 ++- act/runner/logger.go | 110 +++++++++++++++---------- act/runner/logger_test.go | 62 +++++++------- act/runner/run_context.go | 44 ++++++++-- act/runner/run_context_test.go | 46 ++++++++++- act/runner/step.go | 56 ++++++------- act/runner/step_action_remote.go | 2 +- act/runner/step_action_remote_test.go | 17 ++++ act/runner/step_run.go | 57 ++++++++++--- act/runner/step_run_test.go | 108 ++++++++++++++++++++++++ act/runner/step_test.go | 92 +++++++++++++++++++++ internal/pkg/report/reporter.go | 14 +++- internal/pkg/report/reporter_test.go | 5 +- 28 files changed, 771 insertions(+), 206 deletions(-) diff --git a/act/container/docker_run.go b/act/container/docker_run.go index 21ed49d1..60aea8e8 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -583,6 +583,14 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config if err != nil { return nil, nil, fmt.Errorf("cannot process container options: '%s': '%w'", options, err) } + // workflow aliases join the runner's own, deduped so a re-create cannot grow the input + for _, endpoint := range containerConfig.NetworkingConfig.EndpointsConfig { + for _, alias := range endpoint.Aliases { + if !slices.Contains(cr.input.NetworkAliases, alias) { + cr.input.NetworkAliases = append(cr.input.NetworkAliases, alias) + } + } + } // For Gitea, forcing --privileged off is not enough, other options reach the host too if !hostConfig.Privileged { diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index dc274055..728f186b 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -833,6 +833,17 @@ func TestMergeContainerConfigsWarnsOnlyAboutOptionsThatWereGiven(t *testing.T) { assert.Equal(t, 1, warnings("--network host", "")) } +func TestMergeContainerConfigsKeepsNetworkAliasesFromOptions(t *testing.T) { + logger, _ := test.NewNullLogger() + cr := &containerReference{input: &NewContainerInput{ + NetworkMode: "job-network", NetworkAliases: []string{"redis"}, WorkflowOptions: "--network-alias redis-primary", + }} + + _, _, err := cr.mergeContainerConfigs(common.WithLogger(t.Context(), logger), &container.Config{}, &container.HostConfig{}) + require.NoError(t, err) + assert.Equal(t, []string{"redis", "redis-primary"}, cr.input.NetworkAliases) +} + // A dead daemon must fail the job, not panic through logrus and not silently // drop the requested platform. func TestSupportsContainerImagePlatformDaemonError(t *testing.T) { diff --git a/act/container/host_environment.go b/act/container/host_environment.go index 20b02ddf..796ef1eb 100644 --- a/act/container/host_environment.go +++ b/act/container/host_environment.go @@ -629,8 +629,11 @@ func (*HostEnvironment) JoinPathVariable(paths ...string) string { func goArchToActionArch(arch string) string { archMapper := map[string]string{ "x86_64": "X64", + "amd64": "X64", "386": "X86", + "arm": "ARM", "aarch64": "ARM64", + "arm64": "ARM64", } if arch, ok := archMapper[arch]; ok { return arch @@ -640,7 +643,9 @@ func goArchToActionArch(arch string) string { func goOsToActionOs(os string) string { osMapper := map[string]string{ - "darwin": "macOS", + "linux": "Linux", + "darwin": "macOS", + "windows": "Windows", } if os, ok := osMapper[os]; ok { return os diff --git a/act/container/host_environment_test.go b/act/container/host_environment_test.go index 2f5d1c5e..bb2598f7 100644 --- a/act/container/host_environment_test.go +++ b/act/container/host_environment_test.go @@ -27,6 +27,11 @@ import ( // Type assert HostEnvironment implements ExecutionsEnvironment var _ ExecutionsEnvironment = &HostEnvironment{} +func TestActionPlatformNames(t *testing.T) { + assert.Equal(t, []string{"Linux", "macOS", "Windows"}, []string{goOsToActionOs("linux"), goOsToActionOs("darwin"), goOsToActionOs("windows")}) + assert.Equal(t, []string{"X86", "X64", "ARM", "ARM64"}, []string{goArchToActionArch("386"), goArchToActionArch("amd64"), goArchToActionArch("arm"), goArchToActionArch("arm64")}) +} + func TestCopyDir(t *testing.T) { dir := t.TempDir() ctx := context.Background() diff --git a/act/container/parse_env_file.go b/act/container/parse_env_file.go index 593ddb1c..fe1761e7 100644 --- a/act/container/parse_env_file.go +++ b/act/container/parse_env_file.go @@ -50,7 +50,7 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Ex case singleLineEnv != -1 && (multiLineEnv == -1 || singleLineEnv < multiLineEnv): localEnv[line[:singleLineEnv]] = line[singleLineEnv+1:] case multiLineEnv != -1: - multiLineEnvContent := "" + var multiLineEnvContent []string multiLineEnvDelimiter := line[multiLineEnv+2:] delimiterFound := false for s.Scan() { @@ -59,10 +59,7 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Ex delimiterFound = true break } - if multiLineEnvContent != "" { - multiLineEnvContent += "\n" - } - multiLineEnvContent += content + multiLineEnvContent = append(multiLineEnvContent, content) } if err := s.Err(); err != nil { return fmt.Errorf("reading env file: %w", err) @@ -70,7 +67,7 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Ex if !delimiterFound { return fmt.Errorf("invalid format delimiter '%v' not found before end of file", multiLineEnvDelimiter) } - localEnv[line[:multiLineEnv]] = multiLineEnvContent + localEnv[line[:multiLineEnv]] = strings.Join(multiLineEnvContent, "\n") default: return fmt.Errorf("invalid format '%v', expected a line with '=' or '<<'", line) } diff --git a/act/container/parse_env_file_test.go b/act/container/parse_env_file_test.go index 891b0f37..c11831f2 100644 --- a/act/container/parse_env_file_test.go +++ b/act/container/parse_env_file_test.go @@ -86,6 +86,11 @@ func TestParseEnvFileMultiLineKeepsBlankLines(t *testing.T) { env := map[string]string{} require.NoError(t, parseEnvFile(e, envPath, &env)(context.Background())) assert.Equal(t, "line1\n\nline2", env["FOO"]) + + require.NoError(t, os.WriteFile(envPath, []byte("FOO< 0 { - return fields, nil - } entrypoint = runs.Entrypoint + if entrypoint == "" { + if fields := strings.Fields(eval.Interpolate(ctx, step.getStepModel().With["entrypoint"])); len(fields) > 0 { + return fields, nil + } + } } if entrypoint == "" { @@ -393,7 +399,8 @@ func dockerEntrypoint(ctx context.Context, step actionStep, eval *expressionEval return shellquote.Split(entrypoint) } -func evalDockerArgs(ctx context.Context, step step, action *model.Action, cmd *[]string) { +// evalDockerEnv returns an evaluator bound to the environment it installed. +func evalDockerEnv(ctx context.Context, step step, action *model.Action) *expressionEvaluator { rc := step.getRunContext() stepModel := step.getStepModel() @@ -410,16 +417,15 @@ func evalDockerArgs(ctx context.Context, step step, action *model.Action, cmd *[ } mergeIntoMap(step, step.getEnv(), inputs) - stepEE := rc.NewActionInputsExpressionEvaluator(ctx, step) - for i, v := range *cmd { - (*cmd)[i] = stepEE.Interpolate(ctx, v) - } - mergeIntoMap(step, step.getEnv(), action.Runs.Env) + env := make(map[string]string, len(action.Runs.Env)+len(*step.getEnv())) + mergeIntoMap(step, &env, action.Runs.Env, *step.getEnv()) + *step.getEnv() = env ee := rc.NewActionInputsExpressionEvaluator(ctx, step) for k, v := range *step.getEnv() { (*step.getEnv())[k] = ee.Interpolate(ctx, v) } + return ee } func newStepContainer(ctx context.Context, step step, image string, cmd, entrypoint []string, runnerOptions string) container.Container { diff --git a/act/runner/action_composite.go b/act/runner/action_composite.go index 30eb3fc4..eeaf7e0b 100644 --- a/act/runner/action_composite.go +++ b/act/runner/action_composite.go @@ -36,7 +36,13 @@ func evaluateCompositeInputAndEnv(ctx context.Context, parent *RunContext, step // lookup if key is defined in the step but the already // evaluated value from the environment - _, defined := step.getStepModel().With[inputID] + defined := false + for key := range step.getStepModel().With { + if strings.EqualFold(key, inputID) { + defined = true + break + } + } if value, ok := stepEnv[envKey]; defined && ok { env[envKey] = value } else { @@ -51,6 +57,15 @@ func evaluateCompositeInputAndEnv(ctx context.Context, parent *RunContext, step return env } +func (rc *RunContext) setCompositeActionEnv(env map[string]string) { + rc.setActionEnv(env) + for key := range rc.Env { + if strings.HasPrefix(key, "INPUT_") { + delete(rc.Env, key) + } + } +} + func newCompositeRunContext(ctx context.Context, parent *RunContext, step actionStep, actionPath string) *RunContext { env := evaluateCompositeInputAndEnv(ctx, parent, step) @@ -62,12 +77,13 @@ func newCompositeRunContext(ctx context.Context, parent *RunContext, step action compositerc := &RunContext{ Name: parent.Name, JobName: parent.JobName, + Matrix: parent.Matrix, Run: &model.Run{ JobID: parent.Run.JobID, Workflow: &model.Workflow{ Name: parent.Run.Workflow.Name, Jobs: map[string]*model.Job{ - parent.Run.JobID: {}, + parent.Run.JobID: {Strategy: parent.Run.Job().Strategy}, }, }, }, @@ -81,7 +97,7 @@ func newCompositeRunContext(ctx context.Context, parent *RunContext, step action Parent: parent, EventJSON: parent.EventJSON, } - compositerc.setActionEnv(env) + compositerc.setCompositeActionEnv(env) compositerc.ExprEval = compositerc.NewExpressionEvaluator(ctx) return compositerc @@ -131,7 +147,7 @@ func execAsComposite(step actionStep) common.Executor { // repeated composite actions grow rc.Masks exponentially. rc.Masks = appendUniqueMasks(rc.Masks, compositeRC.Masks) rc.ExtraPath = compositeRC.ExtraPath - // compositeRC.Env is dirty, contains INPUT_ and merged step env, only rely on compositeRC.GlobalEnv + // Propagate GlobalEnv only, so composite inputs and step-local values do not escape. mergeIntoMap := mergeIntoMapCaseSensitive if rc.JobContainer.IsEnvironmentCaseInsensitive() { mergeIntoMap = mergeIntoMapCaseInsensitive @@ -194,6 +210,7 @@ func (rc *RunContext) compositeExecutor(action *model.Action) *compositeSteps { } steps = append(steps, common.JobError) + preSteps = append(preSteps, common.JobError) return &compositeSteps{ pre: func(ctx context.Context) error { return common.NewPipelineExecutor(preSteps...)(common.WithJobErrorContainer(ctx)) diff --git a/act/runner/action_composite_test.go b/act/runner/action_composite_test.go index c8faeedb..47550dc6 100644 --- a/act/runner/action_composite_test.go +++ b/act/runner/action_composite_test.go @@ -6,9 +6,53 @@ package runner import ( "testing" + "gitea.com/gitea/runner/act/common" + "gitea.com/gitea/runner/act/common/git" + + "gitea.dev/actionslib/pkg/model" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestCompositeActionParity(t *testing.T) { + t.Run("inherits contexts without leaking inputs", func(t *testing.T) { + ctx := t.Context() + strategy := &model.Strategy{MaxParallel: 3} + parent := &RunContext{ + Config: &Config{}, + Matrix: map[string]any{"os": "linux"}, + Run: &model.Run{JobID: "job", Workflow: &model.Workflow{Name: "workflow", Jobs: map[string]*model.Job{"job": {Strategy: strategy}}}}, + JobContainer: &jobContainerMock{}, + } + composite := newCompositeRunContext(ctx, parent, &stepActionRemote{ + Step: &model.Step{With: map[string]string{"SHARED": "outer"}}, + RunContext: parent, + action: &model.Action{Inputs: map[string]model.Input{"shared": {Default: "outer-default"}}}, + env: map[string]string{"INPUT_SHARED": "outer"}, + }, "/action") + + assert.Same(t, strategy, composite.Run.Job().Strategy) + assert.Equal(t, "linux|3|outer", composite.NewExpressionEvaluator(ctx).Interpolate(ctx, + "${{ matrix.os }}|${{ strategy.max-parallel }}|${{ inputs.shared }}")) + assert.NotContains(t, composite.Env, "INPUT_SHARED") + + nestedEnv := composite.GetEnv() + populateEnvsFromInput(ctx, &nestedEnv, &model.Action{Inputs: map[string]model.Input{"shared": {Default: "inner-default"}}}, composite) + assert.Equal(t, "inner-default", nestedEnv["INPUT_SHARED"]) + }) + + t.Run("propagates pre failures", func(t *testing.T) { + setCloneExecutor(t, func(git.NewGitCloneExecutorInput) common.Executor { return common.NewErrorExecutor(assert.AnError) }) + rc := &RunContext{ + Config: &Config{GitHubInstance: "github.com", ActionCacheDir: t.TempDir()}, + Run: &model.Run{JobID: "job", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"job": {}}}}, + JobContainer: &jobContainerMock{}, + } + + require.ErrorIs(t, rc.compositeExecutor(&model.Action{Runs: model.ActionRuns{Using: "composite", Steps: []model.Step{{ID: "nested", Uses: "org/action@v1"}}}}).pre(t.Context()), assert.AnError) + }) +} + func TestAppendUniqueMasks(t *testing.T) { tests := []struct { name string diff --git a/act/runner/action_test.go b/act/runner/action_test.go index 29e279a7..db4cc4b0 100644 --- a/act/runner/action_test.go +++ b/act/runner/action_test.go @@ -492,8 +492,6 @@ func TestDockerActionImageTag(t *testing.T) { ) } -// Only the entrypoint is stage specific: every stage of a docker action receives runs.args -// and runs.env, and the `entrypoint` input applies to the main stage alone. func TestExecAsDockerStageEntrypoint(t *testing.T) { orig := ContainerNewContainer defer func() { ContainerNewContainer = orig }() @@ -501,25 +499,62 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { for _, tc := range []struct { name string stage stepStage + with map[string]string + runs model.ActionRuns + env map[string]string + wantCmd []string wantEntrypoint []string }{ { - name: "main stage prefers the entrypoint input", - stage: stepStageMain, - wantEntrypoint: []string{"input.sh"}, + name: "main stage prefers manifest values", + stage: stepStageMain, + with: map[string]string{"args": "caller", "entrypoint": "input.sh"}, + runs: model.ActionRuns{ + Entrypoint: "main.sh", + Args: []string{"manifest"}, + Env: map[string]string{"ACTION_ONLY": "manifest"}, + }, + wantCmd: []string{"manifest"}, + wantEntrypoint: []string{"main.sh"}, }, { - name: "pre stage uses runs.pre-entrypoint", - stage: stepStagePre, + name: "main stage uses caller fallbacks", + stage: stepStageMain, + with: map[string]string{"args": "caller --flag", "entrypoint": "input.sh --verbose"}, + runs: model.ActionRuns{Env: map[string]string{"ACTION_ONLY": "manifest"}}, + wantCmd: []string{"caller", "--flag"}, + wantEntrypoint: []string{"input.sh", "--verbose"}, + }, + { + name: "explicit empty manifest args suppress caller args", + stage: stepStageMain, + with: map[string]string{"args": "caller"}, + runs: model.ActionRuns{Args: []string{}}, + wantCmd: []string{}, + }, + { + name: "pre stage keeps step environment", + stage: stepStagePre, + with: map[string]string{"entrypoint": "input.sh"}, + runs: model.ActionRuns{ + PreEntrypoint: "pre.sh --verbose", + Args: []string{"hello"}, + Env: map[string]string{"SHARED": "manifest"}, + }, + env: map[string]string{"SHARED": "step"}, + wantCmd: []string{"hello"}, wantEntrypoint: []string{"pre.sh", "--verbose"}, }, { - name: "post stage uses runs.post-entrypoint", + name: "post stage uses manifest entrypoint", stage: stepStagePost, + runs: model.ActionRuns{PostEntrypoint: "post.sh", Args: []string{"hello"}}, + wantCmd: []string{"hello"}, wantEntrypoint: []string{"post.sh"}, }, } { t.Run(tc.name, func(t *testing.T) { + tc.runs.Using, tc.runs.Image = "docker", "docker://node:14" cm := &containerMock{} var input *container.NewContainerInput ContainerNewContainer = func(in *container.NewContainerInput) container.ExecutionsEnvironment { @@ -528,22 +563,14 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { } step := &stepActionRemote{ - Step: &model.Step{ID: "1", Uses: "org/action@v1", With: map[string]string{"entrypoint": "input.sh"}}, + Step: &model.Step{ID: "1", Uses: "org/action@v1", With: tc.with}, RunContext: &RunContext{ Config: &Config{}, Run: &model.Run{JobID: "1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"1": {}}}}, JobContainer: cm, }, - action: &model.Action{Runs: model.ActionRuns{ - Using: "docker", - Image: "docker://node:14", - PreEntrypoint: "pre.sh --verbose", - Entrypoint: "main.sh", - PostEntrypoint: "post.sh", - Args: []string{"hello"}, - Env: map[string]string{"MY_VAR": "world"}, - }}, - env: map[string]string{}, + action: &model.Action{Runs: tc.runs}, + env: mergeMaps(tc.env), } cm.On("Pull", false).Return(func(context.Context) error { return nil }) @@ -554,9 +581,11 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { require.NoError(t, execAsDocker(context.Background(), step, "action", t.TempDir(), t.TempDir(), false, tc.stage)) require.NotNil(t, input) + assert.Equal(t, tc.wantCmd, input.Cmd) assert.Equal(t, tc.wantEntrypoint, input.Entrypoint) - assert.Equal(t, []string{"hello"}, input.Cmd) - assert.Contains(t, input.Env, "MY_VAR=world") + for key, value := range mergeMaps(tc.runs.Env, tc.env) { + assert.Contains(t, input.Env, key+"="+value) + } }) } } diff --git a/act/runner/cancellation_test.go b/act/runner/cancellation_test.go index 77920003..cc6c15bb 100644 --- a/act/runner/cancellation_test.go +++ b/act/runner/cancellation_test.go @@ -61,6 +61,8 @@ func TestCancelledJobStatusEnablesAlwaysAndCancelledSteps(t *testing.T) { enabled, err := interp.Evaluate("always()", exprparser.DefaultStatusCheckSuccess) require.NoError(t, err) assert.Equal(t, true, enabled, "`if: always()` step must run on a cancelled job") + setJobResult(context.Background(), rc, rc, true) + assert.Equal(t, "cancelled", rc.Run.Job().Result) } // TestMainStepsExecutorRunsAlwaysStepsAfterCancel verifies that newMainStepsExecutor does diff --git a/act/runner/command.go b/act/runner/command.go index 0c7d07a6..a0f79d74 100644 --- a/act/runner/command.go +++ b/act/runner/command.go @@ -18,11 +18,17 @@ var commandPatternGA *regexp.Regexp var commandPatternADO *regexp.Regexp func init() { - commandPatternGA = regexp.MustCompile("^::([^ ]+)( (.+))?::([^\r\n]*)[\r\n]+$") - commandPatternADO = regexp.MustCompile("^##\\[([^ ]+)( (.+))?]([^\r\n]*)[\r\n]+$") + commandPatternGA = regexp.MustCompile("^::([^ ]+?)( (.+?))?::([^\r\n]*)[\r\n]*$") + // excluding ']' ends the command info at the first bracket, as GitHub does + commandPatternADO = regexp.MustCompile("^##\\[([^ \\]]+)( ([^\\]]*))?]([^\r\n]*)[\r\n]*$") } func tryParseRawActionCommand(line string) (command string, kvPairs map[string]string, arg string, ok bool) { + command, kvPairs, arg, _, ok = tryParseActionCommand(line) + return command, kvPairs, arg, ok +} + +func tryParseActionCommand(line string) (command string, kvPairs map[string]string, arg string, legacy, ok bool) { if m := commandPatternGA.FindStringSubmatch(line); m != nil { command = m[1] kvPairs = parseKeyValuePairs(m[3], ",") @@ -32,19 +38,21 @@ func tryParseRawActionCommand(line string) (command string, kvPairs map[string]s command = m[1] kvPairs = parseKeyValuePairs(m[3], ";") arg = m[4] + legacy = true ok = true } - return command, kvPairs, arg, ok + return command, kvPairs, arg, legacy, ok } func (rc *RunContext) commandHandler(ctx context.Context) common.LineHandler { logger := common.Logger(ctx) resumeCommand := "" return func(line string) bool { - command, kvPairs, arg, ok := tryParseRawActionCommand(line) + command, kvPairs, arg, legacy, ok := tryParseActionCommand(line) if !ok { return true } + command = strings.ToLower(command) if resumeCommand != "" { // There should not be any emojis in the log output for Gitea. @@ -54,19 +62,24 @@ func (rc *RunContext) commandHandler(ctx context.Context) common.LineHandler { logger.Infof("%s", line) // Resumed here rather than from the switch, because the end token is arbitrary // and a token naming a real command would otherwise never resume. - if command == resumeCommand { + if strings.EqualFold(command, resumeCommand) { resumeCommand = "" } return true } - arg = UnescapeCommandData(arg) - kvPairs = unescapeKvPairs(kvPairs) + if legacy { + arg = UnescapeLegacyCommand(arg) + kvPairs = unescapeKvPairs(kvPairs, UnescapeLegacyCommand) + } else { + arg = UnescapeCommandData(arg) + kvPairs = unescapeKvPairs(kvPairs, unescapeCommandProperty) + } if (command == "set-env" || command == "add-path") && rc.refuseUnsecureCommand(ctx, command) { return true } switch command { case "set-env": - rc.setEnv(ctx, kvPairs, arg) + rc.setEnv(ctx, kvPairs, arg, true) case "set-output": rc.setOutput(ctx, kvPairs, arg) case "add-path": @@ -139,8 +152,16 @@ func (rc *RunContext) takeUnsecureCommandError() error { return err } -func (rc *RunContext) setEnv(ctx context.Context, kvPairs map[string]string, arg string) { +func (rc *RunContext) setEnv(ctx context.Context, kvPairs map[string]string, arg string, fromCommand bool) { name := kvPairs["name"] + if strings.EqualFold(name, "NODE_OPTIONS") { + message := "Can't store NODE_OPTIONS output parameter using '$GITHUB_ENV' command." + if fromCommand { + message = "Can't update NODE_OPTIONS environment variable using ::set-env:: command." + } + common.Logger(ctx).WithField(rawOutputField, true).Errorf("##[error]%s", EscapeCommandData(message)) + return + } common.Logger(ctx).Infof("::set-env:: %s=%s", name, arg) if rc.Env == nil { rc.Env = make(map[string]string) @@ -159,6 +180,10 @@ func (rc *RunContext) setEnv(ctx context.Context, kvPairs map[string]string, arg mergeIntoMap(rc.GlobalEnv, newenv) } +func (rc *RunContext) setEnvFile(ctx context.Context, kvPairs map[string]string, arg string) { + rc.setEnv(ctx, kvPairs, arg, false) +} + func (rc *RunContext) setOutput(ctx context.Context, kvPairs map[string]string, arg string) { logger := common.Logger(ctx) stepID := rc.CurrentStep @@ -189,7 +214,7 @@ func parseKeyValuePairs(kvPairs, separator string) map[string]string { rtn := make(map[string]string) kvPairList := strings.SplitSeq(kvPairs, separator) for kvPair := range kvPairList { - kv := strings.Split(kvPair, "=") + kv := strings.SplitN(kvPair, "=", 2) if len(kv) == 2 { rtn[kv[0]] = kv[1] } @@ -202,6 +227,7 @@ var ( commandDataEscaper = strings.NewReplacer("%", "%25", "\r", "%0D", "\n", "%0A") commandDataUnescaper = strings.NewReplacer("%25", "%", "%0D", "\r", "%0A", "\n") commandPropertyUnescaper = strings.NewReplacer("%25", "%", "%0D", "\r", "%0A", "\n", "%3A", ":", "%2C", ",") + legacyCommandUnescaper = strings.NewReplacer("%3B", ";", "%0D", "\r", "%0A", "\n", "%5D", "]", "%25", "%") ) // EscapeCommandData encodes the data part of a "::cmd::" or "##[cmd]" line the runner writes itself, @@ -218,9 +244,14 @@ func unescapeCommandProperty(arg string) string { return commandPropertyUnescaper.Replace(arg) } -func unescapeKvPairs(kvPairs map[string]string) map[string]string { +// UnescapeLegacyCommand decodes a "##[cmd]" line, which also spells ";" and "]" escaped. +func UnescapeLegacyCommand(arg string) string { + return legacyCommandUnescaper.Replace(arg) +} + +func unescapeKvPairs(kvPairs map[string]string, unescape func(string) string) map[string]string { for k, v := range kvPairs { - kvPairs[k] = unescapeCommandProperty(v) + kvPairs[k] = unescape(v) } return kvPairs } diff --git a/act/runner/command_test.go b/act/runner/command_test.go index 7f8031a0..afff6842 100644 --- a/act/runner/command_test.go +++ b/act/runner/command_test.go @@ -26,12 +26,21 @@ func unsecureRC() *RunContext { func TestSetEnv(t *testing.T) { a := assert.New(t) - ctx := context.Background() + logger, hook := test.NewNullLogger() + ctx := common.WithLogger(context.Background(), logger) rc := unsecureRC() handler := rc.commandHandler(ctx) handler("::set-env name=x::valz\n") a.Equal("valz", rc.Env["x"]) + handler("::SET-ENV name=NODE_OPTIONS::--require command.js\n") + rc.setEnvFile(ctx, map[string]string{"name": "node_options"}, "--require env.js") + a.NotContains(rc.Env, "NODE_OPTIONS") + a.NotContains(rc.Env, "node_options") + entries := hook.AllEntries() + require.Len(t, entries, 3) + a.Equal("##[error]Can't update NODE_OPTIONS environment variable using ::set-env:: command.", entries[1].Message) + a.Equal("##[error]Can't store NODE_OPTIONS output parameter using '$GITHUB_ENV' command.", entries[2].Message) } func TestStopCommandsKeepsSuppressedLinesInLog(t *testing.T) { @@ -85,6 +94,16 @@ func TestSetOutput(t *testing.T) { handler("::set-output name=x%3A%2C%0A%25%0D%3A::percent2%25%0Atest\n") a.Equal("percent2%\ntest", rc.StepResults["my-step"].Outputs["x:,\n%\r:"]) + handler("::set-output name=symbol::std::vector") + a.Equal("std::vector", rc.StepResults["my-step"].Outputs["symbol"]) + handler("::set-output name=a=b::value\n") + a.Equal("value", rc.StepResults["my-step"].Outputs["a=b"]) + handler("##[set-output name=legacy%3B%5D]value%3B%5D") + a.Equal("value;]", rc.StepResults["my-step"].Outputs["legacy;]"]) + handler("##[set-output name=bracket]value]tail") + a.Equal("value]tail", rc.StepResults["my-step"].Outputs["bracket"]) + handler("::set-output name=modern%3B%5D::value%3B%5D\n") + a.Equal("value%3B%5D", rc.StepResults["my-step"].Outputs["modern%3B%5D"]) } func TestAddpath(t *testing.T) { @@ -110,7 +129,7 @@ func TestStopCommands(t *testing.T) { handler("::set-env name=x::valz\n") a.Equal("valz", rc.Env["x"]) - handler("::stop-commands::my-end-token\n") + handler("::stop-commands::MY-END-TOKEN\n") handler("::set-env name=x::abcd\n") a.Equal("valz", rc.Env["x"]) handler("::my-end-token::\n") @@ -163,10 +182,10 @@ func TestAddmask(t *testing.T) { rc := new(RunContext) handler := rc.commandHandler(loggerCtx) - handler("::add-mask::my-secret-value\n") + handler("::ADD-MASK::my::secret") a.Equal("***", hook.LastEntry().Message) - a.NotEqual("*my-secret-value", hook.LastEntry().Message) + a.Equal([]string{"my::secret"}, rc.Masks) } // based on https://stackoverflow.com/a/10476304 diff --git a/act/runner/job_executor.go b/act/runner/job_executor.go index adf37868..6c908512 100644 --- a/act/runner/job_executor.go +++ b/act/runner/job_executor.go @@ -236,6 +236,13 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo // Ahead of the teardown below, while the job environment is still up. postExecutor = postExecutor.Finally(rc.runJobCompletedHook) + postExecutor = postExecutor.Finally(func(ctx context.Context) error { + // swallowed: a bad output fails this job, it must not abandon the rest of the plan + if err := info.interpolateOutputs()(ctx); err != nil { + reportStepError(ctx, rc, err) + } + return nil + }) postExecutor = postExecutor.Finally(func(ctx context.Context) error { jobError := common.JobError(ctx) @@ -267,7 +274,6 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo defer cancel() return postExecutor(postCtx) }). - Finally(info.interpolateOutputs()). Finally(info.closeContainer())) } @@ -365,7 +371,7 @@ func setJobResult(ctx context.Context, info jobInfo, rc *RunContext, success boo // concurrent succeeding one. job := rc.Run.Job() var continueOnError bool - if !success { + if !success && !rc.jobCancelled { // Use a fresh context so an expired job timeout cannot block expression evaluation. evalCtx := common.WithLogger(context.Background(), common.Logger(ctx)) continueOnError = evaluateJobContinueOnError(evalCtx, rc, job) @@ -378,7 +384,11 @@ func setJobResult(ctx context.Context, info jobInfo, rc *RunContext, success boo if len(info.matrix()) > 0 && job.Result != "" { result = job.Result } - if !success { + // cancelled is sticky, so a sibling combination finishing last cannot mask it + switch { + case rc.jobCancelled: + result = "cancelled" + case !success && result != "cancelled": result = "failure" job.SetContinueOnError(continueOnError) } @@ -392,9 +402,12 @@ func setJobResult(ctx context.Context, info jobInfo, rc *RunContext, success boo return } - jobResultMessage := "succeeded" - if jobResult != "success" { - jobResultMessage = "failed" + jobResultMessage := "failed" + switch jobResult { + case "success": + jobResultMessage = "succeeded" + case "cancelled": + jobResultMessage = "cancelled" } logger.WithField("jobResult", jobResult).Infof("Job %s", jobResultMessage) diff --git a/act/runner/job_executor_test.go b/act/runner/job_executor_test.go index fa6119b1..c8331875 100644 --- a/act/runner/job_executor_test.go +++ b/act/runner/job_executor_test.go @@ -299,6 +299,7 @@ func TestNewJobExecutor(t *testing.T) { executedSteps []string result string hasError bool + output string }{ { name: "zeroSteps", @@ -319,8 +320,8 @@ func TestNewJobExecutor(t *testing.T) { executedSteps: []string{ "startContainer", "step1", - "stopContainer", "interpolateOutputs", + "stopContainer", "closeContainer", }, result: "success", @@ -336,8 +337,8 @@ func TestNewJobExecutor(t *testing.T) { executedSteps: []string{ "startContainer", "step1", - "stopContainer", "interpolateOutputs", + "stopContainer", "closeContainer", }, result: "failure", @@ -354,8 +355,8 @@ func TestNewJobExecutor(t *testing.T) { "startContainer", "pre1", "step1", - "stopContainer", "interpolateOutputs", + "stopContainer", "closeContainer", }, result: "success", @@ -372,8 +373,8 @@ func TestNewJobExecutor(t *testing.T) { "startContainer", "step1", "post1", - "stopContainer", "interpolateOutputs", + "stopContainer", "closeContainer", }, result: "success", @@ -391,8 +392,8 @@ func TestNewJobExecutor(t *testing.T) { "pre1", "step1", "post1", - "stopContainer", "interpolateOutputs", + "stopContainer", "closeContainer", }, result: "success", @@ -418,13 +419,22 @@ func TestNewJobExecutor(t *testing.T) { "step3", "post3", "post2", - "stopContainer", "interpolateOutputs", + "stopContainer", "closeContainer", }, result: "success", hasError: false, }, + { + name: "jobOutputExpressionFailure", + steps: []*model.Step{{ID: "1"}}, + preSteps: []bool{false}, + postSteps: []bool{false}, + executedSteps: []string{"startContainer", "step1", "interpolateOutputs", "stopContainer", "closeContainer"}, + result: "failure", + output: "${{ 'test' != test }}", + }, } contains := func(needle string, haystack []string) bool { @@ -450,6 +460,10 @@ func TestNewJobExecutor(t *testing.T) { }, Config: &Config{}, } + if tt.output != "" { + rc.Run.Job().Outputs = map[string]string{"bad": tt.output} + rc.outputTemplate = map[string]string{"bad": tt.output} + } rc.ExprEval = rc.NewExpressionEvaluator(ctx) executorOrder := make([]string, 0) @@ -497,6 +511,9 @@ func TestNewJobExecutor(t *testing.T) { jim.On("interpolateOutputs").Return(func(ctx context.Context) error { executorOrder = append(executorOrder, "interpolateOutputs") + if tt.output != "" { + return rc.interpolateOutputs()(ctx) + } return nil }) @@ -518,6 +535,7 @@ func TestNewJobExecutor(t *testing.T) { executor := newJobExecutor(jim, sfm, rc) err := executor(ctx) assert.NoError(t, err) //nolint:testifylint // pre-existing issue from nektos/act + assert.Empty(t, rc.Run.Job().Outputs["bad"]) assert.Equal(t, tt.executedSteps, executorOrder) jim.AssertExpectations(t) diff --git a/act/runner/job_hooks.go b/act/runner/job_hooks.go index ace98324..2fa78c82 100644 --- a/act/runner/job_hooks.go +++ b/act/runner/job_hooks.go @@ -51,10 +51,11 @@ func (rc *RunContext) runJobHook(ctx context.Context, hookPath, name string) err rawLogger.Infof("shell: %s", shell) } - env := maps.Clone(rc.GetEnv()) + env := map[string]string{} if jobContainer := rc.Run.Job().Container(); jobContainer != nil { maps.Copy(env, jobContainer.Env) } + maps.Copy(env, rc.GetEnv()) rc.withGithubEnv(ctx, rc.getGithubContext(ctx), env) rc.ApplyExtraPath(ctx, &env) @@ -95,10 +96,11 @@ func (rc *RunContext) setupHookFileCommands(ctx context.Context, env map[string] } func (rc *RunContext) processHookFileCommands(ctx context.Context) error { - if err := processRunnerEnvFileCommand(ctx, hookEnvFileCommand, rc, rc.setEnv); err != nil { - return err + err := processRunnerEnvFileCommand(ctx, hookEnvFileCommand, rc, rc.setEnvFile) + if pathErr := rc.UpdateExtraPath(ctx, path.Join(rc.JobContainer.GetActPath(), hookPathFileCommand)); pathErr != nil && err == nil { + err = pathErr } - return rc.UpdateExtraPath(ctx, path.Join(rc.JobContainer.GetActPath(), hookPathFileCommand)) + return err } // hookCommand mirrors actions/runner, which deliberately does not apply the shell flags it diff --git a/act/runner/logger.go b/act/runner/logger.go index 2bc597c0..401c56ec 100644 --- a/act/runner/logger.go +++ b/act/runner/logger.go @@ -168,49 +168,77 @@ func withStepLogger(ctx context.Context, stepNumber int, stepID, stepName, stage type entryProcessor func(entry *logrus.Entry) *logrus.Entry -// secretValueEncoders are the shapes a secret takes on its way into a log: a base64 -// payload, a JSON string, or a URL component. An action that serializes a secret leaks -// it in one of these forms, which a mask of the verbatim value alone does not catch, so -// every form is masked as well. This mirrors the value encoders of GitHub's runner. var secretValueEncoders = []func(string) string{ func(v string) string { return base64.StdEncoding.EncodeToString([]byte(v)) }, base64ShiftEncoder(1), base64ShiftEncoder(2), + base64InteriorEncoder(0), + base64InteriorEncoder(1), + base64InteriorEncoder(2), + expressionStringEscape, jsonStringEscape, jsonStringEscapeNoHTML, - url.QueryEscape, + uriDataEscape, + url.QueryEscape, // the form-encoded twin of uriDataEscape, which spells a space "+" url.PathEscape, + xmlDataEscape, + trimDoubleQuotes, } -// minShiftedBase64Len is the shortest shifted base64 fragment worth masking. A shorter -// one carries too few bytes of the secret to identify it and would mask unrelated output. -const minShiftedBase64Len = 8 - -// base64ShiftEncoder returns the part of a secret's base64 form that survives when the -// secret does not start on a 3-byte boundary of the payload it is embedded in. base64 -// encodes three bytes at a time, so `Authorization: Basic base64("user:token")` contains -// the base64 of the token alone only when the prefix length happens to be a multiple of -// three; at the other two alignments the encoding of the whole value differs. Encoding -// the secret behind shift filler bytes reproduces those alignments, which is what the -// Base64StringEscapeShift1/2 encoders of GitHub's runner do. -// -// The leading group (filler mixed with the secret's first bytes) and the trailing group -// (padded here, but continuing into whatever follows the secret) are dropped, leaving the -// group-aligned middle that does appear verbatim in the log. +// base64ShiftEncoder reproduces the 3-byte alignments of `Basic base64("user:token")`, and +// its padded tail only matches a secret that ends the payload. func base64ShiftEncoder(shift int) func(string) string { + return func(v string) string { + value := []byte(v) + if len(value) > shift { + value = value[shift:] + } + return base64.StdEncoding.EncodeToString(value) + } +} + +const minInteriorBase64Len = 8 // below this a fragment matches unrelated output + +// base64InteriorEncoder keeps the aligned middle, so a secret with data after it still matches. +func base64InteriorEncoder(shift int) func(string) string { return func(v string) string { buf := make([]byte, shift+len(v)) copy(buf[shift:], v) encoded := base64.StdEncoding.EncodeToString(buf) - // Keep only the aligned middle, and only when enough of it is left to be a - // distinctive pattern rather than a fragment that matches unrelated output. - if len(encoded) < 8+minShiftedBase64Len { + if len(encoded) < 8+minInteriorBase64Len { return "" } return encoded[4 : len(encoded)-4] } } +func expressionStringEscape(v string) string { + return strings.ReplaceAll(v, "'", "''") +} + +func uriDataEscape(v string) string { + return strings.ReplaceAll(url.QueryEscape(v), "+", "%20") +} + +var xmlDataEscaper = strings.NewReplacer( + "&", "&", + "<", "<", + ">", ">", + `"`, """, + "'", "'", +) + +func xmlDataEscape(v string) string { + return xmlDataEscaper.Replace(v) +} + +func trimDoubleQuotes(v string) string { + if len(v) > 8 && strings.HasPrefix(v, `"`) && strings.HasSuffix(v, `"`) { + return v[1 : len(v)-1] + } + return "" +} + // jsonStringEscape returns v as it appears inside a JSON string, without the quotes, // which is what `toJSON(secrets)` or any action logging a JSON body produces. Go's encoder // escapes <, >, & (as act's own toJSON does); the non-HTML variant below covers the runtimes @@ -245,36 +273,34 @@ func AppendSecretMaskers(oldnew []string, secrets map[string]string) []string { return oldnew } +// AppendSecretMasker registers v and each of its lines, as GitHub does. func AppendSecretMasker(oldnew []string, v string) []string { - ret := oldnew - - for l := range strings.SplitSeq(v, "\n") { - tm := strings.TrimSpace(l) - // formatted JSON secrets could otherwise mask {,[,],} everywhere - if len(tm) > 1 { - ret = append(ret, tm, "***") - // command data reaches the log escaped, so "pass%word" also arrives as "pass%25word" - if strings.ContainsAny(tm, "%\r\n") { - ret = append(ret, EscapeCommandData(tm), "***") - } - } + ret := appendMaskedValue(oldnew, v) + for l := range strings.FieldsFuncSeq(v, func(r rune) bool { return r == '\r' || r == '\n' }) { + ret = appendMaskedValue(ret, strings.TrimSpace(l)) } + return ret +} - // The encoded forms are derived from the whole value: a multi-line secret is - // encoded as one string, not line by line. - trimmed := strings.TrimSpace(v) - if len(trimmed) <= 1 { +// appendMaskedValue registers one value and every shape it takes on its way into a log. +func appendMaskedValue(ret []string, v string) []string { + // formatted JSON secrets could otherwise mask {,[,],} everywhere + if len(strings.TrimSpace(v)) <= 1 || slices.Contains(ret, v) { return ret } + ret = append(ret, v, "***") + // command data reaches the log escaped, so "pass%word" also arrives as "pass%25word" + if strings.ContainsAny(v, "%\r\n") { + ret = append(ret, EscapeCommandData(v), "***") + } for _, encode := range secretValueEncoders { - encoded := encode(trimmed) + encoded := encode(v) // An encoding that leaves the value unchanged is already masked above. - if encoded == trimmed || len(encoded) <= 1 || slices.Contains(ret, encoded) { + if encoded == v || len(encoded) <= 1 || slices.Contains(ret, encoded) { continue } ret = append(ret, encoded, "***") } - return ret } diff --git a/act/runner/logger_test.go b/act/runner/logger_test.go index 14d05cff..a0bc5266 100644 --- a/act/runner/logger_test.go +++ b/act/runner/logger_test.go @@ -64,26 +64,27 @@ func TestValueMasker(t *testing.T) { // A secret that reaches the log through an encoding — a base64 payload, a JSON body, a // URL — must be masked as well: masking only the verbatim value leaks it. func TestValueMaskerEncodedSecrets(t *testing.T) { - secret := `p@ss w"rd/1` - masker := valueMasker(false, AppendSecretMaskers(nil, map[string]string{"TOKEN": secret})) - for _, tc := range []struct { - name string - line string + name, secret string + encoded []string }{ - {"verbatim", "the token is " + secret}, - {"base64", "Authorization: Basic " + base64.StdEncoding.EncodeToString([]byte(secret))}, - {"json", `{"token":"` + jsonStringEscape(secret) + `"}`}, - {"query escaped", "https://example.com/?token=" + url.QueryEscape(secret)}, - {"path escaped", "https://example.com/" + url.PathEscape(secret) + "/x"}, + {"common encodings", `p@ss w"rd/1`, []string{ + `p@ss w"rd/1`, base64.StdEncoding.EncodeToString([]byte(`p@ss w"rd/1`)), + jsonStringEscape(`p@ss w"rd/1`), url.PathEscape(`p@ss w"rd/1`), + }}, + {"XML expression and quotes", `"a'b&c"`, []string{ + `"a'b&c<d>"`, `"a''b&c"`, `a'b&c`, + }}, + {"URI spaces", "a b", []string{"a%20b", "a+b"}}, } { t.Run(tc.name, func(t *testing.T) { - entry := masker(&logrus.Entry{Context: t.Context(), Message: tc.line}) + masker := valueMasker(false, AppendSecretMaskers(nil, map[string]string{"TOKEN": tc.secret})) + entry := masker(&logrus.Entry{Context: t.Context(), Message: strings.Join(tc.encoded, " ")}) assert.Contains(t, entry.Message, "***") - assert.NotContains(t, entry.Message, secret) - assert.NotContains(t, entry.Message, base64.StdEncoding.EncodeToString([]byte(secret))) - assert.NotContains(t, entry.Message, url.QueryEscape(secret)) + for _, disallowed := range tc.encoded { + assert.NotContains(t, entry.Message, disallowed) + } }) } } @@ -124,15 +125,21 @@ func TestValueMaskerHidesExtraMasks(t *testing.T) { // ::add-mask:: values go through the same masker, so they get the same treatment. func TestValueMaskerEncodedMasks(t *testing.T) { - masks := []string{"s3cr3t value"} + masks := []string{"s3cr3t value", "first\rsecond", " s3cr3t "} masker := valueMasker(false, AppendSecretMaskers(nil, nil)) - entry := masker(&logrus.Entry{ - Context: WithMasks(t.Context(), &masks), - Message: "encoded: " + base64.StdEncoding.EncodeToString([]byte("s3cr3t value")), - }) - - assert.Equal(t, "encoded: ***", entry.Message) + for _, tc := range []struct { + line, want string + }{ + {"encoded: " + base64.StdEncoding.EncodeToString([]byte("s3cr3t value")), "encoded: ***"}, + {"first and second", "*** and ***"}, + {"encoded: " + base64.StdEncoding.EncodeToString([]byte(" s3cr3t ")), "encoded: ***"}, + {"encoded: " + base64.StdEncoding.EncodeToString([]byte("s3cr3t")), "encoded: ***"}, + {"encoded: " + base64.StdEncoding.EncodeToString([]byte("first")), "encoded: ***"}, + } { + entry := masker(&logrus.Entry{Context: WithMasks(t.Context(), &masks), Message: tc.line}) + assert.Equal(t, tc.want, entry.Message, tc.line) + } } // A token in a Basic auth header is base64'd together with the user name, so the token's @@ -140,7 +147,7 @@ func TestValueMaskerEncodedMasks(t *testing.T) { // alignments must be masked as well, or `Authorization: Basic base64("user:token")` leaks // the token to anyone who can decode the log. func TestValueMaskerBase64Alignments(t *testing.T) { - secret := "s3cr3t-token-value" + secret := "s3cr3t" masker := valueMasker(false, AppendSecretMaskers(nil, map[string]string{"TOKEN": secret})) // One prefix per alignment: len%3 of 0, 1 and 2. @@ -150,8 +157,6 @@ func TestValueMaskerBase64Alignments(t *testing.T) { entry := masker(&logrus.Entry{Context: t.Context(), Message: "Authorization: Basic " + encoded}) assert.Contains(t, entry.Message, "***") - // The aligned middle of the secret must be gone, so the payload can no longer be - // decoded back into the token. assert.NotEqual(t, "Authorization: Basic "+encoded, entry.Message) decodable := strings.TrimPrefix(entry.Message, "Authorization: Basic ") decoded, err := base64.StdEncoding.DecodeString(decodable) @@ -190,15 +195,14 @@ func TestAppendSecretMaskerSkipsUselessEncodings(t *testing.T) { // JSON, query and path escaping all leave it unchanged. pairs := AppendSecretMasker(nil, "plaintoken") assert.Equal(t, []string{ - "plaintoken", "***", - base64.StdEncoding.EncodeToString([]byte("plaintoken")), "***", - // The two shifted alignments, each without its leading and trailing group. - "YWludG9r", "***", - "bGFpbnRv", "***", + "plaintoken", "***", "cGxhaW50b2tlbg==", "***", "bGFpbnRva2Vu", "***", "YWludG9rZW4=", "***", + "aW50b2tl", "***", "YWludG9r", "***", "bGFpbnRv", "***", }, pairs) // Too short to mask. assert.Empty(t, AppendSecretMasker(nil, "x")) + assert.Empty(t, AppendSecretMasker(nil, " \t")) + assert.NotContains(t, AppendSecretMasker(nil, `"123456"`), "123456") } func TestJobLogFormatterDecodesCommandData(t *testing.T) { diff --git a/act/runner/run_context.go b/act/runner/run_context.go index c6b400ef..8c99902a 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -37,6 +37,8 @@ import ( "github.com/moby/moby/api/types/mount" "github.com/opencontainers/selinux/go-selinux" "golang.org/x/sync/errgroup" + "golang.org/x/text/encoding/unicode" + "golang.org/x/text/transform" ) // RunContext contains info about current job @@ -88,6 +90,7 @@ type RunContext struct { jobFailed bool // empty for a host-mode job, which starts no container jobContainerID string + hasBash *bool // memoized implicit-shell probe, only set on the top-level RunContext jobNetworkName string // stepEnv is a copy of the running step's environment, so that workflow commands parsed out // of the container's output can be judged against it. Written by runStepExecutor and read on @@ -506,7 +509,6 @@ func (rc *RunContext) startJobContainer() common.Executor { serviceContainerName := createContainerName(rc.jobContainerName(), serviceID) c := newContainer(&container.NewContainerInput{ Name: serviceContainerName, - WorkingDir: ext.ToContainerPath(rc.Config.Workdir), Image: serviceImage, Username: serviceUsername, Password: servicePassword, @@ -679,13 +681,18 @@ func (rc *RunContext) UpdateExtraPath(ctx context.Context, githubEnvPath string) if err != nil && err != io.EOF { return err } - s := bufio.NewScanner(reader) + decoded := transform.NewReader(reader, unicode.BOMOverride(unicode.UTF8.NewDecoder())) + s := bufio.NewScanner(decoded) + s.Buffer(make([]byte, 0, 64*1024), 16*1024*1024) for s.Scan() { line := s.Text() if len(line) > 0 { rc.addPath(ctx, line) } } + if err := s.Err(); err != nil { + return fmt.Errorf("reading path file: %w", err) + } return nil } @@ -917,11 +924,24 @@ func (rc *RunContext) interpolateOutputs() common.Executor { // pristine snapshot (outputTemplate) and write under the lock, so each combo overwrites // with its own resolved values (last wins, as on GitHub) instead of the first combo's // resolved values freezing the shared template against later combos. - defer lockJob(job)() + // Resolved up front so one failure publishes none of them, as GitHub does. + outputs := make(map[string]string, len(rc.outputTemplate)) + var err error for k, v := range rc.outputTemplate { - job.Outputs[k] = ee.Interpolate(ctx, v) + if outputs[k], err = ee.interpolate(ctx, v); err != nil { + err = fmt.Errorf("failed to evaluate job output %q: %w", k, err) + break + } } - return nil + defer lockJob(job)() + for k := range rc.outputTemplate { + if err != nil { + job.Outputs[k] = "" + continue + } + job.Outputs[k] = outputs[k] + } + return err } } @@ -1245,12 +1265,23 @@ func (rc *RunContext) getRunnerContext(ctx context.Context) map[string]any { } runnerContext["name"] = rc.Config.RunnerName runnerContext["environment"] = "self-hosted" + runnerContext["workspace"] = parentDir(rc.githubWorkspace()) if rc.Config.RunnerDebug() { runnerContext["debug"] = "1" } return runnerContext } +func (rc *RunContext) githubWorkspace() string { + if rc.JobContainer != nil { + return rc.JobContainer.ToContainerPath(rc.Config.Workdir) + } + if workspace := rc.Config.Env["GITHUB_WORKSPACE"]; workspace != "" { + return workspace + } + return rc.Config.Workdir +} + func (rc *RunContext) getGithubContext(ctx context.Context) *model.GithubContext { logger := common.Logger(ctx) ghc := &model.GithubContext{ @@ -1277,11 +1308,10 @@ func (rc *RunContext) getGithubContext(ctx context.Context) *model.GithubContext RefType: rc.Config.Env["GITHUB_REF_TYPE"], BaseRef: rc.Config.Env["GITHUB_BASE_REF"], HeadRef: rc.Config.Env["GITHUB_HEAD_REF"], - Workspace: rc.Config.Env["GITHUB_WORKSPACE"], + Workspace: rc.githubWorkspace(), } if rc.JobContainer != nil { ghc.EventPath = rc.JobContainer.GetActPath() + "/workflow/event.json" - ghc.Workspace = rc.JobContainer.ToContainerPath(rc.Config.Workdir) } if ghc.RunID == "" { diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 77e8e313..a10c9653 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -9,6 +9,7 @@ import ( "context" "errors" "fmt" + "io" "os" "runtime" "strings" @@ -326,6 +327,7 @@ jobs: require.Equal(t, []string{"data"}, redis.ValidVolumes) require.Equal(t, map[string]string{"data": "/data"}, redis.Mounts) require.Empty(t, redis.Binds) // the docker socket is the job container's alone + require.Empty(t, redis.WorkingDir) } // Only the workflow's options may be stripped later, so the two sources have to reach the @@ -1352,10 +1354,15 @@ func TestRunContextGetRunnerContext(t *testing.T) { t.Run("adds the runner values the container cannot know", func(t *testing.T) { rc := createRunsOnRunContext(t, "ubuntu-latest") rc.Config.RunnerName = "runner-1" + rc.Config.Workdir = "/workspace/owner/repo" runnerContext := rc.getRunnerContext(ctx) assert.Equal(t, "runner-1", runnerContext["name"]) assert.Equal(t, "self-hosted", runnerContext["environment"]) + assert.Equal(t, "/workspace/owner", runnerContext["workspace"]) + assert.Equal(t, "/workspace/owner/repo", rc.getGithubContext(ctx).Workspace) + rc.Config.Env = map[string]string{"GITHUB_WORKSPACE": "/configured/work"} + assert.Equal(t, "/configured/work", rc.getGithubContext(ctx).Workspace) assert.NotContains(t, runnerContext, "debug") }) @@ -1368,15 +1375,52 @@ func TestRunContextGetRunnerContext(t *testing.T) { t.Run("keeps the execution environment values", func(t *testing.T) { rc := createRunsOnRunContext(t, "ubuntu-latest") - rc.JobContainer = &container.HostEnvironment{TmpDir: "/tmp/act", ToolCache: "/tmp/tool_cache"} + rc.Config.Workdir = "/host/work/owner/repo" + rc.JobContainer = &container.HostEnvironment{Workdir: rc.Config.Workdir, Path: "/container/work/owner/repo", TmpDir: "/tmp/act", ToolCache: "/tmp/tool_cache"} runnerContext := rc.getRunnerContext(ctx) assert.Equal(t, "/tmp/act", runnerContext["temp"]) assert.Equal(t, "/tmp/tool_cache", runnerContext["tool_cache"]) + assert.Equal(t, "/container/work/owner", runnerContext["workspace"]) + assert.Equal(t, "/container/work/owner/repo", rc.getGithubContext(ctx).Workspace) assert.NotEmpty(t, runnerContext["os"]) }) } +func TestRunContextUpdateExtraPath(t *testing.T) { + longPath := strings.Repeat("x", 64*1024+1) + for _, testcase := range []struct { + name string + content []byte + want string + wantErr bool + }{ + {name: "UTF-8 BOM", content: []byte("\xef\xbb\xbf/utf8/tools\n"), want: "/utf8/tools"}, + {name: "UTF-16 BOM", content: []byte{0xff, 0xfe, 'C', 0, ':', 0, '\\', 0, 't', 0, 'o', 0, 'o', 0, 'l', 0, 's', 0, '\n', 0}, want: `C:\tools`}, + {name: "over 64 KiB", content: []byte(longPath + "\n"), want: longPath}, + {name: "over 16 MiB", content: []byte(strings.Repeat("x", 16*1024*1024+1)), wantErr: true}, + } { + t.Run(testcase.name, func(t *testing.T) { + logger := log.New() + logger.SetOutput(io.Discard) + ctx := common.WithLogger(t.Context(), logger) + jobContainer := &containerMock{} + jobContainer.On("GetContainerArchive", mock.Anything, "/github/path"). + Return(io.NopCloser(bytes.NewReader(tarArchive(t, tarEntry{name: "path", body: string(testcase.content)}))), nil).Once() + defer jobContainer.AssertExpectations(t) + rc := &RunContext{JobContainer: jobContainer} + + err := rc.UpdateExtraPath(ctx, "/github/path") + if testcase.wantErr { + require.ErrorContains(t, err, "reading path file") + return + } + require.NoError(t, err) + assert.Equal(t, []string{testcase.want}, rc.ExtraPath) + }) + } +} + func TestParentDir(t *testing.T) { assert.Empty(t, parentDir("")) assert.Empty(t, parentDir("repo")) diff --git a/act/runner/step.go b/act/runner/step.go index 3f8b514e..cdc9d56d 100644 --- a/act/runner/step.go +++ b/act/runner/step.go @@ -58,13 +58,10 @@ func (s stepStage) String() string { func processRunnerEnvFileCommand(ctx context.Context, fileName string, rc *RunContext, setter func(context.Context, map[string]string, string)) error { env := map[string]string{} err := rc.JobContainer.UpdateFromEnv(path.Join(rc.JobContainer.GetActPath(), fileName), &env)(ctx) - if err != nil { - return err - } for k, v := range env { setter(ctx, map[string]string{"name": k}, v) } - return nil + return err } func runStepExecutor(step step, stage stepStage, executor common.Executor) common.Executor { @@ -91,13 +88,14 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo if err != nil { stepResult.Conclusion = model.StepStatusFailure stepResult.Outcome = model.StepStatusFailure + logger.WithField("stepResult", stepResult.Conclusion).Infof("Failure - %s %s", stage, stepModel) return err } if !runStep { stepResult.Conclusion = model.StepStatusSkipped stepResult.Outcome = model.StepStatusSkipped - logger.WithField("stepResult", stepResult.Outcome).Debugf("Skipping step '%s' due to '%s'", stepModel, ifExpression) + logger.WithField("stepResult", stepResult.Conclusion).Debugf("Skipping step '%s' due to '%s'", stepModel, ifExpression) return nil } @@ -157,11 +155,11 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo if topRC.summaryFileInitialized == nil { topRC.summaryFileInitialized = map[int]bool{} } + _ = rc.JobContainer.Copy(actPath, files...)(ctx) if !topRC.summaryFileInitialized[stepSummaryIndex] { - files = append(files, &container.FileEntry{Name: summaryFileCommand, Mode: 0o666}) + _ = rc.JobContainer.Copy(actPath, &container.FileEntry{Name: summaryFileCommand, Mode: 0o666})(ctx) topRC.summaryFileInitialized[stepSummaryIndex] = true } - _ = rc.JobContainer.Copy(actPath, files...)(ctx) // The command handler needs the step's env to judge ACTIONS_ALLOW_UNSECURE_COMMANDS. // Cloned: the step executor keeps writing to its own env map after this point, on a @@ -179,15 +177,29 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo if err == nil { err = insecureErr } + if fileErr := processRunnerEnvFileCommand(ctx, envFileCommand, rc, rc.setEnvFile); fileErr != nil && err == nil { + err = fileErr + } + if fileErr := processRunnerEnvFileCommand(ctx, stateFileCommand, rc, rc.saveState); fileErr != nil && err == nil { + err = fileErr + } + if fileErr := processRunnerEnvFileCommand(ctx, outputFileCommand, rc, rc.setOutput); fileErr != nil && err == nil { + err = fileErr + } + if fileErr := rc.UpdateExtraPath(ctx, path.Join(actPath, pathFileCommand)); fileErr != nil && err == nil { + err = fileErr + } + _ = rc.JobContainer.Copy(actPath, files...)(ctx) if err == nil { - logger.WithField("stepResult", stepResult.Outcome).Infof("Success - %s %s", stage, stepString) + logger.WithField("stepResult", stepResult.Conclusion).Infof("Success - %s %s", stage, stepString) } else { stepResult.Outcome = model.StepStatusFailure continueOnError, parseErr := isContinueOnError(ctx, stepModel.RawContinueOnError, step, stage) if parseErr != nil { stepResult.Conclusion = model.StepStatusFailure + logger.WithField("stepResult", stepResult.Conclusion).Infof("Failure - %s %s", stage, stepString) return parseErr } @@ -202,28 +214,7 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo // Infof: Errorf entries are promoted to the user log by the reporter, // which would duplicate the ##[error] annotation emitted elsewhere. - logger.WithField("stepResult", stepResult.Outcome).Infof("Failure - %s %s", stage, stepString) - } - // Process Runner File Commands - orgerr := err - err = processRunnerEnvFileCommand(ctx, envFileCommand, rc, rc.setEnv) - if err != nil { - return err - } - err = processRunnerEnvFileCommand(ctx, stateFileCommand, rc, rc.saveState) - if err != nil { - return err - } - err = processRunnerEnvFileCommand(ctx, outputFileCommand, rc, rc.setOutput) - if err != nil { - return err - } - err = rc.UpdateExtraPath(ctx, path.Join(actPath, pathFileCommand)) - if err != nil { - return err - } - if orgerr != nil { - return orgerr + logger.WithField("stepResult", stepResult.Conclusion).Infof("Failure - %s %s", stage, stepString) } return err } @@ -232,7 +223,7 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo func evaluateStepTimeout(ctx context.Context, exprEval *expressionEvaluator, stepModel *model.Step) (context.Context, context.CancelFunc) { timeout := exprEval.Interpolate(ctx, stepModel.TimeoutMinutes) if timeout != "" { - if timeOutMinutes, err := strconv.ParseInt(timeout, 10, 64); err == nil { + if timeOutMinutes, err := strconv.ParseInt(timeout, 10, 64); err == nil && timeOutMinutes > 0 { return context.WithTimeout(ctx, time.Duration(timeOutMinutes)*time.Minute) } } @@ -269,7 +260,8 @@ func mergeEnv(ctx context.Context, step step) { c := job.Container() if c != nil { - mergeIntoMap(step, env, rc.GetEnv(), c.Env) + // container env is the image's baseline, which job env and $GITHUB_ENV override + mergeIntoMap(step, env, c.Env, rc.GetEnv()) } else { mergeIntoMap(step, env, rc.GetEnv()) } diff --git a/act/runner/step_action_remote.go b/act/runner/step_action_remote.go index f13fcf1d..d4ad2c37 100644 --- a/act/runner/step_action_remote.go +++ b/act/runner/step_action_remote.go @@ -245,7 +245,7 @@ func (sar *stepActionRemote) getCompositeRunContext(ctx context.Context) *RunCon // input for this action during the main stage, but the env // was already created during the pre stage) env := evaluateCompositeInputAndEnv(ctx, sar.RunContext, sar) - sar.compositeRunContext.setActionEnv(env) + sar.compositeRunContext.setCompositeActionEnv(env) sar.compositeRunContext.ExtraPath = sar.RunContext.ExtraPath } return sar.compositeRunContext diff --git a/act/runner/step_action_remote_test.go b/act/runner/step_action_remote_test.go index 7de65387..3af74a94 100644 --- a/act/runner/step_action_remote_test.go +++ b/act/runner/step_action_remote_test.go @@ -242,6 +242,23 @@ func TestStepActionRemote(t *testing.T) { cm.AssertExpectations(t) }) } + + t.Run("refreshes composite inputs without leaking environment", func(t *testing.T) { + step := &stepActionRemote{ + Step: &model.Step{Uses: "org/composite@v1", With: map[string]string{"SHARED": "first"}}, + RunContext: &RunContext{Config: &Config{ActionCacheDir: t.TempDir()}, Run: &model.Run{JobID: "job", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"job": {}}}}, JobContainer: &jobContainerMock{}}, + action: &model.Action{Inputs: map[string]model.Input{"shared": {}}}, + env: map[string]string{}, + remoteAction: newRemoteAction("org/composite@v1"), + } + + for _, value := range []string{"first", "second"} { + step.env["INPUT_SHARED"] = value + composite := step.getCompositeRunContext(t.Context()) + assert.Equal(t, map[string]any{"shared": value}, composite.actionInputs) + assert.NotContains(t, composite.Env, "INPUT_SHARED") + } + }) } func TestStepActionRemotePrepare(t *testing.T) { diff --git a/act/runner/step_run.go b/act/runner/step_run.go index a5ba14f2..7e4aba33 100644 --- a/act/runner/step_run.go +++ b/act/runner/step_run.go @@ -7,6 +7,7 @@ package runner import ( "context" "fmt" + "io" "maps" "runtime" "slices" @@ -22,6 +23,8 @@ import ( yaml "go.yaml.in/yaml/v4" ) +var builtinShells = []string{"bash", "sh", "pwsh", "powershell", "cmd", "python"} + type stepRun struct { Step *model.Step RunContext *RunContext @@ -251,7 +254,7 @@ func getScriptName(rc *RunContext, step *model.Step) string { // OCI runtime exec failed: exec failed: container_linux.go:380: starting container process caused: exec: "${{": executable file not found in $PATH: unknown func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string, err error) { logger := common.Logger(ctx) - sr.setupShell(ctx) + implicitShell := sr.setupShell(ctx) sr.setupWorkingDirectory(ctx) step := sr.Step @@ -259,7 +262,18 @@ func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string, script = sr.RunContext.NewStepExpressionEvaluator(ctx, sr).Interpolate(ctx, step.Run) sr.interpolatedScript = script + // GitHub matches the built-in names case-insensitively, so `shell: PWSH` is valid + if slices.Contains(builtinShells, strings.ToLower(step.Shell)) { + step.Shell = strings.ToLower(step.Shell) + } + scCmd := step.ShellCommand() + if implicitShell && (step.Shell == "bash" || step.Shell == "sh") { + scCmd = step.Shell + " -e {0}" + } + if !strings.Contains(scCmd, "{0}") { + return "", "", fmt.Errorf("invalid shell option %q: format must contain {0}", step.Shell) + } sr.shellCommand = scCmd name = getScriptName(sr.RunContext, step) @@ -268,7 +282,8 @@ func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string, // Reference: https://github.com/actions/runner/blob/8109c962f09d9acc473d92c595ff43afceddb347/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs#L19-L27 runPrepend := "" runAppend := "" - switch step.Shell { + shellCommand, _, _ := strings.Cut(step.Shell, " ") + switch shellCommand { case "bash", "sh": name += ".sh" case "pwsh", "powershell": @@ -292,13 +307,13 @@ func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string, rc := sr.getRunContext() scriptPath := fmt.Sprintf("%s/%s", rc.JobContainer.GetActPath(), name) - sr.cmdline = strings.Replace(scCmd, `{0}`, scriptPath, 1) + sr.cmdline = strings.ReplaceAll(scCmd, `{0}`, scriptPath) sr.cmd, err = shellquote.Split(sr.cmdline) return name, script, err } -func (sr *stepRun) setupShell(ctx context.Context) { +func (sr *stepRun) setupShell(ctx context.Context) bool { rc := sr.RunContext step := sr.Step @@ -306,31 +321,47 @@ func (sr *stepRun) setupShell(ctx context.Context) { step.Shell = rc.Run.Job().Defaults.Run.Shell } - step.Shell = rc.NewExpressionEvaluator(ctx).Interpolate(ctx, step.Shell) + step.Shell = rc.NewStepExpressionEvaluator(ctx, sr).Interpolate(ctx, step.Shell) if step.Shell == "" { step.Shell = rc.Run.Workflow.Defaults.Run.Shell } - if step.Shell == "" { + implicitShell := step.Shell == "" + if implicitShell { + shellWithFallback := []string{"bash", "sh"} + env := maps.Clone(sr.env) + rc.ApplyExtraPath(ctx, &env) if _, ok := rc.JobContainer.(*container.HostEnvironment); ok { - shellWithFallback := []string{"bash", "sh"} // Don't use bash on windows by default, if not using a docker container if runtime.GOOS == "windows" { shellWithFallback = []string{"pwsh", "powershell"} } step.Shell = shellWithFallback[0] - env := maps.Clone(sr.env) - sr.getRunContext().ApplyExtraPath(ctx, &env) _, err := lookpath.LookPath2(shellWithFallback[0], env) if err != nil { step.Shell = shellWithFallback[1] } - } else if containerImage := rc.containerImage(ctx); containerImage != "" { - // Currently only linux containers are supported, use sh by default like actions/runner - step.Shell = "sh" + } else { + step.Shell = shellWithFallback[0] + if !rc.containerHasBash(ctx, env) { + step.Shell = shellWithFallback[1] + } } } + return implicitShell +} + +// containerHasBash probes once per job, else every implicit-shell step pays for an exec. +func (rc *RunContext) containerHasBash(ctx context.Context, env map[string]string) bool { + top := rc.topLevelRunContext() + if top.hasBash == nil { + stdout, stderr := rc.JobContainer.ReplaceLogWriter(io.Discard, io.Discard) + found := rc.JobContainer.Exec([]string{"sh", "-c", "command -v bash >/dev/null 2>&1"}, env, "", "")(ctx) == nil + rc.JobContainer.ReplaceLogWriter(stdout, stderr) + top.hasBash = &found + } + return *top.hasBash } func (sr *stepRun) setupWorkingDirectory(ctx context.Context) { @@ -345,7 +376,7 @@ func (sr *stepRun) setupWorkingDirectory(ctx context.Context) { } // jobs can receive context values, so we interpolate - workingdirectory = rc.NewExpressionEvaluator(ctx).Interpolate(ctx, workingdirectory) + workingdirectory = rc.NewStepExpressionEvaluator(ctx, sr).Interpolate(ctx, workingdirectory) // but top level keys in workflow file like `defaults` or `env` can't if workingdirectory == "" { diff --git a/act/runner/step_run_test.go b/act/runner/step_run_test.go index ba6c12cd..f844881e 100644 --- a/act/runner/step_run_test.go +++ b/act/runner/step_run_test.go @@ -8,6 +8,9 @@ import ( "bytes" "context" "io" + "os" + "path/filepath" + "runtime" "testing" "gitea.com/gitea/runner/act/container" @@ -15,8 +18,13 @@ import ( "gitea.dev/actionslib/pkg/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) +type shellContainerMock struct{ *containerMock } + +func (*shellContainerMock) ReplaceLogWriter(_, _ io.Writer) (io.Writer, io.Writer) { return nil, nil } + func TestStepRun(t *testing.T) { cm := &containerMock{} fileEntry := &container.FileEntry{ @@ -73,3 +81,103 @@ func TestStepRun(t *testing.T) { cm.AssertExpectations(t) } + +func TestStepRunShellParity(t *testing.T) { + tests := []struct { + name, shell, workingDir string + env map[string]string + host bool + probeErr error + wantExt string + wantCmd []string + wantErr string + }{ + { + name: "implicit host bash", + host: true, + wantCmd: []string{"bash", "-e", "/var/run/act/workflow/1.sh"}, + }, + { + name: "implicit container bash", + wantCmd: []string{"bash", "-e", "/var/run/act/workflow/1.sh"}, + }, + { + name: "implicit container sh fallback", + probeErr: assert.AnError, + wantCmd: []string{"sh", "-e", "/var/run/act/workflow/1.sh"}, + }, + { + name: "custom pwsh template", + shell: "pwsh -NoProfile -File {0}", + wantExt: ".ps1", + wantCmd: []string{"pwsh", "-NoProfile", "-File", "/var/run/act/workflow/1.ps1"}, + }, + { + name: "missing placeholder", + shell: "bash -e", + wantErr: `invalid shell option "bash -e": format must contain {0}`, + }, + { + name: "all placeholders", + shell: "bash -c '. {0}; . {0}'", + wantCmd: []string{"bash", "-c", ". /var/run/act/workflow/1.sh; . /var/run/act/workflow/1.sh"}, + }, + { + name: "step env expressions", + shell: "${{ env.SHELL }}", + workingDir: "${{ env.DIR }}", + env: map[string]string{"SHELL": "python {0}", "DIR": "subdir"}, + wantExt: ".py", + wantCmd: []string{"python", "/var/run/act/workflow/1.py"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + cm := &containerMock{} + var jobContainer container.ExecutionsEnvironment = &shellContainerMock{cm} + if test.host { + if runtime.GOOS == "windows" { + t.Skip("Linux host shell selection") + } + test.env = map[string]string{"PATH": t.TempDir()} + require.NoError(t, os.WriteFile(filepath.Join(test.env["PATH"], "bash"), nil, 0o755)) + jobContainer = &container.HostEnvironment{ActPath: "/var/run/act"} + } else if test.shell == "" { + cm.On("Exec", []string{"sh", "-c", "command -v bash >/dev/null 2>&1"}, + mock.AnythingOfType("map[string]string"), "", "").Return(func(context.Context) error { + return test.probeErr + }) + } + + sr := &stepRun{ + RunContext: &RunContext{ + Config: &Config{}, + Run: &model.Run{JobID: "1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"1": {}}}}, + JobContainer: jobContainer, + }, + Step: &model.Step{ID: "1", Run: "echo hi", Shell: test.shell, WorkingDirectory: test.workingDir}, + env: test.env, + } + + name, script, err := sr.setupShellCommand(t.Context()) + if test.wantErr != "" { + require.EqualError(t, err, test.wantErr) + return + } + require.NoError(t, err) + if test.wantExt == "" { + test.wantExt = ".sh" + } + wantScript := "\necho hi\n" + if test.wantExt == ".ps1" { + wantScript = "$ErrorActionPreference = 'stop'\necho hi\nif ((Test-Path -LiteralPath variable:/LASTEXITCODE)) { exit $LASTEXITCODE }" + } + assert.Equal(t, "workflow/1"+test.wantExt, name) + assert.Equal(t, wantScript, script) + assert.Equal(t, test.wantCmd, sr.cmd) + assert.Equal(t, test.env["DIR"], sr.WorkingDirectory) + cm.AssertExpectations(t) + }) + } +} diff --git a/act/runner/step_test.go b/act/runner/step_test.go index 1fc27b2b..f9a0a617 100644 --- a/act/runner/step_test.go +++ b/act/runner/step_test.go @@ -7,12 +7,15 @@ package runner import ( "context" "errors" + "os" "testing" "gitea.com/gitea/runner/act/common" + "gitea.com/gitea/runner/act/container" "gitea.dev/actionslib/pkg/model" log "github.com/sirupsen/logrus" + logrustest "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -404,3 +407,92 @@ func TestRunStepExecutorDoesNotLeakRefusalToNextStep(t *testing.T) { errB := runStepExecutor(stepB, stepStageMain, func(context.Context) error { return nil })(ctx) require.NoError(t, errB) } + +func TestRunStepExecutorParity(t *testing.T) { + newStep := func(t *testing.T, stepModel *model.Step) *stepRun { + rc := createRunContext(t) + rc.JobContainer = &container.HostEnvironment{ActPath: t.TempDir()} + rc.ExprEval = rc.NewExpressionEvaluator(context.Background()) + return &stepRun{RunContext: rc, Step: stepModel, env: map[string]string{}} + } + badExpression := "${{ 'test' != test }}" + for _, test := range []struct { + name, wantError string + step *model.Step + executor common.Executor + }{ + {"condition error", "if-expression", &model.Step{ID: "condition", If: yaml.Node{Value: badExpression}}, noopExecutor}, + {"continue-on-error expression error", "continue-on-error expression", &model.Step{ID: "continue", RawContinueOnError: badExpression}, common.NewErrorExecutor(assert.AnError)}, + } { + t.Run(test.name, func(t *testing.T) { + step := newStep(t, test.step) + logger, hook := logrustest.NewNullLogger() + err := runStepExecutor(step, stepStageMain, test.executor)(common.WithLogger(context.Background(), logger)) + + require.ErrorContains(t, err, test.wantError) + assert.Equal(t, model.StepStatusFailure, step.RunContext.StepResults[test.step.ID].Conclusion) + assert.Equal(t, model.StepStatusFailure, hook.LastEntry().Data["stepResult"]) + }) + } + + t.Run("file command error honors continue-on-error", func(t *testing.T) { + step := newStep(t, &model.Step{ID: "commands", RawContinueOnError: "true"}) + logger, hook := logrustest.NewNullLogger() + err := runStepExecutor(step, stepStageMain, func(context.Context) error { + require.NoError(t, os.WriteFile(step.env["GITHUB_ENV"], []byte("GOOD=1\nmalformed\n"), 0o600)) + require.NoError(t, os.WriteFile(step.env["GITHUB_OUTPUT"], []byte("kept=value\n"), 0o600)) + require.NoError(t, os.WriteFile(step.env["GITHUB_STATE"], []byte("saved=value\n"), 0o600)) + return nil + })(common.WithLogger(context.Background(), logger)) + + require.NoError(t, err) + result := step.RunContext.StepResults[step.Step.ID] + assert.Equal(t, model.StepStatusFailure, result.Outcome) + assert.Equal(t, model.StepStatusSuccess, result.Conclusion) + assert.Equal(t, model.StepStatusSuccess, hook.LastEntry().Data["stepResult"]) + assert.Equal(t, "1", step.RunContext.Env["GOOD"]) + assert.Equal(t, "value", result.Outputs["kept"]) + assert.Equal(t, "value", step.RunContext.IntraActionState[step.Step.ID]["saved"]) + for _, name := range []string{"GITHUB_ENV", "GITHUB_OUTPUT", "GITHUB_STATE", "GITHUB_PATH"} { + contents, readErr := os.ReadFile(step.env[name]) + require.NoError(t, readErr) + assert.Empty(t, contents) + } + }) + + t.Run("composite child files do not leak", func(t *testing.T) { + outer := newStep(t, &model.Step{ID: "outer"}) + childRC := &RunContext{ + Config: outer.RunContext.Config, Run: outer.RunContext.Run, Env: map[string]string{}, StepResults: map[string]*model.StepResult{}, + JobContainer: outer.RunContext.JobContainer, Parent: outer.RunContext, + } + childRC.ExprEval = childRC.NewExpressionEvaluator(context.Background()) + child := &stepRun{RunContext: childRC, Step: &model.Step{ID: "child"}, env: map[string]string{}} + + err := runStepExecutor(outer, stepStageMain, func(ctx context.Context) error { + require.NoError(t, runStepExecutor(child, stepStageMain, func(context.Context) error { + require.NoError(t, os.WriteFile(child.env["GITHUB_OUTPUT"], []byte("declared=child\nundeclared=leak\n"), 0o600)) + require.NoError(t, os.WriteFile(child.env["GITHUB_STATE"], []byte("saved=child\n"), 0o600)) + return nil + })(ctx)) + outer.RunContext.setOutput(ctx, map[string]string{"name": "declared"}, "outer") + return nil + })(context.Background()) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"declared": "outer"}, outer.RunContext.StepResults[outer.Step.ID].Outputs) + assert.Equal(t, map[string]string{"declared": "child", "undeclared": "leak"}, childRC.StepResults[child.Step.ID].Outputs) + assert.Empty(t, outer.RunContext.IntraActionState) + assert.Equal(t, "child", childRC.IntraActionState[child.Step.ID]["saved"]) + }) + + t.Run("timeout must be positive", func(t *testing.T) { + exprEval := createRunContext(t).NewExpressionEvaluator(context.Background()) + for timeout, wantDeadline := range map[string]bool{"-1": false, "0": false, "1": true} { + ctx, cancel := evaluateStepTimeout(context.Background(), exprEval, &model.Step{TimeoutMinutes: timeout}) + _, hasDeadline := ctx.Deadline() + cancel() + assert.Equal(t, wantDeadline, hasDeadline, timeout) + } + }) +} diff --git a/internal/pkg/report/reporter.go b/internal/pkg/report/reporter.go index 5891c7eb..3c6e64e3 100644 --- a/internal/pkg/report/reporter.go +++ b/internal/pkg/report/reporter.go @@ -783,8 +783,9 @@ var cmdRegex = regexp.MustCompile(`^::([^ :]+)( [^:]*)?::(.*)$`) // handleCommand takes value still escaped, so that the web UI decodes it exactly once. Only // the branches that consume the payload here decode it. func (r *Reporter) handleCommand(originalContent, command, properties, value string) *string { + command = strings.ToLower(command) // GitHub matches command names case-insensitively if r.stopCommandEndToken != "" { - if command != r.stopCommandEndToken { + if !strings.EqualFold(command, r.stopCommandEndToken) { return &originalContent } // Resumed here rather than from the switch, because the end token is arbitrary and a @@ -871,13 +872,20 @@ func parseCommandProperties(properties string) map[string]string { return props } +func cutPrefixFold(s, prefix string) (string, bool) { + if len(s) < len(prefix) || !strings.EqualFold(s[:len(prefix)], prefix) { + return s, false + } + return s[len(prefix):], true +} + func (r *Reporter) parseLogRow(entry *log.Entry) *runnerv1.LogRow { content := strings.TrimRight(entry.Message, "\r\n") // cmdRegex only covers the ::cmd:: form, so the ##[add-mask] one would otherwise reach // the log carrying its own secret. Registered and dropped like its ::add-mask:: twin. - if arg, ok := strings.CutPrefix(content, "##[add-mask]"); ok { - r.addMask(runner.UnescapeCommandData(arg)) + if arg, ok := cutPrefixFold(content, "##[add-mask]"); ok { + r.addMask(runner.UnescapeLegacyCommand(arg)) return nil } diff --git a/internal/pkg/report/reporter_test.go b/internal/pkg/report/reporter_test.go index ebd29a34..6b4cb507 100644 --- a/internal/pkg/report/reporter_test.go +++ b/internal/pkg/report/reporter_test.go @@ -23,6 +23,7 @@ import ( "gitea.com/gitea/runner/internal/pkg/metrics" connect_go "connectrpc.com/connect" + "gitea.dev/actionslib/pkg/model" runnerv1 "gitea.dev/actionslib/runner/v1" log "github.com/sirupsen/logrus" logrustest "github.com/sirupsen/logrus/hooks/test" @@ -273,14 +274,14 @@ func TestReporter_Fire(t *testing.T) { "stepID": []string{"0", "0"}, "stepNumber": 0, "raw_output": true, - "stepResult": "failure", + "stepResult": model.StepStatusFailure, }})) assert.Equal(t, runnerv1.Result_RESULT_UNSPECIFIED, reporter.state.Steps[0].Result) require.NoError(t, reporter.Fire(&log.Entry{Message: "step result", Data: map[string]any{ "stage": "Main", "stepNumber": 0, "raw_output": true, - "stepResult": "success", + "stepResult": model.StepStatusSuccess, }})) assert.Equal(t, runnerv1.Result_RESULT_SUCCESS, reporter.state.Steps[0].Result)