diff --git a/src/security.cts b/src/security.cts index 0c696a2e2..84e9d9c87 100644 --- a/src/security.cts +++ b/src/security.cts @@ -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' }; } diff --git a/tests/agent-skills.test.cjs b/tests/agent-skills.test.cjs index bc0e77aa4..f1f935c02 100644 --- a/tests/agent-skills.test.cjs +++ b/tests/agent-skills.test.cjs @@ -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'); }); diff --git a/tests/security.test.cjs b/tests/security.test.cjs index 6433bacae..be8cfa297 100644 --- a/tests/security.test.cjs +++ b/tests/security.test.cjs @@ -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 }); });