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

Redirect URI incompatibility with HTML5 history API #458

Closed
1 of 5 tasks
rocketraman opened this issue Oct 14, 2018 · 10 comments · Fixed by #462
Closed
1 of 5 tasks

Redirect URI incompatibility with HTML5 history API #458

rocketraman opened this issue Oct 14, 2018 · 10 comments · Fixed by #462
Assignees
Labels
bug A problem that needs to be fixed for the feature to function as intended.

Comments

@rocketraman
Copy link

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[X] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Other... Please describe:

Browser:

  • Chrome version 69.0.3497.100
  • Firefox version XX
  • IE version XX
  • Edge version XX
  • Safari version XX

Library version


Library version: 0.2.3

Current behavior

When the Msal.UserAgentApplication loads, it appears to determine the redirect URI (if not specified in the options) by using window.location.href.

However, if the HTML5 history API is subsequently used to modify the browser location, and then the loginPopup is initiated, the redirect URI is set to the original value at the time of construction of Msal.UserAgentApplication, and not the current window location.

This is very surprising (and wrong) behavior.

Expected behavior

The expected behavior would be for the redirect URI to match the current window location, unless explicitly set at construction time. Also, for maximum flexibility, I would allow redirectURI option to also be a function -- and the default implementation to be to determine the current window.location.href at the time of the login call. Making it a function allows the user to provide their own logic though, and deep integration with their own code.

Minimal reproduction of the problem with instructions

  1. Create an Msal.UserAgentApplication at page '/'
  2. Use the history.push API to change the page to '/foo'
  3. Initiate a loginPopup call
@adamtay
Copy link

adamtay commented Oct 15, 2018

@sayanghosh123 Have also noticed this issue in our testing.

@emilpalsson
Copy link
Contributor

+1. We also experienced this somewhat unexpected behaviour when implementing msal half a year ago.

@nehaagrawal
Copy link
Contributor

@rocketraman I am not sure if this is a bug? Why can't you set the redirecturi yourself during initialization or in app registration portal?

@nehaagrawal nehaagrawal added the more-information-needed Use this label when you are waiting on information from the issue creator label Oct 22, 2018
@rocketraman
Copy link
Author

rocketraman commented Oct 23, 2018

@rocketraman I am not sure if this is a bug? Why can't you set the redirecturi yourself during initialization or in app registration portal?

Because the redirect uri path changes on the client side, after initialization of msal, as the user navigates the application, via the html5 history api. The origin remains the same and matches the registration so that is not an issue.

Did you look at the reproduction steps?

@nehaagrawal
Copy link
Contributor

@rocketraman The redirect_uri is where authentication responses can be sent and received by your app. It must exactly match one of the redirect_uris you registered in the portal. MSAL will set the redirect_uri to window.location.href only when user doesn't pass anything. I want to understand why you are not setting the redirect_uri during initialization itself, if you know already what the redirect uri is going to be?

@emilpalsson
Copy link
Contributor

@nehaagrawal The redirect_uri does not have to exactly match one of the redirect_uris registred in the portal, just the origin. e.g. https://example.com/my/random/path is a valid client redirect_uri when the registered redirect_uri is https://example.com.

Because the client url can be updated in a SPA after the UserAgentApplication is initialized, we cannot possibly know the redirect_uri at the time of initialization.

This is an issue if you have a SPA and want the client to be returned to whatever exact path he is on when clicking sign in (not the location he was on when UserAgentApplication was initialized).

@rocketraman
Copy link
Author

^- This (thanks @emilpalsson )

@nehaagrawal
Copy link
Contributor

@rocketraman Thanks for raising this issue and creating the PR. We are reviewing the PR. I will update the status here.

@rocketraman
Copy link
Author

Thanks @nehaagrawal. One correction: the pull request is from @emilpalsson, not me.

@nehaagrawal nehaagrawal added bug A problem that needs to be fixed for the feature to function as intended. and removed more-information-needed Use this label when you are waiting on information from the issue creator labels Oct 25, 2018
@rocketraman
Copy link
Author

rocketraman commented Apr 12, 2019

As per my comment on pull #462, Azure support is telling me is that comparison of the redirect URI against the exact URIs listed as allowed URIs is the current expected behavior, which is contrary to what I and others believed above. If that is the case, then the code in pull #462 cannot actually solve this problem, and does break some existing setups as well (the code might still be useful for other specialized use cases, but the default value of the redirectUri probably needs to revert back to its old behavior).

I am actually unable to reproduce this issue, if I use the following settings:

{
  redirectUri: window.location.origin,
  navigateToLoginRequestUrl: false
}

With these settings, with either 0.2.3 or 0.2.4, the behavior using loginPopup is correct -- the app login works even after a push state navigation, and the app location post-login remains constant.

The behavior with loginRedirect will of course still be wrong...

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug A problem that needs to be fixed for the feature to function as intended.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants