fix(gotrue): avoid duplicate auth error on recoverSession with invalid refresh token#1450
fix(gotrue): avoid duplicate auth error on recoverSession with invalid refresh token#1450spydon wants to merge 10 commits into
Conversation
…d refresh token When recoverSession restores a persisted session with an invalid or expired refresh token, _executeRefresh already removes the session and emits a signedOut event. The blanket catch in recoverSession then called notifyException a second time, pushing an AuthException onto onAuthStateChange. For listeners without an onError handler this surfaces as an uncaught async error (reported via Sentry). Notification is now explicit per path: the refresh path defers to _callRefreshToken, unexpected parse failures are still notified, and the missing-data and session-expired paths keep their explicit notifications. Closes #930
github-actions
Bot
added
the
auth
Returning the _callRefreshToken future without awaiting keeps its error out of recoverSession's catch, so the refresh outcome is owned solely by _executeRefresh (signedOut for an invalid token, an exception for a retryable failure). All other error surfacing in recoverSession (parse errors, signOut failures, the explicit missing-data and session-expired notifications) is preserved, so existing behavior and tests are unchanged.
Calling _callRefreshToken after the try/catch (instead of returning an un-awaited future from inside it) keeps its error out of recoverSession's notification path while satisfying the prefer-return-await lint. Behavior is unchanged: the refresh outcome is owned solely by _executeRefresh.
|
Am I right that on a failed recovery there is only a signed out event without an exception on the stream? This removes the ability to have different handling or notification to the user why they got signed out. Also for future refreshes it still emits an exception to the stream on an issue, right? I don't think the solution to people not having a proper stream listener is to remove the errors. |
So there are two cases right:
Removing errors to paper over a missing We should probably add a reason to the |
|
Removing the additional redundant exception on the stream is great. I took now a deeper look in the existing code and saw that we no longer emit an exception on refresh_token_already_used on timer based refreshes as well. I had the impression that we emit an exception there as well. This got changed in a fix by you: I didn't catch this in that pr. So yeah this aligns the behavior of the initial refresh. I think adding a reason to the sign out event that it was not initiated by the user itself would be great. |
@Vinzent03 I opened a stacked PR implementing this here: #1453 |
|
I am currently thinking whether these changes and the one before is actually breaking? There is no analysis breaking change, but an application that depends on the auth exception being emitted would not work quite as before. Really unsure here. |
Returning the future without awaiting dropped recoverSession from the asynchronous stack trace. Await it (still outside the catch, so the error is not re-notified) so the frame is preserved for debugging.
…resh failure The refresh runs fire-and-forget through a completer, so a failure was rooted at _executeRefresh with no recoverSession or caller frames. Rethrow with the current stack (still outside the catch, so the error is not re-notified) so the call site is visible. Add a test asserting recoverSession appears in the trace.
With the one before, do you mean #1351? That is already released. Yeah I guess this is technically breaking in the sense that it is a behavior change, but I don't think it deserves a major bump due to how strange/unintentional it was behaving before. We should surely make a note of it in the changelog though. |
The missing-data and session-expired paths called notifyException explicitly and then threw, and the surrounding catch notified again, so two errors hit onAuthStateChange. Throw without the explicit notify and let the catch be the single notification point. Add a test asserting exactly one error is emitted.
2 participants
What
Fixes the long-standing reports of
AuthException(message: Invalid Refresh Token: Refresh Token Not Found, statusCode: 400)surfacing as an uncaught error (often seen in Sentry).Closes #930
Why
When
recoverSessionrestores a persisted session whose refresh token is invalid or expired, the refresh goes through_callRefreshToken->_executeRefresh. For a non-retryableAuthException(like "Refresh Token Not Found"),_executeRefreshalready does the right thing: it removes the session and emits asignedOutevent, deliberately not callingnotifyException.The problem was that
recoverSession's blanketcatchthen callednotifyException(error, stackTrace)a second time, pushing theAuthExceptiononto theonAuthStateChangestream. For an expected condition (a rotated, expired, or revoked token at app startup), consumers listening toonAuthStateChangewithout anonErrorhandler get an uncaught async error. That is what those Sentry reports were.How
Notification in
recoverSessionis now explicit per path instead of a catch-all re-notify:_callRefreshToken/_executeRefresh, the single owner of that outcome (signedOutfor an invalid token, an exception for a retryable failure). No more duplicate stream error.autoRefreshToken: false) notifications are preserved unchanged.Tests
Sign out on wrong refresh tokento assert the invalid-token path surfaces only asignedOutevent and never a stream error, while still throwing to the direct caller.autoRefreshToken: falsepath still emits its exception (supabase_flutter"emits exception when no auto refresh" still passes).