http: fix perf_hooks detail.req.url port and proxied path

The perf_hooks HTTP client entry built the reported URL from the bare hostname, dropping non-default ports and IPv6 brackets, and appended the request path even after it had been rewritten to absolute-form for proxying, duplicating the protocol and authority.

Report the connection authority captured at request creation and, for proxied requests, use the rewritten absolute-form target as-is.

Fixes: #59625

Author: stefanobaghino

lib/_http_client.js

@@ -121,6 +121,8 @@ let debug = require('internal/util/debuglog').debuglog('http', (fn) => {
 const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/;
 const kError = Symbol('kError');
 const kPath = Symbol('kPath');
+const kAuthority = Symbol('kAuthority');
+const kProxyRewrittenToAbsolute = Symbol('kProxyRewrittenToAbsolute');
 
 const HTTP_CLIENT_TRACE_EVENT_NAME = 'http.client.request';
 
@@ -318,6 +320,7 @@ function rewriteForProxiedHttp(req, reqOptions, proxyAuthority, userHostHeader,
   if (requestURL !== undefined) {
     // Convert the path to absolute-form. The authority is built from options.
     req.path = requestURL.href;
+    req[kProxyRewrittenToAbsolute] = true;
   }
   debug(`updated request for HTTP proxy ${href} with ${req.path} `, req[kOutHeaders]);
   return true;
@@ -479,6 +482,7 @@ function ClientRequest(input, options, cb) {
   this.reusedSocket = false;
   this.host = host;
   this.protocol = protocol;
+  this[kProxyRewrittenToAbsolute] = false;
 
   if (this.agent) {
     // If there is an agent we should default to Connection:keep-alive,
@@ -509,6 +513,9 @@ function ClientRequest(input, options, cb) {
   if (port && +port !== defaultPort) {
     hostHeaderFromOptions += ':' + port;
   }
+  // Preserve the request authority (with the port when non-default) so that
+  // the perf_hooks entry can report a faithful URL.
+  this[kAuthority] = hostHeaderFromOptions;
   const headersArray = ArrayIsArray(options.headers);
   if (!headersArray) {
     if (options.headers) {
@@ -627,7 +634,11 @@ ClientRequest.prototype._finish = function _finish() {
       detail: {
         req: {
           method: this.method,
-          url: `${this.protocol}//${this.host}${this.path}`,
+          // If the path has been rewritten to absolute-form for proxying,
+          // it is already a full URL.
+          url: this[kProxyRewrittenToAbsolute] ?
+            this.path :
+            `${this.protocol}//${this[kAuthority]}${this.path}`,
           headers: typeof this.getHeaders === 'function' ? this.getHeaders() : {},
         },
       },

test/client-proxy/test-http-proxy-perf-hooks.mjs

@@ -0,0 +1,63 @@
+// This tests that when a request path is rewritten to absolute-form for
+// proxying, the perf_hooks HTTP entries report it as the URL as-is, instead
+// of appending it to the protocol and authority again.
+// Refs: https://github.com/nodejs/node/issues/59625
+import * as common from '../common/index.mjs';
+import assert from 'node:assert';
+import { once } from 'events';
+import http from 'node:http';
+import { PerformanceObserver } from 'node:perf_hooks';
+import { createProxyServer } from '../common/proxy-server.js';
+
+const entries = [];
+const obs = new PerformanceObserver(common.mustCallAtLeast((items) => {
+  entries.push(...items.getEntries());
+}));
+obs.observe({ type: 'http' });
+
+// Start a server to process the final request.
+const server = http.createServer(common.mustCall((req, res) => {
+  res.end('Hello world');
+}));
+server.on('error', common.mustNotCall((err) => { console.error('Server error', err); }));
+server.listen(0);
+await once(server, 'listening');
+
+// Start a minimal proxy server.
+const { proxy, logs } = createProxyServer();
+proxy.listen(0);
+await once(proxy, 'listening');
+
+const requestUrl = `http://localhost:${server.address().port}/test`;
+const agent = new http.Agent({
+  proxyEnv: {
+    HTTP_PROXY: `http://localhost:${proxy.address().port}`,
+  },
+});
+
+const res = await new Promise((resolve, reject) => {
+  http.request(requestUrl, { agent }, resolve).on('error', reject).end();
+});
+res.resume();
+await once(res, 'end');
+
+// Verify that the request went through the proxy.
+assert.strictEqual(logs.length, 1);
+assert.strictEqual(logs[0].url, requestUrl);
+
+proxy.close();
+server.close();
+
+process.on('exit', () => {
+  // Two HttpClient entries are expected: one for the proxied request, one for
+  // the request the proxy makes to forward it. Both should report the full
+  // URL, including the port.
+  const clientUrls = entries.filter((entry) => entry.name === 'HttpClient')
+    .map((entry) => entry.detail.req.url);
+  assert.deepStrictEqual(clientUrls, [requestUrl, requestUrl]);
+  // Two HttpRequest entries are expected: the proxy server receives the
+  // request target in absolute-form, the final server in origin-form.
+  const requestUrls = entries.filter((entry) => entry.name === 'HttpRequest')
+    .map((entry) => entry.detail.req.url).sort();
+  assert.deepStrictEqual(requestUrls, ['/test', requestUrl].sort());
+});

test/parallel/test-http-perf_hooks.js

@@ -36,16 +36,18 @@ const server = http.Server(common.mustCall((req, res) => {
   }));
 }, 2));
 
+let port;
 server.listen(0, common.mustCall(async () => {
+  port = server.address().port;
   await Promise.all([
     makeRequest({
-      port: server.address().port,
+      port,
       path: '/',
       method: 'POST',
       data: expected
     }),
     makeRequest({
-      port: server.address().port,
+      port,
       path: '/',
       method: 'POST',
       data: expected
@@ -63,14 +65,17 @@ process.on('exit', () => {
     assert.strictEqual(typeof entry.duration, 'number');
     if (entry.name === 'HttpClient') {
       numberOfHttpClients++;
+      // The reported URL must include the port when it is non-default.
+      // Refs: https://github.com/nodejs/node/issues/59625
+      assert.strictEqual(entry.detail.req.url, `http://localhost:${port}/`);
     } else if (entry.name === 'HttpRequest') {
       numberOfHttpRequests++;
+      assert.strictEqual(entry.detail.req.url, '/');
     }
-    assert.strictEqual(typeof entry.detail.req.method, 'string');
-    assert.strictEqual(typeof entry.detail.req.url, 'string');
+    assert.strictEqual(entry.detail.req.method, 'POST');
     assert.strictEqual(typeof entry.detail.req.headers, 'object');
-    assert.strictEqual(typeof entry.detail.res.statusCode, 'number');
-    assert.strictEqual(typeof entry.detail.res.statusMessage, 'string');
+    assert.strictEqual(entry.detail.res.statusCode, 200);
+    assert.strictEqual(entry.detail.res.statusMessage, 'OK');
     assert.strictEqual(typeof entry.detail.res.headers, 'object');
   }
   assert.strictEqual(numberOfHttpClients, 2);