From c7e5b1f87051843ad397f260db72ac094f2a8b15 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sat, 9 May 2026 23:02:11 +0200 Subject: [PATCH] fix(verify): close service-level typed-nil store + adapter-author guidance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Followups to #281 (verify replay protection) raised on the post-merge review. None blocking, all small. 1. Service-level typed-nil VerifConfirmationStoreFunc guard. The handler-level guard added in round 2 normalizes a typed-nil func to nil, but the AddVerifProvider check at auth.go was still a plain `s.opts.VerifConfirmationStore != nil` test. A typed-nil VerifConfirmationStoreFunc is a non-nil interface wrapping a nil func, so it survived that check, the in-memory default was skipped, and at redemption the handler's typed-nil guard normalized it to nil — net result: a user who wrote `Opts{VerifConfirmationStore: VerifConfirmationStoreFunc(nil)}` got neither their func nor the default, and replay protection was silently disabled for that exact configuration. Same shape as the *avatar.Proxy typed-nil case fixed in #286 with a different consequence (silent loss of protection vs panic). Apply the same shape of guard one layer up. New regression test TestService_AddVerifProvider_TypedNilStoreFuncFallsBackToDefault in both v1 and v2. 2. gofmt -w on auth.go / v2/auth.go. The verifConfirmStoreO -> verifConfirmStoreOnce rename in #281 made the field longer than the surrounding column alignment. Cosmetic; CI doesn't enforce gofmt but a noisy IDE-on-save diff for the next contributor. 3. scrubTokenFromRequest unit test (TestScrubTokenFromRequest) in both v1 and v2 — covers the defensive early-return that coveralls flagged as uncovered after #281 (token-missing returns r unchanged, nil r returns nil, token-present returns redacted clone with other query params preserved). 4. Adapter-author guidance on VerifConfirmationStore.MarkUsed godoc: tells external Redis/DB adapter authors not to embed the supplied key in returned errors, since the handler logs err on the fail-closed branch and the key is the SHA-256 of a still-live JWT. --- auth.go | 30 +++++++++++++++++++----------- auth_test.go | 19 +++++++++++++++++++ provider/verify.go | 7 +++++++ provider/verify_test.go | 20 ++++++++++++++++++++ v2/auth.go | 30 +++++++++++++++++++----------- v2/auth_test.go | 19 +++++++++++++++++++ v2/provider/verify.go | 7 +++++++ v2/provider/verify_test.go | 20 ++++++++++++++++++++ 8 files changed, 130 insertions(+), 22 deletions(-) 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