From cb4ffd29cf67d5eb62cda15b6da0f3dac8a335e3 Mon Sep 17 00:00:00 2001 From: Hailey Somerville Date: Mon, 18 Aug 2025 14:40:07 +0200 Subject: [PATCH] fix: allow Actions tokens to access repos readable by signed in users (#8889) This is an alternate take on https://codeberg.org/forgejo/forgejo/pulls/8808 which allows Actions tokens to access other repos which are readable by by signed in users. In practise this means public repos belonging to public or limited owners. This PR is split into two commits to aid review of the security-sensitive changes I've made: * The first commit is a refactor _which is not intended to change behaviour_. It extracts the permission logic for Actions tokens from the githttp handler and moves it to `repo_permission.go` alongside the permission logic for regular users. The new function, `GetActionRepoPermission` returns a `Permission` object just like `GetUserRepoPermission`. Only code unit access is currently allowed in the interest of keeping this commit from changing any access logic. * The second commit is the broadening of access: this commit changes the logic in `GetActionRepoPermission` to give actions tokens access to repos readable by signed in users. cc @earl-warren ref https://codeberg.org/forgejo/forgejo/issues/5877 ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - 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 added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### 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 - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. ## Release notes - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/8889): allow Actions tokens to access repos readable by signed in users Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8889 Reviewed-by: Earl Warren Reviewed-by: Michael Kriese Co-authored-by: Hailey Somerville Co-committed-by: Hailey Somerville --- models/perm/access/repo_permission.go | 28 ++++++++ models/perm/access/repo_permission_test.go | 78 ++++++++++++++++++++++ routers/web/repo/githttp.go | 23 +++---- tests/integration/git_test.go | 28 ++++++++ 4 files changed, 143 insertions(+), 14 deletions(-) create mode 100644 models/perm/access/repo_permission_test.go diff --git a/models/perm/access/repo_permission.go b/models/perm/access/repo_permission.go index ce9963b83a..f7daf38e5c 100644 --- a/models/perm/access/repo_permission.go +++ b/models/perm/access/repo_permission.go @@ -7,6 +7,7 @@ import ( "context" "fmt" + actions_model "forgejo.org/models/actions" "forgejo.org/models/db" "forgejo.org/models/organization" perm_model "forgejo.org/models/perm" @@ -136,6 +137,33 @@ func (p *Permission) LogString() string { return fmt.Sprintf(format, args...) } +func GetActionRepoPermission(ctx context.Context, repo *repo_model.Repository, task *actions_model.ActionTask) (Permission, error) { + // straight forward case: an actions task is attempting to access its own repo + if task.RepoID == repo.ID { + var mode perm_model.AccessMode + + // determine default access mode for repo: + if task.IsForkPullRequest { + mode = perm_model.AccessModeRead + } else { + mode = perm_model.AccessModeWrite + } + + if err := repo.LoadUnits(ctx); err != nil { + return Permission{}, err + } + + perm := Permission{ + AccessMode: mode, + Units: repo.Units, + } + + return perm, nil + } + + return GetUserRepoPermission(ctx, repo, user_model.NewActionsUser()) +} + // GetUserRepoPermission returns the user permissions to the repository func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, user *user_model.User) (Permission, error) { var perm Permission diff --git a/models/perm/access/repo_permission_test.go b/models/perm/access/repo_permission_test.go new file mode 100644 index 0000000000..55bc975421 --- /dev/null +++ b/models/perm/access/repo_permission_test.go @@ -0,0 +1,78 @@ +package access_test + +import ( + "testing" + + actions_model "forgejo.org/models/actions" + "forgejo.org/models/db" + perm_model "forgejo.org/models/perm" + "forgejo.org/models/perm/access" + repo_model "forgejo.org/models/repo" + "forgejo.org/models/unittest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func assertAccess(t *testing.T, expectedMode perm_model.AccessMode, perm *access.Permission) { + assert.Equal(t, expectedMode, perm.AccessMode) + + for _, unit := range perm.Units { + assert.Equal(t, expectedMode, perm.UnitAccessMode(unit.Type)) + } +} + +func TestActionTaskCanAccessOwnRepo(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + actionTask := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: actionTask.RepoID}) + + perm, err := access.GetActionRepoPermission(db.DefaultContext, repo, actionTask) + require.NoError(t, err) + assertAccess(t, perm_model.AccessModeWrite, &perm) +} + +func TestActionTaskCanAccessPublicRepo(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + actionTask := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + perm, err := access.GetActionRepoPermission(db.DefaultContext, repo, actionTask) + require.NoError(t, err) + assertAccess(t, perm_model.AccessModeRead, &perm) +} + +func TestActionTaskCanAccessPublicRepoOfLimitedOrg(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + actionTask := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 38}) + + perm, err := access.GetActionRepoPermission(db.DefaultContext, repo, actionTask) + require.NoError(t, err) + assertAccess(t, perm_model.AccessModeRead, &perm) +} + +func TestActionTaskNoAccessPublicRepoOfPrivateOrg(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + actionTask := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 40}) + + perm, err := access.GetActionRepoPermission(db.DefaultContext, repo, actionTask) + require.NoError(t, err) + assertAccess(t, perm_model.AccessModeNone, &perm) +} + +func TestActionTaskNoAccessPrivateRepo(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + actionTask := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + + perm, err := access.GetActionRepoPermission(db.DefaultContext, repo, actionTask) + require.NoError(t, err) + assertAccess(t, perm_model.AccessModeNone, &perm) +} diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go index 42302d0e02..a60c213113 100644 --- a/routers/web/repo/githttp.go +++ b/routers/web/repo/githttp.go @@ -192,24 +192,19 @@ func httpBase(ctx *context.Context) *serviceHandler { ctx.ServerError("GetTaskByID", err) return nil } - if task.RepoID != repo.ID { + + p, err := access_model.GetActionRepoPermission(ctx, repo, task) + if err != nil { + ctx.ServerError("GetActionRepoPermission", err) + return nil + } + + if !p.CanAccess(accessMode, unitType) { ctx.PlainText(http.StatusForbidden, "User permission denied") return nil } - if task.IsForkPullRequest { - if accessMode > perm.AccessModeRead { - ctx.PlainText(http.StatusForbidden, "User permission denied") - return nil - } - environ = append(environ, fmt.Sprintf("%s=%d", repo_module.EnvActionPerm, perm.AccessModeRead)) - } else { - if accessMode > perm.AccessModeWrite { - ctx.PlainText(http.StatusForbidden, "User permission denied") - return nil - } - environ = append(environ, fmt.Sprintf("%s=%d", repo_module.EnvActionPerm, perm.AccessModeWrite)) - } + environ = append(environ, fmt.Sprintf("%s=%d", repo_module.EnvActionPerm, p.AccessMode)) } else { p, err := access_model.GetUserRepoPermission(ctx, repo, ctx.Doer) if err != nil { diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index 26cddf7288..fcd81cf6b2 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -20,6 +20,7 @@ import ( "testing" "time" + actions_model "forgejo.org/models/actions" auth_model "forgejo.org/models/auth" "forgejo.org/models/db" git_model "forgejo.org/models/git" @@ -51,6 +52,33 @@ func TestGit(t *testing.T) { onGiteaRun(t, testGit) } +func TestActionsTokenAuth(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + task.GenerateToken() + actions_model.UpdateTask(db.DefaultContext, task) + u.User = url.UserPassword("token", task.Token) + + t.Run("clone task's own repo", func(t *testing.T) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: task.RepoID}) + u.Path = fmt.Sprintf("/%s/%s.git", repo.OwnerName, repo.Name) + doGitClone(t.TempDir(), u)(t) + }) + + t.Run("clone public repo of limited owner", func(t *testing.T) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 38}) + u.Path = fmt.Sprintf("/%s/%s.git", repo.OwnerName, repo.Name) + doGitClone(t.TempDir(), u)(t) + }) + + t.Run("cannot clone private repo", func(t *testing.T) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + u.Path = fmt.Sprintf("/%s/%s.git", repo.OwnerName, repo.Name) + doGitCloneFail(u)(t) + }) + }) +} + func testGit(t *testing.T, u *url.URL) { username := "user2" baseAPITestContext := NewAPITestContext(t, username, "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)