From d19e0bf2e19c797a93acc76dc3e4ed25c3baa0c7 Mon Sep 17 00:00:00 2001 From: ashish Date: Mon, 27 Apr 2026 21:02:53 -0700 Subject: [PATCH 1/2] fix: path traversal guards, report-server robustness, Windows-portable clean scripts Bundles three CodeRabbit findings against main into one PR: Security (PR1 from the brief): - packages/cli/src/reportViewModel.ts: introduce safeResolveWithin and apply to runId resolution and artifact reads so traversal segments are rejected rather than dereferenced. Also classify ENOENT and traversal cases as a new RunManifestNotFoundError so the HTTP layer can map them to 404 only. - packages/cloud-core/src/submit.ts: validate suite/test relativePath via isSafeRelativeSegment and envName via a conservative regex before they flow into the upload archive's path joins. - packages/report-web/src/ui/components/SummaryCard.tsx: replace the iconSvg: string + dangerouslySetInnerHTML contract with icon: ReactNode. The three SummaryCard icons move to a new iconNodes.tsx as React elements; the unused string SVG constants are dropped from icons.ts. Robustness (PR2 from the brief): - packages/cli/src/reportServer.ts: switch SPA file streaming to await pipeline(...) from node:stream/promises so async stream errors surface instead of crashing the process. Map RunManifestNotFoundError to 404 and any other error to 500 so corrupt JSON / EACCES are no longer hidden behind a generic 404. Windows portability (PR3 from the brief): - Replace rm -rf in five package.json clean scripts with a Node one-liner using fs.rmSync so npm run build works on Windows cmd/PowerShell. Tests: - packages/cli/src/reportViewModel.test.ts: covers safeResolveWithin, RunManifestNotFoundError mapping for traversal/missing/ENOENT, and non-not-found errors for corrupt JSON / unsupported schema. - packages/cli/src/reportServer.test.ts: end-to-end checks for the /api/report/runs/:runId 404 (missing + traversal) vs 500 (corrupt) split. - packages/cloud-core/src/submit.test.ts: isSafeRelativeSegment and isSafeEnvName accept/reject cases including Windows-style separators. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/cli/package.json | 2 +- packages/cli/src/reportServer.test.ts | 71 ++++++++++ packages/cli/src/reportServer.ts | 26 +++- packages/cli/src/reportViewModel.test.ts | 125 ++++++++++++++++++ packages/cli/src/reportViewModel.ts | 41 +++++- packages/cloud-core/package.json | 2 +- packages/cloud-core/src/submit.test.ts | 43 ++++++ packages/cloud-core/src/submit.ts | 26 ++++ packages/common/package.json | 2 +- packages/device-node/package.json | 2 +- packages/goal-executor/package.json | 2 +- .../src/ui/components/SummaryCard.tsx | 13 +- packages/report-web/src/ui/iconNodes.tsx | 28 ++++ packages/report-web/src/ui/icons.ts | 13 +- .../report-web/src/ui/pages/RunIndexView.tsx | 18 ++- 15 files changed, 374 insertions(+), 40 deletions(-) create mode 100644 packages/cli/src/reportServer.test.ts create mode 100644 packages/cli/src/reportViewModel.test.ts create mode 100644 packages/cloud-core/src/submit.test.ts create mode 100644 packages/report-web/src/ui/iconNodes.tsx diff --git a/packages/cli/package.json b/packages/cli/package.json index 30f7471..18ed921 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -31,7 +31,7 @@ ], "scripts": { "build": "npm run clean && tsc && node ./scripts/copyReportApp.mjs && node -e \"require('fs').copyFileSync('package.json','dist/package.json')\"", - "clean": "rm -rf dist proto tsconfig.tsbuildinfo", + "clean": "node -e \"const fs=require('node:fs'); for (const p of ['dist','proto','tsconfig.tsbuildinfo']) fs.rmSync(p,{recursive:true,force:true});\"", "start": "tsx bin/finalrun.ts", "dev": "tsx --tsconfig ../../tsconfig.dev.json bin/finalrun.ts", "test": "node ./scripts/runTests.mjs" diff --git a/packages/cli/src/reportServer.test.ts b/packages/cli/src/reportServer.test.ts new file mode 100644 index 0000000..756c1e4 --- /dev/null +++ b/packages/cli/src/reportServer.test.ts @@ -0,0 +1,71 @@ +import assert from 'node:assert/strict'; +import * as fs from 'node:fs'; +import * as fsp from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import test from 'node:test'; +import { rebuildRunIndex } from './runIndex.js'; +import { serveReportWorkspace } from './reportServer.js'; + +async function withWorkspace( + body: (workspace: { workspaceRoot: string; artifactsDir: string }) => Promise, +): Promise { + const rootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'finalrun-report-server-')); + const artifactsDir = path.join(rootDir, 'artifacts'); + await fsp.mkdir(artifactsDir, { recursive: true }); + try { + return await body({ workspaceRoot: rootDir, artifactsDir }); + } finally { + fs.rmSync(rootDir, { recursive: true, force: true }); + } +} + +async function withServer( + workspace: { workspaceRoot: string; artifactsDir: string }, + body: (baseUrl: string) => Promise, +): Promise { + await rebuildRunIndex(workspace.artifactsDir); + const server = await serveReportWorkspace({ + workspaceRoot: workspace.workspaceRoot, + artifactsDir: workspace.artifactsDir, + port: 0, + }); + try { + return await body(server.url); + } finally { + await server.close(); + } +} + +test('GET /api/report/runs/:runId returns 404 when the run is missing', async () => { + await withWorkspace(async (workspace) => { + await withServer(workspace, async (baseUrl) => { + const response = await fetch(`${baseUrl}/api/report/runs/missing-run`); + assert.equal(response.status, 404); + const body = (await response.json()) as { status: string }; + assert.equal(body.status, 'error'); + }); + }); +}); + +test('GET /api/report/runs/:runId returns 404 for path-traversal runIds', async () => { + await withWorkspace(async (workspace) => { + await withServer(workspace, async (baseUrl) => { + const encoded = encodeURIComponent('../../../etc/passwd'); + const response = await fetch(`${baseUrl}/api/report/runs/${encoded}`); + assert.equal(response.status, 404); + }); + }); +}); + +test('GET /api/report/runs/:runId returns 500 for corrupt run.json', async () => { + await withWorkspace(async (workspace) => { + const runDir = path.join(workspace.artifactsDir, 'corrupt-run'); + await fsp.mkdir(runDir, { recursive: true }); + await fsp.writeFile(path.join(runDir, 'run.json'), 'this is not json', 'utf-8'); + await withServer(workspace, async (baseUrl) => { + const response = await fetch(`${baseUrl}/api/report/runs/corrupt-run`); + assert.equal(response.status, 500); + }); + }); +}); diff --git a/packages/cli/src/reportServer.ts b/packages/cli/src/reportServer.ts index 5a2086e..ac5d89b 100644 --- a/packages/cli/src/reportServer.ts +++ b/packages/cli/src/reportServer.ts @@ -12,9 +12,11 @@ import * as fs from 'node:fs'; import * as fsp from 'node:fs/promises'; import * as path from 'node:path'; import { createServer, type IncomingMessage, type ServerResponse } from 'node:http'; +import { pipeline } from 'node:stream/promises'; import { loadReportIndexViewModel, loadReportRunManifestViewModel, + RunManifestNotFoundError, type ReportWorkspaceContext, } from './reportViewModel.js'; import { decodeArtifactPath, serveArtifactHttp } from './reportArtifactStream.js'; @@ -71,10 +73,14 @@ export async function serveReportWorkspace(params: { try { writeJson(response, 200, await loadReportRunManifestViewModel(runId, context)); } catch (error) { - writeJson(response, 404, { - status: 'error', - message: error instanceof Error ? error.message : String(error), - }); + if (error instanceof RunManifestNotFoundError) { + writeJson(response, 404, { status: 'error', message: error.message }); + } else { + writeJson(response, 500, { + status: 'error', + message: error instanceof Error ? error.message : String(error), + }); + } } return; } @@ -192,7 +198,17 @@ async function tryServeFile( response.end(); return true; } - fs.createReadStream(filePath).pipe(response); + try { + await pipeline(fs.createReadStream(filePath), response); + } catch { + if (!response.headersSent) { + response.writeHead(500, { 'Content-Type': 'text/plain; charset=utf-8' }); + response.end('Failed to stream file.'); + } else { + response.destroy(); + } + return false; + } return true; } diff --git a/packages/cli/src/reportViewModel.test.ts b/packages/cli/src/reportViewModel.test.ts new file mode 100644 index 0000000..40e6f95 --- /dev/null +++ b/packages/cli/src/reportViewModel.test.ts @@ -0,0 +1,125 @@ +import assert from 'node:assert/strict'; +import * as fs from 'node:fs'; +import * as fsp from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import test from 'node:test'; +import { + RunManifestNotFoundError, + loadRunManifestRecord, + safeResolveWithin, + type ReportWorkspaceContext, +} from './reportViewModel.js'; + +function mkArtifactsDir(): { artifactsDir: string; cleanup: () => void } { + const artifactsDir = fs.mkdtempSync(path.join(os.tmpdir(), 'finalrun-report-vm-')); + return { + artifactsDir, + cleanup: () => { + fs.rmSync(artifactsDir, { recursive: true, force: true }); + }, + }; +} + +test('safeResolveWithin returns the resolved path for an in-bounds segment', () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + try { + const resolved = safeResolveWithin(artifactsDir, 'run-1', 'run.json'); + assert.equal(resolved, path.join(artifactsDir, 'run-1', 'run.json')); + } finally { + cleanup(); + } +}); + +test('safeResolveWithin returns the base when no extra segments are passed', () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + try { + const resolved = safeResolveWithin(artifactsDir); + assert.equal(resolved, path.resolve(artifactsDir)); + } finally { + cleanup(); + } +}); + +test('safeResolveWithin rejects parent-traversal segments', () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + try { + assert.equal(safeResolveWithin(artifactsDir, '..', 'etc', 'passwd'), undefined); + assert.equal(safeResolveWithin(artifactsDir, '../../../etc/passwd'), undefined); + } finally { + cleanup(); + } +}); + +test('safeResolveWithin rejects absolute segments that escape the base', () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + try { + assert.equal(safeResolveWithin(artifactsDir, '/etc/passwd'), undefined); + } finally { + cleanup(); + } +}); + +test('loadRunManifestRecord throws RunManifestNotFoundError for traversal runIds', async () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; + try { + await assert.rejects( + () => loadRunManifestRecord('../../../etc/passwd', context), + (error: Error) => error instanceof RunManifestNotFoundError, + ); + } finally { + cleanup(); + } +}); + +test('loadRunManifestRecord throws RunManifestNotFoundError for missing runs', async () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; + try { + await assert.rejects( + () => loadRunManifestRecord('does-not-exist', context), + (error: Error) => error instanceof RunManifestNotFoundError, + ); + } finally { + cleanup(); + } +}); + +test('loadRunManifestRecord surfaces non-ENOENT errors as generic errors', async () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; + const runDir = path.join(artifactsDir, 'corrupt-run'); + await fsp.mkdir(runDir, { recursive: true }); + await fsp.writeFile(path.join(runDir, 'run.json'), 'this is not json', 'utf-8'); + try { + await assert.rejects( + () => loadRunManifestRecord('corrupt-run', context), + (error: Error) => !(error instanceof RunManifestNotFoundError), + ); + } finally { + cleanup(); + } +}); + +test('loadRunManifestRecord rejects unsupported schema versions with a generic error', async () => { + const { artifactsDir, cleanup } = mkArtifactsDir(); + const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; + const runDir = path.join(artifactsDir, 'old-schema'); + await fsp.mkdir(runDir, { recursive: true }); + await fsp.writeFile( + path.join(runDir, 'run.json'), + JSON.stringify({ schemaVersion: 1 }), + 'utf-8', + ); + try { + await assert.rejects( + () => loadRunManifestRecord('old-schema', context), + (error: Error) => + !(error instanceof RunManifestNotFoundError) && + /Unsupported schema version/.test(error.message), + ); + } finally { + cleanup(); + } +}); diff --git a/packages/cli/src/reportViewModel.ts b/packages/cli/src/reportViewModel.ts index ce2e842..cadf246 100644 --- a/packages/cli/src/reportViewModel.ts +++ b/packages/cli/src/reportViewModel.ts @@ -21,6 +21,25 @@ import { loadRunIndex } from './runIndex.js'; const MISSING_WORKSPACE_CONFIG_ERROR = 'The FinalRun report server is missing workspace configuration. Start it with `finalrun start-server`.'; +export class RunManifestNotFoundError extends Error { + constructor(runId: string) { + super(`Run manifest not found for runId: ${runId}`); + this.name = 'RunManifestNotFoundError'; + } +} + +// Resolve `parts` relative to `baseDir` and refuse to escape it. Returns +// undefined when the resolved path lies outside the base — callers turn that +// into a not-found / rejection instead of touching the filesystem. +export function safeResolveWithin(baseDir: string, ...parts: string[]): string | undefined { + const base = path.resolve(baseDir); + const resolved = path.resolve(base, ...parts); + if (resolved === base || resolved.startsWith(`${base}${path.sep}`)) { + return resolved; + } + return undefined; +} + export interface ReportWorkspaceContext { workspaceRoot: string; artifactsDir: string; @@ -92,8 +111,19 @@ export async function loadRunManifestRecord( runId: string, context: ReportWorkspaceContext = resolveReportWorkspaceContext(), ): Promise { - const runJsonPath = path.join(context.artifactsDir, runId, 'run.json'); - const raw = await fsp.readFile(runJsonPath, 'utf-8'); + const runJsonPath = safeResolveWithin(context.artifactsDir, runId, 'run.json'); + if (!runJsonPath) { + throw new RunManifestNotFoundError(runId); + } + let raw: string; + try { + raw = await fsp.readFile(runJsonPath, 'utf-8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + throw new RunManifestNotFoundError(runId); + } + throw error; + } const parsed = JSON.parse(raw) as RunManifest; if (parsed.schemaVersion !== 2 && parsed.schemaVersion !== 3) { throw new Error(`Unsupported schema version: ${parsed.schemaVersion}`); @@ -165,8 +195,13 @@ async function readRunArtifactText( const normalizedPath = normalizeRunArtifactPath(runId, artifactPath); if (!normalizedPath) return undefined; + const runRoot = safeResolveWithin(context.artifactsDir, runId); + if (!runRoot) return undefined; + const artifactFsPath = safeResolveWithin(runRoot, normalizedPath); + if (!artifactFsPath) return undefined; + try { - return await fsp.readFile(path.join(context.artifactsDir, runId, normalizedPath), 'utf-8'); + return await fsp.readFile(artifactFsPath, 'utf-8'); } catch { return undefined; } diff --git a/packages/cloud-core/package.json b/packages/cloud-core/package.json index 4bee0a0..2bcc347 100644 --- a/packages/cloud-core/package.json +++ b/packages/cloud-core/package.json @@ -8,7 +8,7 @@ "types": "./dist/index.d.ts", "scripts": { "build": "npm run clean && tsc", - "clean": "rm -rf dist tsconfig.tsbuildinfo", + "clean": "node -e \"const fs=require('node:fs'); for (const p of ['dist','tsconfig.tsbuildinfo']) fs.rmSync(p,{recursive:true,force:true});\"", "test": "node --test \"dist/**/*.test.js\"" }, "dependencies": { diff --git a/packages/cloud-core/src/submit.test.ts b/packages/cloud-core/src/submit.test.ts new file mode 100644 index 0000000..c7eb5bd --- /dev/null +++ b/packages/cloud-core/src/submit.test.ts @@ -0,0 +1,43 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { isSafeEnvName, isSafeRelativeSegment } from './submit.js'; + +test('isSafeRelativeSegment accepts simple relative paths', () => { + assert.equal(isSafeRelativeSegment('foo.yml'), true); + assert.equal(isSafeRelativeSegment('subdir/foo.yml'), true); + assert.equal(isSafeRelativeSegment('a/b/c/d.yaml'), true); +}); + +test('isSafeRelativeSegment rejects parent traversal', () => { + assert.equal(isSafeRelativeSegment('../foo.yml'), false); + assert.equal(isSafeRelativeSegment('../../etc/passwd'), false); + assert.equal(isSafeRelativeSegment('..'), false); + assert.equal(isSafeRelativeSegment('subdir/../../escape'), false); +}); + +test('isSafeRelativeSegment rejects absolute paths', () => { + assert.equal(isSafeRelativeSegment('/etc/passwd'), false); +}); + +test('isSafeRelativeSegment normalises Windows-style separators before checking', () => { + assert.equal(isSafeRelativeSegment('..\\foo.yml'), false); + assert.equal(isSafeRelativeSegment('subdir\\foo.yml'), true); +}); + +test('isSafeRelativeSegment rejects empty values', () => { + assert.equal(isSafeRelativeSegment(''), false); +}); + +test('isSafeEnvName accepts conservative names', () => { + assert.equal(isSafeEnvName('staging'), true); + assert.equal(isSafeEnvName('dev_1'), true); + assert.equal(isSafeEnvName('feature-x.2'), true); +}); + +test('isSafeEnvName rejects path-like and exotic values', () => { + assert.equal(isSafeEnvName('../etc'), false); + assert.equal(isSafeEnvName('/etc'), false); + assert.equal(isSafeEnvName('a/b'), false); + assert.equal(isSafeEnvName('with space'), false); + assert.equal(isSafeEnvName(''), false); +}); diff --git a/packages/cloud-core/src/submit.ts b/packages/cloud-core/src/submit.ts index 6781382..3e878e2 100644 --- a/packages/cloud-core/src/submit.ts +++ b/packages/cloud-core/src/submit.ts @@ -46,6 +46,23 @@ export interface SubmitRunResult { appFilename?: string; } +// Reject paths that escape the upload archive: absolute paths, `..` segments, +// or anything that normalizes outside the implicit per-section root +// (`tests/`, `suites/`, `env/`). +export function isSafeRelativeSegment(value: string): boolean { + if (!value || path.isAbsolute(value)) return false; + const normalized = path.posix.normalize(value.replace(/\\/g, '/')); + return !normalized.startsWith('../') && normalized !== '..'; +} + +// Env names land in filesystem path joins and as form-data values; restrict +// to a conservative charset to keep both safe. +const ENV_NAME_PATTERN = /^[A-Za-z0-9._-]+$/; + +export function isSafeEnvName(value: string): boolean { + return ENV_NAME_PATTERN.test(value); +} + // Generous timeout to accommodate large APK/IPA uploads on slow uplinks while // still catching genuinely stalled connections. Override with // FINALRUN_SUBMIT_TIMEOUT_MS for ultra-large uploads or low-bandwidth tests. @@ -88,6 +105,9 @@ export async function submitRun(input: SubmitRunInput): Promise const filesToZip: Array<{ absolutePath: string; relativePath: string }> = []; if (input.checked.suite?.sourcePath && input.checked.suite.relativePath) { + if (!isSafeRelativeSegment(input.checked.suite.relativePath)) { + throw new Error(`Invalid suite relativePath: ${input.checked.suite.relativePath}`); + } filesToZip.push({ absolutePath: input.checked.suite.sourcePath, relativePath: path.join('suites', input.checked.suite.relativePath), @@ -96,6 +116,9 @@ export async function submitRun(input: SubmitRunInput): Promise for (const spec of input.checked.tests) { if (!spec.sourcePath || !spec.relativePath) continue; + if (!isSafeRelativeSegment(spec.relativePath)) { + throw new Error(`Invalid test relativePath: ${spec.relativePath}`); + } filesToZip.push({ absolutePath: spec.sourcePath, relativePath: path.join('tests', spec.relativePath), @@ -119,6 +142,9 @@ export async function submitRun(input: SubmitRunInput): Promise // .finalrun/env/) avoids leaking other environments' bindings to the // cloud submission. if (input.envName) { + if (!isSafeEnvName(input.envName)) { + throw new Error(`Invalid env name: ${input.envName}`); + } const envDir = path.join(input.workspaceRoot, '.finalrun', 'env'); const candidates = [`${input.envName}.yaml`, `${input.envName}.yml`]; for (const candidate of candidates) { diff --git a/packages/common/package.json b/packages/common/package.json index c4016a4..901bc0c 100644 --- a/packages/common/package.json +++ b/packages/common/package.json @@ -13,7 +13,7 @@ }, "scripts": { "build": "npm run clean && tsc", - "clean": "rm -rf dist tsconfig.tsbuildinfo", + "clean": "node -e \"const fs=require('node:fs'); for (const p of ['dist','tsconfig.tsbuildinfo']) fs.rmSync(p,{recursive:true,force:true});\"", "test": "node --test \"dist/**/*.test.js\"" }, "devDependencies": { diff --git a/packages/device-node/package.json b/packages/device-node/package.json index 094a672..e10b1d7 100644 --- a/packages/device-node/package.json +++ b/packages/device-node/package.json @@ -11,7 +11,7 @@ ], "scripts": { "build": "npm run clean && tsc", - "clean": "rm -rf dist tsconfig.tsbuildinfo", + "clean": "node -e \"const fs=require('node:fs'); for (const p of ['dist','tsconfig.tsbuildinfo']) fs.rmSync(p,{recursive:true,force:true});\"", "generate:proto": "npx grpc_tools_node_protoc --ts_proto_out=./src/generated --ts_proto_opt=outputServices=grpc-js --ts_proto_opt=esModuleInterop=true --proto_path=../../proto ../../proto/finalrun/driver.proto", "test": "node --test \"dist/**/*.test.js\"" }, diff --git a/packages/goal-executor/package.json b/packages/goal-executor/package.json index df3ab29..12732ad 100644 --- a/packages/goal-executor/package.json +++ b/packages/goal-executor/package.json @@ -12,7 +12,7 @@ ], "scripts": { "build": "npm run clean && tsc", - "clean": "rm -rf dist tsconfig.tsbuildinfo", + "clean": "node -e \"const fs=require('node:fs'); for (const p of ['dist','tsconfig.tsbuildinfo']) fs.rmSync(p,{recursive:true,force:true});\"", "test": "node --test \"dist/**/*.test.js\"" }, "dependencies": { diff --git a/packages/report-web/src/ui/components/SummaryCard.tsx b/packages/report-web/src/ui/components/SummaryCard.tsx index c09f9d4..73f2a4d 100644 --- a/packages/report-web/src/ui/components/SummaryCard.tsx +++ b/packages/report-web/src/ui/components/SummaryCard.tsx @@ -1,23 +1,22 @@ +import type { ReactNode } from 'react'; import { summaryIconStyle, type SummaryTone } from '../format'; export function SummaryCard({ label, value, tone, - iconSvg, + icon, }: { label: string; value: string; tone: SummaryTone; - iconSvg: string; + icon: ReactNode; }) { return (
- + + {icon} +
{label}
{value}
diff --git a/packages/report-web/src/ui/iconNodes.tsx b/packages/report-web/src/ui/iconNodes.tsx new file mode 100644 index 0000000..01e936a --- /dev/null +++ b/packages/report-web/src/ui/iconNodes.tsx @@ -0,0 +1,28 @@ +// React-element forms of the inline SVGs used by SummaryCard. Kept as +// ReactElement so the SummaryCard contract can take a typed node instead of +// a string — closing the dangerouslySetInnerHTML XSS boundary at compile +// time. + +import type { ReactElement } from 'react'; + +export const PLAY_CIRCLE_ICON_NODE: ReactElement = ( + +); + +export const CHECK_CIRCLE_ICON_NODE: ReactElement = ( + +); + +export const TIMER_ICON_NODE: ReactElement = ( + +); diff --git a/packages/report-web/src/ui/icons.ts b/packages/report-web/src/ui/icons.ts index 974aea3..917e6b6 100644 --- a/packages/report-web/src/ui/icons.ts +++ b/packages/report-web/src/ui/icons.ts @@ -18,16 +18,9 @@ export const LOCAL_ICON_SRC = svgDataUri( '', ); -// Inline stroke-based SVGs used inside summary cards and buttons. - -export const PLAY_CIRCLE_ICON_SVG = - ''; - -export const CHECK_CIRCLE_ICON_SVG = - ''; - -export const TIMER_ICON_SVG = - ''; +// Inline stroke-based SVGs used inside buttons. The summary-card icons that +// were here previously have moved to iconNodes.tsx as React elements so the +// SummaryCard prop can be typed as ReactNode (no dangerouslySetInnerHTML). export const BACK_ARROW_ICON_SVG = ''; diff --git a/packages/report-web/src/ui/pages/RunIndexView.tsx b/packages/report-web/src/ui/pages/RunIndexView.tsx index 9a5c135..4214eec 100644 --- a/packages/report-web/src/ui/pages/RunIndexView.tsx +++ b/packages/report-web/src/ui/pages/RunIndexView.tsx @@ -6,14 +6,12 @@ import { buildRunRoute } from '../routes'; import { SummaryCard } from '../components/SummaryCard'; import { StatusPill } from '../components/StatusPill'; import { TintedPngIcon } from '../components/TintedPngIcon'; +import { LOCAL_ICON_SRC, TEST_ICON_SRC, TEST_SUITE_ICON_SRC } from '../icons'; import { - CHECK_CIRCLE_ICON_SVG, - LOCAL_ICON_SRC, - PLAY_CIRCLE_ICON_SVG, - TEST_ICON_SRC, - TEST_SUITE_ICON_SRC, - TIMER_ICON_SVG, -} from '../icons'; + CHECK_CIRCLE_ICON_NODE, + PLAY_CIRCLE_ICON_NODE, + TIMER_ICON_NODE, +} from '../iconNodes'; import { formatLongDuration, successRateTone } from '../format'; import '../styles/shared.css'; @@ -46,19 +44,19 @@ export function RunIndexView({ label="Total Runs" value={String(index.summary.totalRuns)} tone="accent" - iconSvg={PLAY_CIRCLE_ICON_SVG} + icon={PLAY_CIRCLE_ICON_NODE} /> From d4c6f6e9bd8aa67d5679554e1015aba755522b1c Mon Sep 17 00:00:00 2001 From: ashish Date: Mon, 27 Apr 2026 22:20:39 -0700 Subject: [PATCH 2/2] fix: address CodeRabbit findings on PR #129 Three actionable + two nitpicks from the bot review: - reportServer.ts: tryServeFile now returns true after a handled stream error so serveSpaAsset doesn't fall through to the SPA index.html and try to write to an already-finalized response. - submit.ts: isSafeRelativeSegment now checks both posix and win32 absolute-path shapes (drive letters and UNC) so a Windows-shaped absolute path can't slip past the guard when the validator runs on POSIX hosts. - SummaryCard.tsx: import CSSProperties directly from react instead of reaching for the React namespace, which isn't globally available under jsx: react-jsx. - submit.test.ts: extend the absolute-path test with C:\..., c:\..., and \\\\server\\share\\... cases. - reportViewModel.test.ts: move tmpdir setup inside the try block so a failure in fs.mkdir / fs.writeFile doesn't leak the temp directory. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/cli/src/reportServer.ts | 5 +++- packages/cli/src/reportViewModel.test.ts | 24 +++++++++---------- packages/cloud-core/src/submit.test.ts | 5 +++- packages/cloud-core/src/submit.ts | 16 +++++++++---- .../src/ui/components/SummaryCard.tsx | 6 ++--- 5 files changed, 34 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/reportServer.ts b/packages/cli/src/reportServer.ts index ac5d89b..b930aaa 100644 --- a/packages/cli/src/reportServer.ts +++ b/packages/cli/src/reportServer.ts @@ -207,7 +207,10 @@ async function tryServeFile( } else { response.destroy(); } - return false; + // Response is already finalized (500 sent) or destroyed; signal "handled" + // so serveSpaAsset doesn't fall through to the SPA index.html and try to + // write to a closed response. + return true; } return true; } diff --git a/packages/cli/src/reportViewModel.test.ts b/packages/cli/src/reportViewModel.test.ts index 40e6f95..b5abae2 100644 --- a/packages/cli/src/reportViewModel.test.ts +++ b/packages/cli/src/reportViewModel.test.ts @@ -88,11 +88,11 @@ test('loadRunManifestRecord throws RunManifestNotFoundError for missing runs', a test('loadRunManifestRecord surfaces non-ENOENT errors as generic errors', async () => { const { artifactsDir, cleanup } = mkArtifactsDir(); - const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; - const runDir = path.join(artifactsDir, 'corrupt-run'); - await fsp.mkdir(runDir, { recursive: true }); - await fsp.writeFile(path.join(runDir, 'run.json'), 'this is not json', 'utf-8'); try { + const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; + const runDir = path.join(artifactsDir, 'corrupt-run'); + await fsp.mkdir(runDir, { recursive: true }); + await fsp.writeFile(path.join(runDir, 'run.json'), 'this is not json', 'utf-8'); await assert.rejects( () => loadRunManifestRecord('corrupt-run', context), (error: Error) => !(error instanceof RunManifestNotFoundError), @@ -104,15 +104,15 @@ test('loadRunManifestRecord surfaces non-ENOENT errors as generic errors', async test('loadRunManifestRecord rejects unsupported schema versions with a generic error', async () => { const { artifactsDir, cleanup } = mkArtifactsDir(); - const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; - const runDir = path.join(artifactsDir, 'old-schema'); - await fsp.mkdir(runDir, { recursive: true }); - await fsp.writeFile( - path.join(runDir, 'run.json'), - JSON.stringify({ schemaVersion: 1 }), - 'utf-8', - ); try { + const context: ReportWorkspaceContext = { workspaceRoot: artifactsDir, artifactsDir }; + const runDir = path.join(artifactsDir, 'old-schema'); + await fsp.mkdir(runDir, { recursive: true }); + await fsp.writeFile( + path.join(runDir, 'run.json'), + JSON.stringify({ schemaVersion: 1 }), + 'utf-8', + ); await assert.rejects( () => loadRunManifestRecord('old-schema', context), (error: Error) => diff --git a/packages/cloud-core/src/submit.test.ts b/packages/cloud-core/src/submit.test.ts index c7eb5bd..86e0b5d 100644 --- a/packages/cloud-core/src/submit.test.ts +++ b/packages/cloud-core/src/submit.test.ts @@ -15,8 +15,11 @@ test('isSafeRelativeSegment rejects parent traversal', () => { assert.equal(isSafeRelativeSegment('subdir/../../escape'), false); }); -test('isSafeRelativeSegment rejects absolute paths', () => { +test('isSafeRelativeSegment rejects absolute paths across platforms', () => { assert.equal(isSafeRelativeSegment('/etc/passwd'), false); + assert.equal(isSafeRelativeSegment('C:\\Windows\\System32\\drivers\\etc\\hosts'), false); + assert.equal(isSafeRelativeSegment('c:\\tmp\\a'), false); + assert.equal(isSafeRelativeSegment('\\\\server\\share\\file.yml'), false); }); test('isSafeRelativeSegment normalises Windows-style separators before checking', () => { diff --git a/packages/cloud-core/src/submit.ts b/packages/cloud-core/src/submit.ts index 3e878e2..f0b7b62 100644 --- a/packages/cloud-core/src/submit.ts +++ b/packages/cloud-core/src/submit.ts @@ -46,12 +46,18 @@ export interface SubmitRunResult { appFilename?: string; } -// Reject paths that escape the upload archive: absolute paths, `..` segments, -// or anything that normalizes outside the implicit per-section root -// (`tests/`, `suites/`, `env/`). +// Reject paths that escape the upload archive: absolute paths (POSIX, Windows +// drive-letter, UNC), `..` segments, or anything that normalizes outside the +// implicit per-section root (`tests/`, `suites/`, `env/`). +// +// Both posix and win32 absolute-path shapes are checked because uploads can +// originate from any client OS — POSIX `path.isAbsolute` alone misses `C:\foo` +// on macOS/Linux. export function isSafeRelativeSegment(value: string): boolean { - if (!value || path.isAbsolute(value)) return false; - const normalized = path.posix.normalize(value.replace(/\\/g, '/')); + if (!value) return false; + const slashNormalized = value.replace(/\\/g, '/'); + if (path.posix.isAbsolute(slashNormalized) || path.win32.isAbsolute(value)) return false; + const normalized = path.posix.normalize(slashNormalized); return !normalized.startsWith('../') && normalized !== '..'; } diff --git a/packages/report-web/src/ui/components/SummaryCard.tsx b/packages/report-web/src/ui/components/SummaryCard.tsx index 73f2a4d..cd6de88 100644 --- a/packages/report-web/src/ui/components/SummaryCard.tsx +++ b/packages/report-web/src/ui/components/SummaryCard.tsx @@ -1,4 +1,4 @@ -import type { ReactNode } from 'react'; +import type { CSSProperties, ReactNode } from 'react'; import { summaryIconStyle, type SummaryTone } from '../format'; export function SummaryCard({ @@ -28,7 +28,7 @@ export function SummaryCard({ // Helper: the legacy renderer emitted raw `style="color: x; background: y;"` // strings. React wants a camelCased object. This parses a CSS declaration list // back into the object form so the rendered HTML is byte-identical in effect. -function inlineStyleFromString(css: string): React.CSSProperties { +function inlineStyleFromString(css: string): CSSProperties { const result: Record = {}; for (const rule of css.split(';')) { const [rawProp, ...rest] = rule.split(':'); @@ -39,5 +39,5 @@ function inlineStyleFromString(css: string): React.CSSProperties { const camel = prop.replace(/-([a-z])/g, (_, c) => c.toUpperCase()); result[camel] = value; } - return result as React.CSSProperties; + return result as CSSProperties; }