Query API should return HTTP 400 if query does not parse

Fixes #1081 curl -G -v "http://localhost:8181/v1/query" --data-urlencode "q=^ -i" * Trying ::1... * TCP_NODELAY set * Connected to localhost (::1) port 8181 (#0) > GET /v1/query?q=%5E%20-i HTTP/1.1 > Host: localhost:8181 > User-Agent: curl/7.54.0 > Accept: */* > < HTTP/1.1 400 Bad Request < Content-Type: application/json < Date: Sat, 15 Dec 2018 09:13:58 GMT < Content-Length: 118 < { "code": "invalid_parameter", "message": "1 error occurred: 1:1: rego_parse_error: no match found\n\t^ -i\n\t^" * Connection #0 to host localhost left intact }% Signed-off-by: repenno <rapenno@gmail.com> Query API should return HTTP 400 if query does not parse Fixes #1081 curl -G -v "http://localhost:8181/v1/query" --data-urlencode "q=^ -i" * Trying ::1... * TCP_NODELAY set * Connected to localhost (::1) port 8181 (#0) > GET /v1/query?q=%5E%20-i HTTP/1.1 > Host: localhost:8181 > User-Agent: curl/7.54.0 > Accept: */* > < HTTP/1.1 400 Bad Request < Content-Type: application/json < Date: Sat, 15 Dec 2018 09:13:58 GMT < Content-Length: 118 < { "code": "invalid_parameter", "message": "1 error occurred: 1:1: rego_parse_error: no match found\n\t^ -i\n\t^" * Connection #0 to host localhost left intact }% Signed-off-by: repenno <rapenno@gmail.com> Query API should return HTTP 400 if query does not parse Fixes #1081 Reworked fix based on comments and current code. v1QueryGet/Post already call ast.parseBody through validateQuery, therefore the solution: - Avoids double parsing query - Keeps original query string around so logs can work - Performs http error checking at the server.go level - Enhanced testing to make sure all messages are sane Optionally error checking in server.go could be factored in a unified function. Signed-off-by: repenno <rapenno@gmail.com> Query API should return HTTP 400 if query does not parse Fixes #1081 Reworked fix based on comments and current code. v1QueryGet/Post already call ast.parseBody through validateQuery, therefore the solution: - Avoids double parsing query - Keeps original query string around so logs can work - Performs http error checking at the server.go level - Enhanced testing to make sure all messages are sane Optionally error checking in server.go could be factored in a unified function. Signed-off-by: repenno <rapenno@gmail.com> Query API should return HTTP 400 if query does not parse Fixes #1081 Reworked fix based on comments and current code. v1QueryGet/Post already call ast.parseBody through validateQuery, therefore the solution: - Avoids double parsing query - Keeps original query string around so logs can work - Performs http error checking at the server.go level - Enhanced testing to make sure all messages are sane - Fixed existing server_test.go tests affected by requested changes Optionally error checking in server.go could be factored in a unified function. Signed-off-by: repenno <rapenno@gmail.com>
2026-08-12 19:32:48 -06:00 · 2018-12-15 01:21:15 -08:00
parent adba37c3cc
commit e2ef042d3d
2 changed files with 67 additions and 19 deletions
@@ -415,7 +415,7 @@ func (s *Server) initRouter() {
 	s.Handler = router
 }

