Skip to content

Commit

Permalink
feat: better control for cookie secure flag
Browse files Browse the repository at this point in the history
  • Loading branch information
aeneasr committed Sep 7, 2022
1 parent cafe89a commit 90d539f
Show file tree
Hide file tree
Showing 12 changed files with 55 additions and 35 deletions.
1 change: 1 addition & 0 deletions Makefile
Expand Up @@ -97,6 +97,7 @@ format: .bin/goimports node_modules
mocks: .bin/mockgen
mockgen -package oauth2_test -destination oauth2/oauth2_provider_mock_test.go github.com/ory/fosite OAuth2Provider
mockgen -package jwk_test -destination jwk/registry_mock_test.go -source=jwk/registry.go
go generate ./...

# Generates the SDKs
.PHONY: sdk
Expand Down
3 changes: 2 additions & 1 deletion cmd/output_client.go
Expand Up @@ -2,9 +2,10 @@ package cmd

import (
"fmt"
"github.com/ory/x/pointerx"
"strings"

"github.com/ory/x/pointerx"

hydra "github.com/ory/hydra-client-go"
)

Expand Down
2 changes: 1 addition & 1 deletion consent/helper.go
Expand Up @@ -78,7 +78,7 @@ func createCsrfSession(w http.ResponseWriter, r *http.Request, conf x.CookieConf

session.Values["csrf"] = csrfValue
session.Options.HttpOnly = true
session.Options.Secure = !conf.IsDevelopmentMode(r.Context())
session.Options.Secure = conf.CookieSecure(r.Context())
session.Options.SameSite = sameSite
session.Options.Domain = conf.CookieDomain(r.Context())
if err := session.Save(r, w); err != nil {
Expand Down
3 changes: 2 additions & 1 deletion consent/helper_test.go
Expand Up @@ -259,6 +259,7 @@ func TestValidateCsrfSession(t *testing.T) {
config := mock.NewMockCookieConfigProvider(ctrl)
config.EXPECT().CookieSameSiteLegacyWorkaround(gomock.Any()).Return(tc.sameSiteLegacyWorkaround).AnyTimes()
config.EXPECT().IsDevelopmentMode(gomock.Any()).Return(false).AnyTimes()
config.EXPECT().CookieSecure(gomock.Any()).Return(false).AnyTimes()

sameSite := http.SameSiteDefaultMode
if tc.sameSite > 0 {
Expand Down Expand Up @@ -402,7 +403,7 @@ func TestCreateCsrfSession(t *testing.T) {
config := mock.NewMockCookieConfigProvider(ctrl)
config.EXPECT().CookieSameSiteMode(gomock.Any()).Return(tc.sameSite).AnyTimes()
config.EXPECT().CookieSameSiteLegacyWorkaround(gomock.Any()).Return(tc.sameSiteLegacyWorkaround).AnyTimes()
config.EXPECT().IsDevelopmentMode(gomock.Any()).Return(!tc.secure).AnyTimes()
config.EXPECT().CookieSecure(gomock.Any()).Return(tc.secure).AnyTimes()
config.EXPECT().CookieDomain(gomock.Any()).Return(tc.domain).AnyTimes()

err := createCsrfSession(rr, req, config, store, tc.name, "value")
Expand Down
6 changes: 3 additions & 3 deletions consent/strategy_default.go
Expand Up @@ -306,7 +306,7 @@ func (s *DefaultStrategy) revokeAuthenticationCookie(w http.ResponseWriter, r *h
cookie.Options.HttpOnly = true
cookie.Options.Path = "/"
cookie.Options.SameSite = s.c.CookieSameSiteMode(ctx)
cookie.Options.Secure = !s.c.IsDevelopmentMode(ctx)
cookie.Options.Secure = s.c.CookieSecure(ctx)
cookie.Options.Domain = s.c.CookieDomain(ctx)
cookie.Options.MaxAge = -1

Expand Down Expand Up @@ -440,7 +440,7 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re
cookie.Options.HttpOnly = true
cookie.Options.Path = "/"
cookie.Options.SameSite = s.c.CookieSameSiteMode(ctx)
cookie.Options.Secure = !s.c.IsDevelopmentMode(ctx)
cookie.Options.Secure = s.c.CookieSecure(ctx)
if err := cookie.Save(r, w); err != nil {
return nil, errorsx.WithStack(err)
}
Expand All @@ -450,7 +450,7 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re
"cookie_name": s.c.SessionCookieName(ctx),
"cookie_http_only": true,
"cookie_same_site": s.c.CookieSameSiteMode(ctx),
"cookie_secure": !s.c.IsDevelopmentMode(ctx),
"cookie_secure": s.c.CookieSecure(ctx),
}).Debug("Authentication session cookie was set.")
return session, nil
}
Expand Down
8 changes: 8 additions & 0 deletions driver/config/provider.go
Expand Up @@ -54,6 +54,7 @@ const (
KeyCookieSameSiteMode = "serve.cookies.same_site_mode"
KeyCookieSameSiteLegacyWorkaround = "serve.cookies.same_site_legacy_workaround"
KeyCookieDomain = "serve.cookies.domain"
KeyCookieSecure = "serve.cookies.secure"
KeyCookieLoginCSRFName = "serve.cookies.names.login_csrf"
KeyCookieConsentCSRFName = "serve.cookies.names.consent_csrf"
KeyCookieSessionName = "serve.cookies.names.session"
Expand Down Expand Up @@ -259,6 +260,13 @@ func (p *DefaultProvider) ExcludeNotBeforeClaim(ctx context.Context) bool {
return p.getProvider(ctx).BoolF(KeyExcludeNotBeforeClaim, false)
}

func (p *DefaultProvider) CookieSecure(ctx context.Context) bool {
if !p.IsDevelopmentMode(ctx) {
return true
}
return p.getProvider(ctx).BoolF(KeyCookieSecure, false)
}

func (p *DefaultProvider) CookieSameSiteMode(ctx context.Context) http.SameSite {
sameSiteModeStr := p.getProvider(ctx).String(KeyCookieSameSiteMode)
switch strings.ToLower(sameSiteModeStr) {
Expand Down
16 changes: 16 additions & 0 deletions driver/config/provider_test.go
Expand Up @@ -379,6 +379,22 @@ func TestInfinitRefreshTokenTTL(t *testing.T) {
assert.Equal(t, -1*time.Nanosecond, c.GetRefreshTokenLifespan(ctx))
}

func TestCookieSecure(t *testing.T) {
ctx := context.Background()
l := logrusx.New("", "")
l.Logrus().SetOutput(ioutil.Discard)
c := MustNew(context.Background(), l, configx.WithValue(KeyDevelopmentMode, true))

c.MustSet(ctx, KeyCookieSecure, true)
assert.True(t, c.CookieSecure(ctx))

c.MustSet(ctx, KeyCookieSecure, false)
assert.False(t, c.CookieSecure(ctx))

c.MustSet(ctx, KeyDevelopmentMode, false)
assert.True(t, c.CookieSecure(ctx))
}

func TestTokenRefreshHookURL(t *testing.T) {
l := logrusx.New("", "")
l.Logrus().SetOutput(ioutil.Discard)
Expand Down
2 changes: 1 addition & 1 deletion driver/registry_base.go
Expand Up @@ -306,7 +306,7 @@ func (m *RegistryBase) CookieStore(ctx context.Context) sessions.Store {
}

cs := sessions.NewCookieStore(keys...)
cs.Options.Secure = !m.Config().IsDevelopmentMode(ctx)
cs.Options.Secure = m.Config().CookieSecure(ctx)
cs.Options.HttpOnly = true

// CookieStore MaxAge is set to 86400 * 30 by default. This prevents secure cookies retrieval with expiration > 30 days.
Expand Down
14 changes: 14 additions & 0 deletions internal/mock/config_cookie.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 0 additions & 28 deletions jwk/registry_mock_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions spec/config.json
Expand Up @@ -400,6 +400,12 @@
"description": "Sets the cookie domain for session and CSRF cookies. Useful when dealing with subdomains. Use with care!",
"type": "string"
},
"secure": {
"title": "HTTP Cookie Secure Flag in Development Mode",
"description": "Sets the HTTP Cookie secure flag in development mode. HTTP Cookies always have the secure flag in production mode.",
"type": "boolean",
"default": false
},
"names": {
"title": "Cookie Names",
"description": "Sets the session cookie name. Use with care!",
Expand Down
1 change: 1 addition & 0 deletions x/config.go
Expand Up @@ -12,4 +12,5 @@ type CookieConfigProvider interface {
IsDevelopmentMode(ctx context.Context) bool
CookieSameSiteMode(ctx context.Context) http.SameSite
CookieSameSiteLegacyWorkaround(ctx context.Context) bool
CookieSecure(ctx context.Context) bool
}

0 comments on commit 90d539f

Please sign in to comment.