Merge pull request #65 from cloudflare/strict-filters

Strict filters

assets/static/filters.js

@@ -106,10 +106,10 @@ var Filters = (function() {
             $.each($('span.tag-badge'), function(j, tag) {
                 if (sha1(filter.text) == $(tag).data('badge-id')) {
                     $(tag).html(filter.hits.toString());
-                    if (filter.isValid) {
-                        $(tag).addClass('label-info').removeClass('label-danger');
+                    if (filter.isValid == true) {
+                        $(tag).parent().addClass('label-info').removeClass('label-danger');
                     } else {
-                        $(tag).addClass('label-danger').removeClass('label-info');
+                        $(tag).parent().addClass('label-danger').removeClass('label-info');
                     }
                 }
             });

filters/filter.go

@@ -3,7 +3,6 @@ package filters
 import (
 	"fmt"
 	"regexp"
-	"strings"
 
 	"github.com/cloudflare/unsee/models"
 )
@@ -55,10 +54,13 @@ type newFilterFactory func() FilterT
 // expression will be parsed and best filter implementation and value matcher
 // will be selected
 func NewFilter(expression string) FilterT {
-	for _, fc := range AllFilters {
+	invalid := alwaysInvalidFilter{}
+	invalid.init("", nil, expression, false, expression)
 
-		reOperators := strings.Join(fc.SupportedOperators, "|")
-		reExp := fmt.Sprintf("^(?P<matched>(%s))(?P<operator>(%s))(?P<value>(.+))", fc.Label, reOperators)
+	for _, fc := range AllFilters {
+		f := fc.Factory()
+
+		reExp := fmt.Sprintf("^(?P<matched>(%s))(?P<operator>(%s))(?P<value>(.*))", fc.Label, matcherRegex)
 		re := regexp.MustCompile(reExp)
 		match := re.FindStringSubmatch(expression)
 		result := make(map[string]string)
@@ -68,33 +70,44 @@ func NewFilter(expression string) FilterT {
 			}
 		}
 
-		if matched, found := result["matched"]; found {
-
-			f := fc.Factory()
-			if fc.IsSimple {
-				matcher, err := newMatcher(regexpOperator)
-				if err != nil {
-					f.init("", nil, expression, true, expression)
-				} else {
-					f.init("", &matcher, expression, true, expression)
-				}
-				return f
-			} else if operator, found := result["operator"]; found {
-				var err error
-				matcher, err := newMatcher(operator)
-				if err != nil {
-					f.init(matched, nil, expression, false, "")
-				} else {
-					if value, found := result["value"]; found {
-						f.init(matched, &matcher, expression, true, value)
-						return f
-					}
-					f.init(matched, &matcher, expression, false, "")
-				}
+		matched, found := result["matched"]
+		if !found && fc.IsSimple {
+			matcher, err := newMatcher(regexpOperator)
+			if err != nil {
+				f.init("", nil, expression, false, expression)
+			} else {
+				f.init("", &matcher, expression, true, expression)
 			}
+			return f
 		}
+		if !found {
+			continue
+		}
+		if value, ok := result["value"]; !ok || value == "" {
+			// value group not found in the expression
+			// example: 'label=''
+			return &invalid
+		}
+		operator, found := result["operator"]
+		if !found {
+			// used operator is not supported by the filter
+			// example: @limit=~0
+			return &invalid
+		}
+		if !stringInSlice(fc.SupportedOperators, operator) {
+			return &invalid
+		}
+		matcher, err := newMatcher(operator)
+		if err != nil {
+			f.init(matched, nil, expression, false, "")
+		} else {
+			if value, found := result["value"]; found {
+				f.init(matched, &matcher, expression, true, value)
+				return f
+			}
+			f.init(matched, &matcher, expression, false, "")
+		}
+
 	}
-	f := alwaysInvalidFilter{}
-	f.init("", nil, expression, false, expression)
-	return &f
+	return &invalid
 }

filters/filter_test.go

@@ -47,6 +47,22 @@ var tests = []filterTest{
 		Expression: "@silenced=xx",
 		IsValid:    false,
 	},
+	filterTest{
+		Expression: "@silenced=:xx",
+		IsValid:    false,
+	},
+	filterTest{
+		Expression: "@silenced==xx",
+		IsValid:    false,
+	},
+	filterTest{
+		Expression: "@silenced=~true",
+		IsValid:    false,
+	},
+	filterTest{
+		Expression: "@silenced=~false",
+		IsValid:    false,
+	},
 
 	filterTest{
 		Expression: "@inhibited=true",
@@ -82,6 +98,22 @@ var tests = []filterTest{
 		Expression: "@inhibited=xx",
 		IsValid:    false,
 	},
+	filterTest{
+		Expression: "@inhibited=:xx",
+		IsValid:    false,
+	},
+	filterTest{
+		Expression: "@inhibited==xx",
+		IsValid:    false,
+	},
+	filterTest{
+		Expression: "@inhibited=~true",
+		IsValid:    false,
+	},
+	filterTest{
+		Expression: "@inhibited=~false",
+		IsValid:    false,
+	},
 
 	filterTest{
 		Expression: "@silence_jira=1",
@@ -132,6 +164,27 @@ var tests = []filterTest{
 		Silence:    models.Silence{ID: "1", JiraID: "xxx"},
 		IsMatch:    false,
 	},
+	filterTest{
+		Expression: "@silence_jira=~",
+		IsValid:    false,
+		Alert:      models.Alert{Silenced: "1"},
+		Silence:    models.Silence{ID: "1", JiraID: "xxx"},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@silence_jira~=",
+		IsValid:    false,
+		Alert:      models.Alert{Silenced: "1"},
+		Silence:    models.Silence{ID: "1", JiraID: "xxx"},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@silence_jira~=1",
+		IsValid:    false,
+		Alert:      models.Alert{Silenced: "1"},
+		Silence:    models.Silence{ID: "1", JiraID: "xxx"},
+		IsMatch:    false,
+	},
 
 	filterTest{
 		Expression: "@silence_author=john",
@@ -168,6 +221,27 @@ var tests = []filterTest{
 		Silence:    models.Silence{ID: "1"},
 		IsMatch:    true,
 	},
+	filterTest{
+		Expression: "@silence_author=~",
+		IsValid:    false,
+		Alert:      models.Alert{Silenced: "1"},
+		Silence:    models.Silence{ID: "1"},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@silence_author===x",
+		IsValid:    false,
+		Alert:      models.Alert{Silenced: "1"},
+		Silence:    models.Silence{ID: "1"},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@silence_author=!!xxx",
+		IsValid:    false,
+		Alert:      models.Alert{Silenced: "1"},
+		Silence:    models.Silence{ID: "1"},
+		IsMatch:    false,
+	},
 
 	filterTest{
 		Expression: "@age<1h",
@@ -193,6 +267,42 @@ var tests = []filterTest{
 		Alert:      models.Alert{StartsAt: time.Now().Add(time.Hour * -2)},
 		IsMatch:    true,
 	},
+	filterTest{
+		Expression: "@age=1h",
+		IsValid:    false,
+		Alert:      models.Alert{StartsAt: time.Now().Add(time.Minute * -55)},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@age=~1h",
+		IsValid:    false,
+		Alert:      models.Alert{StartsAt: time.Now().Add(time.Minute * -55)},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@age>",
+		IsValid:    false,
+		Alert:      models.Alert{StartsAt: time.Now().Add(time.Minute * -55)},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@age<",
+		IsValid:    false,
+		Alert:      models.Alert{StartsAt: time.Now().Add(time.Minute * -55)},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@age>a",
+		IsValid:    false,
+		Alert:      models.Alert{StartsAt: time.Now().Add(time.Minute * -55)},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "@age<10v",
+		IsValid:    false,
+		Alert:      models.Alert{StartsAt: time.Now().Add(time.Minute * -55)},
+		IsMatch:    false,
+	},
 
 	filterTest{
 		Expression: "node=vps1",
@@ -236,6 +346,24 @@ var tests = []filterTest{
 		Alert:      models.Alert{Labels: map[string]string{"node": "vps1"}},
 		IsMatch:    true,
 	},
+	filterTest{
+		Expression: "node!~",
+		IsValid:    false,
+		Alert:      models.Alert{Labels: map[string]string{"node": "vps1"}},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "node=",
+		IsValid:    false,
+		Alert:      models.Alert{Labels: map[string]string{"node": "vps1"}},
+		IsMatch:    false,
+	},
+	filterTest{
+		Expression: "node===",
+		IsValid:    false,
+		Alert:      models.Alert{Labels: map[string]string{"node": "vps1"}},
+		IsMatch:    false,
+	},
 
 	filterTest{
 		Expression: "abc",
@@ -386,6 +514,18 @@ var limitTests = []limitFilterTest{
 		Expression: "@limit=abc",
 		IsValid:    false,
 	},
+	limitFilterTest{
+		Expression: "@limit==0",
+		IsValid:    false,
+	},
+	limitFilterTest{
+		Expression: "@limit>0",
+		IsValid:    false,
+	},
+	limitFilterTest{
+		Expression: "@limit<0",
+		IsValid:    false,
+	},
 }
 
 func TestLimitFilter(t *testing.T) {

filters/registry.go

@@ -9,6 +9,12 @@ const (
 	negativeRegexOperator string = "!~"
 )
 
+// this needs to be hand crafted because any of the supported operator chars
+// should be considered part of the operator expression
+// this is needed to catch errors in operators, for example:
+// a===b should yield an error
+var matcherRegex = "[=!<>~]+"
+
 var matcherConfig = map[string]matcherT{
 	equalOperator:         &equalMatcher{},
 	notEqualOperator:      &notEqualMatcher{},

filters/slices.go

@@ -0,0 +1,10 @@
+package filters
+
+func stringInSlice(stringArray []string, value string) bool {
+	for _, s := range stringArray {
+		if s == value {
+			return true
+		}
+	}
+	return false
+}