fix: handle Node 10 style http requests (#1233)

AaronFriel · web-flow · commit 511b21c8563d · 2020-04-15T21:53:40.000-07:00
* fix: handle Node 10 style http requests In Node 10 and above and libraries which use the `request(url, options, cb?)` overload of the request function such as `got` 10.x, request tracing fails. Environment details ------------------- - OS: any - Node.js version: 10.x - npm version: 6.x - `@google-cloud/trace-agent` version: 4.5.2 Steps to reproduce ------------------ 1. Use `got` version `10.x` or manually invoke a request with a separate URL and options argument 2. In a root span, try to do a `got.post` or any other request as above and log the `x-cloud-trace-header` in the server receiving the request 3. Observe that the request isn't traced and the trace header is absent Bug --- The bug is in three parts: 1. If condition filters out valid requests: https://github.com/googleapis/cloud-trace-nodejs/blob/ac7e886c178ca9c34502e9baa9eb190d23104347/src/plugins/plugin-http.ts#L95-L113 When got 10 makes a request, it sends three arguments: url (as an object), options (as an object), and callback (as a function). This if condition blocks tracing all requests from got 10. 2. `Object.assign` does not work for URL properties because URL properties are not enumerable. https://github.com/googleapis/cloud-trace-nodejs/blob/ac7e886c178ca9c34502e9baa9eb190d23104347/src/plugins/plugin-http.ts#L114-L126 If the if condition is changed above so that we can fall through to line 125, we hit a second bug. This last `options = Object.assign(...)` does not work because URL properties are not enumerable. The result is that properties such as "hostname", "host", etc. are not copied into the options. 3. The wrapped request is called with the deficient object https://github.com/googleapis/cloud-trace-nodejs/blob/ac7e886c178ca9c34502e9baa9eb190d23104347/src/plugins/plugin-http.ts#L171 Given the incorrect Object.assign, even if we repair that defect, we end up sending this rewritten object to the destination and we destroy any non-enumerable symbols or other properties assigned to the url or options object we were passed. Fix --- Looking at the source for CreateRequest, the function called by `http.request`, we see that the "input" argument is always replaced by an object, as opposed to a URL instance, before being `ObjectAssigned` https://github.com/nodejs/node/blob/dccdc51788bd5337f9fd80441ef52932383a2441/lib/_http_client.js#L88-L123 We can do the same. Our needs are a bit different, but we can ensure that before the object assign the url value is always converted to a `RequestOptions` object. * Update src/plugins/plugin-http.ts * fix: let http.request throw error on undefined/falsey URL * chore: failing tests for Node 10.x style http.request calls
diff --git a/src/plugins/plugin-http.ts b/src/plugins/plugin-http.ts
@@ -17,8 +17,7 @@ import {Agent, ClientRequest, ClientRequestArgs, request} from 'http';
 import * as httpsModule from 'https';
 import * as semver from 'semver';
 import * as shimmer from 'shimmer';
-// eslint-disable-next-line node/no-deprecated-api
-import {URL, parse as urlParse} from 'url';
+import {URL, UrlWithStringQuery} from 'url';
 
 import {Plugin, Tracer} from '../plugin-types';
 
@@ -95,6 +94,33 @@ function isTraceAgentRequest(options: httpModule.RequestOptions, api: Tracer) {
   );
 }
 
+/**
+ * Transform a url to a request options.
+ *
+ * From: https://github.com/nodejs/node/blob/v12.16.2/lib/internal/url.js#L1271-L1290
+ */
+function urlToOptions(url: URL): httpModule.RequestOptions {
+  const options: httpModule.RequestOptions | UrlWithStringQuery = {
+    protocol: url.protocol,
+    hostname:
+      typeof url.hostname === 'string' && url.hostname.startsWith('[')
+        ? url.hostname.slice(1, -1)
+        : url.hostname,
+    hash: url.hash,
+    search: url.search,
+    pathname: url.pathname,
+    path: `${url.pathname || ''}${url.search || ''}`,
+    href: url.href,
+  };
+  if (url.port !== '') {
+    options.port = Number(url.port);
+  }
+  if (url.username || url.password) {
+    options.auth = `${url.username}:${url.password}`;
+  }
+  return options;
+}
+
 function makeRequestTrace(
   protocol: string,
   request: RequestFunction,
@@ -110,17 +136,17 @@ function makeRequestTrace(
       | ((res: httpModule.IncomingMessage) => void),
     callback?: (res: httpModule.IncomingMessage) => void
   ): ClientRequest {
-    // These are error conditions; defer to http.request and don't trace.
-    if (!url || (typeof url === 'object' && typeof options === 'object')) {
+    let urlString: string | undefined;
+    if (!url) {
+      // These are error conditions; defer to http.request and don't trace.
       // eslint-disable-next-line prefer-rest-params
       return request.apply(this, arguments);
-    }
-
-    let urlString;
-    if (typeof url === 'string') {
+    } else if (typeof url === 'string') {
       // save the value of uri so we don't have to reconstruct it later
       urlString = url;
-      url = urlParse(url);
+      url = urlToOptions(new URL(url));
+    } else if (url instanceof URL) {
+      url = urlToOptions(url);
     }
     if (typeof options === 'function') {
       callback = options;
diff --git a/test/plugins/test-trace-http.ts b/test/plugins/test-trace-http.ts
@@ -141,6 +141,54 @@ for (const nodule of Object.keys(servers) as Array<keyof typeof servers>) {
           // http(url, options, cb) is not a recognized signature in Node 8.
           versions: '>=10.x',
         },
+        {
+          description: 'calling http.get with a string url and options',
+          fn: async () => {
+            const waitForResponse = new WaitForResponse();
+            http.get(
+              `${nodule}://localhost:${port}`,
+              {
+                rejectUnauthorized: false,
+              },
+              waitForResponse.handleResponse
+            );
+            await waitForResponse.done;
+          },
+          // http(url, options, cb) is not a recognized signature in Node 8.
+          versions: '>=10.x',
+        },
+        {
+          description: 'calling http.get with a URL object and options',
+          fn: async () => {
+            const waitForResponse = new WaitForResponse();
+            http.get(
+              new URL(`${nodule}://localhost:${port}`),
+              {
+                rejectUnauthorized: false,
+              },
+              waitForResponse.handleResponse
+            );
+            await waitForResponse.done;
+          },
+          // http(url, options, cb) is not a recognized signature in Node 8.
+          versions: '>=10.x',
+        },
+        {
+          description: 'calling http.get with a parsed URL and options',
+          fn: async () => {
+            const waitForResponse = new WaitForResponse();
+            http.get(
+              {hostname: 'localhost', port: port, protocol: `${nodule}:`},
+              {
+                rejectUnauthorized: false,
+              },
+              waitForResponse.handleResponse
+            );
+            await waitForResponse.done;
+          },
+          // http(url, options, cb) is not a recognized signature in Node 8.
+          versions: '>=10.x',
+        },
         {
           description: 'calling http.get and using return value',
           fn: async () => {
