Merge pull request #817 from mchrome/mchr/fix-limit-target-length

fix: check if query doesn't exceed allowed length limit

cmd/carbonapi/config/config.go

@@ -107,7 +107,8 @@ type ConfigType struct {
 	TruncateTimeMap map[time.Duration]time.Duration `mapstructure:"truncateTime"`
 	TruncateTime    []DurationTruncate              `mapstructure:"-" json:"-"` // produce from TruncateTimeMap and sort in reverse order
 
-	CombineMultipleTargetsInOne bool `mapstructure:"combineMultipleTargetsInOne"`
+	MaxQueryLength              uint64 `mapstructure:"maxQueryLength"`
+	CombineMultipleTargetsInOne bool   `mapstructure:"combineMultipleTargetsInOne"`
 
 	ResponseCache cache.BytesCache `mapstructure:"-" json:"-"`
 	BackendCache  cache.BytesCache `mapstructure:"-" json:"-"`

cmd/carbonapi/http/expand_handler.go

@@ -76,6 +76,12 @@ func expandHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if queryLengthLimitExceeded(query, config.Config.MaxQueryLength) {
+		setError(w, &accessLogDetails, "query length limit exceeded", http.StatusBadRequest, uid.String())
+		logAsError = true
+		return
+	}
+
 	var pv3Request pbv3.MultiGlobRequest
 	pv3Request.Metrics = query
 	pv3Request.StartTime = from64

cmd/carbonapi/http/find_handlers.go

@@ -221,6 +221,12 @@ func findHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if queryLengthLimitExceeded(query, config.Config.MaxQueryLength) {
+		setError(w, &accessLogDetails, "query length limit exceeded", http.StatusBadRequest, uid.String())
+		logAsError = true
+		return
+	}
+
 	if format == completerFormat {
 		var replacer = strings.NewReplacer("/", ".")
 		for i := range query {

cmd/carbonapi/http/helper.go

@@ -282,3 +282,23 @@ func timestampTruncate(ts int64, duration time.Duration, durations []config.Dura
 	}
 	return ts
 }
+
+func setError(w http.ResponseWriter, accessLogDetails *carbonapipb.AccessLogDetails, msg string, status int, carbonapiUUID string) {
+	w.Header().Set(ctxHeaderUUID, carbonapiUUID)
+	http.Error(w, http.StatusText(status)+": "+msg, status)
+	accessLogDetails.Reason = msg
+	accessLogDetails.HTTPCode = int32(status)
+}
+
+func queryLengthLimitExceeded(query []string, maxLength uint64) bool {
+	if maxLength > 0 {
+		var queryLengthSum uint64 = 0
+		for _, q := range query {
+			queryLengthSum += uint64(len(q))
+		}
+		if queryLengthSum > maxLength {
+			return true
+		}
+	}
+	return false
+}

cmd/carbonapi/http/helper_test.go

@@ -2,6 +2,7 @@ package http
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
@@ -68,3 +69,55 @@ func Test_timestampTruncate(t *testing.T) {
 		})
 	}
 }
+
+func Test_queryLengthLimitExceeded(t *testing.T) {
+	tests := []struct {
+		query     []string
+		maxLength uint64
+		want      bool
+	}{
+		{
+			query:     []string{"a.b.c.d.e", "a.a.a.a.a.a.a.a.a.a.a.a"},
+			maxLength: 20,
+			want:      true,
+		},
+		{
+			query:     []string{"a.b.c", "a.b.d"},
+			maxLength: 10,
+			want:      false,
+		},
+		{
+			query:     []string{"a.b.c", "a.b.d"},
+			maxLength: 9,
+			want:      true,
+		},
+		{
+			query:     []string{"a.b.c.d.e", "a.a.a.a.a.a.a.a.a.a.a.a"},
+			maxLength: 0,
+			want:      false,
+		},
+		{
+			query:     []string{"a.b.b.c.*", "a.d.e", "a.b.c", "a.b.e", "a.a.a.b", "a.f.s.x.w.g"},
+			maxLength: 30,
+			want:      true,
+		},
+		{
+			query:     []string{"a.b.c.d.e", "a.b.c.d.f", "b.a.*"},
+			maxLength: 10000,
+			want:      false,
+		},
+		{
+			query:     []string{},
+			maxLength: 0,
+			want:      false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("queryLengthLimitExceeded([%s], %d) -> %t", strings.Join(tt.query, ", "), tt.maxLength, tt.want), func(t *testing.T) {
+			if got := queryLengthLimitExceeded(tt.query, tt.maxLength); got != tt.want {
+				t.Errorf("queryLengthLimitExceeded() = %t, want %t", got, tt.want)
+			}
+		})
+	}
+}

cmd/carbonapi/http/render_handler.go

@@ -42,13 +42,6 @@ func cleanupParams(r *http.Request) {
 	r.Form.Del("_t") // Used by jquery.graphite.js
 }
 
