T09 · Insecure Skill Coding Practices
Error
- Location
src/services/sync.py:374- Finding
Path Traversal Allows Recursive Deletion Outside Managed Skill Directories
- Content
View full analysis
") 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
View remediation
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. ]]>
