mirror of
https://github.com/ruvnet/ruflo.git
synced 2026-10-23 20:07:01 +09:00
dream(security): hooks_codemod path-containment bypass (CWE-22)
resolveTargets()'s containment check (hooks-tools.ts) used an unanchored abs.startsWith(cwd) string-prefix comparison. An absolute path.resolve() result is returned unchanged for an already-absolute input, so a sibling directory whose name merely starts with the project directory's name (e.g. cwd=/home/user/ruflo, target=/home/user/ruflo-staging/notes.ts) passed containment and let this live, CLAUDE.md-recommended Tier-1 MCP tool read+write outside the project root. Anchors both call sites (file/files[] and glob-match branches) at a path-separator boundary, matching the already-correct pattern used in @claude-flow/security's isWithinPrefix(), @claude-flow/hooks/workers, and @claude-flow/plugins/security elsewhere in this codebase. 4 new tests (2 discriminating, real temp-dir fs I/O; 2 non-discriminating controls). Stash-isolated: both escape tests fail on baseline (file genuinely written outside root), 4/4 pass on candidate. Related suites (mcp-tools-deep, validate-input-*, codemod-engine/routing/corpus): 229/229 unchanged. tsc --noEmit byte-identical (9 pre-existing environmental errors, unbuilt sibling packages). Co-Authored-By: RuFlo <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_0171ELk4aJ9SPhudJw3UnjgL
This commit is contained in:
2 files changed
+108
-3
No files matched your search
@@ -0,0 +1,100 @@
|
||||
/**
|
||||
* Regression test (Dream Cycle 2026-10-20): `hooks_codemod`'s path-containment
|
||||
* check used an unanchored string-prefix comparison (`abs.startsWith(cwd)`)
|
||||
* instead of anchoring at a path-separator boundary. A sibling directory
|
||||
* whose name happens to start with the project directory's name (e.g.
|
||||
* `/project` vs. `/project-staging`) passed the check and let the tool
|
||||
* read-and-write a file outside the intended project root — CWE-22.
|
||||
*
|
||||
* Real filesystem I/O (temp dirs), no mocks — proves the actual read+write
|
||||
* escape, not just the string comparison in isolation.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync } from 'fs';
|
||||
import { tmpdir } from 'os';
|
||||
import { join } from 'path';
|
||||
import { hooksCodemod } from '../src/mcp-tools/hooks-tools.js';
|
||||
|
||||
describe('hooks_codemod path containment', () => {
|
||||
let root: string;
|
||||
let projectDir: string;
|
||||
let siblingDir: string;
|
||||
let prevCwd: string | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
root = mkdtempSync(join(tmpdir(), 'dream-codemod-'));
|
||||
// Sibling directory name shares "project" as a literal string prefix —
|
||||
// the exact shape of the bypass (cwd="<root>/project", escape target
|
||||
// lives under "<root>/project-staging", one directory up from cwd).
|
||||
projectDir = join(root, 'project');
|
||||
siblingDir = join(root, 'project-staging');
|
||||
mkdirSync(projectDir, { recursive: true });
|
||||
mkdirSync(siblingDir, { recursive: true });
|
||||
prevCwd = process.env.CLAUDE_FLOW_CWD;
|
||||
process.env.CLAUDE_FLOW_CWD = projectDir;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (prevCwd === undefined) delete process.env.CLAUDE_FLOW_CWD;
|
||||
else process.env.CLAUDE_FLOW_CWD = prevCwd;
|
||||
rmSync(root, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('rejects an absolute `file` path outside the project root that shares a string prefix with cwd', async () => {
|
||||
const target = join(siblingDir, 'evil.ts');
|
||||
writeFileSync(target, "console.log('leak');\n");
|
||||
|
||||
const result = (await hooksCodemod.handler({ intent: 'remove-console', file: target })) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
|
||||
expect(result.success).toBe(false);
|
||||
expect(String(result.error)).toMatch(/escapes project root/);
|
||||
// The file must be byte-identical — no write occurred outside the root.
|
||||
expect(readFileSync(target, 'utf-8')).toBe("console.log('leak');\n");
|
||||
});
|
||||
|
||||
it('rejects the same escape via the `files[]` array form', async () => {
|
||||
const target = join(siblingDir, 'evil2.ts');
|
||||
writeFileSync(target, "console.log('leak');\n");
|
||||
|
||||
const result = (await hooksCodemod.handler({ intent: 'remove-console', files: [target] })) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
|
||||
expect(result.success).toBe(false);
|
||||
expect(String(result.error)).toMatch(/escapes project root/);
|
||||
expect(readFileSync(target, 'utf-8')).toBe("console.log('leak');\n");
|
||||
});
|
||||
|
||||
it('still allows a legitimate absolute path inside the project root (non-discriminating control)', async () => {
|
||||
const target = join(projectDir, 'ok.ts');
|
||||
writeFileSync(target, "console.log('inside');\n");
|
||||
|
||||
const result = (await hooksCodemod.handler({ intent: 'remove-console', file: target })) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
const results = result.results as Array<Record<string, unknown>>;
|
||||
expect(results[0].written).toBe(true);
|
||||
expect(readFileSync(target, 'utf-8')).not.toMatch(/console\.log/);
|
||||
});
|
||||
|
||||
it('still allows a relative path inside the project root (non-discriminating control)', async () => {
|
||||
writeFileSync(join(projectDir, 'rel.ts'), "console.log('inside');\n");
|
||||
|
||||
const result = (await hooksCodemod.handler({ intent: 'remove-console', file: 'rel.ts' })) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
const results = result.results as Array<Record<string, unknown>>;
|
||||
expect(results[0].written).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -6,7 +6,7 @@ import { feedbackPatternsSchema, validateFeedbackPatterns } from '../memory/feed
|
||||
|
||||
import { mkdirSync, writeFileSync, existsSync, readFileSync, statSync, unlinkSync, readdirSync, rmSync } from 'fs';
|
||||
import * as nodeFs from 'fs';
|
||||
import { dirname, join, resolve } from 'path';
|
||||
import { dirname, join, resolve, sep } from 'path';
|
||||
import { type MCPTool, getProjectCwd } from './types.js';
|
||||
import { validateIdentifier, validateText, validatePath } from './validate-input.js';
|
||||
import { commandRecordingDisabled, redactCommandForStorage } from '../services/command-recording.js';
|
||||
@@ -5429,7 +5429,12 @@ export const hooksCodemod: MCPTool = {
|
||||
const v = validatePath(p, 'path');
|
||||
if (!v.valid) return v.error;
|
||||
const abs = resolve(cwd, v.sanitized);
|
||||
if (!abs.startsWith(cwd)) return `path escapes project root: ${p}`;
|
||||
// Anchor containment at a path-separator boundary — an unanchored
|
||||
// `abs.startsWith(cwd)` string-prefix check would let an absolute
|
||||
// path like `${cwd}-staging/x.ts` pass as "inside" `cwd`. Matches
|
||||
// the pattern already used correctly elsewhere in this codebase
|
||||
// (@claude-flow/security's `isWithinPrefix`, hooks/workers, plugins/security).
|
||||
if (abs !== cwd && !abs.startsWith(cwd + sep)) return `path escapes project root: ${p}`;
|
||||
out.add(abs);
|
||||
return undefined;
|
||||
};
|
||||
@@ -5451,7 +5456,7 @@ export const hooksCodemod: MCPTool = {
|
||||
}
|
||||
for (const m of matches) {
|
||||
const abs = resolve(cwd, m);
|
||||
if (abs.startsWith(cwd) && CODEMOD_EXTENSIONS.has(abs.slice(abs.lastIndexOf('.')).toLowerCase())) {
|
||||
if ((abs === cwd || abs.startsWith(cwd + sep)) && CODEMOD_EXTENSIONS.has(abs.slice(abs.lastIndexOf('.')).toLowerCase())) {
|
||||
out.add(abs);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user