fix: Add protocol detection for get/request calls without explict protocol (#3950)

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`.

Author: thomaspurchas

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);
 

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];

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,83 @@ describe('tracing', () => {
     expect(sentryTraceHeader).not.toBeDefined();
   });
 });
+
+describe('default protocols', () => {
+  function captureBreadcrumb(key: string): Promise<Breadcrumb> {
+    const hub = new Hub();
+    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+
+    let resolve: (value: Breadcrumb | PromiseLike<Breadcrumb>) => void;
+    const p = new Promise<Breadcrumb>(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', async () => {
+    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,
+    });
+
+    const b = await p;
+    expect(b.data?.url).toEqual(expect.stringContaining('https://'));
+  });
+});