T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/common.py:25
- Finding
- Unrestricted Path Inputs Permit File Creation and Overwrite Outside the Run Directory<![CDATA[ ## Vulnerability Details **File Location**: `scripts/common.py:25-38`, `scripts/upsert_node.py:26-38,59-65,91-93`, `scripts/merge_results.py:11-13,25-41` **Vulnerability Type**: Path traversal and unrestricted filesystem write **Risk Level**: Medium ### Vulnerable Code From `scripts/common.py:25-38`: ```python def node_dir(run_dir: str | Path, node_id: str) -> Path: return run_path(run_dir) / 'nodes' / node_id def spec_path(run_dir: str | Path, node_id: str) -> Path: return node_dir(run_dir, node_id) / 'spec.json' def notes_path(run_dir: str | Path, node_id: str) -> Path: return node_dir(run_dir, node_id) / 'notes.md' def result_path(run_dir: str | Path, node_id: str) -> Path: return node_dir(run_dir, node_id) / 'result.md' ``` From `scripts/upsert_node.py:26-38`: ```python parser.add_argument('--id', required=True, help='Node id, e.g. 1.2') parser.add_argument('--parent-id') parser.add_argument('--goal') parser.add_argument('--type', choices=['research', 'coding', 'ops', 'browser', 'synthesis', 'review']) parser.add_argument('--executor') parser.add_argument('--status', choices=['planned', 'running', 'completed', 'failed', 'waiting_for_approval', 'blocked']) parser.add_argument('--depth', type=int) parser.add_argument('--confidence', choices=['unknown', 'low', 'medium', 'high']) parser.add_argument('--workspace-mode', choices=['artifacts', 'worktree']) parser.add_argument('--approval-required', action='store_true') parser.add_argument('--depends-on', action='append') parser.add_argument('--summary') parser.add_argument('--artifact', action='append') ``` From `scripts/upsert_node.py:59-65`: ```python artifacts = list(dict.fromkeys((existing.get('artifacts', []) + (args.artifact or [])))) if not artifacts: artifacts = [ str(spec_path(run_dir, args.id).relative_to(run_dir)), str(result_path(run_dir, args.id).relative_to(run_dir)), ] ``` From `scripts/upsert_node.py:91-93`: ```python nodes[args.id] = ...[truncated 5028 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Strictly validate node identifiers** Require the documented dotted-numeric format before using an ID: ```python import re NODE_ID_PATTERN = re.compile(r'^[1-9][0-9]*(?:\.[1-9][0-9]*)*$') def validate_node_id(node_id: str) -> str: if not NODE_ID_PATTERN.fullmatch(node_id): raise ValueError('Invalid node ID') return node_id ``` Apply this validation in every script accepting or consuming node IDs, including IDs loaded from `tree.json`. 2. **Enforce a filesystem containment boundary** Resolve both the trusted base and candidate path, then verify containment before any read or write: ```python def confined_path(base: Path, candidate: Path) -> Path: base = base.expanduser().resolve() candidate = candidate.expanduser().resolve() try: candidate.relative_to(base) except ValueError: raise ValueError(f'Path escapes permitted directory: {candidate}') return candidate ``` Use the resolved `run_dir / "nodes"` directory as the boundary for node files. 3. **Restrict merge output destinations** Prefer removing unrestricted `--out` support. If custom names are necessary, accept only a filename or a run-relative path and reject absolute paths and traversal components. Validate the destination before calling `write_text()`. 4. **Validate before performing side effects** Move all path validation ahead of directory creation, file creation, or overwrite operations. Do not rely on the later `relative_to()` call used only for event formatting. 5. **Treat loaded tree data as untrusted** Validate `tree.json` against `references/tree-schema.json` and add semantic checks that the schema does not currently express, including: - node dictionary keys must equal each node's `id`; - all node IDs must follow the permitted format; - child and dependency IDs must refer to existing nodes; - all generated node ...[truncated 610 chars]