mirror of https://github.com/openclaw/openclaw.git
fix(marketplace): canonicalize remote plugin paths
This commit is contained in:
Agustin Rivera
Peter Steinberger
parent
f25f147fc3
commit
b1dd3ded35
|
|
@ -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<typeof import("../infra/net/fetch-guard.js")>(
|
||||
"../infra/net/fetch-guard.js",
|
||||
);
|
||||
vi.mock("../infra/net/fetch-guard.js", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("../infra/net/fetch-guard.js")>();
|
||||
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",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<void>;
|
||||
};
|
||||
|
||||
|
|
@ -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,
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue