AppAuth-iOS icon indicating copy to clipboard operation
AppAuth-iOS copied to clipboard

Relax redirect URL matching for the forward slash path case

Open AlexPlekhov opened this issue 3 years ago • 16 comments

Based on issue #446 sometimes a trailing slash added to path for redirect url. Here a fix to make comparison regardless that trailing slash.

This issue affects react-native-app-auth lib: https://github.com/FormidableLabs/react-native-app-auth/issues/123 https://github.com/FormidableLabs/react-native-app-auth/issues/336

and a few others.

AlexPlekhov avatar Jun 10 '22 10:06 AlexPlekhov

Thanks for the contribution @AlexPlekhov, could you sign the contributor agreements so we can get this merged?

petea avatar Jun 10 '22 15:06 petea

Hi @petea, I've already done it immediately after creating PR. I hope you got email notifications. If didn't I'm ready to send those PDFs via email.

AlexPlekhov avatar Jun 14 '22 06:06 AlexPlekhov

Thanks for the contribution @AlexPlekhov, could you sign the contributor agreements so we can get this merged?

Hi @petea, could you confirm that everything OK? I followed the steps in the contributor agreements. Please, write me back if I missed anything and still have to do something.

AlexPlekhov avatar Jun 16 '22 07:06 AlexPlekhov

Would be great to have this PR merged as it fixes an issue with Azure AD we have been experiencing. The Azure Portal gives us a redirect URI which looks like msauth.com.example.app://auth to use with our iOS app. But when the redirect back to our app happens they have appended a slash which breaks AppAuth's redirect URI validation. With the changes in this PR applied AppAuth happily accepts the redirect URI and proceeds.

ntherning avatar Jul 01 '22 09:07 ntherning

Hi all, @petea,

I've resigned all docs as required by Mike Leszcz, was added to a WG, got a subscription email. But it's still in review. Have I missed anything again?

AlexPlekhov avatar Jul 11 '22 12:07 AlexPlekhov

I think this should be merged, as this is mission critical for functioning apps that rely on third party services and thus cannot influence how the backend behaves.

developerfromjokela avatar Aug 17 '22 07:08 developerfromjokela

@developerfromjokela note that there are workarounds for this situation discussed in #446.

petea avatar Aug 18 '22 00:08 petea

@developerfromjokela note that there are workarounds for this situation discussed in #446.

Yep, we already did the same. But wants to help the community to not face with it again.

AlexPlekhov avatar Aug 18 '22 13:08 AlexPlekhov

Hi @petea, will this get merged? We encountered this issue just today. We have support for multiple IdPs that support OIDC and customer wanted to use ADFS but it is not working cause ADFS appends / to the redirect uri 🤷🏽‍♂️

davidkaya avatar Aug 31 '22 11:08 davidkaya

@davidkaya note that there is a straightforward workaround for this issue that is detailed in #446.

petea avatar Sep 08 '22 00:09 petea

Can we get this PR moving again? I think the only remaining question is a missing comment? Would help in our case (using the Flutter AppAuth library where this is an issue as well (and mentioned workaround does not apply in our case)

DelcoigneYves avatar Oct 19 '22 05:10 DelcoigneYves

@DelcoigneYves I did everything I could. Comments, formats - everything has been in place already.

AlexPlekhov avatar Oct 19 '22 06:10 AlexPlekhov

Then poking @petea for a merge 💯

DelcoigneYves avatar Oct 19 '22 13:10 DelcoigneYves

@DelcoigneYves @petea @davidkaya Hi folks! I give up. I see only resistance against this PR and ignorance. May be you will be more successful to resolve this issue on your way. I will close it soon.

AlexPlekhov avatar Nov 16 '22 11:11 AlexPlekhov

@petea LGTM! Can we please now merge it? 🙂 https://github.com/openid/AppAuth-iOS/blob/master/CONTRIBUTING.md#pull-request-reviews

DelcoigneYves avatar Nov 17 '22 12:11 DelcoigneYves

Hi @petea, is this going to get merged?

pawellaskowski avatar Mar 14 '23 10:03 pawellaskowski