chore(engine): close path-traversal class on run-artifact endpoints#5
Open
chore(engine): close path-traversal class on run-artifact endpoints#5
Conversation
Ultrareview finding 1 (work/gaps.md 2026-04-20) called for a
GetRunDirectorySafe helper applied to the whole class of
Path.Combine(artifactsRoot, runId) call sites, not a single
endpoint. Initial patch covered 5 API sites; this extends the
fix to all 9 sites across API + TimeMachine.
New helper:
- src/FlowTime.Contracts/Storage/RunPathResolver.cs — static
GetSafeRunDirectory(artifactsDirectory, runId). Validates
non-null/non-whitespace, rejects path separators and "."/"..",
canonicalizes via Path.GetFullPath, enforces candidate strictly
under root via trailing-separator-aware StartsWith check.
Throws ArgumentException on any violation.
Located in Contracts so both API and TimeMachine consume it.
HTTP-layer helper:
- Program.TryResolveRunDirectory(artifactsDirectory, runId,
out runPath) → IResult? — returns 404 IResult on invalid or
missing runId, null on success. Collapses the endpoint
try/catch/exists pattern from ~10 lines per site to ~4.
Sites patched:
- API/Program.cs — /runs/{runId}/index, /series, POST /export,
GET /export/{format}
- API/Services/StateQueryService.cs — state, state_window
- API/Services/GraphService.cs — /runs/{runId}/graph
- API/Services/MetricsService.cs — /runs/{runId}/metrics
- API/Endpoints/RunOrchestrationEndpoints.cs — /runs/{runId}
- TimeMachine/Orchestration/RunOrchestrationService.cs — run-
reuse check + explicit-runId Directory.Move write path (this
one prevents a write-side traversal: without validation a
user-supplied RunId could relocate the just-written run
outside outputRoot)
- TimeMachine/TelemetryBundleBuilder.cs — explicit run directory
delete/write path
Tests:
- RunPathResolverTests.cs — 26 unit tests: valid, invalid input,
traversal attempts (.., ../other, ../../etc/passwd, absolute
paths), separator rejection, sibling-prefix false-positive
guard (root vs root-sibling), trailing-separator branch,
non-existence allowed.
- TryResolveRunDirectoryTests.cs — 8 tests on the HTTP helper:
existing dir → null, nonexistent → IResult, invalid runId →
IResult + empty path, invalid artifactsDirectory → IResult.
An earlier iteration added RunPathTraversalEndpointTests that
sent .. as a URL segment; ASP.NET's routing normalises those
before parameter binding so the tests would have passed even
without the fix. Deleted. The unit tests on the helper are the
real security assurance; endpoint wiring is a single helper
call at each site.
Also:
- .gitignore: add .claude/worktrees/
- RunPathResolver: static readonly char[] for path separators
(avoid per-call allocation)
Full solution: 1736 passed / 9 skipped / 0 failed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
23min
added a commit
that referenced
this pull request
May 1, 2026
…actions (m-E21-07 smoke-test polish)
Real-bytes manual smoke-test surfaced several UX gaps and one correctness bug
in the validation surface; this bundle ships the bug fix plus the chrome
adjustments needed for the surface to feel coherent against live data.
Bug fix (edge identity):
- Backend BuildEdgeWarnings keys edgeWarnings by analyser edge id (e.g.
'source_to_target' for the lag YAML), but the topology rendering, panel
edge rows, and workbench-edge-card lookup all expect the workbench's
'${from}→${to}' convention. Mocked Playwright spec #5 used hand-rolled
from→to keys so the gap stayed hidden.
- validation.setResponse(response, edges?) now accepts optional graph edge
metadata; when provided the store rewrites raw edgeWarnings keys to the
workbench convention before deriving rows + severity-max maps. Strips
':port' suffixes from graph endpoints (mirrors StateQueryService.
ExtractNodeReference) so SourceNode:out → TargetNode:in resolves to the
dag-map's data-edge-from='SourceNode' selector.
- Spec #2 (real-bytes) tightened to assert the edge indicator renders.
Topology indicator visuals:
- New triangleIndicatorPoints helper (vitest-covered, 3 cases) — upward-
pointing equilateral triangle around (cx, cy) with circumradius r.
- Edge indicator is now a polygon triangle (r=6) at the path midpoint;
reads sharply at small sizes against thin edge strokes. Edge stroke
unmodified — composes with --ft-viz-amber selected-edge highlight.
- Node indicator stays a circle but enlarges to r=3 (matching the
workbench-card severity dot's 6 px diameter) so the two surfaces feel
consistent.
Severity row tinting (light mode):
- New --ft-warn-bg / --ft-err-bg / --ft-info-bg chrome tokens (very
low-saturation hue washes in light mode; transparent in dark mode where
the dot + border carry the signal without a heavy fill).
- New severityRowBackground helper (vitest-covered, 5 cases) — symmetric
with severityChromeToken.
- Validation panel rows tinted by severity. Pinned warning-flagged cards
(node + edge) tinted by severity — bg-card swapped via class:bg-card.
Workbench card interactions:
- Selected card border now turquoise (--ft-highlight) — symmetric with the
validation-panel matching-row border treatment.
- Edge card gained a 'selected' prop; wired at the call site to
'last-pinned wins' so the cross-link convention from validation panel
and the visual selection convention stay aligned.
- Cards are now click-selectable: onSelect prop on both cards (role=button,
tabindex=0, Enter/Space keyboard handler); close button stops propagation.
Node card onSelect → setSelectedCell; edge card onSelect →
bringEdgeToFront. Same selection convention as topology-node click and
validation-row click.
- Sparkline moved above the metrics grid for visual primacy in the card.
Card list animations:
- animate:flip (220 ms) on each card wrapper so reorders / new pins sweep
into place; in:fade (160 ms) for new-card entry, out:fade (120 ms) for
close. Existing keys (pin.id, from→to) are FLIP's prerequisite.
Manual-verification recipe:
- New src/FlowTime.API/FlowTime.API.validation.http carries the lag-
trigger YAML for VS Code REST Client manual smoke-testing. Recommended
workflow: restart the API in test-runs mode (FLOWTIME_DATA_DIR=...
/test-runs) so the verification run lands in data/test-runs/ rather
than data/runs/.
ui-vitest 879 → 897 (+18 across topology-indicators, validation-helpers,
validation.svelte test suites). svelte-check baseline unchanged
(413 errors / 2 warnings). Tracking doc Decisions section updated with the
bug + the chrome adjustments captured during smoke-testing.
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
Closes the full class of
Path.Combine(artifactsRoot, runId)vulnerabilities flagged by ultrareview Finding 1 (work/gaps.md2026-04-20). Originally reported against a single endpoint; this PR applies a sharedGetSafeRunDirectoryhelper to all 9 sites across API + TimeMachine so the whole class is closed in one pass.New helper
FlowTime.Contracts.Storage.RunPathResolver.GetSafeRunDirectory(artifactsDirectory, runId):./..Path.GetFullPath+ trailing-separator-awareStartsWithroot checkFlowTime.Contractsso both API and TimeMachine consume the same helperHTTP-layer convenience
Program.TryResolveRunDirectory(artifactsDirectory, runId, out runPath)returnsIResult?(null on success, 404 on invalid/missing). Collapses the endpoint try/catch/exists pattern from ~10 lines per site to ~4.Sites closed
/v1/runs/{runId}/index,/series/{seriesId},POST /export,GET /export/{format}(APIProgram.cs)/v1/runs/{runId}/state,/state_window(Services/StateQueryService.cs)/v1/runs/{runId}/graph(Services/GraphService.cs)/v1/runs/{runId}/metrics(Services/MetricsService.cs)/v1/runs/{runId}(Endpoints/RunOrchestrationEndpoints.cs)TimeMachine/Orchestration/RunOrchestrationService.cs— run-reuse check and the explicit-runIdDirectory.Movewrite path (prevents user-supplied RunId from relocating the just-written run outsideoutputRoot)TimeMachine/TelemetryBundleBuilder.cs— explicit run directory delete/write pathTests
RunPathResolverTests— 26 unit tests covering valid paths, invalid input, traversal attempts (..,../other,../../etc/passwd, absolute paths), separator rejection, sibling-prefix false-positive guard (rootvsroot-sibling), trailing-separator branch, non-existence allowedTryResolveRunDirectoryTests— 8 tests on the HTTP-layer helperFull solution: 1736 passed / 9 skipped / 0 failed.
Test plan
dotnet build FlowTime.slncleandotnet test FlowTime.slnpasses🤖 Generated with Claude Code