T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/holdings_sync.py:198
- Finding
- Unvalidated Date Values Allow Arbitrary JSON File Writes Outside the Portfolio Directory## Vulnerability Details **File Location**: `scripts/holdings_sync.py:198-201` **Related Location**: `scripts/persist_snapshot.py:56-61` **Vulnerability Type**: Path traversal and unrestricted file write **Risk Level**: High ### Vulnerable Code ```python def write_holdings(portfolio_dir: Path, holdings_payload: dict[str, Any]) -> Path: holdings_path = portfolio_dir / "holdings" / f"{holdings_payload['date']}.json" _write_json(holdings_path, holdings_payload) return holdings_path ``` The same pattern is present in snapshot persistence: ```python def write_snapshot(portfolio_dir: Path, snapshot: dict[str, Any]) -> Path: snap_dir = portfolio_dir / "snapshots" snap_dir.mkdir(parents=True, exist_ok=True) snap_path = snap_dir / f"{snapshot['date']}.json" snap_path.write_text(json.dumps(snapshot, ensure_ascii=False, indent=2), encoding="utf-8") return snap_path ``` ### Technical Analysis The `date` field is used directly as a filename without enforcing the documented `YYYY-MM-DD` format or checking that the resulting path remains inside the intended `holdings` or `snapshots` directory. In `holdings_sync.py`, validation only verifies that the top-level `date` field exists. Normalization converts it to a stripped string but does not reject absolute paths, directory separators, `..` components, or non-date values. With `pathlib`, joining a directory with an absolute path discards the preceding directory. For example: ```python Path("/portfolio/holdings") / "/tmp/controlled.json" ``` resolves to `/tmp/controlled.json`. Because the implementation appends `.json`, an input date such as `/tmp/controlled` results in a write to `/tmp/controlled.json`. Relative traversal strings can similarly escape the portfolio directory. The generic `_write_json()` helper also creates parent directories, increasing the reachable write scope: ```python def _write_json(path: Path, payload ...[truncated 2388 chars]
- Remediation
- ## Remediation Suggestions 1. Strictly validate all dates before using them in paths: ```python from datetime import datetime def validate_date(value: Any) -> str: if not isinstance(value, str): raise ValueError("date must be a YYYY-MM-DD string") try: parsed = datetime.strptime(value, "%Y-%m-%d") except ValueError as exc: raise ValueError("date must use YYYY-MM-DD format") from exc normalized = parsed.strftime("%Y-%m-%d") if normalized != value: raise ValueError("date is not canonical") return normalized ``` 2. Apply this validation in holdings synchronization, portfolio analysis, snapshot persistence, and the refresh CLI. 3. Resolve and confine every generated path before writing: ```python base = (portfolio_dir / "holdings").resolve() destination = (base / f"{validated_date}.json").resolve() if destination.parent != base: raise HoldingsSyncError("Holdings path escapes the holdings directory") ``` 4. Reject absolute paths, `/`, `\`, null bytes, and traversal components independently of date parsing as defense in depth. 5. Refuse to write through symbolic links, or use secure file-opening primitives with no-follow behavior where supported. 6. Write to a temporary file in the same trusted directory, flush and synchronize it, and atomically replace the destination. 7. Add regression tests for absolute paths, `../` traversal, Windows separators, malformed dates, symlink destinations, and direct invocation of every persistence entry point.