Skip to content

Commit 4033b4e

Browse files
betegonclaude
andauthored
fix(init): use opendir for listDir and validate symlinks during traversal (#663)
## Summary - Replace the recursive `readdirSync` walk in `listDir` with `fs.promises.opendir({ recursive, bufferSize: 1024 })` for cleaner incremental iteration - Add symlink validation via `safePath` on each entry so symlinks pointing outside the project directory are excluded from listings — closes the gap where other ops (`readFiles`, `fileExistsBatch`, `applyPatchset`) already validated but `listDir` did not - `precomputeDirListing` is now async to match Addresses [this review comment](#657 (comment)). ## Test plan - [x] Existing `list-dir` tests pass (files, directories, maxEntries, recursive, skip node_modules/dot-dirs, maxDepth) - [x] New test: symlinks pointing outside project directory are excluded - [x] `precomputeDirListing` tests updated for async - [x] `wizard-runner` tests updated for async mock - [x] `tsc --noEmit` passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 6284fd5 commit 4033b4e

File tree

4 files changed

+184
-96
lines changed

4 files changed

+184
-96
lines changed

src/lib/init/local-ops.ts

Lines changed: 132 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -284,8 +284,10 @@ function safePath(cwd: string, relative: string): string {
284284
* Pre-compute directory listing before the first API call.
285285
* Uses the same parameters the server's discover-context step would request.
286286
*/
287-
export function precomputeDirListing(directory: string): DirEntry[] {
288-
const result = listDir({
287+
export async function precomputeDirListing(
288+
directory: string
289+
): Promise<DirEntry[]> {
290+
const result = await listDir({
289291
type: "local-op",
290292
operation: "list-dir",
291293
cwd: directory,
@@ -344,7 +346,7 @@ export async function handleLocalOp(
344346
}
345347
}
346348

347-
function listDir(payload: ListDirPayload): LocalOpResult {
349+
async function listDir(payload: ListDirPayload): Promise<LocalOpResult> {
348350
const { cwd, params } = payload;
349351
const targetPath = safePath(cwd, params.path);
350352
const maxDepth = params.maxDepth ?? 3;
@@ -353,15 +355,15 @@ function listDir(payload: ListDirPayload): LocalOpResult {
353355

354356
const entries: DirEntry[] = [];
355357

356-
// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: walking the directory tree is a complex operation
357-
function walk(dir: string, depth: number): void {
358+
// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: recursive directory walk is inherently complex but straightforward
359+
async function walk(dir: string, depth: number): Promise<void> {
358360
if (entries.length >= maxEntries || depth > maxDepth) {
359361
return;
360362
}
361363

362364
let dirEntries: fs.Dirent[];
363365
try {
364-
dirEntries = fs.readdirSync(dir, { withFileTypes: true });
366+
dirEntries = await fs.promises.readdir(dir, { withFileTypes: true });
365367
} catch {
366368
return;
367369
}
@@ -372,77 +374,111 @@ function listDir(payload: ListDirPayload): LocalOpResult {
372374
}
373375

374376
const relPath = path.relative(cwd, path.join(dir, entry.name));
377+
378+
// Skip symlinks that escape the project directory
379+
if (entry.isSymbolicLink()) {
380+
try {
381+
safePath(cwd, relPath);
382+
} catch {
383+
continue;
384+
}
385+
}
386+
375387
const type = entry.isDirectory() ? "directory" : "file";
376388
entries.push({ name: entry.name, path: relPath, type });
377389

378390
if (
379391
recursive &&
380392
entry.isDirectory() &&
393+
!entry.isSymbolicLink() &&
381394
!entry.name.startsWith(".") &&
382395
entry.name !== "node_modules"
383396
) {
384-
walk(path.join(dir, entry.name), depth + 1);
397+
await walk(path.join(dir, entry.name), depth + 1);
385398
}
386399
}
387400
}
388401

389-
walk(targetPath, 0);
402+
await walk(targetPath, 0);
390403
return { ok: true, data: { entries } };
391404
}
392405

393-
function readFiles(payload: ReadFilesPayload): LocalOpResult {
394-
const { cwd, params } = payload;
395-
const maxBytes = params.maxBytes ?? MAX_FILE_BYTES;
396-
const files: Record<string, string | null> = {};
397-
398-
for (const filePath of params.paths) {
399-
try {
400-
const absPath = safePath(cwd, filePath);
401-
const stat = fs.statSync(absPath);
402-
let content: string;
403-
if (stat.size > maxBytes) {
404-
// Read only up to maxBytes
406+
async function readSingleFile(
407+
cwd: string,
408+
filePath: string,
409+
maxBytes: number
410+
): Promise<string | null> {
411+
try {
412+
const absPath = safePath(cwd, filePath);
413+
const stat = await fs.promises.stat(absPath);
414+
let content: string;
415+
if (stat.size > maxBytes) {
416+
const fh = await fs.promises.open(absPath, "r");
417+
try {
405418
const buffer = Buffer.alloc(maxBytes);
406-
const fd = fs.openSync(absPath, "r");
407-
try {
408-
fs.readSync(fd, buffer, 0, maxBytes, 0);
409-
} finally {
410-
fs.closeSync(fd);
411-
}
419+
await fh.read(buffer, 0, maxBytes, 0);
412420
content = buffer.toString("utf-8");
413-
} else {
414-
content = fs.readFileSync(absPath, "utf-8");
421+
} finally {
422+
await fh.close();
415423
}
424+
} else {
425+
content = await fs.promises.readFile(absPath, "utf-8");
426+
}
416427

417-
// Minify JSON files by stripping whitespace/formatting
418-
if (filePath.endsWith(".json")) {
419-
try {
420-
content = JSON.stringify(JSON.parse(content));
421-
} catch {
422-
// Not valid JSON (truncated, JSONC, etc.) — send as-is
423-
}
428+
// Minify JSON files by stripping whitespace/formatting
429+
if (filePath.endsWith(".json")) {
430+
try {
431+
content = JSON.stringify(JSON.parse(content));
432+
} catch {
433+
// Not valid JSON (truncated, JSONC, etc.) — send as-is
424434
}
425-
426-
files[filePath] = content;
427-
} catch {
428-
files[filePath] = null;
429435
}
436+
437+
return content;
438+
} catch {
439+
return null;
440+
}
441+
}
442+
443+
async function readFiles(payload: ReadFilesPayload): Promise<LocalOpResult> {
444+
const { cwd, params } = payload;
445+
const maxBytes = params.maxBytes ?? MAX_FILE_BYTES;
446+
447+
const results = await Promise.all(
448+
params.paths.map(async (filePath) => {
449+
const content = await readSingleFile(cwd, filePath, maxBytes);
450+
return [filePath, content] as const;
451+
})
452+
);
453+
454+
const files: Record<string, string | null> = {};
455+
for (const [filePath, content] of results) {
456+
files[filePath] = content;
430457
}
431458

432459
return { ok: true, data: { files } };
433460
}
434461

435-
function fileExistsBatch(payload: FileExistsBatchPayload): LocalOpResult {
462+
async function fileExistsBatch(
463+
payload: FileExistsBatchPayload
464+
): Promise<LocalOpResult> {
436465
const { cwd, params } = payload;
437-
const exists: Record<string, boolean> = {};
438466

439-
for (const filePath of params.paths) {
440-
try {
441-
const absPath = safePath(cwd, filePath);
442-
exists[filePath] = fs.existsSync(absPath);
443-
} catch {
444-
exists[filePath] = false;
445-
}
467+
const results = await Promise.all(
468+
params.paths.map(async (filePath) => {
469+
try {
470+
const absPath = safePath(cwd, filePath);
471+
await fs.promises.access(absPath);
472+
return [filePath, true] as const;
473+
} catch {
474+
return [filePath, false] as const;
475+
}
476+
})
477+
);
478+
479+
const exists: Record<string, boolean> = {};
480+
for (const [filePath, found] of results) {
481+
exists[filePath] = found;
446482
}
447483

448484
return { ok: true, data: { exists } };
@@ -580,24 +616,56 @@ function applyPatchsetDryRun(payload: ApplyPatchsetPayload): LocalOpResult {
580616
* indentation style is detected and preserved. For `create` actions, a default
581617
* of 2-space indentation is used.
582618
*/
583-
function resolvePatchContent(
619+
async function resolvePatchContent(
584620
absPath: string,
585621
patch: ApplyPatchsetPayload["params"]["patches"][number]
586-
): string {
622+
): Promise<string> {
587623
if (!patch.path.endsWith(".json")) {
588624
return patch.patch;
589625
}
590626
if (patch.action === "modify") {
591-
const existing = fs.readFileSync(absPath, "utf-8");
627+
const existing = await fs.promises.readFile(absPath, "utf-8");
592628
return prettyPrintJson(patch.patch, detectJsonIndent(existing));
593629
}
594630
return prettyPrintJson(patch.patch, DEFAULT_JSON_INDENT);
595631
}
596632

597-
function applyPatchset(
633+
type Patch = ApplyPatchsetPayload["params"]["patches"][number];
634+
635+
const VALID_PATCH_ACTIONS = new Set(["create", "modify", "delete"]);
636+
637+
async function applySinglePatch(absPath: string, patch: Patch): Promise<void> {
638+
switch (patch.action) {
639+
case "create": {
640+
await fs.promises.mkdir(path.dirname(absPath), { recursive: true });
641+
const content = await resolvePatchContent(absPath, patch);
642+
await fs.promises.writeFile(absPath, content, "utf-8");
643+
break;
644+
}
645+
case "modify": {
646+
const content = await resolvePatchContent(absPath, patch);
647+
await fs.promises.writeFile(absPath, content, "utf-8");
648+
break;
649+
}
650+
case "delete": {
651+
try {
652+
await fs.promises.unlink(absPath);
653+
} catch (err) {
654+
if ((err as NodeJS.ErrnoException).code !== "ENOENT") {
655+
throw err;
656+
}
657+
}
658+
break;
659+
}
660+
default:
661+
break;
662+
}
663+
}
664+
665+
async function applyPatchset(
598666
payload: ApplyPatchsetPayload,
599667
dryRun?: boolean
600-
): LocalOpResult {
668+
): Promise<LocalOpResult> {
601669
if (dryRun) {
602670
return applyPatchsetDryRun(payload);
603671
}
@@ -607,56 +675,34 @@ function applyPatchset(
607675
// Phase 1: Validate all paths and actions before writing anything
608676
for (const patch of params.patches) {
609677
safePath(cwd, patch.path);
610-
if (!["create", "modify", "delete"].includes(patch.action)) {
678+
if (!VALID_PATCH_ACTIONS.has(patch.action)) {
611679
return {
612680
ok: false,
613681
error: `Unknown patch action: "${patch.action}" for path "${patch.path}"`,
614682
};
615683
}
616684
}
617685

618-
// Phase 2: Apply patches
686+
// Phase 2: Apply patches (sequential — later patches may depend on earlier creates)
619687
const applied: Array<{ path: string; action: string }> = [];
620688

621689
for (const patch of params.patches) {
622690
const absPath = safePath(cwd, patch.path);
623691

624-
switch (patch.action) {
625-
case "create": {
626-
const dir = path.dirname(absPath);
627-
fs.mkdirSync(dir, { recursive: true });
628-
const content = resolvePatchContent(absPath, patch);
629-
fs.writeFileSync(absPath, content, "utf-8");
630-
applied.push({ path: patch.path, action: "create" });
631-
break;
632-
}
633-
case "modify": {
634-
if (!fs.existsSync(absPath)) {
635-
return {
636-
ok: false,
637-
error: `Cannot modify "${patch.path}": file does not exist`,
638-
data: { applied },
639-
};
640-
}
641-
const content = resolvePatchContent(absPath, patch);
642-
fs.writeFileSync(absPath, content, "utf-8");
643-
applied.push({ path: patch.path, action: "modify" });
644-
break;
645-
}
646-
case "delete": {
647-
if (fs.existsSync(absPath)) {
648-
fs.unlinkSync(absPath);
649-
}
650-
applied.push({ path: patch.path, action: "delete" });
651-
break;
652-
}
653-
default:
692+
if (patch.action === "modify") {
693+
try {
694+
await fs.promises.access(absPath);
695+
} catch {
654696
return {
655697
ok: false,
656-
error: `Unknown patch action: "${patch.action}" for path "${patch.path}"`,
698+
error: `Cannot modify "${patch.path}": file does not exist`,
657699
data: { applied },
658700
};
701+
}
659702
}
703+
704+
await applySinglePatch(absPath, patch);
705+
applied.push({ path: patch.path, action: patch.action });
660706
}
661707

662708
return { ok: true, data: { applied } };

src/lib/init/wizard-runner.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,7 @@ export async function runWizard(initialOptions: WizardOptions): Promise<void> {
623623
let run: Awaited<ReturnType<typeof workflow.createRun>>;
624624
let result: WorkflowRunResult;
625625
try {
626-
const dirListing = precomputeDirListing(directory);
626+
const dirListing = await precomputeDirListing(directory);
627627
spin.message("Connecting to wizard...");
628628
run = await workflow.createRun();
629629
result = assertWorkflowResult(

0 commit comments

Comments
 (0)