From 829b49c1ae96015e345f9d2a0eb1ea7acd5f31e1 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sun, 18 Oct 2026 19:50:36 +0200 Subject: [PATCH] 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 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 da522d919fb..02a8ea7b082 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 1ea3026010a..3a7dcc54dd8 100644 --- a/models/repo/attachment_test.go +++ b/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) 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 9cc64adccad..aba8f5f33a2 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 c3d9dfac1d8..179548d0532 100644 --- a/routers/api/v1/repo/issue.go +++ b/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) 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 511085bcdd4..e93a6081a75 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 419dce03f30..53df42439c0 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/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")) - 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.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 } @@ -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")) - if err != nil { - ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err) + comment := getRepoIssueCommentForSelfWriteOrAccess(ctx, ctx.PathParamInt64("id"), perm.AccessModeWrite) + if comment == nil { 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 - } 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 } @@ -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")) - 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.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 84b4227ccfc..f679f7df918 100644 --- a/routers/web/web.go +++ b/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 { 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 c1ac350b400..3fab4fe93a7 100644 --- a/templates/swagger/v1-openapi3.generated.json +++ b/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", diff --git a/templates/swagger/v1-swagger.generated.json b/templates/swagger/v1-swagger.generated.json index f7128397a3e..c7b4b651546 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 b77775d1730..1d687fa1323 100644 --- a/tests/integration/api_issue_test.go +++ b/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) { 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 ccc730326e3..b3b58704d9e 100644 --- a/tests/integration/issue_test.go +++ b/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) { 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();