T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/orchestrate_story.py:389
- Finding
- Path Traversal Through Unvalidated Shot and Task Identifiers<![CDATA[ ## Vulnerability Details **File Location**: `scripts/orchestrate_story.py:84-102` and `scripts/orchestrate_story.py:389-396` **Vulnerability Type**: Path traversal leading to arbitrary file write **Risk Level**: High ### Vulnerable Code ```python seen_ids = set() for i, shot in enumerate(shots): sid = shot.get("id") if not sid: raise OrchestratorError(f"shots[{i}] missing required field: id") if sid in seen_ids: raise OrchestratorError(f"Duplicate shot id: {sid}") seen_ids.add(sid) ratio = shot.get("ratio") if ratio and ratio not in ALLOWED_RATIOS: raise OrchestratorError(f"shots[{i}].ratio unsupported: {ratio}") resolution = shot.get("resolution") if resolution and resolution not in ALLOWED_RES: raise OrchestratorError(f"shots[{i}].resolution unsupported: {resolution}") ``` The identifier is subsequently used in a filesystem path: ```python output_file = "" if video_url: filename = f"{idx:02d}-{shot_id}-{task_id}.mp4" output_path = run_dir / filename download_video(video_url, output_path) output_file = str(output_path) ``` The download function creates any required parent directories and writes to the resulting path: ```python def download_video(video_url: str, output_path: Path) -> None: output_path.parent.mkdir(parents=True, exist_ok=True) urllib.request.urlretrieve(video_url, output_path) ``` ### Technical Analysis The storyboard validator requires a nonempty, unique shot identifier but does not restrict path separators, `..` components, absolute-path syntax, control characters, or identifier length. The `task_id` returned by the delegated `seedance.py` process is also not validated. Both values are interpolated directly into a filename. When the resulting value is joined to `run_dir`, embedded traversal components can cause the normalized destination to resolve outside the intended run directory. The code does not resolve the destination and verify t ...[truncated 1781 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Apply strict allowlists to both identifiers before they are used in filenames: ```python import re SAFE_ID = re.compile(r"^[A-Za-z0-9_-]{1,128}$") def validate_identifier(value: str, field: str) -> str: if not isinstance(value, str) or not SAFE_ID.fullmatch(value): raise OrchestratorError(f"Invalid {field}") return value ``` 2. Resolve the destination and enforce containment beneath the run directory: ```python base = run_dir.resolve() destination = (base / filename).resolve() try: destination.relative_to(base) except ValueError: raise OrchestratorError("Output path escapes the run directory") ``` 3. Generate local filenames independently of remote identifiers. Store the original task ID only as JSON metadata. 4. Reject absolute paths, path separators, `.` and `..` components, null bytes, control characters, and excessively long values. 5. Avoid silently overwriting existing files. Use exclusive creation or a collision-resistant, locally generated filename. 6. Add regression tests using traversal payloads in both `shot_id` and `task_id`, including mixed separators and deeply nested parent-directory components. ]]>