Skip to content

fix(adk_documentation): confine docs-agent file tools to the managed repos directory - #5996

Closed
adilburaksen wants to merge 4 commits into
google:mainfrom
adilburaksen:harden/docs-agent-path-confinement
Closed

fix(adk_documentation): confine docs-agent file tools to the managed repos directory#5996
adilburaksen wants to merge 4 commits into
google:mainfrom
adilburaksen:harden/docs-agent-path-confinement

Conversation

@adilburaksen

@adilburaksen adilburaksen commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

The adk_documentation sample tools (read_local_git_repo_file_content, list_directory_contents, search_local_git_repo, and the file-writing step of create_pull_request_from_changes) are invoked by autonomous agents (adk_docs_updater, adk_release_analyzer) that run non-interactively in GitHub Actions and process untrusted input — GitHub issue bodies and release diffs.

read_local_git_repo_file_content only checks os.path.isabs(file_path) before opening the file. Any absolute path is therefore read, including paths outside the repositories cloned under LOCAL_REPOS_DIR_PATH (e.g. the credentials file referenced by GOOGLE_APPLICATION_CREDENTIALS, or /proc/self/environ). list_directory_contents and search_local_git_repo have the same gap, and create_pull_request_from_changes joins caller-provided changes keys onto local_path without rejecting .., so a key like ../../x writes outside the repo.

Because these tools are LLM-driven over untrusted text, a crafted instruction can steer an agent to read a file outside the workspace and place its contents into a pull request it opens. This change keeps the tools within their intended sandbox.

Change

  • Add _resolve_within_repos_dir() that os.path.realpath-resolves a path (collapsing symlinks and ..) and requires the result to stay within LOCAL_REPOS_DIR_PATH.
  • Apply it to read_local_git_repo_file_content, list_directory_contents, and search_local_git_repo.
  • In create_pull_request_from_changes, resolve each target path and reject any that escapes the repository root before writing.
  • Add tools_test.py covering: reads/lists/searches outside the sandbox are denied, in-sandbox reads still succeed, a symlink pointing outside is denied, and a traversing changes key is rejected without writing.

No functional change for legitimate in-repo paths.

Testing Plan

Added contributing/samples/adk_team/adk_documentation/tools_test.py (unittest), covering both the rejection and the no-regression paths:

  • test_read_outside_sandbox_is_deniedread_local_git_repo_file_content rejects /etc/passwd and /proc/self/environ.
  • test_read_symlink_escape_is_denied — a symlink inside the sandbox pointing to /etc/passwd is rejected (paths are realpath-resolved before the check).
  • test_read_inside_sandbox_succeeds — a legitimate file under LOCAL_REPOS_DIR_PATH is still read successfully (no regression).
  • test_list_and_search_outside_sandbox_are_deniedlist_directory_contents and search_local_git_repo reject paths outside the sandbox.
  • test_create_pr_rejects_path_traversal_in_changes — a changes key such as ../../../../tmp/evil.txt is rejected and no file is written outside the repo.

Test logs

$ PYTHONPATH=contributing/samples/adk_team python -m pytest \
    contributing/samples/adk_team/adk_documentation/tools_test.py -v

tools_test.py::PathConfinementTest::test_create_pr_rejects_path_traversal_in_changes PASSED [ 20%]
tools_test.py::PathConfinementTest::test_list_and_search_outside_sandbox_are_denied PASSED [ 40%]
tools_test.py::PathConfinementTest::test_read_inside_sandbox_succeeds            PASSED [ 60%]
tools_test.py::PathConfinementTest::test_read_outside_sandbox_is_denied          PASSED [ 80%]
tools_test.py::PathConfinementTest::test_read_symlink_escape_is_denied           PASSED [100%]

======================== 5 passed in 0.27s =========================

…repos directory

The adk_documentation tools are driven by autonomous agents (adk_docs_updater,
adk_release_analyzer) that run non-interactively in CI and process untrusted
input (issue bodies, release diffs). read_local_git_repo_file_content only
checked os.path.isabs(), so any absolute path was read, including files outside
the cloned repos under LOCAL_REPOS_DIR_PATH (e.g. $GOOGLE_APPLICATION_CREDENTIALS,
/proc/self/environ). list_directory_contents and search_local_git_repo had the
same gap, and create_pull_request_from_changes joined caller-provided change keys
onto local_path without rejecting '..'.

Add _resolve_within_repos_dir() (realpath-resolve + confine to LOCAL_REPOS_DIR_PATH),
apply it to the read/list/search tools, and reject change paths that escape the
repository root before writing. Add tools_test.py covering these cases. No change
for legitimate in-repo paths.
@adk-bot adk-bot added the tools [Component] This issue is related to tools label Jun 6, 2026
@adk-bot

adk-bot commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Response from ADK Triaging Agent

Hello @adilburaksen, thank you for creating this PR! It is a great security improvement for our document-agent tools.

To make it easier for our reviewers to review and merge your PR, please ensure it aligns with our Contribution Guidelines. Specifically, we kindly ask you to update the PR description to include the following:

  1. A Testing Plan section: Please include a dedicated testing plan section describing how you tested these changes (e.g. the scenarios covered by the new tools_test.py).
  2. Logs or Screenshots: Since this is a bug/security fix, please provide the console outputs or pytest logs demonstrating that the tests (especially the newly added unit tests) are passing.

Providing this context will save review time and help us process your contribution faster. Thank you!

@adilburaksen

Copy link
Copy Markdown
Contributor Author

