T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/dir_organizer.py:40
- Finding
- Destination Collision Can Cause Existing Files to Be Overwritten## Vulnerability Details **File Location**: `scripts/dir_organizer.py:40-43` **Vulnerability Type**: Unsafe file move without collision protection **Risk Level**: Medium **Vulnerable Code**: ```python if a.apply: for src, dst in plan: os.makedirs(os.path.dirname(dst), exist_ok=True) shutil.move(src, dst); moved += 1 ``` ### Technical Analysis The destination path is derived from the source filename and category, but the implementation does not test whether that path already exists before calling `shutil.move`. On platforms where the underlying rename operation replaces an existing destination file, such as typical same-filesystem POSIX behavior, the existing categorized file may be silently overwritten. Other platforms may reject the operation, but because exceptions are not handled, such a failure can terminate execution after some earlier files have already been moved. The dry-run output also does not identify collisions, so users cannot reliably detect this condition before applying the plan. ### Attack Path 1. A directory contains a root-level file named `report.pdf`. 2. Its generated destination is `<target>/文档/report.pdf`. 3. A file already exists at that destination. 4. The user reviews the dry-run, which reports the intended category but does not warn about the collision. 5. The user executes the organizer with `--apply`. 6. On a platform permitting replacement, the root-level file replaces the existing destination file. On a platform rejecting replacement, execution may stop after partially applying the organization plan. ### Impact Assessment Exploitation requires the ability to create or influence filenames in the selected directory, or an accidental preexisting filename collision. No additional system privileges are obtained. The impact is confined to files writable by the user running the Skill, but it can include permanent loss of existing local data and a partially compl ...[truncated 30 chars]
- Remediation
- ## Remediation Suggestions - Check `os.path.lexists(dst)` before every move. - Treat collisions as findings during dry-run and display the conflicting source and destination paths. - Refuse to overwrite existing files by default. - If collision handling is required, provide explicit policies such as `--on-conflict skip`, `--on-conflict rename`, or `--on-conflict overwrite`. - Require a separate, explicit confirmation flag for destructive overwrite behavior. - Catch filesystem exceptions per file and return a nonzero exit status with a structured error report. - Consider validating the complete plan before moving any files to reduce partial execution. - Add tests covering collisions, symbolic links, cross-filesystem moves, and platform-specific destination behavior.