1. src/plugins/install-security-scan.runtime.ts, line 34-39 (link)

   Hook is silently skipped when the built-in scanner finds critical findings

   When scanSummary.critical > 0, both scanBundleInstallSourceRuntime and scanPackageInstallSourceRuntime execute return; before reaching the plugin hook block. As a result, the before_skill_install hook is never invoked for the most security-sensitive case — one where the built-in scanner already identified dangerous patterns.

   This is inconsistent with the skills-install.ts path where collectSkillInstallScanWarnings always returns (never short-circuits), so the hook there runs regardless of how many critical findings the built-in scanner produced.

   Consequences:

   • External scanners cannot observe or act on critical findings in the bundle/package paths (e.g. forwarding results to a SIEM).
   • The builtinFindings is populated before the early return, so the data is ready — the hook block just needs to be moved to run even on the early-return path.

   The same early-return issue exists in scanPackageInstallSourceRuntime at the critical > 0 check (lines 118–123).

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/plugins/install-security-scan.runtime.ts
   Line: 34-39
   
   Comment:
   **Hook is silently skipped when the built-in scanner finds critical findings**
   
   When `scanSummary.critical > 0`, both `scanBundleInstallSourceRuntime` and `scanPackageInstallSourceRuntime` execute `return;` before reaching the plugin hook block. As a result, the `before_skill_install` hook is never invoked for the most security-sensitive case — one where the built-in scanner already identified dangerous patterns.
   
   This is inconsistent with the `skills-install.ts` path where `collectSkillInstallScanWarnings` always returns (never short-circuits), so the hook there runs regardless of how many critical findings the built-in scanner produced.
   
   Consequences:
   - External scanners cannot observe or act on critical findings in the bundle/package paths (e.g. forwarding results to a SIEM).
   - The `builtinFindings` is populated before the early return, so the data is ready — the hook block just needs to be moved to run even on the early-return path.
   
   The same early-return issue exists in `scanPackageInstallSourceRuntime` at the `critical > 0` check (lines 118–123).
   
   How can I resolve this? If you propose a fix, please make it concise.

1. 🟠 Fail-open install policy enforcement when before_install hook fails (skill installs)

Description

The before_install hook is intended to let enterprise policy/scanner plugins block risky installs, but errors from the hook execution are silently swallowed and treated as non-fatal.

• before_install is executed before continuing with installation.
• Any exception thrown during payload creation or hook execution is caught and ignored.
• Because the global hook runner is also configured with catchErrors: true, plugin hook failures will be logged at most, and the install proceeds.

This creates a fail-open policy path: an attacker can attempt to induce failures in the policy plugin (e.g., by triggering edge cases, resource exhaustion, or crashes) to bypass blocking decisions.

Vulnerable code:

try {
  const hookResult = await hookRunner.runBeforeInstall(event, ctx);
  if (hookResult?.block) {
    return { ok: false, message: hookResult.blockReason || "Installation blocked by plugin hook", ... };
  }
  ​// ... merge findings ...
} catch {
  ​// Hook errors are non-fatal — built-in scanner results still apply.
}

Recommendation

Make install policy enforcement fail-closed when configured for enforcement.

Options:

1. Add an explicit enforcement mode (e.g., config.plugins.installPolicy.failClosed = true) and block on hook errors when enabled:

const enforcement = config.plugins?.installPolicy?.failClosed === true;
try {
  const hookResult = await hookRunner.runBeforeInstall(event, ctx);
  if (hookResult?.block) return blocked(hookResult.blockReason);
} catch (err) {
  if (enforcement) {
    return blocked(`before_install hook failed: ${String(err)}`);
  }
  logger.warn?.(`before_install hook failed (non-fatal): ${String(err)}`);
}

2. Consider running policy/scanner hooks out-of-process with timeouts so untrusted inputs cannot trivially crash or hang the enforcement mechanism.

3. If before_install is used for compliance, document that hook failures are treated as deny when enforcement is enabled.

2. 🟠 Fail-open install policy enforcement when before_install hook fails (plugin installs)

Description

The plugin install security scan invokes the before_install hook to let external policy/scanner plugins block installs, but all hook errors are swallowed and treated as non-fatal.

• runBeforeInstallHook() wraps hook invocation in a try { ... } catch {} that discards any error.
• The global hook runner is initialized with catchErrors: true, so handler exceptions are suppressed and converted into logs.
• The install continues even if the policy plugin is down, throws, or cannot complete.

