From 43974056885b2509edab6955897a3e1585fcc4fa Mon Sep 17 00:00:00 2001 From: Thomas Purchas Date: Wed, 1 Sep 2021 14:07:19 +0100 Subject: [PATCH] Add protocol detection for get/request calls without explict protocol You can call `get`/`request` on `http`/`https` without providing an explicit protocol in the request options. In this case the protocol is automatically set based on which module you made the call to. Previously calls like this would result in breadcrumbs and traces without a protocol in the URL creating downstream issues for consumers of Breadcrumbs as the missing protocol results in URL throwing parsing errors. We now attempt to get the protocol by either extracting it from the `agent` passed as part of the request options, or by extracting it from the `globalAgent` on which ever http module is pass in `this`. --- packages/node/src/integrations/http.ts | 2 +- packages/node/src/integrations/utils/http.ts | 13 +++ packages/node/test/integrations/http.test.ts | 86 ++++++++++++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 77e72228ee66..ad80462f3c25 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -94,7 +94,7 @@ function _createWrappedRequestMethodFactory( // eslint-disable-next-line @typescript-eslint/no-this-alias const httpModule = this; - const requestArgs = normalizeRequestArgs(args); + const requestArgs = normalizeRequestArgs(this, args); const requestOptions = requestArgs[0]; const requestUrl = extractUrl(requestOptions); diff --git a/packages/node/src/integrations/utils/http.ts b/packages/node/src/integrations/utils/http.ts index 26da20ae86e3..e5d73ebc1d5a 100644 --- a/packages/node/src/integrations/utils/http.ts +++ b/packages/node/src/integrations/utils/http.ts @@ -1,5 +1,6 @@ import { getCurrentHub } from '@sentry/core'; import * as http from 'http'; +import * as https from 'https'; import { URL } from 'url'; /** @@ -122,6 +123,7 @@ export function urlToOptions(url: URL): RequestOptions { * @returns Equivalent args of the form [ RequestOptions ] or [ RequestOptions, RequestCallback ]. */ export function normalizeRequestArgs( + httpModule: typeof http | typeof https, requestArgs: RequestMethodArgs, ): [RequestOptions] | [RequestOptions, RequestCallback] { let callback, requestOptions; @@ -145,6 +147,17 @@ export function normalizeRequestArgs( requestOptions = { ...requestOptions, ...requestArgs[1] }; } + // Figure out the protocol if it's currently missing + if (requestOptions.protocol === undefined) { + // Worst case we end up populating protocol with undefined, which it already is + /* eslint-disable @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any */ + requestOptions.protocol = + (requestOptions.agent as any)?.protocol || + (requestOptions._defaultAgent as any)?.protocol || + (httpModule.globalAgent as any)?.protocol; + /* eslint-enable @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any */ + } + // return args in standardized form if (callback) { return [requestOptions, callback]; diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index 38370650daac..b62294ab7969 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -2,12 +2,17 @@ import * as sentryCore from '@sentry/core'; import { Hub } from '@sentry/hub'; import * as hubModule from '@sentry/hub'; import { addExtensionMethods, Span, TRACEPARENT_REGEXP, Transaction } from '@sentry/tracing'; +import { parseSemver } from '@sentry/utils'; import * as http from 'http'; +import * as https from 'https'; import * as nock from 'nock'; +import { Breadcrumb } from '../../src'; import { NodeClient } from '../../src/client'; import { Http as HttpIntegration } from '../../src/integrations/http'; +const NODE_VERSION = parseSemver(process.versions.node); + describe('tracing', () => { function createTransactionOnScope() { const hub = new Hub( @@ -91,3 +96,84 @@ describe('tracing', () => { expect(sentryTraceHeader).not.toBeDefined(); }); }); + +describe('default protocols', () => { + function captureBreadcrumb(key: string): Promise { + const hub = new Hub(); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + + let resolve: (value: Breadcrumb | PromiseLike) => void; + const p = new Promise(r => { + resolve = r; + }); + hub.bindClient( + new NodeClient({ + dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', + integrations: [new HttpIntegration({ breadcrumbs: true })], + beforeBreadcrumb: (b: Breadcrumb) => { + if ((b.data?.url as string).includes(key)) { + resolve(b); + } + return b; + }, + }), + ); + + return p; + } + + it('http module', async () => { + const key = 'catrunners'; + const p = captureBreadcrumb(key); + + nock(`http://${key}.ingest.sentry.io`) + .get('/api/123122332/store/') + .reply(200); + + http.get({ + host: `${key}.ingest.sentry.io`, + path: '/api/123122332/store/', + }); + + const b = await p; + expect(b.data?.url).toEqual(expect.stringContaining('http://')); + }); + + it('https module', () => { + const key = 'catcatchers'; + const p = captureBreadcrumb(key); + + let nockProtocol = 'https:'; + // NOTE: Prior to Node 9, `https` used internals of `http` module, so + // the integration doesn't patch the `https` module. However this then + // causes issues with nock, because nock will patch the `https` module + // regardless (if it asked to mock a https:// url), preventing the + // request from reaching the integrations patch of the `http` module. + // The result is test failures in Node v8 and lower. + // + // We can work around this by telling giving nock a http:// url, so it + // only patches the `http` module, then Nodes usage of the `http` module + // in the `https` module results in both nock's and the integrations + // patch being called. All this relies on nock not properly checking + // the agent passed to `http.get` / `http.request`, thus resulting in it + // intercepting a https:// request with http:// mock. It's a safe bet + // because the latest versions of nock no longer support Node v8 and lower, + // so won't bother dealing with this old Node edge case. + if (NODE_VERSION.major && NODE_VERSION.major < 9) { + nockProtocol = 'http:'; + } + nock(`${nockProtocol}://${key}.ingest.sentry.io`) + .get('/api/123122332/store/') + .reply(200); + + https.get({ + host: `${key}.ingest.sentry.io`, + path: '/api/123122332/store/', + timeout: 300, + }); + + return p.then(b => { + expect(b.data?.url).toEqual(expect.stringContaining('https://')); + }); + }); +});