-func setError(w http.ResponseWriter, accessLogDetails *carbonapipb.AccessLogDetails, msg string, status int, carbonapiUUID string) {
-	w.Header().Set(ctxHeaderUUID, carbonapiUUID)
-	http.Error(w, http.StatusText(status)+": "+msg, status)
-	accessLogDetails.Reason = msg
-	accessLogDetails.HTTPCode = int32(status)
-}
-
 func getCacheTimeout(logger *zap.Logger, r *http.Request, now32, until32 int64, duration time.Duration, cacheConfig *config.CacheConfig) int32 {
 	if tstr := r.FormValue("cacheTimeout"); tstr != "" {
 		t, err := strconv.Atoi(tstr)
@@ -232,6 +225,12 @@ func renderHandler(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	if queryLengthLimitExceeded(targets, config.Config.MaxQueryLength) {
+		setError(w, accessLogDetails, "total target length limit exceeded", http.StatusBadRequest, uid.String())
+		logAsError = true
+		return
+	}
+
 	if useCache {
 		tc := time.Now()
 		response, err := config.Config.ResponseCache.Get(responseCacheKey)

cmd/carbonapi/http/tags_handler.go

@@ -80,6 +80,12 @@ func tagHandler(w http.ResponseWriter, r *http.Request) {
 	q.Del("pretty")
 	rawQuery := q.Encode()
 
+	if queryLengthLimitExceeded(r.Form["query"], config.Config.MaxQueryLength) {
+		setError(w, accessLogDetails, "query length limit exceeded", http.StatusBadRequest, uuid.String())
+		logAsError = true
+		return
+	}
+
 	// TODO(civil): Implement caching
 	var res []string
 	if strings.HasSuffix(r.URL.Path, "tags") || strings.HasSuffix(r.URL.Path, "tags/") {

cmd/mockbackend/testcases/pr817/carbonapi.yaml

@@ -0,0 +1,61 @@
+listen: "localhost:8081"
+expvar:
+  enabled: true
+  pprofEnabled: false
+  listen: ""
+concurency: 1000
+notFoundStatusCode: 200
+cache:
+   type: "mem"
+   size_mb: 0
+   defaultTimeoutSec: 60
+cpus: 0
+tz: ""
+maxBatchSize: 500
+maxQueryLength: 20
+combineMultipleTargetsInOne: true
+graphite:
+    host: ""
+    interval: "60s"
+    prefix: "carbon.api"
+    pattern: "{prefix}.{fqdn}"
+idleConnections: 10
+pidFile: ""
+upstreams:
+    buckets: 10
+    timeouts:
+        find: "2s"
+        render: "10s"
+        connect: "200ms"
+    concurrencyLimitPerServer: 100
+    keepAliveInterval: "30s"
+    maxIdleConnsPerHost: 100
+    doMultipleRequestsIfSplit: true
+    backendsv2:
+        backends:
+          -
+            groupName: "mock-001"
+            protocol: "auto"
+            lbMethod: "all"
+            maxTries: 3
+            maxBatchSize: 500
+            keepAliveInterval: "10s"
+            concurrencyLimit: 0
+            forceAttemptHTTP2: true
+            maxIdleConnsPerHost: 1000
+            doMultipleRequestsIfSplit: true
+            timeouts:
+                find: "15s"
+                render: "50s"
+                connect: "200ms"
+            servers:
+                - "http://127.0.0.1:9070"
+    graphite09compat: false
+expireDelaySec: 10
+logger:
+    - logger: ""
+      file: "stderr"
+      level: "debug"
+      encoding: "console"
+      encodingTime: "iso8601"
+      encodingDuration: "seconds"

cmd/mockbackend/testcases/pr817/pr817.yaml

@@ -0,0 +1,94 @@
+version: "v1"
+test:
+    apps:
+        - name: "carbonapi"
+          binary: "./carbonapi"
+          args:
+              - "-config"
+              - "./cmd/mockbackend/testcases/pr817/carbonapi.yaml"
+    queries:
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/render?target=a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.b&format=json"
+              expectedResponse:
+                  httpCode: 400
+                  contentType: "text/plain; charset=utf-8"
+                  emptyBody: true 
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/metrics/find?query=a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.*&format=json"
+              expectedResponse:
+                  httpCode: 400
+                  contentType: "text/plain; charset=utf-8"
+                  emptyBody: true 
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/metrics/expand?query=a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.b&format=json"
+              expectedResponse:
+                  httpCode: 400
+                  contentType: "text/plain; charset=utf-8"
+                  emptyBody: true
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/tags/autoComplete/tags?query=a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.b"
+              expectedResponse:
+                  httpCode: 400
+                  contentType: "text/plain; charset=utf-8"
+                  emptyBody: true
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/tags/autoComplete/values?query=a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.b"
+              expectedResponse:
+                  httpCode: 400
+                  contentType: "text/plain; charset=utf-8"
+                  emptyBody: true
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/render/?target=a.b.c&target=a.b.d&format=json"
+              expectedResponse:
+                  httpCode: 200
+                  contentType: "application/json"
+                  expectedResults:
+                          - metrics:
+                                  - target: "a.b.c"
+                                    datapoints: [[0,1],[1,2],[2,3],[2,4],[3,5]]
+                                  - target: "a.b.d"
+                                    datapoints: [[31,1],[10,2],[4,3],[7,4],[3,5]]
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/metrics/find?query=a.b.*&format=json"
+              expectedResponse:
+                  httpCode: 200
+                  contentType: "application/json"
+            # - endpoint: "http://127.0.0.1:8081"
+            #   type: "GET"
+            #   URL: "/metrics/expand?query=a.*&format=json"
+            #   expectedResponse:
+            #       httpCode: 200
+            #       contentType: "application/json"
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/tags/autoComplete/tags?query=a.b.c"
+              expectedResponse:
+                  httpCode: 200
+                  contentType: "application/json"
+            - endpoint: "http://127.0.0.1:8081"
+              type: "GET"
+              URL: "/tags/autoComplete/values?query=a.b.c"
+              expectedResponse:
+                  httpCode: 200
+                  contentType: "application/json"  
+
+listeners:
+  - address: ":9070"
+    expressions:
+      "a.b.c":
+        pathExpression: "a.b.c"
+        data:
+            - metricName: "a.b.c"
+              values: [0,1,2,2,3]
+      "a.b.d":
+        pathExpression: "a.b.d"
+        data:
+            - metricName: "a.b.d"
+              values: [31,10,4,7,3]