fix: path traversal guards, report-server robustness, Windows-portable clean scripts#129
Open
fix: path traversal guards, report-server robustness, Windows-portable clean scripts#129
Conversation
…e 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) <noreply@anthropic.com>
📝 WalkthroughWalkthroughUpdates cross-platform cleanup scripts, adds path/env input validation, hardens report-server path handling and streaming error responses (404 vs 500), introduces safe path resolution and a manifest-not-found error class, adds tests, and refactors three summary-card icons to React nodes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Server as ReportServer
participant Resolver as safeResolveWithin
participant FS as Filesystem
Client->>Server: GET /api/report/runs/:runId
Server->>Resolver: resolve runRoot within artifactsDir
Resolver-->>Server: safe runRoot path / undefined
alt runRoot undefined
Server-->>Client: 404 { error: "RunManifestNotFoundError" }
else runRoot ok
Server->>FS: read run.json
FS-->>Server: file content / error (ENOENT / corrupt / other)
alt ENOENT
Server-->>Client: 404 JSON (manifest not found)
else corrupt / other error
Server-->>Client: 500 text/plain or JSON error
else valid manifest
Server-->>Client: 200 JSON (manifest)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/cloud-core/src/submit.test.ts (1)
18-20: Add Windows-style absolute path assertions to this security test.For portability/security regression protection, include drive-letter and UNC absolute path cases alongside the POSIX absolute-path check.
➕ Suggested test additions
test('isSafeRelativeSegment rejects absolute paths', () => { assert.equal(isSafeRelativeSegment('/etc/passwd'), false); + assert.equal(isSafeRelativeSegment('C:\\Windows\\System32\\drivers\\etc\\hosts'), false); + assert.equal(isSafeRelativeSegment('\\\\server\\share\\file.yml'), false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cloud-core/src/submit.test.ts` around lines 18 - 20, Add Windows-style absolute path cases to the security test for isSafeRelativeSegment: extend the test that currently checks '/etc/passwd' to also assert that a drive-letter absolute path like 'C:\\Windows\\System32' (and its lowercase variant) and a UNC path like '\\\\server\\share\\file.txt' are rejected (return false). Locate the test for isSafeRelativeSegment in submit.test.ts and add assertions for these Windows absolute-path patterns to ensure the function correctly rejects them.packages/cli/src/reportViewModel.test.ts (1)
89-95: Move setup insidetry/finallyfor leak-proof cleanup.On Line 93–Line 94 and Line 109–Line 114, setup occurs before the
try. If setup throws, temp dirs won’t be cleaned up.♻️ Suggested adjustment
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 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), ); } finally {Also applies to: 105-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportViewModel.test.ts` around lines 89 - 95, The test currently performs setup (mkArtifactsDir(), creating runDir, writing run.json) outside the try block so if any setup step throws the temporary artifacts aren't cleaned; move the resource setup (calling mkArtifactsDir(), building context/artifactsDir/runDir, fsp.mkdir and fsp.writeFile) inside the try block and ensure cleanup() is awaited in a finally block so cleanup always runs; update the test function around the loadRunManifestRecord assertions to call cleanup() in finally and keep references to mkArtifactsDir, context, runDir, fsp.mkdir, fsp.writeFile, and cleanup to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/reportServer.ts`:
- Around line 201-211: The catch in tryServeFile currently returns false after
ending or destroying the response, which lets serveSpaAsset continue and may
write to a closed/sent response; update the error path in tryServeFile so that
after you call response.writeHead/.../response.end or response.destroy you
return true (or otherwise signal "handled") instead of false, ensuring
serveSpaAsset does not run fallback logic on a failed stream.
In `@packages/cloud-core/src/submit.ts`:
- Around line 52-56: The isSafeRelativeSegment function misses Windows absolute
paths because path.isAbsolute is OS-specific; update isSafeRelativeSegment to
explicitly detect Windows-style absolute paths and UNC paths before using
path.isAbsolute: check for drive-letter prefixes like /^[A-Za-z]:[\\\/]/ and
network/UNC prefixes starting with '\\' or '//', and return false for those;
keep the existing backslash-to-slash normalize logic and existing checks for
'..' after normalization. Also add a unit test asserting
isSafeRelativeSegment('C:\\tmp\\a') === false (and one for a UNC path like
'\\\\server\\share') to cover Windows inputs.
In `@packages/report-web/src/ui/components/SummaryCard.tsx`:
- Line 1: The types reference React.CSSProperties but only ReactNode is
imported; import CSSProperties from 'react' and update the type annotations in
SummaryCard (the functions/props that currently use React.CSSProperties on the
return types at the lines referencing those types) to use the imported
CSSProperties identifier (e.g., change React.CSSProperties to CSSProperties) so
the type resolves under the react-jsx config.
---
Nitpick comments:
In `@packages/cli/src/reportViewModel.test.ts`:
- Around line 89-95: The test currently performs setup (mkArtifactsDir(),
creating runDir, writing run.json) outside the try block so if any setup step
throws the temporary artifacts aren't cleaned; move the resource setup (calling
mkArtifactsDir(), building context/artifactsDir/runDir, fsp.mkdir and
fsp.writeFile) inside the try block and ensure cleanup() is awaited in a finally
block so cleanup always runs; update the test function around the
loadRunManifestRecord assertions to call cleanup() in finally and keep
references to mkArtifactsDir, context, runDir, fsp.mkdir, fsp.writeFile, and
cleanup to locate and modify the code.
In `@packages/cloud-core/src/submit.test.ts`:
- Around line 18-20: Add Windows-style absolute path cases to the security test
for isSafeRelativeSegment: extend the test that currently checks '/etc/passwd'
to also assert that a drive-letter absolute path like 'C:\\Windows\\System32'
(and its lowercase variant) and a UNC path like '\\\\server\\share\\file.txt'
are rejected (return false). Locate the test for isSafeRelativeSegment in
submit.test.ts and add assertions for these Windows absolute-path patterns to
ensure the function correctly rejects them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a053c55f-caeb-422e-bbaf-e8fcec5832e0
📒 Files selected for processing (15)
packages/cli/package.jsonpackages/cli/src/reportServer.test.tspackages/cli/src/reportServer.tspackages/cli/src/reportViewModel.test.tspackages/cli/src/reportViewModel.tspackages/cloud-core/package.jsonpackages/cloud-core/src/submit.test.tspackages/cloud-core/src/submit.tspackages/common/package.jsonpackages/device-node/package.jsonpackages/goal-executor/package.jsonpackages/report-web/src/ui/components/SummaryCard.tsxpackages/report-web/src/ui/iconNodes.tsxpackages/report-web/src/ui/icons.tspackages/report-web/src/ui/pages/RunIndexView.tsx
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/reportServer.ts (1)
192-206:⚠️ Potential issue | 🟠 MajorThe 500 error handler is unreachable after
writeHead(200)is called on line 192.After
response.writeHead(200, ...)executes,response.headersSentis set to true immediately. Whenpipeline()fails at line 202, the condition!response.headersSentat line 204 will be false, so the error handler that sends a 500 response never runs. Instead, the else branch (line 208) always executes on stream errors, callingresponse.destroy()and leaving clients with a destroyed socket rather than an HTTP 500 response.Use
response.statusCodeandresponse.setHeader()instead ofwriteHead()to preventheadersSentfrom being set until the first byte is written, allowing the 500 handler to execute if streaming fails.Proposed fix
- response.writeHead(200, { - 'Content-Type': contentType, - 'Content-Length': String(stats.size), - 'Cache-Control': cacheControl, - }); + response.statusCode = 200; + response.setHeader('Content-Type', contentType); + response.setHeader('Content-Length', String(stats.size)); + response.setHeader('Cache-Control', cacheControl); if (headOnly) { response.end(); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportServer.ts` around lines 192 - 206, The handler currently calls response.writeHead(200, ...) which sets headersSent immediately and prevents the 500 error branch from ever running; change it to set response.statusCode = 200 and call response.setHeader(...) for 'Content-Type', 'Content-Length', and 'Cache-Control' (using the existing contentType, String(stats.size), and cacheControl) so headers are not marked sent until the first byte is flushed, keep the headOnly check and the await pipeline(fs.createReadStream(filePath), response) call, and then the existing catch branch can correctly check !response.headersSent and send a 500 via response.setHeader()/response.statusCode or response.writeHead if still unset before ending the response.
🧹 Nitpick comments (1)
packages/cli/src/reportServer.ts (1)
79-82: Avoid returning raw internal errors in API 500 responses.Line 81 may expose parser/internal filesystem details to clients. Prefer a stable generic message and log details server-side.
♻️ Suggested adjustment
} else { + console.error('[report-server] /api/report/runs/:runId failed', error); writeJson(response, 500, { status: 'error', - message: error instanceof Error ? error.message : String(error), + message: 'Internal server error', }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportServer.ts` around lines 79 - 82, The current 500 handler calls writeJson(response, 500, { status: 'error', message: error instanceof Error ? error.message : String(error) }) which can leak internal/parser/filesystem details to clients; change this to return a stable generic message (e.g., "Internal server error") in the JSON body and move the detailed error information to server-side logs (use the existing logger or console.error) before calling writeJson so clients never receive raw error.message but admins still see full diagnostics; update the code around the writeJson call in reportServer.ts (the error handling block) to log the full error and send only the generic message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cloud-core/src/submit.ts`:
- Around line 66-70: ENV_NAME_PATTERN and isSafeEnvName currently allow the
special dot names "." and ".."; update the validation to explicitly reject those
values. Fix by modifying isSafeEnvName (or the regex) so it returns false when
value === "." or value === ".." (or by using a negative lookahead in
ENV_NAME_PATTERN to exclude exact "." and ".."), while keeping the existing
character rules intact; reference ENV_NAME_PATTERN and isSafeEnvName to locate
and apply the change.
- Around line 56-61: The isSafeRelativeSegment function currently allows inputs
that normalize to "." (e.g., "a/.."); update the validation in
isSafeRelativeSegment to reject normalized "." in addition to "..".
Specifically, after computing normalized via
path.posix.normalize(slashNormalized), ensure you check normalized !== '.' (and
keep the existing checks for '../' and '..') so that segments normalizing to the
current directory are considered unsafe and return false.
---
Outside diff comments:
In `@packages/cli/src/reportServer.ts`:
- Around line 192-206: The handler currently calls response.writeHead(200, ...)
which sets headersSent immediately and prevents the 500 error branch from ever
running; change it to set response.statusCode = 200 and call
response.setHeader(...) for 'Content-Type', 'Content-Length', and
'Cache-Control' (using the existing contentType, String(stats.size), and
cacheControl) so headers are not marked sent until the first byte is flushed,
keep the headOnly check and the await pipeline(fs.createReadStream(filePath),
response) call, and then the existing catch branch can correctly check
!response.headersSent and send a 500 via
response.setHeader()/response.statusCode or response.writeHead if still unset
before ending the response.
---
Nitpick comments:
In `@packages/cli/src/reportServer.ts`:
- Around line 79-82: The current 500 handler calls writeJson(response, 500, {
status: 'error', message: error instanceof Error ? error.message : String(error)
}) which can leak internal/parser/filesystem details to clients; change this to
return a stable generic message (e.g., "Internal server error") in the JSON body
and move the detailed error information to server-side logs (use the existing
logger or console.error) before calling writeJson so clients never receive raw
error.message but admins still see full diagnostics; update the code around the
writeJson call in reportServer.ts (the error handling block) to log the full
error and send only the generic message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1c66cdb-acfe-4ee8-a2ea-04ed3f2cad36
📒 Files selected for processing (5)
packages/cli/src/reportServer.tspackages/cli/src/reportViewModel.test.tspackages/cloud-core/src/submit.test.tspackages/cloud-core/src/submit.tspackages/report-web/src/ui/components/SummaryCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cloud-core/src/submit.test.ts
- packages/cli/src/reportViewModel.test.ts
Comment on lines
+56
to
+61
| export function isSafeRelativeSegment(value: string): boolean { | ||
| 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 !== '..'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
cat -n packages/cloud-core/src/submit.ts | sed -n '50,70p'Repository: final-run/finalrun-agent
Length of output: 1164
🏁 Script executed:
# Test the normalization behavior mentioned in the review
python3 << 'EOF'
import os
# Simulate what path.posix.normalize does in Node.js
# Python's os.path.normpath behaves similarly
test_cases = [
'a/..',
'.',
'a/../b',
'tests',
]
for case in test_cases:
# posix-style normalization (using os.path.normpath with forward slashes)
normalized = os.path.normpath(case.replace('\\', '/'))
print(f"normalize('{case}') -> '{normalized}'")
# Test path.join behavior
print("\n--- path.join behavior ---")
for case in test_cases:
joined = os.path.join('tests', case)
print(f"join('tests', '{case}') -> '{joined}'")
EOFRepository: final-run/finalrun-agent
Length of output: 352
🏁 Script executed:
# Search for call sites of isSafeRelativeSegment
rg "isSafeRelativeSegment" packages/cloud-core/src/ -B 2 -A 2Repository: final-run/finalrun-agent
Length of output: 4010
🏁 Script executed:
# Search for context around the validators and their usage
rg "isSafeRelativeSegment|isSafeEnvName" packages/cloud-core/src/ --context 5Repository: final-run/finalrun-agent
Length of output: 8108
Add validation to reject paths that normalize to . in isSafeRelativeSegment.
Input values like a/.. normalize to ., which currently passes validation despite the documented guarantee that paths remain confined to their per-section root (tests/, suites/, env/). This is a validation gap: the function checks normalized !== '..' but not normalized !== '.'.
Suggested fix
export function isSafeRelativeSegment(value: string): boolean {
if (!value) return false;
const slashNormalized = value.replace(/\\/g, '/');
if (path.posix.isAbsolute(slashNormalized) || path.win32.isAbsolute(value)) return false;
+ const segments = slashNormalized.split('/').filter(Boolean);
+ if (segments.includes('..')) return false;
const normalized = path.posix.normalize(slashNormalized);
- return !normalized.startsWith('../') && normalized !== '..';
+ return normalized !== '.' && !normalized.startsWith('../') && normalized !== '..';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cloud-core/src/submit.ts` around lines 56 - 61, The
isSafeRelativeSegment function currently allows inputs that normalize to "."
(e.g., "a/.."); update the validation in isSafeRelativeSegment to reject
normalized "." in addition to "..". Specifically, after computing normalized via
path.posix.normalize(slashNormalized), ensure you check normalized !== '.' (and
keep the existing checks for '../' and '..') so that segments normalizing to the
current directory are considered unsafe and return false.
Comment on lines
+66
to
+70
| const ENV_NAME_PATTERN = /^[A-Za-z0-9._-]+$/; | ||
|
|
||
| export function isSafeEnvName(value: string): boolean { | ||
| return ENV_NAME_PATTERN.test(value); | ||
| } |
There was a problem hiding this comment.
Disallow . and .. env names explicitly.
ENV_NAME_PATTERN currently accepts both; they’re ambiguous path-like names and not useful as logical environment identifiers.
Suggested fix
export function isSafeEnvName(value: string): boolean {
- return ENV_NAME_PATTERN.test(value);
+ return ENV_NAME_PATTERN.test(value) && value !== '.' && value !== '..';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const ENV_NAME_PATTERN = /^[A-Za-z0-9._-]+$/; | |
| export function isSafeEnvName(value: string): boolean { | |
| return ENV_NAME_PATTERN.test(value); | |
| } | |
| const ENV_NAME_PATTERN = /^[A-Za-z0-9._-]+$/; | |
| export function isSafeEnvName(value: string): boolean { | |
| return ENV_NAME_PATTERN.test(value) && value !== '.' && value !== '..'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cloud-core/src/submit.ts` around lines 66 - 70, ENV_NAME_PATTERN and
isSafeEnvName currently allow the special dot names "." and ".."; update the
validation to explicitly reject those values. Fix by modifying isSafeEnvName (or
the regex) so it returns false when value === "." or value === ".." (or by using
a negative lookahead in ENV_NAME_PATTERN to exclude exact "." and ".."), while
keeping the existing character rules intact; reference ENV_NAME_PATTERN and
isSafeEnvName to locate and apply the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundled fix for the first three concerns in the CodeRabbit-on-
mainaction brief (PRs 1–3). One PR rather than three so review and CI happen once. PRs 4 (telemetry redaction) and 5 (lockfile + install URL pinning) are still scoped as separate follow-ups per the brief.Changes
Security — path traversal + XSS (PR 1)
reportViewModel.ts— newsafeResolveWithin(baseDir, ...parts)helper.loadRunManifestRecordresolvesrunIdthrough it (so arunIdof../../../etc/passwdis rejected before any filesystem call);readRunArtifactTextapplies it twice — once for the run root, once for the artifact path. ENOENT and traversal cases now throw a typedRunManifestNotFoundError.submit.ts— new exportedisSafeRelativeSegmentrejects../absolute relativePath values for bothsuite.relativePathand everytests[*].relativePathbefore they get joined into the upload zip. NewisSafeEnvNameenforces^[A-Za-z0-9._-]+$onenvNamebefore it's interpolated into a filesystem path and form-data field.SummaryCard.tsx— dropsdangerouslySetInnerHTMLentirely. Prop reshaped fromiconSvg: stringtoicon: ReactNode. The three icons used byRunIndexViewmove to a newiconNodes.tsxasReactElementconstants; the now-unused string SVGs are removed fromicons.ts. (Note: this is a breaking shape change to a publicly exported component in@finalrun/report-web/ui. Other inline-SVG sites elsewhere in the package use separatedangerouslySetInnerHTMLcalls — out of scope here per the brief.)Robustness — report-server (PR 2)
reportServer.ts— SPA file streaming switches fromfs.createReadStream(filePath).pipe(response)toawait pipeline(...)fromnode:stream/promises, so async stream errors surface instead of crashing the process / sending partial responses. The/api/report/runs/:runIdhandler now distinguishesRunManifestNotFoundError(404) from any other error (500), so corrupt JSON / EACCES no longer present as "not found" to the client.Windows portability (PR 3)
package.jsoncleanscripts switch fromrm -rf …to a Node one-liner usingfs.rmSync(…, { recursive: true, force: true })— works on macOS / Linux / Windows without adding arimrafdevDependency. Files:packages/cli,packages/cloud-core,packages/common,packages/device-node,packages/goal-executor. (packages/report-webhas nocleanscript.)Tests
reportViewModel.test.ts— new file. CoverssafeResolveWithin(in-bounds, parent-traversal, absolute-escape) andloadRunManifestRecord's error classification (traversal → not-found, missing → not-found, corrupt JSON → generic, unsupported schema → generic).reportServer.test.ts— new file. End-to-end HTTP checks: missing run → 404; traversal runId → 404; corruptrun.json→ 500.submit.test.ts— new file.isSafeRelativeSegment(simple/nested OK;..,../foo, absolute, embedded..traversal, empty rejected; Windows-style backslashes normalized first) andisSafeEnvNameaccept/reject cases.Test plan
npm run buildgreen from repo root (full monorepo: common, cloud-core, device-node, goal-executor, report-web lib + app, CLI)npm test— all new tests pass; the fourrunDoctorCommand/hostPreflightfailures that surface are pre-existing onmain(verified by running tests onmainbefore applying these changes — same fournot oklines, same banner-text mismatches)/api/report/runs/:runIdroute in a real workspace to confirm a missing run still renders the empty-state page (no manual repro performed in this branch — please verify on review if you can)cleanscripts — I can't exercise this here; flagging for spot-check on Windows if available*_ICON_SVGstring exports fromicons.tsis a public-API removal in@finalrun/report-web@0.2.0. They're internal-only inside this monorepo, but if there's an external consumer pinned to those exports we'd need to either keep them or bump the majorRelated
coderabbit-main-fixes-brief.md(PRs 1–3 of 5)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
UI