Repository navigation
fix(evaluation): Prevent path traversal in local eval managers - #5039
shivan4030 wants to merge 3 commits into
Conversation
|
@mbalandat @siddharthab, could you please review this PR and approve the CI workflows? |
|
Sorry, did you mean to tag me? |
|
Hi @shivan4030 , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the failing mypy-diff tests. Also can you fix the formatting errors. You can use autoformat.sh |
rohityan
added
the
request clarification
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.
Replaces `hashlib.md5` with `hashlib.sha256` for session key generation in `mcp_session_manager.py` to mitigate security risks associated with weak cryptographic hashes. Updated the corresponding unit test to expect SHA256 hashes.
|
I've updated the PR with the formatting fixes and addressed the mypy issues. Ready for another look! |
|
path traversal in the local eval managers was fixed in 7b87f91, there's a shared evaluation/_path_validation.py now applied to the local and gcs managers, in v2.4.0+ and in 1.39.1. the md5 to sha256 change you bundled into mcp_session_manager is unrelated and still unmerged, so open a separate issue if you want it. |
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.Problem:
The
LocalEvalSetsManagerandLocalEvalSetResultsManagerclasses insidesrc/google/adk/evaluation/were missing input sanitization for various path routing IDs (app_name,eval_set_id,eval_case_id, andeval_set_result_id). Because these values were blindly passed intoos.path.join()when executing operations viaadk webendpoints (likeGET /apps/{app_name}/eval-sets/{eval_set_id}...), an attacker could craft malicious directory traversal patterns (e.g.,../..) allowing them to traverse out of the target evaluation directory. This introduces a vulnerability where unauthorized users could read or delete sensitive evaluation records (.evalset.json/.evalset_result.jsonfiles) belonging to other applications on the server's filesystem.Solution:
A new
_validate_id()logic enforcement was introduced across the board inLocalEvalSetsManagerandLocalEvalSetResultsManager. This restricts parameters using a rigid regex pattern (^[a-zA-Z0-9_\-\.]+$) that allows necessary characters like hyphens and floating-point timestamps (which are natively appended to result IDs), but explicitly rejects payloads containing double dots (..). This ensures theadk webrouter endpoints can strictly read, update, or delete payloads within their expected hierarchical folder paths, permanently patching the unauthenticated scope leakage risk.