From ae17c14cffe9d416a38a3bf85f92e5b35fcb9da6 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sun, 18 Oct 2026 15:29:49 +0200 Subject: [PATCH] fix: add missing checks to several handlers and models (#39839) 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 Co-authored-by: bircni --- models/asymkey/ssh_key.go | 2 +- models/asymkey/ssh_key_test.go | 16 ++ models/repo/attachment.go | 3 + models/repo/attachment_test.go | 4 + models/repo/release.go | 8 +- models/repo/release_test.go | 12 ++ routers/api/v1/api.go | 12 +- routers/api/v1/repo/issue.go | 4 + routers/api/v1/repo/issue_comment.go | 10 +- .../api/v1/repo/issue_comment_attachment.go | 15 +- routers/api/v1/repo/issue_label.go | 8 + routers/api/v1/repo/issue_reaction.go | 23 +-- routers/web/org/home.go | 11 +- routers/web/repo/issue.go | 57 +++---- routers/web/repo/issue_comment.go | 153 +++++------------- routers/web/repo/issue_watch.go | 25 +-- routers/web/user/heatmap.go | 7 + routers/web/web.go | 2 +- services/issue/issue.go | 23 +++ services/issue/issue_test.go | 43 +++++ templates/swagger/v1-openapi3.generated.json | 18 +++ templates/swagger/v1-swagger.generated.json | 18 +++ tests/integration/api_issue_test.go | 5 + tests/integration/heatmap_test.go | 6 + tests/integration/issue_test.go | 9 ++ tests/integration/repo_topic_test.go | 13 ++ tests/integration/user_test.go | 3 + web_src/js/features/comp/ReactionSelector.ts | 1 + 28 files changed, 290 insertions(+), 221 deletions(-) diff --git a/models/asymkey/ssh_key.go b/models/asymkey/ssh_key.go index be2484a5342..a84ba7736ca 100644 --- a/models/asymkey/ssh_key.go +++ b/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 diff --git a/models/asymkey/ssh_key_test.go b/models/asymkey/ssh_key_test.go index 8700be3c86f..a90bcad246a 100644 --- a/models/asymkey/ssh_key_test.go +++ b/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)) diff --git a/models/repo/attachment.go b/models/repo/attachment.go index a3adc473baa..09adcd4c9ea 100644 --- a/models/repo/attachment.go +++ b/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) } diff --git a/models/repo/attachment_test.go b/models/repo/attachment_test.go index 362092f8c46..e5b43f9e5ee 100644 --- a/models/repo/attachment_test.go +++ b/models/repo/attachment_test.go @@ -39,6 +39,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) diff --git a/models/repo/release.go b/models/repo/release.go index f97d51aac23..d262abf5710 100644 --- a/models/repo/release.go +++ b/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 { diff --git a/models/repo/release_test.go b/models/repo/release_test.go index 932775ae94d..888af26fea6 100644 --- a/models/repo/release_test.go +++ b/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()) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index d06702fd2b2..4ac65706447 100644 --- a/routers/api/v1/api.go +++ b/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). - Delete(reqToken(), reqAdmin(), context.ReferencesGitRepo(), repo.DeleteIssue) + Patch(reqToken(), mustNotBeArchived, bind(api.EditIssueOption{}), repo.EditIssue). + 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). - Put(reqToken(), bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels). - Delete(reqToken(), repo.ClearIssueLabels) - m.Delete("/{id}", reqToken(), repo.DeleteIssueLabel) + Post(reqToken(), mustNotBeArchived, bind(api.IssueLabelsOption{}), repo.AddIssueLabels). + Put(reqToken(), mustNotBeArchived, bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels). + Delete(reqToken(), mustNotBeArchived, repo.ClearIssueLabels) + m.Delete("/{id}", reqToken(), mustNotBeArchived, repo.DeleteIssueLabel) }) m.Group("/times", func() { m.Combo(""). diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index dd951232eef..2b646d45837 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -688,6 +688,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")) @@ -873,6 +875,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) diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 92d520dabc5..162bc5286fe 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/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 !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { + if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, comment.Issue, comment.PosterID, perm.AccessModeWrite) { ctx.Status(http.StatusForbidden) return } @@ -676,11 +676,11 @@ func deleteIssueComment(ctx *context.APIContext) { ctx.APIErrorAuto(err) return } - - if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.Permission.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { + if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, comment.Issue, comment.PosterID, perm.AccessModeWrite) { ctx.Status(http.StatusForbidden) return - } else if !comment.Type.HasContentSupport() { + } + if !comment.Type.HasContentSupport() { ctx.Status(http.StatusBadRequest) return } diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index 39aa142d68e..9eea903bf36 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/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 diff --git a/routers/api/v1/repo/issue_label.go b/routers/api/v1/repo/issue_label.go index a61f604e857..48854390a44 100644 --- a/routers/api/v1/repo/issue_label.go +++ b/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 { diff --git a/routers/api/v1/repo/issue_reaction.go b/routers/api/v1/repo/issue_reaction.go index a3539af798e..d77958cd473 100644 --- a/routers/api/v1/repo/issue_reaction.go +++ b/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 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) { + if !issue_service.CanSelfChangeOrAccessContent(ctx.Doer, ctx.Repo.Permission, comment.Issue, comment.PosterID, perm.AccessModeRead) { 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 } diff --git a/routers/web/org/home.go b/routers/web/org/home.go index 7086ba358fb..dfb7da079fd 100644 --- a/routers/web/org/home.go +++ b/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() diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index b044257198e..a48cc456291 100644 --- a/routers/web/repo/issue.go +++ b/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.Written() { + if !ctx.Repo.Permission.CanReadIssuesOrPulls(issue.IsPull) { + 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 } diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 04c8814ee8f..efff271b520 100644 --- a/routers/web/repo/issue_comment.go +++ b/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/log" @@ -146,31 +147,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")) - if err != nil { - ctx.ServerError("GetCommentByID", err) - return - } - - if err := comment.LoadIssue(ctx); err != nil { - ctx.ServerError("LoadIssue", 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) + comment := getRepoIssueCommentForSelfWriteOrAccess(ctx, ctx.PathParamInt64("id"), perm.AccessModeWrite) + if comment == nil { return } @@ -186,7 +194,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) { @@ -216,6 +224,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) @@ -232,31 +241,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")) - if err != nil { - ctx.ServerError("GetCommentByID", err) + comment := getRepoIssueCommentForSelfWriteOrAccess(ctx, ctx.PathParamInt64("id"), perm.AccessModeWrite) + if comment == nil { return } - if err := comment.LoadIssue(ctx); err != nil { - ctx.ServerError("LoadIssue", 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 { + if err := issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil { ctx.ServerError("DeleteComment", err) return } @@ -266,51 +256,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.ServerError("GetCommentByID", err) - return - } - - if err := comment.LoadIssue(ctx); err != nil { - ctx.ServerError("LoadIssue", 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) @@ -338,7 +289,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 } @@ -372,24 +323,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")) - if err != nil { - ctx.ServerError("GetCommentByID", err) - return - } - - if err := comment.LoadIssue(ctx); err != nil { - ctx.ServerError("LoadIssue", 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{}) + comment := getRepoIssueComment(ctx, ctx.PathParamInt64("id")) + if comment == nil { return } diff --git a/routers/web/repo/issue_watch.go b/routers/web/repo/issue_watch.go index c00a8191977..3194cc33083 100644 --- a/routers/web/repo/issue_watch.go +++ b/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() { - 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) + if issue == nil { return } diff --git a/routers/web/user/heatmap.go b/routers/web/user/heatmap.go index a9e7ac86ccf..475e48ff580 100644 --- a/routers/web/user/heatmap.go +++ b/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 { diff --git a/routers/web/web.go b/routers/web/web.go index fa65209d8d1..cfd1e769ba2 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1523,7 +1523,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 { diff --git a/services/issue/issue.go b/services/issue/issue.go index b903f15b0b1..d039c406095 100644 --- a/services/issue/issue.go +++ b/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 +} diff --git a/services/issue/issue_test.go b/services/issue/issue_test.go index e8fae5a194c..5c84ba48a08 100644 --- a/services/issue/issue_test.go +++ b/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"}, diff --git a/templates/swagger/v1-openapi3.generated.json b/templates/swagger/v1-openapi3.generated.json index 8ef255a225d..aba2c1bec2f 100644 --- a/templates/swagger/v1-openapi3.generated.json +++ b/templates/swagger/v1-openapi3.generated.json @@ -23825,6 +23825,9 @@ }, "404": { "$ref": "#/components/responses/notFound" + }, + "423": { + "$ref": "#/components/responses/repoArchivedError" } }, "summary": "Delete an issue", @@ -23932,6 +23935,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.", @@ -25151,6 +25157,9 @@ }, "404": { "$ref": "#/components/responses/notFound" + }, + "423": { + "$ref": "#/components/responses/repoArchivedError" } }, "summary": "Remove all labels from an issue", @@ -25254,6 +25263,9 @@ }, "404": { "$ref": "#/components/responses/notFound" + }, + "423": { + "$ref": "#/components/responses/repoArchivedError" } }, "summary": "Add a label to an issue", @@ -25312,6 +25324,9 @@ }, "404": { "$ref": "#/components/responses/notFound" + }, + "423": { + "$ref": "#/components/responses/repoArchivedError" } }, "summary": "Replace an issue's labels", @@ -25375,6 +25390,9 @@ }, "422": { "$ref": "#/components/responses/validationError" + }, + "423": { + "$ref": "#/components/responses/repoArchivedError" } }, "summary": "Remove a label from an issue", diff --git a/templates/swagger/v1-swagger.generated.json b/templates/swagger/v1-swagger.generated.json index 9f6f28bef01..f5bfc6b95d0 100644 --- a/templates/swagger/v1-swagger.generated.json +++ b/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" } } } diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index 28c5a477aec..d53088ffeb4 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -262,6 +262,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) { diff --git a/tests/integration/heatmap_test.go b/tests/integration/heatmap_test.go index 11387c80ff9..da278419c80 100644 --- a/tests/integration/heatmap_test.go +++ b/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") diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 408d5a8d76e..95bd8564ff6 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -381,6 +381,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) { diff --git a/tests/integration/repo_topic_test.go b/tests/integration/repo_topic_test.go index 3dfaa37d056..989416d6879 100644 --- a/tests/integration/repo_topic_test.go +++ b/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") diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go index f6df1ee1722..76bc85adcc1 100644 --- a/tests/integration/user_test.go +++ b/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. diff --git a/web_src/js/features/comp/ReactionSelector.ts b/web_src/js/features/comp/ReactionSelector.ts index efc592597a9..b7e944f9e7f 100644 --- a/web_src/js/features/comp/ReactionSelector.ts +++ b/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();