diff --git a/auth.go b/auth.go index 17faba7..e8b7a8e 100644 --- a/auth.go +++ b/auth.go @@ -27,15 +27,15 @@ type Client struct { // Service provides higher level wrapper allowing to construct everything and get back token middleware type Service struct { - logger logger.L - opts Opts - jwtService *token.Service - providers []provider.Service - authMiddleware middleware.Authenticator - avatarProxy *avatar.Proxy - issuer string - useGravatar bool - verifConfirmStore provider.VerifConfirmationStore + logger logger.L + opts Opts + jwtService *token.Service + providers []provider.Service + authMiddleware middleware.Authenticator + avatarProxy *avatar.Proxy + issuer string + useGravatar bool + verifConfirmStore provider.VerifConfirmationStore verifConfirmStoreOnce sync.Once } @@ -447,8 +447,16 @@ func (s *Service) AddDirectProviderWithUserIDFunc(name string, credChecker provi // AddVerifProvider adds provider user's verification sent by sender func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) { s.verifConfirmStoreOnce.Do(func() { - if s.opts.VerifConfirmationStore != nil { - s.verifConfirmStore = s.opts.VerifConfirmationStore + store := s.opts.VerifConfirmationStore + // guard against a typed-nil VerifConfirmationStoreFunc: a non-nil + // interface wrapping a nil func would survive the != nil check below + // and silently disable replay protection (the handler-level guard + // at LoginHandler then normalizes it to nil). + if fn, ok := store.(provider.VerifConfirmationStoreFunc); ok && fn == nil { + store = nil + } + if store != nil { + s.verifConfirmStore = store return } s.verifConfirmStore = provider.NewInMemoryVerifStore() diff --git a/auth_test.go b/auth_test.go index 7ea8335..15ada10 100644 --- a/auth_test.go +++ b/auth_test.go @@ -129,6 +129,25 @@ func TestService_AddVerifProvider_UsesCustomConfirmationStore(t *testing.T) { assert.Same(t, custom, svc.verifConfirmStore, "custom store from Opts must be used, not the in-memory default") } +func TestService_AddVerifProvider_TypedNilStoreFuncFallsBackToDefault(t *testing.T) { + // Opts.VerifConfirmationStore set to a typed-nil VerifConfirmationStoreFunc + // is a non-nil interface wrapping a nil func. Without a service-level + // guard the != nil check at AddVerifProvider succeeds, NewInMemoryVerifStore + // is skipped, and the handler-level guard then normalizes the typed-nil to + // nil at redemption -- net result is replay protection silently disabled. + var nilFn provider.VerifConfirmationStoreFunc + svc := NewService(Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + URL: "http://127.0.0.1", + Logger: logger.Std, + VerifConfirmationStore: nilFn, + }) + svc.AddVerifProvider("email", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + require.NotNil(t, svc.verifConfirmStore, "typed-nil func must fall back to in-memory default, not silently disable protection") + _, ok := svc.verifConfirmStore.(provider.VerifConfirmationStoreFunc) + assert.False(t, ok, "fallback must be the in-memory default, not the typed-nil func itself") +} + func TestService_AddVerifProvider_DefaultsToInMemory(t *testing.T) { svc := NewService(Opts{ SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), diff --git a/provider/verify.go b/provider/verify.go index 1801db0..681e707 100644 --- a/provider/verify.go +++ b/provider/verify.go @@ -70,6 +70,13 @@ type VerifConfirmationStore interface { // valid reopens the replay window the store is meant to close. err // signals a backend failure (network, disk, capacity, etc.); callers // MUST treat a non-nil err as fail-closed (reject the redemption). + // + // Adapter authors: do NOT embed key (or any caller-supplied data) in + // returned errors. The handler logs err on the fail-closed branch, and + // although key is the SHA-256 of the raw token rather than the token + // itself, it still uniquely identifies the live, unredeemed JWT in + // log destinations. Wrap the underlying backend error with a generic + // description (e.g. "redis SET failed: %w") instead. MarkUsed(key string, ttl time.Duration) (alreadyUsed bool, err error) } diff --git a/provider/verify_test.go b/provider/verify_test.go index 05aa253..10b705a 100644 --- a/provider/verify_test.go +++ b/provider/verify_test.go @@ -338,6 +338,26 @@ func TestInMemoryVerifStore(t *testing.T) { }) } +func TestScrubTokenFromRequest(t *testing.T) { + t.Run("token query is replaced with redacted sentinel", func(t *testing.T) { + req, err := http.NewRequest("GET", "/login?token=secret-jwt&sess=1", http.NoBody) + require.NoError(t, err) + out := scrubTokenFromRequest(req) + assert.Equal(t, "secret-jwt", req.URL.Query().Get("token"), "original request must not be mutated") + assert.Equal(t, "", out.URL.Query().Get("token")) + assert.Equal(t, "1", out.URL.Query().Get("sess"), "other query params preserved") + }) + t.Run("missing token query returns request unchanged", func(t *testing.T) { + req, err := http.NewRequest("GET", "/login?sess=1", http.NoBody) + require.NoError(t, err) + out := scrubTokenFromRequest(req) + assert.Same(t, req, out) + }) + t.Run("nil request returns nil", func(t *testing.T) { + assert.Nil(t, scrubTokenFromRequest(nil)) + }) +} + func TestVerifConfirmationStoreFunc(t *testing.T) { calls := 0 var lastKey string diff --git a/v2/auth.go b/v2/auth.go index 0449915..eee8e2b 100644 --- a/v2/auth.go +++ b/v2/auth.go @@ -27,15 +27,15 @@ type Client struct { // Service provides higher level wrapper allowing to construct everything and get back token middleware type Service struct { - logger logger.L - opts Opts - jwtService *token.Service - providers []provider.Service - authMiddleware middleware.Authenticator - avatarProxy *avatar.Proxy - issuer string - useGravatar bool - verifConfirmStore provider.VerifConfirmationStore + logger logger.L + opts Opts + jwtService *token.Service + providers []provider.Service + authMiddleware middleware.Authenticator + avatarProxy *avatar.Proxy + issuer string + useGravatar bool + verifConfirmStore provider.VerifConfirmationStore verifConfirmStoreOnce sync.Once } @@ -448,8 +448,16 @@ func (s *Service) AddDirectProviderWithUserIDFunc(name string, credChecker provi // AddVerifProvider adds provider user's verification sent by sender func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) { s.verifConfirmStoreOnce.Do(func() { - if s.opts.VerifConfirmationStore != nil { - s.verifConfirmStore = s.opts.VerifConfirmationStore + store := s.opts.VerifConfirmationStore + // guard against a typed-nil VerifConfirmationStoreFunc: a non-nil + // interface wrapping a nil func would survive the != nil check below + // and silently disable replay protection (the handler-level guard + // at LoginHandler then normalizes it to nil). + if fn, ok := store.(provider.VerifConfirmationStoreFunc); ok && fn == nil { + store = nil + } + if store != nil { + s.verifConfirmStore = store return } s.verifConfirmStore = provider.NewInMemoryVerifStore() diff --git a/v2/auth_test.go b/v2/auth_test.go index 946fc42..93a20cb 100644 --- a/v2/auth_test.go +++ b/v2/auth_test.go @@ -129,6 +129,25 @@ func TestService_AddVerifProvider_UsesCustomConfirmationStore(t *testing.T) { assert.Same(t, custom, svc.verifConfirmStore, "custom store from Opts must be used, not the in-memory default") } +func TestService_AddVerifProvider_TypedNilStoreFuncFallsBackToDefault(t *testing.T) { + // Opts.VerifConfirmationStore set to a typed-nil VerifConfirmationStoreFunc + // is a non-nil interface wrapping a nil func. Without a service-level + // guard the != nil check at AddVerifProvider succeeds, NewInMemoryVerifStore + // is skipped, and the handler-level guard then normalizes the typed-nil to + // nil at redemption -- net result is replay protection silently disabled. + var nilFn provider.VerifConfirmationStoreFunc + svc := NewService(Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + URL: "http://127.0.0.1", + Logger: logger.Std, + VerifConfirmationStore: nilFn, + }) + svc.AddVerifProvider("email", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + require.NotNil(t, svc.verifConfirmStore, "typed-nil func must fall back to in-memory default, not silently disable protection") + _, ok := svc.verifConfirmStore.(provider.VerifConfirmationStoreFunc) + assert.False(t, ok, "fallback must be the in-memory default, not the typed-nil func itself") +} + func TestService_AddVerifProvider_DefaultsToInMemory(t *testing.T) { svc := NewService(Opts{ SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), diff --git a/v2/provider/verify.go b/v2/provider/verify.go index 9b945bc..738a322 100644 --- a/v2/provider/verify.go +++ b/v2/provider/verify.go @@ -70,6 +70,13 @@ type VerifConfirmationStore interface { // valid reopens the replay window the store is meant to close. err // signals a backend failure (network, disk, capacity, etc.); callers // MUST treat a non-nil err as fail-closed (reject the redemption). + // + // Adapter authors: do NOT embed key (or any caller-supplied data) in + // returned errors. The handler logs err on the fail-closed branch, and + // although key is the SHA-256 of the raw token rather than the token + // itself, it still uniquely identifies the live, unredeemed JWT in + // log destinations. Wrap the underlying backend error with a generic + // description (e.g. "redis SET failed: %w") instead. MarkUsed(key string, ttl time.Duration) (alreadyUsed bool, err error) } diff --git a/v2/provider/verify_test.go b/v2/provider/verify_test.go index e4dab09..ad5897b 100644 --- a/v2/provider/verify_test.go +++ b/v2/provider/verify_test.go @@ -259,6 +259,26 @@ func TestInMemoryVerifStore(t *testing.T) { }) } +func TestScrubTokenFromRequest(t *testing.T) { + t.Run("token query is replaced with redacted sentinel", func(t *testing.T) { + req, err := http.NewRequest("GET", "/login?token=secret-jwt&sess=1", http.NoBody) + require.NoError(t, err) + out := scrubTokenFromRequest(req) + assert.Equal(t, "secret-jwt", req.URL.Query().Get("token"), "original request must not be mutated") + assert.Equal(t, "", out.URL.Query().Get("token")) + assert.Equal(t, "1", out.URL.Query().Get("sess"), "other query params preserved") + }) + t.Run("missing token query returns request unchanged", func(t *testing.T) { + req, err := http.NewRequest("GET", "/login?sess=1", http.NoBody) + require.NoError(t, err) + out := scrubTokenFromRequest(req) + assert.Same(t, req, out) + }) + t.Run("nil request returns nil", func(t *testing.T) { + assert.Nil(t, scrubTokenFromRequest(nil)) + }) +} + func TestVerifConfirmationStoreFunc(t *testing.T) { calls := 0 var lastKey string