Skip to content

Avoid shell execution in ReadFileTool ranged reads#5268

Open
petrmarinec wants to merge 2 commits intogoogle:mainfrom
petrmarinec:fix-readfile-shell-injection
Open

Avoid shell execution in ReadFileTool ranged reads#5268
petrmarinec wants to merge 2 commits intogoogle:mainfrom
petrmarinec:fix-readfile-shell-injection

Conversation

@petrmarinec
Copy link
Copy Markdown

Link to Issue or Description of Change

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

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.

@adk-bot adk-bot added the tools [Component] This issue is related to tools label Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tools [Component] This issue is related to tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants