fix(security): trust resolved skill-bin paths in allowlist auto-allow

src/infra/exec-approvals-allowlist.ts

@@ -1,3 +1,4 @@
+import path from "node:path";
 import {
   DEFAULT_SAFE_BINS,
   analyzeShellCommand,
@@ -104,6 +105,71 @@ export type ExecAllowlistEvaluation = {
 };
 
 export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | null;
+export type SkillBinTrustEntry = {
+  name: string;
+  resolvedPath: string;
+};
+
+function normalizeSkillBinName(value: string | undefined): string | null {
+  const trimmed = value?.trim().toLowerCase();
+  return trimmed && trimmed.length > 0 ? trimmed : null;
+}
+
+function normalizeSkillBinResolvedPath(value: string | undefined): string | null {
+  const trimmed = value?.trim();
+  if (!trimmed) {
+    return null;
+  }
+  const resolved = path.resolve(trimmed);
+  if (process.platform === "win32") {
+    return resolved.replace(/\\/g, "/").toLowerCase();
+  }
+  return resolved;
+}
+
+function buildSkillBinTrustIndex(
+  entries: readonly SkillBinTrustEntry[] | undefined,
+): Map<string, Set<string>> {
+  const trustByName = new Map<string, Set<string>>();
+  if (!entries || entries.length === 0) {
+    return trustByName;
+  }
+  for (const entry of entries) {
+    const name = normalizeSkillBinName(entry.name);
+    const resolvedPath = normalizeSkillBinResolvedPath(entry.resolvedPath);
+    if (!name || !resolvedPath) {
+      continue;
+    }
+    const paths = trustByName.get(name) ?? new Set<string>();
+    paths.add(resolvedPath);
+    trustByName.set(name, paths);
+  }
+  return trustByName;
+}
+
+function isSkillAutoAllowedSegment(params: {
+  segment: ExecCommandSegment;
+  allowSkills: boolean;
+  skillBinTrust: ReadonlyMap<string, ReadonlySet<string>>;
+}): boolean {
+  if (!params.allowSkills) {
+    return false;
+  }
+  const resolution = params.segment.resolution;
+  if (!resolution?.resolvedPath) {
+    return false;
+  }
+  const rawExecutable = resolution.rawExecutable?.trim() ?? "";
+  if (!rawExecutable || isPathScopedExecutableToken(rawExecutable)) {
+    return false;
+  }
+  const executableName = normalizeSkillBinName(resolution.executableName);
+  const resolvedPath = normalizeSkillBinResolvedPath(resolution.resolvedPath);
+  if (!executableName || !resolvedPath) {
+    return false;
+  }
+  return Boolean(params.skillBinTrust.get(executableName)?.has(resolvedPath));
+}
 
 function evaluateSegments(
   segments: ExecCommandSegment[],
@@ -114,7 +180,7 @@ function evaluateSegments(
     cwd?: string;
     platform?: string | null;
     trustedSafeBinDirs?: ReadonlySet<string>;
-    skillBins?: Set<string>;
+    skillBins?: readonly SkillBinTrustEntry[];
     autoAllowSkills?: boolean;
   },
 ): {
@@ -123,7 +189,8 @@ function evaluateSegments(
   segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
 } {
   const matches: ExecAllowlistEntry[] = [];
-  const allowSkills = params.autoAllowSkills === true && (params.skillBins?.size ?? 0) > 0;
+  const skillBinTrust = buildSkillBinTrustIndex(params.skillBins);
+  const allowSkills = params.autoAllowSkills === true && skillBinTrust.size > 0;
   const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
 
   const satisfied = segments.every((segment) => {
@@ -152,19 +219,11 @@ function evaluateSegments(
       platform: params.platform,
       trustedSafeBinDirs: params.trustedSafeBinDirs,
     });
-    const rawExecutable = segment.resolution?.rawExecutable?.trim() ?? "";
-    const executableName = segment.resolution?.executableName;
-    const usesExplicitPath = isPathScopedExecutableToken(rawExecutable);
-    let skillAllow = false;
-    if (
-      allowSkills &&
-      segment.resolution?.resolvedPath &&
-      rawExecutable.length > 0 &&
-      !usesExplicitPath &&
-      executableName
-    ) {
-      skillAllow = Boolean(params.skillBins?.has(executableName));
-    }
+    const skillAllow = isSkillAutoAllowedSegment({
+      segment,
+      allowSkills,
+      skillBinTrust,
+    });
     const by: ExecSegmentSatisfiedBy = match
       ? "allowlist"
       : safe
@@ -194,7 +253,7 @@ export function evaluateExecAllowlist(params: {
   cwd?: string;
   platform?: string | null;
   trustedSafeBinDirs?: ReadonlySet<string>;
-  skillBins?: Set<string>;
+  skillBins?: readonly SkillBinTrustEntry[];
   autoAllowSkills?: boolean;
 }): ExecAllowlistEvaluation {
   const allowlistMatches: ExecAllowlistEntry[] = [];
@@ -393,7 +452,7 @@ export function evaluateShellAllowlist(params: {
   cwd?: string;
   env?: NodeJS.ProcessEnv;
   trustedSafeBinDirs?: ReadonlySet<string>;
-  skillBins?: Set<string>;
+  skillBins?: readonly SkillBinTrustEntry[];
   autoAllowSkills?: boolean;
   platform?: string | null;
 }): ExecAllowlistAnalysis {

src/infra/exec-approvals.test.ts

@@ -621,7 +621,7 @@ describe("exec approvals allowlist evaluation", () => {
       analysis,
       allowlist: [],
       safeBins: new Set(),
-      skillBins: new Set(["skill-bin"]),
+      skillBins: [{ name: "skill-bin", resolvedPath: "/opt/skills/skill-bin" }],
       autoAllowSkills: true,
       cwd: "/tmp",
     });
@@ -647,7 +647,7 @@ describe("exec approvals allowlist evaluation", () => {
       analysis,
       allowlist: [],
       safeBins: new Set(),
-      skillBins: new Set(["skill-bin"]),
+      skillBins: [{ name: "skill-bin", resolvedPath: "/tmp/skill-bin" }],
       autoAllowSkills: true,
       cwd: "/tmp",
     });
@@ -673,7 +673,7 @@ describe("exec approvals allowlist evaluation", () => {
       analysis,
       allowlist: [],
       safeBins: new Set(),
-      skillBins: new Set(["skill-bin"]),
+      skillBins: [{ name: "skill-bin", resolvedPath: "/opt/skills/skill-bin" }],
       autoAllowSkills: true,
       cwd: "/tmp",
     });