[v15.0/forgejo] fix: ensure runners either belong to owner or repo when updated (#13262) (#13269)

A runner can either belong to an owner (user, organization) or a repository, not both. While `CreateRunner()` enforces that, `UpdateRunner()` does not, which leads to bugs. With this change, `UpdateRunner()` rejects runners that have _both_ fields set to prevent unexpected ownership changes.

Resolves https://codeberg.org/forgejo/forgejo/issues/12106 and makes https://codeberg.org/forgejo/forgejo/pulls/12117 obsolete.

### Tests for Go changes

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
  - [x] `make pr-go` before pushing

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13262
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
(cherry picked from commit 85d38e354d)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13269
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>

models/actions/forgejo_test.go

@@ -129,7 +129,7 @@ func TestActions_RegisterRunner_UpdateWithLabels(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())
 	unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID})
 
-	newOwnerID := int64(1)
+	newOwnerID := int64(0)
 	newRepoID := int64(1)
 	newName := "rennur"
 	newVersion := "v4.5.6"
@@ -164,7 +164,7 @@ func TestActions_RegisterRunner_UpdateWithoutLabels(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())
 	before := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID})
 
-	newOwnerID := int64(1)
+	newOwnerID := int64(0)
 	newRepoID := int64(1)
 	newName := "rennur"
 	newVersion := "v4.5.6"

models/actions/runner.go

