Skip to content

fix(verify): close service-level typed-nil store + adapter-author guidance#289

Merged
umputun merged 1 commit into
masterfrom
fix/verify-replay-typed-nil-followup
May 10, 2026
Merged

fix(verify): close service-level typed-nil store + adapter-author guidance#289
umputun merged 1 commit into
masterfrom
fix/verify-replay-typed-nil-followup

Conversation

@paskal
Copy link
Copy Markdown
Collaborator

@paskal paskal commented May 9, 2026

Summary

Followups to #281 raised on the post-merge review. None blocking, all small.

# Item Where
1 Service-level typed-nil VerifConfirmationStoreFunc guard -- silently disabled replay protection for Opts{VerifConfirmationStore: VerifConfirmationStoreFunc(nil)} auth.go, v2/auth.go (+ tests)
2 gofmt -w after the verifConfirmStoreO -> verifConfirmStoreOnce rename auth.go, v2/auth.go
3 Unit test for scrubTokenFromRequest -- the defensive early-return was uncovered after #281 provider/verify_test.go, v2/provider/verify_test.go
4 Adapter-author godoc note on MarkUsed -- don't embed key in returned errors provider/verify.go, v2/provider/verify.go

Item 1 in detail

The handler-level guard added in #281 round 2 (provider/verify.go LoginHandler) correctly normalises a typed-nil VerifConfirmationStoreFunc to no-store, but the Service-level check in AddVerifProvider was still:

if s.opts.VerifConfirmationStore != nil {
    s.verifConfirmStore = s.opts.VerifConfirmationStore
    return
}
s.verifConfirmStore = provider.NewInMemoryVerifStore()

A typed-nil VerifConfirmationStoreFunc is a non-nil interface wrapping a nil func, so it survives the != nil check, NewInMemoryVerifStore() is skipped, and at redemption time the handler-level guard then normalises the typed-nil to nil. Net effect: a user writing Opts{VerifConfirmationStore: VerifConfirmationStoreFunc(nil)} gets neither their func nor the default in-memory store, and replay protection is silently disabled.

Same shape as the *avatar.Proxy typed-nil case fixed in #286, with a different consequence (silent loss of protection rather than panic). The fix is the same shape applied one layer up:

s.verifConfirmStoreOnce.Do(func() {
    store := s.opts.VerifConfirmationStore
    if fn, ok := store.(provider.VerifConfirmationStoreFunc); ok && fn == nil {
        store = nil
    }
    if store != nil {
        s.verifConfirmStore = store
        return
    }
    s.verifConfirmStore = provider.NewInMemoryVerifStore()
})

Regression test TestService_AddVerifProvider_TypedNilStoreFuncFallsBackToDefault in both v1 and v2: writes var nilFn provider.VerifConfirmationStoreFunc; ConfirmationStore: nilFn, asserts verifConfirmStore is non-nil and is not the typed-nil func itself (i.e. the default fired).

Test

go test -race ./... green in both modules; golangci-lint run --enable-only=misspell ./... clean.

…dance

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.
@paskal paskal requested a review from umputun as a code owner May 9, 2026 21:12
@coveralls
Copy link
Copy Markdown

coveralls commented May 9, 2026

Coverage Report for CI Build 25611934022

Coverage increased (+0.09%) to 85.291%

Details

  • Coverage increased (+0.09%) from the base build.
  • Patch coverage: 10 of 10 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 3474
Covered Lines: 2963
Line Coverage: 85.29%
Coverage Strength: 8.07 hits per line

💛 - Coveralls

Copy link
Copy Markdown
Member

@umputun umputun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all 4 followups look good. Tests cover the typed-nil regression and scrubTokenFromRequest, v1/v2 mirror is right. LGTM.

@umputun umputun merged commit 69ae5cd into master May 10, 2026
9 checks passed
@umputun umputun deleted the fix/verify-replay-typed-nil-followup branch May 10, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants