T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/fetch-issues.sh:838
- Finding
- Arbitrary Recursive Deletion Through Unrestricted History Directory Retention<![CDATA[ ## Vulnerability Details **File Location**: `scripts/fetch-issues.sh`, lines 838-858 **Vulnerability Type**: Unsafe filesystem deletion caused by an unrestricted user-controlled retention root **Risk Level**: High ### Vulnerable Code ```bash if [[ -d "$HISTORY_DIR" ]]; then mapfile -t RUN_DIRS < <( find "$HISTORY_DIR" -mindepth 1 -maxdepth 1 -type d -printf '%T@ %p\n' \ | sort -nr \ | while IFS= read -r line; do printf '%s\n' "${line#* }" done ) if [[ "${#RUN_DIRS[@]}" -gt "$RETAIN" ]]; then for ((i = RETAIN; i < ${#RUN_DIRS[@]}; i++)); do if [[ "${RUN_DIRS[$i]}" == "$WORKDIR" ]]; then continue fi rm -rf "${RUN_DIRS[$i]}" done fi fi ``` ### Technical Analysis The `--history-dir` argument accepts an arbitrary filesystem directory. During retention cleanup, the script assumes that every immediate child directory under `HISTORY_DIR` is an issue-prioritizer run and passes older entries directly to `rm -rf`. There is no validation that a deletion candidate: - Uses the expected run-directory naming convention. - Contains a valid issue-prioritizer manifest. - Was created by this script. - Remains inside a dedicated application-owned state directory after canonicalization. - Is not a sensitive directory such as the user's home directory, a project directory, or a shared state directory. Consequently, setting `HISTORY_DIR` to a broad directory causes unrelated child directories to be included in `RUN_DIRS`. A low retention value can then cause those unrelated directories to be recursively deleted. Quoting the path prevents shell word splitting and command injection, but it does not address the authorization flaw: the script is still intentionally issuing `rm -rf` against insufficiently validated paths. This behavior also exceeds the Skill's declared read-only operation. Although it does not mutate the remote GitHub repository, it can destructively modify the local filesystem. ...[truncated 1827 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Use a dedicated retention root** - Default to the application-specific state directory. - Do not permit broad directories such as `/`, `$HOME`, `/home`, `/tmp`, or an existing project root to serve as retention roots. 2. **Canonicalize and validate all paths** - Resolve `HISTORY_DIR`, `WORKDIR`, and each deletion candidate with `realpath` or `readlink -f`. - Verify that every candidate is a strict descendant of the canonical history directory. - Reject empty paths and dangerous canonical roots. 3. **Identify owned run directories** - Delete only directories whose names match a strict run identifier pattern. - Require a valid `manifest.json` containing the expected application identity and run identifier. - Confirm that the manifest's canonical `workdir` matches the candidate directory. 4. **Separate run discovery from arbitrary child discovery** - Replace unrestricted `find ... -type d` enumeration with a constrained pattern, such as application-generated run names. - Prefer maintaining an explicit run index rather than treating all child directories as owned runs. 5. **Add destructive-operation safeguards** - Refuse retention cleanup when a custom history directory is not empty and contains unrecognized entries. - Consider requiring explicit confirmation before applying retention to a non-default location. - Log each validated deletion target before deletion. 6. **Avoid broad recursive deletion where possible** - Delete only known files and directories within validated run roots. - If `rm -rf` remains necessary, use a helper that enforces ownership markers and containment checks immediately before deletion. 7. **Add regression tests** - Verify that `--history-dir "$HOME"` and other broad paths are rejected. - Verify that unrelated sibling directories are never deleted. - Test canonical-path containment, whitespace in paths, custom work directories, and retention v ...[truncated 25 chars]