Peter Sanchez: 1 bugfix: Invalid cursors raise 500 errors. 11 files changed, 258 insertions(+), 78 deletions(-)
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.code.netlandish.com/~netlandish/links-dev/patches/232/mbox | git am -3Learn more about email & git
Changelog-fixed: invalid cursors no longer trigger ISE's --- admin/routes.go | 38 ++++----------- api/graph/model/cursor.go | 17 ++++++- api/graph/pagination.go | 7 +++ api/graph/pagination_test.go | 47 ++++++++++++++++++ billing/routes.go | 8 +--- core/routes.go | 16 ++----- core/routes_test.go | 53 +++++++++++++++++++++ helpers.go | 28 ++++++++--- helpers_test.go | 92 ++++++++++++++++++++++++++++++++++++ list/routes.go | 22 ++------- short/routes.go | 8 +--- 11 files changed, 258 insertions(+), 78 deletions(-) create mode 100644 helpers_test.go diff --git a/admin/routes.go b/admin/routes.go index e36c97e..2f7e7a7 100644 --- a/admin/routes.go +++ b/admin/routes.go @@ -449,17 +449,13 @@ func (s *Service) OrgDetail(c echo.Context) error { } }`) op.Var("orgSlug", result.Org.Slug) - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) err = links.Execute(c.Request().Context(), op, &historyResult) if err != nil { return err } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(historyResult.Payments.Result) } @@ -995,11 +991,7 @@ func (s *Service) BillingList(c echo.Context) error { } } }`) - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) if sDate != "" && eDate != "" { op.Var("dStart", sDate) @@ -1012,7 +1004,7 @@ func (s *Service) BillingList(c echo.Context) error { return err } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(historyResult.Payments.Result) } @@ -1144,11 +1136,7 @@ func (s *Service) DomainList(c echo.Context) error { gmap["search"] = query } - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) if c.QueryParam("level") != "" { level := c.QueryParam("level") @@ -1175,7 +1163,7 @@ func (s *Service) DomainList(c echo.Context) error { if err != nil { return err } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(result.Organizations.Result) } if result.Organizations.PageInfo.HasPrevPage { @@ -1246,11 +1234,7 @@ func (s *Service) OrgList(c echo.Context) error { gmap["search"] = query } - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + links.SetCursorVars(c, op) err := links.Execute(c.Request().Context(), op, &result) if err != nil { return err @@ -1635,16 +1619,12 @@ func (s *Service) UserList(c echo.Context) error { op.Var("search", query) gmap["search"] = query } - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) err := links.Execute(c.Request().Context(), op, &result) if err != nil { return err } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(result.GetUsers.Result) } if result.GetUsers.PageInfo.HasPrevPage { diff --git a/api/graph/model/cursor.go b/api/graph/model/cursor.go index 3e04a20..f4e05eb 100644 --- a/api/graph/model/cursor.go +++ b/api/graph/model/cursor.go @@ -5,6 +5,9 @@ import ( "encoding/json" "fmt" "io" + "links/valid" + + "github.com/vektah/gqlparser/v2/gqlerror" ) const ( @@ -20,6 +23,16 @@ type Cursor struct { Limit int } +// errInvalidCursor carries a validation code so consumers running the error +// through ParseInputErrors render a form error rather than falling through to +// the raw error path, which the web tier reports as a 500. +func errInvalidCursor() error { + return &gqlerror.Error{ + Message: "Invalid cursor", + Extensions: map[string]any{"code": valid.ErrValidationGlobalCode}, + } +} + func (c *Cursor) UnmarshalGQL(v any) error { enc, ok := v.(string) if !ok { @@ -27,10 +40,10 @@ func (c *Cursor) UnmarshalGQL(v any) error { } plain, err := base64.StdEncoding.DecodeString(enc) if plain == nil || err != nil { - return fmt.Errorf("Invalid cursor") + return errInvalidCursor() } if err = json.Unmarshal(plain, c); err != nil { - return fmt.Errorf("Invalid cursor") + return errInvalidCursor() } return nil } diff --git a/api/graph/pagination.go b/api/graph/pagination.go index 0c16595..09068e1 100644 --- a/api/graph/pagination.go +++ b/api/graph/pagination.go @@ -110,6 +110,13 @@ func QueryModel[T any]( numElements = *limit } + // A client-supplied cursor carries its own Limit; a zero or negative value + // would drop the SQL LIMIT clause entirely in GetBuilder and then panic the + // slice reslice in PaginateResults. + if numElements <= 0 { + numElements = model.PaginationDefault + } + // maxLimit will default to model.PaginationMax maxLimit := ForPaginationContext(ctx) if numElements > maxLimit { diff --git a/api/graph/pagination_test.go b/api/graph/pagination_test.go index 82e7500..6307936 100644 --- a/api/graph/pagination_test.go +++ b/api/graph/pagination_test.go @@ -1,11 +1,13 @@ package graph import ( + "context" "links/api/graph/model" "slices" "testing" "github.com/stretchr/testify/assert" + "netlandish.com/x/gobwebs/database" ) func TestPaginationForwardAndBackward(t *testing.T) { @@ -75,3 +77,48 @@ func TestPaginationForwardAndBackward(t *testing.T) { assert.Equal(t, 26, len(seen)) }) } + +func TestQueryModelClampsCursorLimit(t *testing.T) { + items := make([]int, 0, 200) + for i := 200; i > 0; i-- { + items = append(items, i) + } + + run := func(t *testing.T, limit *int, before, after *model.Cursor) int { + t.Helper() + var gotLimit int + getModels := func(_ context.Context, opts *database.FilterOptions) ([]int, error) { + gotLimit = opts.Limit + if opts.Limit <= 0 || opts.Limit > len(items) { + return items, nil + } + return items[:opts.Limit], nil + } + _, _, err := QueryModel(context.Background(), &database.FilterOptions{}, + "id", "DESC", limit, before, after, getModels, func(v int) int { return v }) + assert.NoError(t, err) + return gotLimit + } + + // A negative cursor limit used to leave opts.Limit at zero, which drops the SQL + // LIMIT clause entirely and then panics on the reslice in PaginateResults. + t.Run("negative after limit", func(t *testing.T) { + assert.Equal(t, model.PaginationDefault+1, + run(t, nil, nil, &model.Cursor{After: 150, Limit: -1})) + }) + + t.Run("zero before limit", func(t *testing.T) { + assert.Equal(t, model.PaginationDefault+1, + run(t, nil, &model.Cursor{Before: 150, Limit: 0}, nil)) + }) + + t.Run("negative explicit limit falls back to cursor", func(t *testing.T) { + negative := -5 + assert.Equal(t, 11, run(t, &negative, nil, &model.Cursor{After: 150, Limit: 10})) + }) + + t.Run("oversized cursor limit still capped", func(t *testing.T) { + assert.Equal(t, model.PaginationMax+1, + run(t, nil, nil, &model.Cursor{After: 150, Limit: 10000})) + }) +} diff --git a/billing/routes.go b/billing/routes.go index 82fd992..524ce9e 100644 --- a/billing/routes.go @@ -253,17 +253,13 @@ func (s *Service) SubscriptionHistory(c echo.Context) error { } }`) op.Var("orgSlug", slug) - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) err = links.Execute(c.Request().Context(), op, &result) if err != nil { return err } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(result.Payments.Result) } diff --git a/core/routes.go b/core/routes.go index 48da02b..2b2f835 100644 --- a/core/routes.go +++ b/core/routes.go @@ -1847,11 +1847,7 @@ func (s *Service) UserFeed(c echo.Context) error { } }`) - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) var ( tag, excludeTag, search string @@ -1898,7 +1894,7 @@ func (s *Service) UserFeed(c echo.Context) error { pd.Data["clear"] = lt.Translate("Clear") pd.Data["followings"] = lt.Translate("Followings") orgLinks := result.OrgLinks.Result - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(orgLinks) } @@ -2287,11 +2283,7 @@ func (s *Service) OrgLinksList(c echo.Context) error { currURL = c.Echo().Reverse(s.RouteName("recent_link_list")) rssURL = c.Echo().Reverse(s.RouteName("recent_link_list_rss")) } - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) var ( hasUnreadFilter, @@ -2485,7 +2477,7 @@ func (s *Service) OrgLinksList(c echo.Context) error { return links.ServerRSSFeed(c.Response(), rss) } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(orgLinks) } diff --git a/core/routes_test.go b/core/routes_test.go index a4c48c7..0c8b320 100644 --- a/core/routes_test.go +++ b/core/routes_test.go @@ -3,7 +3,9 @@ package core_test import ( "bytes" "database/sql" + "encoding/json" "fmt" + "io" "links" "links/cmd" "links/cmd/test" @@ -155,6 +157,57 @@ func TestHandlers(t *testing.T) { c.True(strings.Contains(htmlBody, "Link two")) }) + t.Run("recent link list with hostile cursor", func(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + jsonResponse, err := httpmock.NewJsonResponder(http.StatusOK, httpmock.File("samples/org_link_list.json")) + c.NoError(err) + // Stands in for the API, which rejects an undecodable cursor with a bare + // "Invalid cursor" error that the web tier used to surface as a 500. The + // handler must never forward one, so any after/before variable fails here. + httpmock.RegisterResponder("POST", "http://127.0.0.1:8080/query", + func(req *http.Request) (*http.Response, error) { + body, rerr := io.ReadAll(req.Body) + if rerr != nil { + return nil, rerr + } + var payload struct { + Variables map[string]any `json:"variables"` + } + if rerr = json.Unmarshal(body, &payload); rerr != nil { + return nil, rerr + } + for _, key := range []string{"after", "before"} { + if _, ok := payload.Variables[key]; ok { + return httpmock.NewJsonResponse(http.StatusOK, map[string]any{ + "data": nil, + "errors": []map[string]any{{"message": "Invalid cursor"}}, + }) + } + } + req.Body = io.NopCloser(bytes.NewReader(body)) + return jsonResponse(req) + }) + + hostile := "eyJBZnRlciI6MTk5MDcxLCJCZWZvcmUiOjIwNzc4OSwiTGltaXQiOjI1fQ==\";SELECT SLEEP(20)#" + for _, param := range []string{"next", "prev"} { + request := httptest.NewRequest(http.MethodGet, + "/recent?"+url.Values{param: {hostile}}.Encode(), nil) + recorder := httptest.NewRecorder() + ctx := &server.Context{ + Server: srv, + Context: e.NewContext(request, recorder), + User: test.NewTestUser(1, false, false, false, false), + } + ctx.SetPath("/recent") + err = test.MakeRequestWithDomain(srv, coreService.OrgLinksList, ctx, domains[0]) + c.NoError(err, "hostile %s cursor should not error", param) + htmlBody := recorder.Body.String() + c.True(strings.Contains(htmlBody, "Link one")) + c.True(strings.Contains(htmlBody, "Link two")) + } + }) + t.Run("create link form error", func(t *testing.T) { httpmock.Activate() defer httpmock.DeactivateAndReset() diff --git a/helpers.go b/helpers.go index d191d43..ad2a855 100644 --- a/helpers.go +++ b/helpers.go @@ -9,6 +9,7 @@ import ( "fmt" "html/template" "io" + "links/api/graph/model" "links/domain" "links/internal/localizer" "links/models" @@ -499,6 +500,25 @@ func ParsePendingBaseURLs(ctx context.Context, userAgent string) error { return nil } +func ValidCursor(v string) bool { + var cur model.Cursor + return cur.UnmarshalGQL(v) == nil +} + +func SetCursorVars(c echo.Context, op *gqlclient.Operation) bool { + if v := c.QueryParam("next"); v != "" { + if ValidCursor(v) { + op.Var("after", v) + } + return false + } + if v := c.QueryParam("prev"); v != "" && ValidCursor(v) { + op.Var("before", v) + return true + } + return false +} + // GetPaginationParams returns the params needed for cursor pagination func GetPaginationParams(c echo.Context, pagvar, cursor string, exclude ...string) template.URL { q := make(url.Values) @@ -1340,11 +1360,7 @@ func FetchAuditLogs(c echo.Context, userID int, if limit > 0 { op.Var("limit", limit) } - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := SetCursorVars(c, op) var result AuditLogResponse err := Execute(LangContext(c), op, &result) @@ -1354,7 +1370,7 @@ func FetchAuditLogs(c echo.Context, userID int, gctx := c.(*server.Context) user := gctx.User.(*models.User) - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(result.AuditLogs.Result) } diff --git a/helpers_test.go b/helpers_test.go new file mode 100644 index 0000000..0e44115 --- /dev/null +++ b/helpers_test.go @@ -0,0 +1,92 @@ +package links + +import ( + "encoding/base64" + "links/api/graph/model" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "git.sr.ht/~emersion/gqlclient" + "github.com/labstack/echo/v4" + "github.com/stretchr/testify/require" +) + +func validCursor(t *testing.T) string { + t.Helper() + cur := model.Cursor{After: 199071, Before: 207789, Limit: 25} + rec := httptest.NewRecorder() + cur.MarshalGQL(rec.Body) + return rec.Body.String()[1 : rec.Body.Len()-1] +} + +func TestValidCursor(t *testing.T) { + c := require.New(t) + good := validCursor(t) + + tests := []struct { + name string + value string + want bool + }{ + {"round trip", good, true}, + {"empty", "", false}, + {"sql injection probe", good + `";SELECT SLEEP(20)#`, false}, + {"not base64", "not a cursor", false}, + {"base64 of non json", base64.StdEncoding.EncodeToString([]byte("nope")), false}, + {"base64 of json array", base64.StdEncoding.EncodeToString([]byte(`[1,2]`)), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c.Equal(tt.want, ValidCursor(tt.value)) + }) + } +} + +func TestSetCursorVars(t *testing.T) { + good := validCursor(t) + bad := good + `";SELECT SLEEP(20)#` + + newCtx := func(query url.Values) echo.Context { + req := httptest.NewRequest(http.MethodGet, "/?"+query.Encode(), nil) + return echo.New().NewContext(req, httptest.NewRecorder()) + } + + // gqlclient.Operation keeps its variables unexported and panics when the same + // variable is set twice, so re-setting a key is the only way to observe whether + // SetCursorVars applied it. + assertVarSet := func(t *testing.T, op *gqlclient.Operation, key string, want bool) { + t.Helper() + set := func() { op.Var(key, "probe") } + if want { + require.Panics(t, set, "expected %q to have been set", key) + return + } + require.NotPanics(t, set, "expected %q to be unset", key) + } + + tests := []struct { + name string + query url.Values + wantReversed bool + wantAfter bool + wantBefore bool + }{ + {"no cursor", url.Values{}, false, false, false}, + {"valid next", url.Values{"next": {good}}, false, true, false}, + {"valid prev", url.Values{"prev": {good}}, true, false, true}, + {"invalid next", url.Values{"next": {bad}}, false, false, false}, + {"invalid prev", url.Values{"prev": {bad}}, false, false, false}, + {"next wins over prev", url.Values{"next": {good}, "prev": {good}}, false, true, false}, + {"invalid next does not fall through to prev", url.Values{"next": {bad}, "prev": {good}}, false, false, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + op := gqlclient.NewOperation("query { noop }") + require.Equal(t, tt.wantReversed, SetCursorVars(newCtx(tt.query), op)) + assertVarSet(t, op, "after", tt.wantAfter) + assertVarSet(t, op, "before", tt.wantBefore) + }) + } +} diff --git a/list/routes.go b/list/routes.go index 25392cc..63328ab 100644 --- a/list/routes.go +++ b/list/routes.go @@ -556,11 +556,7 @@ func (s *Service) ListingLinksManage(c echo.Context) error { op.Var("slug", listing.Slug) op.Var("domainId", listing.DomainID) - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) err = links.Execute(c.Request().Context(), op, &result) if err != nil { @@ -570,7 +566,7 @@ func (s *Service) ListingLinksManage(c echo.Context) error { return err } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(result.Listing.Links) } @@ -1099,11 +1095,7 @@ func (s *Service) ListingList(c echo.Context) error { queries.Add("exclude", excludeTag) } - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) var domID int if c.QueryParam("domain") != "" { @@ -1122,7 +1114,7 @@ func (s *Service) ListingList(c echo.Context) error { } return err } - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(result.Listings.Result) } @@ -1640,11 +1632,7 @@ func (r *DetailService) ListDetail(c echo.Context) error { } op.Var("domainId", domain.ID) - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + links.SetCursorVars(c, op) err := links.Execute(c.Request().Context(), op, &result) if err != nil { diff --git a/short/routes.go b/short/routes.go index 4c92a39..02e4c26 100644 --- a/short/routes.go +++ b/short/routes.go @@ -136,11 +136,7 @@ func (s *Service) LinkShortList(c echo.Context) error { queries.Add("exclude", excludeTag) } - if c.QueryParam("next") != "" { - op.Var("after", c.QueryParam("next")) - } else if c.QueryParam("prev") != "" { - op.Var("before", c.QueryParam("prev")) - } + reversed := links.SetCursorVars(c, op) var domID int if c.QueryParam("domain") != "" { @@ -181,7 +177,7 @@ func (s *Service) LinkShortList(c echo.Context) error { pd.Data["domain"] = lt.Translate("Domain") linkShorts := result.LinkShorts.Result - if c.QueryParam("prev") != "" { + if reversed { slices.Reverse(linkShorts) } -- 2.54.0
Applied. To git@git.code.netlandish.com:~netlandish/links 700e742..67901fd master -> master