T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/write.py:263
- Finding
- Preset path traversal can disclose arbitrary local Markdown files<![CDATA[ ## Vulnerability Details **File Location**: `scripts/write.py:263-281, 293-304` **Vulnerability Type**: Path traversal leading to local file disclosure **Risk Level**: Medium ### Vulnerable Code ```python def _find_preset_file( preset_dirs: list[Path], subdir: str, name: str, exts: list[str], ) -> Path | None: for root in preset_dirs: d = root / subdir if not d.exists(): continue for ext in exts: p = d / f"{name}{ext}" if p.exists(): return p return None def _load_closing_block(screening: dict, article_cfg: dict) -> str: """ Closing block: preset selection reads default_closing_block from article.yaml. If no preset is selected, use the inline closing_block from merged context. """ default_name = _coerce_single_preset( "default_closing_block", article_cfg.get("default_closing_block"), ) if default_name: preset_dirs = _preset_dirs(_aws_root()) found = _find_preset_file( preset_dirs, "closing-blocks", default_name, [".md"], ) if found: _info(f"Loading closing block preset: {found}") return found.read_text(encoding="utf-8") return (screening.get("closing_block") or "").strip() ``` The same unsafe lookup is also used by `_load_structure_template()` for `default_structure`. ### Technical Analysis Preset names read from the article's `article.yaml` are appended directly to a preset directory: ```python p = d / f"{name}{ext}" ``` The code does not reject: - `..` path components; - forward or backward path separators; - absolute paths; - paths that resolve outside the intended preset directory. It also does not resolve the resulting candidate and verify that the candidate remains a descendant of `presets/structures` or `presets/closing-blocks`. For example, a preset value conceptually equivalent to ...[truncated 2006 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Treat preset values strictly as logical names rather than paths. 1. Reject empty names, absolute paths, `..`, `/`, and `\`. 2. Restrict names to an explicit safe character set, such as letters, digits, spaces, underscores, and hyphens. 3. Resolve the candidate and verify that it remains below the intended preset directory. 4. Require `candidate.is_file()` rather than only `candidate.exists()`. 5. Apply the same validation to both structure and closing-block presets. 6. Add regression tests for traversal, absolute paths, encoded separators, and valid Unicode preset names. Example hardening: ```python _SAFE_PRESET_NAME = re.compile(r"^[\w -]+$", re.UNICODE) def _safe_preset_candidate( preset_dir: Path, name: str, ext: str, ) -> Path | None: if ( not name or Path(name).is_absolute() or ".." in Path(name).parts or "/" in name or "\\" in name or not _SAFE_PRESET_NAME.fullmatch(name) ): _err("Preset name contains prohibited path characters") root = preset_dir.resolve() candidate = (root / f"{name}{ext}").resolve() try: candidate.relative_to(root) except ValueError: _err("Preset path escapes the permitted preset directory") return candidate if candidate.is_file() else None ``` Where supported, `candidate.is_relative_to(root)` can be used instead of the `relative_to()` exception pattern. ]]>