From 2a8f897e61e2be278018224cd8f021d2424f4df0 Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Tue, 22 Sep 2026 20:30:43 +0800 Subject: [PATCH] fix(api): a session-store outage answers 503, not 401 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolving a session cookie failed identically whether the credential was missing or Postgres was unreachable: local_auth_enabled read errors fell into the fail-closed 'disabled' branch and SessionUser errors into 'invalid session', both surfacing as 401 'authentication required' — a lie that reads as 'log in again' during an outage. Split the enabled-read into (enabled, error), tag non-ErrNotFound store failures with errAuthBackend, and map that to a new 503 auth_unavailable in requireExternal. Fail-closed is unchanged: missing setting / bad value / missing session stay 401. --- internal/api/api_test.go | 45 ++++++++++++++++++++++++++++++++++++++ internal/api/errors.go | 9 ++++++-- internal/api/middleware.go | 6 +++++ internal/api/session.go | 41 +++++++++++++++++++++++++++++----- 4 files changed, 93 insertions(+), 8 deletions(-) diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 888470c..4c0b27a 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -59,6 +59,10 @@ type fakeRepo struct { staff map[string]*StaffUser // username -> staff login row sessions map[string]*fakeSession // token_hash -> session settings map[string][]byte // key -> jsonb value + // failSessionUser / failGetSetting force those reads to fail with a generic + // (non-ErrNotFound) error, simulating a store outage for the 503 auth path. + failSessionUser error + failGetSetting error // player email OTPs (spec §B2). Keyed by row id; the verify path scans for the // newest live (user, purpose) just as the PG query does. otps map[string]*fakeEmailOTP @@ -768,6 +772,9 @@ func (f *fakeRepo) CreateSession(_ context.Context, tokenHash, userID string, ex return nil } func (f *fakeRepo) SessionUser(_ context.Context, tokenHash string, now time.Time) (*SessionedUser, error) { + if f.failSessionUser != nil { + return nil, f.failSessionUser + } s, ok := f.sessions[tokenHash] if !ok || s.revoked || !s.expiresAt.After(now) { return nil, ErrNotFound @@ -788,6 +795,9 @@ func (f *fakeRepo) RevokeSession(_ context.Context, tokenHash string) error { return nil } func (f *fakeRepo) GetSetting(_ context.Context, key string) ([]byte, error) { + if f.failGetSetting != nil { + return nil, f.failGetSetting + } if v, ok := f.settings[key]; ok { return v, nil } @@ -2195,3 +2205,38 @@ func TestAccessVerifier(t *testing.T) { } }) } + +// TestSessionAuthOutageIs503Not401: a session-store outage must surface as 503 +// auth_unavailable, not a 401 that reads as "please log in again". Both failure +// points are covered — the local_auth_enabled read and the session row read — +// plus the regression that a genuinely missing session still answers 401. +func TestSessionAuthOutageIs503Not401(t *testing.T) { + apiWith := func(repo *fakeRepo) *API { + a := newTestAPI(repo, newFakeCluster()) + a.External = SessionAuth{Repo: repo, RootDomain: testRoot, AdminHostname: "op.console." + testRoot} + return a + } + cookie := map[string]string{"Cookie": sessionCookieName + "=any"} + outage := errors.New("dial tcp 10.0.0.5:5432: connect: connection refused") + + repo := newFakeRepo() + repo.settings[LocalAuthEnabledKey] = []byte("true") + repo.failGetSetting = outage + if w := do(apiWith(repo).ExternalHandler(), "GET", "/api/v1/me", "", cookie); w.Code != http.StatusServiceUnavailable || decodeErr(t, w) != "auth_unavailable" { + t.Fatalf("settings read outage = %d body %s, want 503 auth_unavailable", w.Code, w.Body.String()) + } + + repo = newFakeRepo() + repo.settings[LocalAuthEnabledKey] = []byte("true") + repo.failSessionUser = outage + if w := do(apiWith(repo).ExternalHandler(), "GET", "/api/v1/me", "", cookie); w.Code != http.StatusServiceUnavailable || decodeErr(t, w) != "auth_unavailable" { + t.Fatalf("session read outage = %d body %s, want 503 auth_unavailable", w.Code, w.Body.String()) + } + + // Regression: fail-closed auth (missing/invalid session) stays a 401. + repo = newFakeRepo() + repo.settings[LocalAuthEnabledKey] = []byte("true") + if w := do(apiWith(repo).ExternalHandler(), "GET", "/api/v1/me", "", cookie); w.Code != http.StatusUnauthorized || decodeErr(t, w) != "unauthorized" { + t.Fatalf("missing session = %d body %s, want 401 unauthorized", w.Code, w.Body.String()) + } +} diff --git a/internal/api/errors.go b/internal/api/errors.go index 1e2f38b..a79a0f3 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -89,8 +89,13 @@ func newError(status int, code, format string, a ...any) *apiError { // Common errors reused across handlers. var ( errUnauthorized = newError(http.StatusUnauthorized, "unauthorized", "authentication required") - errForbidden = newError(http.StatusForbidden, "forbidden", "not permitted") - errBadRequest = newError(http.StatusBadRequest, "bad_request", "invalid request") + // errAuthUnavailable answers when the session store itself is unreachable + // (Postgres down): an outage is not a credential verdict, so the caller gets + // 503 "retry" instead of a 401 that reads as "log in again". + errAuthUnavailable = newError(http.StatusServiceUnavailable, "auth_unavailable", + "authentication is temporarily unavailable; retry shortly") + errForbidden = newError(http.StatusForbidden, "forbidden", "not permitted") + errBadRequest = newError(http.StatusBadRequest, "bad_request", "invalid request") ) // writeJSON writes v as an indented JSON body with the given status. diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 66ba387..763d849 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rand" "encoding/hex" + "errors" "log" "net/http" "runtime/debug" @@ -95,6 +96,11 @@ func (a *API) requireInternal(next http.Handler) http.Handler { func (a *API) requireExternal(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { p, err := a.External.Authenticate(r) + if errors.Is(err, errAuthBackend) { + // Session store unreachable — an outage, not a missing credential. + writeError(w, r, errAuthUnavailable) + return + } if err != nil || p == nil { writeError(w, r, errUnauthorized) return diff --git a/internal/api/session.go b/internal/api/session.go index e966725..ac1a91b 100644 --- a/internal/api/session.go +++ b/internal/api/session.go @@ -7,6 +7,7 @@ import ( "encoding/base64" "encoding/hex" "encoding/json" + "errors" "fmt" "net" "net/http" @@ -145,14 +146,24 @@ func (s SessionAuth) Authenticate(r *http.Request) (*Principal, error) { } ctx := r.Context() - if !localAuthEnabled(ctx, s.Repo) { + enabled, err := localAuthEnabledStatus(ctx, s.Repo) + if err != nil { + // The session store is unreachable: this is an outage, not a verdict on + // the caller's credentials, so the middleware answers 503 rather than a + // misleading "please log in". + return nil, fmt.Errorf("%w: %v", errAuthBackend, err) + } + if !enabled { // A cookie was presented but local auth is off: reject, never fall through. return nil, fmt.Errorf("local auth disabled") } u, err := s.Repo.SessionUser(ctx, hashCookie(cookie.Value), s.now()) - if err != nil { + switch { + case errors.Is(err, ErrNotFound): return nil, fmt.Errorf("invalid session: %w", err) + case err != nil: + return nil, fmt.Errorf("%w: %v", errAuthBackend, err) } return &Principal{ UserID: u.ID, @@ -164,6 +175,11 @@ func (s SessionAuth) Authenticate(r *http.Request) (*Principal, error) { }, nil } +// errAuthBackend marks an authentication failure caused by the session store +// being unreachable (e.g. Postgres down) rather than by a missing or invalid +// credential. Middleware maps it to 503 so an outage is not misreported as 401. +var errAuthBackend = errors.New("auth backend unavailable") + // localAuthEnabled reports whether the runtime local_auth_enabled toggle is true. // A missing setting, a read error, or a non-true value all read as disabled — the // gate fails closed so local sessions are honored, and new ones minted, only on an @@ -171,15 +187,28 @@ func (s SessionAuth) Authenticate(r *http.Request) (*Principal, error) { // (minting one) consult it, so the two never disagree about whether local auth is // live. func localAuthEnabled(ctx context.Context, repo Repo) bool { + enabled, _ := localAuthEnabledStatus(ctx, repo) + return enabled +} + +// localAuthEnabledStatus is localAuthEnabled with the outage case kept apart: a +// MISSING setting (ErrNotFound — never enabled) reads as (false, nil), while a +// store read failure reads as (false, err) so SessionAuth can tell "local auth +// is off" (401) from "the database is down" (503). An unreadable value still +// fails closed as disabled — it is a config fault, not an outage. +func localAuthEnabledStatus(ctx context.Context, repo Repo) (bool, error) { raw, err := repo.GetSetting(ctx, LocalAuthEnabledKey) - if err != nil { - return false // ErrNotFound (never enabled) or a transient read error → closed + switch { + case errors.Is(err, ErrNotFound): + return false, nil + case err != nil: + return false, err } var enabled bool if err := json.Unmarshal(raw, &enabled); err != nil { - return false + return false, nil } - return enabled + return enabled, nil } // ensure SessionAuth satisfies ExternalAuth at compile time.