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();