-func (s *Server) execQuery(ctx context.Context, r *http.Request, query string, input ast.Value, explainMode types.ExplainModeV1, includeMetrics, includeInstrumentation, pretty bool) (results types.QueryResponseV1, err error) {
+func (s *Server) execQuery(ctx context.Context, r *http.Request, parsedQuery ast.Body, input ast.Value, explainMode types.ExplainModeV1, includeMetrics, includeInstrumentation, pretty bool) (results types.QueryResponseV1, err error) {

 	diagLogger := s.evalDiagnosticPolicy(r)

@@ -436,7 +436,7 @@ func (s *Server) execQuery(ctx context.Context, r *http.Request, query string, i
 	rego := rego.New(
 		rego.Store(s.store),
 		rego.Compiler(compiler),
-		rego.Query(query),
+		rego.ParsedQuery(parsedQuery),
 		rego.ParsedInput(input),
 		rego.Metrics(m),
 		rego.Instrument(instrument),
@@ -446,7 +446,7 @@ func (s *Server) execQuery(ctx context.Context, r *http.Request, query string, i

 	output, err := rego.Eval(ctx)
 	if err != nil {
-		diagLogger.Log(ctx, "", r.RemoteAddr, query, input, nil, err, m, buf)
+		diagLogger.Log(ctx, "", r.RemoteAddr, parsedQuery.String(), input, nil, err, m, buf)
 		return results, err
 	}

@@ -463,7 +463,7 @@ func (s *Server) execQuery(ctx context.Context, r *http.Request, query string, i
 	}

 	var x interface{} = results.Result
-	diagLogger.Log(ctx, "", r.RemoteAddr, query, input, &x, nil, m, buf)
+	diagLogger.Log(ctx, "", r.RemoteAddr, parsedQuery.String(), input, &x, nil, m, buf)
 	return results, nil
 }

@@ -500,7 +500,9 @@ func (s *Server) indexGet(w http.ResponseWriter, r *http.Request) {
 		input = t.Value
 	}

-	results, err := s.execQuery(ctx, r, qStr, input, explainMode, false, false, true)
+	_, parsedQuery, _ := validateQuery(qStr)
+
+	results, err := s.execQuery(ctx, r, parsedQuery, input, explainMode, false, false, true)
 	if err != nil {
 		renderQueryResult(w, nil, err, t0)
 		return
@@ -1368,10 +1370,16 @@ func (s *Server) v1QueryGet(w http.ResponseWriter, r *http.Request) {
 	}
 	qStr := qStrs[len(qStrs)-1]

-	unsafeBuiltins, err := validateQuery(qStr)
+	unsafeBuiltins, parsedQuery, err := validateQuery(qStr)
 	if err != nil {
-		writer.ErrorAuto(w, err)
-		return
+		switch err := err.(type) {
+		case ast.Errors:
+			writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, types.MsgParseQueryError).WithASTErrors(err))
+			return
+		default:
+			writer.ErrorAuto(w, err)
+			return
+		}
 	} else if len(unsafeBuiltins) > 0 {
 		writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, "unsafe built-in function calls in query: %v", strings.Join(unsafeBuiltins, ",")))
 		return
@@ -1388,7 +1396,7 @@ func (s *Server) v1QueryGet(w http.ResponseWriter, r *http.Request) {
 	includeMetrics := getBoolParam(r.URL, types.ParamMetricsV1, true)
 	includeInstrumentation := getBoolParam(r.URL, types.ParamInstrumentV1, true)

-	results, err := s.execQuery(ctx, r, qStr, nil, explainMode, includeMetrics, includeInstrumentation, pretty)
+	results, err := s.execQuery(ctx, r, parsedQuery, nil, explainMode, includeMetrics, includeInstrumentation, pretty)
 	if err != nil {
 		switch err := err.(type) {
 		case ast.Errors:
@@ -1412,10 +1420,16 @@ func (s *Server) v1QueryPost(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	qStr := request.Query
-	unsafeBuiltins, err := validateQuery(qStr)
+	unsafeBuiltins, parsedQuery, err := validateQuery(qStr)
 	if err != nil {
-		writer.ErrorAuto(w, err)
-		return
+		switch err := err.(type) {
+		case ast.Errors:
+			writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, types.MsgParseQueryError).WithASTErrors(err))
+			return
+		default:
+			writer.ErrorAuto(w, err)
+			return
+		}
 	} else if len(unsafeBuiltins) > 0 {
 		writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, "unsafe built-in function calls in query: %v", strings.Join(unsafeBuiltins, ",")))
 		return
@@ -1432,7 +1446,7 @@ func (s *Server) v1QueryPost(w http.ResponseWriter, r *http.Request) {
 	includeMetrics := getBoolParam(r.URL, types.ParamMetricsV1, true)
 	includeInstrumentation := getBoolParam(r.URL, types.ParamInstrumentV1, true)

-	results, err := s.execQuery(ctx, r, qStr, nil, explainMode, includeMetrics, includeInstrumentation, pretty)
+	results, err := s.execQuery(ctx, r, parsedQuery, nil, explainMode, includeMetrics, includeInstrumentation, pretty)
 	if err != nil {
 		switch err := err.(type) {
 		case ast.Errors:
@@ -1740,15 +1754,16 @@ func stringPathToRef(s string) (r ast.Ref) {
 	return r
 }

-func validateQuery(query string) ([]string, error) {
+func validateQuery(query string) ([]string, ast.Body, error) {

 	var body ast.Body
 	body, err := ast.ParseBody(query)
 	if err != nil {
-		return []string{}, err
+		return []string{}, nil, err
 	}

-	return validateParsedQuery(body)
+	unsafeOperators, nil := validateParsedQuery(body)
+	return unsafeOperators, body, nil
 }

 func validateParsedQuery(body ast.Body) ([]string, error) {
@@ -1918,7 +1918,7 @@ func TestDiagnostics(t *testing.T) {
 			metrics: true,
 		},
 		{
-			query:   "a=data.x",
+			query:   "a = data.x",
 			result:  &expMap1,
 			metrics: true,
 		},
@@ -1937,14 +1937,14 @@ func TestDiagnostics(t *testing.T) {
 			explainLen: 3,
 		},
 		{
-			query:      "a=data.x",
+			query:      "a = data.x",
 			result:     &expMap1,
 			metrics:    true,
 			instrument: true,
 			explainLen: 5,
 		},
 		{
-			query:  "a=data.y",
+			query:  "a = data.y",
 			result: &expMap2,
 		},
 		{
@@ -2349,6 +2349,39 @@ func TestQueryV1(t *testing.T) {
 	}
 }

+func TestBadQueryV1(t *testing.T) {
+	f := newFixture(t)
+	get := newReqV1(http.MethodGet, `/query?q=^ -i`, "")
+	f.server.Handler.ServeHTTP(f.recorder, get)
+
+	if f.recorder.Code != 400 {
+		t.Fatalf("Expected success but got %v", f.recorder)
+	}
+
+	expectedErr := `{
+  "code": "invalid_parameter",
+  "message": "error(s) occurred while parsing query",
+  "errors": [
+    {
+      "code": "rego_parse_error",
+      "message": "no match found",
+      "location": {
+        "file": "",
+        "row": 1,
+        "col": 1
+      },
+      "details": {}
+    }
+  ]
+}`
+
+	recvErr := f.recorder.Body.String()
+
+	if recvErr != expectedErr {
+		t.Fatalf(`Expected %v but got: %v`, expectedErr, recvErr)
+	}
+}
+
 func TestQueryV1UnsafeBuiltin(t *testing.T) {
 	f := newFixture(t)
 	get := newReqV1(http.MethodGet, `/query?q=http.send({"method": "get", "url": "foo.com"}, x)`, "")