Avoid shell execution in ReadFileTool ranged reads by petrmarinec · Pull Request #5268 · google/adk-python

petrmarinec · 2026-04-10T20:45:27Z

Link to Issue or Description of Change

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

Related: Avoid shell execution in ReadFileTool ranged reads #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:

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:

On unmodified origin/main, a ranged ReadFileTool call with a crafted path wrote a proof file in the working directory.
After this patch, the same call returns File not found: ... and no proof file is written.

Checklist

I have read the CONTRIBUTING.md document.
I have performed a self-review of my own code.
I have commented my code, particularly in hard-to-understand areas.
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.
I have manually tested my changes end-to-end.
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.

rohityan · 2026-04-13T21:07:01Z

Hi @petrmarinec , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share.

rohityan · 2026-04-13T21:07:15Z

Hi @wukath , can you please review this.

rohityan · 2026-05-12T01:20:50Z

Hi @petrmarinec , can you please resolve the branch conflicts.

petrmarinec · 2026-05-12T11:18:10Z

Hi @petrmarinec , can you please resolve the branch conflicts.

Hi! done.

petrmarinec · 2026-06-19T13:17:07Z

Hi, I rebased this PR onto the current main and re-ran the targeted tool tests locally.

I also noticed #5530 was marked as a duplicate of #5267 and #5558 was closed in favor of this original issue path, so this branch should now be up to date if you'd like to proceed with the original fix here.

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

adk-bot · 2026-07-08T23:34:27Z

Thank you @petrmarinec for your contribution! 🎉

Your changes have been successfully imported and merged via Copybara in commit 1ac6875.

Closing this PR as the changes are now in the main branch.

petrmarinec · 2026-07-09T10:47:44Z

For attribution: this merged fix corresponds to Google OSS VRP report #497794205:
https://issuetracker.google.com/issues/497794205

Backport of the following commits from `main`, released in v2.5.0: 1ac6875 fix: Avoid shell execution in ReadFileTool ranged reads (google#5268, relates to google#5267) 3c0fb65 fix(environment): add integer validation in ReadFileTool Adapted rather than cherry-picked directly: on `main` these commits touch `tools/environment/_read_file_tool.py`, which was split out of `tools/environment/_tools.py` after the v1 branch point. The change is applied here to `_tools.py`, where ReadFileTool still lives on v1. `ReadFileTool` handled ranged reads by building `cat -n '{path}' | sed -n '{range}p'` from the caller-supplied `path` and passing it to `BaseEnvironment.execute()`, which runs the string through `asyncio.create_subprocess_shell`. Full reads already used `environment.read_file()` with Python slicing -- only the ranged branch went through a shell. Shell metacharacters in `path` were therefore interpreted by the shell instead of being treated as part of a literal file path. The shell branch is removed and all reads now use the existing read-and-slice path. `start_line` / `end_line` are validated as non-bool integers, since they now reach a Python slice directly instead of being interpolated into a `sed` range. Ranged-read output, line numbering and `total_lines` are unchanged.

Merge google#5268 ### Link to Issue or Description of Change **1. Link to an existing issue (if applicable):** - Related: google#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=google#5268 from petrmarinec:fix-readfile-shell-injection 34b7c30 PiperOrigin-RevId: 944768002

adk-bot added the tools [Component] This issue is related to tools label Apr 10, 2026

rohityan self-assigned this Apr 13, 2026

rohityan added the needs review [Status] The PR/issue is awaiting review from the maintainer label Apr 13, 2026

sanketpatil06 mentioned this pull request Apr 14, 2026
Avoid shell execution in ReadFileTool ranged reads #5267
Closed

rohityan added request clarification [Status] The maintainer need clarification or more information from the author and removed needs review [Status] The PR/issue is awaiting review from the maintainer labels May 12, 2026

petrmarinec force-pushed the fix-readfile-shell-injection branch from 5df3e52 to 38d8b80Compare May 12, 2026 11:14

petrmarinec force-pushed the fix-readfile-shell-injection branch from 38d8b80 to 34b7c30Compare June 10, 2026 09:04

DeanChensj assigned DeanChensj and unassigned rohityan Jun 16, 2026

fix: avoid shell execution in ReadFileTool
af7289e

petrmarinec force-pushed the fix-readfile-shell-injection branch from 34b7c30 to af7289eCompare June 17, 2026 23:23

wuliang229 self-assigned this Jul 8, 2026

adk-bot added the merged [Status] This PR is merged label Jul 8, 2026

adk-bot closed this Jul 8, 2026

Navigation Menu

Uh oh!

Avoid shell execution in ReadFileTool ranged reads - #5268

Avoid shell execution in ReadFileTool ranged reads#5268
petrmarinec wants to merge 1 commit into
google:mainfrom
petrmarinec:fix-readfile-shell-injection

petrmarinec commented Apr 10, 2026

Uh oh!

rohityan commented Apr 13, 2026

Uh oh!

rohityan commented Apr 13, 2026

Uh oh!

rohityan commented May 12, 2026

Uh oh!

petrmarinec commented May 12, 2026

Uh oh!

petrmarinec commented Jun 19, 2026

Uh oh!

adk-bot commented Jul 8, 2026

Uh oh!

petrmarinec commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

petrmarinec commented Apr 10, 2026

Link to Issue or Description of Change

Testing Plan

Checklist

Additional context

Uh oh!

rohityan commented Apr 13, 2026

Uh oh!

rohityan commented Apr 13, 2026

Uh oh!

rohityan commented May 12, 2026

Uh oh!

petrmarinec commented May 12, 2026

Uh oh!

petrmarinec commented Jun 19, 2026

Uh oh!

adk-bot commented Jul 8, 2026

Uh oh!

petrmarinec commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants