From bdfb408ce6eeb724a638c106104dbbcae50b612c Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Tue, 28 Apr 2026 21:35:32 +0530 Subject: [PATCH] fix(plugins): restrict bundled plugin dir resolution to trusted package roots (#73275) * fix: address issue * fix: address review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address codex review feedback * fix: address codex review feedback * fix: address codex review feedback * fix: address PR review feedback * fix: address review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address review feedback * docs: add changelog entry for PR merge --- CHANGELOG.md | 1 + extensions/signal/src/install-signal-cli.ts | 66 ++++++--- scripts/test-built-plugin-singleton.mjs | 16 ++- src/infra/net/fetch-guard.ts | 5 + src/plugins/bundled-dir.test.ts | 143 +++++++++++++++++++- src/plugins/bundled-dir.ts | 100 +++++++++++--- src/plugins/source-display.test.ts | 16 ++- 7 files changed, 289 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb010932e70..b60cdd6e3da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(plugins): restrict bundled plugin dir resolution to trusted package roots. (#73275) Thanks @pgondhi987. - fix(security): prevent workspace PATH injection via service env and trash helpers. (#73264) Thanks @pgondhi987. - Active Memory: allow `allowedChatTypes` to include explicit portal/webchat sessions and classify `agent:...:explicit:...` session keys before opaque session ids can shadow the chat type. Fixes #65775. (#66285) Thanks @Lidang-Jiang. - Active Memory: allow the hidden recall sub-agent to use both `memory_recall` and the legacy `memory_search`/`memory_get` memory tool contract, so bundled `memory-lancedb` recall works without breaking the default `memory-core` path. Fixes #73502. (#73584) Thanks @Takhoffman. diff --git a/extensions/signal/src/install-signal-cli.ts b/extensions/signal/src/install-signal-cli.ts index 1abebcd4666..81d8df6136a 100644 --- a/extensions/signal/src/install-signal-cli.ts +++ b/extensions/signal/src/install-signal-cli.ts @@ -1,12 +1,13 @@ import { createWriteStream } from "node:fs"; import fs from "node:fs/promises"; -import { request } from "node:https"; import path from "node:path"; +import { Readable, Transform } from "node:stream"; import { pipeline } from "node:stream/promises"; import { formatErrorMessage } from "openclaw/plugin-sdk/error-runtime"; import { runPluginCommandWithTimeout } from "openclaw/plugin-sdk/run-command"; import type { RuntimeEnv } from "openclaw/plugin-sdk/runtime-env"; import { CONFIG_DIR, extractArchive, resolveBrewExecutable } from "openclaw/plugin-sdk/setup-tools"; +import { fetchWithSsrFGuard } from "openclaw/plugin-sdk/ssrf-runtime"; import { resolvePreferredOpenClawTmpDir } from "openclaw/plugin-sdk/temp-path"; import { normalizeLowercaseStringOrEmpty } from "openclaw/plugin-sdk/text-runtime"; @@ -25,6 +26,8 @@ type ReleaseResponse = { assets?: ReleaseAsset[]; }; +const MAX_SIGNAL_CLI_ARCHIVE_BYTES = 256 * 1024 * 1024; + export type SignalInstallResult = { ok: boolean; cliPath?: string; @@ -46,6 +49,20 @@ export function looksLikeArchive(name: string): boolean { return name.endsWith(".tar.gz") || name.endsWith(".tgz") || name.endsWith(".zip"); } +function isNodeReadableStream(value: unknown): value is Readable { + return Boolean(value && typeof (value as { pipe?: unknown }).pipe === "function"); +} + +function chunkByteLength(chunk: unknown): number { + if (typeof chunk === "string") { + return Buffer.byteLength(chunk); + } + if (chunk instanceof Uint8Array) { + return chunk.byteLength; + } + return Buffer.byteLength(String(chunk)); +} + /** * Pick a native release asset from the official GitHub releases. * @@ -95,28 +112,37 @@ export function pickAsset( } async function downloadToFile(url: string, dest: string, maxRedirects = 5): Promise { - await new Promise((resolve, reject) => { - const req = request(url, (res) => { - if (res.statusCode && res.statusCode >= 300 && res.statusCode < 400) { - const location = res.headers.location; - if (!location || maxRedirects <= 0) { - reject(new Error("Redirect loop or missing Location header")); + const { response, release } = await fetchWithSsrFGuard({ + url, + maxRedirects, + requireHttps: true, + capture: false, + auditContext: "signal-cli-install-archive", + }); + try { + if (!response.ok || !response.body) { + throw new Error(`HTTP ${response.status || "?"} downloading file`); + } + + let totalBytes = 0; + const body = response.body; + const readable = isNodeReadableStream(body) ? body : Readable.fromWeb(body as never); + const limiter = new Transform({ + transform(chunk: unknown, _encoding, callback) { + totalBytes += chunkByteLength(chunk); + if (totalBytes > MAX_SIGNAL_CLI_ARCHIVE_BYTES) { + callback(new Error("signal-cli archive exceeds 256 MiB limit")); return; } - const redirectUrl = new URL(location, url).href; - resolve(downloadToFile(redirectUrl, dest, maxRedirects - 1)); - return; - } - if (!res.statusCode || res.statusCode >= 400) { - reject(new Error(`HTTP ${res.statusCode ?? "?"} downloading file`)); - return; - } - const out = createWriteStream(dest); - pipeline(res, out).then(resolve).catch(reject); + callback(null, chunk); + }, }); - req.on("error", reject); - req.end(); - }); + + const out = createWriteStream(dest); + await pipeline(readable, limiter, out); + } finally { + await release(); + } } async function findSignalCliBinary(root: string): Promise { diff --git a/scripts/test-built-plugin-singleton.mjs b/scripts/test-built-plugin-singleton.mjs index e97167cbd10..082e148477e 100644 --- a/scripts/test-built-plugin-singleton.mjs +++ b/scripts/test-built-plugin-singleton.mjs @@ -21,9 +21,14 @@ assert.equal(typeof getPluginCommandSpecs, "function", "getPluginCommandSpecs mi assert.equal(typeof matchPluginCommand, "function", "matchPluginCommand missing"); const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-build-smoke-")); +const pluginId = "build-smoke-plugin"; +const distPluginDir = path.join(repoRoot, "dist", "extensions", pluginId); +const runtimePluginDir = path.join(repoRoot, "dist-runtime", "extensions", pluginId); function cleanup() { clearPluginCommands(); + fs.rmSync(distPluginDir, { recursive: true, force: true }); + fs.rmSync(runtimePluginDir, { recursive: true, force: true }); fs.rmSync(tempRoot, { recursive: true, force: true }); } @@ -37,10 +42,7 @@ process.on("SIGTERM", () => { process.exit(143); }); -const pluginId = "build-smoke-plugin"; -const distPluginDir = path.join(tempRoot, "dist", "extensions", pluginId); fs.mkdirSync(distPluginDir, { recursive: true }); -fs.writeFileSync(path.join(tempRoot, "package.json"), '{ "type": "module" }\n', "utf8"); fs.writeFileSync( path.join(distPluginDir, "package.json"), JSON.stringify( @@ -98,12 +100,12 @@ fs.writeFileSync( "utf8", ); -stageBundledPluginRuntime({ repoRoot: tempRoot }); +stageBundledPluginRuntime({ repoRoot }); -const runtimeEntryPath = path.join(tempRoot, "dist-runtime", "extensions", pluginId, "index.js"); +const runtimeEntryPath = path.join(runtimePluginDir, "index.js"); assert.ok(fs.existsSync(runtimeEntryPath), "runtime overlay entry missing"); assert.equal( - fs.existsSync(path.join(tempRoot, "dist-runtime", "plugins", "commands.js")), + fs.existsSync(path.join(repoRoot, "dist-runtime", "plugins", "commands.js")), false, "dist-runtime must not stage a duplicate commands module", ); @@ -115,7 +117,7 @@ const registry = loadOpenClawPlugins({ workspaceDir: tempRoot, env: { ...process.env, - OPENCLAW_BUNDLED_PLUGINS_DIR: path.join(tempRoot, "dist-runtime", "extensions"), + OPENCLAW_BUNDLED_PLUGINS_DIR: path.join(repoRoot, "dist-runtime", "extensions"), OPENCLAW_DISABLE_PLUGIN_DISCOVERY_CACHE: "1", }, config: { diff --git a/src/infra/net/fetch-guard.ts b/src/infra/net/fetch-guard.ts index 831a2639c35..fd2ddb6a150 100644 --- a/src/infra/net/fetch-guard.ts +++ b/src/infra/net/fetch-guard.ts @@ -67,6 +67,7 @@ export type GuardedFetchOptions = { allowCrossOriginUnsafeRedirectReplay?: boolean; timeoutMs?: number; signal?: AbortSignal; + requireHttps?: boolean; policy?: SsrFPolicy; lookupFn?: LookupFn; dispatcherPolicy?: PinnedDispatcherPolicy; @@ -346,6 +347,10 @@ export async function fetchWithSsrFGuard(params: GuardedFetchOptions): Promise { }, ], [ - "prefers source extensions under vitest to avoid stale staged plugins", + "does not prefer source extensions from VITEST alone", { prefix: "openclaw-bundled-dir-vitest-", hasExtensions: true, @@ -210,7 +210,7 @@ describe("resolveBundledPluginsDir", () => { hasDistExtensions: true, }, { - expectedRelativeDir: "extensions", + expectedRelativeDir: path.join("dist-runtime", "extensions"), vitest: "true", }, ], @@ -248,6 +248,8 @@ describe("resolveBundledPluginsDir", () => { seedBundledPluginTree(repoRoot, path.join("dist-runtime", "extensions")); } else if (expectation.expectedRelativeDir === path.join("dist", "extensions")) { seedBundledPluginTree(repoRoot, path.join("dist", "extensions")); + } else if (expectation.expectedRelativeDir === "extensions") { + seedBundledPluginTree(repoRoot, "extensions"); } expectResolvedBundledDirFromRoot({ repoRoot, @@ -270,6 +272,7 @@ describe("resolveBundledPluginsDir", () => { fs.mkdirSync(path.join(repoRoot, "dist-runtime", "extensions", "discord"), { recursive: true, }); + seedBundledPluginTree(repoRoot, "extensions"); expectResolvedBundledDirFromRoot({ repoRoot, @@ -296,6 +299,140 @@ describe("resolveBundledPluginsDir", () => { expect(fs.readdirSync(bundledDir ?? "")).toEqual([]); }); + it("ignores an existing override under an argv1-derived fake package root", () => { + const installedRoot = createOpenClawRoot({ + prefix: "openclaw-bundled-dir-argv-override-reject-", + hasDistExtensions: true, + }); + seedBundledPluginTree(installedRoot, path.join("dist", "extensions")); + + vi.spyOn(process, "cwd").mockReturnValue(installedRoot); + process.argv[1] = path.join(installedRoot, "openclaw.mjs"); + process.execArgv.length = 0; + delete process.env.VITEST; + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = path.join(installedRoot, "dist", "extensions"); + delete process.env.OPENCLAW_DISABLE_BUNDLED_PLUGINS; + + const bundledDir = resolveBundledPluginsDir(); + + expect(bundledDir).toBeDefined(); + expect(fs.realpathSync(bundledDir!)).not.toBe( + fs.realpathSync(path.join(installedRoot, "dist", "extensions")), + ); + }); + + it("does not let VITEST relax existing override trust checks", () => { + const overrideRoot = makeRepoRoot("openclaw-bundled-dir-vitest-override-reject-"); + seedBundledPluginTree(overrideRoot, "extensions", "memory-core"); + + vi.spyOn(process, "cwd").mockReturnValue(overrideRoot); + process.argv[1] = "/usr/bin/env"; + process.execArgv.length = 0; + process.env.VITEST = "true"; + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = path.join(overrideRoot, "extensions"); + delete process.env.OPENCLAW_DISABLE_BUNDLED_PLUGINS; + + const bundledDir = resolveBundledPluginsDir(); + + expect(bundledDir).toBeDefined(); + expect(fs.realpathSync(bundledDir!)).not.toBe( + fs.realpathSync(path.join(overrideRoot, "extensions")), + ); + }); + + it("does not let VITEST add cwd to bundled plugin resolution candidates", () => { + const cwdRepoRoot = createOpenClawRoot({ + prefix: "openclaw-bundled-dir-vitest-cwd-", + hasExtensions: true, + hasSrc: true, + hasGitCheckout: true, + }); + seedBundledPluginTree(cwdRepoRoot, "extensions", "memory-core"); + + vi.spyOn(process, "cwd").mockReturnValue(cwdRepoRoot); + process.argv[1] = "/usr/bin/env"; + process.execArgv.length = 0; + process.env.VITEST = "true"; + delete process.env.OPENCLAW_BUNDLED_PLUGINS_DIR; + delete process.env.OPENCLAW_DISABLE_BUNDLED_PLUGINS; + + const bundledDir = resolveBundledPluginsDir(); + + expect(bundledDir).toBeDefined(); + expect(fs.realpathSync(bundledDir!)).not.toBe( + fs.realpathSync(path.join(cwdRepoRoot, "extensions")), + ); + }); + + it("falls back from a missing override instead of returning an untrusted future path", () => { + vi.spyOn(process, "cwd").mockReturnValue(makeRepoRoot("openclaw-bundled-dir-missing-cwd-")); + process.argv[1] = "/usr/bin/env"; + process.execArgv.length = 0; + delete process.env.VITEST; + const missingOverride = path.join( + makeRepoRoot("openclaw-bundled-dir-missing-override-"), + "extensions", + ); + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = missingOverride; + delete process.env.OPENCLAW_DISABLE_BUNDLED_PLUGINS; + + const bundledDir = resolveBundledPluginsDir(); + + expect(bundledDir).toBeDefined(); + expect(path.resolve(bundledDir!)).not.toBe(path.resolve(missingOverride)); + }); + + it("falls back to argv root when an existing rejected override is unrelated", () => { + const installedRoot = createOpenClawRoot({ + prefix: "openclaw-bundled-dir-rejected-override-argv-", + hasDistExtensions: true, + }); + seedBundledPluginTree(installedRoot, path.join("dist", "extensions")); + const overrideRoot = makeRepoRoot("openclaw-bundled-dir-rejected-override-"); + seedBundledPluginTree(overrideRoot, "extensions", "memory-core"); + + vi.spyOn(process, "cwd").mockReturnValue(makeRepoRoot("openclaw-bundled-dir-rejected-cwd-")); + process.argv[1] = path.join(installedRoot, "openclaw.mjs"); + process.execArgv.length = 0; + delete process.env.VITEST; + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = path.join(overrideRoot, "extensions"); + delete process.env.OPENCLAW_DISABLE_BUNDLED_PLUGINS; + + const bundledDir = resolveBundledPluginsDir(); + + expect(fs.realpathSync(bundledDir ?? "")).toBe( + fs.realpathSync(path.join(installedRoot, "dist", "extensions")), + ); + }); + + it("does not resolve bundled plugins from cwd when argv1 is not a package root", () => { + const cwdRepoRoot = createOpenClawRoot({ + prefix: "openclaw-bundled-dir-untrusted-cwd-", + hasExtensions: true, + hasSrc: true, + hasGitCheckout: true, + }); + fs.mkdirSync(path.join(cwdRepoRoot, "extensions", "memory-core"), { recursive: true }); + fs.writeFileSync( + path.join(cwdRepoRoot, "extensions", "memory-core", "runtime-api.js"), + "export const marker = 'untrusted-cwd';\n", + "utf8", + ); + vi.spyOn(process, "cwd").mockReturnValue(cwdRepoRoot); + process.argv[1] = "/usr/bin/env"; + process.execArgv.length = 0; + delete process.env.VITEST; + delete process.env.OPENCLAW_BUNDLED_PLUGINS_DIR; + delete process.env.OPENCLAW_DISABLE_BUNDLED_PLUGINS; + + const bundledDir = resolveBundledPluginsDir(); + + expect(bundledDir).toBeDefined(); + expect(fs.realpathSync(bundledDir!)).not.toBe( + fs.realpathSync(path.join(cwdRepoRoot, "extensions")), + ); + }); + it.each([ { name: "prefers the running CLI package root over an unrelated cwd checkout", diff --git a/src/plugins/bundled-dir.ts b/src/plugins/bundled-dir.ts index aa485410481..6f3108e123e 100644 --- a/src/plugins/bundled-dir.ts +++ b/src/plugins/bundled-dir.ts @@ -46,6 +46,65 @@ function hasUsableBundledPluginTree(pluginsDir: string): boolean { } } +function safeRealpathSync(targetPath: string): string | null { + try { + return fs.realpathSync.native(targetPath); + } catch { + return null; + } +} + +function pathContains(parentDir: string, childPath: string): boolean { + const relative = path.relative(parentDir, childPath); + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); +} + +function trustedBundledPluginRootsForPackageRoot(packageRoot: string): string[] { + const roots = [ + path.join(packageRoot, "dist", "extensions"), + path.join(packageRoot, "dist-runtime", "extensions"), + ]; + if (isSourceCheckoutRoot(packageRoot)) { + roots.push(path.join(packageRoot, "extensions")); + } + return roots; +} + +function resolveTrustedExistingOverride(resolvedOverride: string): string | null { + const realOverride = safeRealpathSync(resolvedOverride); + if (!realOverride) { + return null; + } + + const modulePackageRoot = resolveOpenClawPackageRootSync({ moduleUrl: import.meta.url }); + const packageRoots = modulePackageRoot ? [modulePackageRoot] : []; + const trustedRoots = packageRoots + .flatMap((packageRoot) => trustedBundledPluginRootsForPackageRoot(packageRoot)) + .map((trustedRoot) => safeRealpathSync(trustedRoot)) + .filter((entry): entry is string => Boolean(entry)); + if (!trustedRoots.some((trustedRoot) => pathContains(trustedRoot, realOverride))) { + return null; + } + if (!hasUsableBundledPluginTree(realOverride)) { + return null; + } + return realOverride; +} + +function overrideResolvesUnderPackageBundledRoot(params: { + resolvedOverride: string; + packageRoot: string; +}): boolean { + const realOverride = safeRealpathSync(params.resolvedOverride); + if (!realOverride) { + return false; + } + return trustedBundledPluginRootsForPackageRoot(params.packageRoot) + .map((trustedRoot) => safeRealpathSync(trustedRoot)) + .filter((entry): entry is string => Boolean(entry)) + .some((trustedRoot) => pathContains(trustedRoot, realOverride)); +} + function runningSourceTypeScriptProcess(): boolean { const argv1 = process.argv[1]?.toLowerCase(); if ( @@ -83,7 +142,8 @@ function resolveBundledDirFromPackageRoot( const sourceExtensionsDir = path.join(packageRoot, "extensions"); const builtExtensionsDir = path.join(packageRoot, "dist", "extensions"); const sourceCheckout = isSourceCheckoutRoot(packageRoot); - if (preferSourceCheckout && fs.existsSync(sourceExtensionsDir)) { + const hasUsableSourceTree = sourceCheckout && hasUsableBundledPluginTree(sourceExtensionsDir); + if (preferSourceCheckout && hasUsableSourceTree) { return sourceExtensionsDir; } // Local source checkouts stage a runtime-complete bundled plugin tree under @@ -102,7 +162,7 @@ function resolveBundledDirFromPackageRoot( if (hasUsableBuiltTree) { return builtExtensionsDir; } - if (sourceCheckout && fs.existsSync(sourceExtensionsDir)) { + if (hasUsableSourceTree) { return sourceExtensionsDir; } return undefined; @@ -114,37 +174,33 @@ export function resolveBundledPluginsDir(env: NodeJS.ProcessEnv = process.env): } const override = env.OPENCLAW_BUNDLED_PLUGINS_DIR?.trim(); + let rejectedExistingOverride: string | null = null; if (override) { const resolvedOverride = resolveUserPath(override, env); if (fs.existsSync(resolvedOverride)) { - return resolvedOverride; - } - // Installed CLIs can inherit stale bundled-dir overrides from older shells - // or debug sessions. Prefer the package that owns argv[1] over a broken - // override so bundled providers keep working in packaged installs. - try { - const argvPackageRoot = resolveOpenClawPackageRootSync({ argv1: process.argv[1] }); - if (argvPackageRoot && !isSourceCheckoutRoot(argvPackageRoot)) { - const argvFallback = resolveBundledDirFromPackageRoot(argvPackageRoot, false); - if (argvFallback) { - return argvFallback; - } + const trustedOverride = resolveTrustedExistingOverride(resolvedOverride); + if (trustedOverride) { + return trustedOverride; } - } catch { - // ignore + rejectedExistingOverride = resolvedOverride; } - return resolvedOverride; } - const preferSourceCheckout = Boolean(env.VITEST) || runningSourceTypeScriptProcess(); + const preferSourceCheckout = runningSourceTypeScriptProcess(); try { const argvRoot = resolveOpenClawPackageRootSync({ argv1: process.argv[1] }); - const cwdRoot = resolveOpenClawPackageRootSync({ cwd: process.cwd() }); + const rejectedOverrideUsesArgvRoot = Boolean( + argvRoot && + rejectedExistingOverride && + overrideResolvesUnderPackageBundledRoot({ + resolvedOverride: rejectedExistingOverride, + packageRoot: argvRoot, + }), + ); + const safeArgvRoot = rejectedOverrideUsesArgvRoot ? null : argvRoot; const moduleRoot = resolveOpenClawPackageRootSync({ moduleUrl: import.meta.url }); - const packageRoots = ( - preferSourceCheckout ? [cwdRoot, argvRoot, moduleRoot] : [argvRoot, cwdRoot, moduleRoot] - ).filter( + const packageRoots = [safeArgvRoot, moduleRoot].filter( (entry, index, all): entry is string => Boolean(entry) && all.indexOf(entry) === index, ); for (const packageRoot of packageRoots) { diff --git a/src/plugins/source-display.test.ts b/src/plugins/source-display.test.ts index 0229af98db8..b96e5d350f1 100644 --- a/src/plugins/source-display.test.ts +++ b/src/plugins/source-display.test.ts @@ -1,6 +1,7 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; import { withPathResolutionEnv } from "../test-utils/env.js"; +import { resolveBundledPluginsDir } from "./bundled-dir.js"; import { formatPluginSourceForTable, resolvePluginSourceRoots } from "./source-display.js"; const PLUGIN_SOURCE_ROOTS = { @@ -71,17 +72,20 @@ describe("formatPluginSourceForTable", () => { createFormattedSourceExpectation("global", "global", "demo-global", "index.js"), ])("shortens $origin sources under the $sourceKey root", expectFormattedSourceCase); - it("resolves source roots from an explicit env override", () => { + it("ignores untrusted explicit env override for the stock source root", () => { const homeDir = path.resolve(path.sep, "tmp", "openclaw-home"); + const rawEnv = { + OPENCLAW_BUNDLED_PLUGINS_DIR: "~/bundled", + OPENCLAW_STATE_DIR: "~/state", + } as NodeJS.ProcessEnv; + const stock = withPathResolutionEnv(homeDir, rawEnv, (env) => resolveBundledPluginsDir(env)); + expect(stock).toBeDefined(); expectResolvedSourceRoots({ homeDir, - env: { - OPENCLAW_BUNDLED_PLUGINS_DIR: "~/bundled", - OPENCLAW_STATE_DIR: "~/state", - } as NodeJS.ProcessEnv, + env: rawEnv, workspaceDir: "~/ws", expected: { - stock: path.join(homeDir, "bundled"), + stock: stock!, global: path.join(homeDir, "state", "extensions"), workspace: path.join(homeDir, "ws", ".openclaw", "extensions"), },