fix: prevent premature token revocation in reusable workflows (#1081)

## Problem When using reusable workflows, the Forgejo runner prematurely revokes `GITHUB_TOKEN` after the first step completes, causing subsequent steps to fail with authentication errors. ### Reproduction When the reusable workflow contains multiple steps that require authentication: 1. First step (e.g., checkout) completes successfully 2. Reporter receives completion banner from child workflow 3. Token is revoked prematurely 4. Second step fails with authentication error   - bug fixes - [PR](https://code.forgejo.org/forgejo/runner/pulls/1081): fix: prevent premature token revocation in reusable workflows  Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/1081 Reviewed-by: Mathieu Fenniak <mfenniak@noreply.code.forgejo.org> Co-authored-by: Roman K. <devops@syncstack.net> Co-committed-by: Roman K. <devops@syncstack.net>
2025-10-14 01:45:54 +00:00 · 2025-10-14 01:45:54 +00:00 · f48e9b3ba6
commit f48e9b3ba6
parent 4d685c129c
4 changed files with 374 additions and 17 deletions
--- a/act/runner/job_executor.go
+++ b/act/runner/job_executor.go
@ -180,31 +180,35 @@ func setJobResult(ctx context.Context, info jobInfo, rc *RunContext, success boo
 		jobResult = "failure"
 	}

+	// Set local result on current job (child or parent)
 	info.result(jobResult)
+
 	if rc.caller != nil {
-		// set reusable workflow job result
+		// Child reusable workflow:
+		// 1) propagate result to parent job state
 		rc.caller.runContext.result(jobResult)
+
+		// 2) copy workflow_call outputs from child to parent (as in upstream)
+		jobOutputs := make(map[string]string)
+		ee := rc.NewExpressionEvaluator(ctx)
+		if wfcc := rc.Run.Workflow.WorkflowCallConfig(); wfcc != nil {
+			for k, v := range wfcc.Outputs {
+				jobOutputs[k] = ee.Interpolate(ctx, ee.Interpolate(ctx, v.Value))
+			}
+		}
+		rc.caller.runContext.Run.Job().Outputs = jobOutputs
+
+		// 3) DO NOT print banner in child job (prevents premature token revocation)
+		logger.Debugf("Reusable job result=%s (parent will finalize, no banner)", jobResult)
+		return
 	}

+	// Parent job: print the final banner ONCE (job-level)
 	jobResultMessage := "succeeded"
 	if jobResult != "success" {
 		jobResultMessage = "failed"
 	}
-
 	jobOutputs := rc.Run.Job().Outputs
-	if rc.caller != nil {
-		// Rewrite the job's outputs into the workflow_call outputs...
-		jobOutputs = make(map[string]string)
-		ee := rc.NewExpressionEvaluator(ctx)
-		for k, v := range rc.Run.Workflow.WorkflowCallConfig().Outputs {
-			jobOutputs[k] = ee.Interpolate(ctx, ee.Interpolate(ctx, v.Value))
-		}
-		// When running as a daemon and receiving jobs from Forgejo, the next job (and any of it's `needs` outputs) will
-		// be provided by Forgejo based upon the data sent to the logger below.  However, when running `forgejo-runner
-		// exec` with a reusable workflow, the next job will only be able to read outputs if those outputs are stored on
-		// the workflow -- that's what is accomplished here:
-		rc.caller.runContext.Run.Job().Outputs = jobOutputs
-	}

 	logger.
 		WithFields(logrus.Fields{
--- a/act/runner/job_executor_test.go
+++ b/act/runner/job_executor_test.go
@ -444,3 +444,76 @@ func TestSetJobResultConcurrency(t *testing.T) {

 	assert.Equal(t, "failure", lastResult)
 }
+
+func TestSetJobResult_SkipsBannerInChildReusableWorkflow(t *testing.T) {
+	// Test that child reusable workflow does not print final banner
+	// to prevent premature token revocation
+
+	mockLogger := mocks.NewFieldLogger(t)
+	// Allow all variants of Debugf (git operations can call with 1-3 args)
+	mockLogger.On("Debugf", mock.Anything).Return(0).Maybe()
+	mockLogger.On("Debugf", mock.Anything, mock.Anything).Return(0).Maybe()
+	mockLogger.On("Debugf", mock.Anything, mock.Anything, mock.Anything).Return(0).Maybe()
+	// CRITICAL: In CI, git ref detection may fail and call Warningf
+	mockLogger.On("Warningf", mock.Anything, mock.Anything).Return(0).Maybe()
+	mockLogger.On("WithField", mock.Anything, mock.Anything).Return(&logrus.Entry{Logger: &logrus.Logger{}}).Maybe()
+	mockLogger.On("WithFields", mock.Anything).Return(&logrus.Entry{Logger: &logrus.Logger{}}).Maybe()
+
+	ctx := common.WithLogger(common.WithJobErrorContainer(t.Context()), mockLogger)
+
+	// Setup parent job
+	parentJob := &model.Job{
+		Result: "success",
+	}
+	parentRC := &RunContext{
+		Config: &Config{Env: map[string]string{}}, // Must have Config
+		Run: &model.Run{
+			JobID: "parent",
+			Workflow: &model.Workflow{
+				Jobs: map[string]*model.Job{
+					"parent": parentJob,
+				},
+			},
+		},
+	}
+
+	// Setup child job with caller reference
+	childJob := &model.Job{
+		Result: "success",
+	}
+	childRC := &RunContext{
+		Config: &Config{Env: map[string]string{}}, // Must have Config
+		Run: &model.Run{
+			JobID: "child",
+			Workflow: &model.Workflow{
+				Jobs: map[string]*model.Job{
+					"child": childJob,
+				},
+			},
+		},
+		caller: &caller{
+			runContext: parentRC,
+		},
+	}
+
+	jim := &jobInfoMock{}
+	jim.On("matrix").Return(map[string]any{}) // REQUIRED: setJobResult always calls matrix()
+	jim.On("result", "success")
+
+	// Call setJobResult for child workflow
+	setJobResult(ctx, jim, childRC, true)
+
+	// Verify:
+	// 1. Child result is set
+	jim.AssertCalled(t, "result", "success")
+
+	// 2. Parent result is propagated
+	assert.Equal(t, "success", parentJob.Result)
+
+	// 3. Final banner was NOT printed by child (critical for token security)
+	mockLogger.AssertNotCalled(t, "WithFields", mock.MatchedBy(func(fields logrus.Fields) bool {
+		_, okJobResult := fields["jobResult"]
+		_, okJobOutput := fields["jobOutputs"]
+		return okJobOutput && okJobResult
+	}))
+}
--- a/act/runner/reusable_workflow.go
+++ b/act/runner/reusable_workflow.go
@ -16,6 +16,7 @@ import (
 	"code.forgejo.org/forgejo/runner/v11/act/common"
 	"code.forgejo.org/forgejo/runner/v11/act/common/git"
 	"code.forgejo.org/forgejo/runner/v11/act/model"
+	"github.com/sirupsen/logrus"
 )

 func newLocalReusableWorkflowExecutor(rc *RunContext) common.Executor {
@ -115,7 +116,10 @@ func newActionCacheReusableWorkflowExecutor(rc *RunContext, filename string, rem
 			return err
 		}

-		return runner.NewPlanExecutor(plan)(ctx)
+		planErr := runner.NewPlanExecutor(plan)(ctx)
+
+		// Finalize from parent context: one job-level banner
+		return finalizeReusableWorkflow(ctx, rc, planErr)
 	}
 }

@ -171,7 +175,10 @@ func newReusableWorkflowExecutor(rc *RunContext, directory, workflow string) com
 			return err
 		}

-		return runner.NewPlanExecutor(plan)(ctx)
+		planErr := runner.NewPlanExecutor(plan)(ctx)
+
+		// Finalize from parent context: one job-level banner
+		return finalizeReusableWorkflow(ctx, rc, planErr)
 	}
 }

