Changelog-added: security check to ensure redirect urls are always
relative
---
admin/routes.go | 12 ++--------
core/routes.go | 23 +++++++-----------
core/routes_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++-
helpers.go | 10 ++++++++
helpers_test.go | 27 +++++++++++++++++++++
5 files changed, 103 insertions(+), 26 deletions(-)
diff --git a/admin/routes.go b/admin/routes.go
index 2f7e7a7..26c904e 100644
--- a/admin/routes.go
+++ b/admin/routes.go
@@ -2,7 +2,6 @@ package admin
import (
"fmt"
- "html/template"
"links"
"links/internal/localizer"
"links/models"
@@ -1020,19 +1019,12 @@ func (s *Service) BillingList(c echo.Context) error {
"filterSelector": filterSelector,
}
- queryParams := c.QueryParams()
if historyResult.Payments.PageInfo.HasPrevPage {
- tmpQueryParams := queryParams
- tmpQueryParams.Del("next")
- tmpQueryParams.Set("prev", historyResult.Payments.PageInfo.Cursor)
- gmap["prevURL"] = template.URL(tmpQueryParams.Encode())
+ gmap["prevURL"] = links.GetPaginationParams(c, "prev", historyResult.Payments.PageInfo.Cursor, "next")
}
if historyResult.Payments.PageInfo.HasNextPage {
- tmpQueryParams := queryParams
- tmpQueryParams.Del("prev")
- tmpQueryParams.Set("next", historyResult.Payments.PageInfo.Cursor)
- gmap["nextURL"] = template.URL(tmpQueryParams.Encode())
+ gmap["nextURL"] = links.GetPaginationParams(c, "next", historyResult.Payments.PageInfo.Cursor, "prev")
}
return s.Render(c, http.StatusOK, "admin_billing_list.html", gmap)
}
diff --git a/core/routes.go b/core/routes.go
index 2b2f835..2c51149 100644
--- a/core/routes.go
+++ b/core/routes.go
@@ -2716,10 +2716,8 @@ func (s *Service) OrgLinkDelete(c echo.Context) error {
}
messages.Success(c, lt.Translate("Bookmark successfully deleted"))
- redirect := c.QueryParam("next")
- if redirect == "" {
- redirect = c.Echo().Reverse(s.RouteName("home_link_list"))
- }
+ redirect := links.SafeRedirect(c.QueryParam("next"),
+ c.Echo().Reverse(s.RouteName("home_link_list")))
return c.Redirect(http.StatusMovedPermanently, redirect)
}
@@ -2760,7 +2758,7 @@ func (s *Service) OrgLinkDelete(c echo.Context) error {
deleteURL := c.Echo().Reverse(s.RouteName("link_delete"), link.Hash)
backURL := c.Echo().Reverse(s.RouteName("home_link_list"))
- if next := c.QueryParam("next"); next != "" {
+ if next := links.SafeRedirect(c.QueryParam("next"), ""); next != "" {
deleteURL = deleteURL + "?next=" + url.QueryEscape(next)
backURL = next
}
@@ -3075,7 +3073,7 @@ func (s *Service) QRManageDelete(c echo.Context) error {
if err != nil {
return echo.NotFoundHandler(c)
}
- next := c.QueryParam("next")
+ next := links.SafeRedirect(c.QueryParam("next"), "")
if next == "" {
return echo.NotFoundHandler(c)
}
@@ -3441,7 +3439,7 @@ func (s *Service) OrgLinkStarToggle(c echo.Context) error {
if err != nil {
return err
}
- redirect := c.QueryParam("next")
+ redirect := links.SafeRedirect(c.QueryParam("next"), "")
if redirect == "" {
redirect = c.Request().Header.Get("Referer")
}
@@ -3491,7 +3489,7 @@ func (s *Service) OrgLinkAsReadToggle(c echo.Context) error {
if err != nil {
return err
}
- redirect := c.QueryParam("next")
+ redirect := links.SafeRedirect(c.QueryParam("next"), "")
if redirect == "" {
redirect = c.Request().Header.Get("Referer")
}
@@ -3600,13 +3598,8 @@ func (s *Service) FollowToggle(c echo.Context) error {
return err
}
- var nextURL string
- if c.QueryParam("next") != "" {
- nextURL = c.QueryParam("next")
- } else {
- nextURL = c.Echo().Reverse("core:org_link_list", orgSlug)
-
- }
+ nextURL := links.SafeRedirect(c.QueryParam("next"),
+ c.Echo().Reverse("core:org_link_list", orgSlug))
return c.Redirect(http.StatusMovedPermanently, nextURL)
}
diff --git a/core/routes_test.go b/core/routes_test.go
index 0c8b320..6b7bc75 100644
--- a/core/routes_test.go
+++ b/core/routes_test.go
@@ -599,7 +599,7 @@ func TestHandlers(t *testing.T) {
httpmock.RegisterResponder("POST", "http://127.0.0.1:8080/query", jsonResponse)
f := make(url.Values)
- f.Set("next", "http//redirect.com")
+ f.Set("next", "/qr")
request := httptest.NewRequest(http.MethodPost, "/?"+f.Encode(), nil)
recorder := httptest.NewRecorder()
ctx := &server.Context{
@@ -613,6 +613,61 @@ func TestHandlers(t *testing.T) {
err = test.MakeRequestWithDomain(srv, coreService.QRManageDelete, ctx, domains[0])
c.NoError(err)
c.Equal(http.StatusMovedPermanently, recorder.Code)
+ c.Equal("/qr", recorder.Header().Get("Location"))
+ })
+
+ // QRManageDelete treats an unusable ?next= as a missing one and 404s rather
+ // than handing an off-site URL to c.Redirect.
+ t.Run("qr delete rejects off-site next", func(t *testing.T) {
+ for _, hostile := range []string{"https://evil.com", "//evil.com", "http//redirect.com"} {
+ f := make(url.Values)
+ f.Set("next", hostile)
+ request := httptest.NewRequest(http.MethodPost, "/?"+f.Encode(), nil)
+ recorder := httptest.NewRecorder()
+ ctx := &server.Context{
+ Server: srv,
+ Context: e.NewContext(request, recorder),
+ User: loggedInUser,
+ }
+ ctx.SetPath("/qr/:id/delete")
+ ctx.SetParamNames("id")
+ ctx.SetParamValues("100")
+ err := test.MakeRequestWithDomain(srv, coreService.QRManageDelete, ctx, domains[0])
+
+ var httpErr *echo.HTTPError
+ c.ErrorAs(err, &httpErr, "hostile next %q must not redirect", hostile)
+ c.Equal(http.StatusNotFound, httpErr.Code)
+ }
+ })
+
+ // The toggle/follow handlers fall back to an in-app route instead of 404ing.
+ t.Run("follow toggle ignores off-site next", func(t *testing.T) {
+ httpmock.Activate()
+ defer httpmock.DeactivateAndReset()
+ jsonResponse, err := httpmock.NewJsonResponder(http.StatusOK, map[string]any{
+ "data": map[string]any{"follow": map[string]any{"success": true, "message": ""}},
+ })
+ c.NoError(err)
+ httpmock.RegisterResponder("POST", "http://127.0.0.1:8080/query", jsonResponse)
+
+ f := make(url.Values)
+ f.Set("next", "https://evil.com")
+ request := httptest.NewRequest(http.MethodGet, "/?"+f.Encode(), nil)
+ recorder := httptest.NewRecorder()
+ ctx := &server.Context{
+ Server: srv,
+ Context: e.NewContext(request, recorder),
+ User: loggedInUser,
+ }
+ ctx.SetPath("/:slug/follow-toggle/:action")
+ ctx.SetParamNames("slug", "action")
+ ctx.SetParamValues("test-org", "follow")
+ err = test.MakeRequestWithDomain(srv, coreService.FollowToggle, ctx, domains[0])
+ c.NoError(err)
+
+ location := recorder.Header().Get("Location")
+ c.NotEqual("https://evil.com", location)
+ c.True(strings.HasPrefix(location, "/"), "expected in-app path, got %q", location)
})
t.Run("user feed rss with valid hash", func(t *testing.T) {
diff --git a/helpers.go b/helpers.go
index ad2a855..ceaf027 100644
--- a/helpers.go
+++ b/helpers.go
@@ -519,6 +519,16 @@ func SetCursorVars(c echo.Context, op *gqlclient.Operation) bool {
return false
}
+func SafeRedirect(next, fallback string) string {
+ if next == "" || next[0] != '/' {
+ return fallback
+ }
+ if len(next) > 1 && (next[1] == '/' || next[1] == '\\') {
+ return fallback
+ }
+ return next
+}
+
// GetPaginationParams returns the params needed for cursor pagination
func GetPaginationParams(c echo.Context, pagvar, cursor string, exclude ...string) template.URL {
q := make(url.Values)
diff --git a/helpers_test.go b/helpers_test.go
index 0e44115..2cf5938 100644
--- a/helpers_test.go
+++ b/helpers_test.go
@@ -90,3 +90,30 @@ func TestSetCursorVars(t *testing.T) {
})
}
}
+
+func TestSafeRedirect(t *testing.T) {
+ const fallback = "/home"
+
+ tests := []struct {
+ name string
+ next string
+ want string
+ }{
+ {"empty falls back", "", fallback},
+ {"relative path", "/org/links", "/org/links"},
+ {"root", "/", "/"},
+ {"relative with query", "/org/links?tag=go", "/org/links?tag=go"},
+ {"absolute https", "https://evil.com", fallback},
+ {"absolute http", "http://evil.com/path", fallback},
+ {"scheme relative", "//evil.com", fallback},
+ {"backslash scheme relative", "/\\evil.com", fallback},
+ {"schemeless host", "evil.com", fallback},
+ {"javascript uri", "javascript:alert(1)", fallback},
+ {"missing colon host", "http//redirect.com", fallback},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ require.Equal(t, tt.want, SafeRedirect(tt.next, fallback))
+ })
+ }
+}
--
2.54.0