fix(safe_jobs): hoist expression env vars into step env: block to prevent run-script guardrail failure (#32532)

Author: Copilot

.changeset/patch-hoist-safe-job-env-expressions.md

@@ -248,34 +248,19 @@ func (c *Compiler) buildSafeJobs(data *WorkflowData, threatDetectionEnabled bool
 		steps = append(steps, downloadSteps...)
 
 		// the download artifacts always creates a folder, then unpacks in that folder
-		agentOutputArtifactFilename := "${RUNNER_TEMP}/gh-aw/safe-jobs/" + constants.AgentOutputFilename
-
-		// Add environment variables step with GH_AW_AGENT_OUTPUT and job-specific env vars
-		steps = append(steps, "      - name: Configure Safe Outputs Job Environment Variables\n")
-		steps = append(steps, "        id: setup-safe-job-env\n")
-		steps = append(steps, "        run: |\n")
-		steps = append(steps, "          find \"${RUNNER_TEMP}/gh-aw/safe-jobs/\" -type f -print\n")
-		// Configure GH_AW_AGENT_OUTPUT to point to downloaded artifact file
-		steps = append(steps, fmt.Sprintf("          echo \"GH_AW_AGENT_OUTPUT=%s\" >> \"$GITHUB_OUTPUT\"\n", agentOutputArtifactFilename))
-
-		// Add job-specific environment variables
-		if jobConfig.Env != nil {
-			for key, value := range jobConfig.Env {
-				steps = append(steps, fmt.Sprintf("          echo \"%s=%s\" >> \"$GITHUB_OUTPUT\"\n", key, value))
-			}
-		}
 
-		// Add custom steps from the job configuration, injecting env vars from the
-		// setup-safe-job-env step outputs so user steps can access them.
+		// Add custom steps from the job configuration, injecting env vars directly so
+		// user steps can access GH_AW_AGENT_OUTPUT and all job-specific env vars.
 		if len(jobConfig.Steps) > 0 {
-			// Build the env vars that were set in the setup-safe-job-env step so we can inject them.
+			// GH_AW_AGENT_OUTPUT uses the runner.temp Actions expression so the path is
+			// resolved by the runner without requiring a $GITHUB_OUTPUT write.
 			setupEnvVars := map[string]string{
-				"GH_AW_AGENT_OUTPUT": "${{ steps.setup-safe-job-env.outputs.GH_AW_AGENT_OUTPUT }}",
+				"GH_AW_AGENT_OUTPUT": fmt.Sprintf("${{ runner.temp }}/gh-aw/safe-jobs/%s", constants.AgentOutputFilename),
 			}
-			if jobConfig.Env != nil {
-				for key := range jobConfig.Env {
-					setupEnvVars[key] = fmt.Sprintf("${{ steps.setup-safe-job-env.outputs.%s }}", key)
-				}
+			// All job-specific env vars (literal or expression-based) are injected with
+			// their original values. Nothing goes through $GITHUB_OUTPUT.
+			for key, value := range jobConfig.Env {
+				setupEnvVars[key] = value
 			}
 			for _, step := range jobConfig.Steps {
 				if stepMap, ok := step.(map[string]any); ok {

.github/workflows/daily-choice-test.lock.yml

@@ -233,12 +233,8 @@ func TestBuildSafeJobs(t *testing.T) {
 		t.Error("Expected main job output to be available as environment variable")
 	}
 
-	if !strings.Contains(stepsContent, "Configure Safe Outputs Job Environment Variables") {
-		t.Error("Expected safe jobs env setup step to use Safe Outputs terminology")
-	}
-
-	if strings.Contains(stepsContent, "Configure Safe Job Environment Variables") {
-		t.Error("Expected legacy Safe Job terminology to be absent from safe jobs env setup step")
+	if strings.Contains(stepsContent, "Configure Safe Outputs Job Environment Variables") {
+		t.Error("Configure Safe Outputs Job Environment Variables step should have been removed")
 	}
 
 	if strings.Contains(stepsContent, "GLOBAL_VAR=global_value") {
@@ -596,6 +592,67 @@ func TestMergeSafeJobsFromIncludedConfigs(t *testing.T) {
 	}
 }
 
+// TestBuildSafeJobsEnvExpressionHoisting verifies that no env vars — whether expression-based
+// or literal — are ever written to $GITHUB_OUTPUT, and that all values are injected directly
+// into each downstream step's env: block.
+func TestBuildSafeJobsEnvExpressionHoisting(t *testing.T) {
+	c := NewCompiler()
+
+	workflowData := &WorkflowData{
+		Name: "test-workflow",
+		SafeOutputs: &SafeOutputsConfig{
+			Jobs: map[string]*SafeJobConfig{
+				"publish": {
+					RunsOn: "ubuntu-latest",
+					Env: map[string]string{
+						"GH_TOKEN":   "${{ github.token }}",
+						"STATIC_VAR": "literal-value",
+					},
+					Steps: []any{
+						map[string]any{
+							"name": "Publish",
+							"run":  "echo 'Publishing'",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	_, err := c.buildSafeJobs(workflowData, false)
+	if err != nil {
+		t.Fatalf("Unexpected error building safe jobs: %v", err)
+	}
+
+	jobs := c.jobManager.GetAllJobs()
+	if len(jobs) != 1 {
+		t.Fatalf("Expected 1 job to be created, got %d", len(jobs))
+	}
+
+	var job *Job
+	for _, j := range jobs {
+		job = j
+		break
+	}
+
+	stepsContent := strings.Join(job.Steps, "")
+
+	// Neither expression-based nor literal env vars must ever be written to $GITHUB_OUTPUT.
+	if strings.Contains(stepsContent, `GITHUB_OUTPUT`) {
+		t.Error("No env var must ever be written to $GITHUB_OUTPUT in safe-jobs (secrets must not be stored in outputs)")
+	}
+
+	// Expression-based values must be injected directly into downstream step env: blocks.
+	if !strings.Contains(stepsContent, "GH_TOKEN: ${{ github.token }}") {
+		t.Error("Expected GH_TOKEN expression to be injected directly into a downstream step env: block")
+	}
+
+	// Literal values must also be injected directly into downstream step env: blocks.
+	if !strings.Contains(stepsContent, "STATIC_VAR: literal-value") {
+		t.Error("Expected literal env var STATIC_VAR to be injected directly into a downstream step env: block")
+	}
+}
+
 // TestSafeJobsInputTypes tests that safe-jobs inputs support all input types
 // and share the same InputDefinition type with workflow_dispatch inputs
 func TestSafeJobsInputTypes(t *testing.T) {

.github/workflows/mcp-inspector.lock.yml

@@ -368,8 +368,68 @@ Test workflow content
 	}
 }
 
-// TestIsThreatDetectionExplicitlyDisabledInConfigs verifies the helper function
-// that checks whether any imported safe-outputs config explicitly disables detection.
+// TestSafeJobsExpressionEnvNeverWrittenToOutput verifies that job-level env vars containing
+// GitHub Actions expressions are never written to $GITHUB_OUTPUT in the compiled lock file.
+// This is a security guardrail: tokens like ${{ github.token }} must never be stored in outputs.
+func TestSafeJobsExpressionEnvNeverWrittenToOutput(t *testing.T) {
+	c := NewCompiler()
+
+	markdown := `---
+on: issues
+safe-outputs:
+  jobs:
+    publish:
+      env:
+        GH_TOKEN: ${{ github.token }}
+        STATIC: literal-value
+      steps:
+        - name: Do work
+          run: echo "working"
+---
+
+# Test Workflow
+Test workflow content
+`
+
+	tmpDir := testutil.TempDir(t, "test-*")
+	testFile := tmpDir + "/test-safe-jobs-secret.md"
+	if err := os.WriteFile(testFile, []byte(markdown), 0o644); err != nil {
+		t.Fatalf("Failed to write test file: %v", err)
+	}
+
+	if err := c.CompileWorkflow(testFile); err != nil {
+		t.Fatalf("Failed to compile workflow: %v", err)
+	}
+
+	lockFile := tmpDir + "/test-safe-jobs-secret.lock.yml"
+	workflowBytes, err := os.ReadFile(lockFile)
+	if err != nil {
+		t.Fatalf("Failed to read lock file: %v", err)
+	}
+	workflowStr := string(workflowBytes)
+
+	// No env var at all may be written to $GITHUB_OUTPUT by safe-jobs.
+	for _, line := range strings.Split(workflowStr, "\n") {
+		if strings.Contains(line, "GH_TOKEN") && strings.Contains(line, "GITHUB_OUTPUT") {
+			t.Errorf("GH_TOKEN must never be written to GITHUB_OUTPUT (secret leak): %s", strings.TrimSpace(line))
+		}
+		if strings.Contains(line, "STATIC") && strings.Contains(line, "GITHUB_OUTPUT") {
+			t.Errorf("STATIC env var must not be written to GITHUB_OUTPUT: %s", strings.TrimSpace(line))
+		}
+	}
+
+	// The token must be injected into the step env: block, not through step outputs.
+	if !strings.Contains(workflowStr, "GH_TOKEN: ${{ github.token }}") {
+		t.Error("Expected GH_TOKEN expression to be injected into step env: block in compiled output")
+	}
+
+	// Literal value must also be injected directly into the step env: block.
+	if !strings.Contains(workflowStr, "STATIC: literal-value") {
+		t.Error("Expected literal env var to be injected directly into step env: block in compiled output")
+	}
+}
+
+
 func TestIsThreatDetectionExplicitlyDisabledInConfigs(t *testing.T) {
 	tests := []struct {
 		name     string