From 4894725b64497193449dc736db059eb5ad74c4ca Mon Sep 17 00:00:00 2001 From: Andreas Ahlenstorf Date: Wed, 1 Jul 2026 20:20:45 +0200 Subject: [PATCH] [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 (cherry picked from commit 85d38e354d31882038f3fb0ae81ee87c67099e11) Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13269 Reviewed-by: Mathieu Fenniak --- models/actions/forgejo_test.go | 4 +- models/actions/runner.go | 7 ++ models/actions/runner_test.go | 57 ++++++++++++ routers/private/actions.go | 20 ++-- routers/private/actions_test.go | 85 +++++++++++++++++ tests/integration/api_private_actions_test.go | 93 +++++++++++++++++++ tests/integration/cmd_forgejo_actions_test.go | 10 +- 7 files changed, 257 insertions(+), 19 deletions(-) create mode 100644 routers/private/actions_test.go create mode 100644 tests/integration/api_private_actions_test.go diff --git a/models/actions/forgejo_test.go b/models/actions/forgejo_test.go index 2170b0927a..7cb0a02a81 100644 --- a/models/actions/forgejo_test.go +++ b/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" diff --git a/models/actions/runner.go b/models/actions/runner.go index 8061ccf37e..7270e0a787 100644 --- a/models/actions/runner.go +++ b/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 diff --git a/models/actions/runner_test.go b/models/actions/runner_test.go index acca9b1761..842b76ff2b 100644 --- a/models/actions/runner_test.go +++ b/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) + }) +} diff --git a/routers/private/actions.go b/routers/private/actions.go index a85c695752..40205dfe69 100644 --- a/routers/private/actions.go +++ b/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 } diff --git a/routers/private/actions_test.go b/routers/private/actions_test.go new file mode 100644 index 0000000000..9c34de7655 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/tests/integration/api_private_actions_test.go b/tests/integration/api_private_actions_test.go new file mode 100644 index 0000000000..98a5d8f482 --- /dev/null +++ b/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) + } + }) + }) + } +} diff --git a/tests/integration/cmd_forgejo_actions_test.go b/tests/integration/cmd_forgejo_actions_test.go index 088cb5860b..3099752198 100644 --- a/tests/integration/cmd_forgejo_actions_test.go +++ b/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)