fix: add missing checks to several handlers and models (#39839) (#39847)

Backport https://github.com/go-gitea/gitea/pull/39839 by @silverwind

Several handlers and model helpers skipped checks that their siblings
already perform.

- Only match principal keys when looking up SSH certificate principals
- Return no attachments for an unsaved issue instead of every issue-less
one
- Reject issue and comment attachments when adding release attachments
- Share one permission check for editing, deleting and reacting to issue
content and for comment attachments between the web UI and the API,
refusing them on locked issues for users who can't write
- Reject issue edits, deletion and label changes in archived
repositories via the API
- Apply cross-origin protection to the repository topics endpoint
- Return 404 for the repositories tab of individual users and respect
profile visibility for the heatmap

* Fixes: https://github.com/go-gitea/gitea/issues/39226

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: bircni <bircni@icloud.com>

models/asymkey/ssh_key.go

@@ -199,7 +199,7 @@ func SearchPrincipalKey(ctx context.Context, principalKey string) (*PublicKey, e
 	// In the future, the existing principal keys should be migrated to use the fingerprint ("principal:{name}") instead of the content
 	key := new(PublicKey)
 	has, err := db.GetEngine(ctx).
-		Where("content = ?", principalKey).
+		Where("content = ? AND type = ?", principalKey, KeyTypePrincipal).
 		Get(key)
 	if err != nil {
 		return nil, err

models/asymkey/ssh_key_test.go

@@ -12,11 +12,13 @@ import (
 	"strings"
 	"testing"
 
+	"gitea.dev/models/db"
 	"gitea.dev/models/unittest"
 	"gitea.dev/modules/setting"
 
 	"github.com/42wim/sshsig"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_SSHParsePublicKey(t *testing.T) {
@@ -481,6 +483,20 @@ func TestPublicKeysAreExternallyManaged(t *testing.T) {
 	assert.False(t, externals[0])
 }
 
+func TestSearchPrincipalKeyIgnoresOtherKeyTypes(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+	userKey := unittest.AssertExistsAndLoadBean(t, &PublicKey{ID: 1})
+
+	_, err := SearchPrincipalKey(t.Context(), userKey.Content)
+	assert.True(t, IsErrKeyNotExist(err))
+
+	principal := &PublicKey{OwnerID: 1, Name: userKey.Content, Content: userKey.Content, Type: KeyTypePrincipal}
+	require.NoError(t, db.Insert(t.Context(), principal))
+	key, err := SearchPrincipalKey(t.Context(), userKey.Content)
+	require.NoError(t, err)
+	assert.Equal(t, principal.ID, key.ID)
+}
+
 // TestCheckPublicKeyStringOversized tests if oversized SSH2 public key strings are rejected before triggering costly operations.
 func TestCheckPublicKeyStringOversized(t *testing.T) {
 	_, err := parseKeyString(strings.Repeat("a", maxKeyContentBytes+1))

models/repo/attachment.go

@@ -133,6 +133,9 @@ func ExistAttachmentsByUUID(ctx context.Context, uuid string) (bool, error) {
 
 // GetAttachmentsByIssueID returns all attachments of an issue.
 func GetAttachmentsByIssueID(ctx context.Context, issueID int64) ([]*Attachment, error) {
+	if issueID == 0 { // unsaved issue, would match every issue-less attachment
+		return []*Attachment{}, nil
+	}
 	attachments := make([]*Attachment, 0, 10)
 	return attachments, db.GetEngine(ctx).Where("issue_id = ? AND comment_id = 0", issueID).Find(&attachments)
 }

models/repo/attachment_test.go

@@ -36,6 +36,10 @@ func TestGetByCommentOrIssueID(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Len(t, attachments, 1)
 
+	attachments, err = repo_model.GetAttachmentsByIssueID(t.Context(), 0)
+	assert.NoError(t, err)
+	assert.Empty(t, attachments)
+
 	attachments, err = repo_model.GetAttachmentsByCommentID(t.Context(), 1)
 	assert.NoError(t, err)
 	assert.Len(t, attachments, 2)

models/repo/release.go

@@ -191,6 +191,10 @@ func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs
 	}
 
 	for i := range attachments {
+		if attachments[i].IssueID != 0 || attachments[i].CommentID != 0 || attachments[i].ReleaseID != 0 {
+			return util.NewPermissionDeniedErrorf("attachment is already linked")
+		}
+
 		if attachments[i].RepoID == 0 && attachments[i].CreatedUnix < LegacyAttachmentMissingRepoIDCutoff {
 			attachments[i].RepoID = rel.RepoID
 			if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Cols("repo_id").Update(attachments[i]); err != nil {
@@ -201,10 +205,6 @@ func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs
 		if attachments[i].RepoID != rel.RepoID {
 			return util.NewPermissionDeniedErrorf("attachment belongs to different repository")
 		}
-
-		if attachments[i].ReleaseID != 0 {
-			return util.NewPermissionDeniedErrorf("release permission denied")
-		}
 		attachments[i].ReleaseID = releaseID
 		// No assign value could be 0, so ignore AllCols().
 		if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Cols("release_id").Update(attachments[i]); err != nil {

models/repo/release_test.go

@@ -53,6 +53,18 @@ func TestAddReleaseAttachmentsRejectsDifferentRepo(t *testing.T) {
 	assert.Zero(t, attach.ReleaseID, "attachment should not be linked to release on failure")
 }
 
+func TestAddReleaseAttachmentsRejectsIssueAttachment(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	uuid := "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11"
+	err := AddReleaseAttachments(t.Context(), 1, []string{uuid})
+	assert.ErrorIs(t, err, util.ErrPermissionDenied)
+
+	attach, err := GetAttachmentByUUID(t.Context(), uuid)
+	assert.NoError(t, err)
+	assert.Zero(t, attach.ReleaseID)
+}
+
 func TestAddReleaseAttachmentsAllowsLegacyMissingRepoID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 

routers/api/v1/api.go

@@ -1656,8 +1656,8 @@ func Routes() *web.Router {
 					})
 					m.Group("/{index}", func() {
 						m.Combo("").Get(repo.GetIssue).
-							Patch(reqToken(), bind(api.EditIssueOption{}), repo.EditIssue).
+							Patch(reqToken(), mustNotBeArchived, bind(api.EditIssueOption{}), repo.EditIssue).
-							Delete(reqToken(), reqAdmin(), context.ReferencesGitRepo(), repo.DeleteIssue)
+							Delete(reqToken(), reqAdmin(), mustNotBeArchived, context.ReferencesGitRepo(), repo.DeleteIssue)
 						m.Combo("/assignees").
 							Post(reqToken(), mustNotBeArchived, bind(api.IssueAssigneesOption{}), repo.AddIssueAssignees).
 							Delete(reqToken(), mustNotBeArchived, bind(api.IssueAssigneesOption{}), repo.DeleteIssueAssignees)
@@ -1671,10 +1671,10 @@ func Routes() *web.Router {
 						m.Get("/timeline", repo.ListIssueCommentsAndTimeline)
 						m.Group("/labels", func() {
 							m.Combo("").Get(repo.ListIssueLabels).
-								Post(reqToken(), bind(api.IssueLabelsOption{}), repo.AddIssueLabels).
+								Post(reqToken(), mustNotBeArchived, bind(api.IssueLabelsOption{}), repo.AddIssueLabels).
-								Put(reqToken(), bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels).
+								Put(reqToken(), mustNotBeArchived, bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels).
-								Delete(reqToken(), repo.ClearIssueLabels)
+								Delete(reqToken(), mustNotBeArchived, repo.ClearIssueLabels)
-							m.Delete("/{id}", reqToken(), repo.DeleteIssueLabel)
+							m.Delete("/{id}", reqToken(), mustNotBeArchived, repo.DeleteIssueLabel)
 						})
 						m.Group("/times", func() {
 							m.Combo("").

routers/api/v1/repo/issue.go

@@ -694,6 +694,8 @@ func EditIssue(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 	//   "412":
 	//     "$ref": "#/responses/error"
+	//   "423":
+	//     "$ref": "#/responses/repoArchivedError"
 
 	form := web.GetForm[*api.EditIssueOption](ctx)
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
@@ -897,6 +899,8 @@ func DeleteIssue(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "423":
+	//     "$ref": "#/responses/repoArchivedError"
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
 	if err != nil {
 		ctx.APIErrorAuto(err)

routers/api/v1/repo/issue_comment.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	access_model "gitea.dev/models/perm/access"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unit"
@@ -571,8 +572,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
 		ctx.APIErrorAuto(err)
 		return
 	}
-
+	if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, comment.Issue, comment.PosterID, perm.AccessModeWrite) {
-	if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
 		ctx.Status(http.StatusForbidden)
 		return
 	}
@@ -676,11 +676,11 @@ func deleteIssueComment(ctx *context.APIContext) {
 		ctx.APIErrorAuto(err)
 		return
 	}
-
+	if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, comment.Issue, comment.PosterID, perm.AccessModeWrite) {
-	if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
 		ctx.Status(http.StatusForbidden)
 		return
-	} else if !comment.Type.HasContentSupport() {
+	}
+	if !comment.Type.HasContentSupport() {
 		ctx.Status(http.StatusBadRequest)
 		return
 	}

routers/api/v1/repo/issue_comment_attachment.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/log"
@@ -344,21 +345,13 @@ func DeleteIssueCommentAttachment(ctx *context.APIContext) {
 }
 
 func getIssueCommentSafe(ctx *context.APIContext) *issues_model.Comment {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.PathParamInt64("id"))
+	comment, err := issues_model.GetCommentWithRepoID(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("id"))
 	if err != nil {
 		ctx.APIErrorAuto(err)
 		return nil
 	}
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.APIErrorInternal(err)
-		return nil
-	}
-	if comment.Issue == nil || comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.APIError(http.StatusNotFound, "no matching issue comment found")
-		return nil
-	}
-
 	if !ctx.Repo.Permission.CanReadIssuesOrPulls(comment.Issue.IsPull) {
+		ctx.APIErrorNotFound()
 		return nil
 	}
 
@@ -379,7 +372,7 @@ func getIssueCommentAttachmentSafeWrite(ctx *context.APIContext) *repo_model.Att
 }
 
 func canUserWriteIssueCommentAttachment(ctx *context.APIContext, comment *issues_model.Comment) bool {
-	canEditComment := ctx.IsSigned && (ctx.Doer.ID == comment.PosterID || ctx.IsUserRepoAdmin() || ctx.IsUserSiteAdmin()) && ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull)
+	canEditComment := issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, comment.Issue, comment.PosterID, perm.AccessModeWrite)
 	if !canEditComment {
 		ctx.APIError(http.StatusForbidden, "user should have permission to edit comment")
 		return false

routers/api/v1/repo/issue_label.go

@@ -102,6 +102,8 @@ func AddIssueLabels(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "423":
+	//     "$ref": "#/responses/repoArchivedError"
 
 	form := web.GetForm[*api.IssueLabelsOption](ctx)
 	issue, labels, err := prepareForReplaceOrAdd(ctx, *form)
@@ -162,6 +164,8 @@ func DeleteIssueLabel(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 	//   "422":
 	//     "$ref": "#/responses/validationError"
+	//   "423":
+	//     "$ref": "#/responses/repoArchivedError"
 
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
 	if err != nil {
@@ -232,6 +236,8 @@ func ReplaceIssueLabels(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "423":
+	//     "$ref": "#/responses/repoArchivedError"
 	form := web.GetForm[*api.IssueLabelsOption](ctx)
 	issue, labels, err := prepareForReplaceOrAdd(ctx, *form)
 	if err != nil {
@@ -283,6 +289,8 @@ func ClearIssueLabels(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "423":
+	//     "$ref": "#/responses/repoArchivedError"
 
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
 	if err != nil {

routers/api/v1/repo/issue_reaction.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	user_model "gitea.dev/models/user"
 	api "gitea.dev/modules/structs"
 	"gitea.dev/modules/web"
@@ -184,28 +185,12 @@ func DeleteIssueCommentReaction(ctx *context.APIContext) {
 }
 
 func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOption, isCreateType bool) {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.PathParamInt64("id"))
+	comment, err := issues_model.GetCommentWithRepoID(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("id"))
 	if err != nil {
 		ctx.APIErrorAuto(err)
 		return
 	}
-
+	if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, comment.Issue, comment.PosterID, perm.AccessModeRead) {
-	if err = comment.LoadIssue(ctx); err != nil {
-		ctx.APIErrorInternal(err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.APIErrorNotFound()
-		return
-	}
-
-	if !ctx.Repo.Permission.CanReadIssuesOrPulls(comment.Issue.IsPull) {
-		ctx.APIErrorNotFound()
-		return
-	}
-
-	if comment.Issue.IsLocked && !ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull) {
 		ctx.APIError(http.StatusForbidden, "no permission to change reaction")
 		return
 	}
@@ -420,7 +405,7 @@ func changeIssueReaction(ctx *context.APIContext, form api.EditReactionOption, i
 		return
 	}
 
-	if issue.IsLocked && !ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull) {
+	if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, issue, issue.PosterID, perm.AccessModeRead) {
 		ctx.APIError(http.StatusForbidden, "no permission to change reaction")
 		return
 	}

routers/web/org/home.go

@@ -38,15 +38,20 @@ func Home(ctx *context.Context) {
 		return
 	}
 
-	home(ctx, false)
+	orgHome(ctx, false)
 }
 
 func Repositories(ctx *context.Context) {
-	home(ctx, true)
+	orgHome(ctx, true)
 }
 
-func home(ctx *context.Context, viewRepositories bool) {
+func orgHome(ctx *context.Context, viewRepositories bool) {
 	org := ctx.Org.Organization
+	if org == nil {
+		// some requests may reach here by "user" route patterns, e.g.: "/{owner}/-/repositories"
+		ctx.NotFound(nil)
+		return
+	}
 
 	ctx.Data["PageIsUserProfile"] = true
 	ctx.Data["Title"] = org.DisplayName()

routers/web/repo/issue.go

@@ -14,6 +14,7 @@ import (
 
 	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	access_model "gitea.dev/models/perm/access"
 	project_model "gitea.dev/models/project"
 	"gitea.dev/models/renderhelper"
@@ -186,8 +187,8 @@ func GetActionIssue(ctx *context.Context) *issues_model.Issue {
 		return nil
 	}
 	issue.Repo = ctx.Repo.Repository
-	checkIssueRights(ctx, issue)
+	if !ctx.Repo.Permission.CanReadIssuesOrPulls(issue.IsPull) {
-	if ctx.Written() {
+		ctx.NotFound(nil)
 		return nil
 	}
 	if err = issue.LoadAttributes(ctx); err != nil {
@@ -197,13 +198,6 @@ func GetActionIssue(ctx *context.Context) *issues_model.Issue {
 	return issue
 }
 
-func checkIssueRights(ctx *context.Context, issue *issues_model.Issue) {
-	if issue.IsPull && !ctx.Repo.Permission.CanRead(unit.TypePullRequests) ||
-		!issue.IsPull && !ctx.Repo.Permission.CanRead(unit.TypeIssues) {
-		ctx.NotFound(nil)
-	}
-}
-
 func getActionIssues(ctx *context.Context) issues_model.IssueList {
 	commaSeparatedIssueIDs := ctx.FormString("issue_ids")
 	if len(commaSeparatedIssueIDs) == 0 {
@@ -481,39 +475,26 @@ func UpdateIssueAssignee(ctx *context.Context) {
 	ctx.JSONOK()
 }
 
+func canSelfChangeOrAccessContent(ctx *context.Context, refIssue *issues_model.Issue, contentPosterID int64, accessMode perm.AccessMode) bool {
+	if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, refIssue, contentPosterID, accessMode) {
+		ctx.HTTPError(http.StatusForbidden)
+		return false
+	}
+	return true
+}
+
 // ChangeIssueReaction create a reaction for issue
 func ChangeIssueReaction(ctx *context.Context) {
 	form := web.GetForm[*forms.ReactionForm](ctx)
-	issue := GetActionIssue(ctx)
-	if ctx.Written() {
-		return
-	}
-
-	if !ctx.IsSigned || (ctx.Doer.ID != issue.PosterID && !ctx.Repo.Permission.CanReadIssuesOrPulls(issue.IsPull)) {
-		if log.IsTrace() {
-			if ctx.IsSigned {
-				issueType := "issues"
-				if issue.IsPull {
-					issueType = "pulls"
-				}
-				log.Trace("Permission Denied: User %-v not the Poster (ID: %d) and cannot read %s in Repo %-v.\n"+
-					"User in Repo has Permissions: %-+v",
-					ctx.Doer,
-					issue.PosterID,
-					issueType,
-					ctx.Repo.Repository,
-					ctx.Repo.Permission)
-			} else {
-				log.Trace("Permission Denied: Not logged in")
-			}
-		}
-
-		ctx.HTTPError(http.StatusForbidden)
-		return
-	}
-
 	if ctx.HasError() {
-		ctx.ServerError("ChangeIssueReaction", errors.New(ctx.GetErrMsg()))
+		ctx.HTTPError(http.StatusBadRequest, ctx.GetErrMsg())
+		return
+	}
+	issue := GetActionIssue(ctx)
+	if issue == nil {
+		return
+	}
+	if !canSelfChangeOrAccessContent(ctx, issue, issue.PosterID, perm.AccessModeRead) {
 		return
 	}
 

routers/web/repo/issue_comment.go

@@ -12,6 +12,7 @@ import (
 
 	git_model "gitea.dev/models/git"
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	"gitea.dev/models/renderhelper"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
@@ -173,31 +174,38 @@ func NewComment(ctx *context.Context) {
 	ctx.JSONRedirect(redirect)
 }
 
+func getRepoIssueComment(ctx *context.Context, commentID int64) *issues_model.Comment {
+	comment, err := issues_model.GetCommentWithRepoID(ctx, ctx.Repo.Repository.ID, commentID)
+	if err != nil {
+		ctx.ServerError("GetCommentWithRepoID", err)
+		return nil
+	}
+	if !ctx.Repo.Permission.CanReadIssuesOrPulls(comment.Issue.IsPull) {
+		ctx.NotFound(nil)
+		return nil
+	}
+	return comment
+}
+
+func getRepoIssueCommentForSelfWriteOrAccess(ctx *context.Context, commentID int64, accessMode perm.AccessMode) *issues_model.Comment {
+	comment := getRepoIssueComment(ctx, commentID)
+	if comment == nil {
+		return nil
+	}
+	if !canSelfChangeOrAccessContent(ctx, comment.Issue, comment.PosterID, accessMode) {
+		return nil
+	}
+	if !comment.Type.HasContentSupport() {
+		ctx.HTTPError(http.StatusBadRequest)
+		return nil
+	}
+	return comment
+}
+
 // UpdateCommentContent change comment of issue's content
 func UpdateCommentContent(ctx *context.Context) {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.PathParamInt64("id"))
+	comment := getRepoIssueCommentForSelfWriteOrAccess(ctx, ctx.PathParamInt64("id"), perm.AccessModeWrite)
-	if err != nil {
+	if comment == nil {
-		ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err)
-		return
-	}
-
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.NotFoundOrServerError("LoadIssue", issues_model.IsErrIssueNotExist, err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.NotFound(issues_model.ErrCommentNotExist{})
-		return
-	}
-
-	if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
-		ctx.HTTPError(http.StatusForbidden)
-		return
-	}
-
-	if !comment.Type.HasContentSupport() {
-		ctx.HTTPError(http.StatusNoContent)
 		return
 	}
 
@@ -213,7 +221,7 @@ func UpdateCommentContent(ctx *context.Context) {
 		oldContent := comment.Content
 		comment.Content = newContent
 
-		if err = issue_service.UpdateComment(ctx, comment, contentVersion, ctx.Doer, oldContent); err != nil {
+		if err := issue_service.UpdateComment(ctx, comment, contentVersion, ctx.Doer, oldContent); err != nil {
 			if errors.Is(err, user_model.ErrBlockedUser) {
 				ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user"))
 			} else if errors.Is(err, issues_model.ErrCommentAlreadyChanged) {
@@ -243,6 +251,7 @@ func UpdateCommentContent(ctx *context.Context) {
 		rctx := renderhelper.NewRenderContextRepoComment(ctx, ctx.Repo.Repository, renderhelper.RepoCommentOptions{
 			FootnoteContextID: strconv.FormatInt(comment.ID, 10),
 		})
+		var err error
 		renderedContent, err = markdown.RenderString(rctx, comment.Content)
 		if err != nil {
 			ctx.ServerError("RenderString", err)
@@ -259,31 +268,12 @@ func UpdateCommentContent(ctx *context.Context) {
 
 // DeleteComment delete comment of issue
 func DeleteComment(ctx *context.Context) {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.PathParamInt64("id"))
+	comment := getRepoIssueCommentForSelfWriteOrAccess(ctx, ctx.PathParamInt64("id"), perm.AccessModeWrite)
-	if err != nil {
+	if comment == nil {
-		ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err)
 		return
 	}
 
-	if err := comment.LoadIssue(ctx); err != nil {
+	if err := issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil {
-		ctx.NotFoundOrServerError("LoadIssue", issues_model.IsErrIssueNotExist, err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.NotFound(issues_model.ErrCommentNotExist{})
-		return
-	}
-
-	if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
-		ctx.HTTPError(http.StatusForbidden)
-		return
-	} else if !comment.Type.HasContentSupport() {
-		ctx.HTTPError(http.StatusNoContent)
-		return
-	}
-
-	if err = issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil {
 		ctx.ServerError("DeleteComment", err)
 		return
 	}
@@ -293,51 +283,12 @@ func DeleteComment(ctx *context.Context) {
 
 // ChangeCommentReaction create a reaction for comment
 func ChangeCommentReaction(ctx *context.Context) {
+	comment := getRepoIssueCommentForSelfWriteOrAccess(ctx, ctx.PathParamInt64("id"), perm.AccessModeRead)
+	if comment == nil {
+		return
+	}
+
 	form := web.GetForm[*forms.ReactionForm](ctx)
-	comment, err := issues_model.GetCommentByID(ctx, ctx.PathParamInt64("id"))
-	if err != nil {
-		ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err)
-		return
-	}
-
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.NotFoundOrServerError("LoadIssue", issues_model.IsErrIssueNotExist, err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.NotFound(issues_model.ErrCommentNotExist{})
-		return
-	}
-
-	if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.Permission.CanReadIssuesOrPulls(comment.Issue.IsPull)) {
-		if log.IsTrace() {
-			if ctx.IsSigned {
-				issueType := "issues"
-				if comment.Issue.IsPull {
-					issueType = "pulls"
-				}
-				log.Trace("Permission Denied: User %-v not the Poster (ID: %d) and cannot read %s in Repo %-v.\n"+
-					"User in Repo has Permissions: %-+v",
-					ctx.Doer,
-					comment.Issue.PosterID,
-					issueType,
-					ctx.Repo.Repository,
-					ctx.Repo.Permission)
-			} else {
-				log.Trace("Permission Denied: Not logged in")
-			}
-		}
-
-		ctx.HTTPError(http.StatusForbidden)
-		return
-	}
-
-	if !comment.Type.HasContentSupport() {
-		ctx.HTTPError(http.StatusNoContent)
-		return
-	}
-
 	switch ctx.PathParam("action") {
 	case "react":
 		reaction, err := issue_service.CreateCommentReaction(ctx, ctx.Doer, comment, form.Content)
@@ -365,7 +316,7 @@ func ChangeCommentReaction(ctx *context.Context) {
 
 		// Reload new reactions
 		comment.Reactions = nil
-		if err = comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil {
+		if err := comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil {
 			log.Info("comment.LoadReactions: %s", err)
 			break
 		}
@@ -399,24 +350,8 @@ func ChangeCommentReaction(ctx *context.Context) {
 
 // GetCommentAttachments returns attachments for the comment
 func GetCommentAttachments(ctx *context.Context) {
-	comment, err := issues_model.GetCommentByID(ctx, ctx.PathParamInt64("id"))
+	comment := getRepoIssueComment(ctx, ctx.PathParamInt64("id"))
-	if err != nil {
+	if comment == nil {
-		ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err)
-		return
-	}
-
-	if err := comment.LoadIssue(ctx); err != nil {
-		ctx.NotFoundOrServerError("LoadIssue", issues_model.IsErrIssueNotExist, err)
-		return
-	}
-
-	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
-		ctx.NotFound(issues_model.ErrCommentNotExist{})
-		return
-	}
-
-	if !ctx.Repo.Permission.CanReadIssuesOrPulls(comment.Issue.IsPull) {
-		ctx.NotFound(issues_model.ErrCommentNotExist{})
 		return
 	}
 

routers/web/repo/issue_watch.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 
 	issues_model "gitea.dev/models/issues"
-	"gitea.dev/modules/log"
 	"gitea.dev/modules/templates"
 	"gitea.dev/services/context"
 )
@@ -19,29 +18,7 @@ const (
 // IssueWatch sets issue watching
 func IssueWatch(ctx *context.Context) {
 	issue := GetActionIssue(ctx)
-	if ctx.Written() {
+	if issue == nil {
-		return
-	}
-
-	if !ctx.IsSigned || (ctx.Doer.ID != issue.PosterID && !ctx.Repo.Permission.CanReadIssuesOrPulls(issue.IsPull)) {
-		if log.IsTrace() {
-			if ctx.IsSigned {
-				issueType := "issues"
-				if issue.IsPull {
-					issueType = "pulls"
-				}
-				log.Trace("Permission Denied: User %-v not the Poster (ID: %d) and cannot read %s in Repo %-v.\n"+
-					"User in Repo has Permissions: %-+v",
-					ctx.Doer,
-					issue.PosterID,
-					issueType,
-					ctx.Repo.Repository,
-					ctx.Repo.Permission)
-			} else {
-				log.Trace("Permission Denied: Not logged in")
-			}
-		}
-		ctx.HTTPError(http.StatusForbidden)
 		return
 	}
 

routers/web/user/heatmap.go

@@ -8,6 +8,7 @@ import (
 	"net/url"
 
 	activities_model "gitea.dev/models/activities"
+	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/setting"
 	"gitea.dev/services/context"
 )
@@ -51,6 +52,12 @@ func DashboardHeatmap(ctx *context.Context) {
 		ctx.NotFound(nil)
 		return
 	}
+	if ctx.Org.Organization == nil && !user_model.IsUserVisibleToViewer(ctx, ctx.ContextUser, ctx.Doer) {
+		// this end point is mounted on different routes, so we need to
+		// check if the user is visible to the viewer when there is no organization context (no visibility check)
+		ctx.NotFound(nil)
+		return
+	}
 	var data []*activities_model.UserHeatmapData
 	var err error
 	if ctx.Org.Organization == nil {

routers/web/web.go

@@ -1520,7 +1520,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 
 	m.Group("/{username}/{reponame}", func() {
 		m.Post("/topics", repo.TopicsPost)
-	}, context.RepoAssignment, reqRepoAdmin, context.RepoMustNotBeArchived())
+	}, optSignIn, context.RepoAssignment, reqRepoAdmin, context.RepoMustNotBeArchived())
 
 	m.Group("/{username}/{reponame}", func() {
 		if setting.Packages.Enabled {

services/issue/issue.go

@@ -10,14 +10,17 @@ import (
 	activities_model "gitea.dev/models/activities"
 	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	access_model "gitea.dev/models/perm/access"
 	project_model "gitea.dev/models/project"
 	repo_model "gitea.dev/models/repo"
 	system_model "gitea.dev/models/system"
+	"gitea.dev/models/unit"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/storage"
+	"gitea.dev/modules/util"
 	notify_service "gitea.dev/services/notify"
 )
 
@@ -309,3 +312,23 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths []
 
 	return attachmentPaths, err
 }
+
+// CanSelfChangeOrAccessContent checks whether the doer can access the content:
+// * If the doer is the content poster, then the doer can access the content if they have read access to issues.
+// * If the doer is not the content poster, then the doer can access the content if they have the required access mode to the unit type (issues or pull requests).
+// * If the issue is locked, then the doer can only access the content if they have write access.
+func CanSelfChangeOrAccessContent(doer *user_model.User, doerPerm access_model.Permission, refIssue *issues_model.Issue, contentPosterID int64, accessMode perm.AccessMode) bool {
+	unitType := util.Iif(refIssue.IsPull, unit.TypePullRequests, unit.TypeIssues)
+	if doer == nil || !doerPerm.CanAccess(perm.AccessModeRead, unitType) {
+		return false
+	}
+	ok := doer.ID == contentPosterID || doerPerm.CanAccess(accessMode, unitType)
+	if !ok {
+		return false
+	}
+	// TODO: the UI can also be updated in the future to hide the "edit" menu
+	if refIssue.IsLocked && !doerPerm.CanWrite(unitType) {
+		return false
+	}
+	return true
+}

services/issue/issue_test.go

@@ -7,13 +7,56 @@ import (
 	"testing"
 
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
+	access_model "gitea.dev/models/perm/access"
 	repo_model "gitea.dev/models/repo"
+	"gitea.dev/models/unit"
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 
 	"github.com/stretchr/testify/assert"
 )
 
+func TestCanSelfChangeOrAccessContent(t *testing.T) {
+	doer := &user_model.User{ID: 1}
+	for _, u := range []struct {
+		name     string
+		unitType unit.Type
+	}{
+		{"issues", unit.TypeIssues},
+		{"pulls", unit.TypePullRequests},
+	} {
+		t.Run(u.name, func(t *testing.T) {
+			for _, tc := range []struct {
+				name         string
+				doer         *user_model.User
+				posterID     int64
+				mode         perm.AccessMode
+				requiredMode perm.AccessMode
+				locked       bool
+				allowed      bool
+			}{
+				{"anonymous", nil, 1, perm.AccessModeWrite, perm.AccessModeWrite, false, false},
+				{"author without read access", doer, 1, perm.AccessModeNone, perm.AccessModeWrite, false, false},
+				{"author with read access", doer, 1, perm.AccessModeRead, perm.AccessModeWrite, false, true},
+				{"reader cannot edit others", doer, 2, perm.AccessModeRead, perm.AccessModeWrite, false, false},
+				{"reader can access others", doer, 2, perm.AccessModeRead, perm.AccessModeRead, false, true},
+				{"writer can edit others", doer, 2, perm.AccessModeWrite, perm.AccessModeWrite, false, true},
+				{"locked author needs write access", doer, 1, perm.AccessModeRead, perm.AccessModeWrite, true, false},
+				{"locked reader needs write access", doer, 2, perm.AccessModeRead, perm.AccessModeRead, true, false},
+				{"locked writer can edit", doer, 2, perm.AccessModeWrite, perm.AccessModeWrite, true, true},
+			} {
+				t.Run(tc.name, func(t *testing.T) {
+					var permission access_model.Permission
+					permission.SetUnitsWithDefaultAccessMode([]*repo_model.RepoUnit{{Type: u.unitType}}, tc.mode)
+					issue := &issues_model.Issue{IsPull: u.unitType == unit.TypePullRequests, IsLocked: tc.locked}
+					assert.Equal(t, tc.allowed, CanSelfChangeOrAccessContent(tc.doer, permission, issue, tc.posterID, tc.requiredMode))
+				})
+			}
+		})
+	}
+}
+
 func TestGetRefEndNamesAndURLs(t *testing.T) {
 	issues := []*issues_model.Issue{
 		{ID: 1, Ref: "refs/heads/branch1"},

templates/swagger/v1-openapi3.generated.json

@@ -23759,6 +23759,9 @@
           },
           "404": {
             "$ref": "#/components/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/components/responses/repoArchivedError"
           }
         },
         "summary": "Delete an issue",
@@ -23866,6 +23869,9 @@
           },
           "412": {
             "$ref": "#/components/responses/error"
+          },
+          "423": {
+            "$ref": "#/components/responses/repoArchivedError"
           }
         },
         "summary": "Edit an issue. If using deadline only the date will be taken into account, and time of day ignored.",
@@ -25085,6 +25091,9 @@
           },
           "404": {
             "$ref": "#/components/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/components/responses/repoArchivedError"
           }
         },
         "summary": "Remove all labels from an issue",
@@ -25188,6 +25197,9 @@
           },
           "404": {
             "$ref": "#/components/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/components/responses/repoArchivedError"
           }
         },
         "summary": "Add a label to an issue",
@@ -25246,6 +25258,9 @@
           },
           "404": {
             "$ref": "#/components/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/components/responses/repoArchivedError"
           }
         },
         "summary": "Replace an issue's labels",
@@ -25309,6 +25324,9 @@
           },
           "422": {
             "$ref": "#/components/responses/validationError"
+          },
+          "423": {
+            "$ref": "#/components/responses/repoArchivedError"
           }
         },
         "summary": "Remove a label from an issue",

templates/swagger/v1-swagger.generated.json

@@ -11870,6 +11870,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/responses/repoArchivedError"
           }
         }
       },
@@ -11929,6 +11932,9 @@
           },
           "412": {
             "$ref": "#/responses/error"
+          },
+          "423": {
+            "$ref": "#/responses/repoArchivedError"
           }
         }
       }
@@ -13089,6 +13095,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/responses/repoArchivedError"
           }
         }
       },
@@ -13144,6 +13153,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/responses/repoArchivedError"
           }
         }
       },
@@ -13189,6 +13201,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "423": {
+            "$ref": "#/responses/repoArchivedError"
           }
         }
       }
@@ -13247,6 +13262,9 @@
           },
           "422": {
             "$ref": "#/responses/validationError"
+          },
+          "423": {
+            "$ref": "#/responses/repoArchivedError"
           }
         }
       }

tests/integration/api_issue_test.go

@@ -260,6 +260,11 @@ func testAPIEditIssue(t *testing.T) {
 	assert.Equal(t, int64(0), int64(issueAfter.DeadlineUnix))
 	assert.Equal(t, body, issueAfter.Content)
 	assert.Equal(t, title, issueAfter.Title)
+
+	token = getUserToken(t, "user30", auth_model.AccessTokenScopeWriteIssue)
+	req = NewRequestWithJSON(t, "PATCH", "/api/v1/repos/user30/repo51/issues/1", api.EditIssueOption{Title: title}).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusLocked)
+	unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 14, Title: "issue in archived repo"})
 }
 
 func testAPISearchIssues(t *testing.T) {

tests/integration/heatmap_test.go

@@ -34,6 +34,12 @@ func TestHeatmapEndpoints(t *testing.T) {
 		assert.Positive(t, result["totalContributions"])
 	})
 
+	t.Run("PrivateUserProfileAnonymous", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+		req := NewRequest(t, "GET", "/user31/-/heatmap")
+		MakeRequest(t, req, http.StatusNotFound)
+	})
+
 	t.Run("OrgDashboard", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
 		req := NewRequest(t, "GET", "/org/org3/dashboard/-/heatmap")

tests/integration/issue_test.go

@@ -344,6 +344,15 @@ func TestIssueReaction(t *testing.T) {
 		"content": "eyes",
 	})
 	session.MakeRequest(t, req, http.StatusOK)
+
+	assert.NoError(t, issues_model.UpdateIssueCols(t.Context(), &issues_model.Issue{ID: 1, IsLocked: true}, "is_locked"))
+	readerSession := loginUser(t, "user4")
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/issues/1/reactions/react", map[string]string{"content": "eyes"})
+	readerSession.MakeRequest(t, req, http.StatusForbidden)
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/comments/2/reactions/react", map[string]string{"content": "eyes"})
+	readerSession.MakeRequest(t, req, http.StatusForbidden)
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/comments/2/reactions/react", map[string]string{"content": "eyes"})
+	session.MakeRequest(t, req, http.StatusOK)
 }
 
 func TestIssueCrossReference(t *testing.T) {

tests/integration/repo_topic_test.go

@@ -14,6 +14,19 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func TestRepoTopicsPostRejectsCrossOrigin(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	session := loginUser(t, "user2")
+
+	req := NewRequestWithValues(t, "POST", "/user2/repo1/topics", map[string]string{"topics": "cross"}).
+		SetHeader("Sec-Fetch-Site", "cross-site")
+	session.MakeRequest(t, req, http.StatusForbidden)
+
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/topics", map[string]string{"topics": "same"}).
+		SetHeader("Sec-Fetch-Site", "same-origin")
+	session.MakeRequest(t, req, http.StatusOK)
+}
+
 func TestTopicSearch(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	searchURL, _ := url.Parse("/explore/topics/search")

tests/integration/user_test.go

@@ -41,6 +41,9 @@ func testViewUser(t *testing.T) {
 	req := NewRequest(t, "GET", "/user2")
 	MakeRequest(t, req, http.StatusOK)
 
+	req = NewRequest(t, "GET", "/user2/-/repositories")
+	MakeRequest(t, req, http.StatusNotFound)
+
 	req = NewRequest(t, "GET", "/user2.keys")
 	resp := MakeRequest(t, req, http.StatusOK)
 	assert.Equal(t, `# Gitea isn't a key server. The keys are exported as the user uploaded and might not have been fully verified.

web_src/js/features/comp/ReactionSelector.ts

@@ -20,6 +20,7 @@ export function initCompReactionSelector() {
     const res = await POST(`${actionUrl}/${hasReacted ? 'unreact' : 'react'}`, {
       data: new URLSearchParams({content: reactionContent}),
     });
+    if (!res.ok) return;
 
     const data = await res.json();
     bottomReactions?.remove();