Skip to content

Commit

Permalink
misc: Disallow direct usage of globals (#3999)
Browse files Browse the repository at this point in the history
  • Loading branch information
kamilogorek authored Sep 21, 2021
1 parent ba50656 commit 8c77bb6
Show file tree
Hide file tree
Showing 11 changed files with 61 additions and 15 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott

## 6.13.2

- fix(browser): Use getGlobalObject for document check (#3996)

## 6.13.1

- fix(browser): Check for document when sending outcomes (#3993)
Expand Down
6 changes: 4 additions & 2 deletions packages/angular/src/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnIni
import { Event, NavigationEnd, NavigationStart, Router } from '@angular/router';
import { getCurrentHub } from '@sentry/browser';
import { Span, Transaction, TransactionContext } from '@sentry/types';
import { logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils';
import { getGlobalObject, logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils';
import { Observable, Subscription } from 'rxjs';
import { filter, tap } from 'rxjs/operators';

Expand All @@ -12,6 +12,8 @@ let instrumentationInitialized: boolean;
let stashedStartTransaction: (context: TransactionContext) => Transaction | undefined;
let stashedStartTransactionOnLocationChange: boolean;

const global = getGlobalObject<Window>();

/**
* Creates routing instrumentation for Angular Router.
*/
Expand All @@ -26,7 +28,7 @@ export function routingInstrumentation(

if (startTransactionOnPageLoad) {
customStartTransaction({
name: window.location.pathname,
name: global.location.pathname,
op: 'pageload',
});
}
Expand Down
12 changes: 9 additions & 3 deletions packages/browser/src/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { API, captureException, withScope } from '@sentry/core';
import { DsnLike, Event as SentryEvent, Mechanism, Scope, WrappedFunction } from '@sentry/types';
import { addExceptionMechanism, addExceptionTypeValue, logger } from '@sentry/utils';
import { addExceptionMechanism, addExceptionTypeValue, getGlobalObject, logger } from '@sentry/utils';

const global = getGlobalObject<Window>();
let ignoreOnError: number = 0;

/**
Expand Down Expand Up @@ -193,16 +194,21 @@ export interface ReportDialogOptions {
* @hidden
*/
export function injectReportDialog(options: ReportDialogOptions = {}): void {
if (!global.document) {
return;
}

if (!options.eventId) {
logger.error(`Missing eventId option in showReportDialog call`);
return;
}

if (!options.dsn) {
logger.error(`Missing dsn option in showReportDialog call`);
return;
}

const script = document.createElement('script');
const script = global.document.createElement('script');
script.async = true;
script.src = new API(options.dsn).getReportDialogEndpoint(options);

Expand All @@ -211,7 +217,7 @@ export function injectReportDialog(options: ReportDialogOptions = {}): void {
script.onload = options.onLoad;
}

const injectionPoint = document.head || document.body;
const injectionPoint = global.document.head || global.document.body;

if (injectionPoint) {
injectionPoint.appendChild(script);
Expand Down
6 changes: 3 additions & 3 deletions packages/browser/src/transports/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export abstract class BaseTransport implements Transport {
// eslint-disable-next-line deprecation/deprecation
this.url = this._api.getStoreEndpointWithUrlEncodedAuth();

if (this.options.sendClientReports && global && global.document) {
if (this.options.sendClientReports && global.document) {
global.document.addEventListener('visibilitychange', () => {
if (global.document.visibilityState === 'hidden') {
this._flushOutcomes();
Expand Down Expand Up @@ -99,7 +99,7 @@ export abstract class BaseTransport implements Transport {
return;
}

if (!navigator || typeof navigator.sendBeacon !== 'function') {
if (!global.navigator || typeof global.navigator.sendBeacon !== 'function') {
logger.warn('Beacon API not available, skipping sending outcomes.');
return;
}
Expand Down Expand Up @@ -134,7 +134,7 @@ export abstract class BaseTransport implements Transport {
});
const envelope = `${envelopeHeader}\n${itemHeaders}\n${item}`;

navigator.sendBeacon(url, envelope);
global.navigator.sendBeacon(url, envelope);
}

/**
Expand Down
24 changes: 24 additions & 0 deletions packages/eslint-config-sdk/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,30 @@ module.exports = {
// Configuration for files under src
files: ['src/**/*'],
rules: {
'no-restricted-globals': [
'error',
{
name: 'window',
message:
'Some global variables are not available in environments like WebWorker or Node.js. Use getGlobalObject() instead.',
},
{
name: 'document',
message:
'Some global variables are not available in environments like WebWorker or Node.js. Use getGlobalObject() instead.',
},
{
name: 'location',
message:
'Some global variables are not available in environments like WebWorker or Node.js. Use getGlobalObject() instead.',
},
{
name: 'navigator',
message:
'Some global variables are not available in environments like WebWorker or Node.js. Use getGlobalObject() instead.',
},
],

// We want to prevent async await usage in our files to prevent uncessary bundle size.
'@sentry-internal/sdk/no-async-await': 'error',

Expand Down
2 changes: 1 addition & 1 deletion packages/tracing/src/browser/browsertracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ export function getHeaderContext(): Partial<TransactionContext> | undefined {

/** Returns the value of a meta tag */
export function getMetaContent(metaName: string): string | null {
const el = document.querySelector(`meta[name=${metaName}]`);
const el = getGlobalObject<Window>().document.querySelector(`meta[name=${metaName}]`);
return el ? el.getAttribute('content') : null;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/tracing/src/browser/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export class MetricsInstrumentation {
break;
}
case 'resource': {
const resourceName = (entry.name as string).replace(window.location.origin, '');
const resourceName = (entry.name as string).replace(global.location.origin, '');
const endTimestamp = addResourceSpans(transaction, entry, resourceName, startTime, duration, timeOrigin);
// We remember the entry script end time to calculate the difference to the first init mark
if (entryScriptStartTimestamp === undefined && (entryScriptSrc || '').indexOf(resourceName) > -1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@
* limitations under the License.
*/

import { getGlobalObject } from '@sentry/utils';

import { onHidden } from './onHidden';

let firstHiddenTime = -1;

const initHiddenTime = (): number => {
return document.visibilityState === 'hidden' ? 0 : Infinity;
return getGlobalObject<Window>().document.visibilityState === 'hidden' ? 0 : Infinity;
};

const trackChanges = (): void => {
Expand Down
4 changes: 3 additions & 1 deletion packages/tracing/src/browser/web-vitals/lib/onHidden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@
* limitations under the License.
*/

import { getGlobalObject } from '@sentry/utils';

export interface OnHiddenCallback {
(event: Event): void;
}

export const onHidden = (cb: OnHiddenCallback, once?: boolean): void => {
const onHiddenOrPageHide = (event: Event): void => {
if (event.type === 'pagehide' || document.visibilityState === 'hidden') {
if (event.type === 'pagehide' || getGlobalObject<Window>().document.visibilityState === 'hidden') {
cb(event);
if (once) {
removeEventListener('visibilitychange', onHiddenOrPageHide, true);
Expand Down
7 changes: 4 additions & 3 deletions packages/utils/src/misc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ const fallbackGlobalObject = {};
export function getGlobalObject<T>(): T & SentryGlobal {
return (isNodeEnv()
? global
: typeof window !== 'undefined'
? window
: typeof window !== 'undefined' // eslint-disable-line no-restricted-globals
? window // eslint-disable-line no-restricted-globals
: typeof self !== 'undefined'
? self
: fallbackGlobalObject) as T & SentryGlobal;
Expand Down Expand Up @@ -227,8 +227,9 @@ export function addExceptionMechanism(
* A safe form of location.href
*/
export function getLocationHref(): string {
const global = getGlobalObject<Window>();
try {
return document.location.href;
return global.document.location.href;
} catch (oO) {
return '';
}
Expand Down
5 changes: 5 additions & 0 deletions packages/utils/src/object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,15 @@ function normalizeValue<T>(value: T, key?: any): T | string {
return '[Global]';
}

// It's safe to use `window` and `document` here in this manner, as we are asserting using `typeof` first
// which won't throw if they are not present.

// eslint-disable-next-line no-restricted-globals
if (typeof (window as any) !== 'undefined' && (value as unknown) === window) {
return '[Window]';
}

// eslint-disable-next-line no-restricted-globals
if (typeof (document as any) !== 'undefined' && (value as unknown) === document) {
return '[Document]';
}
Expand Down

0 comments on commit 8c77bb6

Please sign in to comment.