Check for permission when fetching user controlled issues (#20133) (#20196)

* Check if project has the same repository id with issue when assign project to issue

* Check if issue's repository id match project's repository id

* Add more permission checking

* Remove invalid argument

* Fix errors

* Add generic check

* Remove duplicated check

* Return error + add check for new issues

* Apply suggestions from code review

Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: 6543 <6543@obermui.de>

3 files changed, 44 insertions, 23 deletions

diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go
index 248743471b..cc384bd013 100644
--- a/routers/web/repo/issue.go
+++ b/routers/web/repo/issue.go
@@ -57,17 +57,15 @@ const (
 	issueTemplateTitleKey = "IssueTemplateTitle"
 )
 
-var (
-	// IssueTemplateCandidates issue templates
-	IssueTemplateCandidates = []string{
-		"ISSUE_TEMPLATE.md",
-		"issue_template.md",
-		".gitea/ISSUE_TEMPLATE.md",
-		".gitea/issue_template.md",
-		".github/ISSUE_TEMPLATE.md",
-		".github/issue_template.md",
-	}
-)
+// IssueTemplateCandidates issue templates
+var IssueTemplateCandidates = []string{
+	"ISSUE_TEMPLATE.md",
+	"issue_template.md",
+	".gitea/ISSUE_TEMPLATE.md",
+	".gitea/issue_template.md",
+	".github/ISSUE_TEMPLATE.md",
+	".github/issue_template.md",
+}
 
 // MustAllowUserComment checks to make sure if an issue is locked.
 // If locked and user has permissions to write to the repository,
@@ -245,7 +243,7 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption uti
 		}
 	}
 
-	var issueList = models.IssueList(issues)
+	issueList := models.IssueList(issues)
 	approvalCounts, err := issueList.GetApprovalCounts()
 	if err != nil {
 		ctx.ServerError("ApprovalCounts", err)
@@ -311,8 +309,7 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption uti
 		assigneeID = 0 // Reset ID to prevent unexpected selection of assignee.
 	}
 
