Bugfix: catch error from ParseConditions and refactor ParseConditions

Signed-off-by: Zhengyi Lai <zheng1@yunify.com>

pkg/kapis/openpitrix/v1/handler.go

@@ -867,6 +867,12 @@ func (h *openpitrixHandler) ListRepos(req *restful.Request, resp *restful.Respon
 	reverse := params.GetBoolValueWithDefault(req, params.ReverseParam, false)
 	conditions, err := params.ParseConditions(req)
 
+	if err != nil {
+		klog.V(4).Infoln(err)
+		api.HandleBadRequest(resp, nil, err)
+		return
+	}
+
 	if req.PathParameter("workspace") != "" {
 		conditions.Match[openpitrix.WorkspaceLabel] = req.PathParameter("workspace")
 	}

pkg/server/params/params.go

@@ -43,39 +43,51 @@ func ParsePaging(req *restful.Request) (limit, offset int) {
 	return
 }
 
-func ParseConditions(req *restful.Request) (*Conditions, error) {
+var (
+	invalidKeyRegex = regexp.MustCompile(`[\s(){}\[\]]`)
+)
 
-	conditionsStr := req.QueryParameter(ConditionsParam)
+// Ref: stdlib url.ParseQuery
+func parseConditions(conditionsStr string) (*Conditions, error) {
+	// string likes: key1=value1,key2~value2,key3=
+	// exact query: key=value, if value is empty means label value must be ""
+	// fuzzy query: key~value, if value is empty means label value is "" or label key not exist
+	var conditions = &Conditions{Match: make(map[string]string, 0), Fuzzy: make(map[string]string, 0)}
 
-	conditions := &Conditions{Match: make(map[string]string, 0), Fuzzy: make(map[string]string, 0)}
-
-	if conditionsStr == "" {
-		return conditions, nil
-	}
-
-	// ?conditions=key1=value1,key2~value2,key3=
-	for _, item := range strings.Split(conditionsStr, ",") {
-		// exact query: key=value, if value is empty means label value must be ""
-		// fuzzy query: key~value, if value is empty means label value is "" or label key not exist
-		if groups := regexp.MustCompile(`(\S+)([=~])(\S+)?`).FindStringSubmatch(item); len(groups) >= 3 {
-			value := ""
-
-			if len(groups) > 3 {
-				value = groups[3]
-			}
-
-			if groups[2] == "=" {
-				conditions.Match[groups[1]] = value
-			} else {
-				conditions.Fuzzy[groups[1]] = value
-			}
+	for conditionsStr != "" {
+		key := conditionsStr
+		if i := strings.Index(key, ","); i >= 0 {
+			key, conditionsStr = key[:i], key[i+1:]
 		} else {
+			conditionsStr = ""
+		}
+		if key == "" {
+			continue
+		}
+		value := ""
+		var isFuzzy = false
+		if i := strings.IndexAny(key, "~="); i >= 0 {
+			if key[i] == '~' {
+				isFuzzy = true
+			}
+			key, value = key[:i], key[i+1:]
+		}
+		if invalidKeyRegex.MatchString(key) {
 			return nil, fmt.Errorf("invalid conditions")
 		}
+		if isFuzzy {
+			conditions.Fuzzy[key] = value
+		} else {
+			conditions.Match[key] = value
+		}
 	}
 	return conditions, nil
 }
 
+func ParseConditions(req *restful.Request) (*Conditions, error) {
+	return parseConditions(req.QueryParameter(ConditionsParam))
+}
+
 type Conditions struct {
 	Match map[string]string
 	Fuzzy map[string]string

pkg/server/params/params_test.go

@@ -0,0 +1,173 @@
+/*
+Copyright 2019 The KubeSphere Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package params
+
+import (
+	"net/http"
+	"net/url"
+	"reflect"
+	"testing"
+
+	"github.com/emicklei/go-restful"
+)
+
+func TestParseConditions(t *testing.T) {
+	type args struct {
+		req *restful.Request
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    *Conditions
+		wantErr bool
+	}{
+		{
+			"good case 1",
+			args{&restful.Request{Request: &http.Request{URL: &url.URL{
+				RawQuery: "conditions=status%3Ddraft%7Cactive%7Csuspended%7Cpassed",
+			}}}},
+			&Conditions{
+				Match: map[string]string{
+					"status": "draft|active|suspended|passed",
+				},
+				Fuzzy: map[string]string{},
+			},
+			false,
+		},
+		{
+			"good case 2",
+			args{&restful.Request{Request: &http.Request{URL: &url.URL{
+				RawQuery: "conditions=status1%3Ddraft%7Cactive%7Csuspended%7Cpassed,status2~draft%7Cactive,status3",
+			}}}},
+			&Conditions{
+				Match: map[string]string{
+					"status1": "draft|active|suspended|passed",
+					"status3": "",
+				},
+				Fuzzy: map[string]string{
+					"status2": "draft|active",
+				},
+			},
+			false,
+		},
+		{
+			"good case 3",
+			args{&restful.Request{Request: &http.Request{URL: &url.URL{
+				RawQuery: "conditions=status%3Ddraft%7Cactive%7Csuspended%7Cpassed%28%29,",
+			}}}},
+			&Conditions{
+				Match: map[string]string{
+					"status": "draft|active|suspended|passed()",
+				},
+				Fuzzy: map[string]string{},
+			},
+			false,
+		},
+
+		{
+			"bad case 1",
+			args{&restful.Request{Request: &http.Request{URL: &url.URL{
+				RawQuery: "conditions=%28select+status%3D%29",
+			}}}},
+			nil,
+			true,
+		},
+		{
+			"bad case 2",
+			args{&restful.Request{Request: &http.Request{URL: &url.URL{
+				RawQuery: "conditions=%28select+status%3D%2C",
+			}}}},
+			nil,
+			true,
+		},
+		{
+			"bad case 3",
+			args{&restful.Request{Request: &http.Request{URL: &url.URL{
+				RawQuery: "conditions=status%3D%2C%28select+status%3D%29",
+			}}}},
+			nil,
+			true,
+		},
+		{
+			"bad case 4",
+			args{&restful.Request{Request: &http.Request{URL: &url.URL{
+				RawQuery: "conditions=%28select+status%3Ddraft%7Cactive%7Csuspended%7Cpassed%29",
+			}}}},
+			nil,
+			true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParseConditions(tt.args.req)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ParseConditions() error = %+v, wantErr %+v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("ParseConditions() = %+v, want %+v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_parseConditions(t *testing.T) {
+	type args struct {
+		conditionsStr string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    *Conditions
+		wantErr bool
+	}{
+		{
+			"good case 1",
+			args{"key1=value1,key2~value2,key3=,key4~,key5"},
+			&Conditions{
+				Match: map[string]string{
+					"key1": "value1",
+					"key3": "",
+					"key5": "",
+				},
+				Fuzzy: map[string]string{
+					"key2": "value2",
+					"key4": "",
+				},
+			},
+			false,
+		},
+		{
+			"bad case 1",
+			args{"key1 error=value1,key2~value2,key3=,key4~,key5"},
+			nil,
+			true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := parseConditions(tt.args.conditionsStr)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("parseConditions() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("parseConditions() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}