-
Notifications
You must be signed in to change notification settings - Fork 255
Fix Safari SecurityError when Block All Cookies is enabled #2539
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
e1a71fb
d1ffffc
c13a592
16f3b92
a488d75
fd68d94
831c9b4
d472d65
d2cd04d
93c0f44
d4677e0
cdf088a
22169a2
cee30cd
9e5c985
d0ee829
2274771
59362d4
b9789af
e775471
d25a325
7e32aa3
ec81543
23f5e50
98c0e2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { | |
| IDiagnosticLogger, _eInternalMessageId, _throwInternal, dumpObj, eLoggingSeverity, getExceptionName, getGlobal, getGlobalInst, | ||
| isNullOrUndefined, objForEachKey | ||
| } from "@microsoft/applicationinsights-core-js"; | ||
| import { objGetOwnPropertyDescriptor } from "@nevware21/ts-utils"; | ||
| import { StorageType } from "./Enums"; | ||
|
|
||
| let _canUseLocalStorage: boolean = undefined; | ||
|
|
@@ -23,31 +24,88 @@ function _getLocalStorageObject(): Storage { | |
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Safely checks if storage object (localStorage or sessionStorage) is available and accessible | ||
| * This helps prevent SecurityError in some browsers (e.g., Safari) when cookies are blocked | ||
| * @param storageType - Type of storage | ||
| * @returns {boolean} Returns whether storage object is safely accessible | ||
| */ | ||
| function _canSafelyAccessStorage(storageType: StorageType): boolean { | ||
| const storageTypeName = storageType === StorageType.LocalStorage ? "localStorage" : "sessionStorage"; | ||
| let result = true; | ||
|
|
||
| try { | ||
| // First, check if window exists and get the global object once | ||
| const gbl: any = getGlobal(); | ||
|
MSNev marked this conversation as resolved.
Outdated
|
||
| if (isNullOrUndefined(gbl)) { | ||
| result = false; | ||
| } else { | ||
| // Try to indirectly check if the property exists and is accessible | ||
| // This avoids direct property access that might throw in Safari with "Block All Cookies" enabled | ||
|
|
||
| // Some browsers throw when accessing the property descriptors with getOwnPropertyDescriptor | ||
| // Others throw when directly accessing the storage objects | ||
| // This approach tries both methods safely | ||
| try { | ||
| // Method 1: Try using property descriptor - safer in Safari with cookies blocked | ||
| const descriptor = objGetOwnPropertyDescriptor(gbl, storageTypeName); | ||
| if (!descriptor || !descriptor.get) { | ||
| result = false; | ||
| } | ||
| } catch (e) { | ||
| // If the above fails, attempt a direct access inside a try-catch | ||
| try { | ||
| const storage = gbl[storageTypeName]; | ||
| if (!storage) { | ||
| result = false; | ||
| } | ||
| } catch (e) { | ||
| // If both approaches fail, storage cannot be safely accessed | ||
| result = false; | ||
| } | ||
| } | ||
| } | ||
| } catch (e) { | ||
| result = false; | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Tests storage object (localStorage or sessionStorage) to verify that it is usable | ||
| * More details here: https://mathiasbynens.be/notes/localstorage-pattern | ||
| * @param storageType - Type of storage | ||
| * @returns {Storage} Returns storage object verified that it is usable | ||
| */ | ||
| function _getVerifiedStorageObject(storageType: StorageType): Storage { | ||
| let result = null; | ||
|
|
||
| try { | ||
| if (isNullOrUndefined(getGlobal())) { | ||
| return null; | ||
| } | ||
| let uid = (new Date).toString(); | ||
| let storage: Storage = getGlobalInst(storageType === StorageType.LocalStorage ? "localStorage" : "sessionStorage"); | ||
| let name:string = _storagePrefix + uid; | ||
| storage.setItem(name, uid); | ||
| let fail = storage.getItem(name) !== uid; | ||
| storage.removeItem(name); | ||
| if (!fail) { | ||
| return storage; | ||
| // First check if we can safely access the storage object | ||
| if (_canSafelyAccessStorage(storageType)) { | ||
| const storageTypeName = storageType === StorageType.LocalStorage ? "localStorage" : "sessionStorage"; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pass the resolved string into the _canSafelyAccessStorage to avoid the duplication of the strings
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've refactored the code to pass the resolved storage type string to the _canSafelyAccessStorage function, eliminating the duplication of string resolution. The storage type name is now determined once in the _getVerifiedStorageObject function and passed to _canSafelyAccessStorage. d472d65 |
||
|
|
||
| // Now we can safely try to use the storage | ||
| try { | ||
| let uid = (new Date).toString(); | ||
| let storage: Storage = getGlobalInst(storageTypeName); | ||
| let name:string = _storagePrefix + uid; | ||
| storage.setItem(name, uid); | ||
| let fail = storage.getItem(name) !== uid; | ||
| storage.removeItem(name); | ||
| if (!fail) { | ||
| result = storage; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change this to return a wrapped version of the storage obejct which wraps the setItem() function in a try / catch that handles the failure and resets the state (by setting the value to null)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a storage wrapper in d0ee829 that wraps all Storage operations in try/catch blocks. All storage methods now reset the cached storage state if they encounter an error. |
||
| } | ||
| } catch (exception) { | ||
| // Storage exists but can't be used (quota exceeded, etc.) | ||
| } | ||
| } | ||
| } catch (exception) { | ||
| // eslint-disable-next-line no-empty | ||
| // Catch any unexpected errors | ||
| } | ||
|
|
||
| return null; | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets merge this function into the called as it should create smaller code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged the _canSafelyAccessStorage function into _getVerifiedStorageObject as requested, which should create smaller and more efficient code. All safety checks for handling Safari with blocked cookies are preserved in the integrated implementation. d2cd04d