feat(skills): reject html comments and links

Author: a-mpch

src/commands/skill.ts

@@ -9,7 +9,11 @@ import { join } from "node:path";
 import { Command } from "commander";
 import { resolveShakaHome } from "../domain/config";
 import { loadManifest } from "../domain/skills-manifest";
-import { type ScanResult, installSkill } from "../services/skill-install-service";
+import {
+  InstallCancelledError,
+  type SecurityReport,
+  installSkill,
+} from "../services/skill-install-service";
 import { removeSkill } from "../services/skill-remove-service";
 import { getProviderByName } from "../services/skill-source";
 import { type UpdateResult, updateAllSkills, updateSkill } from "../services/skill-update-service";
@@ -30,8 +34,7 @@ export function createSkillCommand(): Command {
     .command("install")
     .description("Install a skill (auto-detects source)")
     .argument("<source>", "Skill source (user/repo, URL, or clawdhub slug)")
-    .option("--force", "Skip security scan prompt", false)
-    .option("--safe-only", "Abort if non-text files found", false)
+    .option("--yolo", "Skip security checks and install without confirmation", false)
     .option("--github", "Force GitHub provider")
     .option("--clawdhub", "Force Clawdhub provider")
     .action(handleInstall);
@@ -55,38 +58,54 @@ export function createSkillCommand(): Command {
 
 async function handleInstall(
   source: string,
-  opts: { force: boolean; safeOnly: boolean; github?: boolean; clawdhub?: boolean },
+  opts: { yolo: boolean; github?: boolean; clawdhub?: boolean },
 ): Promise<void> {
   const shakaHome = getShakaHome();
 
   // Resolve provider override from flags
   const providerOverride = resolveProviderFlag(opts);
   if (providerOverride && !providerOverride.ok) {
-    console.error(`✗ ${providerOverride.error.message}`);
+    console.error(`\u2717 ${providerOverride.error.message}`);
     process.exit(1);
   }
 
   console.log(`Installing skill from ${source}...`);
 
   const result = await installSkill(shakaHome, source, {
-    force: opts.force,
-    safeOnly: opts.safeOnly,
+    yolo: opts.yolo,
     provider: providerOverride?.ok ? providerOverride.value : undefined,
-    confirm: async (scan) => {
-      console.log(formatScanWarning(scan));
-      process.stdout.write("\nProceed with installation? [y/N] ");
+    onSecurityCheck: async (report, skillName) => {
+      console.log(formatSecurityReport(report));
+
+      if (!report.allPassed) {
+        const failed = report.checks.filter((c) => !c.passed);
+        for (const check of failed) {
+          console.log(`\n\u{1F6A8}  ${check.failureMessage}`);
+        }
+        console.log(
+          `Run \`shaka skill install ${source} --yolo\` to skip checks and install anyway`,
+        );
+        return false;
+      }
+
+      console.log(
+        "\n\u2139\uFE0F  You should still review the skill and/or get them from trusted sources.",
+      );
+      process.stdout.write(`Install skill "${skillName}"? [Y/n] `);
       const answer = await readLine();
-      return answer.toLowerCase() === "y";
+      return answer === "" || answer.toLowerCase() === "y";
     },
   });
 
   if (!result.ok) {
-    console.error(`\n✗ ${result.error.message}`);
+    if (!(result.error instanceof InstallCancelledError)) {
+      console.error(`\n\u2717 ${result.error.message}`);
+    }
     process.exit(1);
   }
 
   const ver = result.value.skill.version.slice(0, 7);
-  console.log(`\n✓ Installed skill "${result.value.name}" (${ver})`);
+  console.log(`\n\u2713 Installed skill "${result.value.name}" (${ver})`);
 }
 
 async function handleRemove(name: string): Promise<void> {
@@ -177,15 +196,12 @@ function printUpdateResult(r: UpdateResult): void {
   }
 }
 
-function formatScanWarning(scan: ScanResult): string {
-  const lines = ["\n⚠  This skill contains non-text files:\n"];
-  for (const file of scan.executable) {
-    lines.push(`  executable: ${file}`);
-  }
-  for (const file of scan.unknown) {
-    lines.push(`  unknown:    ${file}`);
+function formatSecurityReport(report: SecurityReport): string {
+  const lines: string[] = [];
+  for (const check of report.checks) {
+    const status = check.passed ? "\u2705" : "\u274C";
+    lines.push(`${check.emoji} ${check.label} ${status}`);
   }
-  lines.push("\nThese files could execute code on your machine.");
   return lines.join("\n");
 }
 

src/services/skill-install-service.ts

@@ -2,7 +2,7 @@
  * Skill install service.
  *
  * Delegates fetching to a SkillSourceProvider, then runs the common pipeline:
- * validate → security scan → collision check → deploy → persist.
+ * validate → security checks → collision check → deploy → persist.
  */
 
 import { readdir } from "node:fs/promises";
@@ -26,13 +26,32 @@ export interface ScanResult {
   unknown: string[];
 }
 
+export interface SecurityCheckEntry {
+  emoji: string;
+  label: string;
+  passed: boolean;
+  details: string[];
+  failureMessage: string;
+}
+
+export interface SecurityReport {
+  checks: SecurityCheckEntry[];
+  allPassed: boolean;
+}
+
+/** Sentinel error signalling the install was cancelled via callback. */
+export class InstallCancelledError extends Error {
+  override name = "InstallCancelledError";
+  constructor(message = "Installation cancelled.") {
+    super(message);
+  }
+}
+
 export interface InstallOptions {
-  /** Skip security scan prompt and install regardless. */
-  force?: boolean;
-  /** Abort if any non-text files are found (for scripted usage). */
-  safeOnly?: boolean;
-  /** Callback for confirming installation when executable files are found. */
-  confirm?: (scan: ScanResult) => Promise<boolean>;
+  /** Skip all security checks and confirmation prompt. */
+  yolo?: boolean;
+  /** Called with security check results and skill name. Return true to proceed. */
+  onSecurityCheck?: (report: SecurityReport, skillName: string) => Promise<boolean>;
   /** Callback to let user choose from multiple skills (marketplace repos). */
   selectSkill?: (skills: { name: string; description?: string }[]) => Promise<string | null>;
   /** Override auto-detected provider (e.g., from --github or --clawdhub flag). */
@@ -81,9 +100,15 @@ export async function installSkill(
     const validation = await validateSkillStructure(fetchResult.value.skillDir);
     if (!validation.ok) return validation;
 
-    // Security scan
-    const scanResult = await enforceSecurityScan(fetchResult.value.skillDir, options);
-    if (!scanResult.ok) return scanResult;
+    // Security checks (unless --yolo)
+    if (!options.yolo) {
+      const securityResult = await enforceSecurityChecks(
+        fetchResult.value.skillDir,
+        validation.value.name,
+        options,
+      );
+      if (!securityResult.ok) return securityResult;
+    }
 
     // Check for name collision
     const collisionResult = await checkNameCollision(shakaHome, validation.value.name);
@@ -127,27 +152,87 @@ export async function scanForExecutableContent(skillPath: string): Promise<ScanR
   return result;
 }
 
-// --- Internal helpers ---
+// --- Security checks ---
 
-async function enforceSecurityScan(
-  skillPath: string,
-  options: InstallOptions,
-): Promise<Result<void, Error>> {
+/**
+ * Run all security checks on a skill directory.
+ */
+export async function runSecurityChecks(skillPath: string): Promise<SecurityReport> {
+  const checks = await Promise.all([
+    checkExecutables(skillPath),
+    checkSuspiciousLinks(skillPath),
+    checkHtmlComments(skillPath),
+  ]);
+  return {
+    checks,
+    allPassed: checks.every((c) => c.passed),
+  };
+}
+
+async function checkExecutables(skillPath: string): Promise<SecurityCheckEntry> {
   const scan = await scanForExecutableContent(skillPath);
-  const hasRiskyFiles = scan.executable.length > 0 || scan.unknown.length > 0;
+  const hasRisky = scan.executable.length > 0 || scan.unknown.length > 0;
+  return {
+    emoji: "\u{1F3C3}",
+    label: "No executables",
+    passed: !hasRisky,
+    details: [...scan.executable, ...scan.unknown],
+    failureMessage: "Skill contains executable files, make sure to review it properly.",
+  };
+}
 
-  if (!hasRiskyFiles) return ok(undefined);
+async function checkSuspiciousLinks(skillPath: string): Promise<SecurityCheckEntry> {
+  const files = await collectFiles(skillPath);
+  const mdFiles = files.filter((f) => f.toLowerCase().endsWith(".md"));
+  const flagged: string[] = [];
+  const pattern = /\b(curl|wget)\b/;
+  for (const file of mdFiles) {
+    const content = await Bun.file(join(skillPath, file)).text();
+    if (pattern.test(content)) {
+      flagged.push(file);
+    }
+  }
+  return {
+    emoji: "\u{1F517}",
+    label: "No links",
+    passed: flagged.length === 0,
+    details: flagged,
+    failureMessage: "Skill contains curl/wget commands, make sure to review it properly.",
+  };
+}
 
-  if (options.safeOnly) {
-    const flagged = [...scan.executable, ...scan.unknown].join(", ");
-    return err(new Error(`Skill contains non-text files (${flagged}). Aborting (--safe-only).`));
+async function checkHtmlComments(skillPath: string): Promise<SecurityCheckEntry> {
+  const files = await collectFiles(skillPath);
+  const mdFiles = files.filter((f) => f.toLowerCase().endsWith(".md"));
+  const flagged: string[] = [];
+  for (const file of mdFiles) {
+    const content = await Bun.file(join(skillPath, file)).text();
+    if (content.includes("<!--")) {
+      flagged.push(file);
+    }
   }
+  return {
+    emoji: "\u{1F977}",
+    label: "No html comments",
+    passed: flagged.length === 0,
+    details: flagged,
+    failureMessage: "Skill has HTML comments in markdown, make sure to review it properly.",
+  };
+}
 
-  if (options.force) return ok(undefined);
+// --- Internal helpers ---
 
-  const confirmed = options.confirm ? await options.confirm(scan) : false;
-  if (!confirmed) {
-    return err(new Error("Installation cancelled by user."));
+async function enforceSecurityChecks(
+  skillPath: string,
+  skillName: string,
+  options: InstallOptions,
+): Promise<Result<void, Error>> {
+  const report = await runSecurityChecks(skillPath);
+  if (options.onSecurityCheck) {
+    const proceed = await options.onSecurityCheck(report, skillName);
+    if (!proceed) return err(new InstallCancelledError());
+  } else if (!report.allPassed) {
+    return err(new Error("Security checks failed."));
   }
   return ok(undefined);
 }