T09 · Insecure Skill Coding Practices
Error
- Location
scripts/state_store.py:46- Finding
Unrestricted State Key Allows Path Traversal and Arbitrary JSON File Access
- Content
View full analysis
Path: key = state_key or repo.state_key return DEFAULT_STATE_ROOT / f"{key}.json" def load_state(repo: RepoSpec, state_key: str | None = None) -> tuple[StateData, Path, bool]: path = state_path(repo, state_key) path.parent.mkdir(parents=True, exist_ok=True) if not path.exists(): return StateData(repo=repo.slug), path, True data = json.loads(path.read_text(encoding="utf-8")) state = StateData( repo=data.get("repo", repo.slug), processed_tags=list(data.get("processed_tags", [])), latest_processed_release_id=data.get("latest_processed_release_id"), latest_processed_published_at=data.get("latest_processed_published_at"), last_checked_at=data.get("last_checked_at"), last_success_at=data.get("last_success_at"), initialized_at=data.get("initialized_at"), ) is_first_run = not bool(state.initialized_at) return state, path, is_first_run def save_state(path: Path, state: StateData) -> None: path.parent.mkdir(parents=True, exist_ok=True) path.write_text(json.dumps(state.to_dict(), ensure_ascii=False, indent=2) + "\n", encoding="utf-8") ``` ### Technical Analysis The `--state-key` command-line value is inserted directly into a filesystem path without validation, normalization, or a containment check. A state key containing parent-directory components such as `../` can cause the resulting path to escape `DEFAULT_STATE_ROOT`. Both cron preparation and commit operations call `load_ ...[truncated 2205 chars]- Remediation
View remediation
str: if not STATE_KEY_PATTERN.fullmatch(key): raise ValueError("invalid state key") if key in {".", ".."}: raise ValueError("invalid state key") return key ``` 2. Resolve the state root and candidate path, then enforce containment: ```python def state_path(repo: RepoSpec, state_key: str | None = None) -> Path: key = validate_state_key(state_key or repo.state_key) root = DEFAULT_STATE_ROOT.resolve() candidate = (root / f"{key}.json").resolve() if candidate.parent != root: raise ValueError("state path escapes the configured state root") return candidate ``` 3. Explicitly reject `/`, `\`, absolute paths, drive prefixes, null bytes, and parent-directory components for cross-platform safety. 4. Protect against symbolic-link attacks. Refuse state paths that are symlinks and, where supported, use no-follow file operations. 5. Write state atomically through a securely created temporary file in the same directory, flush and synchronize it as appropriate, and replace the destination only after serialization succeeds. 6. Consider setting restrictive state directory and file permissions because release state should not be writable by unrelated local users. 7. Add automated tests for: - Unix and Windows traversal sequences. - Absolute paths and drive-qualified paths. - Nested separators. - Symbolic-link targets. - Excessively long state keys. - Valid default and explicitly supplied state keys. - Verification that every resolved path remains inside the configured state root. ]]>