T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/video.mjs:144
- Finding
- Unvalidated Storyboard ID Allows Output-Path Traversal## Vulnerability Details **File Location**: `scripts/video.mjs:144-145`, with the write occurring through `scripts/gen.mjs:219-232` **Vulnerability Type**: Path traversal and arbitrary file overwrite **Risk Level**: Medium ### Vulnerable Code ```js // scripts/video.mjs:144-145 const id = sh.id || `s${i + 1}` const save = path.join(o.outdir, `${id}.mp4`) ``` The resulting path is passed to `gen.mjs`, where it is used as a write destination: ```js // scripts/gen.mjs:219-232 if (savePath) { const ext = path.extname(savePath) || f.ext || '.jpg' const base = savePath.slice(0, savePath.length - path.extname(savePath).length) target = files.length > 1 ? `${base}-${i + 1}${ext}` : `${base}${ext}` } else { target = path.join('output', `${Date.now()}-${i + 1}${f.ext || '.jpg'}`) } await mkdir(path.dirname(target), { recursive: true }) const buf = f.buffer || Buffer.from(await (await fetch(f.url)).arrayBuffer()) await writeFile(target, buf) ``` The generated clip paths are subsequently placed in an ffmpeg concat manifest: ```js const listFile = path.join(o.outdir, 'concat.txt') await writeFile(listFile, clips.map((c) => `file '${path.resolve(c)}'`).join('\n')) ``` ### Technical Analysis The `shots[].id` property comes directly from a storyboard JSON file and is incorporated into the output filename without validation. The code does not reject: - Parent-directory components such as `..` - Absolute paths - Platform-specific path separators - Quotes or newline characters - Other characters significant to ffmpeg concat manifests For example, an ID such as `../../target` causes `path.join(o.outdir, "../../target.mp4")` to resolve outside the intended output directory. `gen.mjs` then creates the relevant parent directories and writes the provider-generated content to that path. The path is also inserted into `concat.txt` using single-quoted ffmpeg concat syntax without escaping. A malic ...[truncated 1614 chars]
- Remediation
- ## Remediation Suggestions 1. **Validate storyboard IDs strictly.** Permit only characters required for filenames: ```js const SAFE_ID = /^[A-Za-z0-9_-]+$/ if (!SAFE_ID.test(id)) { throw new Error(`Invalid shot ID: ${id}`) } ``` 2. **Enforce output-directory containment.** Resolve both the base directory and candidate path, then reject escaped destinations: ```js const root = path.resolve(o.outdir) const save = path.resolve(root, `${id}.mp4`) if (path.dirname(save) !== root) { throw new Error('Shot output path escapes the output directory') } ``` 3. **Generate internal filenames independently of untrusted IDs.** Prefer deterministic names such as `shot-1.mp4`; retain the supplied ID only as display metadata. 4. **Harden `gen.mjs` as a second line of defense.** If it is expected to restrict writes to a designated output root, accept that root explicitly and verify containment before calling `mkdir` or `writeFile`. 5. **Safely construct ffmpeg inputs.** Avoid directly interpolating attacker-controlled paths into concat manifests. If concat files remain necessary, reject control characters and correctly escape apostrophes and backslashes according to ffmpeg concat-file rules. 6. **Add regression tests** covering `../`, absolute paths, Windows separators, apostrophes, newlines, Unicode separators, and benign IDs.