ionic-appauth
ionic-appauth copied to clipboard
wi3land
Problems with browser.close() in auth-browser.ts:23
I was recently troubleshooting a problem with our user registration process where our browser tab would close without warning after the user logged in for the first time. I discovered that it's because this library was explicitly invoking window.close() in auth-service.ts in both the internalAuthorizationCallback (line 112) and internalEndSessionCallback (line 118).
When looking at this further, this makes complete sense for both the CapacitorBrowser and CordovaBrowser implementations. However, I don't know why this is being called for the DefaultBrowser. I realized that "window.close()" is actually being called 100% of the time after login/logout within a web browser. The reason that most of the time it doesn't actually just close the browser is because this call only works if the current tab is "closeable". In Chrome and Edge, a tab is closeable if it was opened from a link from another website using target="_blank" AND it does not have rel="noopener" set.
I was able to easily reproduce this by opening my site using a link like this, logging in using Chrome <87.0.4280.88 (Official Build) (64-bit)> or Edge <87.0.664.55 (Official build) (64-bit)>. In each case, the new tab immediately closed after logging in. I proved this worked upon logout as well. If I added 'rel="noopener"' to my link, it stops this behavior and I am able to log in and out without the tab closing.
I am wondering why the call to "window.close()" should ever be legitimately used on auth-browser.ts:23. I think this call should be completely removed, and "closeWindow" should be a no-op. I would understand it if window.open on line 14 opened a new window ("_blank"), but it doesn't, it's "_self".
@mraible Thanks for merging in that PR, what do we need to do to get a new version published with this fix?
I don't think I have permission to publish to npm. The maintainer of this project only recently added me as a committer.
On Sep 1, 2021, at 07:34, artman186 @.***> wrote:
@mraible Thanks for merging in that PR, what do we need to do to get a new version published with this fix?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
@wi3land Are you able to push a new version to npm? The new version will be a huge help to the team I am working on.
