Skip to content
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

fix: Prevent fetch errors loops with invalid fetch implementations #3318

Merged
merged 1 commit into from
Mar 10, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 73 additions & 5 deletions packages/browser/src/transports/fetch.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,82 @@
import { eventToSentryRequest, sessionToSentryRequest } from '@sentry/core';
import { Event, Response, SentryRequest, Session } from '@sentry/types';
import { getGlobalObject, supportsReferrerPolicy, SyncPromise } from '@sentry/utils';
import { Event, Response, SentryRequest, Session, TransportOptions } from '@sentry/types';
import { getGlobalObject, logger, supportsReferrerPolicy, SyncPromise } from '@sentry/utils';

import { BaseTransport } from './base';

const global = getGlobalObject<Window>();
type FetchImpl = typeof fetch;

/**
* A special usecase for incorrectly wrapped Fetch APIs in conjunction with ad-blockers.
* Whenever someone wraps the Fetch API and returns the wrong promise chain,
* this chain becomes orphaned and there is no possible way to capture it's rejections
* other than allowing it bubble up to this very handler. eg.
*
* const f = window.fetch;
* window.fetch = function () {
* const p = f.apply(this, arguments);
*
* p.then(function() {
* console.log('hi.');
* });
*
* return p;
* }
*
* `p.then(function () { ... })` is producing a completely separate promise chain,
* however, what's returned is `p` - the result of original `fetch` call.
*
* This mean, that whenever we use the Fetch API to send our own requests, _and_
* some ad-blocker blocks it, this orphaned chain will _always_ reject,
* effectively causing another event to be captured.
* This makes a whole process become an infinite loop, which we need to somehow
* deal with, and break it in one way or another.
*
* To deal with this issue, we are making sure that we _always_ use the real
* browser Fetch API, instead of relying on what `window.fetch` exposes.
* The only downside to this would be missing our own requests as breadcrumbs,
* but because we are already not doing this, it should be just fine.
*
* Possible failed fetch error messages per-browser:
*
* Chrome: Failed to fetch
* Edge: Failed to Fetch
* Firefox: NetworkError when attempting to fetch resource
* Safari: resource blocked by content blocker
*/
function getNativeFetchImplementation(): FetchImpl {
// Make sure that the fetch we use is always the native one.
const global = getGlobalObject<Window>();
const document = global.document;
// eslint-disable-next-line deprecation/deprecation
if (typeof document?.createElement === `function`) {
try {
const sandbox = document.createElement('iframe');
sandbox.hidden = true;
document.head.appendChild(sandbox);
if (sandbox.contentWindow?.fetch) {
return sandbox.contentWindow.fetch.bind(global);
}
document.head.removeChild(sandbox);
Comment on lines +54 to +60
Copy link
Contributor

@sod sod Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so slow, that it doesn't execute below 50ms for mobile lighthouse. Thus it delays the TTI metric plus adds to TBT in lighthouse, even if you only execute install in a single macrotask, thus including sentry now costs you points in mobile lighthouse and lowers your ranking on google.

desktop

image

mobile

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was 6.0.1 btw., so quite some regression

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting us know, should be patched in #3341

} catch (e) {
logger.warn('Could not create sandbox iframe for pure fetch check, bailing to window.fetch: ', e);
}
}
return global.fetch.bind(global);
}

/** `fetch` based transport */
export class FetchTransport extends BaseTransport {
/**
* Fetch API reference which always points to native browser implementation.
*/
private _fetch: typeof fetch;

constructor(options: TransportOptions, fetchImpl: FetchImpl = getNativeFetchImplementation()) {
super(options);
this._fetch = fetchImpl;
}

/**
* @inheritDoc
*/
Expand Down Expand Up @@ -54,8 +123,7 @@ export class FetchTransport extends BaseTransport {

return this._buffer.add(
new SyncPromise<Response>((resolve, reject) => {
global
.fetch(sentryRequest.url, options)
this._fetch(sentryRequest.url, options)
.then(response => {
const headers = {
'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
Expand Down
28 changes: 17 additions & 11 deletions packages/browser/test/unit/transports/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ let transport: Transports.BaseTransport;
describe('FetchTransport', () => {
beforeEach(() => {
fetch = (stub(window, 'fetch') as unknown) as SinonStub;
transport = new Transports.FetchTransport({ dsn: testDsn });
transport = new Transports.FetchTransport({ dsn: testDsn }, window.fetch);
});

afterEach(() => {
Expand Down Expand Up @@ -83,12 +83,15 @@ describe('FetchTransport', () => {
});

it('passes in headers', async () => {
transport = new Transports.FetchTransport({
dsn: testDsn,
headers: {
Authorization: 'Basic GVzdDp0ZXN0Cg==',
transport = new Transports.FetchTransport(
{
dsn: testDsn,
headers: {
Authorization: 'Basic GVzdDp0ZXN0Cg==',
},
},
});
window.fetch,
);
const response = { status: 200, headers: new Headers() };

fetch.returns(Promise.resolve(response));
Expand All @@ -109,12 +112,15 @@ describe('FetchTransport', () => {
});

it('passes in fetch parameters', async () => {
transport = new Transports.FetchTransport({
dsn: testDsn,
fetchParameters: {
credentials: 'include',
transport = new Transports.FetchTransport(
{
dsn: testDsn,
fetchParameters: {
credentials: 'include',
},
},
});
window.fetch,
);
const response = { status: 200, headers: new Headers() };

fetch.returns(Promise.resolve(response));
Expand Down