From f627005c3bb69450bec102a57808e08c271cf5c3 Mon Sep 17 00:00:00 2001 From: Stefano Baghino Date: Sun, 5 Jul 2026 20:56:04 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/59625 --- lib/_http_client.js | 13 +++- .../test-http-proxy-perf-hooks.mjs | 63 +++++++++++++++++++ test/parallel/test-http-perf_hooks.js | 17 +++-- 3 files changed, 86 insertions(+), 7 deletions(-) create mode 100644 test/client-proxy/test-http-proxy-perf-hooks.mjs diff --git a/lib/_http_client.js b/lib/_http_client.js index 891da4e3f9a984..9eb3c10547d520 100644 --- a/lib/_http_client.js +++ b/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() : {}, }, }, diff --git a/test/client-proxy/test-http-proxy-perf-hooks.mjs b/test/client-proxy/test-http-proxy-perf-hooks.mjs new file mode 100644 index 00000000000000..4469bbf24831ce --- /dev/null +++ b/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()); +}); diff --git a/test/parallel/test-http-perf_hooks.js b/test/parallel/test-http-perf_hooks.js index 9acf54c5617bd8..674bb591f0cbf2 100644 --- a/test/parallel/test-http-perf_hooks.js +++ b/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);