From 3628f47ee1e8566f037118028c6c344ffb6f7ace Mon Sep 17 00:00:00 2001 From: Sam Olsen Date: Mon, 18 Oct 2021 17:46:39 -0700 Subject: [PATCH 1/3] Make auth resilient against localStorage and sessionStorage permissions errors --- packages/auth/demo/src/index.js | 79 ++++++++++--------- .../auth/src/core/strategies/redirect.test.ts | 26 +++++- packages/auth/src/core/strategies/redirect.ts | 8 +- .../persistence/browser.test.ts | 40 ++++++++++ .../platform_browser/persistence/browser.ts | 8 +- .../persistence/local_storage.ts | 2 +- .../persistence/session_storage.ts | 2 +- .../erroring_unavailable_persistence.ts | 55 +++++++++++++ 8 files changed, 174 insertions(+), 46 deletions(-) create mode 100644 packages/auth/src/platform_browser/persistence/browser.test.ts create mode 100644 packages/auth/test/helpers/erroring_unavailable_persistence.ts diff --git a/packages/auth/demo/src/index.js b/packages/auth/demo/src/index.js index 44ec1b9206a..d26be2a67f0 100644 --- a/packages/auth/demo/src/index.js +++ b/packages/auth/demo/src/index.js @@ -1415,6 +1415,7 @@ function getParameterByName(name) { * the input field for the confirm email verification process. */ function populateActionCodes() { + return; let emailForSignIn = null; let signInTime = 0; if ('localStorage' in window && window['localStorage'] !== null) { @@ -1731,45 +1732,45 @@ function initApp() { } // Install servicerWorker if supported. - if ('serviceWorker' in navigator) { - navigator.serviceWorker - .register('/service-worker.js') - .then(reg => { - // Registration worked. - console.log('Registration succeeded. Scope is ' + reg.scope); - }) - .catch(error => { - // Registration failed. - console.log('Registration failed with ' + error.message); - }); - } - - if (window.Worker) { - webWorker = new Worker('/web-worker.js'); - /** - * Handles the incoming message from the web worker. - * @param {!Object} e The message event received. - */ - webWorker.onmessage = function (e) { - console.log('User data passed through web worker: ', e.data); - switch (e.data.type) { - case 'GET_USER_INFO': - alertSuccess( - 'User data passed through web worker: ' + JSON.stringify(e.data) - ); - break; - case 'RUN_TESTS': - if (e.data.status === 'success') { - alertSuccess('Web worker tests ran successfully!'); - } else { - alertError('Error: ' + JSON.stringify(e.data.error)); - } - break; - default: - return; - } - }; - } + // if ('serviceWorker' in navigator) { + // navigator.serviceWorker + // .register('/service-worker.js') + // .then(reg => { + // // Registration worked. + // console.log('Registration succeeded. Scope is ' + reg.scope); + // }) + // .catch(error => { + // // Registration failed. + // console.log('Registration failed with ' + error.message); + // }); + // } + + // if (window.Worker) { + // webWorker = new Worker('/web-worker.js'); + // /** + // * Handles the incoming message from the web worker. + // * @param {!Object} e The message event received. + // */ + // webWorker.onmessage = function (e) { + // console.log('User data passed through web worker: ', e.data); + // switch (e.data.type) { + // case 'GET_USER_INFO': + // alertSuccess( + // 'User data passed through web worker: ' + JSON.stringify(e.data) + // ); + // break; + // case 'RUN_TESTS': + // if (e.data.status === 'success') { + // alertSuccess('Web worker tests ran successfully!'); + // } else { + // alertError('Error: ' + JSON.stringify(e.data.error)); + // } + // break; + // default: + // return; + // } + // }; + // } /** * Asks the web worker, if supported in current browser, to return the user info diff --git a/packages/auth/src/core/strategies/redirect.test.ts b/packages/auth/src/core/strategies/redirect.test.ts index 2da34a84e1f..a579cfb5f19 100644 --- a/packages/auth/src/core/strategies/redirect.test.ts +++ b/packages/auth/src/core/strategies/redirect.test.ts @@ -17,6 +17,7 @@ import { AuthError, + Persistence, PopupRedirectResolver } from '../../model/public_types'; import { OperationType, ProviderId } from '../../model/enums'; @@ -32,7 +33,7 @@ import { import { makeMockPopupRedirectResolver } from '../../../test/helpers/mock_popup_redirect_resolver'; import { AuthInternal } from '../../model/auth'; import { AuthEventManager } from '../auth/auth_event_manager'; -import { RedirectAction, _clearRedirectOutcomes } from './redirect'; +import { RedirectAction, _clearRedirectOutcomes, _getAndClearPendingRedirectStatus } from './redirect'; import { AuthEvent, AuthEventType, @@ -44,6 +45,7 @@ import * as idpTasks from '../strategies/idp'; import { expect, use } from 'chai'; import { AuthErrorCode } from '../errors'; import { RedirectPersistence } from '../../../test/helpers/redirect_persistence'; +import { ErroringUnavailablePersistence } from '../../../test/helpers/erroring_unavailable_persistence'; use(sinonChai); @@ -210,4 +212,26 @@ describe('core/strategies/redirect', () => { expect(await redirectAction.execute()).to.eq(null); expect(resolverInstance._initialize).not.to.have.been.called; }); + + context('_getAndClearPendingRedirectStatus', () => { + // Do not run these tests in node + if (typeof window === 'undefined') { + return; + } + + it('returns false if the key is not set', async () => { + redirectPersistence.hasPendingRedirect = false; + expect(await _getAndClearPendingRedirectStatus(_getInstance(resolver), auth)).to.be.false; + }); + + it('returns true if the key is found', async () => { + redirectPersistence.hasPendingRedirect = true; + expect(await _getAndClearPendingRedirectStatus(_getInstance(resolver), auth)).to.be.true; + }); + + it('returns false if sessionStorage is permission denied', async () => { + _getInstance(resolver)._redirectPersistence = ErroringUnavailablePersistence as unknown as Persistence; + expect(await _getAndClearPendingRedirectStatus(_getInstance(resolver), auth)).to.be.false; + }); + }); }); diff --git a/packages/auth/src/core/strategies/redirect.ts b/packages/auth/src/core/strategies/redirect.ts index 032b6d71181..df6b692655d 100644 --- a/packages/auth/src/core/strategies/redirect.ts +++ b/packages/auth/src/core/strategies/redirect.ts @@ -118,9 +118,13 @@ export async function _getAndClearPendingRedirectStatus( auth: AuthInternal ): Promise { const key = pendingRedirectKey(auth); + const persistence = resolverPersistence(resolver); + if (!(await persistence._isAvailable())) { + return false; + } const hasPendingRedirect = - (await resolverPersistence(resolver)._get(key)) === 'true'; - await resolverPersistence(resolver)._remove(key); + (await persistence._get(key)) === 'true'; + await persistence._remove(key); return hasPendingRedirect; } diff --git a/packages/auth/src/platform_browser/persistence/browser.test.ts b/packages/auth/src/platform_browser/persistence/browser.test.ts new file mode 100644 index 00000000000..2b69f1e75ee --- /dev/null +++ b/packages/auth/src/platform_browser/persistence/browser.test.ts @@ -0,0 +1,40 @@ +/** + * @license + * Copyright 2021 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +import { expect } from 'chai'; +import { + PersistenceType +} from '../../core/persistence'; +import { BrowserPersistenceClass } from './browser'; + +// Most tests for this class exist in the tests for the subclasses. + +class TestPersistence extends BrowserPersistenceClass { + constructor(storageRetriever: () => Storage) { + super(storageRetriever, PersistenceType.NONE); + } +} + +describe('platform_browser/persistence/browser', () => { + it('_isAvailable works if reading the storage accessor throws', async () => { + const browserPersistence = new TestPersistence(() => { + throw new DOMException('no'); + }); + expect(await browserPersistence._isAvailable()).to.be.false; + }); +}); \ No newline at end of file diff --git a/packages/auth/src/platform_browser/persistence/browser.ts b/packages/auth/src/platform_browser/persistence/browser.ts index b27af2a1ef8..49cd6a68409 100644 --- a/packages/auth/src/platform_browser/persistence/browser.ts +++ b/packages/auth/src/platform_browser/persistence/browser.ts @@ -27,11 +27,11 @@ import { export abstract class BrowserPersistenceClass { protected constructor( - protected readonly storage: Storage, + protected readonly storageRetriever: () => Storage, readonly type: PersistenceType ) {} - _isAvailable(this: BrowserPersistenceClass): Promise { + _isAvailable(): Promise { try { if (!this.storage) { return Promise.resolve(false); @@ -58,4 +58,8 @@ export abstract class BrowserPersistenceClass { this.storage.removeItem(key); return Promise.resolve(); } + + protected get storage(): Storage { + return this.storageRetriever(); + } } diff --git a/packages/auth/src/platform_browser/persistence/local_storage.ts b/packages/auth/src/platform_browser/persistence/local_storage.ts index 83a8808d054..be78bd10a19 100644 --- a/packages/auth/src/platform_browser/persistence/local_storage.ts +++ b/packages/auth/src/platform_browser/persistence/local_storage.ts @@ -51,7 +51,7 @@ class BrowserLocalPersistence static type: 'LOCAL' = 'LOCAL'; constructor() { - super(window.localStorage, PersistenceType.LOCAL); + super(() => window.localStorage, PersistenceType.LOCAL); } private readonly boundEventHandler = (event: StorageEvent, poll?: boolean): void => this.onStorageEvent(event, poll); diff --git a/packages/auth/src/platform_browser/persistence/session_storage.ts b/packages/auth/src/platform_browser/persistence/session_storage.ts index 5c8797cbe8d..d677330424c 100644 --- a/packages/auth/src/platform_browser/persistence/session_storage.ts +++ b/packages/auth/src/platform_browser/persistence/session_storage.ts @@ -31,7 +31,7 @@ class BrowserSessionPersistence static type: 'SESSION' = 'SESSION'; constructor() { - super(window.sessionStorage, PersistenceType.SESSION); + super(() => window.sessionStorage, PersistenceType.SESSION); } _addListener(_key: string, _listener: StorageEventListener): void { diff --git a/packages/auth/test/helpers/erroring_unavailable_persistence.ts b/packages/auth/test/helpers/erroring_unavailable_persistence.ts new file mode 100644 index 00000000000..1fdf8372b59 --- /dev/null +++ b/packages/auth/test/helpers/erroring_unavailable_persistence.ts @@ -0,0 +1,55 @@ +/** + * @license + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + import { PersistenceInternal, PersistenceType, PersistenceValue } from '../../src/core/persistence'; + + const PERMISSION_ERROR = typeof window !== 'undefined' ? new DOMException( + 'Failed to read this storage class from the Window object; access is denied') + : new Error('This is Node.'); + + /** + * Helper class for mocking completely broken persistence that errors when + * accessed. + * + * When disabling cookies in Chrome entirely, for example, simply reading the + * "localStorage" field in "window" will throw an error, but this can't be + * checked for by calling `'localStorage' in window`. This class simulates a + * situation where _isAvailable works correctly but all other methods fail. + */ + export class ErroringUnavailablePersistence implements PersistenceInternal { + type = PersistenceType.NONE; + async _isAvailable(): Promise { + return false; + } + async _set(): Promise { + throw PERMISSION_ERROR; + } + async _get(): Promise { + throw PERMISSION_ERROR; + } + async _remove(): Promise { + throw PERMISSION_ERROR; + } + _addListener(): void { + throw PERMISSION_ERROR; + } + _removeListener(): void { + throw PERMISSION_ERROR; + } + _shouldAllowMigration = false; + } + \ No newline at end of file From 25afb3486167aa6b04dfd6ab7e63d9ce68ec01c5 Mon Sep 17 00:00:00 2001 From: Sam Olsen Date: Mon, 18 Oct 2021 17:51:07 -0700 Subject: [PATCH 2/3] Restore demo code --- packages/auth/demo/src/index.js | 79 +++++++++---------- .../erroring_unavailable_persistence.ts | 2 +- 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/packages/auth/demo/src/index.js b/packages/auth/demo/src/index.js index d26be2a67f0..44ec1b9206a 100644 --- a/packages/auth/demo/src/index.js +++ b/packages/auth/demo/src/index.js @@ -1415,7 +1415,6 @@ function getParameterByName(name) { * the input field for the confirm email verification process. */ function populateActionCodes() { - return; let emailForSignIn = null; let signInTime = 0; if ('localStorage' in window && window['localStorage'] !== null) { @@ -1732,45 +1731,45 @@ function initApp() { } // Install servicerWorker if supported. - // if ('serviceWorker' in navigator) { - // navigator.serviceWorker - // .register('/service-worker.js') - // .then(reg => { - // // Registration worked. - // console.log('Registration succeeded. Scope is ' + reg.scope); - // }) - // .catch(error => { - // // Registration failed. - // console.log('Registration failed with ' + error.message); - // }); - // } - - // if (window.Worker) { - // webWorker = new Worker('/web-worker.js'); - // /** - // * Handles the incoming message from the web worker. - // * @param {!Object} e The message event received. - // */ - // webWorker.onmessage = function (e) { - // console.log('User data passed through web worker: ', e.data); - // switch (e.data.type) { - // case 'GET_USER_INFO': - // alertSuccess( - // 'User data passed through web worker: ' + JSON.stringify(e.data) - // ); - // break; - // case 'RUN_TESTS': - // if (e.data.status === 'success') { - // alertSuccess('Web worker tests ran successfully!'); - // } else { - // alertError('Error: ' + JSON.stringify(e.data.error)); - // } - // break; - // default: - // return; - // } - // }; - // } + if ('serviceWorker' in navigator) { + navigator.serviceWorker + .register('/service-worker.js') + .then(reg => { + // Registration worked. + console.log('Registration succeeded. Scope is ' + reg.scope); + }) + .catch(error => { + // Registration failed. + console.log('Registration failed with ' + error.message); + }); + } + + if (window.Worker) { + webWorker = new Worker('/web-worker.js'); + /** + * Handles the incoming message from the web worker. + * @param {!Object} e The message event received. + */ + webWorker.onmessage = function (e) { + console.log('User data passed through web worker: ', e.data); + switch (e.data.type) { + case 'GET_USER_INFO': + alertSuccess( + 'User data passed through web worker: ' + JSON.stringify(e.data) + ); + break; + case 'RUN_TESTS': + if (e.data.status === 'success') { + alertSuccess('Web worker tests ran successfully!'); + } else { + alertError('Error: ' + JSON.stringify(e.data.error)); + } + break; + default: + return; + } + }; + } /** * Asks the web worker, if supported in current browser, to return the user info diff --git a/packages/auth/test/helpers/erroring_unavailable_persistence.ts b/packages/auth/test/helpers/erroring_unavailable_persistence.ts index 1fdf8372b59..6bb325c8b87 100644 --- a/packages/auth/test/helpers/erroring_unavailable_persistence.ts +++ b/packages/auth/test/helpers/erroring_unavailable_persistence.ts @@ -18,7 +18,7 @@ import { PersistenceInternal, PersistenceType, PersistenceValue } from '../../src/core/persistence'; const PERMISSION_ERROR = typeof window !== 'undefined' ? new DOMException( - 'Failed to read this storage class from the Window object; access is denied') + 'Failed to read this storage class from the Window; access is denied') : new Error('This is Node.'); /** From 8ea95e1735dfc3466c949086fc4bf9898f8f38c9 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 18 Oct 2021 17:52:44 -0700 Subject: [PATCH 3/3] Add changeset --- .changeset/quiet-wolves-sing.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/quiet-wolves-sing.md diff --git a/.changeset/quiet-wolves-sing.md b/.changeset/quiet-wolves-sing.md new file mode 100644 index 00000000000..6cc2bf39c7f --- /dev/null +++ b/.changeset/quiet-wolves-sing.md @@ -0,0 +1,5 @@ +--- +"@firebase/auth": patch +--- + +Make the library resilient against localStorage and sessionStorage permissions errors