session: Handle securecookie errors appropriately

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"

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

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=

internal/driver.go

@@ -28,6 +28,7 @@ func resetConfig() {
 }
 
 func NewConfigurationWithDefaults() *configuration.ViperProvider {
+	viper.Reset()
 	resetConfig()
 	return configuration.NewViperProvider(logrusx.New())
 }

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)

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)
 	}))

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
 			}

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 {

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")

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()))
 	}
 

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