From 7e2663360fe74e58541b6afda9bfeeeffee99e76 Mon Sep 17 00:00:00 2001 From: can1357 Date: Wed, 12 Aug 2026 02:02:33 +0200 Subject: [PATCH] fix(read): bounded tar directory-alias resolution and resolved aliased link targets - Capped directory-alias rewrites per lookup (ELOOP-style, 40) so a directory symlink targeting its own subtree (a -> a/b) throws a catchable ToolError instead of looping forever growing the path. - Deferred pending tar link resolution while any directory on the target path is itself an unresolved link, and rewrote targets through established directory aliases before the exact-path lookup, so file symlinks routed through directory aliases materialize instead of dangling. - Regression tests reproduce both shapes: pre-fix the aliased symlink read failed with 'cannot be materialized' and the self-cycle read hung. --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/utils/zip.ts | 100 +++++++++++++++-------- packages/coding-agent/test/tools.test.ts | 36 ++++++++ 3 files changed, 102 insertions(+), 35 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1a69b43d9..fdb92c476 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -21,6 +21,7 @@ - Fixed the MCP Streamable HTTP transport never sending the `MCP-Protocol-Version` header and negotiating the stale `2025-03-26` revision, which made spec-current servers (e.g. AWS Bedrock AgentCore Gateway with an outbound per-user OAuth target) reject every `tools/call` with a generic internal error. The client now negotiates `2025-11-25`, echoes the negotiated version on every request after `initialize`, and resumes server-closed POST response streams with `Last-Event-ID` after the requested SSE retry interval ([#8264](https://github.com/can1357/oh-my-pi/issues/8264)). - Fixed `/handoff` losing the previous session's `local://` artifacts (plans, scratch files, research notes): the handoff document referenced files that became unreadable because the new session's `local/` root was empty. Local artifacts are now copied across the handoff session boundary, mirroring the plan approve-and-execute path ([#8261](https://github.com/can1357/oh-my-pi/issues/8261)). - Fixed valid `.tar` and `.tar.gz` archive reads terminating omp through libarchive by parsing tar members in-process ([#4774](https://github.com/can1357/oh-my-pi/issues/4774)). +- Fixed the in-process tar reader looping forever on directory symlinks targeting their own subtree (`a -> a/b`) and misclassifying file symlinks routed through directory aliases as dangling; alias resolution is now depth-bounded and link targets resolve through directory aliases at index time. ## [17.2.14] - 2026-08-11 diff --git a/packages/coding-agent/src/utils/zip.ts b/packages/coding-agent/src/utils/zip.ts index f3290db45..4af926b69 100644 --- a/packages/coding-agent/src/utils/zip.ts +++ b/packages/coding-agent/src/utils/zip.ts @@ -908,11 +908,36 @@ function readTarEntries(rawBytes: Uint8Array): ArchiveIndexEntry[] { for (const entry of entries) entriesByPath.set(entry.path, entry); const unresolved = new Set(pendingLinks.keys()); + // True while the target itself or any directory on its path is a link + // that has not been classified yet; such targets must wait a pass so a + // file symlink routed through a directory alias is not misjudged + // dangling before the alias resolves. + const dependsOnUnresolvedLink = (targetPath: string): boolean => { + const parts = targetPath.split("/"); + for (let end = parts.length; end > 0; end--) { + const prefixEntry = entriesByPath.get(parts.slice(0, end).join("/")); + if (prefixEntry && unresolved.has(prefixEntry)) return true; + } + return false; + }; + while (unresolved.size > 0) { let resolved = 0; for (const entry of unresolved) { const pending = pendingLinks.get(entry)!; - const target = entriesByPath.get(pending.targetPath); + if (dependsOnUnresolvedLink(pending.targetPath)) continue; + + // Targets may route through directory aliases classified in an + // earlier pass; rewrite before the exact-path lookup. + let targetPath = pending.targetPath; + try { + targetPath = resolveDirectoryAliasPath(entriesByPath, targetPath); + } catch { + // Cyclic alias chain: fall through to the dangling-symlink path. + } + if (targetPath !== pending.targetPath && dependsOnUnresolvedLink(targetPath)) continue; + + const target = entriesByPath.get(targetPath); if (target?.storage && !target.isDirectory) { entry.size = target.size; entry.storage = target.storage; @@ -920,12 +945,11 @@ function readTarEntries(rawBytes: Uint8Array): ArchiveIndexEntry[] { resolved++; continue; } - if (target && unresolved.has(target)) continue; // An empty target is the archive root, which is always a directory. - const targetPrefix = `${pending.targetPath}/`; + const targetPrefix = `${targetPath}/`; const targetIsDirectory = - pending.targetPath === "" || + targetPath === "" || target?.isDirectory === true || entries.some(candidate => candidate.path.startsWith(targetPrefix)); if (!targetIsDirectory) { @@ -976,6 +1000,40 @@ function throwUnreadableTarLink(storage: TarLinkStorage, memberPath: string): ne throw new ToolError(`Archive symlink '${memberPath}' cannot be materialized from target '${storage.targetPath}'`); } +/** ELOOP-style bound on directory-alias rewrites during a single path lookup. */ +const MAX_LINK_RESOLUTION_DEPTH = 40; + +/** + * Rewrite `archivePath` through directory symlink aliases until it no longer + * crosses one. Bounded: an exact revisit and an alias chain that keeps growing + * the path (e.g. a directory symlink targeting its own subtree, `a -> a/b`) + * both throw a catchable cyclic-symlink error instead of looping forever. + */ +function resolveDirectoryAliasPath(entries: ReadonlyMap, archivePath: string): string { + let resolvedPath = archivePath; + const seen = new Set(); + for (let depth = 0; depth < MAX_LINK_RESOLUTION_DEPTH && !seen.has(resolvedPath); depth++) { + seen.add(resolvedPath); + const parts = resolvedPath.split("/"); + let replacement: string | undefined; + for (let end = parts.length; end > 0; end--) { + const prefix = parts.slice(0, end).join("/"); + const entry = entries.get(prefix); + if (!entry?.isDirectory || entry.storage?.type !== "tar-link") continue; + const suffix = parts.slice(end).join("/"); + replacement = suffix + ? entry.storage.targetPath + ? `${entry.storage.targetPath}/${suffix}` + : suffix + : entry.storage.targetPath; + break; + } + if (replacement === undefined) return resolvedPath; + resolvedPath = replacement; + } + throw new ToolError(`Archive path '${archivePath}' crosses a cyclic symlink`); +} + async function readZipEntries(source: ByteSource): Promise { const directoryInfo = await readZipCentralDirectoryInfo(source); const centralDirectory = await source.read(directoryInfo.offset, directoryInfo.offset + directoryInfo.size); @@ -1030,34 +1088,6 @@ export class ArchiveReader { ensureParentDirectories(this.#entries); } - #resolveDirectoryAliases(archivePath: string): string { - let resolvedPath = archivePath; - const seen = new Set(); - while (true) { - if (seen.has(resolvedPath)) { - throw new ToolError(`Archive path '${archivePath}' crosses a cyclic symlink`); - } - seen.add(resolvedPath); - - const parts = resolvedPath.split("/"); - let replacement: string | undefined; - for (let end = parts.length; end > 0; end--) { - const prefix = parts.slice(0, end).join("/"); - const entry = this.#entries.get(prefix); - if (!entry?.isDirectory || entry.storage?.type !== "tar-link") continue; - const suffix = parts.slice(end).join("/"); - replacement = suffix - ? entry.storage.targetPath - ? `${entry.storage.targetPath}/${suffix}` - : suffix - : entry.storage.targetPath; - break; - } - if (replacement === undefined) return resolvedPath; - resolvedPath = replacement; - } - } - getNode(subPath?: string): ArchiveNode | undefined { const normalizedPath = normalizeArchiveLookupPath(subPath); if (normalizedPath === undefined) return undefined; @@ -1065,7 +1095,7 @@ export class ArchiveReader { return { path: "", isDirectory: true, size: 0 }; } - const resolvedPath = this.#resolveDirectoryAliases(normalizedPath); + const resolvedPath = resolveDirectoryAliasPath(this.#entries, normalizedPath); if (resolvedPath === "") { return { path: normalizedPath, isDirectory: true, size: 0 }; } @@ -1085,7 +1115,7 @@ export class ArchiveReader { throw new ToolError("Archive path cannot contain '..'"); } - const resolvedPath = normalizedPath ? this.#resolveDirectoryAliases(normalizedPath) : ""; + const resolvedPath = normalizedPath ? resolveDirectoryAliasPath(this.#entries, normalizedPath) : ""; if (normalizedPath && resolvedPath !== "") { const entry = this.#entries.get(resolvedPath); if (!entry) { @@ -1134,7 +1164,7 @@ export class ArchiveReader { throw new ToolError("Archive file path is required"); } - const resolvedPath = this.#resolveDirectoryAliases(normalizedPath); + const resolvedPath = resolveDirectoryAliasPath(this.#entries, normalizedPath); if (resolvedPath === "") { throw new ToolError(`Archive path '${normalizedPath}' is a directory`); } diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index be0343279..f4bd64744 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -921,6 +921,42 @@ describe("Coding Agent Tools", () => { await expect(readArchiveEntries(archivePath)).rejects.toThrow(/cannot be materialized/); }); + it("should resolve file symlinks routed through directory symlinks", async () => { + const archivePath = path.join(testDir, "aliased-file-symlink.tar"); + fs.writeFileSync( + archivePath, + createTarArchive([ + // The file symlink precedes the directory alias it routes + // through, so resolution must defer until the alias settles. + { path: "pkg/bin/tool", content: "", typeFlag: "2", linkName: "../current/tool.js" }, + { path: "pkg/lib/tool.js", content: "export const linked = true;\n" }, + { path: "pkg/current", content: "", typeFlag: "2", linkName: "lib" }, + ]), + ); + + const linkedResult = await readTool.execute("test-call-tar-aliased-symlink-member", { + path: `${archivePath}:pkg/bin/tool`, + }); + expect(getTextOutput(linkedResult)).toContain("export const linked = true"); + }); + + it("should reject directory symlinks targeting their own subtree instead of looping", async () => { + const archivePath = path.join(testDir, "self-cycle-symlink.tar"); + fs.writeFileSync( + archivePath, + createTarArchive([ + { path: "a/b/f.txt", content: "unreachable\n" }, + // `a -> a/b` grows the resolved path on every rewrite; the + // pre-fix resolver looped forever on this shape. + { path: "a", content: "", typeFlag: "2", linkName: "a/b" }, + ]), + ); + + await expect( + readTool.execute("test-call-tar-self-cycle", { path: `${archivePath}:a/b/f.txt` }), + ).rejects.toThrow(/cyclic or unsupported links/); + }); + it("should resolve tar symlinks whose target is the archive root", async () => { const archivePath = path.join(testDir, "root-symlinks.tar"); fs.writeFileSync(