Skip to content

Potential fix for code scanning alert no. 27: Uncontrolled data used in path expression #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Potential fix for code scanning alert no. 27: Uncontrolled data used in path expression #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/google/adk/cli/fast_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,20 @@
@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
agent_dir = os.path.join(base_path, app_name)
destination_dir = os.path.join(agent_dir, "tmp/" + app_name)
# Safely compose agent_dir
agent_dir = os.path.normpath(os.path.join(base_path, app_name))
# Ensure agent_dir is within base_path
if not str(agent_dir).startswith(str(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))
# Ensure destination_dir is also within base_path
if not str(destination_dir).startswith(str(base_path)):
logger.error(f"Attempted path traversal attack with destination_dir: {destination_dir}")
return False
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

This path depends on a
user-provided value
Loading
.

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 both agent_dir and base_path to get their true, canonical forms.
  • Instead of str(agent_dir).startswith(str(base_path)), use base_path in agent_dir.parents or agent_dir == base_path to 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, and source_dir (lines 251-263).
  • Strengthen the check and use the correct resolved path objects.
Suggested changeset 1
src/google/adk/cli/fast_api.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/google/adk/cli/fast_api.py b/src/google/adk/cli/fast_api.py
--- a/src/google/adk/cli/fast_api.py
+++ b/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
EOF
@@ -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.
for item in os.listdir(destination_dir):
if item in source_items:
continue
Expand All @@ -266,7 +275,11 @@

for item in os.listdir(source_dir):
source_item = os.path.join(source_dir, item)
destination_item = os.path.join(destination_dir, item)
destination_item = os.path.normpath(os.path.join(destination_dir, item))
# Ensure destination_item remains in destination_dir
if not str(destination_item).startswith(str(destination_dir)):
logger.error(f"Attempted path traversal attack with destination_item: {destination_item}")
continue
if item == "tmp" and os.path.isdir(source_item):
continue
if os.path.isdir(source_item):
Expand Down
Loading