Add validation to exclude negative values for limit and offset

ONSdigital · Feb 18, 2020 · 1301d8c · 1301d8c
1 parent 0066333
commit 1301d8c
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 0 deletions.
diff --git a/api/search.go b/api/search.go
@@ -60,6 +60,14 @@ func SearchHandlerFunc(queryBuilder QueryBuilder, elasticSearchClient ElasticSea
 			http.Error(w, "Invalid limit parameter", http.StatusBadRequest)
 			return
 		}
+		if limit < 0 {
+			log.Event(ctx, "numeric search parameter provided with negative value", log.Data{
+				"param": "limit",
+				"value": limitParam,
+			}, log.WARN)
+			http.Error(w, "Invalid limit parameter", http.StatusBadRequest)
+			return
+		}
 
 		offsetParam := paramGet(params, "offset", "0")
 		offset, err := strconv.Atoi(offsetParam)
@@ -71,6 +79,14 @@ func SearchHandlerFunc(queryBuilder QueryBuilder, elasticSearchClient ElasticSea
 			http.Error(w, "Invalid offset parameter", http.StatusBadRequest)
 			return
 		}
+		if offset < 0 {
+			log.Event(ctx, "numeric search parameter provided with negative value", log.Data{
+				"param": "from",
+				"value": offsetParam,
+			}, log.WARN)
+			http.Error(w, "Invalid offset parameter", http.StatusBadRequest)
+			return
+		}
 
 		typesParam := paramGet(params, "content_type", defaultContentTypes)
 

diff --git a/api/search_test.go b/api/search_test.go
@@ -35,6 +35,24 @@ func TestSearchHandlerFunc(t *testing.T) {
 		So(esMock.MultiSearchCalls(), ShouldHaveLength, 0)
 	})
 
+	Convey("Should return BadRequest for negative limit parameter", t, func() {
+		qbMock := newQueryBuilderMock(nil, nil)
+		esMock := newElasticSearcherMock(nil, nil)
+		trMock := newResponseTransformerMock(nil, nil)
+
+		searchHandler := SearchHandlerFunc(qbMock, esMock, trMock)
+
+		req := httptest.NewRequest("GET", "http://localhost:8080/search?limit=-1", nil)
+		resp := httptest.NewRecorder()
+
+		searchHandler.ServeHTTP(resp, req)
+
+		So(resp.Code, ShouldEqual, http.StatusBadRequest)
+		So(resp.Body.String(), ShouldContainSubstring, "Invalid limit parameter")
+		So(qbMock.BuildSearchQueryCalls(), ShouldHaveLength, 0)
+		So(esMock.MultiSearchCalls(), ShouldHaveLength, 0)
+	})
+
 	Convey("Should return BadRequest for invalid offset parameter", t, func() {
 		qbMock := newQueryBuilderMock(nil, nil)
 		esMock := newElasticSearcherMock(nil, nil)
@@ -53,6 +71,24 @@ func TestSearchHandlerFunc(t *testing.T) {
 		So(esMock.MultiSearchCalls(), ShouldHaveLength, 0)
 	})
 
+	Convey("Should return BadRequest for negative offset parameter", t, func() {
+		qbMock := newQueryBuilderMock(nil, nil)
+		esMock := newElasticSearcherMock(nil, nil)
+		trMock := newResponseTransformerMock(nil, nil)
+
+		searchHandler := SearchHandlerFunc(qbMock, esMock, trMock)
+
+		req := httptest.NewRequest("GET", "http://localhost:8080/search?offset=-1", nil)
+		resp := httptest.NewRecorder()
+
+		searchHandler.ServeHTTP(resp, req)
+
+		So(resp.Code, ShouldEqual, http.StatusBadRequest)
+		So(resp.Body.String(), ShouldContainSubstring, "Invalid offset parameter")
+		So(qbMock.BuildSearchQueryCalls(), ShouldHaveLength, 0)
+		So(esMock.MultiSearchCalls(), ShouldHaveLength, 0)
+	})
+
 	Convey("Should return InternalError for errors returned from query builder", t, func() {
 		qbMock := newQueryBuilderMock(nil, errors.New("Something"))
 		esMock := newElasticSearcherMock(nil, nil)