diff --git a/pkg/appview/handlers/oauth_errors.go b/pkg/appview/handlers/oauth_errors.go index 9a9ec41..313d7e0 100644 --- a/pkg/appview/handlers/oauth_errors.go +++ b/pkg/appview/handlers/oauth_errors.go @@ -41,11 +41,23 @@ func isOAuthError(err error) bool { } // Fallback: check for known auth-specific error strings that won't - // appear in digests or URIs + // appear in digests or URIs. + // + // Deliberately absent: use_dpop_nonce. It means the DPoP nonce was stale, + // which indigo normally handles by retrying with the server-supplied nonce — + // a routine handshake step, not a dead session. Treating it as an auth error + // signed users out over ordinary nonce rotation, the same + // transient-misclassified-as-fatal bug this file's detached-delete logic + // exists to avoid. + // + // It can still escape in one case: a server that returns error=use_dpop_nonce + // with no DPoP-Nonce header leaves indigo nothing to retry with, and the + // reason reaches us verbatim. We accept a stuck session there rather than + // signing every user out over the common case; a server doing that is + // broken, and the resulting failures are visible in the logs. errStr := strings.ToLower(err.Error()) return strings.Contains(errStr, "invalid_token") || strings.Contains(errStr, "invalid_grant") || - strings.Contains(errStr, "use_dpop_nonce") || strings.Contains(errStr, "authentication failed") || strings.Contains(errStr, "token expired") } @@ -65,7 +77,9 @@ func handleOAuthError(ctx context.Context, refresher *oauth.Refresher, did strin // Invalidate all UI sessions for this DID. Detached context: once we // decide to delete, the cleanup must finish even if the inbound request // is canceled mid-way. - if delErr := refresher.DeleteSession(context.WithoutCancel(ctx), did); delErr != nil { + delCtx, cancelDel := context.WithTimeout(context.WithoutCancel(ctx), oauth.SessionDeleteTimeout) + defer cancelDel() + if delErr := refresher.DeleteSession(delCtx, did); delErr != nil { slog.Warn("Failed to delete OAuth session after error", "component", "handlers", "did", did, diff --git a/pkg/auth/oauth/client.go b/pkg/auth/oauth/client.go index 439e136..86384d6 100644 --- a/pkg/auth/oauth/client.go +++ b/pkg/auth/oauth/client.go @@ -21,6 +21,13 @@ import ( "github.com/bluesky-social/indigo/xrpc" ) +// SessionDeleteTimeout bounds a detached session delete. These deletes run on +// context.WithoutCancel so a canceled inbound request cannot leave the cleanup +// half-done — but that also strips the only deadline they had, so without a +// replacement a wedged database write blocks the calling goroutine forever with +// no way to shed it. Matches the bound on the detached persist callback. +const SessionDeleteTimeout = 10 * time.Second + // permissionSetExpansions maps lexicon IDs to their expanded scope format. // These must match the collections defined in lexicons/io/atcr/authFullApp.json // Collections are sorted alphabetically for consistent comparison with PDS-expanded scopes. @@ -320,7 +327,9 @@ func (r *Refresher) DoWithSession(ctx context.Context, did string, fn func(sessi // context: once we decide to delete, the cleanup must finish even if // the inbound request is canceled mid-way. mutex.Unlock() - _ = r.DeleteSession(context.WithoutCancel(ctx), did) + delCtx, cancelDel := context.WithTimeout(context.WithoutCancel(ctx), SessionDeleteTimeout) + _ = r.DeleteSession(delCtx, did) + cancelDel() mutex.Lock() // Re-acquire for the deferred unlock } @@ -357,15 +366,32 @@ func IsSessionInvalidError(err error) bool { if apiErr.StatusCode == 401 { return true } + // atproto XRPC error names, which are camel-case. The string match + // below handles a *different* family (OAuth 2.0 token-endpoint codes, + // snake_case) and cannot catch these — the mismatch is the underscore, + // not the case, so lowercasing "InvalidToken" still never reaches + // "invalid_token". Every XRPC name meaning "this session is dead" has + // to be listed right here. + // Deliberately absent: ExpiredToken. It means "refresh me", not "this + // session is revoked" — deleting on it signs the user out of every UI + // session over an ordinary access-token expiry that a refresh would + // have fixed. A genuinely dead session still gets caught by the 401 + // status check above. isAuthError (below) omits it for the same reason; + // the two classifiers must agree about the same condition. switch apiErr.Name { case "InvalidToken", "InvalidGrant", "InsufficientScope": return true } } - // The token-refresh failure from indigo arrives as a plain wrapped error - // ("auth server request failed (HTTP 400): invalid_grant"), not an - // APIError, so a string fallback is required. These substrings are + // OAuth 2.0 token-endpoint failures, which are a separate error family from + // the XRPC names above rather than a looser spelling of them. indigo's + // RefreshTokens returns them as a bare fmt.Errorf carrying the auth + // server's error code verbatim ("token refresh failed (HTTP 400): + // invalid_grant") — never an APIError, never a wrapped typed error — so + // matching the string is the only way to classify them. RFC 6749 defines + // these codes as snake_case, hence the different spellings; ToLower is only + // belt-and-braces for a server that deviates. The substrings are // auth-specific and won't appear in digests or URIs. errStr := strings.ToLower(err.Error()) return strings.Contains(errStr, "invalid_grant") || diff --git a/pkg/auth/oauth/client_test.go b/pkg/auth/oauth/client_test.go index bcc7177..cbf399c 100644 --- a/pkg/auth/oauth/client_test.go +++ b/pkg/auth/oauth/client_test.go @@ -380,6 +380,18 @@ func TestIsSessionInvalidError(t *testing.T) { {"api error InvalidGrant", &atclient.APIError{StatusCode: 400, Name: "InvalidGrant"}, true}, {"api error InvalidToken", &atclient.APIError{StatusCode: 400, Name: "InvalidToken"}, true}, {"api error 500", &atclient.APIError{StatusCode: 500, Name: "InternalServerError"}, false}, + // ExpiredToken means "refresh me", not "revoked". Treating it as a dead + // session signs the user out of every UI session over an ordinary + // access-token expiry that a refresh would have fixed. + {"api error ExpiredToken is refreshable, not dead", &atclient.APIError{StatusCode: 400, Name: "ExpiredToken"}, false}, + // Transient upstream failures must never evict: these are the shapes the + // service-token path now wraps as APIErrors. + {"api error 502", &atclient.APIError{StatusCode: 502, Name: ""}, false}, + {"api error 429", &atclient.APIError{StatusCode: 429, Name: ""}, false}, + {"api error 500 html body", &atclient.APIError{StatusCode: 500, Name: "", Message: "bad gateway"}, false}, + // A revoked session reported as 401 with an atproto name — the case the + // service-token path was previously flattening into an unmatchable string. + {"api error 401 InvalidToken", &atclient.APIError{StatusCode: 401, Name: "InvalidToken"}, true}, // The refresh-replay failure arrives as a plain wrapped string from indigo. {"plain invalid_grant string", errors.New("failed to refresh OAuth tokens: token refresh failed (HTTP 400): invalid_grant"), true}, {"plain invalid_token string", errors.New("auth server request failed (HTTP 401): invalid_token"), true}, diff --git a/pkg/auth/oauth/transport.go b/pkg/auth/oauth/transport.go index fa71c64..396fa32 100644 --- a/pkg/auth/oauth/transport.go +++ b/pkg/auth/oauth/transport.go @@ -13,9 +13,20 @@ import ( // refreshDetachTimeout bounds a detached token-refresh POST. Refreshes are // normally sub-second; this only exists so a hung auth server can't pin the // per-DID session lock forever. +// +// Note this is per-POST, not per-refresh: indigo's postToAuthServer retries once +// on a use_dpop_nonce challenge, so a single refresh can issue two detached +// POSTs and hold the per-DID lock for up to 2x this value. Keep that product +// comfortably under the callers' own deadlines (Docker gives up on /auth/token +// well before it). const refreshDetachTimeout = 30 * time.Second -var badTokenEndpointLogOnce sync.Once +// badTokenEndpointOnce dedupes the "couldn't parse the token endpoint" warning +// per endpoint value rather than once per process. That path fails open — it +// returns the client unwrapped, silently reinstating the refresh-burn bug this +// file exists to fix — so it must stay visible for every distinct auth server +// it affects, not just the first one after boot. +var badTokenEndpointOnce sync.Map // refreshDetachTransport detaches the request context for OAuth token-refresh // POSTs. A refresh is non-idempotent: the auth server rotates the refresh @@ -74,12 +85,12 @@ func (c *cancelOnClose) Close() error { func newRefreshDetachClient(inner *http.Client, tokenEndpoint string) *http.Client { endpoint, err := url.Parse(tokenEndpoint) if err != nil || endpoint.Host == "" { - badTokenEndpointLogOnce.Do(func() { + if _, seen := badTokenEndpointOnce.LoadOrStore(tokenEndpoint, struct{}{}); !seen { slog.Warn("Not detaching token-refresh context: unparseable auth server token endpoint", "component", "oauth/refresher", "tokenEndpoint", tokenEndpoint, "error", err) - }) + } return inner } diff --git a/pkg/auth/servicetoken.go b/pkg/auth/servicetoken.go index 5faa039..bb72bf3 100644 --- a/pkg/auth/servicetoken.go +++ b/pkg/auth/servicetoken.go @@ -17,6 +17,20 @@ import ( indigo_oauth "github.com/bluesky-social/indigo/atproto/auth/oauth" ) +// atprotoErrorName extracts the atproto error name from an XRPC error body +// ({"error":"ExpiredToken","message":"..."}). Returns "" when the body is not +// JSON or carries no name, which callers treat as "unclassified" rather than +// as any particular failure. +func atprotoErrorName(body []byte) string { + var e struct { + Error string `json:"error"` + } + if json.Unmarshal(body, &e) != nil { + return "" + } + return e.Error +} + // getErrorHint provides context-specific troubleshooting hints based on API error type func getErrorHint(apiErr *atclient.APIError) string { switch apiErr.Name { @@ -164,8 +178,20 @@ func GetOrFetchServiceToken( "pdsEndpoint", pdsEndpoint, "statusCode", resp.StatusCode, "responseBody", string(bodyBytes), + "errorName", atprotoErrorName(bodyBytes), "hint", "PDS rejected the service token request - check PDS logs for details") - fetchErr = fmt.Errorf("service auth failed with status %d: %s", resp.StatusCode, string(bodyBytes)) + // Wrap a typed APIError rather than flattening the response into a + // string. This error is what IsSessionInvalidError later inspects to + // decide whether the session is genuinely dead or the failure was + // transient, and it can only tell from the status code and the + // atproto error name. Flattening left it with a lowercased blob in + // which "InvalidToken" no longer matches "invalid_token", so a + // revoked session was classified transient and never evicted. + fetchErr = fmt.Errorf("service auth failed: %w", &atclient.APIError{ + StatusCode: resp.StatusCode, + Name: atprotoErrorName(bodyBytes), + Message: string(bodyBytes), + }) return fetchErr } @@ -223,7 +249,9 @@ func GetOrFetchServiceToken( // everywhere over a blip. The delete runs on a detached context so a // canceled inbound request can't leave it half-done. if oauth.IsSessionInvalidError(err) { - if delErr := refresher.DeleteSession(context.WithoutCancel(ctx), did); delErr != nil { + delCtx, cancelDel := context.WithTimeout(context.WithoutCancel(ctx), oauth.SessionDeleteTimeout) + defer cancelDel() + if delErr := refresher.DeleteSession(delCtx, did); delErr != nil { slog.Warn("Failed to delete stale OAuth session", "component", "token/servicetoken", "did", did,