From 3e99c0373f3a9e17424bdda6ce6ffbc4d3897de8 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 5 Mar 2024 15:05:15 +0100 Subject: [PATCH] fix: improve pagination parser (#12422) --- coderd/httpapi/queryparams.go | 24 ++++++++++++++++ coderd/httpapi/queryparams_test.go | 44 ++++++++++++++++++++++++++++++ coderd/pagination.go | 5 ++-- coderd/pagination_internal_test.go | 20 ++++++++++++++ coderd/workspaces_test.go | 15 +++++++--- 5 files changed, 101 insertions(+), 7 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 822cfea22d..a668362081 100644 --- 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 index b9773bfa25..3f144fa19f 100644 --- 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 index b50358e1f5..02199a390e 100644 --- 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 index 94077b1083..adcfde6bbb 100644 --- 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 index 3100a622e0..b3bccac519 100644 --- 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,19 +1728,19 @@ 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, @@ -1747,13 +1748,19 @@ func TestOffsetLimit(t *testing.T) { 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) {