Skip to content

Commit

Permalink
fix: do not set cookies on api endpoints
Browse files Browse the repository at this point in the history
  • Loading branch information
aeneasr committed Oct 27, 2020
1 parent 3c912e4 commit 2f67c28
Show file tree
Hide file tree
Showing 20 changed files with 64 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ $ curl -s -X GET \
"code": 403,
"status": "Forbidden",
"reason": "This endpoint can only be accessed with a valid session. Please log in and try again.",
"debug": "rid=\nerror=request does not have a valid authentication session\nreason=No active session was found in this request.\ndetails=map[]\ndebug=\n\ngithub.com/ory/kratos/session.(*ManagerHTTP).FetchFromRequest\n\t/go/src/github.com/ory/kratos/session/manager_http.go:119\ngithub.com/ory/kratos/session.(*Handler).IsAuthenticated.func1\n\t/go/src/github.com/ory/kratos/session/handler.go:163\ngithub.com/ory/kratos/x.NoCacheHandler.func1\n\t/go/src/github.com/ory/kratos/x/nocache.go:18\ngithub.com/julienschmidt/httprouter.(*Router).ServeHTTP\n\t/go/pkg/mod/github.com/julienschmidt/[email protected]/router.go:334\ngithub.com/justinas/nosurf.(*CSRFHandler).handleSuccess\n\t/go/pkg/mod/github.com/justinas/[email protected]/handler.go:187\ngithub.com/justinas/nosurf.(*CSRFHandler).ServeHTTP\n\t/go/pkg/mod/github.com/justinas/[email protected]/handler.go:144\ngithub.com/urfave/negroni.Wrap.func1\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:46\ngithub.com/urfave/negroni.HandlerFunc.ServeHTTP\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:29\ngithub.com/urfave/negroni.middleware.ServeHTTP\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:38\ngithub.com/urfave/negroni.(*Negroni).ServeHTTP\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:96\ngithub.com/gorilla/context.ClearHandler.func1\n\t/go/pkg/mod/github.com/gorilla/[email protected]/context.go:141\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\nnet/http.serverHandler.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2843\nnet/http.(*conn).serve\n\t/usr/local/go/src/net/http/server.go:1925\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374",
"debug": "rid=\nerror=request does not have a valid authentication session\nreason=No active session was found in this request.\ndetails=map[]\ndebug=\n\ngithub.com/ory/kratos/session.(*ManagerHTTP).FetchFromRequest\n\t/go/src/github.com/ory/kratos/session/manager_http.go:119\ngithub.com/ory/kratos/session.(*Handler).IsAuthenticated.func1\n\t/go/src/github.com/ory/kratos/session/handler.go:163\ngithub.com/ory/kratos/x.NoCacheHandler.func1\n\t/go/src/github.com/ory/kratos/x/nocache.go:18\ngithub.com/julienschmidt/httprouter.(*Router).ServeHTTP\n\t/go/pkg/mod/github.com/julienschmidt/[email protected]/router.go:334\ngithub.com/ory/nosurf.(*CSRFHandler).handleSuccess\n\t/go/pkg/mod/github.com/ory/[email protected]/handler.go:187\ngithub.com/ory/nosurf.(*CSRFHandler).ServeHTTP\n\t/go/pkg/mod/github.com/ory/[email protected]/handler.go:144\ngithub.com/urfave/negroni.Wrap.func1\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:46\ngithub.com/urfave/negroni.HandlerFunc.ServeHTTP\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:29\ngithub.com/urfave/negroni.middleware.ServeHTTP\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:38\ngithub.com/urfave/negroni.(*Negroni).ServeHTTP\n\t/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:96\ngithub.com/gorilla/context.ClearHandler.func1\n\t/go/pkg/mod/github.com/gorilla/[email protected]/context.go:141\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\nnet/http.serverHandler.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2843\nnet/http.(*conn).serve\n\t/usr/local/go/src/net/http/server.go:1925\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374",
"message": "The requested action was forbidden"
}
}
5 changes: 1 addition & 4 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ go 1.14
// See https:/markbates/pkger/pull/112
replace github.com/markbates/pkger => github.com/falafeljan/pkger v0.17.1-0.20200722132747-95726f5b9b9b

