A saved copy of the signed login cookie verified until it expired, so the callback accepted it again with a second valid authorization code. The login now records its state as pending and the callback consumes it, refusing a replay with the same 400 as a state mismatch. The pending-state set moves from gcal into auth as PendingStates, so login and the Google Calendar connect share one mechanism. It is in memory, so a flow started before a restart fails closed after it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+13
-2
@@ -79,6 +79,7 @@ type Auth struct {
|
|||||||
provider *oidc.Provider
|
provider *oidc.Provider
|
||||||
client *http.Client
|
client *http.Client
|
||||||
cookie loginCookie
|
cookie loginCookie
|
||||||
|
pending *PendingStates
|
||||||
sessions Sessions
|
sessions Sessions
|
||||||
allowed []string
|
allowed []string
|
||||||
// endSessionURL is the provider's end_session_endpoint; logout ends
|
// 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,
|
client: client,
|
||||||
allowed: slices.Clone(cfg.AllowedGroups),
|
allowed: slices.Clone(cfg.AllowedGroups),
|
||||||
cookie: newLoginCookie(cfg.SecretKey),
|
cookie: newLoginCookie(cfg.SecretKey),
|
||||||
|
pending: NewPendingStates(loginCookieTTL),
|
||||||
sessions: sessions,
|
sessions: sessions,
|
||||||
endSessionURL: discovered.EndSessionURI,
|
endSessionURL: discovered.EndSessionURI,
|
||||||
logger: logger,
|
logger: logger,
|
||||||
@@ -250,11 +252,13 @@ func (a *Auth) login(w http.ResponseWriter, r *http.Request) {
|
|||||||
Verifier: oauth2.GenerateVerifier(),
|
Verifier: oauth2.GenerateVerifier(),
|
||||||
Return: returnTo,
|
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)
|
a.logger.ErrorContext(r.Context(), "set login cookie", "error", err)
|
||||||
http.Error(w, "internal error", http.StatusInternalServerError)
|
http.Error(w, "internal error", http.StatusInternalServerError)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
a.pending.Add(state.State, now)
|
||||||
http.Redirect(w, r, a.oauth.AuthCodeURL(state.State,
|
http.Redirect(w, r, a.oauth.AuthCodeURL(state.State,
|
||||||
oidc.Nonce(state.Nonce),
|
oidc.Nonce(state.Nonce),
|
||||||
oauth2.S256ChallengeOption(state.Verifier),
|
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,
|
// 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
|
// 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.
|
// login, so an account removed from the group is refused at its next one.
|
||||||
func (a *Auth) callback(w http.ResponseWriter, r *http.Request) {
|
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)
|
http.Error(w, "sign-in state mismatch; start again", http.StatusBadRequest)
|
||||||
return
|
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") != "" {
|
if q.Get("error") != "" {
|
||||||
// error and error_description are provider-supplied text; only the
|
// error and error_description are provider-supplied text; only the
|
||||||
// fixed category is logged.
|
// fixed category is logged.
|
||||||
|
|||||||
@@ -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) {
|
func TestCallbackRejected(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ import (
|
|||||||
"net/http"
|
"net/http"
|
||||||
"net/url"
|
"net/url"
|
||||||
"strings"
|
"strings"
|
||||||
|
"sync"
|
||||||
"time"
|
"time"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -153,6 +154,45 @@ func (c FlowCookie) Read(r *http.Request, v flowValue, now time.Time) error {
|
|||||||
return nil
|
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 "/".
|
// returnPath validates the optional post-login return path. Empty means "/".
|
||||||
// Only same-origin absolute paths are allowed: no scheme, host, protocol-
|
// Only same-origin absolute paths are allowed: no scheme, host, protocol-
|
||||||
// relative "//" prefix, backslashes or control characters.
|
// relative "//" prefix, backslashes or control characters.
|
||||||
|
|||||||
@@ -137,10 +137,9 @@ type Service struct {
|
|||||||
account sync.Mutex
|
account sync.Mutex
|
||||||
|
|
||||||
// pending holds the state of every connect started and not yet
|
// pending holds the state of every connect started and not yet
|
||||||
// completed, with when it expires. Complete consumes a state once, so a
|
// completed. Complete consumes a state once, so a replayed cookie finds
|
||||||
// replayed cookie finds it gone.
|
// it gone.
|
||||||
pendingMu sync.Mutex
|
pending *auth.PendingStates
|
||||||
pending map[string]time.Time
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// NewService returns the service over st, sealing refresh tokens with box.
|
// 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),
|
cookie: auth.NewFlowCookie(connectCookieName, connectCookieKeyLabel, connectCookieTTL, cfg.SecretKey),
|
||||||
retryDelay: firstRetryDelay,
|
retryDelay: firstRetryDelay,
|
||||||
now: time.Now,
|
now: time.Now,
|
||||||
pending: map[string]time.Time{},
|
pending: auth.NewPendingStates(connectCookieTTL),
|
||||||
}, nil
|
}, 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 {
|
if err := s.cookie.Set(w, &state, now); err != nil {
|
||||||
return "", fmt.Errorf("gcal: set connect cookie: %w", err)
|
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,
|
return s.oauth.AuthCodeURL(state.State,
|
||||||
oauth2.AccessTypeOffline,
|
oauth2.AccessTypeOffline,
|
||||||
oauth2.ApprovalForce,
|
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
|
// A copy of the cookie verifies until it expires, so the state is
|
||||||
// single-use only because it is consumed here.
|
// 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)
|
return fmt.Errorf("%w: already used", ErrBadState)
|
||||||
}
|
}
|
||||||
if q.Get("error") != "" {
|
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)
|
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
|
// checkScopes fails with ErrScopes unless token's granted scopes include
|
||||||
// every scope in Scopes. Google lets the household untick a permission on
|
// every scope in Scopes. Google lets the household untick a permission on
|
||||||
// the consent screen and reports what was granted in "scope".
|
// the consent screen and reports what was granted in "scope".
|
||||||
|
|||||||
Reference in New Issue
Block a user