From 7bf0b9d5d0f04df52e35a7a363b1a003a71d7487 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Fri, 5 May 2017 22:27:21 +0100 Subject: [PATCH 1/2] More strict filter validation @silenced=true should now be marked as invalid filter, but it isn't, this commit refactors it so that instead of looping over all filters and falling back to fuzzy (which is very relaxed checking and can't find typos in filter names like @statuz=active) we tokenize the expression into ' ' and only fallback to fuzzy if there's only a static text (no filter name and operator). This is more manual but provides stronger checks so now @silenced=true is correctly marked as invalid --- filters/filter.go | 77 +++++++++++++++++++++++------------------- filters/filter_test.go | 24 +++++++++++++ filters/registry.go | 8 ++--- 3 files changed, 69 insertions(+), 40 deletions(-) diff --git a/filters/filter.go b/filters/filter.go index abd071dd6..8d186bc3d 100644 --- a/filters/filter.go +++ b/filters/filter.go @@ -57,43 +57,50 @@ func NewFilter(expression string) FilterT { invalid := alwaysInvalidFilter{} invalid.init("", nil, expression, false, expression) + reExp := fmt.Sprintf("^(?P(%s))(?P(%s))(?P(.*))", filterRegex, matcherRegex) + re := regexp.MustCompile(reExp) + match := re.FindStringSubmatch(expression) + result := make(map[string]string) + for i, name := range re.SubexpNames() { + if name != "" && i > 0 && i <= len(match) { + result[name] = match[i] + } + } + + matched, _ := result["matched"] + operator, _ := result["operator"] + value, _ := result["value"] + + if matched == "" && operator == "" && value == "" { + // no "filter=" part, just the value, use fuzzy filter + f := newFuzzyFilter() + matcher, err := newMatcher(regexpOperator) + if err != nil { + f.init("", nil, expression, false, expression) + } else { + f.init("", &matcher, expression, true, expression) + } + return f + } + + if value == "" { + // there's no value, so it's always invalid + return &invalid + } + + if operator == "" { + // no operator, no valid filter here + return &invalid + } + + // we have "filter=" part, lookup filter that matches for _, fc := range AllFilters { f := fc.Factory() - - reExp := fmt.Sprintf("^(?P(%s))(?P(%s))(?P(.*))", fc.Label, matcherRegex) - re := regexp.MustCompile(reExp) - match := re.FindStringSubmatch(expression) - result := make(map[string]string) - for i, name := range re.SubexpNames() { - if name != "" && i > 0 && i <= len(match) { - result[name] = match[i] - } - } - - 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 { + labelRe := regexp.MustCompile("^(?:" + fc.Label + ")$") + if !labelRe.MatchString(matched) { + // filter name doesn't match, keep searching 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 } @@ -101,13 +108,13 @@ func NewFilter(expression string) FilterT { if err != nil { f.init(matched, nil, expression, false, "") } else { - if value, found := result["value"]; found { + if value != "" { f.init(matched, &matcher, expression, true, value) return f } f.init(matched, &matcher, expression, false, "") } - } + return &invalid } diff --git a/filters/filter_test.go b/filters/filter_test.go index 21ad16074..bd4de02ce 100644 --- a/filters/filter_test.go +++ b/filters/filter_test.go @@ -406,6 +406,30 @@ var tests = []filterTest{ Expression: "^abb[****].*****", IsValid: false, }, + filterTest{ + Expression: "@silenced=true", + IsValid: false, + }, + filterTest{ + Expression: "@silenced!=false", + IsValid: false, + }, + filterTest{ + Expression: "@silenced=~false", + IsValid: false, + }, + filterTest{ + Expression: "@inhibited=true", + IsValid: false, + }, + filterTest{ + Expression: "@inhibited!=false", + IsValid: false, + }, + filterTest{ + Expression: "@inhibited=~false", + IsValid: false, + }, } func TestFilters(t *testing.T) { diff --git a/filters/registry.go b/filters/registry.go index 606f5b7da..e8d087cf3 100644 --- a/filters/registry.go +++ b/filters/registry.go @@ -15,6 +15,9 @@ const ( // a===b should yield an error var matcherRegex = "[=!<>~]+" +// same as matcherRegex but for the filter name part +var filterRegex = "^(@)?[a-zA-Z_][a-zA-Z0-9_]*" + var matcherConfig = map[string]matcherT{ equalOperator: &equalMatcher{}, notEqualOperator: ¬EqualMatcher{}, @@ -25,7 +28,6 @@ var matcherConfig = map[string]matcherT{ } type filterConfig struct { - IsSimple bool Label string SupportedOperators []string Factory newFilterFactory @@ -71,8 +73,4 @@ var AllFilters = []filterConfig{ Factory: newLabelFilter, Autocomplete: labelAutocomplete, }, - filterConfig{ - IsSimple: true, - Factory: newFuzzyFilter, - }, } From 003010f73b21b155be26fc183e29d602d7412ebd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Fri, 5 May 2017 22:36:50 +0100 Subject: [PATCH 2/2] Speed up filter matches by re-using global match object Instead of compiling filter regexp on every filter match pre-compile it and use same instance --- filters/filter.go | 3 +-- filters/registry.go | 9 +++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/filters/filter.go b/filters/filter.go index 8d186bc3d..77ccb2c41 100644 --- a/filters/filter.go +++ b/filters/filter.go @@ -96,8 +96,7 @@ func NewFilter(expression string) FilterT { // we have "filter=" part, lookup filter that matches for _, fc := range AllFilters { f := fc.Factory() - labelRe := regexp.MustCompile("^(?:" + fc.Label + ")$") - if !labelRe.MatchString(matched) { + if !fc.LabelRe.MatchString(matched) { // filter name doesn't match, keep searching continue } diff --git a/filters/registry.go b/filters/registry.go index e8d087cf3..88073ceca 100644 --- a/filters/registry.go +++ b/filters/registry.go @@ -1,5 +1,7 @@ package filters +import "regexp" + const ( equalOperator string = "=" notEqualOperator string = "!=" @@ -29,6 +31,7 @@ var matcherConfig = map[string]matcherT{ type filterConfig struct { Label string + LabelRe *regexp.Regexp SupportedOperators []string Factory newFilterFactory Autocomplete autocompleteFactory @@ -39,36 +42,42 @@ type filterConfig struct { var AllFilters = []filterConfig{ filterConfig{ Label: "@status", + LabelRe: regexp.MustCompile("^@status$"), SupportedOperators: []string{equalOperator, notEqualOperator}, Factory: newstatusFilter, Autocomplete: statusAutocomplete, }, filterConfig{ Label: "@age", + LabelRe: regexp.MustCompile("^@age$"), SupportedOperators: []string{lessThanOperator, moreThanOperator}, Factory: newAgeFilter, Autocomplete: ageAutocomplete, }, filterConfig{ Label: "@silence_jira", + LabelRe: regexp.MustCompile("^@silence_jira$"), SupportedOperators: []string{regexpOperator, negativeRegexOperator, equalOperator, notEqualOperator}, Factory: newSilenceJiraFilter, Autocomplete: sinceJiraIDAutocomplete, }, filterConfig{ Label: "@silence_author", + LabelRe: regexp.MustCompile("^@silence_author$"), SupportedOperators: []string{regexpOperator, negativeRegexOperator, equalOperator, notEqualOperator}, Factory: newSilenceAuthorFilter, Autocomplete: sinceAuthorAutocomplete, }, filterConfig{ Label: "@limit", + LabelRe: regexp.MustCompile("^@limit$"), SupportedOperators: []string{equalOperator}, Factory: newLimitFilter, Autocomplete: limitAutocomplete, }, filterConfig{ Label: "[a-zA-Z_][a-zA-Z0-9_]*", + LabelRe: regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9_]*$"), SupportedOperators: []string{regexpOperator, negativeRegexOperator, equalOperator, notEqualOperator, lessThanOperator, moreThanOperator}, Factory: newLabelFilter, Autocomplete: labelAutocomplete,