This enables a fail-open policy bypass: an attacker can attempt to trigger failures (e.g., via crafted install targets, resource exhaustion, or crashing the policy plugin) to avoid blocking.

Vulnerable code:

try {
  const hookResult = await hookRunner.runBeforeInstall(event, ctx);
  if (hookResult?.block) { ... return { blocked: { reason } }; }
  ​// ...
} catch {
  ​// Hook errors are non-fatal.
}
return undefined;

Recommendation

Introduce an explicit enforcement mode for install policy hooks and fail closed when enabled.

Example pattern:

const enforcement = config.plugins?.installPolicy?.failClosed === true;
try {
  const hookResult = await hookRunner.runBeforeInstall(event, ctx);
  if (hookResult?.block) return { blocked: { reason: hookResult.blockReason ?? "blocked" } };
} catch (err) {
  if (enforcement) {
    return { blocked: { reason: `before_install hook failed: ${String(err)}` } };
  }
  params.logger.warn?.(`before_install hook failed (non-fatal): ${String(err)}`);
}

Also consider:

• timeouts around hook execution to prevent hangs
• isolating policy/scanner plugins (separate process) if they must be relied upon for security/compliance

3. 🟡 Information disclosure to all plugins via before_install hook payload (absolute paths and raw specifiers/URLs)

Description

The new before_install plugin hook exposes potentially sensitive local and user-provided data to any installed plugin that registers the hook, without redaction or an explicit permission/allowlist.

Exposed fields include:

• event.sourcePath: absolute filesystem path to the scanned install source (plugin/skill)
• event.request.requestedSpecifier: raw user-provided specifier (can be a path or URL and may include embedded credentials/tokens)
• event.skill.installSpec.* fields such as url, archive, targetDir (may contain signed URLs, internal paths, or other sensitive values)
• event.builtinScan.error: String(err) may include stack traces and local paths

Because hooks run in third-party plugin code, a malicious/compromised plugin can passively collect and exfiltrate this information (install locations, user home/workspace paths, internal URLs/tokens) whenever any install occurs.

Vulnerable code (payload construction and dispatch):

const { event, ctx } = createBeforeInstallHookPayload({
  sourcePath: params.sourcePath,
  request: {
    kind: params.requestKind,
    mode: params.requestMode,
    ...(params.requestedSpecifier ? { requestedSpecifier: params.requestedSpecifier } : {}),
  },
  builtinScan: params.builtinScan,
});
const hookResult = await hookRunner.runBeforeInstall(event, ctx);

Recommendation

Treat before_install payload as sensitive and minimize by default.

Recommended fixes (choose one or combine):

1. Redact/omit sensitive fields by default and only include them when explicitly enabled:

   • send sourcePath as a relative path or basename, or replace with a stable opaque identifier
   • drop requestedSpecifier entirely or sanitize it (remove userinfo from URLs; avoid including full local paths)
   • redact installSpec.url/archive/targetDir or only include non-sensitive derived metadata (host, scheme, etc.)
   • do not pass raw error strings; pass a stable error code/category

2. Permission/allowlist model: only trusted/built-in plugins (or plugins granted an explicit capability such as install_policy:read_sensitive) may receive the full payload.

Example: sanitize requestedSpecifier and avoid absolute paths:

function sanitizeSpecifier(spec?: string) {
  if (!spec) return undefined;
  try {
    const u = new URL(spec);
    ​// drop credentials/token-like userinfo
    u.username = "";
    u.password = "";
    return u.toString();
  } catch {
    ​// for local paths, keep only basename
    return path.basename(spec);
  }
}

const { event, ctx } = createBeforeInstallHookPayload({
  ...,
  sourcePath: path.basename(params.sourcePath),
  request: {
    kind: params.requestKind,
    mode: params.requestMode,
    requestedSpecifier: sanitizeSpecifier(params.requestedSpecifier),
  },
  builtinScan: {
    ...params.builtinScan,
    error: undefined,
  },
});

4. 🟡 Terminal/log injection via unsanitized before_install hook output (blockReason/findings) in skill install flow

Description

Untrusted strings returned by before_install plugin hooks are incorporated into user-visible messages without any escaping/sanitization.

• hookResult.blockReason is returned directly as the install failure message.
• hookResult.findings[].message/file/line are interpolated directly into warnings strings.
• If a hook returns strings containing newlines (\n), carriage returns (\r), tabs, or ANSI escape sequences, downstream CLI/log renderers may:
  • spoof or overwrite terminal output,
  • forge multi-line log entries,
  • hide warnings or make a blocked install look successful.

