From 6a99be718b1ae417aa3024c6aaa0cd49350f9d93 Mon Sep 17 00:00:00 2001 From: adilburaksen Date: Sat, 1 Aug 2026 18:30:59 +0300 Subject: [PATCH 1/2] refactor(core): create the unsafe executor's temp directory with mkdtemp createTempScriptFile built its directory name from Date.now() and Math.random(), then created it with fs.mkdir({recursive: true}), which does not fail on a path that already exists. A predictable name plus a non-exclusive create means a directory pre-created by another local user is adopted rather than rejected, and it keeps that user's permissions. fs.mkdtemp names the directory itself, creates it exclusively, and does so at mode 0o700. Dropping the fixed 'adk_js_unsafe_code_executor' parent in favour of it as a prefix also removes the one directory the executor never cleaned up, since fs.rm only ever removed the leaf. This is hardening rather than a fix for a reachable weakness: the component executes untrusted code locally with no sandbox by design, so a predictable temporary path is a weaker primitive than what it already grants. It should still not be the weak link. --- .../unsafe_local_code_executor.ts | 11 ++++--- .../unsafe_local_code_executor_test.ts | 32 +++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/core/src/code_executors/unsafe_local_code_executor.ts b/core/src/code_executors/unsafe_local_code_executor.ts index 97061719a..417d47a9e 100644 --- a/core/src/code_executors/unsafe_local_code_executor.ts +++ b/core/src/code_executors/unsafe_local_code_executor.ts @@ -65,12 +65,13 @@ async function createTempScriptFile( language: CodeExecutionLanguage, shellCommandPath?: string, ): Promise<{filePath: string; tempDir: string}> { - const tempDir = path.join( - os.tmpdir(), - 'adk_js_unsafe_code_executor', - Date.now().toString() + '_' + Math.random().toString(36).slice(2), + // mkdtemp creates the directory itself, exclusively and mode 0o700. The + // previous Date.now()/Math.random() name was predictable, and mkdir with + // `recursive` does not fail on a path that already exists, so a directory + // pre-created by another local user was adopted rather than rejected. + const tempDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'adk_js_unsafe_code_executor_'), ); - await fs.mkdir(tempDir, {recursive: true}); const ext = getExtensionForLanguage(language, shellCommandPath) || '.js'; const filePath = path.join(tempDir, `script${ext}`); diff --git a/core/test/code_executors/unsafe_local_code_executor_test.ts b/core/test/code_executors/unsafe_local_code_executor_test.ts index 82a0d2d01..ca9e0dfb7 100644 --- a/core/test/code_executors/unsafe_local_code_executor_test.ts +++ b/core/test/code_executors/unsafe_local_code_executor_test.ts @@ -15,6 +15,7 @@ import { } from '@google/adk'; import {EventEmitter} from 'node:events'; import * as os from 'node:os'; +import * as path from 'node:path'; import {beforeEach, describe, expect, it, vi} from 'vitest'; // Only `spawn` is mocked; it defaults to the real implementation (see @@ -90,6 +91,37 @@ describe('UnsafeLocalCodeExecutor', () => { expect(result.stderr).toBe(''); }); + // The script runs with the temporary directory as its cwd, so it can report + // the name and mode the executor actually created. + it('creates a private, unpredictable temporary directory', async () => { + const params: ExecuteCodeParams = { + invocationContext, + codeExecutionInput: { + code: [ + 'const fs = require("node:fs");', + 'const dir = process.cwd();', + 'const mode = (fs.statSync(dir).mode & 0o777).toString(8);', + 'console.log(JSON.stringify({dir, mode}));', + ].join('\n'), + language: CodeExecutionLanguage.JAVASCRIPT, + inputFiles: [], + }, + }; + + const first = JSON.parse((await executor.executeCode(params)).stdout); + const second = JSON.parse((await executor.executeCode(params)).stdout); + + // mkdtemp appends six random characters to the prefix it is given. + expect(path.basename(first.dir)).toMatch( + /^adk_js_unsafe_code_executor_.{6}$/, + ); + expect(second.dir).not.toBe(first.dir); + + if (os.platform() !== 'win32') { + expect(first.mode).toBe('700'); + } + }); + it('should capture stderr', async () => { const params: ExecuteCodeParams = { invocationContext, From ec69431d5baf7c9a6d44a4c6dc2e2b0b0c92e14e Mon Sep 17 00:00:00 2001 From: adilburaksen Date: Sun, 2 Aug 2026 23:01:13 +0300 Subject: [PATCH 2/2] refactor(core): address review nits on the mkdtemp change Drop the archaeology from the comment above mkdtemp: it described the implementation being replaced rather than the one being read, and the PR description already carries that history. In the temp-directory test, assert stderr before parsing stdout. If either run fails, JSON.parse throws "Unexpected end of JSON input" and swallows the stderr that would explain why. --- core/src/code_executors/unsafe_local_code_executor.ts | 5 +---- .../code_executors/unsafe_local_code_executor_test.ts | 9 +++++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/core/src/code_executors/unsafe_local_code_executor.ts b/core/src/code_executors/unsafe_local_code_executor.ts index 417d47a9e..1457d189e 100644 --- a/core/src/code_executors/unsafe_local_code_executor.ts +++ b/core/src/code_executors/unsafe_local_code_executor.ts @@ -65,10 +65,7 @@ async function createTempScriptFile( language: CodeExecutionLanguage, shellCommandPath?: string, ): Promise<{filePath: string; tempDir: string}> { - // mkdtemp creates the directory itself, exclusively and mode 0o700. The - // previous Date.now()/Math.random() name was predictable, and mkdir with - // `recursive` does not fail on a path that already exists, so a directory - // pre-created by another local user was adopted rather than rejected. + // mkdtemp names the directory itself and creates it exclusively at 0o700. const tempDir = await fs.mkdtemp( path.join(os.tmpdir(), 'adk_js_unsafe_code_executor_'), ); diff --git a/core/test/code_executors/unsafe_local_code_executor_test.ts b/core/test/code_executors/unsafe_local_code_executor_test.ts index ca9e0dfb7..7a38ed2c0 100644 --- a/core/test/code_executors/unsafe_local_code_executor_test.ts +++ b/core/test/code_executors/unsafe_local_code_executor_test.ts @@ -108,8 +108,13 @@ describe('UnsafeLocalCodeExecutor', () => { }, }; - const first = JSON.parse((await executor.executeCode(params)).stdout); - const second = JSON.parse((await executor.executeCode(params)).stdout); + const firstResult = await executor.executeCode(params); + const secondResult = await executor.executeCode(params); + expect(firstResult.stderr).toBe(''); + expect(secondResult.stderr).toBe(''); + + const first = JSON.parse(firstResult.stdout); + const second = JSON.parse(secondResult.stdout); // mkdtemp appends six random characters to the prefix it is given. expect(path.basename(first.dir)).toMatch(