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

NPE if browser blocks popups could be handed more gracefully #886

Closed
alexrussell opened this issue Mar 16, 2022 · 1 comment · Fixed by #888
Closed

NPE if browser blocks popups could be handed more gracefully #886

alexrussell opened this issue Mar 16, 2022 · 1 comment · Fixed by #888
Labels
bug report This issue reports a suspect bug or issue with the SDK itself

Comments

@alexrussell
Copy link

alexrussell commented Mar 16, 2022

Describe the problem

If no popup is provided to the loginWithPopup function in its config parameter, it'll try to create its own using openPopup.

That function simply returns whatever window.open returns.

And then loginWithPopup continues under the assumption there's now a WindowProxy object in config.popup. In fact, it later tries to access config.popup.location.

If the browser doesn't support opening windows (e.g. if popups are blocked) window.open returns null, which then gets passed back from openWindow to loginWithPopup, and the config.popup.location line then throws a NPE.

Obviously not being able to open a window is definitely an exceptional situation that means that the execution path is unable to continue. However, it feels like it should be handled more gracefully than bubbling up a NPE.

What was the expected behavior?

The inability to open a window is a known "unhappy path" of trying to open a window for login and is detected.

Either a custom error is thrown that can be caught better than a NPE or the library actually handles a graceful message display to the end user (unlikely to be something the library will do as, to my knnowledge, it contains no UI elements).

Reproduction

Run an application that integrates the SPA SDK, ensure popups are blocked (e.g. run in most modern web browsers in a new profile with preferences set to defaults). Navigate to the application, and when it would use a popup (for me it's not always the login itself but I can be the 2FA flow kicking in when I attempt to get a new token ("silently") with new scopes - it's not actually clear).

Can the behavior be reproduced using the SPA SDK Playground?
I am unfortunately unable to get SPA SDK Playground working.

Screenshot of Sentry issue, if it's any use image

Environment

  • Version of auth0-spa-js used: 1.20.0 (though, as evidenced by the links above, the issue is present in the current trunk)
  • Which browsers have you tested in? Firefox is the browser I use and I have seen this. Sentry suggests it also happens in Edge and Chrome.
  • Which framework are you using, if applicable (Angular, React, etc): Vue 3.2.26
  • Other modules/plugins/libraries that might be involved: None that I can think of that'd affect this. As I say the browser has to be blocking popups, and the applications has to be trying to get a token/authenticate using a popup.
@alexrussell alexrussell added the bug report This issue reports a suspect bug or issue with the SDK itself label Mar 16, 2022
@stevehobbsdev
Copy link
Contributor

Thanks for the report @alexrussell - I agree that if we're unable to get a popup through openPopup we could halt execution with a better error, probably just after here:

if (!config.popup) {
config.popup = openPopup('');
}

Either a custom error is thrown that can be caught better than a NPE or the library actually handles a graceful message display to the end user

You're right in that the library itself assumes no facility to display messages in the target application, so it'll be the former suggestion here.

I'll raise a PR 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report This issue reports a suspect bug or issue with the SDK itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants