Prevent adding nil label to .AddedLabels or .RemovedLabels (#14623)

* Prevent adding nil label to .AddedLabels or .RemovedLabels

There are possibly a few old databases out there with malmigrated data that can
cause panics with empty labels being migrated.

This PR adds a few tests to prevent nil labels being added.

Fix #14466

Signed-off-by: Andrew Thornton <[email protected]>

* Add doctor command to remove the broken label comments

Signed-off-by: Andrew Thornton <[email protected]>

Co-authored-by: 6543 <[email protected]>

Author: zeripath

models/consistency.go

@@ -305,3 +305,13 @@ func CountWrongUserType() (int64, error) {
 func FixWrongUserType() (int64, error) {
 	return x.Where(builder.Eq{"type": 0}.And(builder.Neq{"num_teams": 0})).Cols("type").NoAutoTime().Update(&User{Type: 1})
 }
+
+// CountCommentTypeLabelWithEmptyLabel count label comments with empty label
+func CountCommentTypeLabelWithEmptyLabel() (int64, error) {
+	return x.Where(builder.Eq{"type": CommentTypeLabel, "label_id": 0}).Count(new(Comment))
+}
+
+// FixCommentTypeLabelWithEmptyLabel count label comments with empty label
+func FixCommentTypeLabelWithEmptyLabel() (int64, error) {
+	return x.Where(builder.Eq{"type": CommentTypeLabel, "label_id": 0}).Delete(new(Comment))
+}

modules/doctor/dbconsistency.go

@@ -111,6 +111,24 @@ func checkDBConsistency(logger log.Logger, autofix bool) error {
 		}
 	}
 
+	// find label comments with empty labels
+	count, err = models.CountCommentTypeLabelWithEmptyLabel()
+	if err != nil {
+		logger.Critical("Error: %v whilst counting label comments with empty labels")
+		return err
+	}
+	if count > 0 {
+		if autofix {
+			updatedCount, err := models.FixCommentTypeLabelWithEmptyLabel()
+			if err != nil {
+				logger.Critical("Error: %v whilst removing label comments with empty labels")
+				return err
+			}
+			logger.Info("%d label comments with empty labels removed", updatedCount)
+		} else {
+			logger.Warn("%d label comments with empty labels", count)
+		}
+	}
 	// TODO: function to recalc all counters
 
 	return nil

modules/templates/helper.go

@@ -371,6 +371,10 @@ func NewFuncMap() []template.FuncMap {
 		"RenderLabels": func(labels []*models.Label) template.HTML {
 			html := `<span class="labels-list">`
 			for _, label := range labels {
+				// Protect against nil value in labels - shouldn't happen but would cause a panic if so
+				if label == nil {
+					continue
+				}
 				html += fmt.Sprintf("<div class='ui label' style='color: %s; background-color: %s'>%s</div> ",
 					label.ForegroundColor(), label.Color, RenderEmoji(label.Name))
 			}

routers/repo/issue.go

@@ -2464,7 +2464,7 @@ func combineLabelComments(issue *models.Issue) {
 		if i == 0 || cur.Type != models.CommentTypeLabel ||
 			(prev != nil && prev.PosterID != cur.PosterID) ||
 			(prev != nil && cur.CreatedUnix-prev.CreatedUnix >= 60) {
-			if cur.Type == models.CommentTypeLabel {
+			if cur.Type == models.CommentTypeLabel && cur.Label != nil {
 				if cur.Content != "1" {
 					cur.RemovedLabels = append(cur.RemovedLabels, cur.Label)
 				} else {
@@ -2474,10 +2474,12 @@ func combineLabelComments(issue *models.Issue) {
 			continue
 		}
 
-		if cur.Content != "1" {
-			prev.RemovedLabels = append(prev.RemovedLabels, cur.Label)
-		} else {
-			prev.AddedLabels = append(prev.AddedLabels, cur.Label)
+		if cur.Label != nil {
+			if cur.Content != "1" {
+				prev.RemovedLabels = append(prev.RemovedLabels, cur.Label)
+			} else {
+				prev.AddedLabels = append(prev.AddedLabels, cur.Label)
+			}
 		}
 		prev.CreatedUnix = cur.CreatedUnix
 		issue.Comments = append(issue.Comments[:i], issue.Comments[i+1:]...)