golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(#4653): make validatePath module-internal

Browse Source 
Phase 3 of epic #4636, stage 3a. ADR-4650 decision 2: the engine stops being a
public shape. The only exported containment surface is now assertWithinRoot /
tryWithinRoot / requireSafePath, none of which can hand a caller a usable path
when the answer is unsafe.

The src/security.cts diff is one keyword. The engine body is byte-identical —
the dangling-symlink existence-oracle closure, the ancestor canonicalization and
the separator-aware boundary test are untouched, which is the whole constraint
this phase operates under.

WHAT THE TRANSLATION COST, AND THE RULE THAT KEPT IT AT ZERO. Roughly thirty test
call sites consumed validatePath directly, including the two BLOCKER regressions
that are this refactor's safety net. Translating them all to
`tryWithinRoot(...) === null` would have looked correct and silently destroyed
one of them: BLOCKER-1 asserts the rejection reason contains "unresolvable
symbolic link", which is what distinguishes a DANGLING symlink from an ordinary
escape. tryWithinRoot returns a bare null and cannot tell those apart, so that
assertion would have degenerated into "it failed somehow" — and the
existence-oracle closure could regress with the test still green.

So the rule applied throughout is: an assertion on the rejection REASON goes
through assertWithinRoot, whose throw carries the engine's message verbatim; only
assertions on the boolean go through tryWithinRoot. Under that rule no coverage
is lost. BLOCKER-1 still pins "unresolvable symbolic link" and BLOCKER-2 still
pins the exact canonicalized resolved value.

Three success-path tests came out BETTER than they went in. They previously
carried `expected safe:true, got error: ${result.error}` as an assertion message;
routing them through assertWithinRoot means an engine regression now surfaces the
real reason in the failure itself rather than as a hand-built string.

The two describe blocks named after validatePath are renamed — a block named for
a symbol the module no longer exports is a false signpost.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
sim
2026-09-12 12:22:22 -04:00
parent 4ebcec7750
commit 26384ca988
3 changed files with 66 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
src/security.cts
View File

@@ -28,7 +28,7 @@ import path from 'node:path';
* Validate that a file path resolves within an allowed base directory.
* Prevents path traversal attacks via ../ sequences, symlinks, or absolute paths.
*/
export function validatePath(filePath: unknown, baseDir: unknown, opts: { allowAbsolute?: boolean } = {}): { safe: boolean; resolved: string; error?: string } {
function validatePath(filePath: unknown, baseDir: unknown, opts: { allowAbsolute?: boolean } = {}): { safe: boolean; resolved: string; error?: string } {
if (!filePath || typeof filePath !== 'string') {
return { safe: false, resolved: '', error: 'Empty or invalid file path' };
}

28
tests/agent-skills.test.cjs
View File

@@ -35,7 +35,7 @@ function runGsdToolsWithStderr(args, cwd, env) {
};
}
const { loadTrustedGlobalRoots, validatePath } = require('../gsd-core/bin/lib/security.cjs');
const { loadTrustedGlobalRoots, tryWithinRoot } = require('../gsd-core/bin/lib/security.cjs');
// ─── helpers ──────────────────────────────────────────────────────────────────
@@ -890,7 +890,7 @@ describe('loadTrustedGlobalRoots', () => {
// ─── trusted_global_roots integration guard (#52) ─────────────────────────────
//
// NOTE: These tests validate the trusted-root bypass logic by directly calling
// loadTrustedGlobalRoots + validatePath rather than invoking the full CLI
// loadTrustedGlobalRoots + tryWithinRoot rather than invoking the full CLI
// (which would require controlling the runtime HOME path in a way that also
// triggers a symlink escape scenario through gsd-tools subprocess invocation).
// Full end-to-end symlink testing would require OS-level symlink setup in tmp
@@ -913,24 +913,24 @@ describe('trusted_global_roots guard logic', () => {
cleanup(externalDir);
});
test('validatePath rejects skill outside globalSkillsBase (baseline — no trusted roots)', () => {
test('tryWithinRoot rejects skill outside globalSkillsBase (baseline — no trusted roots)', () => {
const skillMd = path.join(externalDir, 'SKILL.md');
const result = validatePath(skillMd, tmpDir, { allowAbsolute: true });
assert.ok(!result.safe, 'skill outside base must be rejected by validatePath');
const result = tryWithinRoot(skillMd, tmpDir, { allowAbsolute: true });
assert.equal(result, null, 'skill outside base must be rejected by tryWithinRoot');
});
test('with trusted root matching real target dir — validatePath accepts', () => {
test('with trusted root matching real target dir — tryWithinRoot accepts', () => {
// Simulate the trusted-root fallback: skill is outside base but inside trusted root
const skillMd = path.join(externalDir, 'SKILL.md');
const baseCheck = validatePath(skillMd, tmpDir, { allowAbsolute: true });
assert.ok(!baseCheck.safe, 'base check must fail (prerequisite)');
const baseCheck = tryWithinRoot(skillMd, tmpDir, { allowAbsolute: true });
assert.equal(baseCheck, null, 'base check must fail (prerequisite)');
// Trusted root fallback: check against externalDir
const config = { agent_skills_security: { trusted_global_roots: [externalDir] } };
const trustedRoots = loadTrustedGlobalRoots(config);
const acceptedViaTrustedRoot = trustedRoots.some((root) => {
const rootCheck = validatePath(skillMd, root, { allowAbsolute: true });
return rootCheck.safe;
const rootCheck = tryWithinRoot(skillMd, root, { allowAbsolute: true });
return rootCheck !== null;
});
assert.ok(acceptedViaTrustedRoot, 'skill must be accepted when within a trusted root');
});
@@ -942,8 +942,8 @@ describe('trusted_global_roots guard logic', () => {
const config = { agent_skills_security: { trusted_global_roots: [unrelatedDir] } };
const trustedRoots = loadTrustedGlobalRoots(config);
const acceptedViaTrustedRoot = trustedRoots.some((root) => {
const rootCheck = validatePath(skillMd, root, { allowAbsolute: true });
return rootCheck.safe;
const rootCheck = tryWithinRoot(skillMd, root, { allowAbsolute: true });
return rootCheck !== null;
});
assert.ok(!acceptedViaTrustedRoot, 'skill must still be rejected when trusted root is unrelated');
} finally {
@@ -957,8 +957,8 @@ describe('trusted_global_roots guard logic', () => {
const trustedRoots = loadTrustedGlobalRoots(config);
assert.strictEqual(trustedRoots.length, 0, 'no roots loaded');
const acceptedViaTrustedRoot = trustedRoots.some((root) => {
const rootCheck = validatePath(skillMd, root, { allowAbsolute: true });
return rootCheck.safe;
const rootCheck = tryWithinRoot(skillMd, root, { allowAbsolute: true });
return rootCheck !== null;
});
assert.ok(!acceptedViaTrustedRoot, 'skill must be rejected when trusted roots is empty');
});

107
tests/security.test.cjs
View File

@@ -13,7 +13,6 @@ const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
const fc = require('./helpers/fast-check-setup.cjs');
const {
validatePath,
requireSafePath,
scanForInjection,
sanitizeForPrompt,
@@ -30,72 +29,71 @@ const {
// ─── Path Traversal Prevention ──────────────────────────────────────────────
describe('validatePath', () => {
describe('assertWithinRoot / tryWithinRoot — engine invariance', () => {
const base = '/projects/my-app';
test('allows relative paths within base', () => {
const result = validatePath('src/index.js', base);
assert.ok(result.safe);
assert.equal(result.resolved, path.resolve(base, 'src/index.js'));
const r = tryWithinRoot('src/index.js', base);
assert.ok(r !== null);
assert.equal(r, path.resolve(base, 'src/index.js'));
});
test('allows nested relative paths', () => {
const result = validatePath('.planning/phases/01-setup/PLAN.md', base);
assert.ok(result.safe);
const r = tryWithinRoot('.planning/phases/01-setup/PLAN.md', base);
assert.ok(r !== null);
});
test('rejects ../ traversal escaping base', () => {
const result = validatePath('../../etc/passwd', base);
assert.ok(!result.safe);
assert.ok(result.error.includes('escapes allowed directory'));
assert.throws(
() => assertWithinRoot('../../etc/passwd', base, 'test'),
/escapes allowed directory/,
);
});
test('rejects absolute paths by default', () => {
const result = validatePath('/etc/passwd', base);
assert.ok(!result.safe);
assert.ok(result.error.includes('Absolute paths not allowed'));
assert.throws(
() => assertWithinRoot('/etc/passwd', base, 'test'),
/Absolute paths not allowed/,
);
});
test('allows absolute paths within base when opted in', () => {
const result = validatePath(path.join(base, 'src/file.js'), base, { allowAbsolute: true });
assert.ok(result.safe);
const r = tryWithinRoot(path.join(base, 'src/file.js'), base, { allowAbsolute: true });
assert.ok(r !== null);
});
test('rejects absolute paths outside base even when opted in', () => {
const result = validatePath('/etc/passwd', base, { allowAbsolute: true });
assert.ok(!result.safe);
assert.equal(tryWithinRoot('/etc/passwd', base, { allowAbsolute: true }), null);
});
test('rejects null bytes', () => {
const result = validatePath('src/\0evil.js', base);
assert.ok(!result.safe);
assert.ok(result.error.includes('null bytes'));
assert.throws(
() => assertWithinRoot('src/\0evil.js', base, 'test'),
/null bytes/,
);
});
test('rejects empty path', () => {
const result = validatePath('', base);
assert.ok(!result.safe);
assert.equal(tryWithinRoot('', base), null);
});
test('rejects non-string path', () => {
const result = validatePath(42, base);
assert.ok(!result.safe);
assert.equal(tryWithinRoot(42, base), null);
});
test('handles . and ./ correctly (stays in base)', () => {
const result = validatePath('.', base);
assert.ok(result.safe);
assert.equal(result.resolved, path.resolve(base));
const r = tryWithinRoot('.', base);
assert.ok(r !== null);
assert.equal(r, path.resolve(base));
});
test('handles complex traversal like src/../../..', () => {
const result = validatePath('src/../../../etc/shadow', base);
assert.ok(!result.safe);
assert.equal(tryWithinRoot('src/../../../etc/shadow', base), null);
});
test('allows path that resolves back into base after ..', () => {
const result = validatePath('src/../lib/file.js', base);
assert.ok(result.safe);
const r = tryWithinRoot('src/../lib/file.js', base);
assert.ok(r !== null);
});
// ─── Dangling symlink + non-canonical base regression coverage ───────────
@@ -149,9 +147,8 @@ describe('validatePath', () => {
const linkPath = path.join(scratch, 'link.txt');
const ok = withSymlinkGuard(t, () => fs.symlinkSync(targetPath, linkPath, 'file'));
if (ok) {
const result = validatePath('link.txt', scratch);
assert.ok(result.safe, `expected safe:true, got error: ${result.error}`);
assert.equal(result.resolved, fs.realpathSync(targetPath));
const r = assertWithinRoot('link.txt', scratch, 'in-project symlink');
assert.equal(r, fs.realpathSync(targetPath));
}
} finally {
cleanup(scratch);
@@ -167,8 +164,7 @@ describe('validatePath', () => {
const linkPath = path.join(scratch, 'evil-link.txt');
const ok = withSymlinkGuard(t, () => fs.symlinkSync(outsideTarget, linkPath, 'file'));
if (ok) {
const result = validatePath('evil-link.txt', scratch);
assert.ok(!result.safe);
assert.equal(tryWithinRoot('evil-link.txt', scratch), null);
}
} finally {
cleanup(scratch);
@@ -183,11 +179,9 @@ describe('validatePath', () => {
const linkPath = path.join(scratch, 'dangling-link.txt');
const ok = withSymlinkGuard(t, () => fs.symlinkSync(nonExistentOutsideTarget, linkPath, 'file'));
if (ok) {
const result = validatePath('dangling-link.txt', scratch);
assert.ok(!result.safe, 'a dangling symlink must not be accepted as an in-project path');
assert.ok(
result.error && result.error.includes('unresolvable symbolic link'),
`expected the unresolvable-symlink error, got: ${result.error}`,
assert.throws(
() => assertWithinRoot('dangling-link.txt', scratch, 'test'),
/unresolvable symbolic link/,
);
}
} finally {
@@ -202,9 +196,8 @@ describe('validatePath', () => {
nc = makeNonCanonicalBase(scratch, t);
if (nc) {
fs.mkdirSync(path.join(nc.canonical, 'existingSub'));
const result = validatePath('existingSub/newfile.txt', nc.base);
assert.ok(result.safe, `expected safe:true, got error: ${result.error}`);
assert.equal(result.resolved, path.join(nc.canonical, 'existingSub', 'newfile.txt'));
const r = assertWithinRoot('existingSub/newfile.txt', nc.base, 'existing-subdir');
assert.equal(r, path.join(nc.canonical, 'existingSub', 'newfile.txt'));
}
} finally {
cleanup(scratch);
@@ -220,9 +213,8 @@ describe('validatePath', () => {
if (nc) {
// Neither 'sub1' nor 'sub1/sub2' exist — the immediate-parent-only
// fallback fails here, which is exactly the BLOCKER-2 scenario.
const result = validatePath('sub1/sub2/newfile.txt', nc.base);
assert.ok(result.safe, `expected safe:true, got error: ${result.error}`);
assert.equal(result.resolved, path.join(nc.canonical, 'sub1', 'sub2', 'newfile.txt'));
const r = assertWithinRoot('sub1/sub2/newfile.txt', nc.base, 'BLOCKER-2');
assert.equal(r, path.join(nc.canonical, 'sub1', 'sub2', 'newfile.txt'));
}
} finally {
cleanup(scratch);
@@ -239,8 +231,11 @@ describe('validatePath', () => {
// sub1/sub2 don't exist, and the .. segments escape not just the
// not-yet-created subdirs but the base itself — the ancestor walk-up
// must not turn this into an accepted path.
const result = validatePath('sub1/sub2/../../../escape.txt', nc.base);
assert.ok(!result.safe, 'escaping via .. through not-yet-created dirs must still be rejected');
assert.equal(
tryWithinRoot('sub1/sub2/../../../escape.txt', nc.base),
null,
'escaping via .. through not-yet-created dirs must still be rejected',
);
}
} finally {
cleanup(scratch);
@@ -1220,7 +1215,7 @@ describe('SECURE: ASVS level scaling (#1627)', () => {
// accepted. Both directions are required — a predicate rejecting everything
// would vacuously satisfy PR1 alone.
describe('validatePath — containment properties (#4652)', () => {
describe('containment properties (#4652)', () => {
// A path SEGMENT: letters/digits/dash/underscore, non-empty, never '.' or '..'
// by construction so every generated escaping path is escaping ONLY via the
// deliberately-injected `..` components below (never an accidental one).
@@ -1243,10 +1238,10 @@ describe('validatePath — containment properties (#4652)', () => {
const upCount = baseSegments.length + extraUp;
const traversal = path.join(...Array(upCount).fill('..'), ...tailSegments, 'target');
const result = validatePath(traversal, root);
const result = tryWithinRoot(traversal, root);
assert.strictEqual(
result.safe,
false,
result,
null,
`traversal ${JSON.stringify(traversal)} against root ${root} must be rejected, got: ${JSON.stringify(result)}`,
);
},
@@ -1260,13 +1255,13 @@ describe('validatePath — containment properties (#4652)', () => {
const root = path.resolve('/gsd-root-anchor-in');
const relPath = path.join(...segments);
const result = validatePath(relPath, root);
const result = tryWithinRoot(relPath, root);
assert.strictEqual(
result.safe,
result !== null,
true,
`in-root path ${JSON.stringify(relPath)} against root ${root} must be accepted, got: ${JSON.stringify(result)}`,
);
assert.strictEqual(result.resolved, path.resolve(root, relPath));
assert.strictEqual(result, path.resolve(root, relPath));
},
), { seed: 4652, numRuns: 200 });
});