Fix comment permissions

models/fixtures/comment.yml

@@ -66,3 +66,12 @@
  tree_path: "README.md"
  created_unix: 946684812
  invalidated: true
+
+-
+ id: 8
+ type: 0 # comment
+ poster_id: 2
+ issue_id: 4 # in repo_id 2
+ content: "comment in private pository"
+ created_unix: 946684811
+ updated_unix: 946684811

models/issues/comment.go

@@ -1024,6 +1024,7 @@ type FindCommentsOptions struct {
  Type CommentType
  IssueIDs []int64
  Invalidated util.OptionalBool
+ IsPull util.OptionalBool
 }
 
 // ToConds implements FindOptions interface
@@ -1058,14 +1059,17 @@ func (opts FindCommentsOptions) ToConds() builder.Cond {
  if !opts.Invalidated.IsNone() {
  cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()})
  }
+ if opts.IsPull != util.OptionalBoolNone {
+ cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.IsTrue()})
+ }
  return cond
 }
 
 // FindComments returns all comments according options
 func FindComments(ctx context.Context, opts *FindCommentsOptions) (CommentList, error) {
  comments := make([]*Comment, 0, 10)
  sess := db.GetEngine(ctx).Where(opts.ToConds())
- if opts.RepoID > 0 {
+ if opts.RepoID > 0 || opts.IsPull != util.OptionalBoolNone {
  sess.Join("INNER", "issue", "issue.id = comment.issue_id")
  }
 

routers/api/v1/api.go

@@ -1259,8 +1259,8 @@ func Routes() *web.Route {
  m.Group("/{username}/{reponame}", func() {
  m.Group("/issues", func() {
  m.Combo("").Get(repo.ListIssues).
- Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), repo.CreateIssue)
- m.Get("/pinned", repo.ListPinnedIssues)
+ Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue)
+ m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues)
  m.Group("/comments", func() {
  m.Get("", repo.ListRepoIssueComments)
  m.Group("/{id}", func() {

routers/api/v1/repo/issue.go

@@ -462,6 +462,24 @@ func ListIssues(ctx *context.APIContext) {
  isPull = util.OptionalBoolNone
  }
 
+ if isPull != util.OptionalBoolNone && !ctx.Repo.CanWriteIssuesOrPulls(isPull.IsTrue()) {
+ ctx.NotFound()
+ return
+ }
+
+ if isPull == util.OptionalBoolNone {
+ canReadIssues := ctx.Repo.CanRead(unit.TypeIssues)
+ canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests)
+ if !canReadIssues && !canReadPulls {
+ ctx.NotFound()
+ return
+ } else if !canReadIssues {
+ isPull = util.OptionalBoolTrue
+ } else if !canReadPulls {
+ isPull = util.OptionalBoolFalse
+ }
+ }
+
  // FIXME: we should be more efficient here
  createdByID := getUserIDForFilter(ctx, "created_by")
  if ctx.Written() {
@@ -593,6 +611,10 @@ func GetIssue(ctx *context.APIContext) {
  }
  return
  }
+ if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) {
+ ctx.NotFound()
+ return
+ }
  ctx.JSON(http.StatusOK, convert.ToAPIIssue(ctx, issue))
 }
 

routers/api/v1/repo/issue_comment.go

@@ -12,9 +12,11 @@ import (
  issues_model "code.gitea.io/gitea/models/issues"
  access_model "code.gitea.io/gitea/models/perm/access"
  repo_model "code.gitea.io/gitea/models/repo"
+ "code.gitea.io/gitea/models/unit"
  user_model "code.gitea.io/gitea/models/user"
  "code.gitea.io/gitea/modules/context"
  api "code.gitea.io/gitea/modules/structs"
+ "code.gitea.io/gitea/modules/util"
  "code.gitea.io/gitea/modules/web"
  "code.gitea.io/gitea/routers/api/v1/utils"
  "code.gitea.io/gitea/services/convert"
@@ -71,6 +73,11 @@ func ListIssueComments(ctx *context.APIContext) {
  ctx.Error(http.StatusInternalServerError, "GetRawIssueByIndex", err)
  return
  }
+ if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) {
+ ctx.NotFound()
+ return
+ }
+
  issue.Repo = ctx.Repo.Repository
 
  opts := &issues_model.FindCommentsOptions{
@@ -271,12 +278,27 @@ func ListRepoIssueComments(ctx *context.APIContext) {
  return
  }
 
+ var isPull util.OptionalBool
+ canReadIssue := ctx.Repo.CanRead(unit.TypeIssues)
+ canReadPull := ctx.Repo.CanRead(unit.TypePullRequests)
+ if canReadIssue && canReadPull {
+ isPull = util.OptionalBoolNone
+ } else if canReadIssue {
+ isPull = util.OptionalBoolFalse
+ } else if canReadPull {
+ isPull = util.OptionalBoolTrue
+ } else {
+ ctx.NotFound()
+ return
+ }
+
  opts := &issues_model.FindCommentsOptions{
  ListOptions: utils.GetListOptions(ctx),
  RepoID: ctx.Repo.Repository.ID,
  Type: issues_model.CommentTypeComment,
  Since: since,
  Before: before,
+ IsPull: isPull,
  }
 
  comments, err := issues_model.FindComments(ctx, opts)
@@ -367,6 +389,11 @@ func CreateIssueComment(ctx *context.APIContext) {
  return
  }
 
+ if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) {
+ ctx.NotFound()
+ return
+ }
+
  if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin {
  ctx.Error(http.StatusForbidden, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked")))
  return
@@ -436,6 +463,11 @@ func GetIssueComment(ctx *context.APIContext) {
  return
  }
 
+ if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
+ ctx.NotFound()
+ return
+ }
+
  if comment.Type != issues_model.CommentTypeComment {
  ctx.Status(http.StatusNoContent)
  return
@@ -555,7 +587,17 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
  return
  }
 
- if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.IsAdmin()) {
+ if err := comment.LoadIssue(ctx); err != nil {
+ ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
+ return
+ }
+
+ if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+ ctx.Status(http.StatusNotFound)
+ return
+ }
+
+ if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
  ctx.Status(http.StatusForbidden)
  return
  }
@@ -658,7 +700,17 @@ func deleteIssueComment(ctx *context.APIContext) {
  return
  }
 
- if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.IsAdmin()) {
+ if err := comment.LoadIssue(ctx); err != nil {
+ ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
+ return
+ }
+
+ if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+ ctx.Status(http.StatusNotFound)
+ return
+ }
+
+ if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
  ctx.Status(http.StatusForbidden)
  return
  } else if comment.Type != issues_model.CommentTypeComment {

routers/api/v1/repo/issue_comment_attachment.go

@@ -329,6 +329,10 @@ func getIssueCommentSafe(ctx *context.APIContext) *issues_model.Comment {
  return nil
  }
 
+ if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
+ return nil
+ }
+
  comment.Issue.Repo = ctx.Repo.Repository
 
  return comment

routers/api/v1/repo/issue_reaction.go

@@ -61,6 +61,12 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
 
  if err := comment.LoadIssue(ctx); err != nil {
  ctx.Error(http.StatusInternalServerError, "comment.LoadIssue", err)
+ return
+ }
+
+ if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+ ctx.NotFound()
+ return
  }
 
  if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
