refactor: GITHUB_ENV command / remove env.PATH (#1503)

* fix: GITHUB_ENV / PATH handling

* apply workaround

* add ctx to ApplyExtraPath

* fix: Do not leak step env in composite

See #1585 for a test

* add more tests

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

pkg/runner/action.go

@@ -154,7 +154,7 @@ func runActionImpl(step actionStep, actionDir string, remoteAction *remoteAction
 			containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Main)}
 			logger.Debugf("executing remote job container: %s", containerArgs)
 
-			rc.ApplyExtraPath(step.getEnv())
+			rc.ApplyExtraPath(ctx, step.getEnv())
 
 			return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
 		case model.ActionRunsUsingDocker:
@@ -491,7 +491,7 @@ func runPreStep(step actionStep) common.Executor {
 			containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Pre)}
 			logger.Debugf("executing remote job container: %s", containerArgs)
 
-			rc.ApplyExtraPath(step.getEnv())
+			rc.ApplyExtraPath(ctx, step.getEnv())
 
 			return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
 
@@ -580,7 +580,7 @@ func runPostStep(step actionStep) common.Executor {
 			containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Post)}
 			logger.Debugf("executing remote job container: %s", containerArgs)
 
-			rc.ApplyExtraPath(step.getEnv())
+			rc.ApplyExtraPath(ctx, step.getEnv())
 
 			return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
 

pkg/runner/action_composite.go

