T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/build_dashboard_bundle.py:2683
- Finding
- Arbitrary Output Directory Cleanup Can Delete Unrelated Files<![CDATA[ ## Vulnerability Details **File Location**: `scripts/build_dashboard_bundle.py`, lines 2683-2690; invoked at lines 2875-2879 and 2949 **Vulnerability Type**: Unsafe recursive deletion using a user-controlled output path **Risk Level**: High ### Vulnerable Code ```python def cleanup_output_dir(output_dir: Path) -> None: for folder_name in ["png", "excel", "ppt"]: folder = output_dir / folder_name if folder.exists(): shutil.rmtree(folder) for file_path in output_dir.iterdir() if output_dir.exists() else []: if file_path.is_file() and file_path.name != "dashboard.html": file_path.unlink() ``` The cleanup function is invoked against the resolved command-line path before workbook validation: ```python def main() -> None: args = parse_args() output_dir = Path(args.output_dir).resolve() output_dir.mkdir(parents=True, exist_ok=True) cleanup_output_dir(output_dir) ``` It is invoked again after generating the HTML: ```python build_html(meta, html_path, dashboard1_image, dashboard2_image, d3_df, d4_df, d3_meta, d4_meta, d5_df=d5_df, executive_summary=executive_summary, d1_df=d1_df, d1_meta=d1_meta, d2_df=d2_df, d2_meta=d2_meta) cleanup_output_dir(output_dir) print(html_path) ``` ### Technical Analysis The `--output-dir` argument is fully user-controlled. Although the path is normalized with `resolve()`, the script does not verify that it is a newly created, Skill-owned directory or that it resides within the current workspace. The cleanup operation: - Recursively deletes any `png`, `excel`, or `ppt` directory beneath the selected path. - Deletes every top-level regular file except one named `dashboard.html`. - Runs before validating whether the input workbooks are usable. - Runs again after dashboard generation. - Does not require an ownership marker or explicit confirmation. Path normalization does not provide a security boundary. An absolute path, workspace root, or e ...[truncated 1542 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Require a dedicated output directory rather than accepting an arbitrary existing directory. 2. Reject dangerous destinations, including filesystem roots, user home directories, and the current workspace root. 3. Create a Skill-specific ownership marker, such as `.hr-workforce-dashboard-output`, and refuse cleanup unless the marker is present. 4. Validate all input workbooks before performing any destructive output operation. 5. Delete only an explicit allowlist of files generated by this Skill, rather than all top-level files. 6. Avoid recursive deletion of generic directory names. Remove only known artifact files inside Skill-owned directories. 7. Prefer a fresh temporary staging directory and atomically move completed artifacts to the requested destination. 8. If an existing non-empty output directory is supplied, fail safely or require explicit user confirmation. 9. Resolve each deletion target and verify that it remains beneath the approved output root before deletion. 10. Add automated tests covering dangerous values such as `/`, a home directory, the workspace root, and an existing unrelated project directory. ]]>