Potential fix for code scanning alert no. 27: Uncontrolled data used in path expression #11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| source_dir = agent_dir | ||
| source_items = set(os.listdir(source_dir)) | ||
| try: | ||
| source_items = set(os.listdir(source_dir)) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 18 days ago
General Fix:
Normalize and resolve all user-controlled paths before use and verify, using a more robust mechanism than a simple string "startswith" check. Also, when using the path as a directory to be listed or accessed, use the authoritative resolved path object or string.
Detailed steps:
- Use
Path.resolve(strict=False)on bothagent_dirandbase_pathto get their true, canonical forms. - Instead of
str(agent_dir).startswith(str(base_path)), usebase_path in agent_dir.parents or agent_dir == base_pathto check containment. - Do the same for
destination_dir. - When assigning
source_dir, use the resolved, verified path for safety. - Update/add imports as needed for
Path.
Lines to change:
- Lines assigning and checking
agent_dir,destination_dir, andsource_dir(lines 251-263). - Strengthen the check and use the correct resolved path objects.
Suggested changeset
1
src/google/adk/cli/fast_api.py
| @@ -248,16 +248,16 @@ | ||
| @working_in_progress("builder_save is not ready for use.") | ||
| @app.post("/builder/app/{app_name}/cancel", response_model_exclude_none=True) | ||
| async def builder_cancel(app_name: str) -> bool: | ||
| base_path = Path.cwd() / agents_dir | ||
| # Safely compose agent_dir | ||
| agent_dir = os.path.normpath(os.path.join(base_path, app_name)) | ||
| base_path = (Path.cwd() / agents_dir).resolve() | ||
| # Safely compose agent_dir, then resolve | ||
| agent_dir = (base_path / app_name).resolve() | ||
| # Ensure agent_dir is within base_path | ||
| if not str(agent_dir).startswith(str(base_path)): | ||
| if base_path not in agent_dir.parents and agent_dir != base_path: | ||
| logger.error(f"Attempted path traversal attack with agent_dir: {agent_dir}") | ||
| return False | ||
| destination_dir = os.path.normpath(os.path.join(agent_dir, "tmp/" + app_name)) | ||
| destination_dir = (agent_dir / ("tmp/" + app_name)).resolve() | ||
| # Ensure destination_dir is also within base_path | ||
| if not str(destination_dir).startswith(str(base_path)): | ||
| if base_path not in destination_dir.parents and destination_dir != base_path: | ||
| logger.error(f"Attempted path traversal attack with destination_dir: {destination_dir}") | ||
| return False | ||
| source_dir = agent_dir |
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/venkateshpabbati/adk-python/security/code-scanning/27
To safely handle user-provided values like
app_namein filesystem paths, normalization and prefix validation should be performed.Best-Fix Approach:
Normalize all user-derived paths after joining with the intended safe base directory (
base_path) usingos.path.normpath. Then enforce that the normalized path starts with the base directory, preventing path traversal and absolute path exploits. Optionally, reject app names containing suspicious components (such as path separators, '.', '..', etc.). For defense-in-depth,werkzeug.utils.secure_filenamecould further sanitize filenames but for directories that may span multiple segments, normalization and prefix-checking suffices.Required change locations:
agent_diranddestination_dirinbuilder_cancelapp_name/values are used to construct filesystem pathsChange summary:
base_pathandapp_name, normalize the resulting path and check that it remains insidebase_path.Imports needed: None beyond standard libraries (
os,Path, etc.).Suggested fixes powered by Copilot Autofix. Review carefully before merging.