@@ -66,6 +66,7 @@ func newCompositeRunContext(ctx context.Context, parent *RunContext, step action
 		JobContainer: parent.JobContainer,
 		ActionPath:   actionPath,
 		Env:          env,
+		GlobalEnv:    parent.GlobalEnv,
 		Masks:        parent.Masks,
 		ExtraPath:    parent.ExtraPath,
 		Parent:       parent,
@@ -99,6 +100,14 @@ func execAsComposite(step actionStep) common.Executor {
 
 		rc.Masks = append(rc.Masks, compositeRC.Masks...)
 		rc.ExtraPath = compositeRC.ExtraPath
+		// compositeRC.Env is dirty, contains INPUT_ and merged step env, only rely on compositeRC.GlobalEnv
+		for k, v := range compositeRC.GlobalEnv {
+			rc.Env[k] = v
+			if rc.GlobalEnv == nil {
+				rc.GlobalEnv = map[string]string{}
+			}
+			rc.GlobalEnv[k] = v
+		}
 
 		return err
 	}

pkg/runner/command.go

@@ -82,11 +82,17 @@ func (rc *RunContext) commandHandler(ctx context.Context) common.LineHandler {
 }
 
 func (rc *RunContext) setEnv(ctx context.Context, kvPairs map[string]string, arg string) {
-	common.Logger(ctx).Infof("  \U00002699  ::set-env:: %s=%s", kvPairs["name"], arg)
+	name := kvPairs["name"]
+	common.Logger(ctx).Infof("  \U00002699  ::set-env:: %s=%s", name, arg)
 	if rc.Env == nil {
 		rc.Env = make(map[string]string)
 	}
-	rc.Env[kvPairs["name"]] = arg
+	rc.Env[name] = arg
+	// for composite action GITHUB_ENV and set-env passing
+	if rc.GlobalEnv == nil {
+		rc.GlobalEnv = map[string]string{}
+	}
+	rc.GlobalEnv[name] = arg
 }
 func (rc *RunContext) setOutput(ctx context.Context, kvPairs map[string]string, arg string) {
 	logger := common.Logger(ctx)

pkg/runner/run_context.go

@@ -35,6 +35,7 @@ type RunContext struct {
 	Run                 *model.Run
 	EventJSON           string
 	Env                 map[string]string
+	GlobalEnv           map[string]string // to pass env changes of GITHUB_ENV and set-env correctly, due to dirty Env field
 	ExtraPath           []string
 	CurrentStep         string
 	StepResults         map[string]*model.StepResult
@@ -275,8 +276,6 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			rc.stopJobContainer(),
 			rc.JobContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop),
 			rc.JobContainer.Start(false),
-			rc.JobContainer.UpdateFromImageEnv(&rc.Env),
-			rc.JobContainer.UpdateFromEnv("/etc/environment", &rc.Env),
 			rc.JobContainer.Copy(rc.JobContainer.GetActPath()+"/", &container.FileEntry{
 				Name: "workflow/event.json",
 				Mode: 0644,
@@ -296,11 +295,21 @@ func (rc *RunContext) execJobContainer(cmd []string, env map[string]string, user
 	}
 }
 
-func (rc *RunContext) ApplyExtraPath(env *map[string]string) {
+func (rc *RunContext) ApplyExtraPath(ctx context.Context, env *map[string]string) {
 	if rc.ExtraPath != nil && len(rc.ExtraPath) > 0 {
 		path := rc.JobContainer.GetPathVariableName()
 		if (*env)[path] == "" {
-			(*env)[path] = rc.JobContainer.DefaultPathVariable()
+			cenv := map[string]string{}
+			var cpath string
+			if err := rc.JobContainer.UpdateFromImageEnv(&cenv)(ctx); err == nil {
+				if p, ok := cenv[path]; ok {
+					cpath = p
+				}
+			}
+			if len(cpath) == 0 {
+				cpath = rc.JobContainer.DefaultPathVariable()
+			}
+			(*env)[path] = cpath
 		}
 		(*env)[path] = rc.JobContainer.JoinPathVariable(append(rc.ExtraPath, (*env)[path])...)
 	}
@@ -664,7 +673,6 @@ func nestedMapLookup(m map[string]interface{}, ks ...string) (rval interface{})
 
 func (rc *RunContext) withGithubEnv(ctx context.Context, github *model.GithubContext, env map[string]string) map[string]string {
 	env["CI"] = "true"
-	env["GITHUB_ENV"] = rc.JobContainer.GetActPath() + "/workflow/envs.txt"
 	env["GITHUB_WORKFLOW"] = github.Workflow
 	env["GITHUB_RUN_ID"] = github.RunID
 	env["GITHUB_RUN_NUMBER"] = github.RunNumber

pkg/runner/runner_test.go

@@ -168,7 +168,7 @@ func TestRunEvent(t *testing.T) {
 		{workdir, "container-hostname", "push", "", platforms, secrets},
 		{workdir, "remote-action-docker", "push", "", platforms, secrets},
 		{workdir, "remote-action-js", "push", "", platforms, secrets},
-		{workdir, "remote-action-js", "push", "", map[string]string{"ubuntu-latest": "catthehacker/ubuntu:runner-latest"}, secrets}, // Test if this works with non root container
+		{workdir, "remote-action-js-node-user", "push", "", platforms, secrets}, // Test if this works with non root container
 		{workdir, "matrix", "push", "", platforms, secrets},
 		{workdir, "matrix-include-exclude", "push", "", platforms, secrets},
 		{workdir, "matrix-exitcode", "push", "Job 'test' failed", platforms, secrets},
@@ -193,6 +193,8 @@ func TestRunEvent(t *testing.T) {
 		{workdir, "actions-environment-and-context-tests", "push", "", platforms, secrets},
 		{workdir, "uses-action-with-pre-and-post-step", "push", "", platforms, secrets},
 		{workdir, "evalenv", "push", "", platforms, secrets},
+		{workdir, "docker-action-custom-path", "push", "", platforms, secrets},
+		{workdir, "GITHUB_ENV-use-in-env-ctx", "push", "", platforms, secrets},
 		{workdir, "ensure-post-steps", "push", "Job 'second-post-step-should-fail' failed", platforms, secrets},
 		{workdir, "workflow_dispatch", "workflow_dispatch", "", platforms, secrets},
 		{workdir, "workflow_dispatch_no_inputs_mapping", "workflow_dispatch", "", platforms, secrets},
@@ -204,6 +206,8 @@ func TestRunEvent(t *testing.T) {
 		{"../model/testdata", "container-volumes", "push", "", platforms, secrets},
 		{workdir, "path-handling", "push", "", platforms, secrets},
 		{workdir, "do-not-leak-step-env-in-composite", "push", "", platforms, secrets},
+		{workdir, "set-env-step-env-override", "push", "", platforms, secrets},
+		{workdir, "set-env-new-env-file-per-step", "push", "", platforms, secrets},
 	}
 
 	for _, table := range tables {
@@ -304,6 +308,8 @@ func TestRunEventHostEnvironment(t *testing.T) {
 			{workdir, "nix-prepend-path", "push", "", platforms, secrets},
 			{workdir, "inputs-via-env-context", "push", "", platforms, secrets},
 			{workdir, "do-not-leak-step-env-in-composite", "push", "", platforms, secrets},
+			{workdir, "set-env-step-env-override", "push", "", platforms, secrets},
+			{workdir, "set-env-new-env-file-per-step", "push", "", platforms, secrets},
 		}...)
 	}
 

pkg/runner/step.go

@@ -44,6 +44,18 @@ func (s stepStage) String() string {
 	return "Unknown"
 }
 
+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
+}
+
 func runStepExecutor(step step, stage stepStage, executor common.Executor) common.Executor {
 	return func(ctx context.Context) error {
 		logger := common.Logger(ctx)
@@ -92,9 +104,11 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo
 		outputFileCommand := path.Join("workflow", "outputcmd.txt")
 		stateFileCommand := path.Join("workflow", "statecmd.txt")
 		pathFileCommand := path.Join("workflow", "pathcmd.txt")
+		envFileCommand := path.Join("workflow", "envs.txt")
 		(*step.getEnv())["GITHUB_OUTPUT"] = path.Join(actPath, outputFileCommand)
 		(*step.getEnv())["GITHUB_STATE"] = path.Join(actPath, stateFileCommand)
 		(*step.getEnv())["GITHUB_PATH"] = path.Join(actPath, pathFileCommand)
+		(*step.getEnv())["GITHUB_ENV"] = path.Join(actPath, envFileCommand)
 		_ = rc.JobContainer.Copy(actPath, &container.FileEntry{
 			Name: outputFileCommand,
 			Mode: 0666,
@@ -104,6 +118,9 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo
 		}, &container.FileEntry{
 			Name: pathFileCommand,
 			Mode: 0666,
+		}, &container.FileEntry{
+			Name: envFileCommand,
+			Mode: 0666,
 		})(ctx)
 
 		err = executor(ctx)
@@ -131,21 +148,17 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo
 		}
 		// Process Runner File Commands
 		orgerr := err
-		state := map[string]string{}
-		err = rc.JobContainer.UpdateFromEnv(path.Join(actPath, stateFileCommand), &state)(ctx)
+		err = processRunnerEnvFileCommand(ctx, envFileCommand, rc, rc.setEnv)
 		if err != nil {
 			return err
 		}
-		for k, v := range state {
-			rc.saveState(ctx, map[string]string{"name": k}, v)
-		}
-		output := map[string]string{}
-		err = rc.JobContainer.UpdateFromEnv(path.Join(actPath, outputFileCommand), &output)(ctx)
+		err = processRunnerEnvFileCommand(ctx, stateFileCommand, rc, rc.saveState)
 		if err != nil {
 			return err
 		}
-		for k, v := range output {
-			rc.setOutput(ctx, map[string]string{"name": k}, v)
+		err = processRunnerEnvFileCommand(ctx, outputFileCommand, rc, rc.setOutput)
+		if err != nil {
+			return err
 		}
 		err = rc.UpdateExtraPath(ctx, path.Join(actPath, pathFileCommand))
 		if err != nil {
@@ -162,14 +175,6 @@ func setupEnv(ctx context.Context, step step) error {
 	rc := step.getRunContext()
 
 	mergeEnv(ctx, step)
-	err := rc.JobContainer.UpdateFromImageEnv(step.getEnv())(ctx)
-	if err != nil {
-		return err
-	}
-	err = rc.JobContainer.UpdateFromEnv((*step.getEnv())["GITHUB_ENV"], step.getEnv())(ctx)
-	if err != nil {
-		return err
-	}
 	// merge step env last, since it should not be overwritten
 	mergeIntoMap(step.getEnv(), step.getStepModel().GetEnv())
 

pkg/runner/step_action_local_test.go

@@ -69,18 +69,14 @@ func TestStepActionLocalTest(t *testing.T) {
 	salm.On("readAction", sal.Step, filepath.Clean("/tmp/path/to/action"), "", mock.Anything, mock.Anything).
 		Return(&model.Action{}, nil)
 
-	cm.On("UpdateFromImageEnv", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
+	cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error {
 		return nil
 	})
 
 	cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 		return nil
 	})
 
-	cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error {
-		return nil
-	})
-
 	cm.On("UpdateFromEnv", "/var/run/act/workflow/statecmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 		return nil
 	})
@@ -197,7 +193,7 @@ func TestStepActionLocalPost(t *testing.T) {
 				env  bool
 				exec bool
 			}{
-				env:  true,
+				env:  false,
 				exec: false,
 			},
 		},
@@ -260,10 +256,6 @@ func TestStepActionLocalPost(t *testing.T) {
 			}
 			sal.RunContext.ExprEval = sal.RunContext.NewExpressionEvaluator(ctx)
 
-			if tt.mocks.env {
-				cm.On("UpdateFromImageEnv", &sal.env).Return(func(ctx context.Context) error { return nil })
-				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", &sal.env).Return(func(ctx context.Context) error { return nil })
-			}
 			if tt.mocks.exec {
 				suffixMatcher := func(suffix string) interface{} {
 					return mock.MatchedBy(func(array []string) bool {
@@ -276,6 +268,10 @@ func TestStepActionLocalPost(t *testing.T) {
 					return nil
 				})
 
+				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
+					return nil
+				})
+
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/statecmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 					return nil
 				})

pkg/runner/step_action_remote_test.go

@@ -165,10 +165,6 @@ func TestStepActionRemote(t *testing.T) {
 				})
 			}
 
-			if tt.mocks.env {
-				cm.On("UpdateFromImageEnv", &sar.env).Return(func(ctx context.Context) error { return nil })
-				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", &sar.env).Return(func(ctx context.Context) error { return nil })
-			}
 			if tt.mocks.read {
 				sarm.On("readAction", sar.Step, suffixMatcher("act/remote-action@v1"), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil)
 			}
@@ -179,6 +175,10 @@ func TestStepActionRemote(t *testing.T) {
 					return nil
 				})
 
+				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
+					return nil
+				})
+
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/statecmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 					return nil
 				})
