mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
c8bad6bcb4
commit
e70f0eeefc
@@ -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<string> = [],
|
||||
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/<projectId>/…`) 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<string>,
|
||||
files: readonly ProjectFile[],
|
||||
projectId?: string,
|
||||
projectRoot?: string | null,
|
||||
): Set<string> {
|
||||
const names = new Set<string>();
|
||||
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;
|
||||
|
||||
@@ -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/<id>/ 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([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user