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

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
   }