@@ -579,17 +579,17 @@ func TestStepActionRemotePost(t *testing.T) {
 			}
 			sar.RunContext.ExprEval = sar.RunContext.NewExpressionEvaluator(ctx)
 
-			if tt.mocks.env {
-				cm.On("UpdateFromImageEnv", &sar.env).Return(func(ctx context.Context) error { return nil })
-				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", &sar.env).Return(func(ctx context.Context) error { return nil })
-			}
 			if tt.mocks.exec {
 				cm.On("Exec", []string{"node", "/var/run/act/actions/remote-action@v1/post.js"}, sar.env, "", "").Return(func(ctx context.Context) error { return tt.err })
 
 				cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error {
 					return nil
 				})
 
+				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
+					return nil
+				})
+
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/statecmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 					return nil
 				})

pkg/runner/step_docker_test.go

@@ -57,14 +57,6 @@ func TestStepDockerMain(t *testing.T) {
 	}
 	sd.RunContext.ExprEval = sd.RunContext.NewExpressionEvaluator(ctx)
 
-	cm.On("UpdateFromImageEnv", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
-		return nil
-	})
-
-	cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
-		return nil
-	})
-
 	cm.On("Pull", false).Return(func(ctx context.Context) error {
 		return nil
 	})
@@ -89,6 +81,10 @@ func TestStepDockerMain(t *testing.T) {
 		return nil
 	})
 
+	cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
+		return nil
+	})
+
 	cm.On("UpdateFromEnv", "/var/run/act/workflow/statecmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 		return nil
 	})

pkg/runner/step_run.go

@@ -30,7 +30,7 @@ func (sr *stepRun) main() common.Executor {
 	return runStepExecutor(sr, stepStageMain, common.NewPipelineExecutor(
 		sr.setupShellCommandExecutor(),
 		func(ctx context.Context) error {
-			sr.getRunContext().ApplyExtraPath(&sr.env)
+			sr.getRunContext().ApplyExtraPath(ctx, &sr.env)
 			return sr.getRunContext().JobContainer.Exec(sr.cmd, sr.env, "", sr.Step.WorkingDirectory)(ctx)
 		},
 	))

pkg/runner/step_run_test.go

@@ -55,18 +55,14 @@ func TestStepRun(t *testing.T) {
 		return nil
 	})
 
-	cm.On("UpdateFromImageEnv", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
+	cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error {
 		return nil
 	})
 
 	cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 		return nil
 	})
 
-	cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error {
-		return nil
-	})
-
 	cm.On("UpdateFromEnv", "/var/run/act/workflow/statecmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 		return nil
 	})