-
Notifications
You must be signed in to change notification settings - Fork 146
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
fix: Handle external URLs correctly in ProviderFactory (Fixes external redirect URL handling) #452
fix: Handle external URLs correctly in ProviderFactory (Fixes external redirect URL handling) #452
Conversation
9cce4d1
to
5e87089
Compare
5e87089
to
2ea163f
Compare
@bocharsky-bw please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me, but why would we need to use external URLs for the $redirectUri
argument? Why we can't always use relative URLs?
The reason for supporting external URLs in the Here are some common cases where external URLs would be necessary:
|
@bocharsky-bw , appreciate your response. Can we merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it sounds like valid reasons, thanks for providing more context behind this change!
P.S. Sorry for the long reply
Problem Description:
In the current implementation of the createProvider method in the ProviderFactory class, any redirectUri is processed via Symfony's UrlGeneratorInterface::generate. This causes an issue when an absolute URL (external link), such as https://external-site.com/callback, is passed, since generate is only meant to work with Symfony routes. As a result, an error is thrown when trying to handle external URLs. It could be inconvenient if you work with react apps.
Solution:
This feature adds a check to the createProvider method to differentiate between internal routes and external URLs. If the redirectUri is an absolute URL (determined using filter_var with FILTER_VALIDATE_URL), the URL is used directly without invoking the generate method. This prevents calling the route generator for external URLs and fixes the issue.
Changes:
Added a check in ProviderFactory to identify absolute URLs before attempting to generate a Symfony route.
If the redirectUri is an external URL, it is used directly without passing through the UrlGeneratorInterface.
Updated tests to cover cases where external URLs are passed.
Tests:
Added a new test testShouldCreateProviderWithExternalRedirectUrl, which verifies that the generate method is not called for external URLs.
Ran all existing tests to ensure no functionality was broken by this change.