fix: instrumentation of http.request et al has a few edge cases (el…

…astic#3090)

This change updates the argument munging that is done for the wrapping
of `http.request` et al to fix some cases where the instrumentation
would *change the behaviour* of the function.  The main fix here is
that before this change the conversion of a URL instance to request
options would accidentally drop basic auth info (the 'username' and
'password' fields).

This issue has existed since v2.17.0. It was introduced in commit dd60b12
when switching directly from `url.parse()` to `new URL()`. Recent
versions of node have a `url.urlToHttpOptions()` to help with this
conversion.

Fixes: elastic#2044
Fixes: elastic#2382
Refs: https://discuss.elastic.co/t/issue-apm-agent-with-backend-requests-username-password-url/322903

CHANGELOG.asciidoc

@@ -47,6 +47,12 @@ Notes:
 [float]
 ===== Bug fixes
 
+* Fix instrumentation of `http.request()` and `http.get()` (and the same
+  for `https.`) so that Basic auth fields are not lost. Before this change
+  if the first arg was a URL or string with `username` and/or `password`
+  values, e.g. `https://user:pass@...`, then the auth fields were not
+  included in the actual HTTP request.  ({issues}2044[#2044])
+
 [float]
 ===== Chores
 

lib/instrumentation/http-shared.js

@@ -6,15 +6,48 @@
 
 'use strict'
 
-var url = require('url')
+var { URL, urlToHttpOptions } = require('url')
+
 var endOfStream = require('end-of-stream')
+const semver = require('semver')
 
-var { parseUrl } = require('../parsers')
 var { getHTTPDestination } = require('./context')
 
 const transactionForResponse = new WeakMap()
 exports.transactionForResponse = transactionForResponse
 
+const nodeHttpRequestSupportsSeparateUrlArg = semver.gte(process.version, '10.9.0')
+
+if (!urlToHttpOptions) {
+  // Utility function that converts a URL object into an ordinary
+  // options object as expected by the http.request and https.request
+  // APIs.
+  //
+  // Adapted from https://github.com/nodejs/node/blob/v18.13.0/lib/internal/url.js#L1408-L1431
+  // Added in: v15.7.0, v14.18.0.
+  urlToHttpOptions = function (url) {
+    const options = {
+      protocol: url.protocol,
+      hostname: typeof url.hostname === 'string' &&
+                String.prototype.startsWith(url.hostname, '[')
+        ? String.prototype.slice(url.hostname, 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 = `${decodeURIComponent(url.username)}:${decodeURIComponent(url.password)}`
+    }
+    return options
+  }
+}
+
 exports.instrumentRequest = function (agent, moduleName) {
   var ins = agent._instrumentation
   return function (orig) {
@@ -101,73 +134,92 @@ function shouldIgnoreRequest (agent, req) {
   return false
 }
 
-function formatURL (item) {
-  return {
-    href: item.href,
-    pathname: item.pathname,
-    path: item.pathname + (item.search || ''),
-    protocol: item.protocol,
-    host: item.host,
-    port: item.port,
-    hostname: item.hostname,
-    hash: item.hash,
-    search: item.search
-  }
-}
-
-// NOTE: This will also stringify and parse URL instances
-// to a format which can be mixed into the options object.
-function ensureUrl (v) {
-  if (typeof v === 'string') {
-    return formatURL(parseUrl(v))
-  } else if (url.URL && v instanceof url.URL) {
-    return formatURL(v)
-  } else {
-    return v
-  }
-}
-
-function getSafeHost (res) {
-  return res.getHeader ? res.getHeader('Host') : res._headers.host
+/**
+ * Safely get the Host header used in the given client request without incurring
+ * the core Node.js DEP0066 warning for using `req._headers`.
+ *
+ * @param {http.ClientRequest} req
+ * @returns {string}
+ */
+function getSafeHost (req) {
+  return req.getHeader ? req.getHeader('Host') : req._headers.host
 }
 
 exports.traceOutgoingRequest = function (agent, moduleName, method) {
   var ins = agent._instrumentation
 
   return function (orig) {
-    return function (...args) {
+    return function (input, options, cb) {
       const parentRunContext = ins.currRunContext()
       var span = ins.createSpan(null, 'external', 'http', { exitSpan: true })
       var id = span && span.transaction.id
-
       agent.logger.debug('intercepted call to %s.%s %o', moduleName, method, { id: id })
 
-      var options = {}
-      var newArgs = [options]
-      for (const arg of args) {
-        if (typeof arg === 'function') {
-          newArgs.push(ins.bindFunctionToRunContext(parentRunContext, arg))
+      // Reproduce the argument handling from node/lib/_http_client.js#ClientRequest().
+      //
+      // The `new URL(...)` calls in this block *could* throw INVALID_URL, but
+      // that would happen anyway when calling `orig(...)`. The only slight
+      // downside is that the Error stack won't originate inside "_http_client.js".
+      if (!nodeHttpRequestSupportsSeparateUrlArg) {
+        // Signature from node <10.9.0:
+        //    http.request(options[, callback])
+        //        options <Object> | <string> | <URL>
+        cb = options
+        options = input
+        if (typeof options === 'string') {
+          options = urlToHttpOptions(new URL(options))
+        } else if (options instanceof URL) {
+          options = urlToHttpOptions(options)
+        } else {
+          options = Object.assign({}, options)
+        }
+      } else {
+        // Signature from node >=10.9.0:
+        //    http.request(options[, callback])
+        //    http.request(url[, options][, callback])
+        //        url <string> | <URL>
+        //        options <Object>
+        if (typeof input === 'string') {
+          input = urlToHttpOptions(new URL(input))
+        } else if (input instanceof URL) {
+          input = urlToHttpOptions(input)
+        } else {
+          cb = options
+          options = input
+          input = null
+        }
+
+        if (typeof options === 'function') {
+          cb = options
+          options = input || {}
         } else {
-          Object.assign(options, ensureUrl(arg))
+          options = Object.assign(input || {}, options)
         }
       }
 
-      if (!options.headers) options.headers = {}
+      const newArgs = [options]
+      if (cb !== undefined) {
+        if (typeof cb === 'function') {
+          newArgs.push(ins.bindFunctionToRunContext(parentRunContext, cb))
+        } else {
+          newArgs.push(cb)
+        }
+      }
 
       // W3C trace-context propagation.
       // There are a number of reasons why `span` might be null: child of an
       // exit span, `transactionMaxSpans` was hit, unsampled transaction, etc.
       // If so, then fallback to the current run context's span or transaction,
       // if any.
-      var parent = span || parentRunContext.currSpan() || parentRunContext.currTransaction()
-
-      const newHeaders = Object.assign({}, options.headers)
+      const parent = span || parentRunContext.currSpan() || parentRunContext.currTransaction()
       if (parent) {
-        parent.propagateTraceContextHeaders(newHeaders, function (carrier, name, value) {
+        const headers = Object.assign({}, options.headers)
+        parent.propagateTraceContextHeaders(headers, function (carrier, name, value) {
           carrier[name] = value
         })
+        options.headers = headers
       }
-      options.headers = newHeaders
+
       if (!span) {
         return orig.apply(this, newArgs)
       }
@@ -249,7 +301,7 @@ exports.traceOutgoingRequest = function (agent, moduleName, method) {
 // Creates a sanitized URL suitable for the span's HTTP context
 //
 // This function reconstructs a URL using the request object's properties
-// where it can (node versions v14.5.0, v12.19.0 and later) , and falling
+// where it can (node versions v14.5.0, v12.19.0 and later), and falling
 // back to the options where it can not.  This function also strips any
 // authentication information provided with the hostname.  In other words
 //
@@ -260,7 +312,7 @@ exports.traceOutgoingRequest = function (agent, moduleName, method) {
 // NOTE: The options argument may not be the same options that are passed
 // to http.request if the caller uses the the http.request(url,options,...)
 // method signature. The agent normalizes the url and options into a single
-// options array. This function expects those pre-normalized options.
+// options object. This function expects those pre-normalized options.
 //
 // @param {ClientRequest} req
 // @param {object} options
@@ -273,6 +325,7 @@ function getUrlFromRequestAndOptions (req, options, fallbackProtocol) {
   options = options || {}
   req = req || {}
   req.agent = req.agent || {}
+
   if (isProxiedRequest(req)) {
     return req.path
   }