From 81b954e9db5d34a1f2bbd55642df86503c2a6ddf Mon Sep 17 00:00:00 2001 From: Sam Olsen Date: Tue, 23 Feb 2021 13:57:43 -0800 Subject: [PATCH 1/2] Make origin validation happen asynchronously from the popup --- .../auth-exp/src/core/auth/initialize.test.ts | 1 + .../auth-exp/src/model/popup_redirect.ts | 1 + .../platform_browser/popup_redirect.test.ts | 38 ++++++++++--------- .../src/platform_browser/popup_redirect.ts | 5 +-- .../src/platform_browser/strategies/popup.ts | 10 +++-- .../popup_redirect/popup_redirect.ts | 4 ++ .../helpers/mock_popup_redirect_resolver.ts | 2 + 7 files changed, 37 insertions(+), 24 deletions(-) diff --git a/packages-exp/auth-exp/src/core/auth/initialize.test.ts b/packages-exp/auth-exp/src/core/auth/initialize.test.ts index 1bc859c7c03..412115a6b9e 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.test.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.test.ts @@ -116,6 +116,7 @@ describe('core/auth/initialize', () => { ): void { cb(true); } + async _originValidation(): Promise {} async _completeRedirectFn( _auth: Auth, _resolver: PopupRedirectResolver, diff --git a/packages-exp/auth-exp/src/model/popup_redirect.ts b/packages-exp/auth-exp/src/model/popup_redirect.ts index 6cbf400a17e..8f90723c325 100644 --- a/packages-exp/auth-exp/src/model/popup_redirect.ts +++ b/packages-exp/auth-exp/src/model/popup_redirect.ts @@ -98,6 +98,7 @@ export interface PopupRedirectResolverInternal extends PopupRedirectResolver { cb: (support: boolean) => unknown ): void; _redirectPersistence: Persistence; + _originValidation(auth: Auth): Promise; // This is needed so that auth does not have a hard dependency on redirect _completeRedirectFn: ( diff --git a/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts b/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts index 881ae5b12a5..ac24c46b22d 100644 --- a/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts +++ b/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts @@ -117,13 +117,6 @@ describe('platform_browser/popup_redirect', () => { ); }); - it('validates the origin', async () => { - await resolver._initialize(auth); - - await resolver._openPopup(auth, provider, event); - expect(validateOrigin._validateOrigin).to.have.been.calledWith(auth); - }); - it('throws an error if apiKey is unspecified', async () => { delete (auth.config as Partial).apiKey; await resolver._initialize(auth); @@ -132,17 +125,6 @@ describe('platform_browser/popup_redirect', () => { resolver._openPopup(auth, provider, event) ).to.be.rejectedWith(FirebaseError, 'auth/invalid-api-key'); }); - - it('rejects immediately if origin validation fails', async () => { - await resolver._initialize(auth); - (validateOrigin._validateOrigin as sinon.SinonStub).returns( - Promise.reject(new Error('invalid-origin')) - ); - - await expect( - resolver._openPopup(auth, provider, event) - ).to.be.rejectedWith(Error, 'invalid-origin'); - }); }); context('#_openRedirect', () => { @@ -213,6 +195,26 @@ describe('platform_browser/popup_redirect', () => { }); }); + context('#_originValidation', () => { + it('validates the origin', async () => { + await resolver._initialize(auth); + + await resolver._originValidation(auth); + expect(validateOrigin._validateOrigin).to.have.been.calledWith(auth); + }); + + it('rejects if origin validation fails', async () => { + await resolver._initialize(auth); + (validateOrigin._validateOrigin as sinon.SinonStub).returns( + Promise.reject(new Error('invalid-origin')) + ); + + await expect( + resolver._originValidation(auth) + ).to.be.rejectedWith(Error, 'invalid-origin'); + }); + }); + context('#_initialize', () => { it('returns different manager for a different auth', async () => { const manager = await resolver._initialize(auth); diff --git a/packages-exp/auth-exp/src/platform_browser/popup_redirect.ts b/packages-exp/auth-exp/src/platform_browser/popup_redirect.ts index c24cea432ba..4b936a091a3 100644 --- a/packages-exp/auth-exp/src/platform_browser/popup_redirect.ts +++ b/packages-exp/auth-exp/src/platform_browser/popup_redirect.ts @@ -73,7 +73,6 @@ class BrowserPopupRedirectResolver implements PopupRedirectResolverInternal { '_initialize() not called before _openPopup()' ); - await this.originValidation(auth); const url = _getRedirectUrl( auth, provider, @@ -90,7 +89,7 @@ class BrowserPopupRedirectResolver implements PopupRedirectResolverInternal { authType: AuthEventType, eventId?: string ): Promise { - await this.originValidation(auth); + await this._originValidation(auth); _setWindowLocation( _getRedirectUrl(auth, provider, authType, _getCurrentUrl(), eventId) ); @@ -154,7 +153,7 @@ class BrowserPopupRedirectResolver implements PopupRedirectResolverInternal { ); } - private originValidation(auth: AuthInternal): Promise { + _originValidation(auth: AuthInternal): Promise { const key = auth._key(); if (!this.originValidationPromises[key]) { this.originValidationPromises[key] = _validateOrigin(auth); diff --git a/packages-exp/auth-exp/src/platform_browser/strategies/popup.ts b/packages-exp/auth-exp/src/platform_browser/strategies/popup.ts index a895ef2fa6a..fea61a3dc0c 100644 --- a/packages-exp/auth-exp/src/platform_browser/strategies/popup.ts +++ b/packages-exp/auth-exp/src/platform_browser/strategies/popup.ts @@ -241,13 +241,17 @@ class PopupOperation extends AbstractPopupRedirectOperation { ); this.authWindow.associatedEvent = eventId; - // Check for web storage support _after_ the popup is loaded. Checking for - // web storage is slow (on the order of a second or so). Rather than - // waiting on that before opening the window, optimistically open the popup + // Check for web storage support and origin validation _after_ the popup is + // loaded. These operations are slow (~1 second or so) Rather than + // waiting on them before opening the window, optimistically open the popup // and check for storage support at the same time. If storage support is // not available, this will cause the whole thing to reject properly. It // will also close the popup, but since the promise has already rejected, // the popup closed by user poll will reject into the void. + this.resolver._originValidation(this.auth).catch(e => { + this.reject(e); + }); + this.resolver._isIframeWebStorageSupported(this.auth, isSupported => { if (!isSupported) { this.reject( diff --git a/packages-exp/auth-exp/src/platform_cordova/popup_redirect/popup_redirect.ts b/packages-exp/auth-exp/src/platform_cordova/popup_redirect/popup_redirect.ts index 61bf4e6285c..20faf58d326 100644 --- a/packages-exp/auth-exp/src/platform_cordova/popup_redirect/popup_redirect.ts +++ b/packages-exp/auth-exp/src/platform_cordova/popup_redirect/popup_redirect.ts @@ -101,6 +101,10 @@ class CordovaPopupRedirectResolver implements PopupRedirectResolverInternal { throw new Error('Method not implemented.'); } + _originValidation(): Promise { + return Promise.resolve(); + } + private attachCallbackListeners( auth: AuthInternal, manager: AuthEventManager diff --git a/packages-exp/auth-exp/test/helpers/mock_popup_redirect_resolver.ts b/packages-exp/auth-exp/test/helpers/mock_popup_redirect_resolver.ts index e989f097c63..90098f016e5 100644 --- a/packages-exp/auth-exp/test/helpers/mock_popup_redirect_resolver.ts +++ b/packages-exp/auth-exp/test/helpers/mock_popup_redirect_resolver.ts @@ -57,5 +57,7 @@ export function makeMockPopupRedirectResolver( _redirectPersistence?: Persistence; async _completeRedirectFn(): Promise {} + + async _originValidation(): Promise {} }; } From 9856862a423661faf46ce6a3dc5a09f7e330f65b Mon Sep 17 00:00:00 2001 From: Sam Olsen Date: Tue, 23 Feb 2021 13:58:10 -0800 Subject: [PATCH 2/2] Formatting --- .../auth-exp/src/platform_browser/popup_redirect.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts b/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts index ac24c46b22d..07724657e1e 100644 --- a/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts +++ b/packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts @@ -209,9 +209,10 @@ describe('platform_browser/popup_redirect', () => { Promise.reject(new Error('invalid-origin')) ); - await expect( - resolver._originValidation(auth) - ).to.be.rejectedWith(Error, 'invalid-origin'); + await expect(resolver._originValidation(auth)).to.be.rejectedWith( + Error, + 'invalid-origin' + ); }); });