T09 · Insecure Skill Coding Practices
Error
- Location
- index.js:18
- Finding
- Path Prefix Validation Allows Out-of-Scope Recursive Deletion## Vulnerability Details **File Location**: `index.js`, lines 18-25; deletion sink at lines 38-46 **Vulnerability Type**: Improper path containment validation **Risk Level**: High ### Vulnerable Code ```js const allowedPrefixes = [ path.join(workspaceRoot, 'temp'), path.join(workspaceRoot, 'logs'), path.join(workspaceRoot, 'cache') ]; const isAllowed = allowedPrefixes.some(prefix => absolutePath.startsWith(prefix)); if (!isAllowed) { // Also allow specific single files if they are clearly temp files in root (e.g. *.tmp, *.log) // But generally enforce temp/ usage. // For now, strict safety: only allow deletion inside temp/, logs/, or cache/ throw new Error(`Safety Warning: Deletion denied for path '${targetPath}'. Only paths inside 'temp/', 'logs/', or 'cache/' are allowed.`); } if (absolutePath === workspaceRoot || absolutePath === path.join(workspaceRoot, 'skills')) { throw new Error('CRITICAL: Attempted to delete workspace root or skills directory.'); } try { const stats = await fs.promises.stat(absolutePath); if (stats.isDirectory()) { await fs.promises.rm(absolutePath, { recursive: true, force: true }); return `Successfully deleted directory: ${targetPath}`; } else { await fs.promises.unlink(absolutePath); return `Successfully deleted file: ${targetPath}`; } ``` ### Technical Analysis The implementation uses `String.prototype.startsWith()` to determine whether a resolved path is within one of the permitted directories. This checks only the textual prefix and does not enforce a filesystem path-component boundary. Consequently, sibling paths such as `temperature`, `logs-backup`, and `cache-secrets` are incorrectly accepted because their absolute path strings begin with the strings for `temp`, `logs`, or `cache`. Traversal input can produce the same result: for example, `temp/../temperature` resolves to the sibling directory `temperature`, after ...[truncated 1703 chars]
- Remediation
- ## Remediation Suggestions Replace textual prefix checks with path-component-aware containment validation. For example: ```js function isWithinOrEqual(parent, candidate) { const relative = path.relative(parent, candidate); return relative === '' || (relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); } const isAllowed = allowedPrefixes.some(prefix => isWithinOrEqual(prefix, absolutePath) ); ``` If deleting `temp/`, `logs/`, or `cache/` themselves is not required, reject `relative === ''` and permit descendants only. Additional hardening should include: 1. Resolve the workspace root once from an explicitly trusted configuration rather than implicitly relying on the caller's current working directory. 2. Define whether symbolic links are permitted. If not, inspect path components with `lstat()` and reject symlinked components before deletion. 3. Minimize time-of-check/time-of-use exposure where feasible, especially if untrusted users can modify workspace paths concurrently. 4. Add automated negative tests for `temperature`, `logs-backup`, `cache-secrets`, `temp/../temperature`, absolute out-of-scope paths, and other prefix collisions. 5. Add positive tests confirming that genuine descendants such as `temp/session/file.tmp` remain permitted. 6. Test whether deletion of the three allowed root directories should be explicitly prohibited.