From e70f0eeefc7527c5928ded4b64585d00a72b42e8 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Thu, 16 Jul 2026 14:47:50 +0800 Subject: [PATCH] fix(web): contain absolute tool paths to the project root when resolving touched files (#5352) (#5577) attribution, restricted to agent-touched files resolved through findTouchedProjectFile. Two residual gaps in that matching: - A ROOT-level project file (candidate contains no '/') never suffix-matches an absolute event path like /workspace/index.html, so a mixed turn that rewrites index.html plus any resolvable file drops the rewritten HTML from the touched set and nothing auto-opens. - An out-of-root absolute path sharing a tail with a project file (/tmp/site/index.html, or a '..' escape resolving outside the root) CAN suffix-match and mis-attribute an external write. findTouchedProjectFile now takes the project's resolvedDir: absolute paths are lexically normalized ('.'/'..') and must sit under the root, in which case they match by their project-relative form; out-of-root paths (including '..' escapes) are rejected before any suffix matching. Paths whose '..' climbs above their own anchor are always rejected. Callers without a usable root keep the legacy behavior. Red on main / green here: apps/web/tests/components/ProjectView.touched-path-containment.test.ts (6 of 10 cases fail on main: both root-file misses and out-of-root false positives). Claude-Session: https://claude.ai/code/session_017peXvs7Q3sA5dhZ6vsYYwN Co-authored-by: Claude Fable 5 --- apps/web/src/components/ProjectView.tsx | 66 ++++++- ...ojectView.touched-path-containment.test.ts | 168 ++++++++++++++++++ 2 files changed, 230 insertions(+), 4 deletions(-) create mode 100644 apps/web/tests/components/ProjectView.touched-path-containment.test.ts diff --git a/apps/web/src/components/ProjectView.tsx b/apps/web/src/components/ProjectView.tsx index b71175f398..174b923edd 100644 --- a/apps/web/src/components/ProjectView.tsx +++ b/apps/web/src/components/ProjectView.tsx @@ -3918,6 +3918,7 @@ export function ProjectView({ nextFiles, touchedFilePaths, project.id, + projectDetail.resolvedDir, ) ?? [], recoveredExistingArtifact, ); @@ -3929,6 +3930,7 @@ export function ProjectView({ touchedFilePaths, nextFiles, project.id, + projectDetail.resolvedDir, ), }); if (producedArtifactToOpen) requestOpenFile(producedArtifactToOpen); @@ -4240,6 +4242,7 @@ export function ProjectView({ nextFiles, touchedFilePaths, project.id, + projectDetail.resolvedDir, ) ?? [], recoveredExistingArtifact, ); @@ -4251,6 +4254,7 @@ export function ProjectView({ touchedFilePaths, nextFiles, project.id, + projectDetail.resolvedDir, ), }); if (producedArtifactToOpen) requestOpenFile(producedArtifactToOpen); @@ -5679,6 +5683,7 @@ export function ProjectView({ nextFiles, traceTouchedFilePaths, project.id, + projectDetail.resolvedDir, ) ?? []; const producedArtifactToOpen = selectAutoOpenTurnArtifact(produced, nextFiles, { ...autoOpenArtifactOptions, @@ -5688,6 +5693,7 @@ export function ProjectView({ traceTouchedFilePaths, nextFiles, project.id, + projectDetail.resolvedDir, ), }); if (producedArtifactToOpen) requestOpenFile(producedArtifactToOpen); @@ -9745,6 +9751,7 @@ export function computeTraceObjectFiles( next: readonly ProjectFile[], touchedPaths: Iterable = [], projectId?: string, + projectRoot?: string | null, ): ProjectFile[] | undefined { if (!beforeNames) return undefined; const set = beforeNames instanceof Set ? beforeNames : new Set(beforeNames); @@ -9753,7 +9760,7 @@ export function computeTraceObjectFiles( byName.set(file.name, { ...file, traceObjectReason: 'new' }); } for (const rawPath of touchedPaths) { - const file = findTouchedProjectFile(rawPath, next, projectId); + const file = findTouchedProjectFile(rawPath, next, projectId, projectRoot); if (!file) continue; byName.set(file.name, { ...file, @@ -9767,10 +9774,40 @@ function findTouchedProjectFile( rawPath: string, files: readonly ProjectFile[], projectId?: string, + projectRoot?: string | null, ): ProjectFile | null { - const normalized = normalizeComparableFilePath(rawPath); - if (!normalized) return null; + const slashed = rawPath.replace(/\\/g, '/'); + // Lexically resolve `.`/`..` first: a path whose `..` climbs above its own + // anchor can never be proven to stay anywhere, so it is rejected outright — + // before any suffix matching could pair it with an in-project file. + const segments = lexicallyNormalizePathSegments(slashed); + if (!segments || segments.length === 0) return null; + let normalized = segments.join('/'); + // A managed-project alias (`…/projects//…`) identifies the file's + // project-relative form regardless of where the alias mount lives, so it is + // trusted as-is; containment below only anchors paths without that marker. const managedProjectRelativePath = relativePathFromManagedProjectAlias(normalized, projectId); + if (!managedProjectRelativePath && isAbsoluteToolPath(slashed)) { + const rootSegments = projectRoot + ? lexicallyNormalizePathSegments(projectRoot.replace(/\\/g, '/')) + : null; + if (rootSegments && rootSegments.length > 0) { + // An absolute tool path is only trusted when it provably lives under + // the project root: require the root's segments as a prefix and match + // on the remaining project-relative form (/workspace/index.html → + // index.html). Out-of-root paths (including `..` escapes that resolve + // outside the root) are rejected here rather than falling through to + // suffix matching, where /tmp/site/index.html could otherwise pick the + // project's own index.html. + if (segments.length <= rootSegments.length) return null; + for (let i = 0; i < rootSegments.length; i += 1) { + if (segments[i] !== rootSegments[i]) return null; + } + normalized = segments.slice(rootSegments.length).join('/'); + } + // Without a usable root there is no anchor to judge containment against; + // keep the legacy suffix behavior below. + } const comparablePaths = managedProjectRelativePath ? [normalized, managedProjectRelativePath] : [normalized]; @@ -9833,6 +9870,26 @@ function normalizeComparableFilePath(value: string): string { .join('/'); } +// Lexically resolve `.`/`..` segments. Returns null when a `..` climbs above +// the path's own anchor — such a path cannot be proven to resolve anywhere. +function lexicallyNormalizePathSegments(path: string): string[] | null { + const out: string[] = []; + for (const part of path.split('/')) { + if (!part || part === '.') continue; + if (part === '..') { + if (out.length === 0) return null; + out.pop(); + continue; + } + out.push(part); + } + return out; +} + +function isAbsoluteToolPath(path: string): boolean { + return path.startsWith('/') || /^[A-Za-z]:\//.test(path); +} + // Resolve the agent's raw Write/Edit tool paths (absolute or partial) to // project file NAMES for selectAutoOpenTurnArtifact's touched-file // restriction. Paths that do not resolve to a project file (out-of-project @@ -9842,10 +9899,11 @@ export function resolveAgentTouchedFileNames( touchedPaths: Iterable, files: readonly ProjectFile[], projectId?: string, + projectRoot?: string | null, ): Set { const names = new Set(); for (const rawPath of touchedPaths) { - const file = findTouchedProjectFile(rawPath, files, projectId); + const file = findTouchedProjectFile(rawPath, files, projectId, projectRoot); if (file) names.add(file.name); } return names; diff --git a/apps/web/tests/components/ProjectView.touched-path-containment.test.ts b/apps/web/tests/components/ProjectView.touched-path-containment.test.ts new file mode 100644 index 0000000000..d055cd5d2d --- /dev/null +++ b/apps/web/tests/components/ProjectView.touched-path-containment.test.ts @@ -0,0 +1,168 @@ +// Absolute tool-path containment for touched-file resolution (#5352 follow-up). +// +// The turn-end auto-open selector restricts mtime-window candidates to files +// the agent's Write/Edit events touched, resolved through the same matching +// as trace attribution. Two gaps in that matching: +// +// 1. A ROOT-level project file (candidate has no `/`) never suffix-matched an +// absolute event path like /workspace/index.html, so a mixed turn that +// rewrote index.html plus any resolvable file excluded the rewritten HTML +// from the touched set — and nothing auto-opened. +// 2. An out-of-root absolute path that merely shares a tail with a project +// file (/tmp/site/index.html, or a `..` escape resolving outside the +// root) COULD suffix-match and mis-attribute an external write to an +// in-project file. +// +// With the project root threaded in, absolute paths resolve to their +// project-relative form when contained and are rejected outright otherwise. +import { describe, expect, it } from 'vitest'; +import { + computeTraceObjectFiles, + resolveAgentTouchedFileNames, +} from '../../src/components/ProjectView'; +import { selectAutoOpenTurnArtifact } from '../../src/components/auto-open-file'; + +const ROOT = '/workspace/project'; + +interface FileSeed { + name: string; + path?: string; + kind?: string; + mtime?: number; + type?: string; +} + +const file = (seed: FileSeed) => ({ type: 'file', ...seed }) as any; + +const rootHtml = file({ name: 'index.html', path: 'index.html', kind: 'html', mtime: 1_005_000 }); +const subJs = file({ name: 'app.js', path: 'assets/app.js', kind: 'text', mtime: 1_004_000 }); +const planMd = file({ name: 'plan.md', path: 'plan.md', kind: 'text', mtime: 940_000 }); + +describe('resolveAgentTouchedFileNames — absolute paths under the project root', () => { + it('resolves an absolute path to a ROOT-level project file', () => { + const names = resolveAgentTouchedFileNames( + [`${ROOT}/index.html`], + [rootHtml, planMd], + undefined, + ROOT, + ); + expect([...names]).toEqual(['index.html']); + }); + + it('resolves an absolute path to a subdirectory project file', () => { + const names = resolveAgentTouchedFileNames( + [`${ROOT}/assets/app.js`], + [subJs, planMd], + undefined, + ROOT, + ); + expect([...names]).toEqual(['app.js']); + }); + + it('resolves `..` segments that stay inside the root', () => { + const names = resolveAgentTouchedFileNames( + [`${ROOT}/assets/../index.html`], + [rootHtml, planMd], + undefined, + ROOT, + ); + expect([...names]).toEqual(['index.html']); + }); +}); + +describe('resolveAgentTouchedFileNames — out-of-root rejection before suffix matching', () => { + it('rejects an out-of-root absolute path that shares a tail with a project file', () => { + const names = resolveAgentTouchedFileNames( + ['/tmp/site/assets/app.js'], + [subJs, planMd], + undefined, + ROOT, + ); + expect(names.size).toBe(0); + }); + + it('rejects a `..` escape that resolves outside the root', () => { + const names = resolveAgentTouchedFileNames( + [`${ROOT}/../elsewhere/assets/app.js`], + [subJs, planMd], + undefined, + ROOT, + ); + expect(names.size).toBe(0); + }); + + it('rejects a path whose `..` climbs above its own anchor', () => { + const names = resolveAgentTouchedFileNames( + ['/../etc/passwd'], + [rootHtml], + undefined, + ROOT, + ); + expect(names.size).toBe(0); + }); + + it('keeps legacy suffix matching when no root is known', () => { + const names = resolveAgentTouchedFileNames( + ['/anywhere/assets/app.js'], + [subJs, planMd], + undefined, + null, + ); + expect([...names]).toEqual(['app.js']); + }); +}); + +describe('resolveAgentTouchedFileNames — managed-project alias paths', () => { + it('trusts a projects// alias path even when it lives outside the resolved root', () => { + const names = resolveAgentTouchedFileNames( + ['/mnt/managed/projects/proj-1/index.html'], + [rootHtml, planMd], + 'proj-1', + ROOT, + ); + expect([...names]).toEqual(['index.html']); + }); +}); + +describe('turn-end auto-open — mixed turn rewriting a root HTML file', () => { + it('opens the rewritten root index.html when the turn also touched a resolvable file', () => { + const touched = resolveAgentTouchedFileNames( + [`${ROOT}/index.html`, `${ROOT}/assets/app.js`], + [rootHtml, subJs, planMd], + undefined, + ROOT, + ); + const pick = selectAutoOpenTurnArtifact([], [rootHtml, subJs, planMd], { + turnStartedAt: 1_000_000, + turnEndedAt: 1_030_000, + agentTouchedFileNames: touched, + }); + expect(pick).toBe('index.html'); + }); +}); + +describe('computeTraceObjectFiles — root-file trace attribution', () => { + it('marks a rewritten root file as modified from its absolute tool path', () => { + const trace = computeTraceObjectFiles( + new Set(['index.html', 'plan.md']), + [rootHtml, planMd], + [`${ROOT}/index.html`], + undefined, + ROOT, + ); + expect(trace?.map((f) => [f.name, f.traceObjectReason])).toEqual([ + ['index.html', 'modified'], + ]); + }); + + it('does not attribute an out-of-root write with a matching basename', () => { + const trace = computeTraceObjectFiles( + new Set(['index.html', 'plan.md']), + [rootHtml, planMd], + ['/tmp/site/index.html'], + undefined, + ROOT, + ); + expect(trace).toEqual([]); + }); +});