From d0e6c5b3fca7dce5d9b14824f647bae3dbc25a42 Mon Sep 17 00:00:00 2001 From: aeneasr Date: Mon, 4 Nov 2019 18:05:56 +0100 Subject: [PATCH] session: Handle securecookie errors appropriately Previously, IsNotAuthenticated would not handle securecookie errors appropriately. This has been resolved. Closes #97 --- cmd/client/migrate.go | 3 ++- go.mod | 1 + go.sum | 2 ++ internal/driver.go | 1 + selfservice/oidc/strategy_helper_test.go | 2 +- selfservice/password/strategy_test.go | 2 +- session/handler.go | 6 ++--- session/handler_mock.go | 4 ++- session/handler_test.go | 31 ++++++++++++++++++++++++ session/manager.go | 15 +++++++++--- session/manager_test.go | 2 +- 11 files changed, 58 insertions(+), 11 deletions(-) diff --git a/cmd/client/migrate.go b/cmd/client/migrate.go index 600a23e50b77..79e210cd81d8 100644 --- a/cmd/client/migrate.go +++ b/cmd/client/migrate.go @@ -6,9 +6,10 @@ import ( "os" "strings" - "github.com/ory/x/sqlcon" "github.com/spf13/cobra" + "github.com/ory/x/sqlcon" + "github.com/ory/viper" "github.com/ory/x/cmdx" "github.com/ory/x/flagx" diff --git a/go.mod b/go.mod index 30539c750dde..05efc4d126a6 100644 --- a/go.mod +++ b/go.mod @@ -24,6 +24,7 @@ require ( github.com/google/uuid v1.1.1 github.com/gorilla/context v1.1.1 github.com/gorilla/handlers v1.4.1 // indirect + github.com/gorilla/securecookie v1.1.1 github.com/gorilla/sessions v1.1.3 github.com/imdario/mergo v0.3.7 github.com/jessevdk/go-flags v1.4.0 // indirect diff --git a/go.sum b/go.sum index 414bceec9cb4..4388c13f5e28 100644 --- a/go.sum +++ b/go.sum @@ -623,6 +623,7 @@ github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/subosito/gotenv v1.1.1 h1:TWxckSF6WVKWbo2M3tMqCtWa9NFUgqM1SSynxmYONOI= github.com/subosito/gotenv v1.1.1/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= @@ -727,6 +728,7 @@ golang.org/x/net v0.0.0-20190603091049-60506f45cf65/go.mod h1:HSz+uSET+XFnRR8LxR golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20190628185345-da137c7871d7 h1:rTIdg5QFRR7XCaK4LCjBiPbx8j4DQRpdYMnGn/bJUEU= golang.org/x/net v0.0.0-20190628185345-da137c7871d7/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20191014212845-da9a3fd4c582 h1:p9xBe/w/OzkeYVKm234g55gMdD1nSIooTir5kV11kfA= golang.org/x/net v0.0.0-20191014212845-da9a3fd4c582/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20181003184128-c57b0facaced/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= diff --git a/internal/driver.go b/internal/driver.go index 1cadc690bd8a..8c96e6882650 100644 --- a/internal/driver.go +++ b/internal/driver.go @@ -28,6 +28,7 @@ func resetConfig() { } func NewConfigurationWithDefaults() *configuration.ViperProvider { + viper.Reset() resetConfig() return configuration.NewViperProvider(logrusx.New()) } diff --git a/selfservice/oidc/strategy_helper_test.go b/selfservice/oidc/strategy_helper_test.go index 184dcfc6f3f4..9ff0efd6f695 100644 --- a/selfservice/oidc/strategy_helper_test.go +++ b/selfservice/oidc/strategy_helper_test.go @@ -140,7 +140,7 @@ func newHydraIntegration(t *testing.T, remote *string, subject *string, scope *[ func newReturnTs(t *testing.T, reg driver.Registry) *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - sess, err := reg.SessionManager().FetchFromRequest(r.Context(), r) + sess, err := reg.SessionManager().FetchFromRequest(r.Context(), w, r) require.NoError(t, err) require.Empty(t, sess.Identity.Credentials) reg.Writer().Write(w, r, sess) diff --git a/selfservice/password/strategy_test.go b/selfservice/password/strategy_test.go index 000ccffd3667..256b920cb384 100644 --- a/selfservice/password/strategy_test.go +++ b/selfservice/password/strategy_test.go @@ -23,7 +23,7 @@ func newErrTs(t *testing.T, reg driver.Registry) *httptest.Server { func newReturnTs(t *testing.T, reg driver.Registry) *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - sess, err := reg.SessionManager().FetchFromRequest(r.Context(), r) + sess, err := reg.SessionManager().FetchFromRequest(r.Context(), w, r) require.NoError(t, err) reg.Writer().Write(w, r, sess) })) diff --git a/session/handler.go b/session/handler.go index 2cc4086c0c5c..f8620643de86 100644 --- a/session/handler.go +++ b/session/handler.go @@ -43,7 +43,7 @@ func (h *Handler) RegisterAdminRoutes(admin *x.RouterAdmin) { } func (h *Handler) fromCookie(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - s, err := h.r.SessionManager().FetchFromRequest(r.Context(), r) + s, err := h.r.SessionManager().FetchFromRequest(r.Context(), w, r) if err != nil { h.h.WriteError(w, r, err) return @@ -61,8 +61,8 @@ func (h *Handler) fromPath(w http.ResponseWriter, r *http.Request, ps httprouter func (h *Handler) IsNotAuthenticated(wrap httprouter.Handle, onAuthenticated httprouter.Handle) httprouter.Handle { return func(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - if _, err := h.r.SessionManager().FetchFromRequest(r.Context(), r); err != nil { - if errors.Cause(err) == ErrNoActiveSessionFound { + if _, err := h.r.SessionManager().FetchFromRequest(r.Context(), w, r); err != nil { + if errors.Cause(err).Error() == ErrNoActiveSessionFound.Error() { wrap(w, r, ps) return } diff --git a/session/handler_mock.go b/session/handler_mock.go index e36d784ced0c..2866f30cf173 100644 --- a/session/handler_mock.go +++ b/session/handler_mock.go @@ -35,7 +35,7 @@ func MockSetSession(t *testing.T, reg Registry) httprouter.Handle { func MockGetSession(t *testing.T, reg Registry) httprouter.Handle { return func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { - _, err := reg.SessionManager().FetchFromRequest(r.Context(), r) + _, err := reg.SessionManager().FetchFromRequest(r.Context(), w, r) if r.URL.Query().Get("has") == "yes" { require.NoError(t, err) } else { @@ -74,6 +74,8 @@ func MockHydrateCookieClient(t *testing.T, c *http.Client, u string) { require.NoError(t, err) assert.EqualValues(t, http.StatusOK, res.StatusCode) + t.Logf("Cookies: %+v", res.Cookies()) + var found bool for _, c := range res.Cookies() { if c.Name == DefaultSessionCookieName { diff --git a/session/handler_test.go b/session/handler_test.go index 7d7ad0409776..22f274d6af5a 100644 --- a/session/handler_test.go +++ b/session/handler_test.go @@ -5,11 +5,14 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/julienschmidt/httprouter" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/ory/x/urlx" + "github.com/ory/viper" "github.com/ory/herodot" @@ -57,6 +60,33 @@ func TestHandler(t *testing.T) { }) } +func TestIsNotAuthenticatedSecurecookie(t *testing.T) { + _, reg := internal.NewMemoryRegistry(t) + r := x.NewRouterPublic() + r.GET("/public/with-callback", reg.SessionHandler().IsNotAuthenticated(send(http.StatusOK), send(http.StatusBadRequest))) + + ts := httptest.NewServer(r) + defer ts.Close() + viper.Set(configuration.ViperKeyURLsSelfPublic, ts.URL) + + c := MockCookieClient(t) + c.Jar.SetCookies(urlx.ParseOrPanic(ts.URL), []*http.Cookie{ + { + Name: DefaultSessionCookieName, + // This is an invalid cookie because it is generated by a very random secret + Value: "MTU3Mjg4Njg0MXxEdi1CQkFFQ180SUFBUkFCRUFBQU52LUNBQUVHYzNSeWFXNW5EQVVBQTNOcFpBWnpkSEpwYm1jTUd3QVpUWFZXVUhSQlZVeExXRWRUUmxkVVoyUkpUVXhzY201SFNBPT187kdI3dMP-ep389egDR2TajYXGG-6xqC2mAlgnBi0vsg=", + HttpOnly: true, + Path: "/", + Expires: time.Now().Add(time.Hour), + }, + }) + + res, err := c.Get(ts.URL + "/public/with-callback") + require.NoError(t, err) + + assert.EqualValues(t, http.StatusOK, res.StatusCode) +} + func TestIsNotAuthenticated(t *testing.T) { _, reg := internal.NewMemoryRegistry(t) r := x.NewRouterPublic() @@ -66,6 +96,7 @@ func TestIsNotAuthenticated(t *testing.T) { r.GET("/public/without-callback", reg.SessionHandler().IsNotAuthenticated(send(http.StatusOK), nil)) ts := httptest.NewServer(r) defer ts.Close() + viper.Set(configuration.ViperKeyURLsSelfPublic, ts.URL) sessionClient := MockCookieClient(t) MockHydrateCookieClient(t, sessionClient, ts.URL+"/set") diff --git a/session/manager.go b/session/manager.go index bfa0f53d1e79..b499f19189b7 100644 --- a/session/manager.go +++ b/session/manager.go @@ -4,6 +4,7 @@ import ( "context" "net/http" + "github.com/gorilla/securecookie" "github.com/pkg/errors" "github.com/ory/hive/identity" @@ -15,7 +16,7 @@ import ( const DefaultSessionCookieName = "hive_session_manager" var ( - ErrNoActiveSessionFound = herodot.ErrUnauthorized.WithReason("No active session was found in this request.") + ErrNoActiveSessionFound = herodot.ErrUnauthorized.WithError("request does not have a valid authentication session").WithReason("No active session was found in this request.") ) // Manager handles identity sessions. @@ -35,7 +36,7 @@ type Manager interface { SaveToRequest(context.Context, *Session, http.ResponseWriter, *http.Request) error // FetchFromRequest creates an HTTP session using cookies. - FetchFromRequest(context.Context, *http.Request) (*Session, error) + FetchFromRequest(context.Context, http.ResponseWriter, *http.Request) (*Session, error) // PurgeFromRequest removes an HTTP session. PurgeFromRequest(context.Context, http.ResponseWriter, *http.Request) error @@ -91,9 +92,17 @@ func (s *ManagerHTTP) SaveToRequest(ctx context.Context, session *Session, w htt return nil } -func (s *ManagerHTTP) FetchFromRequest(ctx context.Context, r *http.Request) (*Session, error) { +func (s *ManagerHTTP) FetchFromRequest(ctx context.Context, w http.ResponseWriter, r *http.Request) (*Session, error) { cookie, err := s.r.CookieManager().Get(r, s.cookieName) if err != nil { + if _, ok := err.(securecookie.Error); ok { + // If securecookie returns an error, the HMAC is probably invalid. In that case, we really want + // to remove the cookie from the browser as it is invalid anyways. + if err := s.PurgeFromRequest(ctx, w, r); err != nil { + return nil, err + } + } + return nil, errors.WithStack(ErrNoActiveSessionFound.WithDebug(err.Error())) } diff --git a/session/manager_test.go b/session/manager_test.go index 2e18da7318fd..1a4d46ec4c30 100644 --- a/session/manager_test.go +++ b/session/manager_test.go @@ -129,7 +129,7 @@ func TestSessionManagerHTTP(t *testing.T) { }) router.GET("/get", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { - s, err := sm.FetchFromRequest(context.Background(), r) + s, err := sm.FetchFromRequest(context.Background(), w, r) if errors.Cause(err) == ErrNoActiveSessionFound { w.WriteHeader(http.StatusUnauthorized) return