T09 · Insecure Skill Coding Practices
Error
- Location
- src/core/file-manager.js:141
- Finding
- Unrestricted File Deletion Through Temporary-File Cleanup API## Vulnerability Details **File Location**: `src/core/file-manager.js`, lines 141–145 **Vulnerability Type**: Arbitrary file deletion / insufficient path validation **Risk Level**: High ### Vulnerable Code ```javascript async cleanupTempFile(filePath) { try { await fs.unlink(filePath); return true; } catch (error) { console.warn(`Failed to cleanup temp file ${filePath}: ${error.message}`); return false; } } ``` ### Technical Analysis `cleanupTempFile()` is documented as deleting a temporary file, but it passes the caller-controlled `filePath` directly to `fs.unlink()` without verifying that the target is inside the configured temporary directory. No canonicalization, ownership tracking, filename allowlist, or path-boundary check is performed. Consequently, an absolute path such as `/home/service/config.json`, or a traversal path escaping the temporary directory, can target any file writable by the Node.js process. The method is part of the exported `FileManager` public interface. Its documentation in `docs/API.md` also accepts a general `filePath`, so callers may invoke this primitive directly. Symbolic links and filesystem path aliases may further undermine a simple lexical prefix check unless the implementation resolves and validates canonical paths. ### Attack Path 1. An attacker obtains influence over the argument passed to `FileManager.cleanupTempFile()`. This may occur when an integrating agent, API, or application exposes the cleanup operation to untrusted input. 2. The attacker supplies the absolute path of a writable non-temporary file, for example: ```javascript await manager.cleanupTempFile('/home/service/app-config.json'); ``` 3. The method passes that path directly to `fs.unlink()`. 4. Node.js deletes the target using the host process's filesystem privileges. 5. The attacker may repeat the operation against application data, configuration, logs, or other writable files. The repository does not itself expo ...[truncated 805 chars]
- Remediation
- ## Remediation Suggestions Restrict cleanup to files created and tracked by this `FileManager` instance: 1. Resolve the configured temporary directory and candidate path to absolute canonical paths. 2. Reject targets outside the temporary-directory boundary. Use `path.relative()` rather than a raw string-prefix check. 3. Track paths returned by `createTempFile()` in a private set and only permit deletion of tracked entries. 4. Use unpredictable, securely generated filenames, such as `crypto.randomUUID()`. 5. Consider opening and creating temporary files with exclusive semantics and restrictive permissions. 6. Remove a path from the tracking set only after successful deletion. 7. Add tests covering absolute external paths, `../` traversal, prefix-confusion paths, and symbolic links. Example hardening pattern: ```javascript import path from 'path'; import fs from 'fs/promises'; async cleanupTempFile(filePath) { try { const tempRoot = await fs.realpath(this.config.tempDir); const candidate = path.resolve(filePath); const parent = await fs.realpath(path.dirname(candidate)); const resolvedTarget = path.join(parent, path.basename(candidate)); const relative = path.relative(tempRoot, resolvedTarget); if ( relative === '' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative) ) { throw new Error('Refusing to delete a file outside the temporary directory'); } if (!this.createdTempFiles?.has(resolvedTarget)) { throw new Error('Refusing to delete an untracked temporary file'); } await fs.unlink(resolvedTarget); this.createdTempFiles.delete(resolvedTarget); return true; } catch (error) { console.warn(`Temporary-file cleanup failed: ${error.message}`); return false; } } ``` For stronger symbolic-link protection, create and manage a dedicated private temporary directory with restrictive permissions and avoid following attacker-controlled links. The integration la ...[truncated 71 chars]