CL-8991: grant path trust before importing add-by-path plugin code

Author: TheGreatAxios

src/plugins/loader.ts

@@ -223,6 +223,99 @@ async function readPluginMetadataOnly(
   };
 }
 
+// JS entry filenames a plugin directory may carry. A directory with one of
+// these can export its manifest inline (no manifest.json), so presence alone
+// marks the entry as plugin-shaped — see loadPluginEntryMetadata.
+const PLUGIN_JS_ENTRY_CANDIDATES = ["src/index.ts", "index.ts", "index.js"];
+
+/**
+ * No-import metadata probe for explicit add-by-path: reports whether
+ * `entryPath` resolves to something a post-consent `loadPluginEntry` could
+ * load, without executing any plugin code. Returns a metadata-only stub when
+ * plugin-shaped, null when bogus (missing/unreadable, or nothing loadable).
+ * Manifest-backed and data-only layouts resolve their manifest now; a JS
+ * entry with no manifest.json may still export an inline manifest, so its
+ * presence alone counts — the full load after the trust grant re-derives the
+ * manifest and reports the same user-facing errors. Exported so the /plugins
+ * UI can grant path trust before the first import.
+ */
+export async function loadPluginEntryMetadata(
+  entryPath: string,
+  opts: {
+    cwd?: string;
+    onWarning?: (msg: string) => void;
+    diagnostics?: PluginLoadDiagnostics;
+    origin: PluginOrigin;
+  },
+): Promise<PluginModule | null> {
+  const cwd = opts.cwd ?? process.cwd();
+  // Prefer diagnostics collector; else explicit onWarning; else stderrPluginWarning.
+  const onWarning = resolvePluginWarningHandler(
+    opts.diagnostics !== undefined
+      ? { diagnostics: opts.diagnostics }
+      : opts.onWarning !== undefined
+        ? { onWarning: opts.onWarning }
+        : { onWarning: stderrPluginWarning },
+  );
+  let dir = entryPath;
+  try {
+    const info = await stat(entryPath);
+    if (!info.isDirectory()) dir = dirname(entryPath);
+  } catch {
+    // Path missing or unreadable — not a plugin.
+    return null;
+  }
+  const abs = resolve(dir);
+  const manifest =
+    (await readManifestJson(abs, onWarning)) ??
+    (await readClaudeFormatManifestJson(
+      join(abs, ".claude-plugin"),
+      onWarning,
+    ));
+  if (manifest !== null) {
+    return {
+      dir: abs,
+      manifest,
+      origin: opts.origin,
+      pluginPath: abs,
+      metadataOnly: true,
+    };
+  }
+  // Manifest-less data-only layout (agents/commands/skills markdown): no JS
+  // to import, so resolving it now is safe.
+  const dataOnly = await loadDataOnlyPlugin(abs, {
+    cwd,
+    ...(opts.diagnostics !== undefined
+      ? { diagnostics: opts.diagnostics }
+      : { onWarning }),
+  });
+  if (dataOnly !== null) {
+    return {
+      dir: abs,
+      manifest: dataOnly.manifest,
+      origin: opts.origin,
+      pluginPath: abs,
+      metadataOnly: true,
+    };
+  }
+  // JS entry whose manifest may be an inline export — knowable only by
+  // importing, which happens after the trust grant.
+  for (const candidate of PLUGIN_JS_ENTRY_CANDIDATES) {
+    try {
+      await stat(join(abs, candidate));
+      return {
+        dir: abs,
+        origin: opts.origin,
+        pluginPath: abs,
+        metadataOnly: true,
+      };
+    } catch {
+      // Candidate missing; try the next index filename.
+    }
+  }
+  return null;
+}
+
 // Attempt to load a single plugin directory entry (a file or a directory with
 // an index file). Returns null if the entry cannot be resolved to a module.
 // Exported so the /plugins UI can register a plugin from an arbitrary path.
