T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/github_safe_sync.py:234
- Finding
- Arbitrary Backup Prefix Can Cause Broad Repository Branch Deletion<![CDATA[ ## Vulnerability Details **File Location**: `scripts/github_safe_sync.py:234-253` and `scripts/github_safe_sync.py:291-294` **Vulnerability Type**: Insufficient validation of destructive-operation scope **Risk Level**: Medium ### Vulnerable Code ```python def cmd_delete_backups(args: argparse.Namespace) -> int: token = get_token() repo_path = f"/repos/{args.owner}/{args.repo}" branches = [ item["name"] for item in paginate(f"{repo_path}/branches", token) if item["name"].startswith(args.backup_prefix) ] deleted: list[str] = [] for name in branches[: args.limit]: if args.dry_run: deleted.append(name) continue encoded = urllib.parse.quote(f"heads/{name}", safe="") api_request(f"{repo_path}/git/refs/{encoded}", token, method="DELETE") deleted.append(name) print(json.dumps({"matched_count": len(branches), "processed_count": len(deleted), "branches": deleted}, ensure_ascii=False, indent=2)) return 0 ``` ```python delete_backups = sub.add_parser("delete-backups", help="Delete backup/* branches.") delete_backups.add_argument("--owner", required=True) delete_backups.add_argument("--repo", required=True) delete_backups.add_argument("--backup-prefix", default="backup/") delete_backups.add_argument("--limit", type=int, default=1000) delete_backups.add_argument("--dry-run", action="store_true") delete_backups.set_defaults(func=cmd_delete_backups) ``` ### Technical Analysis The `delete-backups` command is documented as deleting branches under the `backup/` namespace, but the namespace is controlled by the unrestricted `--backup-prefix` argument. The code selects branches solely through: ```python item["name"].startswith(args.backup_prefix) ``` An empty string is a valid command-line value and every branch name starts with an empty string. Consequently, passing `--backup-prefix ""` causes all enumerated branches to match. An overly broad value can s ...[truncated 2211 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Reject empty, whitespace-only, or overly broad prefixes before enumerating branches: ```python prefix = args.backup_prefix.strip() if not prefix: raise SystemExit("--backup-prefix must not be empty.") ``` 2. Prefer enforcing the documented namespace instead of accepting an arbitrary prefix: ```python if prefix != "backup/": raise SystemExit("Only the backup/ branch namespace may be deleted.") ``` 3. If custom backup namespaces are required, validate them against a strict allowlist or pattern and require a trailing slash. Do not allow values such as `/`, a single common character, or other prefixes capable of matching unrelated branches. 4. Make preview mode the default. Require an explicit flag such as `--confirm-delete` or `--execute` before issuing any `DELETE` request. 5. Print the complete set of matched branches before mutation and require explicit confirmation when running interactively. 6. Add a second invariant immediately before each deletion: ```python if not name.startswith("backup/"): raise SystemExit(f"Refusing to delete non-backup branch: {name}") ``` 7. Refuse deletion of the repository's default branch and other protected or allowlisted branches, even if they unexpectedly match the configured prefix. 8. Apply a conservative default limit and require additional confirmation when the number of matched branches exceeds a small threshold. 9. Add automated tests covering empty prefixes, whitespace prefixes, broad prefixes, dry-run behavior, protected names, and attempts to select branches outside `backup/`. ]]>