Skip to content

fix: do not forward redirects to a completed session on iOS #1152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zibs merged 2 commits into from
Oct 19, 2026
Merged

fix: do not forward redirects to a completed session on iOS #1152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d221011
fix: do not forward redirects to a completed session on iOS
federicobartoli Oct 9, 2026
aa18f25
chore: comment the completed-flow check in logout
federicobartoli Oct 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-ios-completed-session.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-native-app-auth": patch
---

Fix an iOS crash ("An OAuth redirect was sent to a OIDExternalUserAgentSession after it already completed") by not keeping an authorization or logout session that completed synchronously, and by forwarding at most one redirect to each session.
33 changes: 32 additions & 1 deletion packages/react-native-app-auth/ios/RNAppAuth.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,18 @@ @interface RNAppAuth()<RNAppAuthAuthorizationFlowManagerDelegate> {
@implementation RNAppAuth

-(BOOL)resumeExternalUserAgentFlowWithURL:(NSURL *)url {
return [_currentSession resumeExternalUserAgentFlowWithURL:url];
id<OIDExternalUserAgentSession> session = _currentSession;
if (session == nil) {
return NO;
}
BOOL handled = [session resumeExternalUserAgentFlowWithURL:url];
// A session consumes at most one redirect. Once it has accepted one, stop forwarding URLs to
// it: the code exchange that may still be running does not need the session, and AppAuth
// < 2.1.0 raises an NSException when a completed session is resumed again.
if (handled && _currentSession == session) {
_currentSession = nil;
}
return handled;
}

- (dispatch_queue_t)methodQueue
Expand Down Expand Up @@ -374,6 +385,9 @@ - (void)authorizeWithConfiguration: (OIDServiceConfiguration *) configuration
}
appDelegate.authorizationFlowManagerDelegate = self;
__weak typeof(self) weakSelf = self;
// Set when the flow's callback has run. The callback can run synchronously, before the
// presenting call below returns (e.g. when the user-agent fails to open).
__block BOOL flowCompleted = NO;

