Skip to content

Commit 84a6dc0

Browse files
lobsterkatielobsterkatie
authored
fix(utils): Prevent logger circular dependencies (getsentry#4069)
This aims to prevent the `logger` -> `misc` -> `logger` dependency cycle from ever happening again. - Move `getGlobalObject` to its own module. - Move `consoleSandbox` into the logger module itself, since that's the only place it's used. - Given how easy it would be to recreate this problem - just add to `logger.ts`'s dependencies (not knowing that that's what they are) any function which logs to the console using the logger - add a warning comment at the top of both dependencies. As a bonus, move `getLocationHref` into the browser module, a move which was missed in an earlier rearrangement.
1 parent f44d370 commit 84a6dc0

File tree

11 files changed

+128
-108
lines changed

11 files changed

+128
-108
lines changed

‎packages/utils/src/browser.ts

+13
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { getGlobalObject } from './global';
12
import { isString } from './is';
23

34
/**
@@ -108,3 +109,15 @@ function _htmlElementAsString(el: unknown, keyAttrs?: string[]): string {
108109
}
109110
return out.join('');
110111
}
112+
113+
/**
114+
* A safe form of location.href
115+
*/
116+
export function getLocationHref(): string {
117+
const global = getGlobalObject<Window>();
118+
try {
119+
return global.document.location.href;
120+
} catch (oO) {
121+
return '';
122+
}
123+
}

‎packages/utils/src/global.ts

+44
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/**
2+
* NOTE: In order to avoid circular dependencies, if you add a function to this module and it needs to print something,
3+
* you must either a) use `console.log` rather than the logger, or b) put your function elsewhere.
4+
*/
5+
6+
/* eslint-disable @typescript-eslint/no-explicit-any */
7+
8+
import { Integration } from '@sentry/types';
9+
10+
import { isNodeEnv } from './node';
11+
12+
/** Internal */
13+
interface SentryGlobal {
14+
Sentry?: {
15+
Integrations?: Integration[];
16+
};
17+
SENTRY_ENVIRONMENT?: string;
18+
SENTRY_DSN?: string;
19+
SENTRY_RELEASE?: {
20+
id?: string;
21+
};
22+
__SENTRY__: {
23+
globalEventProcessors: any;
24+
hub: any;
25+
logger: any;
26+
};
27+
}
28+
29+
const fallbackGlobalObject = {};
30+
31+
/**
32+
* Safely get global scope object
33+
*
34+
* @returns Global scope object
35+
*/
36+
export function getGlobalObject<T>(): T & SentryGlobal {
37+
return (isNodeEnv()
38+
? global
39+
: typeof window !== 'undefined' // eslint-disable-line no-restricted-globals
40+
? window // eslint-disable-line no-restricted-globals
41+
: typeof self !== 'undefined'
42+
? self
43+
: fallbackGlobalObject) as T & SentryGlobal;
44+
}

‎packages/utils/src/index.ts

+1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ export * from './async';
22
export * from './browser';
33
export * from './dsn';
44
export * from './error';
5+
export * from './global';
56
export * from './instrument';
67
export * from './is';
78
export * from './logger';

‎packages/utils/src/instrument.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
/* eslint-disable @typescript-eslint/ban-types */
33
import { WrappedFunction } from '@sentry/types';
44

5+
import { getGlobalObject } from './global';
56
import { isInstanceOf, isString } from './is';
67
import { logger } from './logger';
7-
import { getGlobalObject } from './misc';
88
import { fill } from './object';
99
import { getFunctionName } from './stacktrace';
1010
import { supportsHistory, supportsNativeFetch } from './supports';

‎packages/utils/src/logger.ts

+47-1
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,58 @@
11
/* eslint-disable @typescript-eslint/no-explicit-any */
2-
import { consoleSandbox, getGlobalObject } from './misc';
2+
import { WrappedFunction } from '@sentry/types';
3+
4+
import { getGlobalObject } from './global';
35

