T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/drawio_version.py:135
- Finding
- Directory Traversal Enables Recursive Deletion Outside Version Storage<![CDATA[ ## Vulnerability Details **File Location**: `scripts/drawio_version.py:29-32`, `scripts/drawio_version.py:135-146`; equivalent cleanup logic also exists at `scripts/drawio_hooks.py:570-584` **Vulnerability Type**: Unvalidated path construction followed by recursive deletion **Risk Level**: High ### Vulnerable Code ```python def _version_dir(self, filepath: str, version: str) -> Path: """Get the directory for a specified version.""" filename = Path(filepath).stem return self.versions_dir / filename / version ``` ```python if changelog_path.exists(): with open(changelog_path, "r", encoding="utf-8") as f: changelog = json.load(f) else: changelog = [] changelog.append(meta) # Delete the oldest version when the configured limit is exceeded while len(changelog) > self.max_versions: oldest = changelog.pop(0) old_dir = self._version_dir(filepath, oldest["version"]) if old_dir.exists(): shutil.rmtree(old_dir) ``` The hook-based cleanup contains the same unsafe construction: ```python changelog = vm.list_versions(output_path) while len(changelog) >= max_versions: oldest = changelog.pop(0) old_dir = Path(vm.versions_dir) / Path(output_path).stem / oldest['version'] if old_dir.exists(): import shutil shutil.rmtree(old_dir) ``` ### Technical Analysis Version identifiers are loaded from the writable file: ```text .drawio_versions/<diagram-name>/changelog.json ``` The `version` property is treated as a trusted path component without format validation. `pathlib.Path` does not prevent traversal components such as `../`, and joining an absolute version path can discard the preceding base path entirely. The resulting path is passed to `shutil.rmtree`, which recursively deletes the resolved directory. No canonicalization or containment check verifies that the deletion target remains under the expected per-diagram version directory. Although ordinary application-generated versions use v ...[truncated 1240 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Strictly validate every version identifier before using it: ```python import re VERSION_PATTERN = re.compile(r"^v[1-9][0-9]*$") def validate_version(version: str) -> str: if not isinstance(version, str) or not VERSION_PATTERN.fullmatch(version): raise ValueError("Invalid version identifier") return version ``` 2. Resolve the expected parent and candidate target, then enforce containment: ```python parent = (self.versions_dir / Path(filepath).stem).resolve() candidate = (parent / validate_version(version)).resolve() if candidate.parent != parent: raise ValueError("Version path escapes its storage directory") ``` 3. Reject absolute paths, traversal components, symbolic links, and unexpected filesystem object types. 4. Treat `changelog.json` as untrusted input. Validate its schema, entry types, required fields, version syntax, and maximum number of records before performing filesystem operations. 5. Refuse deletion when validation fails rather than catching the error and reporting cleanup as successful. 6. Consolidate deletion logic in one hardened `VersionManager` method so the hook implementation cannot bypass validation. 7. Add regression tests using values such as `../target`, `../../target`, absolute paths, malformed JSON records, and symlinked version directories. Verify that no path outside the expected version root can be removed. ]]>