rnAppAuthTaskId = [UIApplication.sharedApplication beginBackgroundTaskWithExpirationHandler:^{
[UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
Expand All @@ -388,6 +402,7 @@ - (void)authorizeWithConfiguration: (OIDServiceConfiguration *) configuration

OIDAuthorizationCallback callback = ^(OIDAuthorizationResponse *_Nullable authorizationResponse, NSError *_Nullable error) {
typeof(self) strongSelf = weakSelf;
flowCompleted = YES;
strongSelf->_currentSession = nil;
[UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
rnAppAuthTaskId = UIBackgroundTaskInvalid;
Expand Down Expand Up @@ -424,6 +439,7 @@ - (void)authorizeWithConfiguration: (OIDServiceConfiguration *) configuration
NSError *_Nullable error
) {
typeof(self) strongSelf = weakSelf;
flowCompleted = YES;
strongSelf->_currentSession = nil;
[UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
rnAppAuthTaskId = UIBackgroundTaskInvalid;
Expand Down Expand Up @@ -456,6 +472,12 @@ - (void)authorizeWithConfiguration: (OIDServiceConfiguration *) configuration
}
}
}

// If the callback already ran, the session returned above has completed: do not keep it,
// otherwise a later redirect would be forwarded to a finished session.
if (flowCompleted) {

@ilteoood ilteoood Oct 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new __block BOOL flowCompleted flag is set in three callbacks (lines 405, 442, 568) and read once on this line 478 (and again at 581 in endSessionWithConfiguration:). The invariant "callback fired synchronously → flag is YES → drop the session" lives in the heads of maintainers, not in the code. AppAuth 2.0.0's OIDAuthorizationService.presentAuthorizationRequest: factory returns the session unconditionally even after didFinishWithResponse: fired synchronously, so a future present-callback author who forgets to set the flag silently re-introduces the exact crash this PR closes — no compile error, no test failure.

Why it needs to change

  • Six present-call sites today (skipCodeExchange × externalUserAgent × iOS-13 availability, plus endSession). An eighth within a year is plausible (new provider, new browser, an OIDAuthState overload the author hasn't read). Each new callback widens the open trap.
  • The post-present check on this very line is the only safeguard against the race; today it depends on every present-callback author remembering a magic three-word declaration that lives 100 lines earlier in the same method.

Code suggestion

Wrap the flag-set inside a tiny adapter the present callbacks always go through, so flowCompleted is unreachable directly:

__block BOOL didCallbackFireBeforePresentReturned = NO;
typeof(self) __weak weakSelf = self;
OIDAuthorizationCallback wrappedCallback = ^(OIDAuthorizationResponse *_Nullable response, NSError *_Nullable error) {
    didCallbackFireBeforePresentReturned = YES;
    typeof(self) strongSelf = weakSelf;
    strongSelf->_currentSession = nil;
    [UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
    rnAppAuthTaskId = UIBackgroundTaskInvalid;
    if (response) {
        resolve([self formatAuthorizationResponse:response withCodeVerifier:codeVerifier]);
    } else {
        [self rejectPromise:reject defaultCode:@"authentication_failed" error:error];
    }
};
_currentSession = [OIDAuthorizationService presentAuthorizationRequest:request
                                                externalUserAgent:externalUserAgent
                                                         callback:wrappedCallback];
if (didCallbackFireBeforePresentReturned) {
    _currentSession = nil;
}

Or, more aggressively: factor the six present calls plus their six callbacks into a single -presentAndMarkFired:externalUserAgent:callback: helper, and a sibling for end-session. The flag then exists in exactly one location per flow type.

Why this differs

  • The crash this PR closes becomes structurally hard to regress instead of structurally easy: a new present-callback author can't avoid the flag because the wrappedCallback already sets it.
  • All six present sites lose the duplicated flowCompleted = YES; line plus the inline __block declaration (18 lines removed, ~24 added across both methods). The maintainability win is bigger than the diff cost.
  • If a native test harness is ever wired up, the wrapper makes a one-shot unit test of "present fails synchronously → _currentSession is nil afterwards" trivially possible; today the same test would need to hook into six separate callback bodies.

@federicobartoli federicobartoli Oct 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick review, and for the approval!

Good point about the invariant. Today the three callbacks set the flag themselves (the six authorize call sites share two callback blocks, plus one for logout), so a new callback could indeed skip it. I'll go with your second suggestion and route every flow through a single helper that wraps the callback and decides whether to keep the session, so the bookkeeping lives in one place. I'll re-run the verification steps on the simulator and push an update.

_currentSession = nil;
}
}

/*
Expand Down Expand Up @@ -523,6 +545,9 @@ - (void)endSessionWithConfiguration: (OIDServiceConfiguration *) configuration
}
appDelegate.authorizationFlowManagerDelegate = self;
__weak typeof(self) weakSelf = self;
// Set when the flow's callback has run. The callback can run synchronously, before the
// presenting call below returns (e.g. when the user-agent fails to open).
__block BOOL flowCompleted = NO;

rnAppAuthTaskId = [UIApplication.sharedApplication beginBackgroundTaskWithExpirationHandler:^{
[UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
Expand All @@ -540,6 +565,7 @@ - (void)endSessionWithConfiguration: (OIDServiceConfiguration *) configuration
externalUserAgent: externalUserAgent
callback: ^(OIDEndSessionResponse *_Nullable response, NSError *_Nullable error) {
typeof(self) strongSelf = weakSelf;
flowCompleted = YES;
strongSelf->_currentSession = nil;
[UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
rnAppAuthTaskId = UIBackgroundTaskInvalid;
Expand All @@ -551,6 +577,11 @@ - (void)endSessionWithConfiguration: (OIDServiceConfiguration *) configuration
error:error];
}
}];

// As in authorizeWithConfiguration:, do not keep a session whose callback already ran.
if (flowCompleted) {
_currentSession = nil;
}
}

- (void)configureUrlSession: (NSDictionary*) headers sessionTimeout: (double) sessionTimeout{
Expand Down
Loading