diff --git a/src/plugins/marketplace.test.ts b/src/plugins/marketplace.test.ts index 100e6abdb7f..641a60b49b3 100644 --- a/src/plugins/marketplace.test.ts +++ b/src/plugins/marketplace.test.ts @@ -23,15 +23,14 @@ const runCommandWithTimeoutMock = vi.hoisted(() => vi.fn()); let installPluginFromMarketplace: typeof import("./marketplace.js").installPluginFromMarketplace; let listMarketplacePlugins: typeof import("./marketplace.js").listMarketplacePlugins; let resolveMarketplaceInstallShortcut: typeof import("./marketplace.js").resolveMarketplaceInstallShortcut; +const tempOutsideDirs: string[] = []; vi.mock("./install.js", () => ({ installPluginFromPath: (...args: unknown[]) => installPluginFromPathMock(...args), })); -vi.mock("../infra/net/fetch-guard.js", async () => { - const actual = await vi.importActual( - "../infra/net/fetch-guard.js", - ); +vi.mock("../infra/net/fetch-guard.js", async (importOriginal) => { + const actual = await importOriginal(); return { ...actual, fetchWithSsrFGuard: (params: { url: string; init?: RequestInit }) => @@ -78,11 +77,17 @@ async function writeRemoteMarketplaceFixture(params: { repoDir: string; manifest: unknown; pluginDir?: string; + pluginFile?: string; }) { await fs.mkdir(path.join(params.repoDir, ".claude-plugin"), { recursive: true }); if (params.pluginDir) { await fs.mkdir(path.join(params.repoDir, params.pluginDir), { recursive: true }); } + if (params.pluginFile) { + const pluginFilePath = path.join(params.repoDir, params.pluginFile); + await fs.mkdir(path.dirname(pluginFilePath), { recursive: true }); + await fs.writeFile(pluginFilePath, "plugin fixture"); + } await fs.writeFile( path.join(params.repoDir, ".claude-plugin", "marketplace.json"), JSON.stringify(params.manifest), @@ -100,7 +105,11 @@ async function writeLocalMarketplaceFixture(params: { return writeMarketplaceManifest(params.rootDir, params.manifest); } -function mockRemoteMarketplaceClone(params: { manifest: unknown; pluginDir?: string }) { +function mockRemoteMarketplaceClone(params: { + manifest: unknown; + pluginDir?: string; + pluginFile?: string; +}) { runCommandWithTimeoutMock.mockImplementationOnce(async (argv: string[]) => { const repoDir = argv.at(-1); expect(typeof repoDir).toBe("string"); @@ -108,11 +117,33 @@ function mockRemoteMarketplaceClone(params: { manifest: unknown; pluginDir?: str repoDir: repoDir as string, manifest: params.manifest, ...(params.pluginDir ? { pluginDir: params.pluginDir } : {}), + ...(params.pluginFile ? { pluginFile: params.pluginFile } : {}), }); return { code: 0, stdout: "", stderr: "", killed: false }; }); } +function mockRemoteMarketplaceCloneWithOutsideSymlink(params: { + manifest: unknown; + symlinkPath: string; +}) { + runCommandWithTimeoutMock.mockImplementationOnce(async (argv: string[]) => { + const repoDir = argv.at(-1); + expect(typeof repoDir).toBe("string"); + await writeRemoteMarketplaceFixture({ + repoDir: repoDir as string, + manifest: params.manifest, + }); + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-marketplace-outside-")); + tempOutsideDirs.push(outsideDir); + await fs.mkdir(path.dirname(path.join(repoDir as string, params.symlinkPath)), { + recursive: true, + }); + await fs.symlink(outsideDir, path.join(repoDir as string, params.symlinkPath)); + return { code: 0, stdout: "", stderr: "", killed: false }; + }); +} + async function expectRemoteMarketplaceError(params: { manifest: unknown; expectedError: string }) { mockRemoteMarketplaceClone({ manifest: params.manifest }); @@ -185,11 +216,16 @@ function expectLocalMarketplaceInstallResult(params: { } describe("marketplace plugins", () => { - afterEach(() => { + afterEach(async () => { fetchWithSsrFGuardMock.mockClear(); installPluginFromPathMock.mockReset(); runCommandWithTimeoutMock.mockReset(); vi.unstubAllGlobals(); + await Promise.all( + tempOutsideDirs.splice(0, tempOutsideDirs.length).map(async (dir) => { + await fs.rm(dir, { recursive: true, force: true }); + }), + ); }); it("lists plugins from a local marketplace root", async () => { @@ -247,6 +283,49 @@ describe("marketplace plugins", () => { }); }); + it.runIf(process.platform !== "win32")( + "preserves relative local marketplace installs when the marketplace root is symlinked", + async () => { + await withTempDir(async (parentDir) => { + const realRootDir = path.join(parentDir, "real-marketplace"); + const symlinkRootDir = path.join(parentDir, "marketplace-link"); + const pluginDir = path.join(realRootDir, "plugins", "frontend-design"); + await fs.mkdir(realRootDir, { recursive: true }); + await writeLocalMarketplaceFixture({ + rootDir: realRootDir, + pluginDir, + manifest: { + plugins: [ + { + name: "frontend-design", + source: "./plugins/frontend-design", + }, + ], + }, + }); + await fs.symlink(realRootDir, symlinkRootDir); + installPluginFromPathMock.mockResolvedValue({ + ok: true, + pluginId: "frontend-design", + targetDir: "/tmp/frontend-design", + version: "0.1.0", + extensions: ["index.ts"], + }); + + const result = await installPluginFromMarketplace({ + marketplace: symlinkRootDir, + plugin: "frontend-design", + }); + + expectLocalMarketplaceInstallResult({ + result, + pluginDir, + marketplaceSource: symlinkRootDir, + }); + }); + }, + ); + it("passes dangerous force unsafe install through to marketplace path installs", async () => { await withTempDir(async (rootDir) => { const pluginDir = path.join(rootDir, "plugins", "frontend-design"); @@ -345,6 +424,111 @@ describe("marketplace plugins", () => { expectRemoteMarketplaceInstallResult(result); }); + it("preserves remote marketplace file path sources inside the cloned repo", async () => { + mockRemoteMarketplaceClone({ + pluginFile: path.join("plugins", "frontend-design.tgz"), + manifest: { + plugins: [ + { + name: "frontend-design", + source: "./plugins/frontend-design.tgz", + }, + ], + }, + }); + installPluginFromPathMock.mockResolvedValue({ + ok: true, + pluginId: "frontend-design", + targetDir: "/tmp/frontend-design", + version: "0.1.0", + extensions: ["index.ts"], + }); + + const result = await installPluginFromMarketplace({ + marketplace: "owner/repo", + plugin: "frontend-design", + }); + + expect(runCommandWithTimeoutMock).toHaveBeenCalledTimes(1); + expect(installPluginFromPathMock).toHaveBeenCalledWith( + expect.objectContaining({ + path: expect.stringMatching(/[\\/]repo[\\/]plugins[\\/]frontend-design\.tgz$/), + }), + ); + expect(result).toMatchObject({ + ok: true, + pluginId: "frontend-design", + marketplacePlugin: "frontend-design", + marketplaceSource: "owner/repo", + }); + }); + + it("lists remote marketplace file path sources inside the cloned repo", async () => { + mockRemoteMarketplaceClone({ + pluginFile: path.join("plugins", "frontend-design.tgz"), + manifest: { + plugins: [ + { + name: "frontend-design", + source: "./plugins/frontend-design.tgz", + }, + ], + }, + }); + + const result = await listMarketplacePlugins({ marketplace: "owner/repo" }); + + expect(result).toEqual({ + ok: true, + manifest: { + name: undefined, + version: undefined, + plugins: [ + { + name: "frontend-design", + description: undefined, + version: undefined, + source: { + kind: "path", + path: "./plugins/frontend-design.tgz", + }, + }, + ], + }, + sourceLabel: "owner/repo", + }); + }); + + it.runIf(process.platform !== "win32")( + "rejects remote marketplace plugin paths that resolve through symlinks outside the cloned repo", + async () => { + mockRemoteMarketplaceCloneWithOutsideSymlink({ + symlinkPath: "plugins/evil-link", + manifest: { + plugins: [ + { + name: "frontend-design", + source: "./plugins/evil-link", + }, + ], + }, + }); + + const result = await installPluginFromMarketplace({ + marketplace: "owner/repo", + plugin: "frontend-design", + }); + + expect(result).toEqual({ + ok: false, + error: + 'invalid marketplace entry "frontend-design" in owner/repo: ' + + "plugin source escapes marketplace root: ./plugins/evil-link", + }); + expect(installPluginFromPathMock).not.toHaveBeenCalled(); + }, + ); + it("returns a structured error for archive downloads with an empty response body", async () => { await withTempDir(async (rootDir) => { const release = vi.fn(async () => undefined); @@ -798,4 +982,58 @@ describe("marketplace plugins", () => { ] as const)("$name", async ({ manifest, expectedError }) => { await expectRemoteMarketplaceError({ manifest, expectedError }); }); + + it.runIf(process.platform !== "win32")( + "rejects remote marketplace symlink plugin paths during manifest validation", + async () => { + mockRemoteMarketplaceCloneWithOutsideSymlink({ + symlinkPath: "evil-link", + manifest: { + plugins: [ + { + name: "frontend-design", + source: { + type: "path", + path: "evil-link", + }, + }, + ], + }, + }); + + const result = await listMarketplacePlugins({ marketplace: "owner/repo" }); + + expect(result).toEqual({ + ok: false, + error: + 'invalid marketplace entry "frontend-design" in owner/repo: ' + + "plugin source escapes marketplace root: evil-link", + }); + }, + ); + + it("reports missing remote marketplace paths as not found instead of escapes", async () => { + mockRemoteMarketplaceClone({ + manifest: { + plugins: [ + { + name: "frontend-design", + source: { + type: "path", + path: "plugins/missing-plugin", + }, + }, + ], + }, + }); + + const result = await listMarketplacePlugins({ marketplace: "owner/repo" }); + + expect(result).toEqual({ + ok: false, + error: + 'invalid marketplace entry "frontend-design" in owner/repo: ' + + "plugin source not found in marketplace root: plugins/missing-plugin", + }); + }); }); diff --git a/src/plugins/marketplace.ts b/src/plugins/marketplace.ts index ad68395058a..23534cbe708 100644 --- a/src/plugins/marketplace.ts +++ b/src/plugins/marketplace.ts @@ -5,6 +5,7 @@ import { resolveArchiveKind } from "../infra/archive.js"; import { formatErrorMessage } from "../infra/errors.js"; import { resolveOsHomeRelativePath } from "../infra/home-dir.js"; import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js"; +import { isPathInside } from "../infra/path-guards.js"; import { runCommandWithTimeout } from "../process/exec.js"; import { redactSensitiveUrlLikeString } from "../shared/net/redact-sensitive-url.js"; import { sanitizeForLog } from "../terminal/ansi.js"; @@ -55,6 +56,7 @@ type LoadedMarketplace = { manifest: MarketplaceManifest; rootDir: string; sourceLabel: string; + origin: MarketplaceManifestOrigin; cleanup?: () => Promise; }; @@ -362,9 +364,10 @@ async function resolveLocalMarketplaceSource( const stat = await fs.stat(resolved); if (stat.isFile()) { + const rootDir = deriveMarketplaceRootFromManifestPath(resolved); return { ok: true, - rootDir: deriveMarketplaceRootFromManifestPath(resolved), + rootDir: await fs.realpath(rootDir), manifestPath: resolved, }; } @@ -374,10 +377,11 @@ async function resolveLocalMarketplaceSource( } const rootDir = path.basename(resolved) === ".claude-plugin" ? path.dirname(resolved) : resolved; + const canonicalRootDir = await fs.realpath(rootDir); for (const candidate of MARKETPLACE_MANIFEST_CANDIDATES) { const manifestPath = path.join(rootDir, candidate); if (await pathExists(manifestPath)) { - return { ok: true, rootDir, manifestPath }; + return { ok: true, rootDir: canonicalRootDir, manifestPath }; } } @@ -485,7 +489,7 @@ async function loadMarketplace(params: { if (!parsed.ok) { return parsed; } - const validated = validateMarketplaceManifest({ + const validated = await validateMarketplaceManifest({ manifest: parsed.manifest, sourceLabel: local.manifestPath, rootDir: local.rootDir, @@ -500,6 +504,7 @@ async function loadMarketplace(params: { manifest: validated.manifest, rootDir: local.rootDir, sourceLabel, + origin: "local", }, }; }; @@ -561,7 +566,7 @@ async function loadMarketplace(params: { await cloned.cleanup(); return parsed; } - const validated = validateMarketplaceManifest({ + const validated = await validateMarketplaceManifest({ manifest: parsed.manifest, sourceLabel: cloned.label, rootDir: cloned.rootDir, @@ -578,6 +583,7 @@ async function loadMarketplace(params: { manifest: validated.manifest, rootDir: cloned.rootDir, sourceLabel: cloned.label, + origin: "remote", cleanup: cloned.cleanup, }, }; @@ -802,11 +808,13 @@ async function downloadUrlToTempFile( } } -function ensureInsideMarketplaceRoot( +async function ensureInsideMarketplaceRoot( rootDir: string, candidate: string, -): { ok: true; path: string } | { ok: false; error: string } { + options?: { requireCanonicalRoot?: boolean }, +): Promise<{ ok: true; path: string } | { ok: false; error: string }> { const resolved = path.resolve(rootDir, candidate); + const resolvedExists = await pathExists(resolved); const relative = path.relative(rootDir, resolved); if (relative === ".." || relative.startsWith(`..${path.sep}`)) { return { @@ -814,15 +822,55 @@ function ensureInsideMarketplaceRoot( error: `plugin source escapes marketplace root: ${candidate}`, }; } + + try { + const rootLstat = await fs.lstat(rootDir); + if ( + !rootLstat.isDirectory() || + (options?.requireCanonicalRoot === true && rootLstat.isSymbolicLink()) + ) { + throw new Error("invalid marketplace root"); + } + + const rootRealPath = await fs.realpath(rootDir); + let existingPath = resolved; + // `pathExists` uses `fs.access`, so dangling symlinks are treated as missing and we walk up + // to the nearest existing ancestor. Live symlinks stop here and are canonicalized below. + while (!(await pathExists(existingPath))) { + const parentPath = path.dirname(existingPath); + if (parentPath === existingPath) { + throw new Error("unreachable marketplace path"); + } + existingPath = parentPath; + } + + const existingRealPath = await fs.realpath(existingPath); + if (!isPathInside(rootRealPath, existingRealPath)) { + throw new Error("marketplace path escapes canonical root"); + } + } catch { + return { + ok: false, + error: `plugin source escapes marketplace root: ${candidate}`, + }; + } + + if (!resolvedExists) { + return { + ok: false, + error: `plugin source not found in marketplace root: ${candidate}`, + }; + } + return { ok: true, path: resolved }; } -function validateMarketplaceManifest(params: { +async function validateMarketplaceManifest(params: { manifest: MarketplaceManifest; sourceLabel: string; rootDir: string; origin: MarketplaceManifestOrigin; -}): { ok: true; manifest: MarketplaceManifest } | { ok: false; error: string } { +}): Promise<{ ok: true; manifest: MarketplaceManifest } | { ok: false; error: string }> { if (params.origin === "local") { return { ok: true, manifest: params.manifest }; } @@ -846,7 +894,7 @@ function validateMarketplaceManifest(params: { "remote marketplaces may only use relative plugin paths", }; } - const resolved = ensureInsideMarketplaceRoot(params.rootDir, source.path); + const resolved = await ensureInsideMarketplaceRoot(params.rootDir, source.path); if (!resolved.ok) { return { ok: false, @@ -870,6 +918,7 @@ function validateMarketplaceManifest(params: { async function resolveMarketplaceEntryInstallPath(params: { source: MarketplaceEntrySource; marketplaceRootDir: string; + marketplaceOrigin: MarketplaceManifestOrigin; logger?: MarketplaceLogger; timeoutMs?: number; }): Promise< @@ -895,7 +944,9 @@ async function resolveMarketplaceEntryInstallPath(params: { } const resolved = path.isAbsolute(params.source.path) ? { ok: true as const, path: params.source.path } - : ensureInsideMarketplaceRoot(params.marketplaceRootDir, params.source.path); + : await ensureInsideMarketplaceRoot(params.marketplaceRootDir, params.source.path, { + requireCanonicalRoot: params.marketplaceOrigin === "remote", + }); if (!resolved.ok) { return resolved; } @@ -923,7 +974,7 @@ async function resolveMarketplaceEntryInstallPath(params: { params.source.kind === "github" || params.source.kind === "git" ? params.source.path?.trim() || "." : params.source.path.trim(); - const target = ensureInsideMarketplaceRoot(cloned.rootDir, subPath); + const target = await ensureInsideMarketplaceRoot(cloned.rootDir, subPath); if (!target.ok) { await cloned.cleanup(); return target; @@ -1069,6 +1120,7 @@ export async function installPluginFromMarketplace( const resolved = await resolveMarketplaceEntryInstallPath({ source: entry.source, marketplaceRootDir: loaded.marketplace.rootDir, + marketplaceOrigin: loaded.marketplace.origin, logger: params.logger, timeoutMs: params.timeoutMs, });