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

[federicobartoli]

Fixes #1151

Description

On iOS, RNAppAuth could keep an AppAuth session that had already completed and forward a later redirect URL to it. With AppAuth < 2.1.0 that raises an NSException and terminates the app. Details and reproduction are in #1151.

Two changes in ios/RNAppAuth.m, no API change:

1. Do not keep a session that completed during the presenting call. When the user-agent cannot be opened, AppAuth invokes the callback synchronously, before the presenting call returns. The callback cleared _currentSession, and then the assignment stored the finished session again. A __block flag set by the callbacks is now checked after the call, in both authorize and logout.
2. Forward at most one consumed redirect to a session. resumeExternalUserAgentFlowWithURL: now clears _currentSession once the session has accepted a URL. Before, the reference was kept until the code exchange finished, so a second matching URL arriving during the token request reached a finished session. The code exchange does not use the session: the token request only captures the authorization response and the callback.

state and PKCE validation are untouched; they stay inside AppAuth. A URL that arrives when no flow is pending returns NO and falls through to RCTLinkingManager, as it already does when no login was ever started.

Alternative I considered: requiring AppAuth >= 2.1.0 in the podspec would also remove the crash, but it changes other behaviour (2.1.0 removed the external-browser fallback, openid/AppAuth-iOS#954) and the module would still hold a finished session.

Possibly related: #1080 (same exception and stack). I have not reproduced that trigger, so I am not marking it as fixed.

Steps to verify

iOS simulator with examples/demo (AppAuth 2.0.0 from the demo Podfile.lock, Xcode 26.3, iPhone 17 Pro simulator on iOS 26.1).

The crash is gone

1. Apply the two temporary demo changes from iOS: redirect after a failed authorize() crashes the app (finished session kept as _currentSession) #1151 (iosCustomBrowser: 'chrome' in App.tsx, googlechromes in LSApplicationQueriesSchemes).
2. Tap Login with IdentityServer. An alert appears: "Failed to log in", Unable to open Safari.
3. Run xcrun simctl openurl booted "io.identityserver.demo:/oauthredirect?code=x&state=y".
4. Before this change the app crashes. With it, the URL is ignored and the app keeps running.

Login still works

• With iosCustomBrowser: 'safari': login in Safari, redirect back into the app, tokens are shown. This is the path that goes through resumeExternalUserAgentFlowWithURL:.
• Default user-agent (ASWebAuthenticationSession, no iosCustomBrowser): login completes, tokens are shown.

Automated checks

• jest (87 tests) and eslint . pass locally in packages/react-native-app-auth. Neither covers the Objective-C file.
• No automated test added: RNAppAuth.m has no native test harness (the existing native-tests script only covers RNAppAuthPresentation.m). Happy to add one if you can point me to the approach you prefer.
• A changeset is included.

Not tested

• logout: the demo app has no logout button. The change there is the same pattern as in authorize.
• A second redirect during the code exchange (change 2): based on reading the code, not reproduced.
• Android: this change is iOS only, and I have not checked Android for an equivalent problem.

[changeset-bot]

🦋 Changeset detected

Latest commit: aa18f25

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
react-native-app-auth	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

@federicobartoli is attempting to deploy a commit to the Nearform Team on Vercel.

A member of the Team first needs to authorize it.

[ilteoood]

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]

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.

[Francesco-Voto]

Good work!

Few comments from me as well:

The logic is correct. Clearing the session after the present call returns, and forwarding at most one redirect, closes the crash in #1151.

There are 7 present sites (6 in authorize, 1 in endSession) but only 3 callbacks.
The post-call check sits once per method, after all the branches. A new branch in authorize reuses an existing callback and is already covered.
The risk is a new flow copying the pattern without the check. A one-line comment at each check would be enough here. A helper that owns the flag would enforce it, but it can be a follow-up.

the callbacks do strongSelf->_currentSession = nil without a nil check, which would crash if self was deallocated. A if (!strongSelf) return; guard could go in a follow-up

[federicobartoli]

Thanks both!

@Francesco-Voto agreed on all points. I pushed a one-line comment at the endSession check; the authorize check already had one.

@ilteoood I said I'd move this into a single helper here. Following Francesco's suggestion I'll keep this PR minimal instead and send the helper as a follow-up, together with the nil guard on strongSelf. I have a draft of that version and will open it once this is merged, after re-running the same simulator checks.

[ilteoood]

LGTM, that's why I've already approved it

[zibs]

Thanks @federicobartoli I've published this as 8.5.1 as I get ready to publish 9.0.0 later