Skip to content

Commit 1ac6875

Browse files
petrmarinecDeanChensj
authored andcommitted
fix: Avoid shell execution in ReadFileTool ranged reads
Merge #5268 ### Link to Issue or Description of Change **1. Link to an existing issue (if applicable):** - Related: #5267 **2. Or, if no issue exists, describe the change:** **Problem:** `ReadFileTool` handles ranged reads by building `cat -n '{path}' | sed -n ...` from the caller-supplied `path`, while full reads use `environment.read_file(path)` and Python slicing. Shell metacharacters in `path` are therefore interpreted by the shell in the ranged-read branch instead of being treated as a literal file path. **Solution:** Remove the shell-based ranged-read branch and reuse the existing Python file-read logic for all reads. This keeps ranged output behavior while eliminating the shell dependency from `ReadFileTool`. ### Testing Plan **Unit Tests:** - [x] I have added or updated unit tests for my change. - [ ] All unit tests pass locally. Passed locally in Linux Docker (`python:3.11-bookworm`): - `pytest tests/unittests/tools/test_environment_tools.py tests/unittests/tools/environment_simulation` - `pytest tests/unittests/tools` - Result: `1519 passed` **Manual End-to-End (E2E) Tests:** - [x] On unmodified `origin/main`, a ranged `ReadFileTool` call with a crafted path wrote a proof file in the working directory. - [x] After this patch, the same call returns `File not found: ...` and no proof file is written. ### Checklist - [x] I have read the [CONTRIBUTING.md](https://github.com/google/adk-python/blob/main/CONTRIBUTING.md) document. - [x] I have performed a self-review of my own code. - [ ] I have commented my code, particularly in hard-to-understand areas. - [x] I have added tests that prove my fix is effective or that my feature works. - [ ] New and existing unit tests pass locally with my changes. - [x] I have manually tested my changes end-to-end. - [x] Any dependent changes have been merged and published in downstream modules. ### Additional context This is a small fix that removes the shell-based ranged-read implementation and makes `ReadFileTool` use the same direct file-read path for both full reads and ranged reads. Co-authored-by: Shangjie Chen <deanchen@google.com> COPYBARA_INTEGRATE_REVIEW=#5268 from petrmarinec:fix-readfile-shell-injection 34b7c30 PiperOrigin-RevId: 944768002
1 parent b44d2c9 commit 1ac6875

2 files changed

Lines changed: 106 additions & 26 deletions

File tree

src/google/adk/tools/environment/_read_file_tool.py

Lines changed: 10 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
from __future__ import annotations
1818

1919
import logging
20-
import shlex
2120
from typing import Any
2221
from typing import Optional
2322
from typing import TYPE_CHECKING
@@ -103,31 +102,12 @@ async def run_async(
103102
start_line = args.get('start_line')
104103
end_line = args.get('end_line')
105104

106-
# Use `sed` to read the file if start_line or end_line are specified.
107-
if (start_line and start_line > 1) or end_line:
108-
start = start_line or 1
109-
if end_line:
110-
sed_range = f'{start},{end_line}'
111-
else:
112-
sed_range = f'{start},$'
113-
path_arg = shlex.quote(path)
114-
sed_arg = shlex.quote(f'{sed_range}p')
115-
cmd = f'cat -n {path_arg} | sed -n {sed_arg}'
116-
res = await self._environment.execute(cmd)
117-
if res.exit_code == 0:
118-
return {
119-
'status': 'ok',
120-
'content': _truncate(
121-
res.stdout,
122-
limit=self._max_output_chars,
123-
),
124-
}
125-
126105
try:
106+
# TODO: Avoid loading the entire file into memory to prevent OOM on large files.
127107
data_bytes = await self._environment.read_file(path)
128-
text = data_bytes.decode('utf-8', errors='replace')
129-
lines = text.splitlines(True)
130-
total = len(lines)
108+
# Slice data_bytes by line boundaries before decoding.
109+
lines_bytes = data_bytes.splitlines(keepends=True)
110+
total = len(lines_bytes)
131111
start = max(1, start_line or 1)
132112
end = min(total, end_line or total)
133113
if start > total:
@@ -144,9 +124,13 @@ async def run_async(
144124
'error': f'`start_line` ({start}) is after `end_line` ({end}).',
145125
'total_lines': total,
146126
}
147-
selected = lines[start - 1 : end]
127+
selected_bytes = lines_bytes[start - 1 : end]
128+
lines = [
129+
line_bytes.decode('utf-8', errors='replace')
130+
for line_bytes in selected_bytes
131+
]
148132
numbered = ''.join(
149-
f'{start + i:6d}\t{line}' for i, line in enumerate(selected)
133+
f'{start + i:6d}\t{line}' for i, line in enumerate(lines)
150134
)
151135
result = {
152136
'status': 'ok',
Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
# Copyright 2026 Google LLC
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
from pathlib import Path
16+
from typing import Optional
17+
18+
from google.adk.environment._base_environment import BaseEnvironment
19+
from google.adk.environment._base_environment import ExecutionResult
20+
from google.adk.tools.environment._tools import ReadFileTool
21+
import pytest
22+
23+
24+
class _StubEnvironment(BaseEnvironment):
25+
"""Minimal environment double for ReadFileTool tests."""
26+
27+
def __init__(self, files: dict[str, bytes]):
28+
self._files = files
29+
self.execute_calls: list[str] = []
30+
31+
@property
32+
def working_dir(self) -> Path:
33+
return Path('/tmp/adk-test')
34+
35+
async def execute(
36+
self,
37+
command: str,
38+
*,
39+
timeout: Optional[float] = None,
40+
) -> ExecutionResult:
41+
del timeout
42+
self.execute_calls.append(command)
43+
raise AssertionError('ReadFileTool should not invoke execute().')
44+
45+
async def read_file(self, path: Path) -> bytes:
46+
key = str(path)
47+
if key not in self._files:
48+
raise FileNotFoundError(key)
49+
return self._files[key]
50+
51+
async def write_file(self, path: Path, content: str | bytes) -> None:
52+
del path, content
53+
raise NotImplementedError
54+
55+
56+
@pytest.mark.asyncio
57+
async def test_read_file_with_line_range_uses_direct_file_read():
58+
"""ReadFileTool slices lines without shelling out."""
59+
environment = _StubEnvironment({
60+
'notes.txt': b'alpha\nbeta\ngamma\ndelta\n',
61+
})
62+
63+
result = await ReadFileTool(environment).run_async(
64+
args={'path': 'notes.txt', 'start_line': 2, 'end_line': 3},
65+
tool_context=None,
66+
)
67+
68+
assert result == {
69+
'status': 'ok',
70+
'content': ' 2\tbeta\n 3\tgamma\n',
71+
'total_lines': 4,
72+
}
73+
assert environment.execute_calls == []
74+
75+
76+
@pytest.mark.asyncio
77+
async def test_read_file_with_line_range_treats_shell_payload_as_literal_path():
78+
"""Shell metacharacters in the path do not trigger command execution."""
79+
environment = _StubEnvironment({
80+
'safe.txt': b'line1\nline2\n',
81+
})
82+
payload = (
83+
'\'; python3 -c "from pathlib import Path;'
84+
" Path('pwned.txt').write_text('owned')\"; echo '"
85+
)
86+
87+
result = await ReadFileTool(environment).run_async(
88+
args={'path': payload, 'start_line': 1, 'end_line': 2},
89+
tool_context=None,
90+
)
91+
92+
assert result == {
93+
'status': 'error',
94+
'error': f'File not found: {payload}',
95+
}
96+
assert environment.execute_calls == []

0 commit comments

Comments
 (0)