Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail when API & web endpoints use unrelated ids #28211

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion models/issues/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -1014,6 +1014,7 @@ type FindCommentsOptions struct {
Type CommentType
IssueIDs []int64
Invalidated util.OptionalBool
IsPull util.OptionalBool
}

// ToConds implements FindOptions interface
Expand Down Expand Up @@ -1048,14 +1049,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")
}

Expand Down
4 changes: 2 additions & 2 deletions routers/api/v1/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -1148,8 +1148,8 @@ func Routes(ctx gocontext.Context) *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() {
Expand Down
22 changes: 22 additions & 0 deletions routers/api/v1/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,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() {
Expand Down Expand Up @@ -561,6 +579,10 @@ func GetIssue(ctx *context.APIContext) {
}
return
}
if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) {
ctx.NotFound()
return
}
ctx.JSON(http.StatusOK, convert.ToAPIIssue(ctx, issue))
}

Expand Down
56 changes: 54 additions & 2 deletions routers/api/v1/repo/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -69,6 +71,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{
Expand Down Expand Up @@ -265,12 +272,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)
Expand Down Expand Up @@ -357,6 +379,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
Expand Down Expand Up @@ -426,6 +453,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
Expand Down Expand Up @@ -544,7 +576,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
}
Expand Down Expand Up @@ -647,7 +689,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 {
Expand Down
20 changes: 18 additions & 2 deletions routers/api/v1/repo/issue_reaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,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) {
Expand Down Expand Up @@ -184,9 +190,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) {
Expand Down
5 changes: 5 additions & 0 deletions routers/api/v1/repo/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ func GetDeployKey(ctx *context.APIContext) {
return
}

if key.RepoID != ctx.Repo.Repository.ID {
ctx.Status(http.StatusNotFound)
return
}

if err = key.GetContent(); err != nil {
ctx.Error(http.StatusInternalServerError, "GetContent", err)
return
Expand Down
25 changes: 25 additions & 0 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -2968,6 +2968,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
Expand Down Expand Up @@ -3034,6 +3039,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
Expand Down Expand Up @@ -3160,6 +3170,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 {
Expand Down Expand Up @@ -3303,6 +3318,16 @@ 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 !comment.Type.HasAttachmentSupport() {
ctx.ServerError("GetCommentAttachments", fmt.Errorf("comment type %v does not support attachments", comment.Type))
return
Expand Down
12 changes: 12 additions & 0 deletions routers/web/repo/issue_content_history.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ func GetContentHistoryDetail(ctx *context.Context) {
})
return
}
if history.IssueID != issue.ID {
ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{})
return
}

// get the related comment if this history revision is for a comment, otherwise the history revision is for an issue.
var comment *issues_model.Comment
Expand Down Expand Up @@ -194,11 +198,19 @@ func SoftDeleteContentHistory(ctx *context.Context) {
log.Error("can not get comment for issue content history %v. err=%v", historyID, err)
return
}
if comment.IssueID != issue.ID {
ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{})
return
}
}
if history, err = issues_model.GetIssueContentHistoryByID(ctx, historyID); err != nil {
log.Error("can not get issue content history %v. err=%v", historyID, err)
return
}
if history.IssueID != issue.ID {
ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{})
return
}

canSoftDelete := canSoftDeleteContentHistory(ctx, issue, comment, history)
if !canSoftDelete {
Expand Down
4 changes: 4 additions & 0 deletions routers/web/repo/issue_pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ func IssuePinMove(ctx *context.Context) {
log.Error(err.Error())
return
}
if issue.RepoID != ctx.Repo.Repository.ID {
ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{})
return
}

err = issue.MovePin(ctx, form.Position)
if err != nil {
Expand Down
12 changes: 11 additions & 1 deletion routers/web/repo/release.go
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,17 @@ func DeleteTag(ctx *context.Context) {
}

func deleteReleaseOrTag(ctx *context.Context, isDelTag bool) {
if err := releaseservice.DeleteReleaseByID(ctx, ctx.FormInt64("id"), ctx.Doer, isDelTag); err != nil {
id := ctx.FormInt64("id")
rel, err := repo_model.GetReleaseByID(ctx, id)
if err != nil {
ctx.ServerError("GetRelease", err)
return
}
if ctx.Repo.Repository.ID != rel.RepoID {
ctx.NotFound("CompareRepoID", repo_model.ErrReleaseNotExist{})
return
}
if err := releaseservice.DeleteReleaseByID(ctx, id, ctx.Doer, isDelTag); err != nil {
if models.IsErrProtectedTagName(err) {
ctx.Flash.Error(ctx.Tr("repo.release.tag_name_protected"))
} else {
Expand Down