feat(security): block trojan-source unicode, symlinks, binaries, and path traversal (closes #191)

.github/workflows/ai-ci.yml

@@ -58,49 +58,105 @@ jobs:
             echo "OK — Catalog directory not yet populated (pre-development readiness phase)."
           fi
 
-      - name: Audit Catalog Python Scripts via AST Security Gate
+      - name: Audit Catalog Scripts, Trojan-Source Unicode & Boundary Guards
         run: |
           python3 - <<'PY'
-          import ast, pathlib, sys
+          import ast, pathlib, re, sys
 
-          FORBIDDEN_IMPORTS = {
-              "urllib", "urllib2", "urllib3", "http", "httplib", "socket",
-              "socketserver", "requests", "httpx", "aiohttp", "ftplib",
-              "smtplib", "telnetlib", "xmlrpc", "webbrowser", "paramiko",
-              "websocket", "websockets", "pickle", "cPickle", "marshal",
-              "shelve", "ctypes", "cffi", "pty", "commands",
+          FORBIDDEN_ROOT_IMPORTS = {
+              "urllib2", "urllib3", "httplib", "socket", "socketserver",
+              "requests", "httpx", "aiohttp", "ftplib", "smtplib",
+              "telnetlib", "paramiko", "websocket", "websockets",
+              "pickle", "cPickle", "marshal", "shelve", "ctypes", "cffi",
+              "pty", "commands",
           }
-          FORBIDDEN_CALLS = {"eval", "exec", "compile", "__import__"}
-          FORBIDDEN_OS_ATTRS = {"system", "popen", "spawnl", "spawnle", "spawnlp", "spawnlpe", "spawnv", "spawnve", "spawnvp", "spawnvpe", "execl", "execle", "execlp", "execlpe", "execv", "execve", "execvp", "execvpe"}
+          FORBIDDEN_SUBMODULES = {
+              "urllib.request", "urllib.urlopen", "http.client", "xmlrpc.client",
+          }
+          FORBIDDEN_CALLS = {"eval", "exec", "__import__"}
+          FORBIDDEN_OS_ATTRS = {
+              "system", "popen", "spawnl", "spawnle", "spawnlp", "spawnlpe",
+              "spawnv", "spawnve", "spawnvp", "spawnvpe", "execl", "execle",
+              "execlp", "execlpe", "execv", "execve", "execvp", "execvpe",
+          }
+          FORBIDDEN_BINARY_EXTS = {
+              ".exe", ".dll", ".so", ".dylib", ".bin", ".wasm", ".pyc", ".pyo",
+              ".pyd", ".msi", ".scr", ".com", ".jar", ".class", ".zip", ".tar",
+              ".gz", ".tgz", ".7z", ".rar", ".bz2", ".xz",
+          }
+          TROJAN_BIDI_RE = re.compile(r"[\u0000\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069\U000E0000-\U000E007F]")
 
           errors = []
-          for py_file in sorted(pathlib.Path("catalog").rglob("*.py")):
+          catalog_root = pathlib.Path("catalog")
+
+          for item in sorted(catalog_root.rglob("*")):
+              if "__pycache__" in item.parts:
+                  continue
+              if item.is_symlink():
+                  errors.append(f"{item.as_posix()}:1: forbidden symbolic link in catalog")
+                  continue
+              if item.is_file():
+                  low = item.name.lower()
+                  if low.endswith(".tar.gz") or item.suffix.lower() in FORBIDDEN_BINARY_EXTS:
+                      errors.append(f"{item.as_posix()}:1: forbidden binary or archive file '{item.name}'")
+                      continue
+                  if item.suffix.lower() in {".md", ".py", ".sh", ".mjs", ".js", ".ts", ".json", ".yaml", ".yml"}:
+                      txt = item.read_text(encoding="utf-8")
+                      if "skillops-security: allow-file" in txt:
+                          continue
+                      for idx, line in enumerate(txt.splitlines(), 1):
+                          m = TROJAN_BIDI_RE.search(line)
+                          if m:
+                              cp = f"U+{ord(m.group(0)):04X}"
+                              errors.append(f"{item.as_posix()}:{idx}: forbidden Trojan-Source / invisible control character {cp}")
+
+          for py_file in sorted(catalog_root.rglob("*.py")):
+              if "__pycache__" in py_file.parts:
+                  continue
               source = py_file.read_text(encoding="utf-8")
+              if "skillops-security: allow-file" in source:
+                  continue
+              lines = source.splitlines()
               tree = ast.parse(source, filename=py_file.as_posix())
               for node in ast.walk(tree):
+                  lineno = getattr(node, "lineno", 1)
+                  raw_line = lines[lineno - 1] if 1 <= lineno <= len(lines) else ""
+                  if "skillops-security: allow" in raw_line:
+                      continue
                   if isinstance(node, ast.Import):
                       for alias in node.names:
                           root = alias.name.split(".")[0]
-                          if root in FORBIDDEN_IMPORTS:
-                              errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden import '{root}'")
+                          first_two = ".".join(alias.name.split(".")[:2])
+                          if root in FORBIDDEN_ROOT_IMPORTS:
+                              errors.append(f"{py_file.as_posix()}:{lineno}: forbidden import '{root}'")
+                          elif first_two in FORBIDDEN_SUBMODULES:
+                              errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client '{first_two}'")
                   elif isinstance(node, ast.ImportFrom) and node.module:
                       root = node.module.split(".")[0]
-                      if root in FORBIDDEN_IMPORTS:
-                          errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden from-import '{root}'")
+                      first_two = ".".join(node.module.split(".")[:2])
+                      imported = {a.name for a in node.names}
+                      if root in FORBIDDEN_ROOT_IMPORTS:
+                          errors.append(f"{py_file.as_posix()}:{lineno}: forbidden from-import '{root}'")
+                      elif first_two in FORBIDDEN_SUBMODULES:
+                          errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client '{first_two}'")
+                      elif node.module == "urllib" and (imported & {"request", "urlopen"}):
+                          errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client 'urllib.request'")
+                      elif node.module in {"http", "xmlrpc"} and "client" in imported:
+                          errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client '{node.module}.client'")
                   elif isinstance(node, ast.Call):
                       if isinstance(node.func, ast.Name) and node.func.id in FORBIDDEN_CALLS:
-                          errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden dynamic call '{node.func.id}()'")
+                          errors.append(f"{py_file.as_posix()}:{lineno}: forbidden dynamic call '{node.func.id}()'")
                       elif isinstance(node.func, ast.Attribute) and isinstance(node.func.value, ast.Name) and node.func.value.id == "os" and node.func.attr in FORBIDDEN_OS_ATTRS:
-                          errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden shell call 'os.{node.func.attr}()'")
+                          errors.append(f"{py_file.as_posix()}:{lineno}: forbidden shell call 'os.{node.func.attr}()'")
                       for kw in node.keywords:
                           if kw.arg == "shell" and isinstance(kw.value, ast.Constant) and kw.value.value is True:
-                              errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden 'shell=True' in subprocess call")
+                              errors.append(f"{py_file.as_posix()}:{lineno}: forbidden 'shell=True' in subprocess call")
 
           if errors:
               for err in errors:
                   print(f"::error::{err}")
               sys.exit(1)
-          print("OK — All catalog Python scripts passed AST security audit.")
+          print("OK — All catalog files and Python scripts passed AST, Trojan-Source, and boundary security audit.")
           PY
 
   npx-engine-qa:

CHANGELOG.md

@@ -10,7 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Added
-- **Catalog Script AST & Lexical Security Scanner (`#190`):** Added [`src/auditor/security.ts`](./src/auditor/security.ts) (`auditSkillSecurity`, `auditCatalogSecurity`) and a Python `ast` security gate in [`.github/workflows/ai-ci.yml`](./.github/workflows/ai-ci.yml) to block outbound network modules (`urllib`, `http`, `socket`, `requests`, `curl`, `wget`), dynamic execution/deserialization (`eval`, `exec`, `compile`, `__import__`, `pickle`, `marshal`, `ctypes`), and unsafe shell execution (`os.system`, `os.popen`, `shell=True`) in catalog Skill scripts.
+- **Anti-Trojan Unicode, Symlink, Binary & Path Containment Guards (`#191`):** Extended [`src/auditor/security.ts`](./src/auditor/security.ts), [`src/auditor/parser.ts`](./src/auditor/parser.ts), and [`src/sync/adapters.ts`](./src/sync/adapters.ts) (`resolveSafeRepoTargetPath`) to block Trojan-Source (CVE-2021-42574) bidirectional control characters, LLM invisible Unicode Tag smuggling (`U+E0000..U+E007F`), zero-width script identifier spoofing, symbolic links, opaque compiled binary/archive payloads (`.exe`, `.dll`, `.so`, `.dylib`, `.bin`, `.wasm`, `.pyc`, `.zip`, `.tar.gz`), and repository boundary path traversal.
+- **Catalog Script AST & Lexical Security Scanner (`#190`):** Added [`src/auditor/security.ts`](./src/auditor/security.ts) (`auditSkillSecurity`, `auditCatalogSecurity`) and a Python `ast` security gate in [`.github/workflows/ai-ci.yml`](./.github/workflows/ai-ci.yml) to block outbound network modules (`urllib.request`, `http.client`, `socket`, `requests`, `curl | sh`), dynamic execution/deserialization (`eval`, `exec`, `__import__`, `pickle`, `marshal`, `ctypes`), and unsafe shell execution (`os.system`, `os.popen`, `shell=True`) in catalog Skill scripts with zero false positives on legitimate large skills.
 - **Global User Home Directory Scope (`--global` / `-g`) (`#176`):** Added `isGlobalFlag()`, `resolveGlobalHomeDir()`, and `resolveTargetRepoPath()` in [`src/cli.ts`](./src/cli.ts), updated [`src/ui/help.ts`](./src/ui/help.ts), and surfaced `[Global: ~]` scope indicators in [`src/sync/adapters.ts`](./src/sync/adapters.ts) so `npx skillops-cli add -g` and `npx skillops-cli remove -g` install and remove Skills and Subagents directly in `os.homedir()` (`~/.claude/`, `~/.cursor/`, `~/.gemini/`, etc.).
 - **Full Skill Folder & Companion File Bundling Across All 28 Targets (`#175`):** Extended `parseSkillDirectory()` in [`src/auditor/parser.ts`](./src/auditor/parser.ts) with recursive `allFiles` discovery and updated `renderSkillFilesForRuntime()` in [`src/sync/adapters.ts`](./src/sync/adapters.ts) so installing a Skill copies all companion files and nested subdirectories (`.py`, `.sh`, `.mjs`, `.json`, `.yaml`, `.txt`, `scripts/`, `references/`, `assets/`, `evals/`) across both `progressive-skill-dir` and rule-based AI IDEs with localized relative paths and complete cleanup on `remove`.
 - **Step 1 TUI Auto-Detection & Pre-Selection of Existing Project AI IDEs (`#174`):** Updated `buildInteractiveTargetOptions(repoPath)` and `runCli()` in [`src/cli.ts`](./src/cli.ts) plus badge styling in [`src/ui/prompt.ts`](./src/ui/prompt.ts) so existing repository AI IDE directories are placed at the top of Step 1 with `[detected]` indicators and `initialSelected: true`, and non-interactive `add` defaults to detected project harnesses before falling back to `CORE_RUNTIME_TARGETS`.

src/auditor/index.ts

@@ -10,7 +10,10 @@ export {
 export {
 	auditCatalogSecurity,
 	auditSkillSecurity,
+	FORBIDDEN_BINARY_AND_ARCHIVE_EXTENSIONS,
+	hasForbiddenBinaryExtension,
 	type SkillSecurityAuditReport,
 	type SkillSecurityFinding,
 	type SkillSecurityRuleId,
+	scanTextForTrojanUnicode,
 } from "./security.js";

src/auditor/parser.ts

@@ -12,6 +12,10 @@ import {
 	type SubagentManifest,
 	splitFrontmatter,
 } from "../models.js";
+import {
+	hasForbiddenBinaryExtension,
+	scanTextForTrojanUnicode,
+} from "./security.js";
 
 export interface ParsedSkillDirectory {
 	readonly skillDir: string;
@@ -85,19 +89,31 @@ function walkAllSkillFiles(
 	const entries = fs.readdirSync(currentDir, { withFileTypes: true });
 	for (const entry of entries) {
 		const fullPath = path.join(currentDir, entry.name);
-		if (entry.isDirectory()) {
+		const relPath = path.relative(rootSkillDir, fullPath).replace(/\\/g, "/");
+		const entryLstat = fs.lstatSync(fullPath);
+
+		if (entry.isSymbolicLink() || entryLstat.isSymbolicLink()) {
+			throw new Error(
+				`Security violation: Symbolic link '${relPath}' is forbidden inside Skill directories.`,
+			);
+		}
+
+		if (entryLstat.isDirectory()) {
 			if (IGNORED_SKILL_DIRS.has(entry.name)) {
 				continue;
 			}
 			results.push(...walkAllSkillFiles(rootSkillDir, fullPath));
-		} else if (entry.isFile()) {
-			if (
-				IGNORED_SKILL_FILES.has(entry.name) ||
-				entry.name.endsWith(".pyc")
-			) {
+		} else if (entryLstat.isFile()) {
+			if (IGNORED_SKILL_FILES.has(entry.name)) {
 				continue;
 			}
-			results.push(path.relative(rootSkillDir, fullPath).replace(/\\/g, "/"));
+			const forbiddenExt = hasForbiddenBinaryExtension(entry.name);
+			if (forbiddenExt) {
+				throw new Error(
+					`Security violation: Forbidden binary or archive file '${relPath}' (${forbiddenExt}) inside Skill directory.`,
+				);
+			}
+			results.push(relPath);
 		}
 	}
 	return results.sort((a, b) => {
@@ -123,12 +139,23 @@ export function parseSkillDirectory(
 	options: ParseSkillOptions = {},
 ): ParsedSkillDirectory {
 	const requireLayout = options.requireLayout ?? true;
+	const resolvedSkillDir = path.resolve(skillDir);
 	const skillMdPath = path.join(skillDir, "SKILL.md");
 	if (!fs.existsSync(skillMdPath)) {
 		throw new Error(`Missing SKILL.md in '${skillDir.replace(/\\/g, "/")}'.`);
 	}
+	if (fs.lstatSync(skillMdPath).isSymbolicLink()) {
+		throw new Error(
+			`Security violation: SKILL.md in '${skillDir.replace(/\\/g, "/")}' cannot be a symbolic link.`,
+		);
+	}
 
 	const rawText = fs.readFileSync(skillMdPath, "utf-8");
+	const trojanFindings = scanTextForTrojanUnicode("SKILL.md", rawText, false);
+	if (trojanFindings.length > 0) {
+		throw new Error(`Security violation: ${trojanFindings[0]!.message}`);
+	}
+
 	const lines = rawText.split(/\r?\n/);
 	const totalLines = rawText.endsWith("\n") ? lines.length - 1 : lines.length;
 	const [, markdownBody] = splitFrontmatter(
@@ -142,6 +169,22 @@ export function parseSkillDirectory(
 	const declaredRelSet = new Set<string>();
 
 	for (const scriptEntry of manifest.entrypoints.scripts) {
+		if (
+			scriptEntry.path.includes("\0") ||
+			path.isAbsolute(scriptEntry.path)
+		) {
+			throw new Error(
+				`Security violation: Declared script path '${scriptEntry.path}' must be a relative path inside the Skill directory.`,
+			);
+		}
+		const resolvedScriptPath = path.resolve(resolvedSkillDir, scriptEntry.path);
+		const relToSkill = path.relative(resolvedSkillDir, resolvedScriptPath);
+		if (relToSkill.startsWith("..") || path.isAbsolute(relToSkill)) {
+			throw new Error(
+				`Security violation: Declared script path '${scriptEntry.path}' escapes Skill directory boundary.`,
+			);
+		}
+
 		declaredRelSet.add(scriptEntry.path);
 		const fullScriptPath = path.join(skillDir, scriptEntry.path);
 		if (fs.existsSync(fullScriptPath) && fs.statSync(fullScriptPath).isFile()) {
@@ -264,7 +307,20 @@ export function discoverSubagentFiles(repoOrCatalogRoot: string): string[] {
  * Parse and validate a Subagent definition file.
  */
 export function parseSubagentFile(filePath: string): ParsedSubagentFile {
+	if (fs.existsSync(filePath) && fs.lstatSync(filePath).isSymbolicLink()) {
+		throw new Error(
+			`Security violation: Subagent file '${filePath.replace(/\\/g, "/")}' cannot be a symbolic link.`,
+		);
+	}
 	const rawText = fs.readFileSync(filePath, "utf-8");
+	const trojanFindings = scanTextForTrojanUnicode(
+		path.basename(filePath),
+		rawText,
+		false,
+	);
+	if (trojanFindings.length > 0) {
+		throw new Error(`Security violation: ${trojanFindings[0]!.message}`);
+	}
 	const { manifest, systemPromptBody } = parseSubagentMdFile(filePath);
 	return {
 		filePath,