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/10037.md b/release-notes/10037.md new file mode 100644 index 0000000000..f0019ef55b --- /dev/null +++ b/release-notes/10037.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 ad4aa6530e..80b4faffa3 100644 --- a/routers/api/v1/repo/issue_dependency.go +++ b/routers/api/v1/repo/issue_dependency.go @@ -206,7 +206,7 @@ func CreateIssueDependency(ctx *context.APIContext) { return } - dependencyPerm := getPermissionForRepo(ctx, target.Repo) + dependencyPerm := getPermissionForRepo(ctx, dependency.Repo) if ctx.Written() { return } @@ -268,7 +268,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 0bf4485c17..ce2d0e0646 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 fd363855d6..c7b83cc3ef 100644 --- a/routers/web/repo/issue_content_history.go +++ b/routers/web/repo/issue_content_history.go @@ -210,12 +210,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 d638b338fc..be0c936ef3 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1335,8 +1335,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 563c11fff3..91c9d14790 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -136,7 +136,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 90eb1320ed..69889a1638 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 797d315f27..df04a3a15b 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -813,3 +813,48 @@ func TestAPIInternalAndExternalIssueTracker(t *testing.T) { runTest(t, externalIssueRepo, false) runTest(t, disabledIssueRepo, false) } + +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 41a539aa35..133a290284 100644 --- a/tests/integration/api_releases_test.go +++ b/tests/integration/api_releases_test.go @@ -24,7 +24,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}) @@ -102,7 +102,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}) @@ -159,7 +159,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}) @@ -184,7 +184,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}) @@ -195,7 +195,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}) @@ -213,7 +213,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}) @@ -231,7 +231,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}) @@ -246,7 +246,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}) @@ -266,13 +266,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}) @@ -298,7 +314,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}) @@ -362,7 +378,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}) @@ -398,7 +414,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}) @@ -421,7 +437,7 @@ func TestAPIExternalAssetRelease(t *testing.T) { assert.Equal(t, "external", attachment.Type) } -func TestAPIAllowedAPIURLInRelease(t *testing.T) { +func TestAPIReleaseAllowedAPIURL(t *testing.T) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -445,7 +461,7 @@ func TestAPIAllowedAPIURLInRelease(t *testing.T) { 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}) @@ -466,7 +482,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 6898a5b966..374777f48c 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) { @@ -154,3 +155,55 @@ func TestAPIGetTagsPaginated(t *testing.T) { assert.Contains(t, link, "rel=\"next\"") assert.Contains(t, link, "page=2") } + +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 ad117355e5..42f9e841e1 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) { + onApplicationRun(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) { onApplicationRun(t, func(t *testing.T, giteaURL *url.URL) { // Create PR to test @@ -181,8 +224,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, @@ -193,7 +241,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 cb3aaf233a..5c9fc0bbbe 100644 --- a/tests/integration/release_test.go +++ b/tests/integration/release_test.go @@ -110,22 +110,50 @@ 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 := NewRequest(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", otherRepo.Link(), release.ID)) 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 = NewRequest(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", repo.Link(), release.ID)) + session5.MakeRequest(t, req, http.StatusNotFound) + + // can't delete a release while anonymous + req = NewRequest(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", repo.Link(), release.ID)) + anonSession.MakeRequest(t, req, http.StatusSeeOther) // login redirect + + // can't delete a release by ID from the wrong repository context (otherRepo) as the correct user + req = NewRequest(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", otherRepo.Link(), release.ID)) + session.MakeRequest(t, req, http.StatusNotFound) + + // but when everything aligns, we can delete the release req = NewRequest(t, "POST", fmt.Sprintf("%s/releases/delete?id=%d", repo.Link(), release.ID)) session.MakeRequest(t, req, http.StatusOK) 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 = NewRequest(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", otherRepo.Link(), release.ID)) session5.MakeRequest(t, req, http.StatusNotFound) + // can't delete a release that the current user isn't a writer for + req = NewRequest(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", repo.Link(), release.ID)) + session5.MakeRequest(t, req, http.StatusNotFound) + + // can't delete a release while anonymous + req = NewRequest(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", repo.Link(), release.ID)) + anonSession.MakeRequest(t, req, http.StatusSeeOther) // login redirect + + // can't delete a release by ID from the wrong repository context (otherRepo) as the correct user + req = NewRequest(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", otherRepo.Link(), release.ID)) + session.MakeRequest(t, req, http.StatusNotFound) + + // but when everything aligns, we can delete the tag req = NewRequest(t, "POST", fmt.Sprintf("%s/tags/delete?id=%d", repo.Link(), release.ID)) session.MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/repo_tag_test.go b/tests/integration/repo_tag_test.go index 972a51089b..1773d9bde5 100644 --- a/tests/integration/repo_tag_test.go +++ b/tests/integration/repo_tag_test.go @@ -220,7 +220,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 7ef316d79a..3034375eb1 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -370,6 +370,7 @@ type DeclarativeRepoOptions struct { AutoInit optional.Option[bool] IsTemplate optional.Option[bool] ObjectFormat optional.Option[string] + IsPrivate optional.Option[bool] } func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts DeclarativeRepoOptions) (*repo_model.Repository, string, func()) { @@ -402,6 +403,7 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts DefaultBranch: "main", IsTemplate: opts.IsTemplate.Value(), ObjectFormatName: opts.ObjectFormat.Value(), + IsPrivate: opts.IsPrivate.Value(), }) require.NoError(t, err) assert.NotEmpty(t, repo)