From 2136598e87731ecbae44d7e2a7f923f00343ba28 Mon Sep 17 00:00:00 2001 From: cornmander Date: Mon, 10 Nov 2025 13:48:44 -0500 Subject: [PATCH] Harden modifiable tool temp workspace (#12837) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- .../core/src/tools/modifiable-tool.test.ts | 71 ++++++++++--------- packages/core/src/tools/modifiable-tool.ts | 45 +++++++++--- 2 files changed, 72 insertions(+), 44 deletions(-) diff --git a/packages/core/src/tools/modifiable-tool.test.ts b/packages/core/src/tools/modifiable-tool.test.ts index 1290fc7fc9..0ed23d610c 100644 --- a/packages/core/src/tools/modifiable-tool.test.ts +++ b/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); }); }); diff --git a/packages/core/src/tools/modifiable-tool.ts b/packages/core/src/tools/modifiable-tool.ts index 0a2409e1c4..0857d86884 100644 --- a/packages/core/src/tools/modifiable-tool.ts +++ b/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( @@ -125,7 +138,11 @@ function getUpdatedParams( 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( 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( return result; } finally { - deleteTempFiles(oldPath, newPath); + deleteTempFiles(oldPath, newPath, dirPath); } }