refactor(plugins): extract safety and provenance helpers

2026-02-19 15:24:02 +01:00
parent 775816035e
commit 77c748304b
6 changed files with 282 additions and 152 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,7 @@ Docs: https://docs.openclaw.ai
 - Security/ACP: harden ACP bridge session management with duplicate-session refresh, idle-session reaping, oldest-idle soft-cap eviction, and burst rate limiting on session creation to reduce local DoS risk without disrupting normal IDE usage.
 - Security/Plugins/Hooks: add optional `--pin` for npm plugin/hook installs, persist resolved npm metadata (`name`, `version`, `spec`, integrity, shasum, timestamp), warn/confirm on integrity drift during updates, and extend `openclaw security audit` to flag unpinned specs, missing integrity metadata, and install-record version drift.
 - Security/Plugins: harden plugin discovery by blocking unsafe candidates (root escapes, world-writable paths, suspicious ownership), add startup warnings when `plugins.allow` is empty with discoverable non-bundled plugins, and warn on loaded plugins without install/load-path provenance.
+- Refactor/Plugins: extract shared plugin path-safety utilities, split discovery safety checks into typed reasoned guards, precompute provenance matchers during plugin load, and switch ownership tests to injected uid inputs.
 - Security/Gateway: rate-limit control-plane write RPCs (`config.apply`, `config.patch`, `update.run`) to 3 requests per minute per `deviceId+clientIp`, add restart single-flight coalescing plus a 30-second restart cooldown, and log actor/device/ip with changed-path audit details for config/update-triggered restarts.
 - Commands/Doctor: skip embedding-provider warnings when `memory.backend` is `qmd`, because QMD manages embeddings internally and does not require `memorySearch` providers. (#17263) Thanks @miloudbelarebia.
 - Security/Webhooks: harden Feishu and Zalo webhook ingress with webhook-mode token preconditions, loopback-default Feishu bind host, JSON content-type enforcement, per-path rate limiting, replay dedupe for Zalo events, constant-time Zalo secret comparison, and anomaly status counters.
--- a/src/plugins/discovery.test.ts
+++ b/src/plugins/discovery.test.ts
@@ -206,21 +206,14 @@ describe("discoverOpenClawPlugins", () => {
        "utf-8",
      );

-      const proc = process as NodeJS.Process & { getuid: () => number };
-      const originalGetUid = proc.getuid;
-      const actualUid = originalGetUid();
-      try {
-        proc.getuid = () => actualUid + 1;
-        const result = await withStateDir(stateDir, async () => {
-          return discoverOpenClawPlugins({});
-        });
-        expect(result.candidates).toHaveLength(0);
-        expect(
-          result.diagnostics.some((diag) => diag.message.includes("suspicious ownership")),
-        ).toBe(true);
-      } finally {
-        proc.getuid = originalGetUid;
-      }
+      const actualUid = (process as NodeJS.Process & { getuid: () => number }).getuid();
+      const result = await withStateDir(stateDir, async () => {
+        return discoverOpenClawPlugins({ ownershipUid: actualUid + 1 });
+      });
+      expect(result.candidates).toHaveLength(0);
+      expect(result.diagnostics.some((diag) => diag.message.includes("suspicious ownership"))).toBe(
+        true,
+      );
    },
  );
 });
--- a/src/plugins/discovery.ts
+++ b/src/plugins/discovery.ts
@@ -7,6 +7,7 @@ import {
  type OpenClawPackageManifest,
  type PackageManifest,
 } from "./manifest.js";
+import { formatPosixMode, isPathInside, safeRealpathSync, safeStatSync } from "./path-safety.js";
 import type { PluginDiagnostic, PluginOrigin } from "./types.js";

 const EXTENSION_EXTS = new Set([".ts", ".js", ".mts", ".cts", ".mjs", ".cjs"]);
@@ -29,35 +30,10 @@ export type PluginDiscoveryResult = {
  diagnostics: PluginDiagnostic[];
 };

-function isPathInside(baseDir: string, targetPath: string): boolean {
-  const rel = path.relative(baseDir, targetPath);
-  if (!rel) {
-    return true;
+function currentUid(overrideUid?: number | null): number | null {
+  if (overrideUid !== undefined) {
+    return overrideUid;
  }
-  return !rel.startsWith("..") && !path.isAbsolute(rel);
-}
-
-function safeRealpathSync(targetPath: string): string | null {
-  try {
-    return fs.realpathSync(targetPath);
-  } catch {
-    return null;
-  }
-}
-
-function safeStatSync(targetPath: string): fs.Stats | null {
-  try {
-    return fs.statSync(targetPath);
-  } catch {
-    return null;
-  }
-}
-
-function formatMode(mode: number): string {
-  return (mode & 0o777).toString(8).padStart(3, "0");
-}
-
-function currentUid(): number | null {
  if (process.platform === "win32") {
    return null;
  }
@@ -67,28 +43,55 @@ function currentUid(): number | null {
  return process.getuid();
 }

-function isUnsafePluginCandidate(params: {
+export type CandidateBlockReason =
+  | "source_escapes_root"
+  | "path_stat_failed"
+  | "path_world_writable"
+  | "path_suspicious_ownership";
+
+type CandidateBlockIssue = {
+  reason: CandidateBlockReason;
+  sourcePath: string;
+  rootPath: string;
+  targetPath: string;
+  sourceRealPath?: string;
+  rootRealPath?: string;
+  modeBits?: number;
+  foundUid?: number;
+  expectedUid?: number;
+};
+
+function checkSourceEscapesRoot(params: {
+  source: string;
+  rootDir: string;
+}): CandidateBlockIssue | null {
+  const sourceRealPath = safeRealpathSync(params.source);
+  const rootRealPath = safeRealpathSync(params.rootDir);
+  if (!sourceRealPath || !rootRealPath) {
+    return null;
+  }
+  if (isPathInside(rootRealPath, sourceRealPath)) {
+    return null;
+  }
+  return {
+    reason: "source_escapes_root",
+    sourcePath: params.source,
+    rootPath: params.rootDir,
+    targetPath: params.source,
+    sourceRealPath,
+    rootRealPath,
+  };
+}
+
+function checkPathStatAndPermissions(params: {
  source: string;
  rootDir: string;
  origin: PluginOrigin;
-  diagnostics: PluginDiagnostic[];
-}): boolean {
-  const sourceReal = safeRealpathSync(params.source);
-  const rootReal = safeRealpathSync(params.rootDir);
-  if (sourceReal && rootReal && !isPathInside(rootReal, sourceReal)) {
-    params.diagnostics.push({
-      level: "warn",
-      source: params.source,
-      message: `blocked plugin candidate: source escapes plugin root (${params.source} -> ${sourceReal}; root=${rootReal})`,
-    });
-    return true;
-  }
-
+  uid: number | null;
+}): CandidateBlockIssue | null {
  if (process.platform === "win32") {
-    return false;
+    return null;
  }
-
-  const uid = currentUid();
  const pathsToCheck = [params.rootDir, params.source];
  const seen = new Set<string>();
  for (const targetPath of pathsToCheck) {
@@ -99,39 +102,99 @@ function isUnsafePluginCandidate(params: {
    seen.add(normalized);
    const stat = safeStatSync(targetPath);
    if (!stat) {
-      params.diagnostics.push({
-        level: "warn",
-        source: targetPath,
-        message: `blocked plugin candidate: cannot stat path (${targetPath})`,
-      });
-      return true;
+      return {
+        reason: "path_stat_failed",
+        sourcePath: params.source,
+        rootPath: params.rootDir,
+        targetPath,
+      };
    }
    const modeBits = stat.mode & 0o777;
    if ((modeBits & 0o002) !== 0) {
-      params.diagnostics.push({
-        level: "warn",
-        source: targetPath,
-        message: `blocked plugin candidate: world-writable path (${targetPath}, mode=${formatMode(modeBits)})`,
-      });
-      return true;
+      return {
+        reason: "path_world_writable",
+        sourcePath: params.source,
+        rootPath: params.rootDir,
+        targetPath,
+        modeBits,
+      };
    }
    if (
      params.origin !== "bundled" &&
-      uid !== null &&
+      params.uid !== null &&
      typeof stat.uid === "number" &&
-      stat.uid !== uid &&
+      stat.uid !== params.uid &&
      stat.uid !== 0
    ) {
-      params.diagnostics.push({
-        level: "warn",
-        source: targetPath,
-        message: `blocked plugin candidate: suspicious ownership (${targetPath}, uid=${stat.uid}, expected uid=${uid} or root)`,
-      });
-      return true;
+      return {
+        reason: "path_suspicious_ownership",
+        sourcePath: params.source,
+        rootPath: params.rootDir,
+        targetPath,
+        foundUid: stat.uid,
+        expectedUid: params.uid,
+      };
    }
  }
+  return null;
+}

-  return false;
+function findCandidateBlockIssue(params: {
+  source: string;
+  rootDir: string;
+  origin: PluginOrigin;
+  ownershipUid?: number | null;
+}): CandidateBlockIssue | null {
+  const escaped = checkSourceEscapesRoot({
+    source: params.source,
+    rootDir: params.rootDir,
+  });
+  if (escaped) {
+    return escaped;
+  }
+  return checkPathStatAndPermissions({
+    source: params.source,
+    rootDir: params.rootDir,
+    origin: params.origin,
+    uid: currentUid(params.ownershipUid),
+  });
+}
+
+function formatCandidateBlockMessage(issue: CandidateBlockIssue): string {
+  if (issue.reason === "source_escapes_root") {
+    return `blocked plugin candidate: source escapes plugin root (${issue.sourcePath} -> ${issue.sourceRealPath}; root=${issue.rootRealPath})`;
+  }
+  if (issue.reason === "path_stat_failed") {
+    return `blocked plugin candidate: cannot stat path (${issue.targetPath})`;
+  }
+  if (issue.reason === "path_world_writable") {
+    return `blocked plugin candidate: world-writable path (${issue.targetPath}, mode=${formatPosixMode(issue.modeBits ?? 0)})`;
+  }
+  return `blocked plugin candidate: suspicious ownership (${issue.targetPath}, uid=${issue.foundUid}, expected uid=${issue.expectedUid} or root)`;
+}
+
+function isUnsafePluginCandidate(params: {
+  source: string;
+  rootDir: string;
+  origin: PluginOrigin;
+  diagnostics: PluginDiagnostic[];
+  ownershipUid?: number | null;
+}): boolean {
+  const issue = findCandidateBlockIssue({
+    source: params.source,
+    rootDir: params.rootDir,
+    origin: params.origin,
+    ownershipUid: params.ownershipUid,
+  });
+  if (!issue) {
+    return false;
+  }
+  params.diagnostics.push({
+    level: "warn",
+    source: issue.targetPath,
+    message: formatCandidateBlockMessage(issue),
+  });
+  return true;
 }

 function isExtensionFile(filePath: string): boolean {
@@ -194,6 +257,7 @@ function addCandidate(params: {
  source: string;
  rootDir: string;
  origin: PluginOrigin;
+  ownershipUid?: number | null;
  workspaceDir?: string;
  manifest?: PackageManifest | null;
  packageDir?: string;
@@ -209,6 +273,7 @@ function addCandidate(params: {
      rootDir: resolvedRoot,
      origin: params.origin,
      diagnostics: params.diagnostics,
+      ownershipUid: params.ownershipUid,
    })
  ) {
    return;
@@ -232,6 +297,7 @@ function addCandidate(params: {
 function discoverInDirectory(params: {
  dir: string;
  origin: PluginOrigin;
+  ownershipUid?: number | null;
  workspaceDir?: string;
  candidates: PluginCandidate[];
  diagnostics: PluginDiagnostic[];
@@ -266,6 +332,7 @@ function discoverInDirectory(params: {
        source: fullPath,
        rootDir: path.dirname(fullPath),
        origin: params.origin,
+        ownershipUid: params.ownershipUid,
        workspaceDir: params.workspaceDir,
      });
    }
@@ -291,6 +358,7 @@ function discoverInDirectory(params: {
          source: resolved,
          rootDir: fullPath,
          origin: params.origin,
+          ownershipUid: params.ownershipUid,
          workspaceDir: params.workspaceDir,
          manifest,
          packageDir: fullPath,
@@ -312,6 +380,7 @@ function discoverInDirectory(params: {
        source: indexFile,
        rootDir: fullPath,
        origin: params.origin,
+        ownershipUid: params.ownershipUid,
        workspaceDir: params.workspaceDir,
        manifest,
        packageDir: fullPath,
@@ -323,6 +392,7 @@ function discoverInDirectory(params: {
 function discoverFromPath(params: {
  rawPath: string;
  origin: PluginOrigin;
+  ownershipUid?: number | null;
  workspaceDir?: string;
  candidates: PluginCandidate[];
  diagnostics: PluginDiagnostic[];
@@ -356,6 +426,7 @@ function discoverFromPath(params: {
      source: resolved,
      rootDir: path.dirname(resolved),
      origin: params.origin,
+      ownershipUid: params.ownershipUid,
      workspaceDir: params.workspaceDir,
    });
    return;
@@ -380,6 +451,7 @@ function discoverFromPath(params: {
          source,
          rootDir: resolved,
          origin: params.origin,
+          ownershipUid: params.ownershipUid,
          workspaceDir: params.workspaceDir,
          manifest,
          packageDir: resolved,
@@ -402,6 +474,7 @@ function discoverFromPath(params: {
        source: indexFile,
        rootDir: resolved,
        origin: params.origin,
+        ownershipUid: params.ownershipUid,
        workspaceDir: params.workspaceDir,
        manifest,
        packageDir: resolved,
@@ -412,6 +485,7 @@ function discoverFromPath(params: {
    discoverInDirectory({
      dir: resolved,
      origin: params.origin,
+      ownershipUid: params.ownershipUid,
      workspaceDir: params.workspaceDir,
      candidates: params.candidates,
      diagnostics: params.diagnostics,
@@ -424,6 +498,7 @@ function discoverFromPath(params: {
 export function discoverOpenClawPlugins(params: {
  workspaceDir?: string;
  extraPaths?: string[];
+  ownershipUid?: number | null;
 }): PluginDiscoveryResult {
  const candidates: PluginCandidate[] = [];
  const diagnostics: PluginDiagnostic[] = [];
@@ -442,6 +517,7 @@ export function discoverOpenClawPlugins(params: {
    discoverFromPath({
      rawPath: trimmed,
      origin: "config",
+      ownershipUid: params.ownershipUid,
      workspaceDir: workspaceDir?.trim() || undefined,
      candidates,
      diagnostics,
@@ -455,6 +531,7 @@ export function discoverOpenClawPlugins(params: {
      discoverInDirectory({
        dir,
        origin: "workspace",
+        ownershipUid: params.ownershipUid,
        workspaceDir: workspaceRoot,
        candidates,
        diagnostics,
@@ -467,6 +544,7 @@ export function discoverOpenClawPlugins(params: {
  discoverInDirectory({
    dir: globalDir,
    origin: "global",
+    ownershipUid: params.ownershipUid,
    candidates,
    diagnostics,
    seen,
@@ -477,6 +555,7 @@ export function discoverOpenClawPlugins(params: {
    discoverInDirectory({
      dir: bundledDir,
      origin: "bundled",
+      ownershipUid: params.ownershipUid,
      candidates,
      diagnostics,
      seen,
--- a/src/plugins/loader.ts
+++ b/src/plugins/loader.ts
@@ -17,6 +17,7 @@ import {
 import { discoverOpenClawPlugins } from "./discovery.js";
 import { initializeGlobalHookRunner } from "./hook-runner-global.js";
 import { loadPluginManifestRegistry } from "./manifest-registry.js";
+import { isPathInside, safeStatSync } from "./path-safety.js";
 import { createPluginRegistry, type PluginRecord, type PluginRegistry } from "./registry.js";
 import { setActivePluginRegistry } from "./runtime.js";
 import { createPluginRuntime } from "./runtime/index.js";
@@ -177,61 +178,100 @@ function pushDiagnostics(diagnostics: PluginDiagnostic[], append: PluginDiagnost
  diagnostics.push(...append);
 }

-function isPathInside(baseDir: string, targetPath: string): boolean {
-  const rel = path.relative(baseDir, targetPath);
-  if (!rel) {
-    return true;
-  }
-  return !rel.startsWith("..") && !path.isAbsolute(rel);
+type PathMatcher = {
+  exact: Set<string>;
+  dirs: string[];
+};
+
+type InstallTrackingRule = {
+  trackedWithoutPaths: boolean;
+  matcher: PathMatcher;
+};
+
+type PluginProvenanceIndex = {
+  loadPathMatcher: PathMatcher;
+  installRules: Map<string, InstallTrackingRule>;
+};
+
+function createPathMatcher(): PathMatcher {
+  return { exact: new Set<string>(), dirs: [] };
 }

-function pathMatchesBaseOrFile(params: { baseOrFile: string; targetFile: string }): boolean {
-  const baseResolved = resolveUserPath(params.baseOrFile);
-  const targetResolved = resolveUserPath(params.targetFile);
-  if (baseResolved === targetResolved) {
+function addPathToMatcher(matcher: PathMatcher, rawPath: string): void {
+  const trimmed = rawPath.trim();
+  if (!trimmed) {
+    return;
+  }
+  const resolved = resolveUserPath(trimmed);
+  if (!resolved) {
+    return;
+  }
+  if (matcher.exact.has(resolved) || matcher.dirs.includes(resolved)) {
+    return;
+  }
+  const stat = safeStatSync(resolved);
+  if (stat?.isDirectory()) {
+    matcher.dirs.push(resolved);
+    return;
+  }
+  matcher.exact.add(resolved);
+}
+
+function matchesPathMatcher(matcher: PathMatcher, sourcePath: string): boolean {
+  if (matcher.exact.has(sourcePath)) {
    return true;
  }
-  try {
-    const stat = fs.statSync(baseResolved);
-    if (!stat.isDirectory()) {
-      return false;
+  return matcher.dirs.some((dirPath) => isPathInside(dirPath, sourcePath));
+}
+
+function buildProvenanceIndex(params: {
+  config: OpenClawConfig;
+  normalizedLoadPaths: string[];
+}): PluginProvenanceIndex {
+  const loadPathMatcher = createPathMatcher();
+  for (const loadPath of params.normalizedLoadPaths) {
+    addPathToMatcher(loadPathMatcher, loadPath);
+  }
+
+  const installRules = new Map<string, InstallTrackingRule>();
+  const installs = params.config.plugins?.installs ?? {};
+  for (const [pluginId, install] of Object.entries(installs)) {
+    const rule: InstallTrackingRule = {
+      trackedWithoutPaths: false,
+      matcher: createPathMatcher(),
+    };
+    const trackedPaths = [install.installPath, install.sourcePath]
+      .map((entry) => (typeof entry === "string" ? entry.trim() : ""))
+      .filter(Boolean);
+    if (trackedPaths.length === 0) {
+      rule.trackedWithoutPaths = true;
+    } else {
+      for (const trackedPath of trackedPaths) {
+        addPathToMatcher(rule.matcher, trackedPath);
+      }
    }
-  } catch {
-    return false;
+    installRules.set(pluginId, rule);
  }
-  return isPathInside(baseResolved, targetResolved);
+
+  return { loadPathMatcher, installRules };
 }

-function isTrackedByInstallRecord(params: {
+function isTrackedByProvenance(params: {
  pluginId: string;
  source: string;
-  config: OpenClawConfig;
+  index: PluginProvenanceIndex;
 }): boolean {
-  const install = params.config.plugins?.installs?.[params.pluginId];
-  if (!install) {
-    return false;
+  const sourcePath = resolveUserPath(params.source);
+  const installRule = params.index.installRules.get(params.pluginId);
+  if (installRule) {
+    if (installRule.trackedWithoutPaths) {
+      return true;
+    }
+    if (matchesPathMatcher(installRule.matcher, sourcePath)) {
+      return true;
+    }
  }
-  const trackedPaths = [install.installPath, install.sourcePath]
-    .map((entry) => (typeof entry === "string" ? entry.trim() : ""))
-    .filter(Boolean);
-  if (trackedPaths.length === 0) {
-    return true;
-  }
-  return trackedPaths.some((trackedPath) =>
-    pathMatchesBaseOrFile({
-      baseOrFile: trackedPath,
-      targetFile: params.source,
-    }),
-  );
-}
-
-function isTrackedByLoadPath(params: { source: string; loadPaths: string[] }): boolean {
-  return params.loadPaths.some((loadPath) =>
-    pathMatchesBaseOrFile({
-      baseOrFile: loadPath,
-      targetFile: params.source,
-    }),
-  );
+  return matchesPathMatcher(params.index.loadPathMatcher, sourcePath);
 }

 function warnWhenAllowlistIsOpen(params: {
@@ -262,8 +302,7 @@ function warnWhenAllowlistIsOpen(params: {

 function warnAboutUntrackedLoadedPlugins(params: {
  registry: PluginRegistry;
-  config: OpenClawConfig;
-  normalizedLoadPaths: string[];
+  provenance: PluginProvenanceIndex;
  logger: PluginLogger;
 }) {
  for (const plugin of params.registry.plugins) {
@@ -271,18 +310,10 @@ function warnAboutUntrackedLoadedPlugins(params: {
      continue;
    }
    if (
-      isTrackedByInstallRecord({
+      isTrackedByProvenance({
        pluginId: plugin.id,
        source: plugin.source,
-        config: params.config,
-      })
-    ) {
-      continue;
-    }
-    if (
-      isTrackedByLoadPath({
-        source: plugin.source,
-        loadPaths: params.normalizedLoadPaths,
+        index: params.provenance,
      })
    ) {
      continue;
@@ -351,6 +382,10 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi
      origin: plugin.origin,
    })),
  });
+  const provenance = buildProvenanceIndex({
+    config: cfg,
+    normalizedLoadPaths: normalized.loadPaths,
+  });

  // Lazy: avoid creating the Jiti loader when all plugins are disabled (common in unit tests).
  let jitiLoader: ReturnType<typeof createJiti> | null = null;
@@ -605,8 +640,7 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi

  warnAboutUntrackedLoadedPlugins({
    registry,
-    config: cfg,
-    normalizedLoadPaths: normalized.loadPaths,
+    provenance,
    logger,
  });

--- a/src/plugins/manifest-registry.ts
+++ b/src/plugins/manifest-registry.ts
@@ -4,6 +4,7 @@ import { resolveUserPath } from "../utils.js";
 import { normalizePluginsConfig, type NormalizedPluginsConfig } from "./config-state.js";
 import { discoverOpenClawPlugins, type PluginCandidate } from "./discovery.js";
 import { loadPluginManifest, type PluginManifest } from "./manifest.js";
+import { safeRealpathSync } from "./path-safety.js";
 import type { PluginConfigUiHint, PluginDiagnostic, PluginKind, PluginOrigin } from "./types.js";

 type SeenIdEntry = {
@@ -19,20 +20,6 @@ const PLUGIN_ORIGIN_RANK: Readonly<Record<PluginOrigin, number>> = {
  bundled: 3,
 };

-function safeRealpathSync(rootDir: string, cache: Map<string, string>): string | null {
-  const cached = cache.get(rootDir);
-  if (cached) {
-    return cached;
-  }
-  try {
-    const resolved = fs.realpathSync(rootDir);
-    cache.set(rootDir, resolved);
-    return resolved;
-  } catch {
-    return null;
-  }
-}
-
 export type PluginManifestRecord = {
  id: string;
  name?: string;
--- a/src/plugins/path-safety.ts
+++ b/src/plugins/path-safety.ts
@@ -0,0 +1,36 @@
+import fs from "node:fs";
+import path from "node:path";
+
+export function isPathInside(baseDir: string, targetPath: string): boolean {
+  const rel = path.relative(baseDir, targetPath);
+  if (!rel) {
+    return true;
+  }
+  return !rel.startsWith("..") && !path.isAbsolute(rel);
+}
+
+export function safeRealpathSync(targetPath: string, cache?: Map<string, string>): string | null {
+  const cached = cache?.get(targetPath);
+  if (cached) {
+    return cached;
+  }
+  try {
+    const resolved = fs.realpathSync(targetPath);
+    cache?.set(targetPath, resolved);
+    return resolved;
+  } catch {
+    return null;
+  }
+}
+
+export function safeStatSync(targetPath: string): fs.Stats | null {
+  try {
+    return fs.statSync(targetPath);
+  } catch {
+    return null;
+  }
+}
+
+export function formatPosixMode(mode: number): string {
+  return (mode & 0o777).toString(8).padStart(3, "0");
+}