Prevent addition of labels from outside the repository or organisation in issues

[zeripath]

It has been possible to add labels to issues that do not belong to the repository or organisation as there were insufficient checks on this process.

This PRs enforces these checks and adds comments to the basic functions involved that ensure that future users will need to check.

It adds a migration to the system that simply deletes these issue_labels and comments - however, clearly this is not ideal as we should really attempt to match the labels with labels that are available. A perhaps better migration would also add the necessary labels to the repository.

On repository transfer all organization labels and comments related will be removed. I've decided this because: a) it's simpler and b) it prevents leaking of organization labels

It adds a consistency check to find these labels.

Fix #14908

Signed-off-by: Andrew Thornton art27@cantab.net

[CirnoT]

Excuse me for sounding dumb, but what is the point of this doctor check? Neither API nor web interface endpoints gate the label added to belong to repo or org. I can send API request or change data-id in DOM to point to outside label and the Gitea will be more than happy to perform the requested action.

Sounds to me like we need to add the appropriate check in code and solve this in single-time migration, rather than in Doctor check.

[zeripath]

Thanks @CirnoT I had presumed that the rest of the code would be doing the checks especially given the issue report which stated that all that was needed was a db consistency check.

Clearly they were not.

This is a much larger task than first thought.

[noerw]

Excuse me for sounding dumb, but what is the point of this doctor check? Neither API nor web interface endpoints gate the label added to belong to repo or org. I can send API request or change data-id in DOM to point to outside label and the Gitea will be more than happy to perform the requested action.

Sounds to me like we need to add the appropriate check in code and solve this in single-time migration, rather than in Doctor check.

newIssue() guards against foreign labels:

gitea/models/issue.go

Lines 937 to 940 in 9b261f5

	// Silently drop invalid labels.
	if label.RepoID != opts.Repo.ID && label.OrgID != opts.Repo.OwnerID {
	continue
	}

..and newly created issues had these foreign labels attached too. That's what confused me so much when I tried to investigate a couple days ago

[zeripath]

@noerw I think it depends on how these labels are being added - I suspect there is another issue here too. Hence the WIP.

I think this one needs actual testing.

Easiest way I've found to test any PR:

git fetch git@github:go-gitea/gitea refs/pull/14912/head:pr-14912
git checkout pr-14912

---

I have a little script:

#!/bin/sh
git fetch -f git@github.com:go-gitea/gitea refs/pull/$1/head:pr-$1
git checkout pr-$1

[lunny]

Should we copy or delete the org labels when transfer a repository? It's a question.

However, the transfer of repository should become a task running in background like migrations and there is a middle state BeingTransfering.

Move the refactor of Check outside the PR will make the code more clear.

[lunny]

diff --git a/models/issue_label.go b/models/issue_label.go
index 54b286fe7..2da0d2f20 100644
--- a/models/issue_label.go
+++ b/models/issue_label.go
@@ -632,7 +632,43 @@ func HasIssueLabel(issueID, labelID int64) bool {
        return hasIssueLabel(x, issueID, labelID)
 }

+// ErrAddIssueLabelNotAllowed represents the error adding the label to issue not allowed
+type ErrAddIssueLabelNotAllowed struct {
+       LabelID     int64
+       LabelRepoID int64
+       LabelOrgID  int64
+       IssueID     int64
+       IssueRepoID int64
+       IssueOrgID  int64
+}
+
+func (err ErrAddIssueLabelNotAllowed) Error() string {
+       return fmt.Sprintf("Add label %d[%d,%d] to issue/pr %d failed because it's not belong respository %d or orgnization %d",
+               err.LabelID, err.LabelRepoID, err.LabelOrgID,
+               err.IssueID,
+               err.IssueRepoID,
+               err.IssueOrgID,
+       )
+}
+
 func newIssueLabel(e *xorm.Session, issue *Issue, label *Label, doer *User) (err error) {
+       if label.RepoID > 0 && label.RepoID != issue.RepoID {
+               return ErrAddIssueLabelNotAllowed{
+                       LabelID:     label.ID,
+                       LabelRepoID: label.RepoID,
+                       LabelOrgID:  label.OrgID,
+                       IssueID:     issue.ID,
+                       IssueRepoID: issue.RepoID,
+               }
+       } else if label.OrgID > 0 && label.OrgID != issue.Repo.OwnerID {
+               return ErrAddIssueLabelNotAllowed{
+                       LabelID:     label.ID,
+                       LabelRepoID: label.RepoID,
+                       LabelOrgID:  label.OrgID,
+                       IssueID:     issue.ID,
+                       IssueOrgID:  issue.Repo.OwnerID,
+               }
+       }
        if _, err = e.Insert(&IssueLabel{
                IssueID: issue.ID,
                LabelID: label.ID,

[zeripath]

That's gonna need to be added to a lot more places as there are lots of other places we silently drop these labels.

[zeripath]

Should we copy or delete the org labels when transfer a repository? It's a question.

I think delete - at least for this PR. Copying and reassigning is really complex. The only issue is the migration - perhaps the migration should be dropped.

However, the transfer of repository should become a task running in background like migrations and there is a middle state BeingTransfering.

Not this PR.

Move the refactor of Check outside the PR will make the code more clear.

Done.

[zeripath]

conflicts fixed.