46
// TODO: Implement different loggers for different environments
57
const global = getGlobalObject<Window | NodeJS.Global>();
68

79
/** Prefix for logging strings */
810
const PREFIX = 'Sentry Logger ';
911

12+
/** JSDoc */
13+
interface ExtensibleConsole extends Console {
14+
[key: string]: any;
15+
}
16+
17+
/**
18+
* Temporarily unwrap `console.log` and friends in order to perform the given callback using the original methods.
19+
* Restores wrapping after the callback completes.
20+
*
21+
* @param callback The function to run against the original `console` messages
22+
* @returns The results of the callback
23+
*/
24+
export function consoleSandbox(callback: () => any): any {
25+
const global = getGlobalObject<Window>();
26+
const levels = ['debug', 'info', 'warn', 'error', 'log', 'assert'];
27+
28+
if (!('console' in global)) {
29+
return callback();
30+
}
31+
32+
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
33+
const originalConsole = (global as any).console as ExtensibleConsole;
34+
const wrappedLevels: { [key: string]: any } = {};
35+
36+
// Restore all wrapped console methods
37+
levels.forEach(level => {
38+
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
39+
if (level in (global as any).console && (originalConsole[level] as WrappedFunction).__sentry_original__) {
40+
wrappedLevels[level] = originalConsole[level] as WrappedFunction;
41+
originalConsole[level] = (originalConsole[level] as WrappedFunction).__sentry_original__;
42+
}
43+
});
44+
45+
// Perform callback manipulations
46+
const result = callback();
47+
48+
// Revert restoration to wrapped state
49+
Object.keys(wrappedLevels).forEach(level => {
50+
originalConsole[level] = wrappedLevels[level];
51+
});
52+
53+
return result;
54+
}
55+
1056
/** JSDoc */
1157
class Logger {
1258
/** JSDoc */

‎packages/utils/src/misc.ts

+2-86
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,9 @@
11
/* eslint-disable @typescript-eslint/no-explicit-any */
2-
import { Event, Integration, StackFrame, WrappedFunction } from '@sentry/types';
2+
import { Event, StackFrame } from '@sentry/types';
33

4-
import { isNodeEnv } from './node';
4+
import { getGlobalObject } from './global';
55
import { snipLine } from './string';
66

7-
/** Internal */
8-
interface SentryGlobal {
9-
Sentry?: {
10-
Integrations?: Integration[];
11-
};
12-
SENTRY_ENVIRONMENT?: string;
13-
SENTRY_DSN?: string;
14-
SENTRY_RELEASE?: {
15-
id?: string;
16-
};
17-
__SENTRY__: {
18-
globalEventProcessors: any;
19-
hub: any;
20-
logger: any;
21-
};
22-
}
23-
24-
const fallbackGlobalObject = {};
25-
26-
/**
27-
* Safely get global scope object
28-
*
29-
* @returns Global scope object
30-
*/
31-
export function getGlobalObject<T>(): T & SentryGlobal {
32-
return (isNodeEnv()
33-
? global
34-
: typeof window !== 'undefined' // eslint-disable-line no-restricted-globals
35-
? window // eslint-disable-line no-restricted-globals
36-
: typeof self !== 'undefined'
37-
? self
38-
: fallbackGlobalObject) as T & SentryGlobal;
39-
}
40-
417
/**
428
* Extended Window interface that allows for Crypto API usage in IE browsers
439
*/
@@ -143,44 +109,6 @@ export function getEventDescription(event: Event): string {
143109
return event.event_id || '<unknown>';
144110
}
145111

146-
/** JSDoc */
147-
interface ExtensibleConsole extends Console {
148-
[key: string]: any;
149-
}
150-
151-
/** JSDoc */
152-
export function consoleSandbox(callback: () => any): any {
153-
const global = getGlobalObject<Window>();
154-
const levels = ['debug', 'info', 'warn', 'error', 'log', 'assert'];
155-
156-
if (!('console' in global)) {
157-
return callback();
158-
}
159-
160-
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
161-
const originalConsole = (global as any).console as ExtensibleConsole;
162-
const wrappedLevels: { [key: string]: any } = {};
163-
164-
// Restore all wrapped console methods
165-
levels.forEach(level => {
166-
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
167-
if (level in (global as any).console && (originalConsole[level] as WrappedFunction).__sentry_original__) {
168-
wrappedLevels[level] = originalConsole[level] as WrappedFunction;
169-
originalConsole[level] = (originalConsole[level] as WrappedFunction).__sentry_original__;
170-
}
171-
});
172-
173-
// Perform callback manipulations
174-
const result = callback();
175-
176-
// Revert restoration to wrapped state
177-
Object.keys(wrappedLevels).forEach(level => {
178-
originalConsole[level] = wrappedLevels[level];
179-
});
180-
181-
return result;
182-
}
183-
184112
/**
185113
* Adds exception values, type and value to an synthetic Exception.
186114
* @param event The event to modify.
@@ -223,18 +151,6 @@ export function addExceptionMechanism(
223151
}
224152
}
225153

226-
/**
227-
* A safe form of location.href
228-
*/
229-
export function getLocationHref(): string {
230-
const global = getGlobalObject<Window>();
231-
try {
232-
return global.document.location.href;
233-
} catch (oO) {
234-
return '';
235-
}
236-
}
237-
238154
// https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
239155
const SEMVER_REGEXP = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$/;
240156

‎packages/utils/src/node.ts

+5
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
/**
2+
* NOTE: In order to avoid circular dependencies, if you add a function to this module and it needs to print something,
3+
* you must either a) use `console.log` rather than the logger, or b) put your function elsewhere.
4+
*/
5+
16
/**
27
* Checks whether we're in the Node.js or Browser environment
38
*

‎packages/utils/src/supports.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1+
import { getGlobalObject } from './global';
12
import { logger } from './logger';
2-
import { getGlobalObject } from './misc';
33

44
/**
55
* Tells whether current environment supports ErrorEvent objects

‎packages/utils/src/time.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { getGlobalObject } from './misc';
1+
import { getGlobalObject } from './global';
22
import { dynamicRequire, isNodeEnv } from './node';
33

44
/**

‎packages/utils/test/global.test.ts

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { getGlobalObject } from '../src/global';
2+
3+
describe('getGlobalObject()', () => {
4+
test('should return the same object', () => {
5+
const backup = global.process;
6+
delete global.process;
7+
const first = getGlobalObject();
8+
const second = getGlobalObject();
9+
expect(first).toEqual(second);
10+
global.process = backup;
11+
});
12+
});

‎packages/utils/test/misc.test.ts

+1-18
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,6 @@
11
import { StackFrame } from '@sentry/types';
22

3-
import {
4-
addContextToFrame,
5-
getEventDescription,
6-
getGlobalObject,
7-
parseRetryAfterHeader,
8-
stripUrlQueryAndFragment,
9-
} from '../src/misc';
3+
import { addContextToFrame, getEventDescription, parseRetryAfterHeader, stripUrlQueryAndFragment } from '../src/misc';
104

115
describe('getEventDescription()', () => {
126
test('message event', () => {
@@ -117,17 +111,6 @@ describe('getEventDescription()', () => {
117111
});
118112
});
119113

120-
describe('getGlobalObject()', () => {
121-
test('should return the same object', () => {
122-
const backup = global.process;
123-
delete global.process;
124-
const first = getGlobalObject();
125-
const second = getGlobalObject();
126-
expect(first).toEqual(second);
127-
global.process = backup;
128-
});
129-
});
130-
131114
describe('parseRetryAfterHeader', () => {
132115
test('no header', () => {
133116
expect(parseRetryAfterHeader(Date.now())).toEqual(60 * 1000);

0 commit comments

Comments
 (0)