@@ -190,9 +196,19 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp
  return
  }
 
- err = comment.LoadIssue(ctx)
- if err != nil {
+ if err = comment.LoadIssue(ctx); err != nil {
  ctx.Error(http.StatusInternalServerError, "comment.LoadIssue() failed", err)
+ return
+ }
+
+ if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+ ctx.NotFound()
+ return
+ }
+
+ if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) {
+ ctx.NotFound()
+ return
  }
 
  if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) {

routers/web/repo/issue.go

@@ -3106,6 +3106,11 @@ func UpdateCommentContent(ctx *context.Context) {
  return
  }
 
+ if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+ ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{})
+ return
+ }
+
  if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
  ctx.Error(http.StatusForbidden)
  return
@@ -3172,6 +3177,11 @@ func DeleteComment(ctx *context.Context) {
  return
  }
 
+ if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+ ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{})
+ return
+ }
+
  if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
  ctx.Error(http.StatusForbidden)
  return
@@ -3298,6 +3308,11 @@ func ChangeCommentReaction(ctx *context.Context) {
  return
  }
 
+ if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+ ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{})
+ return
+ }
+
  if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull)) {
  if log.IsTrace() {
  if ctx.IsSigned {
@@ -3441,6 +3456,21 @@ func GetCommentAttachments(ctx *context.Context) {
  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("CompareRepoID", issues_model.ErrCommentNotExist{})
+ return
+ }
+
+ if !ctx.Repo.Permission.CanReadIssuesOrPulls(comment.Issue.IsPull) {
+ ctx.NotFound("CanReadIssuesOrPulls", issues_model.ErrCommentNotExist{})
+ return
+ }
+
  if !comment.Type.HasAttachmentSupport() {
  ctx.ServerError("GetCommentAttachments", fmt.Errorf("comment type %v does not support attachments", comment.Type))
  return

tests/integration/api_comment_attachment_test.go

@@ -35,6 +35,14 @@ func TestAPIGetCommentAttachment(t *testing.T) {
  repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: comment.Issue.RepoID})
  repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
 
+ t.Run("UnrelatedCommentID", func(t *testing.T) {
+ repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+ repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+ token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue)
+ req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/issues/comments/%d/assets/%d?token=%s", repoOwner.Name, repo.Name, comment.ID, attachment.ID, token)
+ MakeRequest(t, req, http.StatusNotFound)
+ })
+
  session := loginUser(t, repoOwner.Name)
  token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadIssue)
  req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/issues/comments/%d/assets/%d?token=%s", repoOwner.Name, repo.Name, comment.ID, attachment.ID, token)

tests/integration/api_comment_test.go

@@ -177,12 +177,25 @@ func TestAPIEditComment(t *testing.T) {
  defer tests.PrepareTestEnv(t)()
  const newCommentBody = "This is the new comment body"
 
- comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{},
+ comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 8},
  unittest.Cond("type = ?", issues_model.CommentTypeComment))
  issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID})
  repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID})
  repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
 
