Peter Sanchez: 1 ensure redirect url's are relative 5 files changed, 103 insertions(+), 26 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/233/mbox | git am -3Learn more about email & git
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
Applied. To git@git.code.netlandish.com:~netlandish/links 67901fd..d914636 master -> master