-
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 2 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 |
|---|---|---|
|
|
@@ -11,4 +11,4 @@ export interface IApplication { | |
| * The application build version. | ||
| */ | ||
| build: string; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,4 +25,4 @@ export interface IDevice { | |
| * The IP address. | ||
| */ | ||
| ip: string; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,4 +26,4 @@ export interface IInternal { | |
| * Identifies the source of the sdk script | ||
| */ | ||
| sdkSrc: string; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,4 +6,4 @@ export interface ILocation { | |
| * Client IP address for reverse lookup | ||
| */ | ||
| ip: string; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,4 +10,4 @@ export interface ISample { | |
| sampleRate: number; | ||
|
|
||
| isSampledIn(envelope: ITelemetryItem): boolean; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,4 +30,4 @@ export interface ITelemetryTrace { | |
| } | ||
|
|
||
| export interface ITraceState { | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,4 +7,4 @@ | |
| export const enum DataPointType { | ||
| Measurement = 0, | ||
| Aggregation = 1 | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,4 +97,4 @@ export interface ICorrelationConfig { | |
| * should not be excluded. | ||
| */ | ||
| addIntEndpoints?: boolean; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,4 +44,4 @@ export interface IEnvelope extends ISerializable { | |
| * Telemetry data item. | ||
| */ | ||
| data: any; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,53 @@ 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 { | ||
|
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. Lets merge this function into the called as it should create smaller code
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 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 |
||
| const storageTypeName = storageType === StorageType.LocalStorage ? "localStorage" : "sessionStorage"; | ||
|
|
||
| try { | ||
| // First, check if window exists | ||
| if (isNullOrUndefined(getGlobal())) { | ||
| return false; | ||
|
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. refactor the code to only have a single return at the end, using local "result" variable which defaults to true
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 _canSafelyAccessStorage function to use a single return statement with a local 'result' variable initialized to true in commit a488d75. |
||
| } | ||
|
|
||
| // 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 | ||
| const gbl: any = getGlobal(); | ||
|
MSNev marked this conversation as resolved.
Outdated
|
||
|
|
||
| // 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 = Object.getOwnPropertyDescriptor(gbl, storageTypeName); | ||
|
MSNev marked this conversation as resolved.
Outdated
|
||
| if (!descriptor || !descriptor.get) { | ||
| return false; | ||
| } | ||
| } catch (e) { | ||
| // If the above fails, attempt a direct access inside a try-catch | ||
| try { | ||
| const storage = gbl[storageTypeName]; | ||
| if (!storage) { | ||
| return false; | ||
| } | ||
| } catch (e) { | ||
| // If both approaches fail, storage cannot be safely accessed | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Tests storage object (localStorage or sessionStorage) to verify that it is usable | ||
| * More details here: https://mathiasbynens.be/notes/localstorage-pattern | ||
|
|
@@ -31,20 +78,30 @@ function _getLocalStorageObject(): Storage { | |
| */ | ||
| function _getVerifiedStorageObject(storageType: StorageType): Storage { | ||
| try { | ||
| if (isNullOrUndefined(getGlobal())) { | ||
| // First check if we can safely access the storage object | ||
| if (!_canSafelyAccessStorage(storageType)) { | ||
|
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. Also refactor this to have a single return at the end
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 _getVerifiedStorageObject function to also use a single return statement at the end in commit a488d75. |
||
| 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; | ||
|
|
||
| const storageTypeName = storageType === StorageType.LocalStorage ? "localStorage" : "sessionStorage"; | ||
|
|
||
| // 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) { | ||
| return storage; | ||
| } | ||
| } catch (exception) { | ||
| // Storage exists but can't be used (quota exceeded, etc.) | ||
| return null; | ||
| } | ||
| } catch (exception) { | ||
| // eslint-disable-next-line no-empty | ||
| // Catch any unexpected errors | ||
| } | ||
|
|
||
| return null; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.