+ t.Run("UnrelatedCommentID", func(t *testing.T) {
+ // Using the ID of a comment that does not belong to the repository must fail
+ repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+ repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+ token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue)
+ urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d?token=%s",
+ repoOwner.Name, repo.Name, comment.ID, token)
+ req := NewRequestWithValues(t, "PATCH", urlStr, map[string]string{
+ "body": newCommentBody,
+ })
+ MakeRequest(t, req, http.StatusNotFound)
+ })
+
  token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue)
  urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d?token=%s",
  repoOwner.Name, repo.Name, comment.ID, token)
@@ -201,12 +214,22 @@ func TestAPIEditComment(t *testing.T) {
 func TestAPIDeleteComment(t *testing.T) {
  defer tests.PrepareTestEnv(t)()
 
- comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{},
+ comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 8},
  unittest.Cond("type = ?", issues_model.CommentTypeComment))
  issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID})
  repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID})
  repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
 
+ t.Run("UnrelatedCommentID", func(t *testing.T) {
+ // Using the ID of a comment that does not belong to the repository must fail
+ repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+ repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+ token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue)
+ req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/issues/comments/%d?token=%s",
+ repoOwner.Name, repo.Name, comment.ID, token)
+ MakeRequest(t, req, http.StatusNotFound)
+ })
+
  token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue)
  req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/issues/comments/%d?token=%s",
  repoOwner.Name, repo.Name, comment.ID, token)