-	ctx.Data["IssueRefEndNames"], ctx.Data["IssueRefURLs"] =
-		issue_service.GetRefEndNamesAndURLs(issues, ctx.Repo.RepoLink)
+	ctx.Data["IssueRefEndNames"], ctx.Data["IssueRefURLs"] = issue_service.GetRefEndNamesAndURLs(issues, ctx.Repo.RepoLink)
 
 	ctx.Data["ApprovalCounts"] = func(issueID int64, typ string) int64 {
 		counts, ok := approvalCounts[issueID]
@@ -442,7 +439,6 @@ func RetrieveRepoMilestonesAndAssignees(ctx *context.Context, repo *repo_model.R
 }
 
 func retrieveProjects(ctx *context.Context, repo *repo_model.Repository) {
-
 	var err error
 
 	ctx.Data["OpenProjects"], _, err = models.GetProjects(models.ProjectSearchOptions{
@@ -796,7 +792,8 @@ func NewIssue(ctx *context.Context) {
 	body := ctx.FormString("body")
 	ctx.Data["BodyQuery"] = body
 
-	ctx.Data["IsProjectsEnabled"] = ctx.Repo.CanRead(unit.TypeProjects)
+	isProjectsEnabled := ctx.Repo.CanRead(unit.TypeProjects)
+	ctx.Data["IsProjectsEnabled"] = isProjectsEnabled
 	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
 	upload.AddUploadContext(ctx, "comment")
 
@@ -812,7 +809,7 @@ func NewIssue(ctx *context.Context) {
 	}
 
 	projectID := ctx.FormInt64("project")
-	if projectID > 0 {
+	if projectID > 0 && isProjectsEnabled {
 		project, err := models.GetProjectByID(projectID)
 		if err != nil {
 			log.Error("GetProjectByID: %d: %v", projectID, err)
@@ -1017,6 +1014,12 @@ func NewIssuePost(ctx *context.Context) {
 	}
 
 	if projectID > 0 {
+		if !ctx.Repo.CanRead(unit.TypeProjects) {
+			// User must also be able to see the project.
+			ctx.Error(http.StatusBadRequest, "user hasn't permissions to read projects")
+			return
+		}
+
 		if err := models.ChangeProjectAssign(issue, ctx.User, projectID); err != nil {
 			ctx.ServerError("ChangeProjectAssign", err)
 			return
@@ -1713,6 +1716,11 @@ func getActionIssues(ctx *context.Context) []*models.Issue {
 	issueUnitEnabled := ctx.Repo.CanRead(unit.TypeIssues)
 	prUnitEnabled := ctx.Repo.CanRead(unit.TypePullRequests)
 	for _, issue := range issues {
+		if issue.RepoID != ctx.Repo.Repository.ID {
+			ctx.NotFound("some issue's RepoID is incorrect", errors.New("some issue's RepoID is incorrect"))
+			return nil
+		}
+
 		if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled {
 			ctx.NotFound("IssueOrPullRequestUnitNotAllowed", nil)
 			return nil
@@ -2515,7 +2523,7 @@ func filterXRefComments(ctx *context.Context, issue *models.Issue) error {
 // GetIssueAttachments returns attachments for the issue
 func GetIssueAttachments(ctx *context.Context) {
 	issue := GetActionIssue(ctx)
-	var attachments = make([]*api.Attachment, len(issue.Attachments))
+	attachments := make([]*api.Attachment, len(issue.Attachments))
 	for i := 0; i < len(issue.Attachments); i++ {
 		attachments[i] = convert.ToReleaseAttachment(issue.Attachments[i])
 	}
@@ -2529,7 +2537,7 @@ func GetCommentAttachments(ctx *context.Context) {
 		ctx.NotFoundOrServerError("GetCommentByID", models.IsErrCommentNotExist, err)
 		return
 	}
-	var attachments = make([]*api.Attachment, 0)
+	attachments := make([]*api.Attachment, 0)
 	if comment.Type == models.CommentTypeComment {
 		if err := comment.LoadAttachments(); err != nil {
 			ctx.ServerError("LoadAttachments", err)
@@ -2674,7 +2682,7 @@ func handleTeamMentions(ctx *context.Context) {
 	var isAdmin bool
 	var err error
 	var teams []*models.Team
-	var org = models.OrgFromUser(ctx.Repo.Owner)
+	org := models.OrgFromUser(ctx.Repo.Owner)
 	// Admin has super access.
 	if ctx.User.IsAdmin {
 		isAdmin = true
diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go
index f6be8add0b..aae973511e 100644
--- a/routers/web/repo/projects.go
+++ b/routers/web/repo/projects.go
@@ -5,6 +5,7 @@
 package repo
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -531,7 +532,6 @@ func EditProjectBoard(ctx *context.Context) {
 
 // SetDefaultProjectBoard set default board for uncategorized issues/pulls
 func SetDefaultProjectBoard(ctx *context.Context) {
-
 	project, board := checkProjectBoardChangePermissions(ctx)
 	if ctx.Written() {
 		return
@@ -631,10 +631,17 @@ func MoveIssues(ctx *context.Context) {
 	}
 
 	if len(movedIssues) != len(form.Issues) {
-		ctx.ServerError("IssuesNotFound", err)
+		ctx.ServerError("some issues do not exist", errors.New("some issues do not exist"))
 		return
 	}
 
+	for _, issue := range movedIssues {
+		if issue.RepoID != project.RepoID {
+			ctx.ServerError("Some issue's repoID is not equal to project's repoID", errors.New("Some issue's repoID is not equal to project's repoID"))
+			return
+		}
+	}
+
 	if err = models.MoveIssuesOnProjectBoard(board, sortedIssueIDs); err != nil {
 		ctx.ServerError("MoveIssuesOnProjectBoard", err)
 		return
diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go
index 257aa737f6..383e2d6cab 100644
--- a/routers/web/repo/pull_review.go
+++ b/routers/web/repo/pull_review.go
@@ -5,6 +5,7 @@
 package repo
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -116,6 +117,11 @@ func UpdateResolveConversation(ctx *context.Context) {
 		return
 	}
 
+	if comment.Issue.RepoID != ctx.Repo.Repository.ID {
+		ctx.NotFound("comment's repoID is incorrect", errors.New("comment's repoID is incorrect"))
+		return
+	}
+
 	var permResult bool
 	if permResult, err = models.CanMarkConversation(comment.Issue, ctx.User); err != nil {
 		ctx.ServerError("CanMarkConversation", err)
@@ -234,7 +240,7 @@ func SubmitReview(ctx *context.Context) {
 // DismissReview dismissing stale review by repo admin
 func DismissReview(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.DismissReviewForm)
-	comm, err := pull_service.DismissReview(form.ReviewID, form.Message, ctx.User, true)
+	comm, err := pull_service.DismissReview(form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.User, true)
 	if err != nil {
 		ctx.ServerError("pull_service.DismissReview", err)
 		return