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 <art27@cantab.net>
* Add doctor command to remove the broken label comments

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: 6543 <6543@obermui.de>

diff --git a/models/consistency.go b/models/consistency.go
index f689261a55a18431df3c1b6d35bd0829dd4e608b..aa46e787f160a1f9f13193ae2182aa38dc4b829d 100644 (file)
--- a/models/consistency.go
+++ b/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))
+}

diff --git a/modules/doctor/dbconsistency.go b/modules/doctor/dbconsistency.go
index f09aaa6d1be06ea5a6599520300e9c6a6ae87fdd..942a45cb30d45f67e9001008b7a7b3336a8b0496 100644 (file)
--- a/modules/doctor/dbconsistency.go
+++ b/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

diff --git a/modules/templates/helper.go b/modules/templates/helper.go
index 4e767ad44dc0720d6b466550b919df411b3f3cf2..987a6ad9833a0ee7e78a7bc43e617b335a174bfa 100644 (file)
--- a/modules/templates/helper.go
+++ b/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))
                        }

diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 3bc20839aa44c564e5fd38c2be96a892ad2d94b9..1f57f2e9c808dc3ffcd061aaef2e70025da31ab3 100644 (file)
--- a/routers/repo/issue.go
+++ b/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:]...)