T09 · Insecure Skill Coding Practices
Error
- Location
- core.mjs:137
- Finding
- Manifest path traversal allows filesystem moves and writes outside the repository root<![CDATA[ ## Vulnerability Details **File Location**: `core.mjs:137-196`, with additional arbitrary writes at `core.mjs:309-394` **Vulnerability Type**: Path traversal and insufficient filesystem boundary enforcement **Risk Level**: High ### Complete Code Snippet ```javascript export function planSync(manifestPath, reposRoot) { const manifest = loadManifest(manifestPath); const diskPaths = walkRepos(reposRoot); const moves = []; // Build a map of remote -> manifest path const remoteToManifest = new Map(); for (const [mPath, info] of Object.entries(manifest.repos)) { if (info.remote) { remoteToManifest.set(info.remote, mPath); } } // For each repo on disk, check if its remote matches a manifest entry at a different path for (const diskPath of diskPaths) { const fullPath = join(reposRoot, diskPath); const gitConfig = join(fullPath, '.git', 'config'); if (!existsSync(gitConfig)) continue; const configText = readFileSync(gitConfig, 'utf8'); const match = configText.match(/url\s*=\s*.*[:/]([^/]+\/[^/\s.]+?)(?:\.git)?\s*$/m); if (!match) continue; const remote = match[1]; const expectedPath = remoteToManifest.get(remote); if (expectedPath && expectedPath !== diskPath) { moves.push({ from: diskPath, to: expectedPath, remote, fromFull: fullPath, toFull: join(reposRoot, expectedPath), }); } } return moves; } export function executeSync(moves, reposRoot) { const results = []; for (const move of moves) { const parentDir = dirname(move.toFull); if (!existsSync(parentDir)) { mkdirSync(parentDir, { recursive: true }); } if (existsSync(move.toFull)) { results.push({ ...move, status: 'skipped', reason: 'target exists' }); continue; } try { renameSync(move.fromFull, move.toFull); results.push({ ...move, status: 'moved' }); } catch (err) { results.push({ ...move, status: 'error' ...[truncated 4448 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Validate every manifest repository key before using it: - Require a nonempty relative path. - Reject absolute paths. - Reject `.` and `..` components. - Reject null bytes and platform-specific drive or UNC paths. - Define an explicit allowlist for path characters if practical. 2. Resolve and enforce containment: ```javascript import { resolve, relative, isAbsolute } from 'node:path'; function resolveWithinRoot(root, candidate) { if (typeof candidate !== 'string' || candidate.length === 0 || isAbsolute(candidate)) { throw new Error('Repository path must be a nonempty relative path'); } const resolvedRoot = resolve(root); const resolvedCandidate = resolve(resolvedRoot, candidate); const rel = relative(resolvedRoot, resolvedCandidate); if (rel === '' || rel === '..' || rel.startsWith(`..${path.sep}`) || isAbsolute(rel)) { throw new Error(`Repository path escapes root: ${candidate}`); } return resolvedCandidate; } ``` 3. Account for symbolic links: - Resolve the real path of the repository root. - Resolve the real path of existing source paths and nearest existing destination parents. - Confirm their real paths remain under the root. - Consider rejecting symlinks in organizational directory components. 4. Change `executeSync()` to accept only logical relative moves: ```javascript executeSync([{ from, to }], reposRoot) ``` It should derive validated full paths internally. Do not trust caller-provided `fromFull` or `toFull`. 5. Apply the same containment function in: - `planSync`; - `executeSync`; - `checkCompliance`; - `fixCompliance`; - any future file-writing operation. 6. Require explicit confirmation before non-dry-run synchronization, particularly when invoked by an agent. 7. Add tests covering: - `../` and nested traversal; - absolute Unix and Windows paths; - UNC paths; - symbolic-link escapes; - external compliance targets; - d ...[truncated 47 chars]