[query] Fix step parsing (#1617)

src/query/api/v1/handler/prometheus/native/common.go

@@ -53,6 +53,9 @@ const (
 	blockTypeParam    = "block-type"
 
 	formatErrStr = "error parsing param: %s, error: %v"
+
+	maxInt64 = float64(math.MaxInt64)
+	minInt64 = float64(math.MinInt64)
 )
 
 func parseTime(r *http.Request, key string) (time.Time, error) {
@@ -75,9 +78,14 @@ func parseDuration(r *http.Request, key string) (time.Duration, error) {
 		return value, nil
 	}
 
-	// Try parsing as an integer value specifying seconds, the Prometheus default
-	if seconds, intErr := strconv.ParseInt(str, 10, 64); intErr == nil {
-		return time.Duration(seconds) * time.Second, nil
+	// Try parsing as a float value specifying seconds, the Prometheus default
+	if seconds, floatErr := strconv.ParseFloat(str, 64); floatErr == nil {
+		ts := seconds * float64(time.Second)
+		if ts > maxInt64 || ts < minInt64 {
+			return 0, fmt.Errorf("cannot parse %s to a valid duration: int64 overflow", str)
+		}
+
+		return time.Duration(ts), nil
 	}
 
 	return 0, err
@@ -111,6 +119,10 @@ func parseParams(r *http.Request, timeoutOpts *prometheus.TimeoutOpts) (models.R
 	if err != nil {
 		return params, xhttp.NewParseError(fmt.Errorf(formatErrStr, stepParam, err), http.StatusBadRequest)
 	}
+	if step <= 0 {
+		err := fmt.Errorf("expected postive step size, instead got: %d", step)
+		return params, xhttp.NewParseError(fmt.Errorf(formatErrStr, stepParam, err), http.StatusBadRequest)
+	}
 	params.Step = step
 
 	query, err := parseQuery(r)

src/query/api/v1/handler/prometheus/native/common_test.go

@@ -23,6 +23,7 @@ package native
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"math"
 	"net/http"
 	"net/url"
@@ -104,6 +105,17 @@ func TestInvalidTarget(t *testing.T) {
 	require.Equal(t, err.Code(), http.StatusBadRequest)
 }
 
+func TestInvalidStep(t *testing.T) {
+	req, _ := http.NewRequest("GET", PromReadURL, nil)
+	vals := defaultParams()
+	vals.Del(stepParam)
+	vals.Add(stepParam, "-10.50s")
+	req.URL.RawQuery = vals.Encode()
+	_, err := parseParams(req, timeoutOpts)
+	require.NotNil(t, err, "unable to parse request")
+	require.Equal(t, err.Code(), http.StatusBadRequest)
+}
+
 func TestParseDuration(t *testing.T) {
 	r, err := http.NewRequest(http.MethodGet, "/foo?step=10s", nil)
 	require.NoError(t, err)
@@ -112,6 +124,20 @@ func TestParseDuration(t *testing.T) {
 	assert.Equal(t, 10*time.Second, v)
 }
 
+func TestParseFloatDuration(t *testing.T) {
+	r, err := http.NewRequest(http.MethodGet, "/foo?step=10.50m", nil)
+	require.NoError(t, err)
+	v, err := parseDuration(r, stepParam)
+	require.NoError(t, err)
+	assert.Equal(t, 10*time.Minute+30*time.Second, v)
+
+	r, err = http.NewRequest(http.MethodGet, "/foo?step=10.00m", nil)
+	require.NoError(t, err)
+	v, err = parseDuration(r, stepParam)
+	require.NoError(t, err)
+	assert.Equal(t, 10*time.Minute, v)
+}
+
 func TestParseDurationParsesIntAsSeconds(t *testing.T) {
 	r, err := http.NewRequest(http.MethodGet, "/foo?step=30", nil)
 	require.NoError(t, err)
@@ -120,13 +146,28 @@ func TestParseDurationParsesIntAsSeconds(t *testing.T) {
 	assert.Equal(t, 30*time.Second, v)
 }
 
+func TestParseDurationParsesFloatAsSeconds(t *testing.T) {
+	r, err := http.NewRequest(http.MethodGet, "/foo?step=30.00", nil)
+	require.NoError(t, err)
+	v, err := parseDuration(r, stepParam)
+	require.NoError(t, err)
+	assert.Equal(t, 30*time.Second, v)
+}
+
 func TestParseDurationError(t *testing.T) {
 	r, err := http.NewRequest(http.MethodGet, "/foo?step=bar10", nil)
 	require.NoError(t, err)
 	_, err = parseDuration(r, stepParam)
 	assert.Error(t, err)
 }
 
+func TestParseDurationOverflowError(t *testing.T) {
+	r, err := http.NewRequest(http.MethodGet, fmt.Sprintf("/foo?step=%f", float64(math.MaxInt64)), nil)
+	require.NoError(t, err)
+	_, err = parseDuration(r, stepParam)
+	assert.Error(t, err)
+}
+
 func TestParseBlockType(t *testing.T) {
 	logging.InitWithCores(nil)