Fix: consolidate path containment into one shared helper and stop over-rejecting '..'-prefixed artifact filenames

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');
   }
 

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<File[]> {
-  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}`,
       );

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');
+      });
     });
   });
 });

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,
+      );
+    });
+  });
 });