Vulnerable code:

message: hookResult.blockReason || "Installation blocked by plugin hook",
...
warnings.push(`Plugin scanner: ${finding.message} (${finding.file}:${finding.line})`);

Recommendation

Sanitize/escape any plugin-provided text before returning it to callers or printing it.

This repo already contains sanitizeTerminalText() (strips ANSI and escapes control characters). Use it for blockReason, finding.message, and finding.file (and any other untrusted fields).

Example fix:

import { sanitizeTerminalText } from "../terminal/safe-text.js";

const safeReason = sanitizeTerminalText(hookResult.blockReason ?? "Installation blocked by plugin hook");
...
message: safeReason,
...
const safeMsg = sanitizeTerminalText(finding.message);
const safeFile = sanitizeTerminalText(finding.file);
warnings.push(`Plugin scanner: ${safeMsg} (${safeFile}:${finding.line})`);

Also consider enforcing a single-line constraint (or JSON-encoding) for log output destined for terminals/structured logs.

5. 🟡 Plugin file install security scan can be bypassed with oversized JS/TS file (no scan performed)

Description

installPluginFromFile() now invokes scanFileInstallSourceRuntime(), which scans a single file by calling scanDirectoryWithSummary() with includeFiles=[filePath].

However, the underlying scanner silently skips files larger than maxFileBytes (default 1 MiB) and reports them as scanned: false with no findings. This results in scannedFiles remaining 0 and the install proceeding without any warning/critical findings, allowing a malicious plugin author to evade the built-in install-time scan by shipping a large .js/.ts plugin file.

Vulnerable behavior:

• input: attacker-controlled plugin file path during installPluginFromFile()
• scanner decision: scanFileWithCache() skips when st.size > maxFileBytes
• effect: scanDirectoryWithSummary() returns scannedFiles: 0 and no findings; scanFileInstallSourceRuntime() returns status: ok and does not warn/block

Vulnerable code:

if (st.size > maxFileBytes) {
  ...
  return { scanned: false, findings: [] };
}

Recommendation

Treat oversized files as a security-relevant condition during install scanning, not a silent skip.

Options:

1. Fail closed / block install for plugin installs when the target file exceeds the scan limit.
2. Emit a prominent warning and mark scan status as error, so policy hooks can block.
3. Scan a capped prefix (e.g., first N bytes) and still warn that the file exceeded the limit.

Example (mark as error and surface in summary):

if (st.size > maxFileBytes) {
  return {
    scanned: true,
    findings: [{
      ruleId: "file_too_large_to_scan",
      severity: "warn",
      file: filePath,
      line: 1,
      message: `File exceeds max scan size (${maxFileBytes} bytes); install-time scan incomplete`,
      evidence: "",
    }],
  };
}

Additionally, in scanFileInstallSourceRuntime(), consider blocking or warning when builtinScan.scannedFiles === 0 for a supposedly scannable plugin file.

6. 🟡 Terminal/log injection via unsanitized before_install hook output (blockReason/findings) in plugin install security scan runtime

Description

Untrusted strings returned by before_install plugin hooks are written directly to logs/terminal output without escaping.

• hookResult.blockReason is interpolated into a logger.warn message.
• hookResult.findings[].message/file/line are interpolated into logger.warn messages.
• A malicious or compromised hook can include newlines/CRLF or ANSI escape sequences, enabling log forging and terminal UI spoofing (e.g., hiding the real warning or injecting fake status lines).

Vulnerable code:

params.logger.warn?.(`WARNING: ${params.installLabel} blocked by plugin hook: ${reason}`);
...
params.logger.warn?.(`Plugin scanner: ${finding.message} (${finding.file}:${finding.line})`);

Recommendation

Escape/sanitize hook-provided fields before logging.

Use the existing terminal-safe sanitizer to strip ANSI escape codes and encode control characters:

import { sanitizeTerminalText } from "../terminal/safe-text.js";

const safeReason = sanitizeTerminalText(reason);
params.logger.warn?.(
  `WARNING: ${sanitizeTerminalText(params.installLabel)} blocked by plugin hook: ${safeReason}`,
);

const safeMsg = sanitizeTerminalText(finding.message);
const safeFile = sanitizeTerminalText(finding.file);
params.logger.warn?.(`Plugin scanner: ${safeMsg} (${safeFile}:${finding.line})`);

If logs are consumed by UIs (markdown/HTML), apply the appropriate context-sensitive escaping there as well.