fix: improve pagination parser (#12422)

mtojek · web-flow · commit 3e99c0373f3a · 2024-03-05T14:05:15.000Z
diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go
@@ -79,6 +79,30 @@ func (p *QueryParamParser) Int(vals url.Values, def int, queryParam string) int
 	return v
 }
 
+// PositiveInt32 function checks if the given value is 32-bit and positive.
+//
+// We can't use `uint32` as the value must be within the range  <0,2147483647>
+// as database expects it. Otherwise, the database query fails with `pq: OFFSET must not be negative`.
+func (p *QueryParamParser) PositiveInt32(vals url.Values, def int32, queryParam string) int32 {
+	v, err := parseQueryParam(p, vals, func(v string) (int32, error) {
+		intValue, err := strconv.ParseInt(v, 10, 32)
+		if err != nil {
+			return 0, err
+		}
+		if intValue < 0 {
+			return 0, xerrors.Errorf("value is negative")
+		}
+		return int32(intValue), nil
+	}, def, queryParam)
+	if err != nil {
+		p.Errors = append(p.Errors, codersdk.ValidationError{
+			Field:  queryParam,
+			Detail: fmt.Sprintf("Query param %q must be a valid 32-bit positive integer (%s)", queryParam, err.Error()),
+		})
+	}
+	return v
+}
+
 func (p *QueryParamParser) Boolean(vals url.Values, def bool, queryParam string) bool {
 	v, err := parseQueryParam(p, vals, strconv.ParseBool, def, queryParam)
 	if err != nil {
diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go
@@ -236,6 +236,50 @@ func TestParseQueryParams(t *testing.T) {
 		testQueryParams(t, expParams, parser, parser.Int)
 	})
 
+	t.Run("PositiveInt32", func(t *testing.T) {
+		t.Parallel()
+		expParams := []queryParamTestCase[int32]{
+			{
+				QueryParam: "valid_integer",
+				Value:      "100",
+				Expected:   100,
+			},
+			{
+				QueryParam: "empty",
+				Value:      "",
+				Expected:   0,
+			},
+			{
+				QueryParam: "no_value",
+				NoSet:      true,
+				Default:    5,
+				Expected:   5,
+			},
+			{
+				QueryParam:            "negative",
+				Value:                 "-1",
+				Expected:              0,
+				Default:               5,
+				ExpectedErrorContains: "must be a valid 32-bit positive integer",
+			},
+			{
+				QueryParam:            "invalid_integer",
+				Value:                 "bogus",
+				Expected:              0,
+				ExpectedErrorContains: "must be a valid 32-bit positive integer",
+			},
+			{
+				QueryParam:            "max_int_plus_one",
+				Value:                 "2147483648",
+				Expected:              0,
+				ExpectedErrorContains: "must be a valid 32-bit positive integer",
+			},
+		}
+
+		parser := httpapi.NewQueryParamParser()
+		testQueryParams(t, expParams, parser, parser.PositiveInt32)
+	})
+
 	t.Run("UInt", func(t *testing.T) {
 		t.Parallel()
 		expParams := []queryParamTestCase[uint64]{
diff --git a/coderd/pagination.go b/coderd/pagination.go
@@ -17,9 +17,8 @@ func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Paginat
 	parser := httpapi.NewQueryParamParser()
 	params := codersdk.Pagination{
 		AfterID: parser.UUID(queryParams, uuid.Nil, "after_id"),
-		// Limit default to "-1" which returns all results
-		Limit:  parser.Int(queryParams, 0, "limit"),
-		Offset: parser.Int(queryParams, 0, "offset"),
+		Limit:   int(parser.PositiveInt32(queryParams, 0, "limit")),
+		Offset:  int(parser.PositiveInt32(queryParams, 0, "offset")),
 	}
 	if len(parser.Errors) > 0 {
 		httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
diff --git a/coderd/pagination_internal_test.go b/coderd/pagination_internal_test.go
@@ -46,11 +46,31 @@ func TestPagination(t *testing.T) {
 			Limit:         "bogus",
 			ExpectedError: invalidValues,
 		},
+		{
+			Name:          "TooHighLimit",
+			Limit:         "2147483648",
+			ExpectedError: invalidValues,
+		},
+		{
+			Name:          "NegativeLimit",
+			Limit:         "-1",
+			ExpectedError: invalidValues,
+		},
 		{
 			Name:          "BadOffset",
 			Offset:        "bogus",
 			ExpectedError: invalidValues,
 		},
+		{
+			Name:          "TooHighOffset",
+			Offset:        "2147483648",
+			ExpectedError: invalidValues,
+		},
+		{
+			Name:          "NegativeOffset",
+			Offset:        "-1",
+			ExpectedError: invalidValues,
+		},
 
 		// Valid values
 		{
diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go
@@ -6,6 +6,7 @@ import (
 	"database/sql"
 	"encoding/json"
 	"fmt"
+	"math"
 	"net/http"
 	"os"
 	"strings"
@@ -1727,33 +1728,39 @@ func TestOffsetLimit(t *testing.T) {
 	_ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
 	_ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
 
-	// empty finds all workspaces
+	// Case 1: empty finds all workspaces
 	ws, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{})
 	require.NoError(t, err)
 	require.Len(t, ws.Workspaces, 3)
 
-	// offset 1 finds 2 workspaces
+	// Case 2: offset 1 finds 2 workspaces
 	ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
 		Offset: 1,
 	})
 	require.NoError(t, err)
 	require.Len(t, ws.Workspaces, 2)
 
-	// offset 1 limit 1 finds 1 workspace
+	// Case 3: offset 1 limit 1 finds 1 workspace
 	ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
 		Offset: 1,
 		Limit:  1,
 	})
 	require.NoError(t, err)
 	require.Len(t, ws.Workspaces, 1)
 
-	// offset 3 finds no workspaces
+	// Case 4: offset 3 finds no workspaces
 	ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
 		Offset: 3,
 	})
 	require.NoError(t, err)
 	require.Len(t, ws.Workspaces, 0)
 	require.Equal(t, ws.Count, 3) // can't find workspaces, but count is non-zero
+
+	// Case 5: offset out of range
+	ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
+		Offset: math.MaxInt32 + 1, // Potential risk: pq: OFFSET must not be negative
+	})
+	require.Error(t, err)
 }
 
 func TestWorkspaceUpdateAutostart(t *testing.T) {
