Fix release draft access check logic (#36720)

1. remove hasRepoWriteScope to avoid abuse
2. clarify "ctx.Written" behavior
3. merge "read-only" tests to slightly improve performance

routers/api/v1/repo/release.go

@@ -10,7 +10,6 @@ import (
 
 	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
-	"code.gitea.io/gitea/models/perm"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/modules/git"
@@ -22,26 +21,19 @@ import (
 	release_service "code.gitea.io/gitea/services/release"
 )
 
-func hasRepoWriteScope(ctx *context.APIContext) bool {
+func canAccessReleaseDraft(ctx *context.APIContext) bool {
-	scope, ok := ctx.Data["ApiTokenScope"].(auth_model.AccessTokenScope)
-	if ctx.Data["IsApiToken"] != true || !ok {
-		return true
-	}
-
-	requiredScopes := auth_model.GetRequiredScopes(auth_model.Write, auth_model.AccessTokenScopeCategoryRepository)
-	allow, err := scope.HasScope(requiredScopes...)
-	if err != nil {
-		ctx.APIError(http.StatusForbidden, "checking scope failed: "+err.Error())
-		return false
-	}
-	return allow
-}
-
-func canAccessDraftRelease(ctx *context.APIContext) bool {
 	if !ctx.IsSigned || !ctx.Repo.CanWrite(unit.TypeReleases) {
 		return false
 	}
-	return hasRepoWriteScope(ctx)
+	if ctx.Data["IsApiToken"] != true {
+		// not API token request, the request is from a user session with write access
+		return true
+	}
+	// the request is from an access token with scope
+	scope := ctx.Data["ApiTokenScope"].(auth_model.AccessTokenScope)
+	requiredScopes := auth_model.GetRequiredScopes(auth_model.Write, auth_model.AccessTokenScopeCategoryRepository)
+	allow, _ := scope.HasScope(requiredScopes...) // err (invalid token) can be safely ignored
+	return allow
 }
 
 // GetRelease get a single release of a repository
@@ -85,13 +77,9 @@ func GetRelease(ctx *context.APIContext) {
 		return
 	}
 
-	if release.IsDraft { // only the users with write access can see draft releases
+	if release.IsDraft && !canAccessReleaseDraft(ctx) { // only the users with write access can see draft releases
-		if !canAccessDraftRelease(ctx) {
+		ctx.APIErrorNotFound()
-			if !ctx.Written() {
+		return
-				ctx.APIErrorNotFound()
-			}
-			return
-		}
 	}
 
 	if err := release.LoadAttributes(ctx); err != nil {
@@ -182,14 +170,12 @@ func ListReleases(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 	listOptions := utils.GetListOptions(ctx)
-
-	includeDrafts := (ctx.Repo.AccessMode >= perm.AccessModeWrite || ctx.Repo.UnitAccessMode(unit.TypeReleases) >= perm.AccessModeWrite) && hasRepoWriteScope(ctx)
 	if ctx.Written() {
 		return
 	}
 	opts := repo_model.FindReleasesOptions{
 		ListOptions:   listOptions,
-		IncludeDrafts: includeDrafts,
+		IncludeDrafts: canAccessReleaseDraft(ctx),
 		IncludeTags:   false,
 		IsDraft:       ctx.FormOptionalBool("draft"),
 		IsPreRelease:  ctx.FormOptionalBool("pre-release"),

routers/api/v1/repo/release_attachment.go

@@ -34,13 +34,9 @@ func checkReleaseMatchRepo(ctx *context.APIContext, releaseID int64) bool {
 		ctx.APIErrorNotFound()
 		return false
 	}
-	if release.IsDraft {
+	if release.IsDraft && !canAccessReleaseDraft(ctx) {
-		if !canAccessDraftRelease(ctx) {
+		ctx.APIErrorNotFound()
-			if !ctx.Written() {
+		return false
-				ctx.APIErrorNotFound()
-			}
-			return false
-		}
 	}
 	return true
 }
@@ -149,13 +145,9 @@ func ListReleaseAttachments(ctx *context.APIContext) {
 		ctx.APIErrorNotFound()
 		return
 	}
-	if release.IsDraft {
+	if release.IsDraft && !canAccessReleaseDraft(ctx) {
-		if !canAccessDraftRelease(ctx) {
+		ctx.APIErrorNotFound()
-			if !ctx.Written() {
+		return
-				ctx.APIErrorNotFound()
-			}
-			return
-		}
 	}
 	if err := release.LoadAttributes(ctx); err != nil {
 		ctx.APIErrorInternal(err)

tests/integration/api_releases_attachment_test.go

@@ -15,16 +15,13 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/test"
-	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func TestAPIEditReleaseAttachmentWithUnallowedFile(t *testing.T) {
+func testAPIEditReleaseAttachmentWithUnallowedFile(t *testing.T) {
 	// Limit the allowed release types (since by default there is no restriction)
 	defer test.MockVariableValue(&setting.Repository.Release.AllowedTypes, ".exe")()
-	defer tests.PrepareTestEnv(t)()
-
 	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 9})
 	release := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: attachment.ReleaseID})
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: attachment.RepoID})
@@ -42,9 +39,7 @@ func TestAPIEditReleaseAttachmentWithUnallowedFile(t *testing.T) {
 	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
 }
 
-func TestAPIDraftReleaseAttachmentAccess(t *testing.T) {
+func testAPIDraftReleaseAttachmentAccess(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
 	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 13})
 	release := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: attachment.ReleaseID})
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: attachment.RepoID})

tests/integration/api_releases_test.go

@@ -29,9 +29,19 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestAPIListReleasesWithWriteToken(t *testing.T) {
+func TestAPIReleaseRead(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
+	t.Run("DraftReleaseAttachmentAccess", testAPIDraftReleaseAttachmentAccess)
+	t.Run("ListReleasesWithWriteToken", testAPIListReleasesWithWriteToken)
+	t.Run("ListReleasesWithReadToken", testAPIListReleasesWithReadToken)
+	t.Run("GetDraftRelease", testAPIGetDraftRelease)
+	t.Run("GetLatestRelease", testAPIGetLatestRelease)
+	t.Run("GetReleaseByTag", testAPIGetReleaseByTag)
+	t.Run("GetDraftReleaseByTag", testAPIGetDraftReleaseByTag)
+	t.Run("EditReleaseAttachmentWithUnallowedFile", testAPIEditReleaseAttachmentWithUnallowedFile) // failed attempt, so it is also a read test
+}
 
+func testAPIListReleasesWithWriteToken(t *testing.T) {
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
 	token := getUserToken(t, user2.LowerName, auth_model.AccessTokenScopeWriteRepository)
@@ -81,9 +91,7 @@ func TestAPIListReleasesWithWriteToken(t *testing.T) {
 	testFilterByLen(true, url.Values{"draft": {"true"}, "pre-release": {"true"}}, 0, "there is no pre-release draft")
 }
 
-func TestAPIListReleasesWithReadToken(t *testing.T) {
+func testAPIListReleasesWithReadToken(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
 	token := getUserToken(t, user2.LowerName, auth_model.AccessTokenScopeReadRepository)
@@ -129,9 +137,7 @@ func TestAPIListReleasesWithReadToken(t *testing.T) {
 	testFilterByLen(true, url.Values{"draft": {"true"}, "pre-release": {"true"}}, 0, "there is no pre-release draft")
 }
 
-func TestAPIGetDraftRelease(t *testing.T) {
+func testAPIGetDraftRelease(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	release := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: 4})
 	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
@@ -300,9 +306,7 @@ func TestAPICreateReleaseGivenInvalidTarget(t *testing.T) {
 	MakeRequest(t, req, http.StatusNotFound)
 }
 
-func TestAPIGetLatestRelease(t *testing.T) {
+func testAPIGetLatestRelease(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
 
@@ -315,9 +319,7 @@ func TestAPIGetLatestRelease(t *testing.T) {
 	assert.Equal(t, "testing-release", release.Title)
 }
 
-func TestAPIGetReleaseByTag(t *testing.T) {
+func testAPIGetReleaseByTag(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
 
@@ -341,9 +343,7 @@ func TestAPIGetReleaseByTag(t *testing.T) {
 	assert.NotEmpty(t, err.Message)
 }
 
-func TestAPIGetDraftReleaseByTag(t *testing.T) {
+func testAPIGetDraftReleaseByTag(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})