diff --git a/pkg/auth/servicetoken.go b/pkg/auth/servicetoken.go index bb72bf3..207e629 100644 --- a/pkg/auth/servicetoken.go +++ b/pkg/auth/servicetoken.go @@ -31,6 +31,23 @@ func atprotoErrorName(body []byte) string { return e.Error } +// isStaleBearerToken reports whether an atproto XRPC error name means the +// bearer token presented was rejected as expired or otherwise unusable, and so +// should be discarded rather than replayed. +// +// This is scoped to the app-password path, where the cached access token is the +// only credential state and dropping it simply forces a re-mint on the next +// authentication. Do not reuse this for OAuth sessions: ExpiredToken there is +// recoverable by refreshing, and treating it as fatal would sign users out. +func isStaleBearerToken(name string) bool { + switch name { + case "ExpiredToken", "InvalidToken", "ExpiredSession", "InvalidSession": + return true + default: + return false + } +} + // getErrorHint provides context-specific troubleshooting hints based on API error type func getErrorHint(apiErr *atclient.APIError) string { switch apiErr.Name { @@ -365,6 +382,30 @@ func GetOrFetchServiceTokenWithAppPassword( // Service auth failed bodyBytes, _ := io.ReadAll(resp.Body) InvalidateServiceToken(did, holdDID) + + // Some PDSes report a stale bearer token as 400 + an atproto error name + // rather than 401, so the status check above misses it. Without this the + // expired token stays in the cache, every subsequent request replays it, + // and the user is wedged until the process restarts: one account + // generated 16,110 of these and 4,254 retryable 503s over 33 hours. + // + // Dropping the cached token is the app-password equivalent of a refresh + // — the next authentication re-mints it via createSession. Note this is + // deliberately not routed through oauth.IsSessionInvalidError, which + // excludes ExpiredToken on purpose: there it would delete a recoverable + // OAuth session, whereas here the only thing discarded is a cache entry. + if name := atprotoErrorName(bodyBytes); isStaleBearerToken(name) { + GetGlobalTokenCache().Delete(did) + slog.Warn("App-password token reported stale by PDS, evicting from cache", + "component", "token/servicetoken", + "did", did, + "holdDID", holdDID, + "statusCode", resp.StatusCode, + "atprotoError", name, + "hint", "next authentication will re-mint; if it persists the user must re-authenticate") + return "", fmt.Errorf("app-password token stale (%s): re-authentication required", name) + } + slog.Error("Service token request returned non-200 status (app-password)", "component", "token/servicetoken", "did", did, diff --git a/pkg/auth/servicetoken_test.go b/pkg/auth/servicetoken_test.go index 94bc8e9..2b00b9b 100644 --- a/pkg/auth/servicetoken_test.go +++ b/pkg/auth/servicetoken_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "sync" "testing" "time" @@ -217,3 +218,89 @@ func TestGetOrFetchServiceToken_InvalidGrantDeletesSession(t *testing.T) { t.Errorf("expected UI session invalidation for %s, got: %v", did, uiStore.deleted) } } + +func TestIsStaleBearerToken(t *testing.T) { + stale := []string{"ExpiredToken", "InvalidToken", "ExpiredSession", "InvalidSession"} + for _, name := range stale { + if !isStaleBearerToken(name) { + t.Errorf("isStaleBearerToken(%q) = false, want true", name) + } + } + // Anything else must not discard the credential — an unrelated server-side + // failure should not force the user to re-authenticate. + for _, name := range []string{"", "InvalidRequest", "RateLimitExceeded", "InternalServerError"} { + if isStaleBearerToken(name) { + t.Errorf("isStaleBearerToken(%q) = true, want false", name) + } + } +} + +// TestAppPasswordServiceToken_EvictsOnStaleToken is the regression test for a +// wedged-account loop: a PDS reporting an expired bearer token as 400 with an +// atproto error name (rather than 401) left the dead token in the cache, so +// every subsequent request replayed it. One account produced 16,110 such errors +// and 4,254 retryable 503s over 33 hours, recovering only on process restart. +func TestAppPasswordServiceToken_EvictsOnStaleToken(t *testing.T) { + tests := []struct { + name string + status int + body string + wantEvicted bool + }{ + { + name: "400 with ExpiredToken evicts", + status: http.StatusBadRequest, + body: `{"error":"ExpiredToken","message":"Token has expired"}`, + wantEvicted: true, + }, + { + name: "401 evicts (pre-existing path)", + status: http.StatusUnauthorized, + body: `{"error":"AuthMissing"}`, + wantEvicted: true, + }, + { + name: "400 with an unrelated error keeps the token", + status: http.StatusBadRequest, + body: `{"error":"InvalidRequest","message":"bad aud"}`, + wantEvicted: false, + }, + { + name: "500 keeps the token", + status: http.StatusInternalServerError, + body: `{"error":"InternalServerError"}`, + wantEvicted: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pds := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(tt.body)) + })) + defer pds.Close() + + did := "did:plc:" + strings.ToLower(strings.ReplaceAll(t.Name(), "/", "")) + holdDID := "did:web:hold.example.com" + + GetGlobalTokenCache().Set(did, "stale-access-token", time.Hour) + t.Cleanup(func() { GetGlobalTokenCache().Delete(did) }) + + if _, err := GetOrFetchServiceTokenWithAppPassword( + context.Background(), did, holdDID, pds.URL, + ); err == nil { + t.Fatal("expected an error from a non-200 PDS response") + } + + _, stillCached := GetGlobalTokenCache().Get(did) + if tt.wantEvicted && stillCached { + t.Error("expected the stale app-password token to be evicted, but it is still cached") + } + if !tt.wantEvicted && !stillCached { + t.Error("token was evicted on an unrelated failure; the user is forced to re-authenticate needlessly") + } + }) + } +}