From 0f7f8014f8e6018b2ea785646a1e1fcb815114ab Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 01:40:18 -0400 Subject: [PATCH] Bound pending flow states The login route is unauthenticated, so its pending states could grow without limit and every Add scanned them all. Expired states now leave from the front of an add-ordered queue, and Add refuses a new flow with ErrTooManyPending once 10000 are pending; login answers 503. Both flows record the state before setting the cookie. Co-Authored-By: Claude Opus 5.5 --- internal/auth/auth.go | 6 +++++- internal/auth/auth_test.go | 37 +++++++++++++++++++++++++++++++++++++ internal/auth/cookie.go | 38 ++++++++++++++++++++++++++++++++------ internal/gcal/connect.go | 4 +++- 4 files changed, 77 insertions(+), 8 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 1928451..fca62b3 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -253,12 +253,16 @@ func (a *Auth) login(w http.ResponseWriter, r *http.Request) { Return: returnTo, } now := time.Now() + if err := a.pending.Add(state.State, now); err != nil { + a.logger.WarnContext(r.Context(), "login refused", "error", err) + http.Error(w, "too many sign-ins in progress; try again later", http.StatusServiceUnavailable) + return + } 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), diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 2cacb86..9ac6fb9 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -872,6 +872,43 @@ func TestLoginCookieExpires(t *testing.T) { } } +func TestPendingStates(t *testing.T) { + t.Parallel() + p := NewPendingStates(time.Minute) + now := time.Now() + if err := p.Add("first", now); err != nil { + t.Fatal(err) + } + if !p.Consume("first") { + t.Error("Consume of a pending state = false") + } + if p.Consume("first") { + t.Error("second Consume of a state = true") + } + if p.Consume("never-added") { + t.Error("Consume of an unknown state = true") + } + + for i := range maxPendingStates { + if err := p.Add(fmt.Sprint("s", i), now); err != nil { + t.Fatalf("Add %d: %v", i, err) + } + } + if err := p.Add("over", now); !errors.Is(err, ErrTooManyPending) { + t.Errorf("Add past the bound = %v, want ErrTooManyPending", err) + } + // Once the earlier states expire, they are forgotten and a new one fits. + if err := p.Add("later", now.Add(time.Minute)); err != nil { + t.Errorf("Add after expiry: %v", err) + } + if p.Consume("s0") { + t.Error("an expired state was still pending") + } + if !p.Consume("later") { + t.Error("the state added after expiry is not pending") + } +} + func TestLoginRejectsOffOriginReturn(t *testing.T) { t.Parallel() h := newHarness(t) diff --git a/internal/auth/cookie.go b/internal/auth/cookie.go index 32a5473..df00244 100644 --- a/internal/auth/cookie.go +++ b/internal/auth/cookie.go @@ -154,6 +154,15 @@ func (c FlowCookie) Read(r *http.Request, v flowValue, now time.Time) error { return nil } +// maxPendingStates bounds each flow's pending states. The login route is +// unauthenticated, so without a bound anyone could grow the set without +// limit; one household never has close to this many flows in progress. +const maxPendingStates = 10000 + +// ErrTooManyPending means a flow's pending states are at maxPendingStates, +// so a new flow is refused until older ones expire. +var ErrTooManyPending = errors.New("too many sign-in flows in progress") + // 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 @@ -163,6 +172,15 @@ type PendingStates struct { ttl time.Duration mu sync.Mutex states map[string]time.Time + // order is every added state in the order added. Every state is pending + // for the same ttl, so it is also expiry order and Add forgets expired + // states from its front. + order []pendingState +} + +type pendingState struct { + state string + expires time.Time } // NewPendingStates returns an empty set whose states are pending for ttl, @@ -172,15 +190,23 @@ func NewPendingStates(ttl time.Duration) *PendingStates { } // Add records state, started at now, as pending and forgets expired ones. -func (p *PendingStates) Add(state string, now time.Time) { +// It fails with ErrTooManyPending when maxPendingStates are pending. +func (p *PendingStates) Add(state string, now time.Time) error { p.mu.Lock() defer p.mu.Unlock() - for k, expires := range p.states { - if !now.Before(expires) { - delete(p.states, k) - } + expired := 0 + for expired < len(p.order) && !now.Before(p.order[expired].expires) { + delete(p.states, p.order[expired].state) + expired++ } - p.states[state] = now.Add(p.ttl) + p.order = p.order[expired:] + if len(p.states) >= maxPendingStates { + return ErrTooManyPending + } + expires := now.Add(p.ttl) + p.states[state] = expires + p.order = append(p.order, pendingState{state: state, expires: expires}) + return nil } // Consume removes state from the pending states, reporting whether it was diff --git a/internal/gcal/connect.go b/internal/gcal/connect.go index f7962a8..b97f042 100644 --- a/internal/gcal/connect.go +++ b/internal/gcal/connect.go @@ -209,10 +209,12 @@ func (s *Service) Connect(w http.ResponseWriter, session auth.Session) (string, Session: sessionBinding(session), } now := s.now() + if err := s.pending.Add(state.State, now); err != nil { + return "", fmt.Errorf("gcal: %w", err) + } if err := s.cookie.Set(w, &state, now); err != nil { return "", fmt.Errorf("gcal: set connect cookie: %w", err) } - s.pending.Add(state.State, now) return s.oauth.AuthCodeURL(state.State, oauth2.AccessTypeOffline, oauth2.ApprovalForce,