From afb5825b73bc486e600e1204b526359a524bd292 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 21 Nov 2025 04:31:31 +0100 Subject: [PATCH] [v11.0/forgejo] 2025-11-21 combined security patches (#10039) [CVSS 5.3 Medium](https://www.first.org/cvss/calculator/4-0#CVSS:4.0/AV:N/AC:L/AT:N/PR:L/UI:N/VC:L/VI:N/VA:N/SC:N/SI:N/SA:N) -- The `/repos/{owner}/{repo}/issues/{index}/dependencies` APIs allow a user to link an issue in one repository as "depending upon" an issue in another repository. Forgejo's implementation had an incorrect permission check which would verify only that the user had write permissions on the issue being modified, and not on the issue it was linking to. Due to the incorrect permission check, it was possible to view limited information (the existence of, and title of) an issue in a private repository that the user does not have access to view. The permission check has been corrected to take into account visibility of the remote repository. [CVSS 5.3 Medium](https://www.first.org/cvss/calculator/4-0#CVSS:4.0/AV:N/AC:L/AT:N/PR:L/UI:N/VC:L/VI:N/VA:N/SC:N/SI:N/SA:N) -- Fetching information about a release via the `/repos/{owner}/{repo}/releases/tag/{tag}` API endpoint did not check whether the release was a draft, allowing accessing to information about a draft release to users who could predict an upcoming release tag but didn't have access to view it. The missing check has been added, returning a 404 response when the release is not published. [CVSS 6.3 Medium](https://www.first.org/cvss/calculator/4-0#CVSS:4.0/AV:N/AC:L/AT:P/PR:N/UI:N/VC:N/VI:L/VA:N/SC:N/SI:N/SA:N) -- Forgejo's web interface allows deleting tags on a git repository through a form post. The endpoint for this form post had misconfigured middleware handlers which enforce security rights, allowing an anonymous user, or a logged-in user without the correct permissions, to delete tags on repositories that they did not own by injecting arbitrary internal tag identifiers into the form. The middleware handler configuration has been corrected. [CVSS 2.1 Low](https://www.first.org/cvss/calculator/4-0#CVSS:4.0/AV:N/AC:L/AT:P/PR:H/UI:N/VC:N/VI:L/VA:N/SC:N/SI:N/SA:N) -- When the head branch of a pull request matches a branch protection rule, the head branch should be able to be merged or rebased only according to the "Push" rules defined in the protection rule. An implementation error checked those branch protection rules in the context of the base repository rather than the head repository, allowing users with write access to the base repository to be considered able to push to the branch, bypassing the "Enable push" option's expected security control. [CVSS 2.1 Low](https://www.first.org/cvss/calculator/4-0#CVSS:4.0/AV:N/AC:L/AT:P/PR:H/UI:N/VC:N/VI:L/VA:N/SC:N/SI:N/SA:N) -- An issue owner can manipulate form inputs to delete the content history of comments they did not create, as long as those comments are on issues that they own. Although comment content is not affected, the history of edits on the comment can be trimmed. The validation in the form handler was corrected. [CVSS 5.1 Medium](https://www.first.org/cvss/calculator/4-0#CVSS:4.0/AV:N/AC:L/AT:N/PR:H/UI:N/VC:N/VI:L/VA:N/SC:N/SI:N/SA:N) -- When a repository is configured with tag protection rules, it should not be possible for a user that is outside the whitelisted users or teams from modifying the protected tags. An incorrect parameter being passed to a security verification method allowed a user with write access to the repo to delete tags even if they were protected, as long as the tag was originally created by a user who is still authorized by the protection rules. ## Release notes - Security bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/10039): fix(api): fix dependency repo perms in Create/RemoveIssueDependency - [PR](https://codeberg.org/forgejo/forgejo/pulls/10039): fix(api): draft releases could be read before being published - [PR](https://codeberg.org/forgejo/forgejo/pulls/10039): misconfigured security checks on tag delete web form - [PR](https://codeberg.org/forgejo/forgejo/pulls/10039): incorrect logic in "Update PR" did not enforce head branch protection rules correctly - [PR](https://codeberg.org/forgejo/forgejo/pulls/10039): issue owner can delete another user's comment's edit history on same issue - [PR](https://codeberg.org/forgejo/forgejo/pulls/10039): tag protection rules can be bypassed during tag delete operation Co-authored-by: Joshua Rogers Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10039 Reviewed-by: 0ko <0ko@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- modules/web/route.go | 2 +- release-notes/10039.md | 6 +++ routers/api/v1/repo/issue_dependency.go | 4 +- routers/api/v1/repo/release_tags.go | 8 ++++ routers/web/repo/issue_content_history.go | 9 ++-- routers/web/web.go | 4 +- services/pull/update.go | 2 +- services/release/release.go | 2 +- tests/integration/api_issue_test.go | 46 +++++++++++++++++++ tests/integration/api_releases_test.go | 52 ++++++++++++++-------- tests/integration/api_repo_tags_test.go | 53 ++++++++++++++++++++++ tests/integration/pull_update_test.go | 54 +++++++++++++++++++++-- tests/integration/release_test.go | 44 ++++++++++++++++-- tests/integration/repo_tag_test.go | 2 +- tests/test_utils.go | 2 + 15 files changed, 252 insertions(+), 38 deletions(-) create mode 100644 release-notes/10039.md diff --git a/modules/web/route.go b/modules/web/route.go index 046c9f4ba7..ceb97ba333 100644 --- a/modules/web/route.go +++ b/modules/web/route.go @@ -88,7 +88,7 @@ func (r *Route) wrapMiddlewareAndHandler(h []any) ([]func(http.Handler) http.Han } } for _, m := range h { - if h != nil { + if m != nil { handlerProviders = append(handlerProviders, toHandlerProvider(m)) } } diff --git a/release-notes/10039.md b/release-notes/10039.md new file mode 100644 index 0000000000..f0019ef55b --- /dev/null +++ b/release-notes/10039.md @@ -0,0 +1,6 @@ +fix(api): fix dependency repo perms in Create/RemoveIssueDependency +fix(api): draft releases could be read before being published +fix: misconfigured security checks on tag delete web form +fix: incorrect logic in "Update PR" did not enforce head branch protection rules correctly +fix: issue owner can delete another user's comment's edit history on same issue +fix: tag protection rules can be bypassed during tag delete operation diff --git a/routers/api/v1/repo/issue_dependency.go b/routers/api/v1/repo/issue_dependency.go index bed1e7ecf9..ae46b087a1 100644 --- a/routers/api/v1/repo/issue_dependency.go +++ b/routers/api/v1/repo/issue_dependency.go @@ -204,7 +204,7 @@ func CreateIssueDependency(ctx *context.APIContext) { return } - dependencyPerm := getPermissionForRepo(ctx, target.Repo) + dependencyPerm := getPermissionForRepo(ctx, dependency.Repo) if ctx.Written() { return } @@ -265,7 +265,7 @@ func RemoveIssueDependency(ctx *context.APIContext) { return } - dependencyPerm := getPermissionForRepo(ctx, target.Repo) + dependencyPerm := getPermissionForRepo(ctx, dependency.Repo) if ctx.Written() { return } diff --git a/routers/api/v1/repo/release_tags.go b/routers/api/v1/repo/release_tags.go index b27f8584bc..e6b8a18abd 100644 --- a/routers/api/v1/repo/release_tags.go +++ b/routers/api/v1/repo/release_tags.go @@ -8,6 +8,7 @@ import ( "forgejo.org/models" repo_model "forgejo.org/models/repo" + unit_model "forgejo.org/models/unit" "forgejo.org/services/context" "forgejo.org/services/convert" releaseservice "forgejo.org/services/release" @@ -59,6 +60,13 @@ func GetReleaseByTag(ctx *context.APIContext) { return } + if release.IsDraft { + if !ctx.IsSigned || !ctx.Repo.CanWrite(unit_model.TypeReleases) { + ctx.NotFound() + return + } + } + if err = release.LoadAttributes(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) return diff --git a/routers/web/repo/issue_content_history.go b/routers/web/repo/issue_content_history.go index 5c71d75f80..4d3a2b1a66 100644 --- a/routers/web/repo/issue_content_history.go +++ b/routers/web/repo/issue_content_history.go @@ -204,12 +204,11 @@ func SoftDeleteContentHistory(ctx *context.Context) { ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } + if history.CommentID != commentID { + ctx.NotFound("CompareCommentID", issues_model.ErrCommentNotExist{}) + return + } if commentID != 0 { - if history.CommentID != commentID { - ctx.NotFound("CompareCommentID", issues_model.ErrCommentNotExist{}) - return - } - if comment, err = issues_model.GetCommentByID(ctx, commentID); err != nil { log.Error("can not get comment for issue content history %v. err=%v", historyID, err) return diff --git a/routers/web/web.go b/routers/web/web.go index 835a408276..5bd5c3fa81 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1285,8 +1285,8 @@ func registerRoutes(m *web.Route) { m.Get(".atom", feedEnabled, repo.TagsListFeedAtom) }, ctxDataSet("EnableFeed", setting.Other.EnableFeed), repo.MustBeNotEmpty, reqRepoCodeReader, context.RepoRefByType(context.RepoRefTag, true)) - m.Post("/tags/delete", repo.DeleteTag, reqSignIn, - repo.MustBeNotEmpty, context.RepoMustNotBeArchived(), reqRepoCodeWriter, context.RepoRef()) + m.Post("/tags/delete", reqSignIn, repo.MustBeNotEmpty, context.RepoMustNotBeArchived(), reqRepoCodeWriter, + context.RepoRef(), repo.DeleteTag) }, ignSignIn, context.RepoAssignment, context.UnitTypes()) // Releases diff --git a/services/pull/update.go b/services/pull/update.go index 1b4b6b039d..84b628ab4c 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -135,7 +135,7 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, // Update function need push permission if pb != nil { - pb.Repo = pull.BaseRepo + pb.Repo = pull.HeadRepo if !pb.CanUserPush(ctx, user) { return false, false, nil } diff --git a/services/release/release.go b/services/release/release.go index f0682c4dca..23cd82c64d 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -399,7 +399,7 @@ func DeleteReleaseByID(ctx context.Context, repo *repo_model.Repository, rel *re if err != nil { return fmt.Errorf("GetProtectedTags: %w", err) } - isAllowed, err := git_model.IsUserAllowedToControlTag(ctx, protectedTags, rel.TagName, rel.PublisherID) + isAllowed, err := git_model.IsUserAllowedToControlTag(ctx, protectedTags, rel.TagName, doer.ID) if err != nil { return err } diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index 99fc158fdd..8235be7817 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -18,6 +18,7 @@ import ( repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/optional" "forgejo.org/modules/setting" api "forgejo.org/modules/structs" "forgejo.org/tests" @@ -620,3 +621,48 @@ func TestAPISearchIssuesWithLabels(t *testing.T) { DecodeJSON(t, resp, &apiIssues) assert.Len(t, apiIssues, 2) } + +func TestAPIIssueDependencyPermissions(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + actingUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + token := getUserToken(t, actingUser.Name, auth_model.AccessTokenScopeAll) + + actingUserRepo, _, reset := tests.CreateDeclarativeRepoWithOptions(t, actingUser, tests.DeclarativeRepoOptions{}) + defer reset() + actingUserIssue := createIssue(t, actingUser, actingUserRepo, "source issue", "some content") + + otherUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + otherUserRepo, _, reset := tests.CreateDeclarativeRepoWithOptions(t, otherUser, tests.DeclarativeRepoOptions{ + IsPrivate: optional.Some(true), + }) + defer reset() + otherUserIssue := createIssue(t, otherUser, otherUserRepo, "target issue", "some content") + + apiEndpoint := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/dependencies", actingUserRepo.OwnerName, actingUserRepo.Name, actingUserIssue.Index) + req := NewRequest(t, "GET", apiEndpoint).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + var blockingIssues []*api.Issue + DecodeJSON(t, resp, &blockingIssues) + require.Empty(t, blockingIssues) + + req = NewRequestWithJSON(t, "POST", apiEndpoint, api.IssueMeta{ + Owner: otherUserRepo.OwnerName, + Name: otherUserRepo.Name, + Index: otherUserIssue.Index, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) // as otherUserRepo is a private repo we can't link a dependency to it + + req = NewRequest(t, "GET", apiEndpoint).AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + blockingIssues = []*api.Issue{} // reset + DecodeJSON(t, resp, &blockingIssues) + require.Empty(t, blockingIssues) + + req = NewRequestWithJSON(t, "DELETE", apiEndpoint, api.IssueMeta{ + Owner: otherUserRepo.OwnerName, + Name: otherUserRepo.Name, + Index: otherUserIssue.Index, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) // as otherUserRepo is a private repo we can't link a dependency to it +} diff --git a/tests/integration/api_releases_test.go b/tests/integration/api_releases_test.go index 98fc293fbc..b0915c0855 100644 --- a/tests/integration/api_releases_test.go +++ b/tests/integration/api_releases_test.go @@ -26,7 +26,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestAPIListReleases(t *testing.T) { +func TestAPIReleaseList(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -104,7 +104,7 @@ func createNewReleaseUsingAPI(t *testing.T, token string, owner *user_model.User return &newRelease } -func TestAPICreateAndUpdateRelease(t *testing.T) { +func TestAPIReleaseCreateAndUpdate(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -161,7 +161,7 @@ func TestAPICreateAndUpdateRelease(t *testing.T) { assert.True(t, newRelease.HideArchiveLinks) } -func TestAPICreateProtectedTagRelease(t *testing.T) { +func TestAPIReleaseCreateProtectedTag(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) @@ -186,7 +186,7 @@ func TestAPICreateProtectedTagRelease(t *testing.T) { MakeRequest(t, req, http.StatusUnprocessableEntity) } -func TestAPICreateReleaseToDefaultBranch(t *testing.T) { +func TestAPIReleaseCreateToDefaultBranch(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -197,7 +197,7 @@ func TestAPICreateReleaseToDefaultBranch(t *testing.T) { createNewReleaseUsingAPI(t, token, owner, repo, "v0.0.1", "", "v0.0.1", "test") } -func TestAPICreateReleaseToDefaultBranchOnExistingTag(t *testing.T) { +func TestAPIReleaseCreateToDefaultBranchOnExistingTag(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -215,7 +215,7 @@ func TestAPICreateReleaseToDefaultBranchOnExistingTag(t *testing.T) { createNewReleaseUsingAPI(t, token, owner, repo, "v0.0.1", "", "v0.0.1", "test") } -func TestAPICreateReleaseGivenInvalidTarget(t *testing.T) { +func TestAPIReleaseCreateGivenInvalidTarget(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -233,7 +233,7 @@ func TestAPICreateReleaseGivenInvalidTarget(t *testing.T) { MakeRequest(t, req, http.StatusNotFound) } -func TestAPIGetLatestRelease(t *testing.T) { +func TestAPIReleaseGetLatest(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -248,7 +248,7 @@ func TestAPIGetLatestRelease(t *testing.T) { assert.Equal(t, "testing-release", release.Title) } -func TestAPIGetReleaseByTag(t *testing.T) { +func TestAPIReleaseGetByTag(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -268,13 +268,29 @@ func TestAPIGetReleaseByTag(t *testing.T) { req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/releases/tags/%s", owner.Name, repo.Name, nonexistingtag)) resp = MakeRequest(t, req, http.StatusNotFound) - var err *api.APIError DecodeJSON(t, resp, &err) assert.NotEmpty(t, err.Message) } -func TestAPIDeleteReleaseByTagName(t *testing.T) { +func TestAPIReleaseGetDraftByTag(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + rel := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ + RepoID: repo.ID, + TagName: "draft-release", + }) + assert.True(t, rel.IsDraft) + + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/releases/tags/%s", repo.OwnerName, repo.Name, rel.TagName)) + resp := MakeRequest(t, req, http.StatusNotFound) + var err *api.APIError + DecodeJSON(t, resp, &err) + assert.NotEmpty(t, err.Message) +} + +func TestAPIReleaseDeleteByTagName(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -300,7 +316,7 @@ func TestAPIDeleteReleaseByTagName(t *testing.T) { _ = MakeRequest(t, req, http.StatusNoContent) } -func TestAPIUploadAssetRelease(t *testing.T) { +func TestAPIReleaseUploadAsset(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -371,7 +387,7 @@ func TestAPIUploadAssetRelease(t *testing.T) { }) } -func TestAPIGetReleaseArchiveDownloadCount(t *testing.T) { +func TestAPIReleaseGetArchiveDownloadCount(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -407,7 +423,7 @@ func TestAPIGetReleaseArchiveDownloadCount(t *testing.T) { assert.Equal(t, int64(0), release.ArchiveDownloadCount.Zip) } -func TestAPIExternalAssetRelease(t *testing.T) { +func TestAPIReleaseExternalAsset(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -424,13 +440,13 @@ func TestAPIExternalAssetRelease(t *testing.T) { var attachment *api.Attachment DecodeJSON(t, resp, &attachment) - assert.EqualValues(t, "test-asset", attachment.Name) + assert.Equal(t, "test-asset", attachment.Name) assert.EqualValues(t, 0, attachment.Size) - assert.EqualValues(t, "https://forgejo.org/", attachment.DownloadURL) - assert.EqualValues(t, "external", attachment.Type) + assert.Equal(t, "https://forgejo.org/", attachment.DownloadURL) + assert.Equal(t, "external", attachment.Type) } -func TestAPIDuplicateAssetRelease(t *testing.T) { +func TestAPIReleaseDuplicateAsset(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -458,7 +474,7 @@ func TestAPIDuplicateAssetRelease(t *testing.T) { MakeRequest(t, req, http.StatusBadRequest) } -func TestAPIMissingAssetRelease(t *testing.T) { +func TestAPIReleaseMissingAsset(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) diff --git a/tests/integration/api_repo_tags_test.go b/tests/integration/api_repo_tags_test.go index 81475249a1..fe36706933 100644 --- a/tests/integration/api_repo_tags_test.go +++ b/tests/integration/api_repo_tags_test.go @@ -16,6 +16,7 @@ import ( "forgejo.org/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIRepoTags(t *testing.T) { @@ -121,3 +122,55 @@ func TestAPIGetTagArchiveDownloadCount(t *testing.T) { assert.Equal(t, int64(1), tagInfo.ArchiveDownloadCount.TarGz) assert.Equal(t, int64(0), tagInfo.ArchiveDownloadCount.Zip) } + +func TestAPIRepoTagDeleteProtection(t *testing.T) { + defer tests.PrepareTestEnv(t)() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + // Login as User2. + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + repoName := "repo1" + + req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/tags", user.Name, repoName). + AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + var tags []*api.Tag + DecodeJSON(t, resp, &tags) + require.Len(t, tags, 1) + require.Equal(t, "v1.1", tags[0].Name) + + // Create a tag protection rule for the repo so that `user2` cannot create/remove tags, even if they have write + // perms to the repo... which they do becase they own it. + req = NewRequestWithJSON(t, "POST", + fmt.Sprintf("/api/v1/repos/%s/%s/tag_protections", user.Name, repoName), + &api.CreateTagProtectionOption{ + NamePattern: "v*", + WhitelistUsernames: []string{"user1"}, + }). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusCreated) + var tagProtection api.TagProtection + DecodeJSON(t, resp, &tagProtection) + require.Equal(t, "v*", tagProtection.NamePattern) + + // Delete the release associated with v1.1, so that it's possible to delete the tag. + delReq := NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/releases/tags/%s", user.Name, repoName, tags[0].Name). + AddTokenAuth(token) + MakeRequest(t, delReq, http.StatusNoContent) + + // Attempt to delete the tag, which should be denied by the tag protection rule. + delReq = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/tags/%s", user.Name, repoName, tags[0].Name). + AddTokenAuth(token) + MakeRequest(t, delReq, http.StatusUnprocessableEntity) + + // Remove the tag protection rule. + delReq = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/tag_protections/%d", user.Name, repoName, tagProtection.ID). + AddTokenAuth(token) + MakeRequest(t, delReq, http.StatusNoContent) + + // Attempt to delete the tag again, which should now be permitted. + delReq = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/tags/%s", user.Name, repoName, tags[0].Name). + AddTokenAuth(token) + MakeRequest(t, delReq, http.StatusNoContent) +} diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index e1fec8c77e..f6a7173b48 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -87,6 +87,49 @@ func TestAPIPullUpdateByRebase(t *testing.T) { }) } +func TestAPIPullUpdateBranchProtection(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + baseRepoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) + pr := createOutdatedPR(t, user, org26, baseRepoOwner) + + // Allow edits from maintainers on the PR + pr.AllowMaintainerEdit = true + err := issues_model.UpdateAllowEdits(t.Context(), pr) + require.NoError(t, err) + + session := loginUser(t, user.LoginName) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + // Set up a branch protection rule on the *head* branch such that it cannot be pushed to, which should block + // updating the PR. + pr.LoadBaseRepo(t.Context()) + pr.LoadHeadRepo(t.Context()) + req := NewRequestWithJSON(t, "POST", + fmt.Sprintf("/api/v1/repos/%s/%s/branch_protections", pr.HeadRepo.OwnerName, pr.HeadRepo.Name), + &api.BranchProtection{ + BranchName: "*", + RuleName: "*", + EnablePush: true, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + // `session/token` is from the owner of the head branch, and should be allowed to do the update: + req = NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update", pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Issue.Index). + AddTokenAuth(token) + session.MakeRequest(t, req, http.StatusOK) + + // Switch over to the base repo owner. Even though this PR is set to allow edits by maintainers, they shouldn't + // be allowed to update the PR because the head branch is protected by a branch protection rule. + session = loginUser(t, baseRepoOwner.LoginName) + token = getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + req = NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update", pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Issue.Index). + AddTokenAuth(token) + session.MakeRequest(t, req, http.StatusForbidden) + }) +} + func TestAPIViewUpdateSettings(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { defer tests.PrepareTestEnv(t)() @@ -183,8 +226,13 @@ func assertViewPullUpdate(t *testing.T, pr *issues_model.PullRequest, session *T } } -func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_model.PullRequest { - baseRepo, _, _ := tests.CreateDeclarativeRepo(t, actor, "repo-pr-update", nil, nil, nil) +func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User, baseRepoOwnerOption ...*user_model.User) *issues_model.PullRequest { + baseRepoOwner := actor + if len(baseRepoOwnerOption) == 1 { + baseRepoOwner = baseRepoOwnerOption[0] + } + + baseRepo, _, _ := tests.CreateDeclarativeRepo(t, baseRepoOwner, "repo-pr-update", nil, nil, nil) headRepo, err := repo_service.ForkRepositoryAndUpdates(git.DefaultContext, actor, forkOrg, repo_service.ForkRepoOptions{ BaseRepo: baseRepo, @@ -195,7 +243,7 @@ func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_mod assert.NotEmpty(t, headRepo) // create a commit on base Repo - _, err = files_service.ChangeRepoFiles(git.DefaultContext, baseRepo, actor, &files_service.ChangeRepoFilesOptions{ + _, err = files_service.ChangeRepoFiles(git.DefaultContext, baseRepo, baseRepoOwner, &files_service.ChangeRepoFilesOptions{ Files: []*files_service.ChangeRepoFile{ { Operation: "create", diff --git a/tests/integration/release_test.go b/tests/integration/release_test.go index b1162780ab..cf7f400fd9 100644 --- a/tests/integration/release_test.go +++ b/tests/integration/release_test.go @@ -113,16 +113,34 @@ func TestDeleteRelease(t *testing.T) { release := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{TagName: "v2.0"}) assert.False(t, release.IsTag) - // Using the ID of a comment that does not belong to the repository must fail - session5 := loginUser(t, "user5") + session := loginUser(t, "user2") // owner user session + session5 := loginUser(t, "user5") // different user session; using the ID of a release that does not belong to the repository must fail + anonSession := emptyTestSession(t) // anonymous session otherRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user5", LowerName: "repo4"}) + // can't delete a release by ID from the wrong repository context (otherRepo) req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", otherRepo.Link(), release.ID), map[string]string{ "_csrf": GetCSRF(t, session5, otherRepo.Link()), }) session5.MakeRequest(t, req, http.StatusNotFound) - session := loginUser(t, "user2") + // can't delete a release that the current user isn't a writer for + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", repo.Link(), release.ID), map[string]string{ + "_csrf": GetCSRF(t, session5, repo.Link()), + }) + session5.MakeRequest(t, req, http.StatusNotFound) + + // can't delete a release while anonymous + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", repo.Link(), release.ID), map[string]string{}) + anonSession.MakeRequest(t, req, http.StatusBadRequest) // no CSRF token + + // can't delete a release by ID from the wrong repository context (otherRepo) as the correct user + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", otherRepo.Link(), release.ID), map[string]string{ + "_csrf": GetCSRF(t, session, repo.Link()), + }) + session.MakeRequest(t, req, http.StatusNotFound) + + // but when everything aligns, we can delete the release req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", repo.Link(), release.ID), map[string]string{ "_csrf": GetCSRF(t, session, repo.Link()), }) @@ -130,13 +148,31 @@ func TestDeleteRelease(t *testing.T) { release = unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: release.ID}) if assert.True(t, release.IsTag) { + // can't delete a release by ID from the wrong repository context (otherRepo) req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", otherRepo.Link(), release.ID), map[string]string{ "_csrf": GetCSRF(t, session5, otherRepo.Link()), }) session5.MakeRequest(t, req, http.StatusNotFound) + // can't delete a release that the current user isn't a writer for req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", repo.Link(), release.ID), map[string]string{ - "_csrf": GetCSRF(t, session, repo.Link()), + "_csrf": GetCSRF(t, session5, otherRepo.Link()), + }) + session5.MakeRequest(t, req, http.StatusNotFound) + + // can't delete a release while anonymous + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", repo.Link(), release.ID), map[string]string{}) + anonSession.MakeRequest(t, req, http.StatusBadRequest) // no CSRF token + + // can't delete a release by ID from the wrong repository context (otherRepo) as the correct user + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", otherRepo.Link(), release.ID), map[string]string{ + "_csrf": GetCSRF(t, session, otherRepo.Link()), + }) + session.MakeRequest(t, req, http.StatusNotFound) + + // but when everything aligns, we can delete the tag + req = NewRequestWithValues(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", repo.Link(), release.ID), map[string]string{ + "_csrf": GetCSRF(t, session, otherRepo.Link()), }) session.MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/repo_tag_test.go b/tests/integration/repo_tag_test.go index 9be33ec8a8..56f09d26a6 100644 --- a/tests/integration/repo_tag_test.go +++ b/tests/integration/repo_tag_test.go @@ -191,7 +191,7 @@ func TestRepushTag(t *testing.T) { _, _, err = git.NewCommand(git.DefaultContext, "push", "origin", "--delete", "v2.0").RunStdString(&git.RunOpts{Dir: dstPath}) require.NoError(t, err) // query the release by API and it should be a draft - req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/releases/tags/%s", owner.Name, repo.Name, "v2.0")) + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/releases/tags/%s", owner.Name, repo.Name, "v2.0")).AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) var respRelease *api.Release DecodeJSON(t, resp, &respRelease) diff --git a/tests/test_utils.go b/tests/test_utils.go index b338ddc052..e44dcf5882 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -345,6 +345,7 @@ type DeclarativeRepoOptions struct { WikiBranch optional.Option[string] AutoInit optional.Option[bool] IsTemplate optional.Option[bool] + IsPrivate optional.Option[bool] } func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts DeclarativeRepoOptions) (*repo_model.Repository, string, func()) { @@ -376,6 +377,7 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts Readme: "Default", DefaultBranch: "main", IsTemplate: opts.IsTemplate.Value(), + IsPrivate: opts.IsPrivate.Value(), }) require.NoError(t, err) assert.NotEmpty(t, repo)