// Remove once https:/justinas/nosurf/pull/62 is merged
replace github.com/justinas/nosurf => github.com/aeneasr/nosurf v1.1.1-0.20201014095004-b10b0e9ff0d6

replace gopkg.in/DataDog/dd-trace-go.v1 => gopkg.in/DataDog/dd-trace-go.v1 v1.27.1-0.20201005154917-54b73b3e126a

require (
Expand Down Expand Up @@ -51,7 +48,6 @@ require (
github.com/imdario/mergo v0.3.7
github.com/jteeuwen/go-bindata v3.0.7+incompatible
github.com/julienschmidt/httprouter v1.2.0
github.com/justinas/nosurf v1.1.1
github.com/leodido/go-urn v1.1.0 // indirect
github.com/luna-duclos/instrumentedsql/opentracing v0.0.0-20201015064105-f9d01e123f16 // indirect
github.com/markbates/pkger v0.17.0
Expand All @@ -69,6 +65,7 @@ require (
github.com/ory/herodot v0.9.0
github.com/ory/jsonschema/v3 v3.0.1
github.com/ory/mail/v3 v3.0.0
github.com/ory/nosurf v1.2.2
github.com/ory/viper v1.7.5
github.com/ory/x v0.0.153
github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 h1:d+Bc7a5rLufV
github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE=
github.com/Shopify/sarama v1.19.0/go.mod h1:FVkBWblsNy7DGZRfXLU0O9RCGt5g3g3yEuWXgklEdEo=
github.com/Shopify/toxiproxy v2.1.4+incompatible/go.mod h1:OXgGpZ6Cli1/URJOF1DMxUHB2q5Ap20/P/eIdh4G0pI=
github.com/aeneasr/nosurf v1.1.1-0.20201014095004-b10b0e9ff0d6 h1:vwQCZ4WZuWxqiHWbXxVlTwkOtnJb8dGWDek7tDOxqxs=
github.com/aeneasr/nosurf v1.1.1-0.20201014095004-b10b0e9ff0d6/go.mod h1:ALpWdSbuNGy2lZWtyXdjkYv4edL23oSEgfBT1gPJ5BQ=
github.com/ajg/form v0.0.0-20160822230020-523a5da1a92f h1:zvClvFQwU++UpIUBGC8YmDlfhUrweEy1R1Fj1gu5iIM=
github.com/ajg/form v0.0.0-20160822230020-523a5da1a92f/go.mod h1:uL1WgH+h2mgNtvBq0339dVnzXdBETtL2LeUXaIv25UY=
github.com/ajstarks/svgo v0.0.0-20180226025133-644b8db467af/go.mod h1:K08gAheRH3/J6wwsYMMT4xOr94bZjxIelGM0+d/wbFw=
Expand Down Expand Up @@ -1089,6 +1087,8 @@ github.com/ory/mail v2.3.1+incompatible h1:vHntHDHtQXamt2T+iwTTlCoBkDvILUeujE9Oc
github.com/ory/mail v2.3.1+incompatible/go.mod h1:87D9/1gB6ewElQoN0lXJ0ayfqcj3cW3qCTXh+5E9mfU=
github.com/ory/mail/v3 v3.0.0 h1:8LFMRj473vGahFD/ntiotWEd4S80FKYFtiZTDfOQ+sM=
github.com/ory/mail/v3 v3.0.0/go.mod h1:JGAVeZF8YAlxbaFDUHqRZAKBCSeW2w1vuxf28hFbZAw=
github.com/ory/nosurf v1.2.2 h1:lUNdxAl45nFdvR95m774e9ShgKzrSMJHvgeNSjZdarI=
github.com/ory/nosurf v1.2.2/go.mod h1:d4L3ZBa7Amv55bqxCBtCs63wSlyaiCkWVl4vKf3OUxA=
github.com/ory/viper v1.5.6/go.mod h1:TYmpFpKLxjQwvT4f0QPpkOn4sDXU1kDgAwJpgLYiQ28=
github.com/ory/viper v1.7.4 h1:3RWBt7Pq9kSFNxLaRT0ljNdbtaWisCQG1cLPn2Yd4UY=
github.com/ory/viper v1.7.4/go.mod h1:T6sodNZKNGPpashUOk7EtXz2isovz8oCd57GNVkkNmE=
Expand Down
2 changes: 1 addition & 1 deletion selfservice/errorx/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"net/http"

"github.com/julienschmidt/httprouter"
"github.com/justinas/nosurf"
"github.com/ory/nosurf"

"github.com/ory/herodot"

Expand Down
2 changes: 1 addition & 1 deletion selfservice/errorx/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"testing"

"github.com/julienschmidt/httprouter"
"github.com/justinas/nosurf"
"github.com/ory/nosurf"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down
3 changes: 2 additions & 1 deletion selfservice/flow/login/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ func NewHandler(d handlerDependencies, c configuration.Provider) *Handler {
}

func (h *Handler) RegisterPublicRoutes(public *x.RouterPublic) {
h.d.CSRFHandler().ExemptPath(RouteInitAPIFlow)
h.d.CSRFHandler().IgnorePath(RouteInitAPIFlow)

public.GET(RouteInitBrowserFlow, h.initBrowserFlow)
public.GET(RouteInitAPIFlow, h.initAPIFlow)
public.GET(RouteGetFlow, h.fetchFlow)
Expand Down
3 changes: 3 additions & 0 deletions selfservice/flow/login/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ func TestInitFlow(t *testing.T) {
req := x.NewTestHTTPRequest(t, "GET", ts.URL+route, nil)
req.URL.RawQuery = extQuery.Encode()
body, res := testhelpers.MockMakeAuthenticatedRequest(t, reg, conf, router.Router, req)
if isAPI {
assert.Len(t, res.Header.Get("Set-Cookie"), 0)
}
return res, body
}

Expand Down
2 changes: 1 addition & 1 deletion selfservice/flow/logout/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (

"github.com/gobuffalo/httptest"
"github.com/julienschmidt/httprouter"
"github.com/justinas/nosurf"
"github.com/ory/nosurf"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand Down
2 changes: 1 addition & 1 deletion selfservice/flow/recovery/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func NewHandler(d handlerDependencies, c configuration.Provider) *Handler {
}

func (h *Handler) RegisterPublicRoutes(public *x.RouterPublic) {
h.d.CSRFHandler().ExemptPath(RouteInitAPIFlow)
h.d.CSRFHandler().IgnorePath(RouteInitAPIFlow)

redirect := session.RedirectOnAuthenticated(h.c)
public.GET(RouteInitBrowserFlow, h.d.SessionHandler().IsNotAuthenticated(h.initBrowserFlow, redirect))
Expand Down
3 changes: 3 additions & 0 deletions selfservice/flow/recovery/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ func TestInitFlow(t *testing.T) {
}
req := x.NewTestHTTPRequest(t, "GET", publicTS.URL+route, nil)
body, res := testhelpers.MockMakeAuthenticatedRequest(t, reg, conf, router.Router, req)
if isAPI {
assert.Len(t, res.Header.Get("Set-Cookie"), 0)
}
return res, body
}

Expand Down
2 changes: 1 addition & 1 deletion selfservice/flow/registration/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func NewHandler(d handlerDependencies, c configuration.Provider) *Handler {
}

func (h *Handler) RegisterPublicRoutes(public *x.RouterPublic) {
h.d.CSRFHandler().ExemptPath(RouteInitAPIFlow)
h.d.CSRFHandler().IgnorePath(RouteInitAPIFlow)

public.GET(RouteInitBrowserFlow, h.d.SessionHandler().IsNotAuthenticated(h.initBrowserFlow, session.RedirectOnAuthenticated(h.c)))
public.GET(RouteInitAPIFlow, h.d.SessionHandler().IsNotAuthenticated(h.initApiFlow,
Expand Down
6 changes: 6 additions & 0 deletions selfservice/flow/registration/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ func TestInitFlow(t *testing.T) {
}
req := x.NewTestHTTPRequest(t, "GET", publicTS.URL+route, nil)
body, res := testhelpers.MockMakeAuthenticatedRequest(t, reg, conf, router.Router, req)
if isAPI {
assert.Len(t, res.Header.Get("Set-Cookie"), 0)
}
return res, body
}

Expand All @@ -90,6 +93,9 @@ func TestInitFlow(t *testing.T) {
c := publicTS.Client()
res, err := c.Get(publicTS.URL + route)
require.NoError(t, err)
if isAPI {
assert.Len(t, res.Header.Get("Set-Cookie"), 0)
}
defer res.Body.Close()
body, err := ioutil.ReadAll(res.Body)
require.NoError(t, err)
Expand Down
7 changes: 6 additions & 1 deletion selfservice/flow/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package flow
import (
"net/http"

"github.com/justinas/nosurf"
"github.com/ory/nosurf"
"github.com/pkg/errors"

"github.com/ory/herodot"
Expand All @@ -19,11 +19,16 @@ var ErrCookieHeaderNeedsBrowserFlow = herodot.ErrBadRequest.
func VerifyRequest(
r *http.Request,
flowType Type,
disableAPIFlowEnforcement bool,
generator func(r *http.Request) string,
actual string,
) error {
switch flowType {
case TypeAPI:
if disableAPIFlowEnforcement {
return nil
}

// API Based flows to not require anti-CSRF tokens because we can not leverage a session, making this
// endpoint pointless.

Expand Down
10 changes: 5 additions & 5 deletions selfservice/flow/request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ import (
)

func TestVerifyRequest(t *testing.T) {
require.EqualError(t, VerifyRequest(&http.Request{}, TypeBrowser, x.FakeCSRFTokenGenerator, "not_csrf_token"), x.ErrInvalidCSRFToken.Error())
require.NoError(t, VerifyRequest(&http.Request{}, TypeBrowser, x.FakeCSRFTokenGenerator, x.FakeCSRFToken), nil)
require.NoError(t, VerifyRequest(&http.Request{}, TypeAPI, x.FakeCSRFTokenGenerator, ""))
require.EqualError(t, VerifyRequest(&http.Request{}, TypeBrowser, false, x.FakeCSRFTokenGenerator, "not_csrf_token"), x.ErrInvalidCSRFToken.Error())
require.NoError(t, VerifyRequest(&http.Request{}, TypeBrowser, false, x.FakeCSRFTokenGenerator, x.FakeCSRFToken), nil)
require.NoError(t, VerifyRequest(&http.Request{}, TypeAPI, false, x.FakeCSRFTokenGenerator, ""))
require.EqualError(t, VerifyRequest(&http.Request{
Header: http.Header{"Origin": {"https://www.ory.sh"}},
}, TypeAPI, x.FakeCSRFTokenGenerator, ""), ErrOriginHeaderNeedsBrowserFlow.Error())
}, TypeAPI, false, x.FakeCSRFTokenGenerator, ""), ErrOriginHeaderNeedsBrowserFlow.Error())
require.EqualError(t, VerifyRequest(&http.Request{
Header: http.Header{"Cookie": {"cookie=ory"}},
}, TypeAPI, x.FakeCSRFTokenGenerator, ""), ErrCookieHeaderNeedsBrowserFlow.Error())
}, TypeAPI, false, x.FakeCSRFTokenGenerator, ""), ErrCookieHeaderNeedsBrowserFlow.Error())
}
4 changes: 2 additions & 2 deletions selfservice/flow/settings/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"time"

"github.com/julienschmidt/httprouter"
"github.com/justinas/nosurf"
"github.com/ory/nosurf"
"github.com/pkg/errors"

"github.com/ory/x/urlx"
Expand Down Expand Up @@ -74,7 +74,7 @@ func NewHandler(d handlerDependencies, c configuration.Provider) *Handler {
}

func (h *Handler) RegisterPublicRoutes(public *x.RouterPublic) {
h.d.CSRFHandler().ExemptPath(RouteInitAPIFlow)
h.d.CSRFHandler().IgnorePath(RouteInitAPIFlow)

redirect := session.RedirectOnUnauthenticated(h.c.SelfServiceFlowLoginUI().String())
public.GET(RouteInitBrowserFlow, h.d.SessionHandler().IsAuthenticated(h.initBrowserFlow, redirect))
Expand Down
1 change: 1 addition & 0 deletions selfservice/flow/settings/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ func TestHandler(t *testing.T) {
res, err := user1.Get(publicTS.URL + settings.RouteInitAPIFlow)
require.NoError(t, err)
defer res.Body.Close()
assert.Len(t, res.Header.Get("Set-Cookie"), 0)
body := x.MustReadAll(res.Body)
id := gjson.GetBytes(body, "id")
require.NotEmpty(t, id)
Expand Down
2 changes: 1 addition & 1 deletion selfservice/flow/verification/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func NewHandler(d handlerDependencies, c configuration.Provider) *Handler {
}

func (h *Handler) RegisterPublicRoutes(public *x.RouterPublic) {
h.d.CSRFHandler().ExemptPath(RouteInitAPIFlow)
h.d.CSRFHandler().IgnorePath(RouteInitAPIFlow)

public.GET(RouteInitBrowserFlow, h.initBrowserFlow)
public.GET(RouteInitAPIFlow, h.initAPIFlow)
Expand Down
21 changes: 18 additions & 3 deletions selfservice/flow/verification/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,16 @@ func TestGetFlow(t *testing.T) {
}))
}

assertFlowPayload := func(t *testing.T, body []byte) {
assertFlowPayload := func(t *testing.T, body []byte, isApi bool) {
if isApi {
assert.Equal(t, "api", gjson.GetBytes(body, "type").String(), "%s", body)
assert.Empty(t, gjson.GetBytes(body, "methods.link.config.fields.#(name==csrf_token).value").String(), "%s", body)
} else {
assert.Equal(t, "browser", gjson.GetBytes(body, "type").String(), "%s", body)
assert.NotEmpty(t, gjson.GetBytes(body, "methods.link.config.fields.#(name==csrf_token).value").String(), "%s", body)
}

assert.Equal(t, "link", gjson.GetBytes(body, "methods.link.method").String(), "%s", body)
assert.NotEmpty(t, gjson.GetBytes(body, "methods.link.config.fields.#(name==csrf_token).value").String(), "%s", body)
assert.NotEmpty(t, gjson.GetBytes(body, "id").String(), "%s", body)
assert.Empty(t, gjson.GetBytes(body, "headers").Value(), "%s", body)
assert.Contains(t, gjson.GetBytes(body, "methods.link.config.action").String(), gjson.GetBytes(body, "id").String(), "%s", body)
Expand Down Expand Up @@ -76,7 +83,15 @@ func TestGetFlow(t *testing.T) {
map[string]interface{}{"enabled": true})

t.Run("case=valid", func(t *testing.T) {
assertFlowPayload(t, x.EasyGetBody(t, endpoint.Client(), public.URL+verification.RouteInitBrowserFlow))
t.Run("type=browser", func(t *testing.T) {
assertFlowPayload(t, x.EasyGetBody(t, endpoint.Client(), public.URL+verification.RouteInitBrowserFlow), false)
})

t.Run("type=api", func(t *testing.T) {
res, body := x.EasyGet(t, endpoint.Client(), public.URL+verification.RouteInitAPIFlow)
assert.Len(t, res.Header.Get("Set-Cookie"), 0)
assertFlowPayload(t, body, true)
})
})

t.Run("case=expired", func(t *testing.T) {
Expand Down
3 changes: 3 additions & 0 deletions session/manager_http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ type mockCSRFHandler struct {
func (f *mockCSRFHandler) ExemptPath(s string) {
}

func (f *mockCSRFHandler) IgnorePath(s string) {
}

func (f *mockCSRFHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}

Expand Down
6 changes: 5 additions & 1 deletion x/nosurf.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"encoding/base64"
"net/http"

"github.com/justinas/nosurf"
"github.com/ory/nosurf"
"github.com/pkg/errors"

"github.com/ory/x/randx"
Expand Down Expand Up @@ -62,6 +62,9 @@ func NewFakeCSRFHandler(name string) *FakeCSRFHandler {
func (f *FakeCSRFHandler) ExemptPath(s string) {
}

func (f *FakeCSRFHandler) IgnorePath(s string) {
}

func (f *FakeCSRFHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}

Expand All @@ -77,6 +80,7 @@ type CSRFHandler interface {
http.Handler
RegenerateToken(w http.ResponseWriter, r *http.Request) string
ExemptPath(string)
IgnorePath(string)
}

func NewCSRFHandler(
Expand Down

0 comments on commit 2f67c28

Please sign in to comment.