Skip to content
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.

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

Relax redirect URL matching for the forward slash path case #710

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AlexPlekhov
Copy link

@AlexPlekhov AlexPlekhov commented Jun 10, 2022

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:
FormidableLabs/react-native-app-auth#123
FormidableLabs/react-native-app-auth#336

and a few others.

@petea
Copy link
Collaborator

petea commented Jun 10, 2022

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

@AlexPlekhov
Copy link
Author

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
Copy link
Author

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.

@ntherning
Copy link

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.

@AlexPlekhov
Copy link
Author

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?

@developerfromjokela
Copy link

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.

@petea petea changed the title Update OIDAuthorizationService.m Improve redirect URL matching for the forward slash path case Aug 18, 2022
@petea petea linked an issue Aug 18, 2022 that may be closed by this pull request
Source/AppAuthCore/OIDAuthorizationService.m Outdated Show resolved Hide resolved
Source/AppAuthCore/OIDAuthorizationService.m Show resolved Hide resolved
Source/AppAuthCore/OIDAuthorizationService.m Outdated Show resolved Hide resolved
@petea
Copy link
Collaborator

petea commented Aug 18, 2022

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

@petea petea changed the title Improve redirect URL matching for the forward slash path case Relax redirect URL matching for the forward slash path case Aug 18, 2022
@AlexPlekhov
Copy link
Author

@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 AlexPlekhov requested a review from petea August 18, 2022 13:49
@davidkaya
Copy link

davidkaya commented Aug 31, 2022

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 🤷🏽‍♂️

@petea
Copy link
Collaborator

petea commented Sep 8, 2022

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

@petea petea added this to the 1.6.0 milestone Sep 8, 2022
Source/AppAuthCore/OIDAuthorizationService.m Outdated Show resolved Hide resolved
Source/AppAuthCore/OIDAuthorizationService.m Outdated Show resolved Hide resolved
Source/AppAuthCore/OIDAuthorizationService.m Show resolved Hide resolved
@petea petea modified the milestones: 1.6.0, 1.6.1 Sep 9, 2022
Add a comment for changes.
@DelcoigneYves
Copy link

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)

@AlexPlekhov
Copy link
Author

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

@DelcoigneYves
Copy link

Then poking @petea for a merge 💯

@AlexPlekhov
Copy link
Author

@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.

@DelcoigneYves
Copy link

@pawellaskowski
Copy link

Hi @petea, is this going to get merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trailing slash in URL breaks the auth flow
7 participants