@@ -307,6 +307,13 @@ func GetVisibleRunnerByID(ctx context.Context, id, ownerID, repoID int64) (*Acti
 
 // UpdateRunner updates runner's information.
 func UpdateRunner(ctx context.Context, r *ActionRunner, cols ...string) error {
+	if r.OwnerID != 0 && r.RepoID != 0 {
+		// The ownership of existing runners should not be changed silently. That leads to subtle bugs and inscrutable
+		// behaviour.
+		return fmt.Errorf("OwnerID (%d) and RepoID (%d) of runner %d cannot be set simultaneously",
+			r.OwnerID, r.RepoID, r.ID)
+	}
+
 	e := db.GetEngine(ctx)
 	r.Name, _ = util.SplitStringAtByteN(r.Name, 255)
 	var err error

models/actions/runner_test.go

@@ -12,6 +12,7 @@ import (
 	"forgejo.org/models/db"
 	"forgejo.org/models/repo"
 	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/timeutil"
 
 	"github.com/stretchr/testify/assert"
@@ -479,3 +480,59 @@ func TestRunner_FindRunnerOptionsToConds(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdateRunner(t *testing.T) {
+	t.Run("ownership is not altered", func(t *testing.T) {
+		require.NoError(t, unittest.PrepareTestDatabase())
+
+		runnerUUID := "86b2f19a-3fbb-410b-ace6-2a2ace078a28"
+
+		require.NoError(t, CreateRunner(t.Context(), &ActionRunner{UUID: runnerUUID, Name: "old name"}))
+
+		runner := unittest.AssertExistsAndLoadBean(t, &ActionRunner{UUID: runnerUUID})
+
+		assert.Zero(t, runner.OwnerID)
+		assert.Zero(t, runner.RepoID)
+		assert.Equal(t, "old name", runner.Name)
+
+		runner.Name = "new name"
+
+		require.NoError(t, UpdateRunner(t.Context(), runner))
+
+		runner = unittest.AssertExistsAndLoadBean(t, &ActionRunner{UUID: runnerUUID})
+
+		assert.Zero(t, runner.OwnerID)
+		assert.Zero(t, runner.RepoID)
+		assert.Equal(t, "new name", runner.Name)
+	})
+
+	t.Run("OwnerID and RepoID cannot be set simultaneously", func(t *testing.T) {
+		require.NoError(t, unittest.PrepareTestDatabase())
+
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+		repo62 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 62, OwnerID: user2.ID})
+		runnerUUID := "86b2f19a-3fbb-410b-ace6-2a2ace078a28"
+
+		require.NoError(t, CreateRunner(t.Context(), &ActionRunner{UUID: runnerUUID, OwnerID: user2.ID, Name: "old name"}))
+
+		runner := unittest.AssertExistsAndLoadBean(t, &ActionRunner{UUID: runnerUUID})
+
+		assert.Equal(t, user2.ID, runner.OwnerID)
+		assert.Zero(t, runner.RepoID)
+		assert.Equal(t, "old name", runner.Name)
+
+		// Attempt to violate scoping rules by simultaneously setting OwnerID and RepoID.
+		runner.RepoID = repo62.ID
+		runner.Name = "new name"
+
+		err := UpdateRunner(t.Context(), runner)
+		require.ErrorContains(t, err, "OwnerID (2) and RepoID (62) of runner")
+
+		// Verify that runner has not been changed.
+		runner = unittest.AssertExistsAndLoadBean(t, &ActionRunner{UUID: runnerUUID})
+
+		assert.Equal(t, user2.ID, runner.OwnerID)
+		assert.Zero(t, runner.RepoID)
+		assert.Equal(t, "old name", runner.Name)
+	})
+}

routers/private/actions.go

@@ -35,7 +35,7 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) {
 		return
 	}
 
-	owner, repo, err := parseScope(ctx, genRequest.Scope)
+	owner, repo, err := ParseScope(ctx, genRequest.Scope)
 	if err != nil {
 		log.Error("parseScope failed: %v", err)
 		ctx.JSON(http.StatusInternalServerError, private.Response{
@@ -77,32 +77,24 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) {
 }
 
 func ParseScope(ctx gocontext.Context, scope string) (ownerID, repoID int64, err error) {
-	return parseScope(ctx, scope)
-}
-
-func parseScope(ctx gocontext.Context, scope string) (ownerID, repoID int64, err error) {
-	ownerID = 0
-	repoID = 0
 	if scope == "" {
-		return ownerID, repoID, nil
+		return 0, 0, nil
 	}
 
 	ownerName, repoName, found := strings.Cut(scope, "/")
 
 	u, err := user_model.GetUserByName(ctx, ownerName)
 	if err != nil {
-		return ownerID, repoID, err
+		return 0, 0, err
 	}
-	ownerID = u.ID
 
 	if !found {
-		return ownerID, repoID, nil
+		return u.ID, 0, nil
 	}
 
 	r, err := repo_model.GetRepositoryByName(ctx, u.ID, repoName)
 	if err != nil {
-		return ownerID, repoID, err
+		return 0, 0, err
 	}
-	repoID = r.ID
-	return ownerID, repoID, nil
+	return 0, r.ID, nil
 }

routers/private/actions_test.go

@@ -0,0 +1,85 @@
+// Copyright 2026 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package private
+
+import (
+	"testing"
+
+	"forgejo.org/models/unittest"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseScope(t *testing.T) {
+	testCases := []struct {
+		name          string
+		scope         string
+		expectedOwner int64
+		expectedRepo  int64
+		expectedError string
+	}{
+		{
+			name:          "instance scope",
+			scope:         "",
+			expectedOwner: 0,
+			expectedRepo:  0,
+		},
+		{
+			name:          "user scope",
+			scope:         "user2",
+			expectedOwner: 2,
+			expectedRepo:  0,
+		},
+		{
+			name:          "organization scope",
+			scope:         "org3",
+			expectedOwner: 3,
+			expectedRepo:  0,
+		},
+		{
+			name:          "unknown user",
+			scope:         "does-not-exist",
+			expectedError: "user does not exist",
+		},
+		{
+			name:          "repository scope",
+			scope:         "user2/test_workflows",
+			expectedOwner: 0,
+			expectedRepo:  62,
+		},
+		{
+			name:          "empty repository",
+			scope:         "user2/",
+			expectedError: "repository does not exist",
+		},
+		{
+			name:          "unknown repository",
+			scope:         "user2/does-not-exist",
+			expectedError: "repository does not exist",
+		},
+		{
+			name:          "owner mismatch",
+			scope:         "org3/test_workflows",
+			expectedError: "repository does not exist",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			require.NoError(t, unittest.PrepareTestDatabase())
+
+			owner, repo, err := ParseScope(t.Context(), testCase.scope)
+
+			if testCase.expectedError == "" {
+				require.NoError(t, err)
+
+				assert.Equal(t, testCase.expectedOwner, owner)
+				assert.Equal(t, testCase.expectedRepo, repo)
+			} else {
+				require.ErrorContains(t, err, testCase.expectedError)
+			}
+		})
+	}
+}

tests/integration/api_private_actions_test.go

@@ -0,0 +1,93 @@
+// Copyright 2026 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package integration
+
+import (
+	"net/url"
+	"testing"
+
+	"forgejo.org/models/actions"
+	"forgejo.org/models/unittest"
+	"forgejo.org/modules/optional"
+	"forgejo.org/modules/private"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGenerateActionsRunnerToken(t *testing.T) {
+	testCases := []struct {
+		name          string
+		scope         string
+		expectedOwner optional.Option[int64]
+		expectedRepo  optional.Option[int64]
+		expectedError string
+	}{
+		{
+			name:          "instance scope",
+			scope:         "",
+			expectedOwner: optional.None[int64](),
+			expectedRepo:  optional.None[int64](),
+		},
+
+		{
+			name:          "user scope",
+			scope:         "user2",
+			expectedOwner: optional.Some[int64](2),
+			expectedRepo:  optional.None[int64](),
+		},
+
+		{
+			name:          "organization scope",
+			scope:         "org3",
+			expectedOwner: optional.Some[int64](3),
+			expectedRepo:  optional.None[int64](),
+		},
+		{
+			name:          "unknown user",
+			scope:         "does-not-exist",
+			expectedError: "user does not exist",
+		},
+		{
+			name:          "repository scope",
+			scope:         "user2/test_workflows",
+			expectedOwner: optional.None[int64](),
+			expectedRepo:  optional.Some[int64](62),
+		},
+		{
+			name:          "empty repository",
+			scope:         "user2/",
+			expectedError: "repository does not exist",
+		},
+		{
+			name:          "unknown repository",
+			scope:         "user2/does-not-exist",
+			expectedError: "repository does not exist",
+		},
+		{
+			name:          "owner mismatch",
+			scope:         "org3/test_workflows",
+			expectedError: "repository does not exist",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			onApplicationRun(t, func(*testing.T, *url.URL) {
+				text, extra := private.GenerateActionsRunnerToken(t.Context(), testCase.scope)
+
+				if testCase.expectedError == "" {
+					require.NoError(t, extra.Error)
+
+					newToken := unittest.AssertExistsAndLoadBean(t,
+						&actions.ActionRunnerToken{OwnerID: testCase.expectedOwner, RepoID: testCase.expectedRepo})
+
+					assert.Equal(t, newToken.Token, text.Text)
+				} else {
+					assert.ErrorContains(t, extra.Error, testCase.expectedError)
+				}
+			})
+		})
+	}
+}

tests/integration/cmd_forgejo_actions_test.go

@@ -191,12 +191,16 @@ func TestActions_CmdForgejo_Actions(t *testing.T) {
 						action, err := actions_model.GetRunnerByUUID(t.Context(), uuid)
 						require.NoError(t, err)
 
-						user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: action.OwnerID})
-						assert.Equal(t, ownerName, user.Name, action.OwnerID)
-
 						if found {
+							assert.Zero(t, action.OwnerID)
+
 							repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: action.RepoID})
 							assert.Equal(t, repoName, repo.Name, action.RepoID)
+						} else {
+							assert.Zero(t, action.RepoID)
+
+							user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: action.OwnerID})
+							assert.Equal(t, ownerName, user.Name, action.OwnerID)
 						}
 						if testCase.name != "" {
 							assert.Equal(t, testCase.name, action.Name)