From 44deae4f114f4e93a4a74d1b40edcdfd575fd0c8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= <l.mierzwa@gmail.com>
Date: Thu, 20 Apr 2017 13:36:18 -0700
Subject: [PATCH] More strict filter syntax checking

Current filter factory rules are very relaxed, anything that doesn't match filter syntax perfectly is passed down to the fuzzy filter, that happily accepts every input and doesn't complain. This change makes syntax checks more strict, we explicitly look for roughly <anything><any of the chars used as operators><anything> (in reality we match the first <anything> used filter specific regex, but that can still be any regex) and only if evaluated filter expression doesn't match this regex we pass it to the fuzzy filter. Once we have a match we check if operator is supported by the filter and we're more strict there, in case of a===b we currently accept = as the operator and ==b as the value, now operator will be === (as we match for any of the operator chars in use) and fail once that doesn't match.
---
 filters/filter.go   | 73 ++++++++++++++++++++++++++-------------------
 filters/registry.go |  6 ++++
 filters/slices.go   | 10 +++++++
 3 files changed, 59 insertions(+), 30 deletions(-)
 create mode 100644 filters/slices.go

diff --git a/filters/filter.go b/filters/filter.go
index 403a33fa8..abd071dd6 100644
--- a/filters/filter.go
+++ b/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
 }
diff --git a/filters/registry.go b/filters/registry.go
index 8fbf2bc69..de3bce4bb 100644
--- a/filters/registry.go
+++ b/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{},
diff --git a/filters/slices.go b/filters/slices.go
new file mode 100644
index 000000000..218e4d991
--- /dev/null
+++ b/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
+}