fix(security): address remaining AVIO local-input findings

.github/workflows/ci.yml

@@ -102,6 +102,8 @@ jobs:
         with:
           node-version: ${{ env.NODE_VERSION }}
       - run: node --test tests/opencode/skillspector_scan_lib.test.ts
+      - name: Run Pi extension security tests
+        run: node --test tests/unit/test_pi_extension.mjs
 
   docker-smoke:
     needs: changes

extensions/skillspector.ts

@@ -1,8 +1,9 @@
 import type { ExtensionAPI } from "@earendil-works/pi-coding-agent";
 import { StringEnum } from "@earendil-works/pi-ai";
 import { Type, type Static } from "typebox";
-import { existsSync } from "node:fs";
-import { dirname, isAbsolute, resolve } from "node:path";
+import { chmodSync, constants, copyFileSync, existsSync, lstatSync, mkdtempSync, realpathSync, renameSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { basename, dirname, isAbsolute, join, relative, resolve, sep } from "node:path";
 import { fileURLToPath } from "node:url";
 
 const scanSchema = Type.Object({
@@ -12,7 +13,7 @@ const scanSchema = Type.Object({
       description: "SkillSpector output format. Defaults to terminal.",
     }),
   ),
-  output: Type.Optional(Type.String({ description: "Optional report output path." })),
+  output: Type.Optional(Type.String({ description: "Optional report output path within the current workspace." })),
   noLlm: Type.Optional(Type.Boolean({ description: "Skip LLM analysis. Defaults to true." })),
   provider: Type.Optional(
     StringEnum(["openai", "anthropic", "anthropic_proxy", "nv_build", "nv_inference"] as const, {
@@ -57,20 +58,57 @@ function packageRoot(): string {
 }
 
 function findSkillSpectorBin(): string {
-  if (process.env.SKILLSPECTOR_BIN) return process.env.SKILLSPECTOR_BIN;
-  const localBin = resolve(packageRoot(), ".venv/bin/skillspector");
-  if (existsSync(localBin)) return localBin;
-  return "skillspector";
+  const bundledBin = process.platform === "win32" ? ".venv/Scripts/skillspector.exe" : ".venv/bin/skillspector";
+  const bin = process.env.SKILLSPECTOR_BIN ?? resolve(packageRoot(), bundledBin);
+  if (!isAbsolute(bin) || !existsSync(bin)) {
+    throw new Error("Install SkillSpector in the extension's .venv or set SKILLSPECTOR_BIN to an absolute executable path.");
+  }
+  return bin;
 }
 
-function buildScanArgs(params: SkillSpectorScanParams, cwd: string): string[] {
+function isWithin(root: string, path: string): boolean {
+  const rel = relative(root, path);
+  return rel !== ".." && !rel.startsWith(`..${sep}`) && !isAbsolute(rel);
+}
+
+function reportOutputPath(cwd: string, value?: string): string | undefined {
+  if (!value) return undefined;
+  const output = resolve(cwd, value);
+  if (output === resolve(cwd) || !isWithin(resolve(cwd), output)) {
+    throw new Error("Report output must be a file within the current workspace.");
+  }
+  const parent = realpathSync(dirname(output));
+  if (!isWithin(realpathSync(cwd), parent)) {
+    throw new Error("Report output must be a file within the current workspace.");
+  }
+  const destination = join(parent, basename(output));
+  const existing = lstatSync(destination, { throwIfNoEntry: false });
+  if (existing && !existing.isFile()) {
+    throw new Error("Report output must be a regular file, not a symlink or directory.");
+  }
+  return destination;
+}
+
+function publishReport(source: string, destination: string): void {
+  const staging = mkdtempSync(join(dirname(destination), ".skillspector-report-"));
+  try {
+    const report = join(staging, "report");
+    copyFileSync(source, report, constants.COPYFILE_EXCL);
+    chmodSync(report, 0o600);
+    // Replacing the directory entry never follows an existing hard link.
+    renameSync(report, destination);
+  } finally {
+    rmSync(staging, { recursive: true, force: true });
+  }
+}
+
+function buildScanArgs(params: SkillSpectorScanParams, cwd: string, output?: string): string[] {
   const args = ["scan", resolveMaybePath(cwd, params.target) ?? params.target];
   args.push("--format", params.format ?? "terminal");
 
   const noLlm = params.noLlm ?? true;
   if (noLlm) args.push("--no-llm");
 
-  const output = resolveMaybePath(cwd, params.output);
   if (output) args.push("--output", output);
 
   const yaraRulesDir = resolveMaybePath(cwd, params.yaraRulesDir);
@@ -93,50 +131,65 @@ export default function (pi: ExtensionAPI) {
     parameters: scanSchema,
     async execute(_toolCallId, params, signal, onUpdate, ctx) {
       const bin = findSkillSpectorBin();
-      const args = buildScanArgs(params, ctx.cwd);
-      const env: Record<string, string> = {};
-
-      if (params.provider) env.SKILLSPECTOR_PROVIDER = params.provider;
-      if (params.model) env.SKILLSPECTOR_MODEL = params.model;
-
-      onUpdate?.({ content: [{ type: "text", text: `Running ${bin} ${args.slice(0, 2).join(" ")} ...` }] });
-
-      const result = await pi.exec(bin, args, {
-        cwd: ctx.cwd,
-        env,
-        signal,
-        timeout: 120000,
-      });
-
-      const stdout = truncateText(redactSecrets(result.stdout ?? ""));
-      const stderr = truncateText(redactSecrets(result.stderr ?? ""), 6000);
-
-      if (result.code !== 0) {
-        throw new Error(`SkillSpector failed with exit code ${result.code}.\n${stderr.text}`);
+      const outputPath = reportOutputPath(ctx.cwd, params.output);
+      const reportDir = outputPath ? mkdtempSync(join(tmpdir(), "skillspector-report-")) : undefined;
+      const reportPath = reportDir ? join(reportDir, "report") : undefined;
+      try {
+        const args = buildScanArgs(params, ctx.cwd, reportPath);
+        const env: Record<string, string> = {};
+
+        if (params.provider) env.SKILLSPECTOR_PROVIDER = params.provider;
+        if (params.model) env.SKILLSPECTOR_MODEL = params.model;
+
+        onUpdate?.({ content: [{ type: "text", text: `Running ${bin} ${args.slice(0, 2).join(" ")} ...` }] });
+
+        const result = await pi.exec(bin, args, {
+          cwd: ctx.cwd,
+          env,
+          signal,
+          timeout: 120000,
+        });
+
+        const stdout = truncateText(redactSecrets(result.stdout ?? ""));
+        const stderr = truncateText(redactSecrets(result.stderr ?? ""), 6000);
+
+        // Exit 2 can follow a diagnostic report; failures before reporting produce no file.
+        const failureReport = result.code === 2 && reportPath
+          ? lstatSync(reportPath, { throwIfNoEntry: false }) : undefined;
+        const hasFailureReport = failureReport?.isFile() && failureReport.size > 0;
+        if ((result.code === 0 || result.code === 1 || hasFailureReport) && outputPath && reportPath) {
+          if (reportOutputPath(ctx.cwd, params.output) !== outputPath) {
+            throw new Error("Report output directory changed during the scan.");
+          }
+          publishReport(reportPath, outputPath);
+        }
+        if (result.code !== 0) {
+          throw new Error(`SkillSpector failed with exit code ${result.code}.\n${stderr.text}`);
+        }
+        const lines = [
+          `SkillSpector scan complete: ${params.target}`,
+          `format: ${params.format ?? "terminal"}`,
+          `noLlm: ${params.noLlm ?? true}`,
+        ];
+        if (outputPath) lines.push(`output: ${outputPath}`);
+        if (stdout.truncated) lines.push("stdout truncated: true");
+        if (stderr.text.trim()) lines.push(`stderr:\n${stderr.text}`);
+        if (stdout.text.trim()) lines.push(`stdout:\n${stdout.text}`);
+
+        return {
+          content: [{ type: "text", text: lines.join("\n") }],
+          details: {
+            code: result.code,
+            command: bin,
+            args,
+            outputPath,
+            stdoutTruncated: stdout.truncated,
+            stderrTruncated: stderr.truncated,
+          },
+        };
+      } finally {
+        if (reportDir) rmSync(reportDir, { recursive: true, force: true });
       }
-
-      const outputPath = resolveMaybePath(ctx.cwd, params.output);
-      const lines = [
-        `SkillSpector scan complete: ${params.target}`,
-        `format: ${params.format ?? "terminal"}`,
-        `noLlm: ${params.noLlm ?? true}`,
-      ];
-      if (outputPath) lines.push(`output: ${outputPath}`);
-      if (stdout.truncated) lines.push("stdout truncated: true");
-      if (stderr.text.trim()) lines.push(`stderr:\n${stderr.text}`);
-      if (stdout.text.trim()) lines.push(`stdout:\n${stdout.text}`);
-
-      return {
-        content: [{ type: "text", text: lines.join("\n") }],
-        details: {
-          code: result.code,
-          command: bin,
-          args,
-          outputPath,
-          stdoutTruncated: stdout.truncated,
-          stderrTruncated: stderr.truncated,
-        },
-      };
     },
   });
 }

src/skillspector/mcp_registry.py

@@ -7,17 +7,22 @@
 
 import hashlib
 import json
+import os
 import re
 from dataclasses import asdict, dataclass
 from datetime import UTC, datetime
 from itertools import chain
 from pathlib import Path
+from stat import S_ISREG
 from typing import Any, TypedDict
 
 import httpx
 
 REGISTRY_URL = "https://registry.modelcontextprotocol.io/v0/servers"
 OFFICIAL_META_KEY = "io.modelcontextprotocol.registry/official"
+MAX_REGISTRY_BYTES = 16 * 1024 * 1024
+MAX_REGISTRY_DEPTH = 64
+MAX_REGISTRY_RECORDS = 10_000
 FILE_SHA256_RE = re.compile(r"^[a-f0-9]{64}$")
 MUTABLE_VERSION_TAGS = frozenset(
     {
@@ -224,6 +229,24 @@ def normalize_server(
 def normalize_payload(payload: dict[str, Any], *, source: str) -> list[RegistryServerSnapshot]:
     if not isinstance(payload, dict) or not isinstance(payload.get("servers"), list):
         raise ValueError(f"MCP Registry payload from {source} must contain a servers list")
+    # Check the full expansion before constructing any snapshots or findings.
+    records = len(payload["servers"])
+    if records <= MAX_REGISTRY_RECORDS:
+        for entry in payload["servers"]:
+            record = entry.get("server") if isinstance(entry, dict) else None
+            if isinstance(record, dict):
+                records += sum(
+                    len(record[field])
+                    for field in ("packages", "remotes")
+                    if isinstance(record.get(field), list)
+                )
+            if records > MAX_REGISTRY_RECORDS:
+                break
+    if records > MAX_REGISTRY_RECORDS:
+        raise ValueError(
+            f"MCP Registry payload from {source} exceeds {MAX_REGISTRY_RECORDS} records "
+            "(servers, packages, and remotes)"
+        )
     scanned_at = datetime.now(UTC).isoformat()
     return [
         normalize_server(entry, source=source, scanned_at=scanned_at)
@@ -344,14 +367,47 @@ def _dict_payload(payload: object, *, source: str) -> dict[str, Any]:
     return payload
 
 
+def _load_local_registry(path: Path) -> dict[str, Any]:
+    with open(
+        path,
+        "rb",
+        opener=lambda path, flags: os.open(path, flags | getattr(os, "O_NONBLOCK", 0)),
+    ) as source_file:
+        if not S_ISREG(os.fstat(source_file.fileno()).st_mode):
+            raise ValueError(f"Registry input must be a regular file: {path}")
+        raw = source_file.read(MAX_REGISTRY_BYTES + 1)
+    if len(raw) > MAX_REGISTRY_BYTES:
+        raise ValueError(f"input exceeds {MAX_REGISTRY_BYTES} bytes")
+    text = raw.decode("utf-8")
+    # The JSON decoder owns syntax validation; only bound nesting here, while
+    # ignoring delimiters in strings, before its recursive parser runs.
+    depth = 0
+    in_string = False
+    escaped = False
+    for char in text:
+        if in_string:
+            if escaped:
+                escaped = False
+            elif char == "\\":
+                escaped = True
+            elif char == '"':
+                in_string = False
+        elif char == '"':
+            in_string = True
+        elif char in "[{":
+            depth += 1
+            if depth > MAX_REGISTRY_DEPTH:
+                raise ValueError(f"JSON nesting exceeds {MAX_REGISTRY_DEPTH} levels")
+        elif char in "]}":
+            depth -= 1
+    return _dict_payload(json.loads(text), source=str(path))
+
+
 def _load_payload(input_path: str) -> dict[str, Any]:
     source = input_path
     try:
         if Path(input_path).is_file():
-            return _dict_payload(
-                json.loads(Path(input_path).read_text(encoding="utf-8")),
-                source=source,
-            )
+            return _load_local_registry(Path(input_path))
         if input_path.startswith(("http://", "https://")):
             if input_path != REGISTRY_URL:
                 raise ValueError(
@@ -372,7 +428,7 @@ def _load_payload(input_path: str) -> dict[str, Any]:
         # assesses the owner's latest record, not the historical tail.
         latest = [entry for entry in matches if _official_meta(entry).get("isLatest") is True]
         return {"servers": latest or matches}
-    except (OSError, json.JSONDecodeError, httpx.HTTPError, ValueError) as exc:
+    except (OSError, httpx.HTTPError, ValueError, RecursionError) as exc:
         if isinstance(exc, ValueError) and str(exc).startswith("MCP Registry source"):
             raise
         raise ValueError(f"MCP Registry source failed: {source}: {exc}") from exc

src/skillspector/providers/_agent_cli.py

@@ -79,7 +79,10 @@
 # Includes API keys, cloud creds, SSH agent, and SkillSpector's own keys.
 _SECRET_ENV_PREFIXES: tuple[str, ...] = (
     "ANTHROPIC_API_KEY",
+    "ANTHROPIC_PROXY_API_KEY",
     "OPENAI_API_KEY",
+    "SKILLSPECTOR_API_KEYS",
+    "SKILLSPECTOR_COMPAT_API_KEY",
     "NVIDIA_INFERENCE_KEY",
     "NVIDIA_INFERENCE_METADATA_KEY",
     "AWS_",