diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 2000308..1928451 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -79,6 +79,7 @@ type Auth struct { provider *oidc.Provider client *http.Client cookie loginCookie + pending *PendingStates sessions Sessions allowed []string // endSessionURL is the provider's end_session_endpoint; logout ends @@ -157,6 +158,7 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger client: client, allowed: slices.Clone(cfg.AllowedGroups), cookie: newLoginCookie(cfg.SecretKey), + pending: NewPendingStates(loginCookieTTL), sessions: sessions, endSessionURL: discovered.EndSessionURI, logger: logger, @@ -250,11 +252,13 @@ func (a *Auth) login(w http.ResponseWriter, r *http.Request) { Verifier: oauth2.GenerateVerifier(), Return: returnTo, } - if err := a.cookie.set(w, state, time.Now()); err != nil { + now := time.Now() + if err := a.cookie.set(w, state, now); err != nil { a.logger.ErrorContext(r.Context(), "set login cookie", "error", err) http.Error(w, "internal error", http.StatusInternalServerError) return } + a.pending.Add(state.State, now) http.Redirect(w, r, a.oauth.AuthCodeURL(state.State, oidc.Nonce(state.Nonce), oauth2.S256ChallengeOption(state.Verifier), @@ -262,7 +266,7 @@ func (a *Auth) login(w http.ResponseWriter, r *http.Request) { } // callback completes the flow: it checks state against the login cookie, -// exchanges the code, verifies the ID token and nonce, then starts a session +// consumes it, exchanges the code, verifies the ID token and nonce, then starts a session // if the account is in an allowed group. Membership is checked on every // login, so an account removed from the group is refused at its next one. func (a *Auth) callback(w http.ResponseWriter, r *http.Request) { @@ -283,6 +287,13 @@ func (a *Auth) callback(w http.ResponseWriter, r *http.Request) { http.Error(w, "sign-in state mismatch; start again", http.StatusBadRequest) return } + // A copy of the cookie verifies until it expires, so the state is + // single-use only because it is consumed here. + if !a.pending.Consume(state.State) { + a.logger.WarnContext(ctx, "login callback state already used") + http.Error(w, "sign-in state mismatch; start again", http.StatusBadRequest) + return + } if q.Get("error") != "" { // error and error_description are provider-supplied text; only the // fixed category is logged. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index af9f53e..2cacb86 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -497,6 +497,59 @@ func TestCallbackDefaultReturnPath(t *testing.T) { } } +// A saved copy of the login cookie still verifies until it expires, so a +// replay with a second valid code is refused only because the server +// consumed the state on first use. +func TestCallbackRefusesReplayedLoginState(t *testing.T) { + t.Parallel() + h := newHarness(t) + l := h.login(t, LoginPath) + idToken := h.provider.signRS256(h.provider.claims(l.query.Get("nonce"))) + + if resp := h.callback(l, l.query.Get("state"), idToken); resp.StatusCode != http.StatusSeeOther { + t.Fatalf("first callback status = %d, want 303", resp.StatusCode) + } + resp := h.callback(l, l.query.Get("state"), idToken) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("replayed callback status = %d, want 400", resp.StatusCode) + } + if n := len(h.sessions.started()); n != 1 { + t.Errorf("%d sessions started, want 1", n) + } + assertLoginCookieCleared(t, resp) +} + +// Pending login states live in memory, so a login started before a restart +// fails closed after it even though its cookie still verifies. +func TestCallbackRefusesLoginStartedBeforeRestart(t *testing.T) { + t.Parallel() + h := newHarness(t) + l := h.login(t, LoginPath) + + restarted, err := New(context.Background(), Config{ + Issuer: h.provider.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("server-secret-key"), + AllowedGroups: testAllowedGroups, + }, h.sessions, slog.New(slog.DiscardHandler)) + if err != nil { + t.Fatal(err) + } + r := chi.NewRouter() + restarted.Mount(r) + h.router = r + + resp := h.callback(l, l.query.Get("state"), h.provider.signRS256(h.provider.claims(l.query.Get("nonce")))) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("callback after restart status = %d, want 400", resp.StatusCode) + } + if n := len(h.sessions.started()); n != 0 { + t.Errorf("%d sessions started, want none", n) + } +} + func TestCallbackRejected(t *testing.T) { t.Parallel() diff --git a/internal/auth/cookie.go b/internal/auth/cookie.go index ed85350..32a5473 100644 --- a/internal/auth/cookie.go +++ b/internal/auth/cookie.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" ) @@ -153,6 +154,45 @@ func (c FlowCookie) Read(r *http.Request, v flowValue, now time.Time) error { return nil } +// PendingStates holds the state of every redirect flow started and not yet +// completed, until its flow cookie expires. A copy of a flow cookie verifies +// until it expires, so a flow is single-use only because its callback +// consumes the state here. The states live in memory — MealQuest runs as one +// process — so a flow started before a restart fails closed after it. +type PendingStates struct { + ttl time.Duration + mu sync.Mutex + states map[string]time.Time +} + +// NewPendingStates returns an empty set whose states are pending for ttl, +// the flow cookie's lifetime. +func NewPendingStates(ttl time.Duration) *PendingStates { + return &PendingStates{ttl: ttl, states: map[string]time.Time{}} +} + +// Add records state, started at now, as pending and forgets expired ones. +func (p *PendingStates) Add(state string, now time.Time) { + p.mu.Lock() + defer p.mu.Unlock() + for k, expires := range p.states { + if !now.Before(expires) { + delete(p.states, k) + } + } + p.states[state] = now.Add(p.ttl) +} + +// Consume removes state from the pending states, reporting whether it was +// there. +func (p *PendingStates) Consume(state string) bool { + p.mu.Lock() + defer p.mu.Unlock() + _, ok := p.states[state] + delete(p.states, state) + return ok +} + // returnPath validates the optional post-login return path. Empty means "/". // Only same-origin absolute paths are allowed: no scheme, host, protocol- // relative "//" prefix, backslashes or control characters. diff --git a/internal/gcal/connect.go b/internal/gcal/connect.go index 28aadc7..f7962a8 100644 --- a/internal/gcal/connect.go +++ b/internal/gcal/connect.go @@ -137,10 +137,9 @@ type Service struct { account sync.Mutex // pending holds the state of every connect started and not yet - // completed, with when it expires. Complete consumes a state once, so a - // replayed cookie finds it gone. - pendingMu sync.Mutex - pending map[string]time.Time + // completed. Complete consumes a state once, so a replayed cookie finds + // it gone. + pending *auth.PendingStates } // NewService returns the service over st, sealing refresh tokens with box. @@ -175,7 +174,7 @@ func NewService(st *store.Store, box *secretbox.Box, cfg Config) (*Service, erro cookie: auth.NewFlowCookie(connectCookieName, connectCookieKeyLabel, connectCookieTTL, cfg.SecretKey), retryDelay: firstRetryDelay, now: time.Now, - pending: map[string]time.Time{}, + pending: auth.NewPendingStates(connectCookieTTL), }, nil } @@ -213,7 +212,7 @@ func (s *Service) Connect(w http.ResponseWriter, session auth.Session) (string, if err := s.cookie.Set(w, &state, now); err != nil { return "", fmt.Errorf("gcal: set connect cookie: %w", err) } - s.addPending(state.State, now) + s.pending.Add(state.State, now) return s.oauth.AuthCodeURL(state.State, oauth2.AccessTypeOffline, oauth2.ApprovalForce, @@ -243,7 +242,7 @@ func (s *Service) Complete(w http.ResponseWriter, r *http.Request, session auth. } // A copy of the cookie verifies until it expires, so the state is // single-use only because it is consumed here. - if !s.consumePending(state.State) { + if !s.pending.Consume(state.State) { return fmt.Errorf("%w: already used", ErrBadState) } if q.Get("error") != "" { @@ -276,29 +275,6 @@ func (s *Service) Complete(w http.ResponseWriter, r *http.Request, session auth. return s.store(ctx, sealed) } -// addPending records state, started at now, as pending until the connect -// cookie expires, and forgets expired ones. -func (s *Service) addPending(state string, now time.Time) { - s.pendingMu.Lock() - defer s.pendingMu.Unlock() - for k, expires := range s.pending { - if !now.Before(expires) { - delete(s.pending, k) - } - } - s.pending[state] = now.Add(connectCookieTTL) -} - -// consumePending removes state from the pending states, reporting whether -// it was there. -func (s *Service) consumePending(state string) bool { - s.pendingMu.Lock() - defer s.pendingMu.Unlock() - _, ok := s.pending[state] - delete(s.pending, state) - return ok -} - // checkScopes fails with ErrScopes unless token's granted scopes include // every scope in Scopes. Google lets the household untick a permission on // the consent screen and reports what was granted in "scope".