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