T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/graph_manager.py:41
- Finding
- Path Traversal in Graph File Operations## Vulnerability Details **File Location**: `scripts/graph_manager.py:41-42`, `scripts/graph_manager.py:52-56`, `scripts/graph_manager.py:74-80`, `scripts/graph_manager.py:280-284` **Vulnerability Type**: Path traversal leading to unauthorized file read, write, and deletion **Risk Level**: High ### Vulnerable Code ```python def _get_graph_path(self, pipeline_id: str) -> str: """Get the graph file path.""" return os.path.join(self.storage_dir, f"{pipeline_id}.json") ``` ```python graph_path = self._get_graph_path(pipeline_id) if os.path.exists(graph_path): try: with open(graph_path, 'r', encoding='utf-8') as f: data = json.load(f) self.cache[pipeline_id] = data return data ``` ```python def _save_graph(self, pipeline_id: str, data: Dict): """Save graph data.""" graph_path = self._get_graph_path(pipeline_id) self.cache[pipeline_id] = data with open(graph_path, 'w', encoding='utf-8') as f: json.dump(data, f, ensure_ascii=False, indent=2) ``` ```python graph_path = self._get_graph_path(pipeline_id) if os.path.exists(graph_path): os.remove(graph_path) return {"success": True, "message": "Graph deleted"} ``` ### Technical Analysis `pipeline_id` is incorporated directly into a filesystem path without validating its format, rejecting path separators, or verifying that the resulting canonical path remains under `storage_dir`. A value containing traversal components such as `../` can escape the intended `data/graphs` directory. An absolute path can also cause `os.path.join()` to discard the storage-directory prefix. The implementation then uses the resulting path in read, write, and deletion operations. The `.json` suffix limits the vulnerable operations to paths ending in `.json`, and reads require valid JSON because the code calls `json.load()`. These restrictions do not prevent access to other JSON co ...[truncated 1216 chars]
- Remediation
- ## Remediation Suggestions 1. Enforce a strict allowlist for identifiers, for example: ```python import re PIPELINE_ID_PATTERN = re.compile(r"\Apipeline_[0-9]{14}\Z") def _validate_pipeline_id(self, pipeline_id: str) -> None: if not PIPELINE_ID_PATTERN.fullmatch(pipeline_id): raise ValueError("Invalid pipeline ID") ``` 2. Explicitly reject absolute paths, path separators, `.` components, and `..` components. 3. Resolve and verify canonical paths before every operation: ```python def _get_graph_path(self, pipeline_id: str) -> str: self._validate_pipeline_id(pipeline_id) base = os.path.realpath(self.storage_dir) target = os.path.realpath(os.path.join(base, f"{pipeline_id}.json")) if os.path.commonpath([base, target]) != base: raise ValueError("Graph path escapes storage directory") return target ``` 4. Run the application with a dedicated, least-privileged account that cannot modify unrelated files. 5. Use atomic writes through a safely created temporary file in the validated graph directory, followed by `os.replace()`. 6. Add tests covering `../`, absolute paths, nested traversal, alternate separators, empty identifiers, and symbolic-link edge cases.