diff --git a/bin/run.ts b/bin/run.ts index 6fb094521..a7ee56ef7 100644 --- a/bin/run.ts +++ b/bin/run.ts @@ -25,7 +25,7 @@ import * as keys from '../security/keys.ts'; import { startHTTPThreads } from '../server/threads/socketRouter.ts'; import * as hdbInfoController from '../dataLayer/hdbInfoController.ts'; import { isReadOnlyMode } from '../resources/databases.ts'; -import { getThisNodeName } from '../server/nodeName.ts'; +import { getThisNodeName, getThisNodeHostname } from '../server/nodeName.ts'; import * as hdbTerms from '../utility/hdbTerms.ts'; import { getHdbPid, isProcessRunning } from '../utility/processManagement/processManagement.js'; import { PACKAGE_ROOT } from '../utility/packageUtils.js'; @@ -321,17 +321,10 @@ function startupLog(portResolutions: any) { }`; logMsg += `, unix socket: ${configUtils.getConfigPath(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_DOMAINSOCKET)}\n`; if (env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_PORT)) { - logMsg += pad('') + 'http://' + getThisNodeName() + ':' + env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_PORT) + '/\n'; + logMsg += `${pad('')}http://${getThisNodeHostname()}:${env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_PORT)}/\n`; } if (env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT)) { - logMsg += - '\n' + - pad('') + - 'https://' + - getThisNodeName() + - ':' + - env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT) + - '/\n'; + logMsg += `\n${pad('')}https://${getThisNodeHostname()}:${env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT)}/\n`; } // MQTT Log @@ -383,7 +376,7 @@ function startupLog(portResolutions: any) { if (!restLog.includes(pair) && name === 'rest') { restLog += pair; if (value.protocol_name === 'HTTP' || value.protocol_name === 'HTTPS') { - restHostnames.push(`${value.protocol_name.toLowerCase()}://${getThisNodeName()}:${key}/`); + restHostnames.push(`${value.protocol_name.toLowerCase()}://${getThisNodeHostname()}:${key}/`); } } diff --git a/server/nodeName.ts b/server/nodeName.ts index f731ca7e9..228e790bd 100644 --- a/server/nodeName.ts +++ b/server/nodeName.ts @@ -58,6 +58,25 @@ export function clearThisNodeName() { nodeName = undefined; } +// node.hostname may be configured as a full URL; reduce it to a bare host so composing a +// display URL from it does not double-wrap the scheme and port. +export function nodeNameToDisplayHost(name: string): string { + if (!name) return name; + const hasScheme = /^[a-z][a-z0-9+.-]*:\/\//i.test(name); + const candidates = hasScheme ? [name] : [`http://${name}`, `http://[${name}]`]; + for (const candidate of candidates) { + try { + const { hostname } = new URL(candidate); + if (hostname) return hostname; + } catch {} + } + return name; +} + +export function getThisNodeHostname(): string { + return nodeNameToDisplayHost(getThisNodeName()); +} + function getHostFromListeningPort(key: string) { const port: string | undefined = env.get(key); const lastColon = port?.lastIndexOf?.(':'); diff --git a/unitTests/bin/startupBanner.test.js b/unitTests/bin/startupBanner.test.js new file mode 100644 index 000000000..5d74caf3f --- /dev/null +++ b/unitTests/bin/startupBanner.test.js @@ -0,0 +1,116 @@ +'use strict'; + +// End-to-end guard for the #2218 fix (double-wrapped startup URLs). The pure helper +// nodeNameToDisplayHost() is unit-tested in unitTests/server/nodeName.test.js, but nothing exercised +// startupLog() itself, so a call site that switched back from getThisNodeHostname() to +// getThisNodeName() would recreate the `http://http://host:port/` banner while that suite stayed +// green. This drives the real, exported startupLog() with a real (URL-valued) node.hostname and +// asserts the Operations-API HTTP/HTTPS and REST URL lines it prints are well-formed. + +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const assert = require('assert'); + +const env = require('#src/utility/environment/environmentManager'); +const { CONFIG_PARAMS } = require('#src/utility/hdbTerms'); +const { clearThisNodeName } = require('#src/server/nodeName'); +const { startupLog } = require('#src/bin/run'); + +describe('startupLog banner URLs (#2218 double-wrapped startup URLs)', () => { + const originalConsoleLog = console.log; + // initTestEnvironment() sets operationsApi_network_port=9925 and http_port=9926; the secure + // operations port is unset by default, so tests that need it set (and restore) it explicitly. Read + // the live ops port at assert time rather than caching it, so the expected URL always tracks the + // same config value startupLog() reads (no load-time-vs-run-time skew if another suite mutates it). + const opsPort = () => env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_PORT); + + let originalNodeHostname; + let originalSecurePort; + + beforeEach(() => { + originalNodeHostname = env.get(CONFIG_PARAMS.NODE_HOSTNAME); + originalSecurePort = env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT); + }); + + afterEach(() => { + console.log = originalConsoleLog; + // Restore (not blank) the config this suite mutated so it can't leak into other suites in + // the same mocha process. + env.setProperty(CONFIG_PARAMS.NODE_HOSTNAME, originalNodeHostname); + env.setProperty(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT, originalSecurePort); + clearThisNodeName(); + }); + + // Runs startupLog() with node.hostname set to `hostname`, captures the emitted banner, and + // returns it as a single string. + function bannerFor(hostname, portResolutions = new Map()) { + env.setProperty(CONFIG_PARAMS.NODE_HOSTNAME, hostname); + clearThisNodeName(); // getThisNodeName() memoizes; drop any value cached by an earlier test + + const lines = []; + console.log = (...args) => lines.push(args.join(' ')); + try { + startupLog(portResolutions); + } finally { + console.log = originalConsoleLog; + } + return lines.join('\n'); + } + + it('composes well-formed ops + REST URLs when node.hostname is a full URL', () => { + // A URL-valued node.hostname is exactly the #2218 trigger: getThisNodeName() returns the whole + // URL, so the pre-fix banner produced `http://http://localhost:9926:9925/`. + const restPort = 9926; + const portResolutions = new Map([[restPort, [{ name: 'rest', protocol_name: 'HTTP' }]]]); + + const banner = bannerFor('http://localhost:9926', portResolutions); + + assert.ok(!banner.includes('http://http://'), `banner double-wrapped a scheme:\n${banner}`); + assert.ok( + banner.includes(`http://localhost:${opsPort()}/`), + `expected the Operations-API URL http://localhost:${opsPort()}/ in banner:\n${banner}` + ); + assert.ok( + banner.includes(`http://localhost:${restPort}/`), + `expected the REST URL http://localhost:${restPort}/ in banner:\n${banner}` + ); + }); + + it('composes well-formed HTTPS ops + REST URLs when node.hostname is a full URL', () => { + const securePort = 9935; + const restPort = 9927; + env.setProperty(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT, securePort); + const portResolutions = new Map([[restPort, [{ name: 'rest', protocol_name: 'HTTPS' }]]]); + + const banner = bannerFor('https://localhost:9926', portResolutions); + + assert.ok(!banner.includes('https://https://'), `banner double-wrapped a scheme:\n${banner}`); + assert.ok( + banner.includes(`https://localhost:${securePort}/`), + `expected the Operations-API URL https://localhost:${securePort}/ in banner:\n${banner}` + ); + assert.ok( + banner.includes(`https://localhost:${restPort}/`), + `expected the REST URL https://localhost:${restPort}/ in banner:\n${banner}` + ); + }); + + it('leaves a plain bare-host node.hostname unchanged in composed URLs', () => { + // The complement of the URL case: normalization must not corrupt a hostname that was already + // bare (over-stripping would be just as wrong as double-wrapping). + const restPort = 9926; + const portResolutions = new Map([[restPort, [{ name: 'rest', protocol_name: 'HTTP' }]]]); + + const banner = bannerFor('node-1.example.com', portResolutions); + + assert.ok( + banner.includes(`http://node-1.example.com:${opsPort()}/`), + `expected the Operations-API URL http://node-1.example.com:${opsPort()}/ in banner:\n${banner}` + ); + assert.ok( + banner.includes(`http://node-1.example.com:${restPort}/`), + `expected the REST URL http://node-1.example.com:${restPort}/ in banner:\n${banner}` + ); + }); +}); diff --git a/unitTests/server/nodeName.test.js b/unitTests/server/nodeName.test.js index 900bf1063..faeceffa7 100644 --- a/unitTests/server/nodeName.test.js +++ b/unitTests/server/nodeName.test.js @@ -8,7 +8,13 @@ const sinon = require('sinon'); const env = require('#src/utility/environment/environmentManager'); const { logger } = require('#src/utility/logging/logger'); -const { hostnameToUrl, getThisNodeName, clearThisNodeName } = require('#src/server/nodeName'); +const { + hostnameToUrl, + getThisNodeName, + getThisNodeHostname, + nodeNameToDisplayHost, + clearThisNodeName, +} = require('#src/server/nodeName'); describe('getThisNodeName precedence (harper-pro#351)', () => { let sandbox; @@ -78,6 +84,54 @@ describe('getThisNodeName precedence (harper-pro#351)', () => { }); }); +describe('nodeNameToDisplayHost (#2218 double-wrapped startup URLs)', () => { + it('returns a bare host unchanged', () => { + assert.strictEqual(nodeNameToDisplayHost('localhost'), 'localhost'); + }); + + it('strips the scheme and port when node.hostname is a full URL', () => { + assert.strictEqual(nodeNameToDisplayHost('http://localhost:9926'), 'localhost'); + }); + + it('strips a bare host:port down to the host', () => { + assert.strictEqual(nodeNameToDisplayHost('localhost:9926'), 'localhost'); + }); + + it('preserves an already-bracketed IPv6 host', () => { + assert.strictEqual(nodeNameToDisplayHost('https://[::1]:9926'), '[::1]'); + }); + + it('brackets a bare IPv6 literal so composed URLs stay valid', () => { + assert.strictEqual(nodeNameToDisplayHost('::1'), '[::1]'); + }); + + it('returns an unparseable value unchanged rather than dropping it', () => { + assert.strictEqual(nodeNameToDisplayHost('node with space'), 'node with space'); + }); +}); + +describe('getThisNodeHostname reads and normalizes the configured node.hostname', () => { + let originalNodeHostname; + + beforeEach(() => { + originalNodeHostname = env.get('node_hostname'); + clearThisNodeName(); + }); + + afterEach(() => { + env.setProperty('node_hostname', originalNodeHostname); + clearThisNodeName(); + }); + + // Guards the wiring bin/run.ts depends on: the wrapper must normalize the resolved node name, + // not return it raw. + it('normalizes a URL-valued node.hostname to a bare host', () => { + env.setProperty('node_hostname', 'http://localhost:9926'); + clearThisNodeName(); + assert.strictEqual(getThisNodeHostname(), 'localhost'); + }); +}); + describe('hostnameToUrl', () => { let sandbox;