@ -229,3 +236,29 @@ func newRemoteReusableWorkflowWithPlat(url, uses string) *remoteReusableWorkflow
 		URL:         url,
 	}
 }
+
+// finalizeReusableWorkflow prints the final job banner from the parent job context.
+//
+// The Forgejo reporter waits for this banner (log entry with "jobResult"
+// field and without stage="Main") before marking the job as complete and revoking
+// tokens. Printing this banner from the child reusable workflow would cause
+// premature token revocation, breaking subsequent steps in the parent workflow.
+func finalizeReusableWorkflow(ctx context.Context, rc *RunContext, planErr error) error {
+	jobResult := "success"
+	jobResultMessage := "succeeded"
+	if planErr != nil {
+		jobResult = "failure"
+		jobResultMessage = "failed"
+	}
+
+	// Outputs should already be present in the parent context:
+	// - copied by child's setJobResult branch (rc.caller != nil)
+	jobOutputs := rc.Run.Job().Outputs
+
+	common.Logger(ctx).WithFields(logrus.Fields{
+		"jobResult":  jobResult,
+		"jobOutputs": jobOutputs,
+	}).Infof("\U0001F3C1  Job %s", jobResultMessage)
+
+	return planErr
+}
--- a/act/runner/reusable_workflow_test.go
+++ b/act/runner/reusable_workflow_test.go
@ -0,0 +1,247 @@
+package runner
+
+import (
+	"errors"
+	"testing"
+
+	"code.forgejo.org/forgejo/runner/v11/act/common"
+	"code.forgejo.org/forgejo/runner/v11/act/model"
+	"code.forgejo.org/forgejo/runner/v11/act/runner/mocks"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+)
+
+func TestConfig_GetToken(t *testing.T) {
+	t.Run("returns GITEA_TOKEN when both GITEA_TOKEN and GITHUB_TOKEN present", func(t *testing.T) {
+		c := &Config{
+			Secrets: map[string]string{
+				"GITHUB_TOKEN": "github-token",
+				"GITEA_TOKEN":  "gitea-token",
+			},
+		}
+		assert.Equal(t, "gitea-token", c.GetToken())
+	})
+
+	t.Run("returns GITHUB_TOKEN when only GITHUB_TOKEN present", func(t *testing.T) {
+		c := &Config{
+			Secrets: map[string]string{
+				"GITHUB_TOKEN": "github-token",
+			},
+		}
+		assert.Equal(t, "github-token", c.GetToken())
+	})
+
+	t.Run("returns empty string when no tokens present", func(t *testing.T) {
+		c := &Config{
+			Secrets: map[string]string{},
+		}
+		assert.Equal(t, "", c.GetToken())
+	})
+
+	t.Run("returns empty string when Secrets is nil", func(t *testing.T) {
+		c := &Config{}
+		assert.Equal(t, "", c.GetToken())
+	})
+}
+
+func TestRemoteReusableWorkflow_CloneURL(t *testing.T) {
+	t.Run("adds https prefix when missing", func(t *testing.T) {
+		rw := &remoteReusableWorkflow{
+			URL:  "code.forgejo.org",
+			Org:  "owner",
+			Repo: "repo",
+		}
+		assert.Equal(t, "https://code.forgejo.org/owner/repo", rw.CloneURL())
+	})
+
+	t.Run("preserves https prefix", func(t *testing.T) {
+		rw := &remoteReusableWorkflow{
+			URL:  "https://code.forgejo.org",
+			Org:  "owner",
+			Repo: "repo",
+		}
+		assert.Equal(t, "https://code.forgejo.org/owner/repo", rw.CloneURL())
+	})
+
+	t.Run("preserves http prefix", func(t *testing.T) {
+		rw := &remoteReusableWorkflow{
+			URL:  "http://localhost:3000",
+			Org:  "owner",
+			Repo: "repo",
+		}
+		assert.Equal(t, "http://localhost:3000/owner/repo", rw.CloneURL())
+	})
+}
+
+func TestRemoteReusableWorkflow_FilePath(t *testing.T) {
+	tests := []struct {
+		name         string
+		gitPlatform  string
+		filename     string
+		expectedPath string
+	}{
+		{
+			name:         "github platform",
+			gitPlatform:  "github",
+			filename:     "test.yml",
+			expectedPath: "./.github/workflows/test.yml",
+		},
+		{
+			name:         "gitea platform",
+			gitPlatform:  "gitea",
+			filename:     "build.yaml",
+			expectedPath: "./.gitea/workflows/build.yaml",
+		},
+		{
+			name:         "forgejo platform",
+			gitPlatform:  "forgejo",
+			filename:     "deploy.yml",
+			expectedPath: "./.forgejo/workflows/deploy.yml",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rw := &remoteReusableWorkflow{
+				GitPlatform: tt.gitPlatform,
+				Filename:    tt.filename,
+			}
+			assert.Equal(t, tt.expectedPath, rw.FilePath())
+		})
+	}
+}
+
+func TestNewRemoteReusableWorkflowWithPlat(t *testing.T) {
+	tests := []struct {
+		name             string
+		url              string
+		uses             string
+		expectedOrg      string
+		expectedRepo     string
+		expectedPlatform string
+		expectedFilename string
+		expectedRef      string
+		shouldFail       bool
+	}{
+		{
+			name:             "valid github workflow",
+			url:              "github.com",
+			uses:             "owner/repo/.github/workflows/test.yml@main",
+			expectedOrg:      "owner",
+			expectedRepo:     "repo",
+			expectedPlatform: "github",
+			expectedFilename: "test.yml",
+			expectedRef:      "main",
+			shouldFail:       false,
+		},
+		{
+			name:             "valid gitea workflow",
+			url:              "code.forgejo.org",
+			uses:             "forgejo/runner/.gitea/workflows/build.yaml@v1.0.0",
+			expectedOrg:      "forgejo",
+			expectedRepo:     "runner",
+			expectedPlatform: "gitea",
+			expectedFilename: "build.yaml",
+			expectedRef:      "v1.0.0",
+			shouldFail:       false,
+		},
+		{
+			name:       "invalid format - missing platform",
+			url:        "github.com",
+			uses:       "owner/repo/workflows/test.yml@main",
+			shouldFail: true,
+		},
+		{
+			name:       "invalid format - no ref",
+			url:        "github.com",
+			uses:       "owner/repo/.github/workflows/test.yml",
+			shouldFail: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := newRemoteReusableWorkflowWithPlat(tt.url, tt.uses)
+
+			if tt.shouldFail {
+				assert.Nil(t, result)
+			} else {
+				assert.NotNil(t, result)
+				assert.Equal(t, tt.expectedOrg, result.Org)
+				assert.Equal(t, tt.expectedRepo, result.Repo)
+				assert.Equal(t, tt.expectedPlatform, result.GitPlatform)
+				assert.Equal(t, tt.expectedFilename, result.Filename)
+				assert.Equal(t, tt.expectedRef, result.Ref)
+				assert.Equal(t, tt.url, result.URL)
+			}
+		})
+	}
+}
+
+func TestFinalizeReusableWorkflow_PrintsBannerSuccess(t *testing.T) {
+	mockLogger := mocks.NewFieldLogger(t)
+
+	bannerCalled := false
+	mockLogger.On("WithFields",
+		mock.MatchedBy(func(fields logrus.Fields) bool {
+			result, ok := fields["jobResult"].(string)
+			if !ok || result != "success" {
+				return false
+			}
+			outs, ok := fields["jobOutputs"].(map[string]string)
+			return ok && outs["foo"] == "bar"
+		}),
+	).Run(func(args mock.Arguments) {
+		bannerCalled = true
+	}).Return(&logrus.Entry{Logger: &logrus.Logger{}}).Once()
+
+	ctx := common.WithLogger(t.Context(), mockLogger)
+	rc := &RunContext{
+		Run: &model.Run{
+			JobID: "parent",
+			Workflow: &model.Workflow{
+				Jobs: map[string]*model.Job{
+					"parent": {
+						Outputs: map[string]string{"foo": "bar"},
+					},
+				},
+			},
+		},
+	}
+
+	err := finalizeReusableWorkflow(ctx, rc, nil)
+	assert.NoError(t, err)
+	assert.True(t, bannerCalled, "final banner should be printed from parent")
+}
+
+func TestFinalizeReusableWorkflow_PrintsBannerFailure(t *testing.T) {
+	mockLogger := mocks.NewFieldLogger(t)
+
+	bannerCalled := false
+	mockLogger.On("WithFields",
+		mock.MatchedBy(func(fields logrus.Fields) bool {
+			result, ok := fields["jobResult"].(string)
+			return ok && result == "failure"
+		}),
+	).Run(func(args mock.Arguments) {
+		bannerCalled = true
+	}).Return(&logrus.Entry{Logger: &logrus.Logger{}}).Once()
+
+	ctx := common.WithLogger(t.Context(), mockLogger)
+	rc := &RunContext{
+		Run: &model.Run{
+			JobID: "parent",
+			Workflow: &model.Workflow{
+				Jobs: map[string]*model.Job{
+					"parent": {},
+				},
+			},
+		},
+	}
+
+	planErr := errors.New("workflow failed")
+	err := finalizeReusableWorkflow(ctx, rc, planErr)
+	assert.EqualError(t, err, "workflow failed")
+	assert.True(t, bannerCalled, "banner should be printed even on failure")
+}