diff --git a/core/src/artifacts/file_artifact_service.ts b/core/src/artifacts/file_artifact_service.ts index 2d5b7c496..3759baffb 100644 --- a/core/src/artifacts/file_artifact_service.ts +++ b/core/src/artifacts/file_artifact_service.ts @@ -9,6 +9,7 @@ import * as fs from 'fs/promises'; import * as path from 'path'; import {fileURLToPath, pathToFileURL} from 'url'; +import {isInsideDir} from '../utils/file_utils.js'; import {logger} from '../utils/logger.js'; import { @@ -423,9 +424,9 @@ export function assertInsideRoot( rootDir: string, label: string, ): void { - const root = path.resolve(rootDir); - const resolved = path.resolve(resolvedPath); - if (!resolved.startsWith(root + path.sep) && resolved !== root) { + if (!isInsideDir(resolvedPath, rootDir)) { + const resolved = path.resolve(resolvedPath); + const root = path.resolve(rootDir); throw new Error( `[FileArtifactService] ${label} escapes storage root. Resolved: ${resolved}, Root: ${root}`, ); @@ -503,12 +504,12 @@ function getArtifactDir( throw new Error(`Absolute artifact filename ${filename} is not permitted.`); } + const resolvedScopeRoot = path.resolve(scopeRoot); const artifactDir = path.resolve(scopeRoot, cleanFilename); - const relative = path.relative(scopeRoot, artifactDir); - if (relative.startsWith('..') || path.isAbsolute(relative)) { + if (!isInsideDir(artifactDir, resolvedScopeRoot)) { throw new Error(`Artifact filename ${filename} escapes storage directory.`); } - if (relative === '' || relative === '.') { + if (artifactDir === resolvedScopeRoot) { return path.join(scopeRoot, 'artifact'); } diff --git a/core/src/utils/file_utils.ts b/core/src/utils/file_utils.ts index d71330482..03b07f6fd 100644 --- a/core/src/utils/file_utils.ts +++ b/core/src/utils/file_utils.ts @@ -9,19 +9,23 @@ import * as path from 'node:path'; import {File} from '../code_executors/code_execution_utils.js'; /** - * Reports whether resolvedPath is resolvedBaseDir itself, or a path nested - * inside it. + * Reports whether `targetPath` is `baseDir` itself, or a path nested inside it. * - * A plain `resolvedPath.startsWith(resolvedBaseDir)` check is a path-separator- - * unaware prefix match: it also accepts sibling directories whose name merely - * starts with the same string, e.g. base dir `/tmp/agent` wrongly "contains" - * `/tmp/agent-evil/x`. Requiring the trailing separator (or exact equality) - * closes that gap. + * Both arguments are resolved with `path.resolve` before comparison, and + * containment requires a path-separator boundary (or exact equality). A plain + * `startsWith` prefix match is separator-unaware: it also accepts a sibling + * whose name merely starts with the same string, e.g. base dir `/tmp/agent` + * wrongly "containing" `/tmp/agent-evil/x`. + * + * Lexical comparison only: it is case-sensitive on every platform and says + * nothing about symlinks or a TOCTOU race with the following filesystem call. */ -function isInsideDir(resolvedPath: string, resolvedBaseDir: string): boolean { +export function isInsideDir(targetPath: string, baseDir: string): boolean { + const resolvedBase = path.resolve(baseDir); + const resolvedTarget = path.resolve(targetPath); return ( - resolvedPath === resolvedBaseDir || - resolvedPath.startsWith(resolvedBaseDir + path.sep) + resolvedTarget === resolvedBase || + resolvedTarget.startsWith(resolvedBase + path.sep) ); } @@ -33,12 +37,11 @@ export async function materializeFiles( files: File[], dir = process.cwd(), ): Promise { - const resolvedBaseDir = path.resolve(dir); const createdFiles: File[] = []; for (const file of files) { const fullPath = path.resolve(dir, file.name); - if (!isInsideDir(fullPath, resolvedBaseDir)) { + if (!isInsideDir(fullPath, dir)) { throw new Error( `Path traversal detected: ${file.name} resolves outside of ${dir}`, ); @@ -68,7 +71,7 @@ export async function materializeFiles( } } - if (!isInsideDir(finalPath, resolvedBaseDir)) { + if (!isInsideDir(finalPath, dir)) { throw new Error( `Path traversal detected: ${file.name} resolves outside of ${dir}`, ); diff --git a/core/test/artifacts/file_artifact_service_test.ts b/core/test/artifacts/file_artifact_service_test.ts index 0356b0edd..bd5ff92c0 100644 --- a/core/test/artifacts/file_artifact_service_test.ts +++ b/core/test/artifacts/file_artifact_service_test.ts @@ -102,6 +102,89 @@ describe('FileArtifactService', () => { } }); + it('accepts a filename that starts with two dots', async () => { + rootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'adk-artifacts-test-')); + const service = new FileArtifactService(rootDir); + const appName = 'test-app'; + const userId = 'test-user'; + const sessionId = 'test-session'; + + try { + await service.saveArtifact({ + appName, + userId, + sessionId, + filename: '..foo.txt', + artifact: {text: 'hello'}, + }); + + const versionDir = path.join( + getSessionArtifactsDir(getUserRoot(rootDir, userId), sessionId), + '..foo.txt', + 'versions', + '0', + ); + await expect(fs.access(versionDir)).resolves.toBeUndefined(); + + const loaded = await service.loadArtifact({ + appName, + userId, + sessionId, + filename: '..foo.txt', + }); + expect(loaded?.text).toBe('hello'); + } finally { + await fs.rm(rootDir, {recursive: true, force: true}); + } + }); + + it("still rejects a bare '..' filename", async () => { + rootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'adk-artifacts-test-')); + const service = new FileArtifactService(rootDir); + + try { + await expect( + service.saveArtifact({ + appName: 'test-app', + userId: 'test-user', + sessionId: 'test-session', + filename: '..', + artifact: {text: 'dangerous'}, + }), + ).rejects.toThrow('escapes storage directory'); + } finally { + await fs.rm(rootDir, {recursive: true, force: true}); + } + }); + + it("stores a '.' filename under the artifact sentinel directory", async () => { + rootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'adk-artifacts-test-')); + const service = new FileArtifactService(rootDir); + const appName = 'test-app'; + const userId = 'test-user'; + const sessionId = 'test-session'; + + try { + await service.saveArtifact({ + appName, + userId, + sessionId, + filename: '.', + artifact: {text: 'sentinel'}, + }); + + const versionDir = path.join( + getSessionArtifactsDir(getUserRoot(rootDir, userId), sessionId), + 'artifact', + 'versions', + '0', + ); + await expect(fs.access(versionDir)).resolves.toBeUndefined(); + } finally { + await fs.rm(rootDir, {recursive: true, force: true}); + } + }); + const ROOT = '/tmp/adk-test-root'; describe('assertSafeSegment - valid inputs', () => { @@ -183,6 +266,11 @@ describe('FileArtifactService', () => { assertInsideRoot('/tmp/root/users/alice', '/tmp/root', 'test'), ).not.toThrow(); }); + it('rejects a sibling sharing a name prefix with root', () => { + expect(() => + assertInsideRoot('/tmp/root-evil/x', '/tmp/root', 'test'), + ).toThrow('escapes storage root'); + }); }); }); }); diff --git a/core/test/utils/file_utils_test.ts b/core/test/utils/file_utils_test.ts index 75b6178fc..69eb3a8bb 100644 --- a/core/test/utils/file_utils_test.ts +++ b/core/test/utils/file_utils_test.ts @@ -9,7 +9,7 @@ import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import {afterEach, beforeEach, describe, expect, it} from 'vitest'; -import {materializeFiles} from '../../src/utils/file_utils.js'; +import {isInsideDir, materializeFiles} from '../../src/utils/file_utils.js'; describe('file_utils', () => { let tempDir: string; @@ -182,4 +182,37 @@ describe('file_utils', () => { expect(content3).toBe('third'); }); }); + + describe('isInsideDir', () => { + const baseDir = path.resolve(path.sep, 'tmp', 'agent'); + + it('accepts the base directory itself', () => { + expect(isInsideDir(baseDir, baseDir)).toBe(true); + }); + + it('accepts a path nested inside the base directory', () => { + expect(isInsideDir(path.join(baseDir, 'sub', 'file.txt'), baseDir)).toBe( + true, + ); + }); + + it('rejects a sibling whose name shares the base directory prefix', () => { + expect(isInsideDir(`${baseDir}-evil${path.sep}x`, baseDir)).toBe(false); + }); + + it('rejects the parent of the base directory', () => { + expect(isInsideDir(path.dirname(baseDir), baseDir)).toBe(false); + }); + + it('accepts a filename starting with two dots and no separator', () => { + expect(isInsideDir(path.join(baseDir, '..foo.txt'), baseDir)).toBe(true); + }); + + it('resolves unnormalized arguments before comparing', () => { + const unnormalizedTarget = `${baseDir}${path.sep}sub${path.sep}..${path.sep}in.txt`; + expect(isInsideDir(unnormalizedTarget, `${baseDir}${path.sep}`)).toBe( + true, + ); + }); + }); });