T09 · Insecure Skill Coding Practices
Error
- Location
- src/services/sync.py:374
- Finding
- Path Traversal Allows Recursive Deletion Outside Managed Skill Directories<![CDATA[ ## Vulnerability Details **File Location**: `src/controllers/cli.py:228-232`; `src/services/sync.py:374-415` **Vulnerability Type**: Unvalidated path component used in recursive filesystem deletion **Risk Level**: High ### Vulnerable Code ```python # src/controllers/cli.py:228-232 elif command == "remove": if len(sys.argv) < 3: print("Usage: askill remove <skill-name>") return _print_remove(sys.argv[2]) ``` ```python # src/services/sync.py:374-415 def remove_skill(skill_name: str, verbose: bool = True) -> list[str]: """Remove a skill from central repo and all products.""" skill_dir = CENTRAL_DIR / skill_name if not skill_dir.exists(): if verbose: print(f"Skill not found in central repo: {skill_name}") return [] removed = [] for p in PRODUCTS: if p["sync_method"] in ("native", "pack"): continue target = get_product_path(p) if target is None: continue link_path = target / skill_name if link_path.exists() or link_path.is_symlink(): remove_path(link_path) removed.append(p["short"]) for d in get_all_product_dirs(p)[1:]: link_path = d / skill_name if link_path.exists() or link_path.is_symlink(): remove_path(link_path) removed.append(f"{p['short']}-alt") if p.get("settings_file"): _remove_from_workbuddy_settings( p["settings_file"], skill_name, removed, verbose=verbose ) shutil.rmtree(skill_dir) removed.append("central") ``` The deletion helper can recursively remove ordinary directories: ```python # src/utils/filesystem.py:122-135 def remove_path(path: Path) -> None: path = Path(path) if not path.exists() and not path.is_symlink(): return if is_symlink_or_junction(path): path.rmdir() elif path.is_dir(): shutil.rmtree(path) else ...[truncated 2306 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Restrict skill names to one safe path component using a strict allowlist, for example: ```python import re SKILL_NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$") def validate_skill_name(name: str) -> str: if not SKILL_NAME_RE.fullmatch(name): raise ValueError("Invalid skill name") if name in {".", ".."}: raise ValueError("Invalid skill name") return name ``` 2. Reject absolute paths and path separators explicitly: ```python candidate = Path(skill_name) if candidate.is_absolute() or len(candidate.parts) != 1: raise ValueError("Skill name must be a single path component") ``` 3. Before every removal, resolve the candidate and verify containment under the expected root: ```python def contained_path(root: Path, name: str) -> Path: root = root.resolve() candidate = (root / name).resolve(strict=False) if candidate.parent != root: raise ValueError("Path escapes managed root") return candidate ``` 4. Apply containment checks independently to the central directory, each product directory, and every alternate directory. 5. Confirm that the central target contains a regular `SKILL.md` before treating it as a removable skill. 6. Add regression tests covering `../x`, `../../x`, absolute paths, platform-specific separators, symlink edge cases, and paths sharing only a textual prefix with the managed root. ]]>