diff --git a/scripts/dashboard.js b/scripts/dashboard.js index cea47aa1..4dbc937b 100644 --- a/scripts/dashboard.js +++ b/scripts/dashboard.js @@ -58,21 +58,14 @@ function readPid() { try { return Number.parseInt(fs.readFileSync(PID_FILE, 'utf8').trim(), 10); } catch (_) { return null; } // NOSONAR: missing or unreadable PID file simply means not running } -async function start() { - if (!fs.existsSync(SERVER_SCRIPT)) { - console.error('EGC Dashboard not found. Expected: ' + SERVER_SCRIPT); - process.exit(1); - } - - const PORT = getPort(); - - // Dependency gate shared with the init/install launcher: the list comes - // from dashboard/package.json (never drifts from the manifest) and a dep - // counts as present when require() can resolve it walking up from - // dashboard/. Only when something does not resolve is an on-demand - // install attempted, and only where it can succeed: inside a root-owned - // global prefix, npm install can only die with EACCES behind the - // detached spawn, which is how #1233 was hit. +// Dependency gate shared with the init/install launcher: the list comes +// from dashboard/package.json (never drifts from the manifest) and a dep +// counts as present when require() can resolve it walking up from +// dashboard/. Only when something does not resolve is an on-demand +// install attempted, and only where it can succeed: inside a root-owned +// global prefix, npm install can only die with EACCES behind the +// detached spawn, which is how #1233 was hit. +function ensureDashboardDeps() { const { checkDashboardDeps } = require(path.join(__dirname, 'lib', 'dashboard-deps')); const depsReport = checkDashboardDeps(DASHBOARD_DIR); if (depsReport.manifestError) { @@ -82,22 +75,30 @@ async function start() { console.error('The installed package looks damaged. Reinstall with: npm install -g @egchq/egc'); process.exit(1); } - if (depsReport.missing.length > 0) { - let writable = true; - try { fs.accessSync(DASHBOARD_DIR, fs.constants.W_OK); } catch (_) { writable = false; } // NOSONAR: probe only - if (!writable) { - console.error('Dashboard dependencies are missing and the package directory is not writable:'); - console.error(' ' + DASHBOARD_DIR); - console.error('Update EGC to a release that ships them preinstalled, or install them once with:'); - console.error(' sudo npm --prefix "' + DASHBOARD_DIR + '" install'); - process.exit(1); - } - console.log('Installing dashboard dependencies...'); - const r = spawnSync(process.platform === 'win32' ? 'npm.cmd' : 'npm', ['install'], { - cwd: DASHBOARD_DIR, stdio: 'inherit', - }); - if (r.status !== 0) { console.error('npm install failed.'); process.exit(1); } + if (depsReport.missing.length === 0) return; + if (!depsReport.writable) { + console.error('Dashboard dependencies are missing and the package directory is not writable:'); + console.error(' ' + DASHBOARD_DIR); + console.error('Update EGC to a release that ships them preinstalled, or install them once with:'); + console.error(' sudo npm --prefix "' + DASHBOARD_DIR + '" install'); + process.exit(1); } + console.log('Installing dashboard dependencies...'); + const r = spawnSync(process.platform === 'win32' ? 'npm.cmd' : 'npm', ['install'], { + cwd: DASHBOARD_DIR, stdio: 'inherit', + }); + if (r.status !== 0) { console.error('npm install failed.'); process.exit(1); } +} + +async function start() { + if (!fs.existsSync(SERVER_SCRIPT)) { + console.error('EGC Dashboard not found. Expected: ' + SERVER_SCRIPT); + process.exit(1); + } + + const PORT = getPort(); + + ensureDashboardDeps(); const already = await isRunning(); if (already) { diff --git a/scripts/lib/dashboard-deps.js b/scripts/lib/dashboard-deps.js index bdc6f970..6dff7fb9 100644 --- a/scripts/lib/dashboard-deps.js +++ b/scripts/lib/dashboard-deps.js @@ -10,17 +10,23 @@ const path = require('node:path'); // dashboard/node_modules (git checkout with a local install) or in the // package root's node_modules (global npm install ships them at the root). function checkDashboardDeps(dashboardDir) { + // Whether an on-demand `npm install` inside dashboardDir could even + // succeed. In a root-owned global prefix it cannot (#1233), and callers + // use this to pick between a long install budget and a fast refusal. + let writable = true; + try { fs.accessSync(dashboardDir, fs.constants.W_OK); } catch (_) { writable = false; } // NOSONAR: probe only + let deps; try { const manifest = JSON.parse(fs.readFileSync(path.join(dashboardDir, 'package.json'), 'utf8')); deps = Object.keys(manifest.dependencies || {}); } catch (err) { - return { deps: [], missing: [], manifestError: err }; + return { deps: [], missing: [], manifestError: err, writable }; } const missing = deps.filter(dep => { try { require.resolve(dep, { paths: [dashboardDir] }); return false; } catch (_) { return true; } // NOSONAR: an unresolved dep is the datum being collected }); - return { deps, missing, manifestError: null }; + return { deps, missing, manifestError: null, writable }; } module.exports = { checkDashboardDeps }; diff --git a/scripts/lib/dashboard-launch.js b/scripts/lib/dashboard-launch.js index 095675d7..c76e69b0 100644 --- a/scripts/lib/dashboard-launch.js +++ b/scripts/lib/dashboard-launch.js @@ -78,13 +78,16 @@ function launchDashboard({ log = () => {} } = {}) { // startup (missing deps in a root-owned prefix, #1233) leaves the // success line as the last word while the port refuses connections. // The budget covers the child's own path: when every dashboard dep - // already resolves the server answers in well under 4s, but a first - // launch in a writable checkout may run a full npm install first, so + // already resolves the server answers in well under 4s, and a first + // launch in a WRITABLE checkout may run a full npm install first, so // announcing failure at 4s there would be a false verdict (PR #1234 - // review). + // review). When deps are missing but the directory is not writable + // (the exact #1233 scenario) no install can run at all: the child + // refuses within a second, so the long budget would only delay the + // honest failure line by a minute (post-merge review of #1234). const { checkDashboardDeps } = require(path.join(__dirname, 'dashboard-deps')); const depsReport = checkDashboardDeps(path.join(__dirname, '..', '..', 'dashboard')); - const installAhead = depsReport.manifestError !== null || depsReport.missing.length > 0; + const installAhead = depsReport.missing.length > 0 && depsReport.writable && depsReport.manifestError === null; if (installAhead) log('First launch may install dashboard dependencies; giving it up to a minute.'); const budgetMs = installAhead ? 60000 : 4000; return waitForDashboard(budgetMs).then(ready => { diff --git a/tests/scripts/dashboard-launch.test.js b/tests/scripts/dashboard-launch.test.js index 7a565a32..5a3b1a7d 100644 --- a/tests/scripts/dashboard-launch.test.js +++ b/tests/scripts/dashboard-launch.test.js @@ -64,29 +64,36 @@ async function runTests() { if (test('both the CLI and the launcher gate through the shared dependency helper', () => { const launcherSource = fs.readFileSync(LAUNCHER, 'utf8'); const cliSource = fs.readFileSync(DASHBOARD_CLI, 'utf8'); + const helperSource = fs.readFileSync(DEPS_HELPER, 'utf8'); // One source of truth: the dependency list comes from // dashboard/package.json via scripts/lib/dashboard-deps.js, never a - // hardcoded array that can drift from the manifest. + // hardcoded array that can drift from the manifest, and the + // writability probe lives in the helper so both callers see it. assert.ok(cliSource.includes("require(path.join(__dirname, 'lib', 'dashboard-deps'))"), 'dashboard.js must use the shared deps helper'); assert.ok(launcherSource.includes("require(path.join(__dirname, 'dashboard-deps'))"), 'dashboard-launch.js must use the shared deps helper'); assert.ok(!/\[\s*'express'\s*,\s*'ws'\s*\]/.test(cliSource), 'no hardcoded dependency list may remain'); - assert.ok(/W_OK/.test(cliSource), 'the on-demand install must be gated on the directory being writable'); + assert.ok(/W_OK/.test(helperSource), 'the writability probe must live in the shared helper'); assert.ok(cliSource.includes('not writable'), 'the unwritable case must be reported honestly'); assert.ok(cliSource.includes('manifest missing or unreadable'), 'a broken manifest must produce a clear message, not a raw stack trace'); })) passed++; else failed++; - if (test('launcher polls for readiness with a budget that covers a first-time dependency install', () => { + if (test('launcher widens the poll budget only when an install can actually run', () => { const source = fs.readFileSync(LAUNCHER, 'utf8'); assert.ok(/waitForDashboard\(/.test(source), 'launcher must poll the server after the detached spawn'); assert.ok(source.includes('did not respond within'), 'a dead server must be reported without claiming a false verdict'); assert.ok(source.includes('See the startup error with:'), 'the failure line must point at the foreground command'); assert.ok(!/setTimeout\(openBrowser/.test(source), 'the browser must only open after readiness, not on a blind timer'); - assert.ok(/60000/.test(source) && /4000/.test(source), 'the poll budget must widen when an on-demand install is ahead (PR #1234 review)'); + // The 60s budget exists for a real on-demand install (writable + // checkout). In the unwritable #1233 scenario the child refuses within + // a second, so the launcher must keep the short budget there instead + // of stalling a minute before the honest failure line. + assert.ok(/missing\.length > 0 && depsReport\.writable/.test(source), 'the long budget must require the directory to be writable'); + assert.ok(/60000/.test(source) && /4000/.test(source), 'both budgets must exist'); })) passed++; else failed++; - if (test('checkDashboardDeps reports resolvable, missing, and broken-manifest states', () => { + if (test('checkDashboardDeps reports resolvable, missing, broken-manifest, and writability states', () => { const { checkDashboardDeps } = require(DEPS_HELPER); const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'egc-dash-deps-')); @@ -95,6 +102,7 @@ async function runTests() { const broken = checkDashboardDeps(tmp); assert.ok(broken.manifestError, 'a missing manifest must surface as manifestError'); assert.deepStrictEqual(broken.deps, [], 'no deps can be known without a manifest'); + assert.strictEqual(broken.writable, true, 'a fresh tmpdir must probe as writable'); // Unresolvable dependency. fs.writeFileSync(path.join(tmp, 'package.json'), JSON.stringify({ @@ -108,41 +116,48 @@ async function runTests() { fs.writeFileSync(path.join(tmp, 'package.json'), JSON.stringify({ dependencies: {} })); const clean = checkDashboardDeps(tmp); assert.deepStrictEqual(clean.missing, []); + + // Unwritable directory (POSIX only: chmod cannot revoke directory + // write access on Windows, matching how install-sh.test.js skips; + // also skipped as root, which bypasses the W_OK probe entirely, so + // the case would fail in root-running Docker/CI). + if (process.platform !== 'win32' && typeof process.getuid === 'function' && process.getuid() !== 0) { + fs.chmodSync(tmp, 0o555); + try { + const readonly = checkDashboardDeps(tmp); + assert.strictEqual(readonly.writable, false, 'a read-only directory must probe as not writable'); + } finally { + fs.chmodSync(tmp, 0o755); + } + } } finally { fs.rmSync(tmp, { recursive: true, force: true }); } })) passed++; else failed++; - // Behavioral coverage of the poll itself (PR #1234 review asked for more - // than source scanning): run waitForDashboard against a throwaway HTTP - // server on a free port, then against the same port once it is closed. - const freePort = await new Promise(resolve => { - const probe = http.createServer(); - probe.listen(0, '127.0.0.1', () => { - const { port } = probe.address(); - probe.close(() => resolve(port)); - }); + // Behavioral coverage of the poll itself (PR #1234 review): the positive + // case binds port 0 and keeps that server ALIVE while waitForDashboard + // runs against it (no release-and-rebind race); the negative case then + // reuses the port only after the server is closed. + const server = http.createServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end('{"ts":0}'); }); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const livePort = server.address().port; - process.env.EGC_PORT = String(freePort); + process.env.EGC_PORT = String(livePort); delete require.cache[require.resolve(LAUNCHER)]; delete require.cache[require.resolve(path.join(__dirname, '..', '..', 'dashboard', 'port'))]; const { waitForDashboard } = require(LAUNCHER); - if (await asyncTest('waitForDashboard resolves true once the server answers /ping', async () => { - const server = http.createServer((req, res) => { - res.writeHead(200, { 'Content-Type': 'application/json' }); - res.end('{"ts":0}'); - }); - await new Promise(resolve => server.listen(freePort, '127.0.0.1', resolve)); - try { - assert.strictEqual(await waitForDashboard(2000), true); - } finally { - await new Promise(resolve => server.close(resolve)); - } + if (await asyncTest('waitForDashboard resolves true while the server answers /ping', async () => { + assert.strictEqual(await waitForDashboard(2000), true); })) passed++; else failed++; - if (await asyncTest('waitForDashboard resolves false when nothing listens within the budget', async () => { + await new Promise(resolve => server.close(resolve)); + + if (await asyncTest('waitForDashboard resolves false once nothing listens within the budget', async () => { const started = Date.now(); assert.strictEqual(await waitForDashboard(700), false); assert.ok(Date.now() - started >= 600, 'the poll must keep trying until the budget is spent');