Skip to content

refactor(core): create the unsafe executor's temp directory with mkdtemp - #600

Open
adilburaksen wants to merge 2 commits into
google:mainfrom
adilburaksen:harden/mkdtemp-unsafe-local-executor
Open

refactor(core): create the unsafe executor's temp directory with mkdtemp#600
adilburaksen wants to merge 2 commits into
google:mainfrom
adilburaksen:harden/mkdtemp-unsafe-local-executor

Conversation

@adilburaksen

Copy link
Copy Markdown
Contributor

The hardening @AmaadMartin offered to review in #598 (comment), split out from #599 since it is unrelated to randomUUID.

createTempScriptFile built its directory name from Date.now() and Math.random(), then created it with fs.mkdir(tempDir, {recursive: true}). recursive does not fail on a path that already exists, so a directory pre-created by another local user under os.tmpdir() is adopted rather than rejected, and it keeps whatever permissions that user gave it.

fs.mkdtemp names the directory itself, creates it exclusively, and does so at mode 0o700. Using the old fixed segment as its prefix instead of as a parent directory also removes the one directory the executor never cleaned up — fs.rm at the end of executeCode only ever removed the leaf, so os.tmpdir()/adk_js_unsafe_code_executor accumulated and stayed behind.

-  const tempDir = path.join(
-    os.tmpdir(),
-    'adk_js_unsafe_code_executor',
-    Date.now().toString() + '_' + Math.random().toString(36).slice(2),
-  );
-  await fs.mkdir(tempDir, {recursive: true});
+  const tempDir = await fs.mkdtemp(
+    path.join(os.tmpdir(), 'adk_js_unsafe_code_executor_'),
+  );

To be clear about what this is and is not: it is ordinary hardening, not a fix for a reachable weakness. UnsafeLocalCodeExecutor executes untrusted code locally with no sandbox by design, so a predictable temporary path is a weaker primitive than what the component already grants. That is the reason I did not file it as a security issue — it just should not be the weak link.

Test

creates a private, unpredictable temporary directory uses the public API: the script runs with the temporary directory as its cwd, so it reports its own directory name and mode back through stdout. It asserts the mkdtemp naming, that two runs differ, and mode 700 off Windows.

Negative control run rather than assumed — with the old mkdir restored, that test is the only one that fails, on expected '1785598236728_3zkn5o6rehu' to match /^adk_js_unsafe_code_executor_.{6}$/.

tsc --noEmit, prettier and eslint are clean, and the executor suite is green at 19 tests.

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.

@AmaadMartin AmaadMartin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct and minimal, and it matches what the repo already does everywhere else (core/test/skills/loader_test.ts:128, file_artifact_service_test.ts:24, core/test/utils/file_utils_test.ts:18 all use mkdtemp with a prefix). Cleanup story checks out: fs.rm in the finally at unsafe_local_code_executor.ts:304 now removes the whole thing, so the never-deleted os.tmpdir()/adk_js_unsafe_code_executor parent goes away with it. Dependent search is clean — that name appears nowhere else in the repo and createTempScriptFile has one caller; I also checked dev/src/utils/file_utils.ts:107 for the same shape, but it names dirs with randomUUID so it isn't exposed the same way. Two nits, neither blocking.

Comment on lines +68 to +71
// 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. The comment narrates the old code rather than the new.

  // 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.

Nothing else in core/src explains a prior implementation (grepped — zero hits), and your PR description already covers the archaeology, which is where it survives a rename. Same shape as the doc-comment nit on #599, so treat it as one habit rather than two findings.

Comment on lines +111 to +112
const first = JSON.parse((await executor.executeCode(params)).stdout);
const second = JSON.parse((await executor.executeCode(params)).stdout);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, optional. If either run fails, JSON.parse throws Unexpected end of JSON input and swallows the stderr that would explain why.

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

should execute code and return stdout at :91 already asserts stderr is '' for a plain node script, so the added assertion is one that passes today.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants