Skip to content

[Auth] Make auth resilient against localStorage and sessionStorage permissions errors #5635

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

Merged
merged 3 commits into from
Oct 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/quiet-wolves-sing.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@firebase/auth": patch
---

Make the library resilient against localStorage and sessionStorage permissions errors
26 changes: 25 additions & 1 deletion packages/auth/src/core/strategies/redirect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import {
AuthError,
Persistence,
PopupRedirectResolver
} from '../../model/public_types';
import { OperationType, ProviderId } from '../../model/enums';
Expand All @@ -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,
Expand All @@ -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);

Expand Down Expand Up @@ -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<PopupRedirectResolverInternal>(resolver)._redirectPersistence = ErroringUnavailablePersistence as unknown as Persistence;
expect(await _getAndClearPendingRedirectStatus(_getInstance(resolver), auth)).to.be.false;
});
});
});
8 changes: 6 additions & 2 deletions packages/auth/src/core/strategies/redirect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,13 @@ export async function _getAndClearPendingRedirectStatus(
auth: AuthInternal
): Promise<boolean> {
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;
}

Expand Down
40 changes: 40 additions & 0 deletions packages/auth/src/platform_browser/persistence/browser.test.ts
Original file line number Diff line number Diff line change
@@ -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;
});
});
8 changes: 6 additions & 2 deletions packages/auth/src/platform_browser/persistence/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean> {
_isAvailable(): Promise<boolean> {
try {
if (!this.storage) {
return Promise.resolve(false);
Expand All @@ -58,4 +58,8 @@ export abstract class BrowserPersistenceClass {
this.storage.removeItem(key);
return Promise.resolve();
}

protected get storage(): Storage {
return this.storageRetriever();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
55 changes: 55 additions & 0 deletions packages/auth/test/helpers/erroring_unavailable_persistence.ts
Original file line number Diff line number Diff line change
@@ -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; 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<boolean> {
return false;
}
async _set(): Promise<void> {
throw PERMISSION_ERROR;
}
async _get<T extends PersistenceValue>(): Promise<T | null> {
throw PERMISSION_ERROR;
}
async _remove(): Promise<void> {
throw PERMISSION_ERROR;
}
_addListener(): void {
throw PERMISSION_ERROR;
}
_removeListener(): void {
throw PERMISSION_ERROR;
}
_shouldAllowMigration = false;
}