Skip to content

Commit 90d9e1b

Browse files
committed
fix(ui): thread doc-base through code-path validator
Out-of-tree linked docs (and annotate-mode files outside cwd) reference files relative to themselves. The validator was resolving against cwd only, so those paths got marked missing and the renderer demoted them to plain text — even though clicks still resolved correctly via base. Also tightens the suffix-match's leading-segment strip so `../foo.ts` no longer silently misresolves to an unrelated `foo.ts` in cwd. Cleanup: delete unused extract-code-paths import in reference-handlers, add the export entry to packages/shared so consumers don't rely on Bun's lenient subpath resolution. Add TODO(security) comments at both handleDocExists sites flagging that absolute paths bypass project-root containment. 223 tests pass (3 new resolver cases for baseDir + ../ regression).
1 parent 9516fc8 commit 90d9e1b

8 files changed

Lines changed: 95 additions & 11 deletions

File tree

apps/pi-extension/server/reference.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,11 @@ export async function handleDocRequest(res: Res, url: URL): Promise<void> {
201201
/**
202202
* Batch existence check for code-file paths the renderer wants to linkify.
203203
* POST /api/doc/exists with { paths: string[] }.
204+
*
205+
* TODO(security): see packages/server/reference-handlers.ts handleDocExists —
206+
* absolute paths are probed verbatim with no project-root containment check,
207+
* leaking file existence back to the caller. Fix in lockstep with the Bun
208+
* handler (reject absolute inputs or filter results via isWithinProjectRoot).
204209
*/
205210
export async function handleDocExistsRequest(res: Res, req: IncomingMessage): Promise<void> {
206211
const body = await parseBody(req);
@@ -213,6 +218,10 @@ export async function handleDocExistsRequest(res: Res, req: IncomingMessage): Pr
213218
json(res, { error: "Too many paths (max 500)" }, 400);
214219
return;
215220
}
221+
const baseRaw = (body as { base?: unknown }).base;
222+
const baseDir = typeof baseRaw === "string" && baseRaw.length > 0
223+
? resolveUserPath(baseRaw)
224+
: undefined;
216225

217226
const projectRoot = process.cwd();
218227
const results: Record<
@@ -225,7 +234,7 @@ export async function handleDocExistsRequest(res: Res, req: IncomingMessage): Pr
225234

226235
await Promise.all(
227236
(paths as string[]).map(async (p) => {
228-
const r = await resolveCodeFile(p, projectRoot);
237+
const r = await resolveCodeFile(p, projectRoot, baseDir);
229238
if (r.kind === "found") {
230239
results[p] = { status: "found", resolved: r.path };
231240
} else if (r.kind === "ambiguous") {

packages/editor/App.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1933,6 +1933,9 @@ const App: React.FC = () => {
19331933
onOpenCodeFile={codeFilePopout.open}
19341934
linkedDocInfo={linkedDocHook.isActive ? { filepath: linkedDocHook.filepath!, onBack: handleLinkedDocBack, label: fileBrowser.dirs.find(d => d.path === fileBrowser.activeDirPath)?.isVault ? 'Vault File' : fileBrowser.activeFile ? 'File' : undefined, backLabel } : null}
19351935
imageBaseDir={imageBaseDir}
1936+
codePathBaseDir={linkedDocHook.filepath
1937+
? linkedDocHook.filepath.replace(/\/[^/]+$/, '')
1938+
: imageBaseDir}
19361939
copyLabel={annotateSource === 'message' ? 'Copy message' : annotateSource === 'file' || annotateSource === 'folder' ? 'Copy file' : undefined}
19371940
archiveInfo={archive.currentInfo}
19381941
sourceInfo={sourceInfo}

packages/server/reference-handlers.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import {
1818
isWithinProjectRoot,
1919
warmFileListCache,
2020
} from "@plannotator/shared/resolve-file";
21-
import { extractCandidateCodePaths } from "@plannotator/shared/extract-code-paths";
2221
import { htmlToMarkdown } from "@plannotator/shared/html-to-markdown";
2322
import { preloadFile } from "@pierre/diffs/ssr";
2423

@@ -172,6 +171,14 @@ export async function handleDoc(req: Request): Promise<Response> {
172171
* Batch existence check for code-file paths the renderer wants to linkify.
173172
* POST /api/doc/exists with { paths: string[] } returns { results: { [path]: ValidationEntry } }.
174173
* Reads from the warm file-list cache populated at plan/annotate load.
174+
*
175+
* TODO(security): absolute paths sent here are probed verbatim against the
176+
* filesystem — `resolveCodeFile` returns `{ kind: 'found', path: abs }` for any
177+
* existing absolute file with no project-root containment check. A malicious
178+
* shared plan with backtick-wrapped absolute paths (e.g. `/Users/x/.aws/…`)
179+
* leaks file existence + canonical path back to the caller. Mitigation:
180+
* reject absolute inputs (or `isWithinProjectRoot`-filter `r.path`) before
181+
* recording a found result. Mirror in apps/pi-extension/server/reference.ts.
175182
*/
176183
export async function handleDocExists(req: Request): Promise<Response> {
177184
let body: unknown;
@@ -187,6 +194,10 @@ export async function handleDocExists(req: Request): Promise<Response> {
187194
if (paths.length > 500) {
188195
return Response.json({ error: "Too many paths (max 500)" }, { status: 400 });
189196
}
197+
const baseRaw = (body as { base?: unknown })?.base;
198+
const baseDir = typeof baseRaw === "string" && baseRaw.length > 0
199+
? resolveUserPath(baseRaw)
200+
: undefined;
190201

191202
const projectRoot = process.cwd();
192203
const results: Record<
@@ -199,7 +210,7 @@ export async function handleDocExists(req: Request): Promise<Response> {
199210

200211
await Promise.all(
201212
(paths as string[]).map(async (p) => {
202-
const r = await resolveCodeFile(p, projectRoot);
213+
const r = await resolveCodeFile(p, projectRoot, baseDir);
203214
if (r.kind === "found") {
204215
results[p] = { status: "found", resolved: r.path };
205216
} else if (r.kind === "ambiguous") {

packages/shared/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
"./favicon": "./favicon.ts",
2222
"./code-file": "./code-file.ts",
2323
"./resolve-file": "./resolve-file.ts",
24+
"./extract-code-paths": "./extract-code-paths.ts",
2425
"./external-annotation": "./external-annotation.ts",
2526
"./agent-jobs": "./agent-jobs.ts",
2627
"./config": "./config.ts",

packages/shared/resolve-file.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,4 +82,32 @@ describe("resolveCodeFile", () => {
8282
expect(r.path).toBe(join(root, "packages/editor/App.tsx"));
8383
}
8484
});
85+
86+
test("does NOT strip leading ../ — without baseDir, refuses to fabricate", async () => {
87+
// `../foo.tsx` is meaningful (escape parent). With no baseDir context,
88+
// we can't honor it, so we must fail rather than silently returning
89+
// an unrelated file with the same basename from inside cwd.
90+
const r = await resolveCodeFile("../editor/App.tsx", root);
91+
expect(r.kind).toBe("not_found");
92+
});
93+
94+
test("resolves via baseDir when input is relative to active doc", async () => {
95+
// Linked doc at `<root>/packages/review-editor/...` references `../editor/App.tsx`
96+
const baseDir = join(root, "packages/review-editor");
97+
const r = await resolveCodeFile("../editor/App.tsx", root, baseDir);
98+
expect(r.kind).toBe("found");
99+
if (r.kind === "found") {
100+
expect(r.path).toBe(join(root, "packages/editor/App.tsx"));
101+
}
102+
});
103+
104+
test("baseDir miss falls through to suffix walk", async () => {
105+
// baseDir doesn't have the file, but cwd tree does — walk catches it.
106+
const baseDir = join(root, "packages/review-editor");
107+
const r = await resolveCodeFile("ui/components/Button.tsx", root, baseDir);
108+
expect(r.kind).toBe("found");
109+
if (r.kind === "found") {
110+
expect(r.path).toBe(join(root, "packages/ui/components/Button.tsx"));
111+
}
112+
});
85113
});

packages/shared/resolve-file.ts

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -281,15 +281,23 @@ export function warmFileListCache(
281281
* Strategies:
282282
* 1. Absolute path → use as-is.
283283
* 2. Exact relative from project root.
284-
* 3. Case-insensitive suffix match against the cached file list:
284+
* 3. If `baseDir` provided, literal `<baseDir>/<input>` existence check —
285+
* lets out-of-tree linked docs resolve their own relative references
286+
* (e.g. `../script.ts` in `~/notes/foo.md` finds `~/script.ts`).
287+
* 4. Case-insensitive suffix match against the cached file list:
285288
* - bare basename input → match any file with that basename;
286289
* - input with `/` → match files whose path equals or ends with `/<input>`
287290
* on a segment boundary (so `editor/App.tsx` matches `packages/editor/App.tsx`
288291
* but not `myeditor/App.tsx`).
292+
*
293+
* `..` segments in the input are honored: only `./` is stripped before suffix
294+
* matching. `../foo.ts` without a `baseDir` correctly falls through to
295+
* not_found rather than fabricating a match against `foo.ts` somewhere in cwd.
289296
*/
290297
export async function resolveCodeFile(
291298
input: string,
292299
projectRoot: string,
300+
baseDir?: string,
293301
): Promise<ResolveResult> {
294302
const originalInput = input.trim();
295303
const unquotedInput = stripWrappingQuotes(originalInput);
@@ -313,15 +321,25 @@ export async function resolveCodeFile(
313321
return { kind: "found", path: fromRoot };
314322
}
315323

324+
if (baseDir) {
325+
const fromBase = resolve(baseDir, searchInput);
326+
if (fileExists(fromBase)) {
327+
return { kind: "found", path: fromBase };
328+
}
329+
}
330+
316331
const fileList = await warmFileListCache(projectRoot, "code");
317332
if (fileList === null) {
318333
return { kind: "unavailable", input: originalInput };
319334
}
320335

321-
// Strip leading `./` or `../` so suffix matching works on inputs like
322-
// `./editor/App.tsx` — file list entries never carry those segments.
323-
const cleanedInput = searchInput.replace(/^(?:\.\.?\/)+/, "");
324-
if (!cleanedInput) {
336+
// Strip leading `./` so suffix matching works on inputs like
337+
// `./editor/App.tsx` — file list entries never carry that segment.
338+
// `../` is intentionally NOT stripped: `..` is meaningful (escape parent),
339+
// not noise. If we can't honor it via baseDir, the input has no
340+
// suffix-match equivalent in the in-tree file list.
341+
const cleanedInput = searchInput.replace(/^(?:\.\/)+/, "");
342+
if (!cleanedInput || cleanedInput.startsWith("../")) {
325343
return { kind: "not_found", input: originalInput };
326344
}
327345
const target = cleanedInput.toLowerCase();

packages/ui/components/Viewer.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,10 @@ interface ViewerProps {
6868
onOpenLinkedDoc?: (path: string) => void;
6969
onOpenCodeFile?: (path: string) => void;
7070
imageBaseDir?: string;
71+
/** Directory the active document lives in — used by the code-path validator
72+
* so out-of-tree relative references (e.g. `../foo.ts` in a linked doc)
73+
* resolve against the doc's own directory rather than only cwd. */
74+
codePathBaseDir?: string;
7175
linkedDocInfo?: { filepath: string; onBack: () => void; label?: string; backLabel?: string } | null;
7276
// Plan diff props
7377
planDiffStats?: { additions: number; deletions: number; modifications: number } | null;
@@ -159,6 +163,7 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
159163
onOpenCodeFile,
160164
linkedDocInfo,
161165
imageBaseDir,
166+
codePathBaseDir,
162167
copyLabel,
163168
actionsLabelMode = 'full',
164169
archiveInfo,
@@ -512,7 +517,7 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
512517
setViewerCommentPopover(null);
513518
}, []);
514519

515-
const codePathValidation = useValidatedCodePaths(markdown);
520+
const codePathValidation = useValidatedCodePaths(markdown, codePathBaseDir);
516521

517522
return (
518523
<CodePathValidationContext.Provider value={codePathValidation}>

packages/ui/hooks/useValidatedCodePaths.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,16 @@ export type ValidatedMap = Map<string, ValidationEntry>;
1919
* renderer dispatches on status — see InlineMarkdown.
2020
*
2121
* Empty candidate set short-circuits — no fetch, ready: true immediately.
22+
*
23+
* `baseDir` is the directory the active document lives in (linked-doc parent
24+
* or the annotate source file's parent). When set, the server tries
25+
* `<baseDir>/<input>` literal-resolve before its cwd walk so out-of-tree
26+
* relative references (e.g. `../script.ts` in `~/notes/foo.md`) don't get
27+
* demoted to plain text.
2228
*/
2329
export function useValidatedCodePaths(
2430
markdown: string,
31+
baseDir?: string,
2532
): { validated: ValidatedMap; ready: boolean } {
2633
const [validated, setValidated] = useState<ValidatedMap>(new Map());
2734
const [ready, setReady] = useState<boolean>(false);
@@ -42,7 +49,9 @@ export function useValidatedCodePaths(
4249
const res = await fetch("/api/doc/exists", {
4350
method: "POST",
4451
headers: { "Content-Type": "application/json" },
45-
body: JSON.stringify({ paths: candidates }),
52+
body: JSON.stringify(
53+
baseDir ? { paths: candidates, base: baseDir } : { paths: candidates },
54+
),
4655
});
4756
if (cancelled) return;
4857
if (!res.ok) {
@@ -67,7 +76,7 @@ export function useValidatedCodePaths(
6776
return () => {
6877
cancelled = true;
6978
};
70-
}, [markdown]);
79+
}, [markdown, baseDir]);
7180

7281
// Stable reference: only changes when validated/ready actually change.
7382
// Without memoization, the parent provider's value is a fresh object every

0 commit comments

Comments
 (0)