T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/cleanup.sh:10
- Finding
- Destructive Cleanup Executes by Default Without Confirmation## Vulnerability Details **File Location**: `scripts/cleanup.sh:10-15`, `scripts/cleanup.sh:22-28`, `scripts/cleanup.sh:104-112`; conflicting claims in `SKILL.md:8-16` **Vulnerability Type**: Unsafe destructive default and ineffective confirmation control **Risk Level**: High ### Vulnerable Code ```bash DRY_RUN=false SKIP_KERNELS=false SKIP_SNAP=false SKIP_DOCKER=false SKIP_BREW=false AUTO_YES=false ``` ```bash for arg in "$@"; do case "$arg" in --dry-run) DRY_RUN=true ;; --skip-kernels) SKIP_KERNELS=true ;; --skip-snap) SKIP_SNAP=true ;; --skip-docker) SKIP_DOCKER=true ;; --skip-brew) SKIP_BREW=true ;; --yes|-y) AUTO_YES=true ;; ``` ```bash do_clean() { local desc="$1" shift if $DRY_RUN; then echo "[DRY-RUN] Would: $desc" else echo "Cleaning: $desc" "$@" 2>/dev/null || true fi } ``` ### Technical Analysis The script initializes `DRY_RUN` to `false`, so invoking it without arguments enables destructive operations immediately. Although it parses `--yes` into `AUTO_YES`, that variable is never consulted before cleanup begins. There is also no interactive confirmation prompt. This behavior conflicts with the documentation's claims that the Skill is “safe by default” and that dry-run mode protects users from unintended changes. The implementation instead requires users to explicitly request safety with `--dry-run`. The generic `do_clean` function executes every supplied deletion or cleanup command whenever `DRY_RUN` is false. This includes `rm -rf`, package cleanup, journal vacuuming, snap removal, Docker pruning, and kernel package removal. Several Linux operations invoke `sudo`, increasing the affected scope from user-owned files to system resources. Errors are suppressed through `2>/dev/null || true`, which can also conceal partial failures and make it difficult for users to determine exactly which destruct ...[truncated 1717 chars]
- Remediation
- ## Remediation Suggestions 1. Make dry-run behavior the default: ```bash DRY_RUN=true ``` 2. Require explicit authorization before enabling destructive mode: ```bash --yes|-y) AUTO_YES=true DRY_RUN=false ;; ``` 3. If neither `--dry-run` nor `--yes` is supplied, show the planned operations and require an interactive confirmation. Abort when standard input is not an interactive terminal. 4. Check `AUTO_YES` before the first destructive operation rather than merely parsing it. 5. Use separate opt-in flags for high-impact categories such as kernels, Docker, Xcode archives, snaps, and system logs. These should not be included in a generic default cleanup. 6. Avoid blanket error suppression. Log command failures and return a nonzero status when critical cleanup operations fail. 7. Update `SKILL.md` so the documented default behavior precisely matches the implementation and clearly identifies operations requiring `sudo`.