Thanks! I've updated the PR description with a Testing Plan section (the five scenarios covered by the new tools_test.py) and the pytest logs showing all 5 tests passing. Let me know if anything else is needed.

@rohityan

rohityan commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

/adk-pr-analyze

adilburaksen and others added 2 commits June 9, 2026 21:44
Reformat tools.py and tools_test.py with pyink (repo pre-commit config)
to fix the failing pre-commit / pyink check.
@adilburaksen

Copy link
Copy Markdown
Contributor Author

@rohityan thanks for picking this up. I've cleared the two blockers:

  • pre-commit / pyink: reformatted tools.py and tools_test.py with pyink (repo config) — pyink --check is now clean.
  • out-of-date: updated the branch with main (cleanly merged).

CI is re-running on the latest HEAD; the change is now ready for review whenever you have a moment. Happy to address anything the /adk-pr-analyze pass surfaces.

@rohityan rohityan added the needs review [Status] The PR/issue is awaiting review from the maintainer label Jun 11, 2026
@rohityan
rohityan requested a review from GWeale June 11, 2026 23:36
@rohityan

Copy link
Copy Markdown
Collaborator

Hi @GWeale can you please review this.

@rohityan

Copy link
Copy Markdown
Collaborator

Hi @adilburaksen , 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
rohityan requested review from wukath and removed request for wukath June 15, 2026 21:59
@adilburaksen

Copy link
Copy Markdown
Contributor Author

@GWeale @rohityan friendly ping on this one — it's been in review since the 11th and all checks are green. It confines the docs-agent file tools (read_local_git_repo_file_content / list_directory_contents / search_local_git_repo) to the managed repos dir and rejects ..-escaping write paths in create_pull_request_from_changes, with tests. Happy to rebase or adjust anything — just would appreciate getting it merged when you have a moment.

@GWeale GWeale assigned GWeale and unassigned rohityan Jun 18, 2026
@adilburaksen

Copy link
Copy Markdown
Contributor Author

@GWeale @rohityan — second gentle nudge. This has been in review since June 11 and all checks are green (check-changes, check-file-contents, cla/google, header-check, mypy-diff 3.10–3.13 all pass). It's a small, contained security fix: it confines the docs-agent file tools to the managed repos directory so a crafted issue body can't drive reads/writes outside the intended path.

If the original reviewers are unavailable, could this be routed to a security maintainer? Happy to rebase or adjust naming if anything's blocking. Thanks!

@adilburaksen

Copy link
Copy Markdown
Contributor Author

Follow-up with some consolidation: the sibling PR #6034 (Antigravity run_antigravity.py confinement) is now moot — that script was removed upstream in 9127feb, so #5996 is the only remaining fix for this file-tool confinement class. The unconfined read is still present on main (read_local_git_repo_file_content still guards only with os.path.isabs, no directory confinement). All checks are green; the branch is just behind main — happy to rebase.

@GWeale @rohityan could you help get this reviewed/landed? The Google OSS VRP report tracking this is gated specifically on #5996 being merged. Thanks!

@adilburaksen

Copy link
Copy Markdown
Contributor Author

@xuanyang15 — would you be able to take this one (or route it to the right owner)? You imported the sibling fix #5826 (config_path traversal) and own the recent adk_documentation changes, so you're likely the right reviewer here.

Status: green on all checks and in review since Jun 11 (check-changes, check-file-contents, cla/google, header, pyink). It confines the three docs-agent file tools (read_local_git_repo_file_content, list_directory_contents, search_local_git_repo) to LOCAL_REPOS_DIR_PATH (realpath-resolved, symlink-safe) and rejects ..-escaping write paths in create_pull_request_from_changes, with unit tests. The other layer (#6034, run_antigravity.py) is now moot — that script was removed upstream in 9127feb.

Happy to rebase if it has drifted. Thanks!

@xuanyang15 xuanyang15 self-assigned this Jul 9, 2026
copybara-service Bot pushed a commit that referenced this pull request Jul 13, 2026
Merge #5996

The `adk_documentation` sample tools (`read_local_git_repo_file_content`,
`list_directory_contents`, `search_local_git_repo`, and the file-writing
step of `create_pull_request_from_changes`) are invoked by autonomous
agents (`adk_docs_updater`, `adk_release_analyzer`) that run non-interactively
in GitHub Actions and process untrusted input — GitHub issue bodies and
release diffs.

This change keeps the tools within their intended sandbox by resolving
symlinks and `..` segments, then requiring results to stay inside the
managed repositories directory.

PiperOrigin-RevId: 947146756
@adk-bot

adk-bot commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Thank you @adilburaksen for your contribution! 🎉

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

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

@adk-bot adk-bot added the merged [Status] This PR is merged label Jul 13, 2026
@adk-bot adk-bot closed this Jul 13, 2026
FrigaZzz pushed a commit to FrigaZzz/adk-python that referenced this pull request Aug 11, 2026
Merge google#5996

The `adk_documentation` sample tools (`read_local_git_repo_file_content`,
`list_directory_contents`, `search_local_git_repo`, and the file-writing
step of `create_pull_request_from_changes`) are invoked by autonomous
agents (`adk_docs_updater`, `adk_release_analyzer`) that run non-interactively
in GitHub Actions and process untrusted input — GitHub issue bodies and
release diffs.

This change keeps the tools within their intended sandbox by resolving
symlinks and `..` segments, then requiring results to stay inside the
managed repositories directory.

PiperOrigin-RevId: 947146756
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged [Status] This PR is merged needs review [Status] The PR/issue is awaiting review from the maintainer tools [Component] This issue is related to tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants