fix : ReadFileTool shells out on ranged reads and interpolates untrusted path

[hamdi-sakly]

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• Closes: ReadFileTool shells out on ranged reads and interpolates untrusted path #5530

2. Description of change:

Problem:

ReadFileTool shells out for ranged reads (start_line > 1 or end_line is set)
instead of reading the file directly:

# Vulnerable code
cmd = f"cat -n '{path}' | sed -n '{sed_range}p'"
res = await self._environment.execute(cmd)

The user-supplied path is interpolated directly into the shell command without
any sanitization. A path containing shell-sensitive characters (e.g. single quotes,
semicolons) is interpreted as shell syntax, allowing unintended commands to be
executed as a side effect of a file-read operation.

Solution:

Used Python's built-in shlex.quote() to safely escape the path before embedding
it in the shell command. shlex.quote() ensures the shell always treats the entire
path as a single string argument regardless of its content.

# Fixed code
import shlex

safe_path = shlex.quote(path)
cmd = f"cat -n {safe_path} | sed -n '{sed_range}p'"
res = await self._environment.execute(cmd)

shlex is a Python standard library module — no new dependencies are required.

---

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

import asyncio
from pathlib import Path
import tempfile

from google.adk.environment._local_environment import LocalEnvironment
from google.adk.tools.environment._tools import ReadFileTool


async def main():
  with tempfile.TemporaryDirectory() as td:
    env = LocalEnvironment(working_dir=Path(td))
    await env.initialize()

    target = Path(td) / "sample.txt"
    target.write_text("line1\nline2\nline3\n", encoding="utf-8")

    marker = Path(td) / "marker.txt"
    # ReadFileTool is expected to treat `path` as data, not as shell syntax.
    # This payload closes the quoted filename used by the ranged-read path,
    # injects an extra command, and reopens the quote to keep the shell happy.
    injected_path = f"sample.txt'; touch {marker}; echo '"

    tool = ReadFileTool(env)
    result = await tool.run_async(
        # `start_line=2` is important because it triggers the code path that
        # shells out through `cat ... | sed ...` instead of doing pure Python
        # file I/O.
        args={"path": injected_path, "start_line": 2},
        tool_context=None,
    )

    print(result)
    # If this prints True, a file-read API caused an unintended side effect.
    # That demonstrates the bug: the path was interpreted by the shell.
    print("marker exists:", marker.exists())
    await env.close()


asyncio.run(main())

The marker.txt file is no longer created, confirming the injected shell
command is no longer executed.

[sasha-gitg]

This is great, thanks! Can you add a few unit tests?

[failuresmith]

Even if path is safely shell-quoted, the ranged-read path still interpolates end_line into sed -n '...p'.

[failuresmith]

Now if you replace the previous POC with the

Updated POC

import asyncio
from pathlib import Path
import tempfile

from google.adk.environment._local_environment import LocalEnvironment
from google.adk.tools.environment._tools import ReadFileTool


async def main():
  with tempfile.TemporaryDirectory() as td:
    env = LocalEnvironment(working_dir=Path(td))
    await env.initialize()

    target = Path(td) / "sample.txt"
    target.write_text("line1\nline2\nline3\n", encoding="utf-8")

    marker = Path(td) / "marker.txt"
    injected_end_line = f"1'; touch {marker}; echo '"

    tool = ReadFileTool(env)
    result = await tool.run_async(
        args={"path": "sample.txt", "end_line": injected_end_line},
        tool_context=None,
    )

    print(result)
    # If this prints True, a file-read API caused an unintended side effect.
    # That demonstrates the bug: the path was interpreted by the shell.
    print("marker exists:", marker.exists())
    await env.close()


asyncio.run(main())

You could see that the current implementation of the file-read API could cause unintended side effects.

#5558 is how I would fix it.