@@ -265,7 +358,7 @@ export async function loadPluginEntry(
     if (info.isDirectory()) {
       pluginDir = entryPath;
       // Prefer src/index.ts, then index.ts, then index.js.
-      for (const candidate of ["src/index.ts", "index.ts", "index.js"]) {
+      for (const candidate of PLUGIN_JS_ENTRY_CANDIDATES) {
         const candidatePath = join(entryPath, candidate);
         try {
           await stat(candidatePath);

src/trust/path-plugin-trust.test.ts

@@ -1,7 +1,10 @@
 import { describe, expect, test } from "bun:test";
 import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
-import { join } from "node:path";
+import { isAbsolute, join } from "node:path";
+import type { GlobalSettingsWriter } from "../../src/mcp/add-server.js";
+import type { ProjectTrustStore } from "../../src/trust/project-trust.js";
+import { withMockedModule } from "../helpers/mock-module.js";
 import {
   dedupePluginModules,
   discoverUserPlugins,
@@ -305,3 +308,213 @@ describe("path plugin trust across working directories", () => {
     }
   });
 });
+
+// The /plugins add-by-path ordering probe (CL-8991): `trustPathPlugins` is
+// mocked once for this file so each `addPath` below can observe whether any
+// plugin code ran before the grant resolved. The wrapper delegates to the
+// real store with an explicit home, so behavior — including the existing
+// tests above, which always pass their own home — is unchanged.
+const addPathProbe = {
+  home: "",
+  marker: "",
+  trustCalls: [] as string[][],
+  markerAtTrust: [] as boolean[],
+};
+
+function resetAddPathProbe(home: string, marker: string): void {
+  addPathProbe.home = home;
+  addPathProbe.marker = marker;
+  addPathProbe.trustCalls = [];
+  addPathProbe.markerAtTrust = [];
+}
+
+await withMockedModule(
+  import.meta.resolve("../../src/trust/path-trust.js"),
+  (real: typeof import("../../src/trust/path-trust.js")) => ({
+    ...real,
+    trustPathPlugins: async (paths: string[], home?: string) => {
+      addPathProbe.trustCalls.push([...paths]);
+      addPathProbe.markerAtTrust.push(
+        await Bun.file(addPathProbe.marker).exists(),
+      );
+      return real.trustPathPlugins(paths, home ?? addPathProbe.home);
+    },
+  }),
+);
+
+function stubSettingsWriter(): GlobalSettingsWriter {
+  return {
+    enqueue: async <T>(job: () => Promise<T>): Promise<T> => job(),
+    update: async () => null,
+    updateAt: async () => null,
+    mutate: async () => "ok" as const,
+    mutateAt: async () => "ok" as const,
+  };
+}
+
+async function makeAddPathAdmin(base: string): Promise<{
+  addPath: (
+    path: string,
+  ) => Promise<{ ok: boolean; message: string; id?: string }>;
+}> {
+  const backend = await import("../../src/tui/plugins-admin-backend.js");
+  const emptyProjectTrust: ProjectTrustStore = {
+    trustedPluginPaths: [],
+    trustedMcpFingerprints: [],
+    trustedGrantFingerprints: [],
+  };
+  const state = backend.createPluginsAdminState({
+    cwd: base,
+    settings: undefined,
+    modules: [],
+    pathTrust: { trustedPluginPaths: [] },
+    projectTrust: emptyProjectTrust,
+  });
+  return backend.createPluginsAdmin({
+    state,
+    globalSettingsPath: join(base, "settings.json"),
+    globalSettingsWriter: stubSettingsWriter(),
+    noteWarnings: () => undefined,
+  });
+}
+
+describe("addPath grants path trust before importing plugin code", () => {
+  test("single plugin: no JS runs before trustPathPlugins resolves", async () => {
+    const base = await mkdtemp(join(tmpdir(), "corbits-addpath-order-"));
+    try {
+      const home = join(base, "home");
+      await mkdir(home, { recursive: true });
+      const pluginDir = join(base, "shared-plugin");
+      const marker = join(base, "RCE_MARKER");
+      await writeCommandPlugin(pluginDir, "shared-plugin", marker);
+      resetAddPathProbe(home, marker);
+
+      const admin = await makeAddPathAdmin(base);
+      const result = await admin.addPath("shared-plugin");
+      expect(result).toEqual({
+        ok: true,
+        message: "Added shared-plugin",
+        id: "shared-plugin",
+      });
+
+      expect(addPathProbe.trustCalls).toEqual([[pluginDir]]);
+      for (const p of addPathProbe.trustCalls.flat())
+        expect(isAbsolute(p)).toBe(true);
+      expect(addPathProbe.markerAtTrust).toEqual([false]);
+      expect(await Bun.file(marker).exists()).toBe(true);
+      expect((await loadPathTrust(home)).trustedPluginPaths).toEqual([
+        pluginDir,
+      ]);
+    } finally {
+      await rm(base, { recursive: true, force: true });
+    }
+  });
+
+  test("marketplace root: trust covers expanded members, still before any import", async () => {
+    const base = await mkdtemp(join(tmpdir(), "corbits-addpath-mkt-"));
+    try {
+      const home = join(base, "home");
+      await mkdir(home, { recursive: true });
+      const root = join(base, "marketplace");
+      const marker = join(base, "ROOT_MARKER");
+      await writeCommandPlugin(root, "mkt-root", marker);
+      const alpha = join(root, "plugins", "alpha");
+      const beta = join(root, "plugins", "beta");
+      await writeCommandPlugin(alpha, "alpha");
+      await writeCommandPlugin(beta, "beta");
+      await mkdir(join(root, ".claude-plugin"), { recursive: true });
+      await writeFile(
+        join(root, ".claude-plugin", "marketplace.json"),
+        JSON.stringify({
+          plugins: [
+            { name: "alpha", source: "./plugins/alpha" },
+            { name: "beta", source: "./plugins/beta" },
+          ],
+        }),
+        "utf8",
+      );
+      resetAddPathProbe(home, marker);
+
+      const admin = await makeAddPathAdmin(base);
+      const result = await admin.addPath("marketplace");
+      expect(result).toEqual({
+        ok: true,
+        message: "Added mkt-root",
+        id: "mkt-root",
+      });
+
+      expect(addPathProbe.trustCalls).toEqual([[alpha, beta]]);
+      for (const p of addPathProbe.trustCalls.flat())
+        expect(isAbsolute(p)).toBe(true);
+      expect(addPathProbe.markerAtTrust).toEqual([false]);
+      expect(await Bun.file(marker).exists()).toBe(true);
+      expect((await loadPathTrust(home)).trustedPluginPaths).toEqual([
+        alpha,
+        beta,
+      ]);
+    } finally {
+      await rm(base, { recursive: true, force: true });
+    }
+  });
+
+  test("bogus path grants nothing", async () => {
+    const base = await mkdtemp(join(tmpdir(), "corbits-addpath-bogus-"));
+    try {
+      const home = join(base, "home");
+      await mkdir(home, { recursive: true });
+      resetAddPathProbe(home, join(base, "NEVER"));
+
+      const admin = await makeAddPathAdmin(base);
+      const result = await admin.addPath("does-not-exist");
+      expect(result).toEqual({
+        ok: false,
+        message: "Could not load a plugin at does-not-exist",
+      });
+      expect(addPathProbe.trustCalls).toEqual([]);
+      expect((await loadPathTrust(home)).trustedPluginPaths).toEqual([]);
+    } finally {
+      await rm(base, { recursive: true, force: true });
+    }
+  });
+
+  test("post-trust import failure reports the load error and keeps the grant", async () => {
+    const base = await mkdtemp(join(tmpdir(), "corbits-addpath-broken-"));
+    try {
+      const home = join(base, "home");
+      await mkdir(home, { recursive: true });
+      const pluginDir = join(base, "broken-plugin");
+      await mkdir(pluginDir, { recursive: true });
+      await writeFile(
+        join(pluginDir, "manifest.json"),
+        JSON.stringify({
+          id: "broken-plugin",
+          name: "broken-plugin",
+          kind: "command",
+        }),
+        "utf8",
+      );
+      await writeFile(
+        join(pluginDir, "index.ts"),
+        `throw new Error("boom");\n`,
+        "utf8",
+      );
+      resetAddPathProbe(home, join(base, "NEVER"));
+
+      const admin = await makeAddPathAdmin(base);
+      const result = await admin.addPath("broken-plugin");
+      // Explicit add-by-path is consent: the grant is recorded before the
+      // import runs, so a failed import keeps the grant. Only bogus
+      // (unresolvable) paths return before the grant.
+      expect(result).toEqual({
+        ok: false,
+        message: "Could not load a plugin at broken-plugin",
+      });
+      expect(addPathProbe.trustCalls).toEqual([[pluginDir]]);
+      expect((await loadPathTrust(home)).trustedPluginPaths).toEqual([
+        pluginDir,
+      ]);
+    } finally {
+      await rm(base, { recursive: true, force: true });
+    }
+  });
+});

src/tui/plugins-admin-backend.ts

@@ -17,6 +17,7 @@ import {
   expandPluginPath,
   expandSkipDiagnosticsHandler,
   loadPluginEntry,
+  loadPluginEntryMetadata,
   type PluginModule,
   type PluginOrigin,
 } from "../plugins/loader.js";
@@ -287,8 +288,34 @@ export function createPluginsAdmin(args: {
       const path = rawPath.trim();
       if (path.length === 0) return { ok: false, message: "Enter a path" };
       const abs = isAbsolute(path) ? path : resolvePath(state.cwd, path);
-      // Explicit add-by-path is user consent to load that absolute path.
+      // Explicit add-by-path is user consent to load that absolute path — but
+      // the grant below must resolve before any plugin code runs. Probe
+      // metadata-only first: this never import()s, so a hostile index.ts
+      // cannot execute pre-trust. The probe takes its own diagnostics because
+      // the full load below re-reads the same files.
+      const probeDiag = createPluginLoadDiagnostics();
+      const probe = await loadPluginEntryMetadata(abs, {
+        cwd: state.cwd,
+        origin: "path",
+        diagnostics: probeDiag,
+      });
+      if (probe === null)
+        return { ok: false, message: `Could not load a plugin at ${path}` };
+      // Persist global path trust only once the path resolves to a real
+      // plugin, so a bogus path never leaves a dangling entry. Expand
+      // marketplaces so each member is trusted (exact-path match on reload).
+      // `onSkip` collects into `addDiag` instead of a raw stderr write — a raw
+      // write lands mid-frame and corrupts the rendered transcript.
       const addDiag = createPluginLoadDiagnostics();
+      const members = await expandPluginPath(abs, {
+        onSkip: expandSkipDiagnosticsHandler(addDiag),
+      });
+      state.pathTrust = await trustPathPlugins(
+        members.length > 0 ? members : [abs],
+      );
+      // Only now import plugin code: the grant above is the user's explicit
+      // consent. A failed import keeps the grant and reports the same load
+      // error as before — only bogus paths return grantless above.
       const mod = await loadPluginEntry(abs, {
         cwd: state.cwd,
         origin: "path",
@@ -308,17 +335,6 @@ export function createPluginsAdmin(args: {
       const descriptor = buildPluginDescriptor(mod);
       if (descriptor === undefined)
         return { ok: false, message: "Invalid plugin manifest" };
-      // Persist global path trust only once it resolves to a real plugin, so a
-      // bogus path never leaves a dangling entry. Expand marketplaces so each
-      // member is trusted (exact-path match on reload). `onSkip` collects into
-      // `addDiag` instead of a raw stderr write — same reasoning as
-      // `loadPluginEntry` above.
-      const members = await expandPluginPath(abs, {
-        onSkip: expandSkipDiagnosticsHandler(addDiag),
-      });
-      state.pathTrust = await trustPathPlugins(
-        members.length > 0 ? members : [abs],
-      );
       // Replace any existing descriptor/candidate with the same id so re-adding
       // refreshes rather than duplicates.
       const existingIdx = state.descriptors.findIndex(