SetBrokerContinuationEventArgs crashes on non-broker urls

Which Version of ADAL are you using ?
5.2.3

Which platform has the issue?
Xamarin.iOS

What authentication flow has the issue?

Desktop / Mobile
- [ X ] Interactive

Other? - please describe;

Is this a new or existing app?
a. The app is in production, and I have upgraded to a new version of ADAL

Repro

public override bool OpenUrl(UIApplication application, NSUrl url, string sourceApplication, NSObject annotation)
{
    if (AuthenticationContinuationHelper.IsBrokerResponse(sourceApplication))
    {
        AuthenticationContinuationHelper.SetBrokerContinuationEventArgs(url);
    }

    //OMITTED: Url handling for other types of urls
}

Expected behavior
Since IsBrokerResponse can't be fully relied on anymore since iOS 13, I expected SetBrokerContinuationEventArgs to gracefully handle non-broker urls.

Actual behavior
Since the IsBrokerResponse method is broken due to iOS 13 limitations (it returns true if sourceApplication is null) the SetBrokerContinuationEventArgs will be invoked with nearly every call to the app delegate's OpenUrl method (including those unrelated to the broker)

The problem here is that SetBrokerContinuationEventArgs crashes with a null reference exception every time it is called with a non-broker url.

Possible Solution
It seems the bug is related to this bit of code:

azure-activedirectory-library-for-dotnet/src/Microsoft.IdentityModel.Clients.ActiveDirectory/Platforms/iOS/iOSBroker.cs

Lines 317 to 321 in 4c9c9ea

    
           public static void SetBrokerResponse(NSUrl responseUrl) 
        
           { 
        
               brokerResponse = responseUrl; 
        
               brokerResponseReady.Release(); 
        
           }

brokerResponseReady is null here (because the broker flow was likely never initiated in case of a non-broker url coming in) causing a null pointer related crash on the brokerResponseReady.Release() line.

A simple null check like this would probably suffice:

public static void SetBrokerResponse(NSUrl responseUrl)
{
    if(brokerResponseReady == null)
    {
        //No broker flow initiated, so no broker flow to continue
        return;
    }
    brokerResponse = responseUrl;
    brokerResponseReady.Release();
}

An alternative would be checking the content of the incoming NSUrl to determine if it's a broker url or not.

@leonluc-dev Thanks for all the info, will take a look.

@leonluc-dev i'm not able to repro this. Do you have a basic repro you can share? Thanks.

@leonluc-dev do you have a repro you can share?

Apologies for the delay.

Here is a small project with a repro: TRTestApp.zip (Updated: 11/26)

The app has the url scheme trtestapp registered, so opening any url like trtestapp://anyarbitrarydata in Safari or any other app should open the test app and call the openurl method in the app delegate. This should cause the crash mentioned earlier.

@leonluc-dev thanks! I'll probably take a look tomorrow (please ping me if you hear back in a day or two). appreciate the repro.

I've updated the repro to fix a minor issue in the repro project, to make sure the repro only has the issue described earlier.

@leonluc-dev have you had a chance to try this out? dev branch has the fix. thanks

I have built the dev branch code and tested on iOS 12 and 13 and while the IsBrokerResponse method still returns true on iOS 13, the SetBrokerContinuationEventArgs now seemingly ignores the non-broker URL and gracefully completes without a null pointer exception and code execution continues as usual.

So with that the issue seems resolved.

@leonluc-dev Fixed in 5.2.5 release

	public static void SetBrokerResponse(NSUrl responseUrl)
	{
	brokerResponse = responseUrl;
	brokerResponseReady.Release();
	}