Harden modifiable tool temp workspace (#12837)

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

packages/core/src/tools/modifiable-tool.test.ts

@@ -85,11 +85,19 @@ describe('modifyWithEditor', () => {
   afterEach(async () => {
     vi.restoreAllMocks();
     await fsp.rm(testProjectDir, { recursive: true, force: true });
-    const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
-    await fsp.rm(diffDir, { recursive: true, force: true });
   });
 
   describe('successful modification', () => {
+    const assertMode = (mode: number, expected: number): void => {
+      if (process.platform === 'win32') {
+        // Windows reports POSIX modes as 0o666 regardless of requested bits.
+        // At minimum confirm the owner read/write bits are present.
+        expect(mode & 0o600).toBe(0o600);
+        return;
+      }
+      expect(mode & 0o777).toBe(expected);
+    };
+
     it('should successfully modify content with VSCode editor', async () => {
       const result = await modifyWithEditor(
         mockParams,
@@ -142,9 +150,21 @@ describe('modifyWithEditor', () => {
       });
     });
 
-    it('should create temp directory if it does not exist', async () => {
-      const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
-      await fsp.rm(diffDir, { recursive: true, force: true }).catch(() => {});
+    it('should create temp directory and files with restrictive permissions', async () => {
+      mockOpenDiff.mockImplementation(async (oldPath, newPath) => {
+        const diffDir = path.dirname(oldPath);
+        expect(diffDir).toBe(path.dirname(newPath));
+
+        const dirStats = await fsp.stat(diffDir);
+        const oldStats = await fsp.stat(oldPath);
+        const newStats = await fsp.stat(newPath);
+
+        assertMode(dirStats.mode, 0o700);
+        assertMode(oldStats.mode, 0o600);
+        assertMode(newStats.mode, 0o600);
+
+        await fsp.writeFile(newPath, modifiedContent, 'utf8');
+      });
 
       await modifyWithEditor(
         mockParams,
@@ -153,27 +173,6 @@ describe('modifyWithEditor', () => {
         abortSignal,
         vi.fn(),
       );
-
-      const stats = await fsp.stat(diffDir);
-      expect(stats.isDirectory()).toBe(true);
-    });
-
-    it('should not create temp directory if it already exists', async () => {
-      const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
-      await fsp.mkdir(diffDir, { recursive: true });
-
-      const mkdirSpy = vi.spyOn(fs, 'mkdirSync');
-
-      await modifyWithEditor(
-        mockParams,
-        mockModifyContext,
-        'vscode' as EditorType,
-        abortSignal,
-        vi.fn(),
-      );
-
-      expect(mkdirSpy).not.toHaveBeenCalled();
-      mkdirSpy.mockRestore();
     });
   });
 
@@ -269,6 +268,9 @@ describe('modifyWithEditor', () => {
     vi.spyOn(fs, 'unlinkSync').mockImplementation(() => {
       throw new Error('Failed to delete file');
     });
+    vi.spyOn(fs, 'rmdirSync').mockImplementation(() => {
+      throw new Error('Failed to delete directory');
+    });
 
     await modifyWithEditor(
       mockParams,
@@ -278,10 +280,13 @@ describe('modifyWithEditor', () => {
       vi.fn(),
     );
 
-    expect(consoleErrorSpy).toHaveBeenCalledTimes(2);
+    expect(consoleErrorSpy).toHaveBeenCalledTimes(3);
     expect(consoleErrorSpy).toHaveBeenCalledWith(
       expect.stringContaining('Error deleting temp diff file:'),
     );
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      expect.stringContaining('Error deleting temp diff directory:'),
+    );
 
     consoleErrorSpy.mockRestore();
   });
@@ -307,9 +312,9 @@ describe('modifyWithEditor', () => {
     expect(oldFilePath).toMatch(/gemini-cli-modify-test-file-old-\d+\.txt$/);
     expect(newFilePath).toMatch(/gemini-cli-modify-test-file-new-\d+\.txt$/);
 
-    const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
-    expect(path.dirname(oldFilePath)).toBe(diffDir);
-    expect(path.dirname(newFilePath)).toBe(diffDir);
+    const diffDirPrefix = path.join(os.tmpdir(), 'gemini-cli-tool-modify-');
+    expect(path.dirname(oldFilePath).startsWith(diffDirPrefix)).toBe(true);
+    expect(path.dirname(newFilePath).startsWith(diffDirPrefix)).toBe(true);
   });
 
   it('should create temp files with correct naming without extension', async () => {
@@ -329,9 +334,9 @@ describe('modifyWithEditor', () => {
     expect(oldFilePath).toMatch(/gemini-cli-modify-test-file-old-\d+$/);
     expect(newFilePath).toMatch(/gemini-cli-modify-test-file-new-\d+$/);
 
-    const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
-    expect(path.dirname(oldFilePath)).toBe(diffDir);
-    expect(path.dirname(newFilePath)).toBe(diffDir);
+    const diffDirPrefix = path.join(os.tmpdir(), 'gemini-cli-tool-modify-');
+    expect(path.dirname(oldFilePath).startsWith(diffDirPrefix)).toBe(true);
+    expect(path.dirname(newFilePath).startsWith(diffDirPrefix)).toBe(true);
   });
 });
 

packages/core/src/tools/modifiable-tool.ts

@@ -59,12 +59,19 @@ function createTempFilesForModify(
   currentContent: string,
   proposedContent: string,
   file_path: string,
-): { oldPath: string; newPath: string } {
-  const tempDir = os.tmpdir();
-  const diffDir = path.join(tempDir, 'gemini-cli-tool-modify-diffs');
+): { oldPath: string; newPath: string; dirPath: string } {
+  const diffDir = fs.mkdtempSync(
+    path.join(os.tmpdir(), 'gemini-cli-tool-modify-'),
+  );
 
-  if (!fs.existsSync(diffDir)) {
-    fs.mkdirSync(diffDir, { recursive: true });
+  try {
+    fs.chmodSync(diffDir, 0o700);
+  } catch (e) {
+    debugLogger.error(
+      `Error setting permissions on temp diff directory: ${diffDir}`,
+      e,
+    );
+    throw e;
   }
 
   const ext = path.extname(file_path);
@@ -79,10 +86,16 @@ function createTempFilesForModify(
     `gemini-cli-modify-${fileName}-new-${timestamp}${ext}`,
   );
 
-  fs.writeFileSync(tempOldPath, currentContent, 'utf8');
-  fs.writeFileSync(tempNewPath, proposedContent, 'utf8');
+  fs.writeFileSync(tempOldPath, currentContent, {
+    encoding: 'utf8',
+    mode: 0o600,
+  });
+  fs.writeFileSync(tempNewPath, proposedContent, {
+    encoding: 'utf8',
+    mode: 0o600,
+  });
 
-  return { oldPath: tempOldPath, newPath: tempNewPath };
+  return { oldPath: tempOldPath, newPath: tempNewPath, dirPath: diffDir };
 }
 
 function getUpdatedParams<ToolParams>(
@@ -125,7 +138,11 @@ function getUpdatedParams<ToolParams>(
   return { updatedParams, updatedDiff };
 }
 
-function deleteTempFiles(oldPath: string, newPath: string): void {
+function deleteTempFiles(
+  oldPath: string,
+  newPath: string,
+  dirPath: string,
+): void {
   try {
     fs.unlinkSync(oldPath);
   } catch {
@@ -137,6 +154,12 @@ function deleteTempFiles(oldPath: string, newPath: string): void {
   } catch {
     debugLogger.error(`Error deleting temp diff file: ${newPath}`);
   }
+
+  try {
+    fs.rmdirSync(dirPath);
+  } catch {
+    debugLogger.error(`Error deleting temp diff directory: ${dirPath}`);
+  }
 }
 
 /**
@@ -154,7 +177,7 @@ export async function modifyWithEditor<ToolParams>(
   const proposedContent =
     await modifyContext.getProposedContent(originalParams);
 
-  const { oldPath, newPath } = createTempFilesForModify(
+  const { oldPath, newPath, dirPath } = createTempFilesForModify(
     currentContent,
     proposedContent,
     modifyContext.getFilePath(originalParams),
@@ -171,6 +194,6 @@ export async function modifyWithEditor<ToolParams>(
 
     return result;
   } finally {
-    deleteTempFiles(oldPath, newPath);
+    deleteTempFiles(oldPath, newPath, dirPath);
   }
 }