Skip to content

fix(tools): remove shell-based ranged file reads#5558

Open
failuresmith wants to merge 2 commits intogoogle:mainfrom
failuresmith:fix/readfiletool-ranged-read-injection
Open

fix(tools): remove shell-based ranged file reads#5558
failuresmith wants to merge 2 commits intogoogle:mainfrom
failuresmith:fix/readfiletool-ranged-read-injection

Conversation

@failuresmith
Copy link
Copy Markdown

Problem:
ReadFileTool uses a shell-based cat -n ... | sed -n ... path for ranged reads when start_line > 1 or end_line is provided. PR #5537 improves the original report by quoting path, but the ranged-read path still shells out and still interpolates end_line into the sed command.

Because tool arguments are not enforced as runtime-validated integers in this path, that leaves a remaining command-injection surface. The shell path also masks missing-file errors for ranged reads by returning ok with empty content.

Solution:
Remove the shell-based ranged-read branch entirely and route all reads through self._environment.read_file(path) plus the existing Python line-slicing logic.

This fixes the issue by eliminating shell interpretation from ReadFileTool rather than trying to quote individual inputs. It also preserves correct missing-file behavior for ranged reads and adds regression coverage for:

  • normal ranged reads
  • missing-file ranged reads
  • non-integer end_line input

Testing Plan

Verified with targeted unit tests for ranged reads and with a manual PoC that previously triggered command execution via end_line.

Unit Tests:

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

Manual End-to-End (E2E) Tests:

  1. Run the adjusted PoC that passes a malicious end_line value to ReadFileTool.
  2. Before this change, the marker file is created as a side effect.
  3. After this change, ReadFileTool returns an error for non-integer end_line and the marker file is not created.

Observed after the fix:

{'status': 'error', 'error': '`end_line` must be an integer if provided.'}
marker exists: False

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 intended as a complete fix for the ranged-read injection surface, rather than a partial mitigation limited to quoting path.

@adk-bot adk-bot added the tools [Component] This issue is related to tools label Apr 30, 2026
@rohityan rohityan self-assigned this Apr 30, 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.

ReadFileTool shells out on ranged reads and interpolates untrusted path

3 participants