Repository navigation
fix(core): open the popup-OAuth window synchronously in the click - #688
Open
AminDhouib wants to merge 1 commit into
Open
AminDhouib wants to merge 1 commit into
AminDhouib wants to merge 1 commit into
Conversation
…en point it at the provider, so Edge stops blocking it
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Member
Author
|
Review of 4ca57d8: APPROVE No blocking defect. CI is green on all 18 checks at this head.
Non-blocking nits, no action required:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the "Popup was blocked by the browser" error on the OneDrive / Dropbox / Box sign-in popup. Found through uNotes task T-12111.
Evidence
In uNotes' PostHog,
Popup was blocked by the browserappeared as about 21$exceptionevents from about 20 users in 14 days, mostly Edge on Windows. It is raised byPopupOAuthPlugin.authenticateViaPopup()and only the popup providers use it. Google Drive goes through Google Identity Services and is not affected.Cause
authenticateViaPopup()didconst url = await this.getAuthUrl()beforewindow.open(...).getAuthUrl()awaits the PKCE digest (crypto.subtle.digest), so the open no longer ran inside the user gesture. Strict blockers treat that as an unrequested popup and returnnull.Fix
authenticateViaPopup()now opensabout:blankunder the provider'spopupNamebefore any await, then builds the auth URL and assigns it topopup.location.href.clientIdis refused before any window opens, so it never flashes a blank popup.AUTH_POPUP_BLOCKEDpath (window.openreturnednull) is unchanged, andgetAuthUrl()is no longer called when the window was refused.The React side needed no change.
DriveAuthFallbackalready starts sign-in from the tile click (on mount) and has a "Sign in" button whose click handler callssignIndirectly; with the core change thewindow.openinside both runs in the same task as the gesture. The tests pin both call paths.Tests
popup-oauth-plugin.test.ts:window.openis called synchronously withabout:blankwhile the PKCE digest is still pending; the blank popup is closed and the error rethrown when URL building fails; no PKCE work when the window is refused; no window for an unconfigured provider; a popup closed by sign-out mid-build is not navigated. Four of these fail on the old code. The sharedstubPopupWindowhelper now recordslocation.hrefassignments instead of letting them overwrite the simulated post-redirect href.drive-browser-controller.test.ts:signIn()andretryAuth()startauthenticateViaPopup()synchronously, with no await in between.drive-auth-decline-reporting.test.tsx: the Sign in button callsonRetryfrom inside the click event, not from an effect.Not run locally:
pnpm run e2e(needs the MinIO setup) and the full turbo suite.@useupup/core(1893 tests) and@useupup/reactunit suites, prettier-check,test:quality,test:scripts, eslint for core and react, and both typechecks were run.Includes a patch changeset for
@useupup/core; the packages are in afixedgroup, so the release bumps all nine together.