diff --git a/.changeset/glob-absolute-outside-paths.md b/.changeset/glob-absolute-outside-paths.md new file mode 100644 index 00000000..650aabaf --- /dev/null +++ b/.changeset/glob-absolute-outside-paths.md @@ -0,0 +1,6 @@ +--- +"@moonshot-ai/agent-core": patch +"@moonshot-ai/kimi-code": patch +--- + +Allow glob searches to target explicit absolute paths outside the workspace. diff --git a/packages/agent-core/src/tools/builtin/file/glob.ts b/packages/agent-core/src/tools/builtin/file/glob.ts index 6b0da554..690f56a8 100644 --- a/packages/agent-core/src/tools/builtin/file/glob.ts +++ b/packages/agent-core/src/tools/builtin/file/glob.ts @@ -5,8 +5,8 @@ * time (most recent first). Uses `kaos.glob`. * * Output convention: `content` shown to the LLM is relativized to the - * search base to save tokens; `output.paths` keeps absolute paths so - * downstream Read/Edit can consume them directly. + * search base only when the base is inside the primary workspace. External + * roots stay absolute so downstream Read/Edit target the same file. * * Behaviour: * - Brace expansion (`*.{ts,tsx}`, `{src,test}/**`) is expanded at @@ -15,8 +15,9 @@ * fan-out has to happen here for any results to come back. Cartesian * and one level of nesting are supported; unbalanced or comma-less * braces fall through as literals. - * - `path` is validated by `resolvePathAccess` in strict mode. Explicit - * paths must be absolute and within the workspace roots. + * - `path` is validated by `resolvePathAccess` in `absolute-outside-allowed` + * mode. Explicit absolute paths outside the workspace are allowed; relative + * paths that escape the workspace stay rejected. * - Match count is capped at `MAX_MATCHES` (unique paths). A separate * `YIELD_SAFETY_CAP` on the raw yield stream is a secondary belt that * still terminates the stream if the kaos layer's own symlink-cycle @@ -37,7 +38,7 @@ import { z } from 'zod'; import type { BuiltinTool } from '../../../agent/tool'; import { ToolAccesses } from '../../../loop/tool-access'; import type { ExecutableToolResult, ToolExecution } from '../../../loop/types'; -import { resolvePathAccessPath } from '../../policies/path-access'; +import { isWithinDirectory, resolvePathAccessPath } from '../../policies/path-access'; import type { PathClass } from '../../policies/path-access'; import { toInputJsonSchema } from '../../support/input-schema'; import { literalRulePattern, matchesGlobRuleSubject } from '../../support/rule-match'; @@ -123,7 +124,7 @@ export class GlobTool implements BuiltinTool { kaos: this.kaos, workspace: this.workspace, operation: 'search', - policy: { guardMode: 'strict', checkSensitive: false }, + policy: { guardMode: 'absolute-outside-allowed', checkSensitive: false }, }); } const searchRoots = [path ?? this.workspace.workspaceDir]; @@ -258,11 +259,15 @@ export class GlobTool implements BuiltinTool { const paths = entries.map((e) => e.path); // Content shown to the LLM uses paths relative to the search base - // to save tokens; `output.paths` keeps the absolute form so callers - // can feed them into Read/Edit without further resolution. + // to save tokens, but only for the primary workspace. Relative paths + // are later resolved against workspaceDir, so additionalDir matches + // must stay absolute to keep follow-up Read/Edit calls on the same file. const pathClass = this.kaos.pathClass(); const relBase = searchRoots[0] ?? this.workspace.workspaceDir; - const displayLines = paths.map((p) => relativizeIfUnder(p, relBase, pathClass)); + const shouldRelativize = isWithinDirectory(relBase, this.workspace.workspaceDir, pathClass); + const displayLines = paths.map((p) => + shouldRelativize ? relativizeIfUnder(p, relBase, pathClass) : p, + ); if (entries.length === 0 && !truncated) { return { output: 'No matches found' }; diff --git a/packages/agent-core/src/tools/policies/path-access.ts b/packages/agent-core/src/tools/policies/path-access.ts index 947df726..d930cb81 100644 --- a/packages/agent-core/src/tools/policies/path-access.ts +++ b/packages/agent-core/src/tools/policies/path-access.ts @@ -22,18 +22,13 @@ import { isSensitiveFile } from './sensitive'; export type PathClass = 'posix' | 'win32'; export type PathSecurityCode = 'PATH_OUTSIDE_WORKSPACE' | 'PATH_SENSITIVE' | 'PATH_INVALID'; export type PathAccessOperation = 'read' | 'write' | 'search'; -export type WorkspaceGuardMode = 'strict' | 'absolute-outside-allowed' | 'disabled'; +export type WorkspaceGuardMode = 'absolute-outside-allowed' | 'disabled'; export interface WorkspaceAccessPolicy { readonly guardMode: WorkspaceGuardMode; readonly checkSensitive: boolean; } -export const STRICT_WORKSPACE_ACCESS_POLICY: WorkspaceAccessPolicy = { - guardMode: 'strict', - checkSensitive: true, -}; - export const DEFAULT_WORKSPACE_ACCESS_POLICY: WorkspaceAccessPolicy = { guardMode: 'absolute-outside-allowed', checkSensitive: true, @@ -190,21 +185,6 @@ export interface ResolvePathAccessPathOptions { readonly expandHome?: boolean; } -function outsideWorkspaceMessage( - path: string, - canonical: string, - config: WorkspaceConfig, - operation: PathAccessOperation, -): string { - const allowed = [config.workspaceDir, ...config.additionalDirs].join(', '); - const verb = operation === 'write' ? 'written' : operation === 'search' ? 'searched' : 'read'; - return ( - `"${path}" (canonical: "${canonical}") is outside the workspace ` + - `and outside the working directory "${config.workspaceDir}". ` + - `Cannot be ${verb}. Allowed roots: ${allowed}` - ); -} - function relativeOutsideMessage(path: string, operation: PathAccessOperation): string { const verb = operation === 'write' @@ -242,29 +222,8 @@ export function resolvePathAccess( ); } - // Strict mode requires the input itself to be absolute, even if it - // would canonicalize to a path inside the workspace. The python Glob - // contract is "directory must be an absolute path"; resolving a - // relative argument against the workspace cwd silently re-targets the - // search and is rejected outright in that contract. - if (policy.guardMode === 'strict' && !rawIsAbsolute) { - throw new PathSecurityError( - 'PATH_OUTSIDE_WORKSPACE', - path, - canonical, - relativeOutsideMessage(path, options.operation), - ); - } - if (outsideWorkspace) { switch (policy.guardMode) { - case 'strict': - throw new PathSecurityError( - 'PATH_OUTSIDE_WORKSPACE', - path, - canonical, - outsideWorkspaceMessage(path, canonical, config, options.operation), - ); case 'absolute-outside-allowed': if (!rawIsAbsolute) { throw new PathSecurityError( @@ -297,9 +256,9 @@ export function resolvePathAccessPath( } /** - * Throw `PathSecurityError` if `path` is outside the workspace, a known - * sensitive file, or an empty string. Returns the canonical absolute path - * when the check passes. + * Throw `PathSecurityError` if `path` escapes the workspace through a relative + * path, matches a known sensitive file, or is empty. Returns the canonical + * absolute path when the check passes. * * Note: this is purely lexical. It does NOT protect against symlink * targets that point outside the workspace — that would require kaos-layer @@ -315,8 +274,8 @@ export function assertPathAllowed( operation: options.mode, pathClass: options.pathClass, policy: { - guardMode: 'strict', - checkSensitive: options.checkSensitive ?? STRICT_WORKSPACE_ACCESS_POLICY.checkSensitive, + guardMode: 'absolute-outside-allowed', + checkSensitive: options.checkSensitive ?? DEFAULT_WORKSPACE_ACCESS_POLICY.checkSensitive, }, }).path; } diff --git a/packages/agent-core/test/tools/glob.test.ts b/packages/agent-core/test/tools/glob.test.ts index 6879d396..8fc23791 100644 --- a/packages/agent-core/test/tools/glob.test.ts +++ b/packages/agent-core/test/tools/glob.test.ts @@ -170,7 +170,7 @@ describe('GlobTool', () => { const result = await executeTool(tool, context({ pattern: 'pkg/**/*.ts', path: '/extra' })); - expect(result.output).toBe('pkg/a.ts'); + expect(result.output).toBe('/extra/pkg/a.ts'); expect(glob).toHaveBeenCalledTimes(1); expect(glob).toHaveBeenCalledWith('/extra', 'pkg/**/*.ts'); }); @@ -229,8 +229,8 @@ describe('GlobTool', () => { const result = await executeTool(tool, context({ pattern: '*.py', path: '/skills' })); - expect(result.output).toContain('read_content.py'); - expect(result.output).toContain('utils.py'); + expect(result.output).toContain('/skills/read_content.py'); + expect(result.output).toContain('/skills/utils.py'); expect(glob).toHaveBeenCalledWith('/skills', '*.py'); }); @@ -247,7 +247,7 @@ describe('GlobTool', () => { context({ pattern: '*.py', path: '/skills/feishu/scripts' }), ); - expect(result.output).toContain('read_content.py'); + expect(result.output).toContain('/skills/feishu/scripts/read_content.py'); }); it('rejects a relative path that escapes both workspace and additionalDirs', async () => { @@ -277,7 +277,7 @@ describe('GlobTool', () => { context({ pattern: '*.py', path: '/skills/my-skill/scripts' }), ); - expect(result.output).toContain('helper.py'); + expect(result.output).toContain('/skills/my-skill/scripts/helper.py'); }); }); @@ -472,37 +472,43 @@ describe('GlobTool', () => { expect(result.output).toContain('.github/workflows/ci.yml'); }); - it('picks up a freshly appended additionalDir without rebuilding the tool', async () => { - // py rejects `/extra` before it is registered in additional_dirs, then - // allows it after a runtime append. TS Glob runs with the - // `absolute-outside-allowed` policy so the first call is NOT rejected. - // Divergence lockdown — captures the cost of TS's looser default. - const additionalDirs: string[] = []; - const mutable: WorkspaceConfig = { workspaceDir: '/workspace', additionalDirs }; + it('shows absolute paths when explicit search root is outside all workspace roots', async () => { + // When the search root is not inside workspaceDir or any additionalDir, + // matches must stay absolute in the output. Otherwise the model would + // resolve a relativized path against the workspace cwd and hit the + // wrong file. const glob = vi.fn((root: string) => asyncPaths(root === '/extra' ? ['/extra/test.py'] : []), ); const tool = new GlobTool( createFakeKaos({ glob, stat: vi.fn().mockResolvedValue(stat(1)) }), - mutable, + { workspaceDir: '/workspace', additionalDirs: [] }, ); - const before = await executeTool(tool, context({ pattern: '*.py', path: '/extra' })); - expect(before).toMatchObject({ isError: true }); - expect(before.output).toContain('outside the working directory'); + const result = await executeTool(tool, context({ pattern: '*.py', path: '/extra' })); + expect(result.isError).toBeFalsy(); + expect(result.output).toBe('/extra/test.py'); + }); - additionalDirs.push('/extra'); + it('keeps absolute paths when explicit search root is an additionalDir', async () => { + // AdditionalDirs are searchable, but model-visible relative paths still + // resolve against workspaceDir in follow-up Read/Edit calls. + const registered: WorkspaceConfig = { workspaceDir: '/workspace', additionalDirs: ['/extra'] }; + const glob = vi.fn((root: string) => + asyncPaths(root === '/extra' ? ['/extra/test.py'] : []), + ); + const tool = new GlobTool( + createFakeKaos({ glob, stat: vi.fn().mockResolvedValue(stat(1)) }), + registered, + ); - const after = await executeTool(tool, context({ pattern: '*.py', path: '/extra' })); - expect(after.isError).toBeFalsy(); - expect(after.output).toContain('test.py'); + const result = await executeTool(tool, context({ pattern: '*.py', path: '/extra' })); + expect(result.isError).toBeFalsy(); + expect(result.output).toBe('/extra/test.py'); }); - it('rejects a relative path argument before resolving it against any cwd', async () => { - // py rejects any relative `directory` outright with "not an absolute - // path". TS currently joins relative paths onto the workspace cwd and - // proceeds — divergence lockdown. - const glob = vi.fn().mockReturnValue(asyncPaths([])); + it('allows a relative path argument that resolves inside the workspace', async () => { + const glob = vi.fn().mockReturnValue(asyncPaths(['/workspace/relative/path/test.py'])); const tool = new GlobTool( createFakeKaos({ glob, stat: vi.fn().mockResolvedValue(stat(1)) }), workspace, @@ -510,19 +516,12 @@ describe('GlobTool', () => { const result = await executeTool(tool, context({ pattern: '*.py', path: 'relative/path' })); - expect(result).toMatchObject({ isError: true }); - expect(result.output).toContain('not an absolute path'); - }); - - it('expands a leading "~/" path before applying the workspace guard', async () => { - // py: `~/` is expanded to the home dir, which is outside the - // workspace; the guard then rejects with "outside the workspace". - // The key invariant under test is that tilde expansion happens BEFORE - // the absolute-path check — otherwise the user would see the misleading - // "not an absolute path" error. TS currently runs Glob with - // `absolute-outside-allowed` so the workspace check does NOT reject - // outside paths once tilde expansion makes them absolute — divergence - // lockdown. + expect(result.isError).toBeFalsy(); + expect(result.output).toContain('test.py'); + expect(glob).toHaveBeenCalledWith('/workspace/relative/path', '*.py'); + }); + + it('expands a leading "~/" path before searching outside the workspace', async () => { const glob = vi.fn().mockReturnValue(asyncPaths([])); const tool = new GlobTool( createFakeKaos({ glob, gethome: () => '/home/test', stat: vi.fn().mockResolvedValue(stat(1)) }), @@ -531,15 +530,12 @@ describe('GlobTool', () => { const result = await executeTool(tool, context({ pattern: '*.py', path: '~/' })); - expect(result).toMatchObject({ isError: true }); - expect(result.output).toContain('outside the workspace'); - expect(result.output).not.toContain('not an absolute path'); + expect(result.isError).toBeFalsy(); + expect(result.output).toBe('No matches found'); + expect(glob).toHaveBeenCalledWith('/home/test', '*.py'); }); - it('rejects a path sharing the workspace prefix but outside it', async () => { - // py rejects shared-prefix outside paths with "outside the workspace". - // TS Glob uses `absolute-outside-allowed`, so an absolute path outside - // the workspace is accepted by design. Divergence lockdown. + it('allows a path sharing the workspace prefix when it is absolute', async () => { const glob = vi.fn().mockReturnValue(asyncPaths([])); const tool = new GlobTool( createFakeKaos({ glob, stat: vi.fn().mockResolvedValue(stat(1)) }), @@ -550,8 +546,9 @@ describe('GlobTool', () => { context({ pattern: '*.py', path: '/parent/workdir-sneaky' }), ); - expect(result).toMatchObject({ isError: true }); - expect(result.output).toMatch(/outside the workspace|outside the working directory/); + expect(result.isError).toBeFalsy(); + expect(result.output).toBe('No matches found'); + expect(glob).toHaveBeenCalledWith('/parent/workdir-sneaky', '*.py'); }); it('locks down brace-expansion mention and large-directory caveats in the description', () => { @@ -621,4 +618,3 @@ describe('expandBraces', () => { expect(expandBraces(pathological)).toEqual([pathological]); }); }); - diff --git a/packages/agent-core/test/tools/path-guard.test.ts b/packages/agent-core/test/tools/path-guard.test.ts index 5332fafa..b28be682 100644 --- a/packages/agent-core/test/tools/path-guard.test.ts +++ b/packages/agent-core/test/tools/path-guard.test.ts @@ -7,7 +7,6 @@ import { isWithinWorkspace, normalizeUserPath, PathSecurityError, - STRICT_WORKSPACE_ACCESS_POLICY, assertPathAllowed, resolvePathAccess, resolvePathAccessPath, @@ -31,15 +30,6 @@ const POSIX_KAOS = { }; describe('path access policy', () => { - it('strict policy rejects absolute paths outside workspace roots', () => { - expect(() => - resolvePathAccess('/etc/hosts', '/workspace', WORKSPACE, { - operation: 'read', - policy: STRICT_WORKSPACE_ACCESS_POLICY, - }), - ).toThrow(PathSecurityError); - }); - it('default policy allows absolute paths outside workspace roots', () => { const result = resolvePathAccess('/etc/hosts', '/workspace', WORKSPACE, { operation: 'read', @@ -147,12 +137,18 @@ describe('path access policy', () => { ).toBe('/workspace/~/notes/today.txt'); }); - it('legacy assertPathAllowed keeps strict shared-prefix behavior', () => { - expect(() => + it('legacy assertPathAllowed allows absolute outside paths but rejects relative escapes', () => { + expect( assertPathAllowed('/workspace-evil/secrets.txt', '/workspace', WORKSPACE, { mode: 'read', }), - ).toThrow(/outside the workspace/); + ).toBe('/workspace-evil/secrets.txt'); + + expect(() => + assertPathAllowed('../../outside.txt', '/workspace/project', WORKSPACE, { + mode: 'read', + }), + ).toThrow(/absolute path/); }); it('canonicalizes paths with an explicit posix path class', () => { @@ -278,18 +274,18 @@ describe('path access policy', () => { expect(isWithinWorkspace('/elsewhere/file', multi)).toBe(false); }); - it('rejects a shared-prefix attack against an additionalDir entry', () => { + it('does not classify shared-prefix paths as additionalDir entries', () => { const cfg: WorkspaceConfig = { workspaceDir: '/workspace', additionalDirs: ['/lib'], }; expect(isWithinWorkspace('/lib-evil/hack.py', cfg)).toBe(false); - expect(() => - resolvePathAccess('/lib-evil/hack.py', '/workspace', cfg, { - operation: 'read', - policy: STRICT_WORKSPACE_ACCESS_POLICY, - }), - ).toThrow(PathSecurityError); + + const result = resolvePathAccess('/lib-evil/hack.py', '/workspace', cfg, { + operation: 'read', + policy: DEFAULT_WORKSPACE_ACCESS_POLICY, + }); + expect(result).toEqual({ path: '/lib-evil/hack.py', outsideWorkspace: true }); }); it('uses path-segment containment rather than naive startsWith for additionalDir entries', () => {