diff --git a/changelog.d/1057-windows-npm-dep0190.md b/changelog.d/1057-windows-npm-dep0190.md new file mode 100644 index 000000000..1e8e1da55 --- /dev/null +++ b/changelog.d/1057-windows-npm-dep0190.md @@ -0,0 +1,4 @@ +- On Windows, `npm run doctor` and the Pinokio install no longer print Node's + DEP0190 warning ("Passing args to a child process with shell option true can + lead to security vulnerabilities"): npm.cmd's fixed subcommand is now part of + the shell command string instead of an argument array (quanticsoul4772, #1057). diff --git a/changelog.d/1117-google-viewer-lifecycle.md b/changelog.d/1117-google-viewer-lifecycle.md new file mode 100644 index 000000000..66d73d4a2 --- /dev/null +++ b/changelog.d/1117-google-viewer-lifecycle.md @@ -0,0 +1,3 @@ +- Release superseded Google Street View open waits and suppress metadata + requests whose viewer was unmounted while the service loaded (rudycelekli, + #1117). diff --git a/changelog.d/1172-cctv-missing-numbers.md b/changelog.d/1172-cctv-missing-numbers.md new file mode 100644 index 000000000..d401021e9 --- /dev/null +++ b/changelog.d/1172-cctv-missing-numbers.md @@ -0,0 +1,13 @@ +- Keep missing CCTV numbers unknown instead of reading them as zero. `Number` + turns null, blank strings and booleans into 0 or 1, and the camera source + list sends an unknown number as JSON null, so a source-pack camera that left + out its pose reached the browser as zeros clamped to each field's floor — + facing north, pitch −2°, FOV 20°, range 120 m, mount 6 m and ground 0 m — + instead of the defaults (id-hash heading, −17°, 74°, 700 m, 24 m and the + city's ground); a null or blank position placed the camera at 0,0. The + server's `toFiniteNumber` and the browser's `safeNumber` now treat absent, + blank and boolean values as missing. The same conversion also stops a + Tallinn row with a null heading reading as a confident north-facing camera, + a Caltrans camera with a blank elevation losing its 150 m ground prior, and + a TfL or Catalonia camera with a null or blank coordinate being kept at 0,0 + or on the zero meridian (omerbberk, #1172). diff --git a/docs/CURRENT-STATE.md b/docs/CURRENT-STATE.md index 394ae6aaf..7f15631eb 100644 --- a/docs/CURRENT-STATE.md +++ b/docs/CURRENT-STATE.md @@ -1352,6 +1352,19 @@ Node-only package entries expose both provider factories. Routes, payloads, fallback behavior and existing dev/preview hook registration are preserved; browser rendering is unchanged. +## CCTV missing numeric fields + +CCTV numbers are read as missing when they are absent, null, blank or boolean, +on the server (`toFiniteNumber`, every pack and the file/env source packs) and +in the browser (`safeNumber`). `/api/cctv/sources` sends an unknown number as +JSON null; the browser then uses the bundled seed camera's value when one +shares the id, else the field's default — id-hash heading, pitch −17°, FOV 74° +(which a matched datasheet can still replace), range 700 m, mount 24 m, the +city's ground elevation — rather than reading zero and clamping it to the +field's floor. A camera with a missing latitude or longitude and no seed +position is skipped by the browser catalog instead of being placed at 0,0. +Real zeros, including `"0"`, are kept. + ## Terrain, traffic, fire and bike-share provider modules Local composition now imports separate Node modules for Re:Earth heights, @@ -3469,7 +3482,10 @@ controls (the address goes in the caption); a pose goes out at once and again when the panorama's month, photographer and address arrive (one successful lookup per panorama per session, seeded by the nearest lookup; a details lookup that fails or takes over ten seconds waits five seconds -before a view event can retry, and an unmount drops lookups in flight). A key +before a view event can retry, and an unmount drops lookups in flight). Opening +another panorama settles the previous open before registering its wait; closing +then releases the current wait immediately. A details lookup released while its +service loads makes no panorama request after that service becomes available. A key Google refuses (`gm_authFailure`: the API not enabled, a referrer it does not allow, billing off) reads KEY REJECTED and fails a pending open at once; a lookup or open Google never answers fails after 10 or 20 s. In CI diff --git a/scripts/format-scope.json b/scripts/format-scope.json index a2b3e01f8..8bcc4c04d 100644 --- a/scripts/format-scope.json +++ b/scripts/format-scope.json @@ -117,6 +117,7 @@ "server/providers/cctv/constants.js", "server/providers/cctv/media.js", "server/providers/cctv/normalize.js", + "src/data/cctvMissingNumbers.test.mjs", "server/providers/cctv/sources.js", "server/providers/radio.js", "server/providers/radio/catalog.js", diff --git a/scripts/pinokio-install.mjs b/scripts/pinokio-install.mjs index 01a26ccce..4e52bf3af 100644 --- a/scripts/pinokio-install.mjs +++ b/scripts/pinokio-install.mjs @@ -24,8 +24,8 @@ export function runChecked(command, args, { shell = false } = {}) { export function installPinokioDependencies() { applyPinokioEnvironment(); rmSync(READY_FILE, { force: true }); - const npm = npmProcessSpec(); - runChecked(npm.command, ['ci'], { shell: npm.shell }); + const npm = npmProcessSpec(['ci']); + runChecked(npm.command, npm.args, { shell: npm.shell }); // Pinokio starts Vite directly and loads only its ENVIRONMENT file plus the // normal dotenv ladder. Unlike dev-fresh.sh, it does not import macOS diff --git a/scripts/setup-doctor.mjs b/scripts/setup-doctor.mjs index 49c6722c2..01e048091 100644 --- a/scripts/setup-doctor.mjs +++ b/scripts/setup-doctor.mjs @@ -76,10 +76,15 @@ export function hasRequiredDependencies(rootDir = ROOT) { } } -/** Return the npm command and spawn mode required by the target platform. */ -export function npmProcessSpec(platform = process.platform) { - const windows = platform === 'win32'; - return { command: windows ? 'npm.cmd' : 'npm', shell: windows }; +/** + * Return the npm command, arguments and spawn mode for the target platform. + * Windows must run npm.cmd through a shell, and Node 24 deprecates passing an + * argument array with `shell: true` (DEP0190), so there the arguments, which + * are fixed npm subcommands, join the command string. + */ +export function npmProcessSpec(args, platform = process.platform) { + if (platform !== 'win32') return { command: 'npm', args, shell: false }; + return { command: ['npm.cmd', ...args].join(' '), args: [], shell: true }; } /** Read one key from Vite's dotenv file ladder without depending on Vite. */ @@ -190,8 +195,8 @@ export function buildCapabilitySummary( export function inspectSetup({ includeKeychain = true, authoritativeEnvironment = false, rootDir = ROOT } = {}) { const node = classifyNodeVersion(); - const npm = npmProcessSpec(); - const npmResult = spawnSync(npm.command, ['--version'], { + const npm = npmProcessSpec(['--version']); + const npmResult = spawnSync(npm.command, npm.args, { encoding: 'utf8', shell: npm.shell, }); diff --git a/server/providers/cctv/normalize.js b/server/providers/cctv/normalize.js index 6c54622e9..2d4e16e11 100644 --- a/server/providers/cctv/normalize.js +++ b/server/providers/cctv/normalize.js @@ -39,11 +39,21 @@ export function escapeXml(text) { /** * Coerce a value to a finite number, returning fallback if NaN/Infinity. * + * Absent, blank and boolean values take the fallback too: `Number` reads + * null, '' and false as 0, which would turn a missing coordinate, heading or + * elevation into a real zero. + * * @param {*} value * @param {number} [fallback=NaN] * @returns {number} */ export function toFiniteNumber(value, fallback = NaN) { + if ( + value == null || + typeof value === 'boolean' || + (typeof value === 'string' && !value.trim()) + ) + return fallback; const num = Number(value); return Number.isFinite(num) ? num : fallback; } diff --git a/src/data/cctvMissingNumbers.test.mjs b/src/data/cctvMissingNumbers.test.mjs new file mode 100644 index 000000000..68911110f --- /dev/null +++ b/src/data/cctvMissingNumbers.test.mjs @@ -0,0 +1,252 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { + fallbackHeadingFromId, + isLikelyCataloniaCoordinate, + toFiniteNumber, +} from '../../server/providers/cctv/normalize.js'; +import { + loadCaltransSourcesFromOpenData, + loadTallinnSourcesFromCatalog, + loadTflSourcesFromOpenData, + parseCataloniaXml, +} from '../../server/providers/cctv/sources.js'; +import { cctvProxy } from '../../server/providers/cctv.js'; +import { createCatalog } from '../layers/cctv/catalog.js'; +import { createModel } from '../layers/cctv/model.js'; + +/** Values `Number` reads as 0 (or 1) although they carry no number. */ +const ABSENT = [null, undefined, '', ' ', false, true]; + +/** Set env vars for one test and restore them afterwards. */ +function withEnv(t, env) { + for (const [name, value] of Object.entries(env)) { + const previous = process.env[name]; + t.after(() => { + if (previous === undefined) delete process.env[name]; + else process.env[name] = previous; + }); + if (value === undefined) delete process.env[name]; + else process.env[name] = value; + } +} + +/** Silence the loaders' progress and failure logging. */ +function quiet(t) { + t.mock.method(console, 'log', () => {}); + t.mock.method(console, 'warn', () => {}); +} + +/** The production browser model; only the catalog's pose pass is stubbed. */ +function browserModel() { + const parts = {}; + parts.model = createModel({ + state: {}, + services: { focus: {} }, + parts, + source: {}, + }); + return parts.model; +} + +/** The browser catalog over the production model. */ +function browserCatalog(model, cityPois = {}) { + return createCatalog({ + state: {}, + services: { locations: { CITY_POIS: cityPois } }, + parts: { model: { ...model, ensureCameraPose: () => {} } }, + source: {}, + }); +} + +/** GET /api/cctv/sources through the real proxy middleware. */ +async function requestSources(plugin) { + let handler; + plugin.configureServer({ + middlewares: { + use(_route, fn) { + handler = fn; + }, + }, + }); + const res = { + writeHead(status, headers) { + Object.assign(this, { status, headers }); + }, + end(body) { + this.body = body; + }, + }; + await handler({ url: '/sources', method: 'GET' }, res); + assert.equal(res.status, 200); + return JSON.parse(res.body); +} + +test('absent, blank and boolean values take the fallback on both sides of the source list', () => { + const { safeNumber } = browserModel(); + for (const value of ABSENT) { + assert.ok(Number.isNaN(toFiniteNumber(value)), `server ${String(value)}`); + assert.equal(toFiniteNumber(value, 15), 15, `server ${String(value)}`); + assert.ok(Number.isNaN(safeNumber(value)), `browser ${String(value)}`); + assert.equal(safeNumber(value, 74), 74, `browser ${String(value)}`); + } + for (const [value, expected] of [ + [0, 0], + ['0', 0], + [-17, -17], + [' 12.5 ', 12.5], + ]) { + assert.equal(toFiniteNumber(value, 99), expected); + assert.equal(safeNumber(value, 99), expected); + } +}); + +test('a source-pack camera that omits or nulls its pose gets the browser defaults, not the clamp floors', async (t) => { + quiet(t); + const pose = { + headingDeg: null, + pitchDeg: null, + fovDeg: null, + rangeM: null, + mountHeightM: null, + groundElevationM: null, + }; + const at = { cityId: 'austin', lat: 30.2672, lon: -97.7431 }; + withEnv(t, { + CCTV_SOURCES_FILE: path.join(tmpdir(), 'gev-no-such-pack.json'), + CCTV_SOURCES_JSON: JSON.stringify([ + { id: 'pack-pose-omitted', ...at, feedType: 'image' }, + { id: 'pack-pose-null', ...at, ...pose, feedType: 'image' }, + { id: 'pack-coords-null', lat: null, lon: null, feedType: 'image' }, + { id: 'pack-coords-blank', lat: '', lon: ' ', feedType: 'image' }, + ]), + }); + + const body = await requestSources(cctvProxy({ sourceRoot: tmpdir() })); + const model = browserModel(); + const cameras = browserCatalog(model, { + austin: { name: 'Austin', groundElevation: 150 }, + }).buildCatalogFromSources(body.sources); + const byId = new Map(cameras.map((camera) => [camera.id, camera])); + + for (const id of ['pack-pose-omitted', 'pack-pose-null']) { + const camera = byId.get(id); + assert.ok(camera, id); + assert.equal(camera.headingDeg, model.headingFromId(id), id); + assert.equal(camera.pitchDeg, -17, id); + assert.equal(camera.fovDeg, 74, id); + assert.equal(camera.rangeM, 700, id); + assert.equal(camera.mountHeightM, 24, id); + assert.equal(camera.groundElevationM, 150, id); + } + // No position is not a camera in the Gulf of Guinea. + assert.equal(byId.has('pack-coords-null'), false); + assert.equal(byId.has('pack-coords-blank'), false); +}); + +test('Tallinn rows with a null heading or ground keep the low-confidence prior', (t) => { + quiet(t); + const root = mkdtempSync(path.join(tmpdir(), 'gev-tallinn-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const file = path.join(root, 'tallinn.json'); + writeFileSync( + file, + JSON.stringify([ + { + id: 'tln-null-heading', + lat: 59.437, + lon: 24.753, + headingDeg: null, + groundElevationM: null, + url: 'https://ristmikud.tallinn.ee/last/cam001.jpg', + }, + ]), + ); + withEnv(t, { CCTV_TALLINN_SOURCES_FILE: file }); + + const [camera] = loadTallinnSourcesFromCatalog({ sourceRoot: root }); + assert.equal(camera.headingConfidence, 'low'); + assert.equal(camera.headingDeg, fallbackHeadingFromId('tln-null-heading')); + assert.equal(camera.groundElevationM, 15); +}); + +test('TfL JamCams with null coordinates are dropped instead of placed at 0,0', async (t) => { + quiet(t); + const jamCam = (id, lat, lon) => ({ + id: `JamCams_${id}`, + commonName: `JamCam ${id}`, + lat, + lon, + additionalProperties: [ + { key: 'available', value: 'true' }, + { + key: 'imageUrl', + value: `https://s3-eu-west-1.amazonaws.com/jamcams.tfl.gov.uk/${id}.jpg`, + }, + ], + }); + t.mock.method(globalThis, 'fetch', async () => + Response.json([ + jamCam('00001.00001', 51.5072, -0.1276), + jamCam('00001.00002', null, null), + jamCam('00001.00003', 51.5072, ''), + ]), + ); + + const cameras = await loadTflSourcesFromOpenData(); + assert.deepEqual( + cameras.map((camera) => camera.id), + ['tfl-00001.00001'], + ); +}); + +test('a Catalonia feature with a blank longitude is not read as the zero meridian', () => { + const feature = (coordinates) => ` + + ${coordinates} + https://example.org/cam.jpg + + `; + const [blank, valid] = parseCataloniaXml( + feature(',41.5') + feature('2.17,41.39'), + ); + // 0 °E at 41.5 °N is inside the Catalonia box, so a zero would pass it. + assert.ok(Number.isNaN(blank.lon)); + assert.equal(isLikelyCataloniaCoordinate(blank.lat, blank.lon), false); + assert.equal(isLikelyCataloniaCoordinate(valid.lat, valid.lon), true); +}); + +test('a Caltrans camera with a blank elevation keeps the 150 m ground prior', async (t) => { + quiet(t); + withEnv(t, { CCTV_CALTRANS_DISTRICTS: '4' }); + t.mock.method(globalThis, 'fetch', async () => + Response.json({ + data: [ + { + cctv: { + inService: 'true', + location: { + latitude: '37.8044', + longitude: '-122.2712', + elevation: '', + locationName: 'TV101 -- I-880 : Test Ave', + }, + imageData: { + static: { + currentImageURL: + 'https://cwwp2.dot.ca.gov/data/d4/cctv/image/tv101/tv101.jpg', + }, + }, + }, + }, + ], + }), + ); + + const [camera] = await loadCaltransSourcesFromOpenData(); + assert.ok(camera); + assert.equal(camera.groundElevationM, 150); +}); diff --git a/src/layers/cctv/model.js b/src/layers/cctv/model.js index 48c8cddd4..bdf350b4e 100644 --- a/src/layers/cctv/model.js +++ b/src/layers/cctv/model.js @@ -97,12 +97,23 @@ export function createModel({ state: layerState, services, parts, source }) { /** * Coerces a value to a finite number or returns the fallback. + * + * Absent, blank and boolean values return the fallback as well. The source + * list serializes an unknown number (NaN) as JSON null, and `Number(null)` + * is 0, so without this an unknown pose field arrived as zero and was + * clamped to the field's floor instead of taking its default. * @param {*} value * @param {number} [fallback=NaN] * @returns {number} */ function safeNumber(value, fallback = NaN) { + if ( + value == null || + typeof value === 'boolean' || + (typeof value === 'string' && !value.trim()) + ) + return fallback; const n = Number(value); return Number.isFinite(n) ? n : fallback; } diff --git a/src/layers/streetLevel/providers/google/viewer.js b/src/layers/streetLevel/providers/google/viewer.js index 422f52c0c..25e0b08fe 100644 --- a/src/layers/streetLevel/providers/google/viewer.js +++ b/src/layers/streetLevel/providers/google/viewer.js @@ -97,6 +97,8 @@ export function createGoogleViewer({ loader, getService, render } = {}) { service ||= getService ? await getService() : new (await ensureLibrary()).StreetViewService(); + if (!current()) + throw new Error('Street View details lookup was released'); return service.getPanorama({ pano: panoId }); })().then(resolve, reject); }); @@ -236,6 +238,7 @@ export function createGoogleViewer({ loader, getService, render } = {}) { const panoId = String(imageId); if (!host) throw new Error('Street View is not mounted'); if (loader.authFailed()) throw new Error(KEY_REJECTED_MESSAGE); + cancelShown?.(); pendingOpen = panoId; const instance = await ensurePanorama(host); if (pendingOpen !== panoId) return; diff --git a/src/layers/streetLevel/providers/google/viewer.test.mjs b/src/layers/streetLevel/providers/google/viewer.test.mjs index a3d746a34..e1d4a187c 100644 --- a/src/layers/streetLevel/providers/google/viewer.test.mjs +++ b/src/layers/streetLevel/providers/google/viewer.test.mjs @@ -403,3 +403,69 @@ test('a remount looks up again a panorama whose lookup hung', async () => { await settle(); assert.equal(poses.length, count); }); + +test('superseded panorama waits cannot steal the current close cleanup', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout'] }); + const { OPEN_TIMEOUT_MS } = await import('./policy.js'); + const flush = async () => { + for (let i = 0; i < 10; i++) await Promise.resolve(); + }; + const { fake, loader, viewer, host } = setup(); + await viewer.mount(host); + const [panorama] = fake.built; + panorama.setPano = (id) => { + panorama.pano = id; + }; + const first = viewer.open('pano-a'); + await flush(); + const second = viewer.open('pano-b'); + await flush(); + panorama.fire('position_changed'); // new panorama has not reached OK + await flush(); + viewer.close(); + const remaining = loader.listenerCount(); + t.mock.timers.tick(OPEN_TIMEOUT_MS); + await Promise.allSettled([first, second]); + assert.equal( + remaining, + 0, + 'close disposes every pending open without waiting 20 seconds', + ); + assert.equal(panorama.visible, false); +}); + +test('unmount during details-service acquisition makes no later billed lookup', async () => { + const fake = fakeStreetViewLibrary({ panoramas: PANORAMAS }); + const loader = fakeMapsLoader(fake.library); + const { createGoogleProvider } = await import('./index.js'); + let release; + const acquiring = new Promise((resolve) => { + release = resolve; + }); + const importLibrary = loader.importLibrary.bind(loader); + let imports = 0; + loader.importLibrary = async (name) => { + if (++imports > 1) await acquiring; + return importLibrary(name); + }; + const provider = createGoogleProvider({ + getApiKey: () => 'fixture-key', + loader, + }); + const instance = provider.create({ + notify() {}, + actions: { reportError() {} }, + }); + const viewer = instance.viewer; + await viewer.mount(fakeHost()); + await viewer.open('pano-a'); + viewer.unmount(); + release(); + for (let i = 0; i < 10; i++) await Promise.resolve(); + assert.equal( + fake.lookups.length, + 0, + 'teardown invalidates the request before calling getPanorama', + ); + instance.destroy(); +}); diff --git a/src/setupDoctor.test.mjs b/src/setupDoctor.test.mjs index 71c6e44af..26d695ba1 100644 --- a/src/setupDoctor.test.mjs +++ b/src/setupDoctor.test.mjs @@ -69,12 +69,22 @@ test('placeholder values are never counted as configured credentials', () => { }); test('doctor selects a Windows-safe npm process without changing Unix behavior', () => { - assert.deepEqual(npmProcessSpec('win32'), { - command: 'npm.cmd', + // Windows passes no argument array with shell: true (Node DEP0190). + assert.deepEqual(npmProcessSpec(['ci'], 'win32'), { + command: 'npm.cmd ci', + args: [], shell: true, }); - assert.deepEqual(npmProcessSpec('darwin'), { command: 'npm', shell: false }); - assert.deepEqual(npmProcessSpec('linux'), { command: 'npm', shell: false }); + assert.deepEqual(npmProcessSpec(['--version'], 'darwin'), { + command: 'npm', + args: ['--version'], + shell: false, + }); + assert.deepEqual(npmProcessSpec(['ci'], 'linux'), { + command: 'npm', + args: ['ci'], + shell: false, + }); }); test('doctor recognizes every OpenSky OAuth keychain alias used by dev-fresh', () => {