Skip to content

Commit 25933a8

Browse files
committed
fix(evaluation): Prevent path traversal in local eval managers
This commit adds a strict validation regex (^[a-zA-Z0-9_\-\.]+$) and explicit `..` checks for app_name, eval_set_id, eval_case_id, and eval_set_result_id in LocalEvalSetsManager and LocalEvalSetResultsManager. By sanitizing path parameters, this prevents directory traversal attacks when the FastAPI endpoints attempt to read or modify evaluation JSON files on the local filesystem.
1 parent f973673 commit 25933a8

File tree

4 files changed

+36
-7
lines changed

4 files changed

+36
-7
lines changed

src/google/adk/evaluation/local_eval_set_results_manager.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import logging
1818
import os
19+
import re
1920

2021
from typing_extensions import override
2122

@@ -67,6 +68,7 @@ def get_eval_set_result(
6768
self, app_name: str, eval_set_result_id: str
6869
) -> EvalSetResult:
6970
"""Returns an EvalSetResult identified by app_name and eval_set_result_id."""
71+
self._validate_id("Eval Set Result ID", eval_set_result_id)
7072
# Load the eval set result file data.
7173
maybe_eval_result_file_path = (
7274
os.path.join(
@@ -97,4 +99,12 @@ def list_eval_set_results(self, app_name: str) -> list[str]:
9799
return eval_result_files
98100

99101
def _get_eval_history_dir(self, app_name: str) -> str:
102+
self._validate_id("App Name", app_name)
100103
return os.path.join(self._agents_dir, app_name, _ADK_EVAL_HISTORY_DIR)
104+
105+
def _validate_id(self, id_name: str, id_value: str):
106+
pattern = r"^[a-zA-Z0-9_\-\.]+$"
107+
if not bool(re.fullmatch(pattern, id_value)) or ".." in id_value:
108+
raise ValueError(
109+
f"Invalid {id_name}. {id_name} should have the `{pattern}` format and not contain `..`",
110+
)

src/google/adk/evaluation/local_eval_sets_manager.py

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ def get_eval_set(self, app_name: str, eval_set_id: str) -> Optional[EvalSet]:
201201
try:
202202
eval_set_file_path = self._get_eval_set_file_path(app_name, eval_set_id)
203203
return load_eval_set_from_file(eval_set_file_path, eval_set_id)
204-
except FileNotFoundError:
204+
except (FileNotFoundError, ValueError):
205205
return None
206206

207207
@override
@@ -211,8 +211,6 @@ def create_eval_set(self, app_name: str, eval_set_id: str) -> EvalSet:
211211
Raises:
212212
ValueError: If Eval Set ID is not valid or an eval set already exists.
213213
"""
214-
self._validate_id(id_name="Eval Set ID", id_value=eval_set_id)
215-
216214
# Define the file path
217215
new_eval_set_path = self._get_eval_set_file_path(app_name, eval_set_id)
218216

@@ -247,6 +245,7 @@ def list_eval_sets(self, app_name: str) -> list[str]:
247245
Raises:
248246
NotFoundError: If the eval directory for the app is not found.
249247
"""
248+
self._validate_id("App Name", app_name)
250249
eval_set_file_path = os.path.join(self._agents_dir, app_name)
251250
eval_sets = []
252251
try:
@@ -266,6 +265,7 @@ def get_eval_case(
266265
self, app_name: str, eval_set_id: str, eval_case_id: str
267266
) -> Optional[EvalCase]:
268267
"""Returns an EvalCase if found; otherwise, None."""
268+
self._validate_id("Eval Case ID", eval_case_id)
269269
eval_set = self.get_eval_set(app_name, eval_set_id)
270270
if not eval_set:
271271
return None
@@ -310,17 +310,19 @@ def delete_eval_case(
310310
self._save_eval_set(app_name, eval_set_id, updated_eval_set)
311311

312312
def _get_eval_set_file_path(self, app_name: str, eval_set_id: str) -> str:
313+
self._validate_id("App Name", app_name)
314+
self._validate_id("Eval Set ID", eval_set_id)
313315
return os.path.join(
314316
self._agents_dir,
315317
app_name,
316318
eval_set_id + _EVAL_SET_FILE_EXTENSION,
317319
)
318320

319321
def _validate_id(self, id_name: str, id_value: str):
320-
pattern = r"^[a-zA-Z0-9_]+$"
321-
if not bool(re.fullmatch(pattern, id_value)):
322+
pattern = r"^[a-zA-Z0-9_\-\.]+$"
323+
if not bool(re.fullmatch(pattern, id_value)) or ".." in id_value:
322324
raise ValueError(
323-
f"Invalid {id_name}. {id_name} should have the `{pattern}` format",
325+
f"Invalid {id_name}. {id_name} should have the `{pattern}` format and not contain `..`",
324326
)
325327

326328
def _write_eval_set_to_path(self, eval_set_path: str, eval_set: EvalSet):

tests/unittests/evaluation/test_local_eval_set_results_manager.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,3 +174,11 @@ def test_list_eval_set_results_empty(self):
174174
# No eval set results saved for the app
175175
results = self.manager.list_eval_set_results(self.app_name)
176176
assert results == []
177+
178+
def test_get_eval_history_dir_invalid_app_name(self):
179+
with pytest.raises(ValueError, match="Invalid App Name"):
180+
self.manager.list_eval_set_results("../invalid")
181+
182+
def test_get_eval_set_result_invalid_id(self):
183+
with pytest.raises(ValueError, match="Invalid Eval Set Result ID"):
184+
self.manager.get_eval_set_result(self.app_name, "../invalid_id")

tests/unittests/evaluation/test_local_eval_sets_manager.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -390,11 +390,20 @@ def test_local_eval_sets_manager_create_eval_set_invalid_id(
390390
self, local_eval_sets_manager
391391
):
392392
app_name = "test_app"
393-
eval_set_id = "invalid-id"
393+
eval_set_id = "invalid/id"
394394

395395
with pytest.raises(ValueError, match="Invalid Eval Set ID"):
396396
local_eval_sets_manager.create_eval_set(app_name, eval_set_id)
397397

398+
def test_local_eval_sets_manager_create_eval_set_invalid_app_name(
399+
self, local_eval_sets_manager
400+
):
401+
app_name = "../test_app"
402+
eval_set_id = "test_eval_set"
403+
404+
with pytest.raises(ValueError, match="Invalid App Name"):
405+
local_eval_sets_manager.create_eval_set(app_name, eval_set_id)
406+
398407
def test_local_eval_sets_manager_create_eval_set_already_exists(
399408
self, local_eval_sets_manager, mocker
400409
):

0 commit comments

Comments
 (0)