From 79ed95383dd0bc13984b6499913babef232d4dab Mon Sep 17 00:00:00 2001 From: George Weale Date: Tue, 24 Mar 2026 13:10:08 -0700 Subject: [PATCH 1/7] fix: Exclude compromised LiteLLM versions from dependencies pin to 1.82.6 Versions 1.82.7 and 1.82.8 of LiteLLM were affected by a supply chain attack and are now explicitly excluded from the dependency constraints for both project and dev dependencies. Co-authored-by: George Weale PiperOrigin-RevId: 888818704 --- pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 824a3dcee0..ddeb50bf21 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -125,7 +125,7 @@ test = [ "kubernetes>=29.0.0", # For GkeCodeExecutor "langchain-community>=0.3.17", "langgraph>=0.2.60, <0.4.8", # For LangGraphAgent - "litellm>=1.75.5, <2.0.0", # For LiteLLM tests + "litellm>=1.75.5, <=1.82.6", # For LiteLLM tests. Upper bound pinned: versions 1.82.7+ compromised in supply chain attack. "llama-index-readers-file>=0.4.0", # For retrieval tests "openai>=1.100.2", # For LiteLLM "opentelemetry-instrumentation-google-genai>=0.3b0, <1.0.0", @@ -158,7 +158,7 @@ extensions = [ "kubernetes>=29.0.0", # For GkeCodeExecutor "k8s-agent-sandbox>=0.1.1.post2", # For GkeCodeExecutor sandbox mode "langgraph>=0.2.60, <0.4.8", # For LangGraphAgent - "litellm>=1.75.5, <2.0.0", # For LiteLlm class. Currently has OpenAI limitations. TODO: once LiteLlm fix it + "litellm>=1.75.5, <=1.82.6", # For LiteLlm class. Upper bound pinned: versions 1.82.7+ compromised in supply chain attack. "llama-index-readers-file>=0.4.0", # For retrieval using LlamaIndex. "llama-index-embeddings-google-genai>=0.3.0", # For files retrieval using LlamaIndex. "lxml>=5.3.0", # For load_web_page tool. From 0019801f6080a371a71b09cbd8291f2e79a49336 Mon Sep 17 00:00:00 2001 From: George Weale Date: Mon, 23 Mar 2026 14:57:57 -0700 Subject: [PATCH 2/7] fix: add protection for arbitrary module imports Close #4947 Co-authored-by: George Weale PiperOrigin-RevId: 888296476 --- src/google/adk/cli/adk_web_server.py | 180 +++++++++++++++++- .../cli/test_adk_web_server_run_live.py | 73 +++++++ tests/unittests/cli/test_fast_api.py | 42 +++- 3 files changed, 292 insertions(+), 3 deletions(-) diff --git a/src/google/adk/cli/adk_web_server.py b/src/google/adk/cli/adk_web_server.py index bdecdbacd0..47c0a940bd 100644 --- a/src/google/adk/cli/adk_web_server.py +++ b/src/google/adk/cli/adk_web_server.py @@ -20,6 +20,7 @@ import json import logging import os +import re import sys import time import traceback @@ -140,6 +141,158 @@ def _parse_cors_origins( return literal_origins, combined_regex +def _is_origin_allowed( + origin: str, + allowed_literal_origins: list[str], + allowed_origin_regex: Optional[re.Pattern[str]], +) -> bool: + """Check whether the given origin matches the allowed origins.""" + if "*" in allowed_literal_origins: + return True + if origin in allowed_literal_origins: + return True + if allowed_origin_regex is not None: + return allowed_origin_regex.fullmatch(origin) is not None + return False + + +def _normalize_origin_scheme(scheme: str) -> str: + """Normalize request schemes to the browser Origin scheme space.""" + if scheme == "ws": + return "http" + if scheme == "wss": + return "https" + return scheme + + +def _strip_optional_quotes(value: str) -> str: + """Strip a single pair of wrapping quotes from a header value.""" + if len(value) >= 2 and value[0] == '"' and value[-1] == '"': + return value[1:-1] + return value + + +def _get_scope_header( + scope: dict[str, Any], header_name: bytes +) -> Optional[str]: + """Return the first matching header value from an ASGI scope.""" + for candidate_name, candidate_value in scope.get("headers", []): + if candidate_name == header_name: + return candidate_value.decode("latin-1").split(",", 1)[0].strip() + return None + + +def _get_request_origin(scope: dict[str, Any]) -> Optional[str]: + """Compute the effective origin for the current HTTP/WebSocket request.""" + forwarded = _get_scope_header(scope, b"forwarded") + if forwarded is not None: + proto = None + host = None + for element in forwarded.split(",", 1)[0].split(";"): + if "=" not in element: + continue + name, value = element.split("=", 1) + if name.strip().lower() == "proto": + proto = _strip_optional_quotes(value.strip()) + elif name.strip().lower() == "host": + host = _strip_optional_quotes(value.strip()) + if proto is not None and host is not None: + return f"{_normalize_origin_scheme(proto)}://{host}" + + host = _get_scope_header(scope, b"x-forwarded-host") + if host is None: + host = _get_scope_header(scope, b"host") + if host is None: + return None + + proto = _get_scope_header(scope, b"x-forwarded-proto") + if proto is None: + proto = scope.get("scheme", "http") + return f"{_normalize_origin_scheme(proto)}://{host}" + + +def _is_request_origin_allowed( + origin: str, + scope: dict[str, Any], + allowed_literal_origins: list[str], + allowed_origin_regex: Optional[re.Pattern[str]], + has_configured_allowed_origins: bool, +) -> bool: + """Validate an Origin header against explicit config or same-origin.""" + if has_configured_allowed_origins and _is_origin_allowed( + origin, allowed_literal_origins, allowed_origin_regex + ): + return True + + request_origin = _get_request_origin(scope) + if request_origin is None: + return False + return origin == request_origin + + +_SAFE_HTTP_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) + + +class _OriginCheckMiddleware: + """ASGI middleware that blocks cross-origin state-changing requests.""" + + def __init__( + self, + app: Any, + has_configured_allowed_origins: bool, + allowed_origins: list[str], + allowed_origin_regex: Optional[re.Pattern[str]], + ) -> None: + self._app = app + self._has_configured_allowed_origins = has_configured_allowed_origins + self._allowed_origins = allowed_origins + self._allowed_origin_regex = allowed_origin_regex + + async def __call__( + self, + scope: dict[str, Any], + receive: Any, + send: Any, + ) -> None: + if scope["type"] != "http": + await self._app(scope, receive, send) + return + + method = scope.get("method", "GET") + if method in _SAFE_HTTP_METHODS: + await self._app(scope, receive, send) + return + + origin = _get_scope_header(scope, b"origin") + if origin is None: + await self._app(scope, receive, send) + return + + if _is_request_origin_allowed( + origin, + scope, + self._allowed_origins, + self._allowed_origin_regex, + self._has_configured_allowed_origins, + ): + await self._app(scope, receive, send) + return + + response_body = b"Forbidden: origin not allowed" + await send({ + "type": "http.response.start", + "status": 403, + "headers": [ + (b"content-type", b"text/plain"), + (b"content-length", str(len(response_body)).encode()), + ], + }) + await send({ + "type": "http.response.body", + "body": response_body, + }) + + class ApiServerSpanExporter(export_lib.SpanExporter): def __init__(self, trace_dict): @@ -759,8 +912,12 @@ async def internal_lifespan(app: FastAPI): # Run the FastAPI server. app = FastAPI(lifespan=internal_lifespan) + has_configured_allowed_origins = bool(allow_origins) if allow_origins: literal_origins, combined_regex = _parse_cors_origins(allow_origins) + compiled_origin_regex = ( + re.compile(combined_regex) if combined_regex is not None else None + ) app.add_middleware( CORSMiddleware, allow_origins=literal_origins, @@ -769,6 +926,16 @@ async def internal_lifespan(app: FastAPI): allow_methods=["*"], allow_headers=["*"], ) + else: + literal_origins = [] + compiled_origin_regex = None + + app.add_middleware( + _OriginCheckMiddleware, + has_configured_allowed_origins=has_configured_allowed_origins, + allowed_origins=literal_origins, + allowed_origin_regex=compiled_origin_regex, + ) @app.get("/health") async def health() -> dict[str, str]: @@ -1755,14 +1922,23 @@ async def run_agent_live( enable_affective_dialog: bool | None = Query(default=None), enable_session_resumption: bool | None = Query(default=None), ) -> None: + ws_origin = websocket.headers.get("origin") + if ws_origin is not None and not _is_request_origin_allowed( + ws_origin, + websocket.scope, + literal_origins, + compiled_origin_regex, + has_configured_allowed_origins, + ): + await websocket.close(code=1008, reason="Origin not allowed") + return + await websocket.accept() session = await self.session_service.get_session( app_name=app_name, user_id=user_id, session_id=session_id ) if not session: - # Accept first so that the client is aware of connection establishment, - # then close with a specific code. await websocket.close(code=1002, reason="Session not found") return diff --git a/tests/unittests/cli/test_adk_web_server_run_live.py b/tests/unittests/cli/test_adk_web_server_run_live.py index 1c3c42593c..012af92d56 100644 --- a/tests/unittests/cli/test_adk_web_server_run_live.py +++ b/tests/unittests/cli/test_adk_web_server_run_live.py @@ -22,6 +22,7 @@ from google.adk.events.event import Event from google.adk.sessions.in_memory_session_service import InMemorySessionService import pytest +from starlette.websockets import WebSocketDisconnect class _DummyAgent(BaseAgent): @@ -203,3 +204,75 @@ async def _get_runner_async(_self, _app_name: str): run_config.session_resumption.transparent is expected_session_resumption_transparent ) + + +_WS_BASE_URL = ( + "/run_live" + "?app_name=test_app" + "&user_id=user" + "&session_id=session" + "&modalities=AUDIO" +) + + +def _build_ws_client(): + """Build a TestClient wired to a capturing runner.""" + session_service = InMemorySessionService() + asyncio.run( + session_service.create_session( + app_name="test_app", + user_id="user", + session_id="session", + state={}, + ) + ) + + runner = _CapturingRunner() + adk_web_server = AdkWebServer( + agent_loader=_DummyAgentLoader(), + session_service=session_service, + memory_service=types.SimpleNamespace(), + artifact_service=types.SimpleNamespace(), + credential_service=types.SimpleNamespace(), + eval_sets_manager=types.SimpleNamespace(), + eval_set_results_manager=types.SimpleNamespace(), + agents_dir=".", + ) + + async def _get_runner_async(_self, _app_name: str): + return runner + + adk_web_server.get_runner_async = _get_runner_async.__get__(adk_web_server) # pytype: disable=attribute-error + + fast_api_app = adk_web_server.get_fast_api_app( + setup_observer=lambda _observer, _server: None, + tear_down_observer=lambda _observer, _server: None, + ) + return TestClient(fast_api_app) + + +def test_run_live_rejects_disallowed_origin(): + client = _build_ws_client() + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect( + _WS_BASE_URL, + headers={"origin": "https://evil.com"}, + ) as ws: + ws.receive_text() + assert exc_info.value.code == 1008 + + +def test_run_live_allows_matching_origin(): + client = _build_ws_client() + with client.websocket_connect( + _WS_BASE_URL, + headers={"origin": "http://testserver"}, + ) as ws: + _ = ws.receive_text() + + +def test_run_live_allows_no_origin_header(): + """Non-browser clients (curl, wscat, SDKs) send no Origin header.""" + client = _build_ws_client() + with client.websocket_connect(_WS_BASE_URL) as ws: + _ = ws.receive_text() diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index 0ea28e6683..13b5a670e9 100755 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -593,7 +593,7 @@ def builder_test_client( session_service_uri="", artifact_service_uri="", memory_service_uri="", - allow_origins=["*"], + allow_origins=None, a2a=False, host="127.0.0.1", port=8000, @@ -1595,6 +1595,46 @@ def test_builder_final_save_preserves_tools_and_cleans_tmp( assert not tmp_dir.exists() or not any(tmp_dir.iterdir()) +def test_builder_save_rejects_cross_origin_post(builder_test_client, tmp_path): + response = builder_test_client.post( + "/builder/save?tmp=true", + headers={"origin": "https://evil.com"}, + files=[( + "files", + ("app/root_agent.yaml", b"name: app\n", "application/x-yaml"), + )], + ) + + assert response.status_code == 403 + assert response.text == "Forbidden: origin not allowed" + assert not (tmp_path / "app" / "tmp" / "app").exists() + + +def test_builder_save_allows_same_origin_post(builder_test_client, tmp_path): + response = builder_test_client.post( + "/builder/save?tmp=true", + headers={"origin": "http://testserver"}, + files=[( + "files", + ("app/root_agent.yaml", b"name: app\n", "application/x-yaml"), + )], + ) + + assert response.status_code == 200 + assert response.json() is True + assert (tmp_path / "app" / "tmp" / "app" / "root_agent.yaml").is_file() + + +def test_builder_get_allows_cross_origin_get(builder_test_client): + response = builder_test_client.get( + "/builder/app/missing?tmp=true", + headers={"origin": "https://evil.com"}, + ) + + assert response.status_code == 200 + assert response.text == "" + + def test_builder_cancel_deletes_tmp_idempotent(builder_test_client, tmp_path): tmp_agent_root = tmp_path / "app" / "tmp" / "app" tmp_agent_root.mkdir(parents=True, exist_ok=True) From 584283e2d8209af9db719fdeae506892d4a44ca2 Mon Sep 17 00:00:00 2001 From: Sasha Sobran Date: Tue, 24 Mar 2026 14:16:10 -0700 Subject: [PATCH 3/7] fix: gate builder endpoints behind web flag Co-authored-by: Sasha Sobran PiperOrigin-RevId: 888850792 --- src/google/adk/cli/fast_api.py | 443 ++++++++++++++------------- tests/unittests/cli/test_fast_api.py | 116 ++++++- 2 files changed, 343 insertions(+), 216 deletions(-) diff --git a/src/google/adk/cli/fast_api.py b/src/google/adk/cli/fast_api.py index 8f78c15f9b..4d666f78d3 100644 --- a/src/google/adk/cli/fast_api.py +++ b/src/google/adk/cli/fast_api.py @@ -266,150 +266,192 @@ def tear_down_observer(observer: Observer, _: AdkWebServer): **extra_fast_api_args, ) - agents_base_path = (Path.cwd() / agents_dir).resolve() - - def _get_app_root(app_name: str) -> Path: - if app_name in ("", ".", ".."): - raise ValueError(f"Invalid app name: {app_name!r}") - if Path(app_name).name != app_name or "\\" in app_name: - raise ValueError(f"Invalid app name: {app_name!r}") - app_root = (agents_base_path / app_name).resolve() - if not app_root.is_relative_to(agents_base_path): - raise ValueError(f"Invalid app name: {app_name!r}") - return app_root - - def _normalize_relative_path(path: str) -> str: - return path.replace("\\", "/").lstrip("/") - - def _has_parent_reference(path: str) -> bool: - return any(part == ".." for part in path.split("/")) - - def _parse_upload_filename(filename: Optional[str]) -> tuple[str, str]: - if not filename: - raise ValueError("Upload filename is missing.") - filename = _normalize_relative_path(filename) - if "/" not in filename: - raise ValueError(f"Invalid upload filename: {filename!r}") - app_name, rel_path = filename.split("/", 1) - if not app_name or not rel_path: - raise ValueError(f"Invalid upload filename: {filename!r}") - if rel_path.startswith("/"): - raise ValueError(f"Absolute upload path rejected: {filename!r}") - if _has_parent_reference(rel_path): - raise ValueError(f"Path traversal rejected: {filename!r}") - return app_name, rel_path - - def _parse_file_path(file_path: str) -> str: - file_path = _normalize_relative_path(file_path) - if not file_path: - raise ValueError("file_path is missing.") - if file_path.startswith("/"): - raise ValueError(f"Absolute file_path rejected: {file_path!r}") - if _has_parent_reference(file_path): - raise ValueError(f"Path traversal rejected: {file_path!r}") - return file_path - - def _resolve_under_dir(root_dir: Path, rel_path: str) -> Path: - file_path = root_dir / rel_path - resolved_root_dir = root_dir.resolve() - resolved_file_path = file_path.resolve() - if not resolved_file_path.is_relative_to(resolved_root_dir): - raise ValueError(f"Path escapes root_dir: {rel_path!r}") - return file_path - - def _get_tmp_agent_root(app_root: Path, app_name: str) -> Path: - tmp_agent_root = app_root / "tmp" / app_name - resolved_tmp_agent_root = tmp_agent_root.resolve() - if not resolved_tmp_agent_root.is_relative_to(app_root): - raise ValueError(f"Invalid tmp path for app: {app_name!r}") - return tmp_agent_root - - def copy_dir_contents(source_dir: Path, dest_dir: Path) -> None: - dest_dir.mkdir(parents=True, exist_ok=True) - for source_path in source_dir.iterdir(): - if source_path.name == "tmp": - continue - - dest_path = dest_dir / source_path.name - if source_path.is_dir(): - if dest_path.exists() and dest_path.is_file(): - dest_path.unlink() - shutil.copytree(source_path, dest_path, dirs_exist_ok=True) - elif source_path.is_file(): - if dest_path.exists() and dest_path.is_dir(): - shutil.rmtree(dest_path) - shutil.copy2(source_path, dest_path) - - def cleanup_tmp(app_name: str) -> bool: - try: - app_root = _get_app_root(app_name) - except ValueError as exc: - logger.exception("Error in cleanup_tmp: %s", exc) - return False - - try: - tmp_agent_root = _get_tmp_agent_root(app_root, app_name) - except ValueError as exc: - logger.exception("Error in cleanup_tmp: %s", exc) - return False - - try: - shutil.rmtree(tmp_agent_root) - except FileNotFoundError: - pass - except OSError as exc: - logger.exception("Error deleting tmp agent root: %s", exc) - return False - - tmp_dir = app_root / "tmp" - resolved_tmp_dir = tmp_dir.resolve() - if not resolved_tmp_dir.is_relative_to(app_root): - logger.error( - "Refusing to delete tmp outside app_root: %s", resolved_tmp_dir - ) - return False + # --- Builder endpoints (agent editor UI) --- + # Only register when the web UI is enabled. In headless / production + # deployments (e.g. `adk deploy cloud_run`) these endpoints are unnecessary + # and expose an attack surface that allows arbitrary file writes under the + # agents directory. + # See https://github.com/google/adk-python/issues/4947 + if web: + agents_base_path = (Path.cwd() / agents_dir).resolve() + + def _get_app_root(app_name: str) -> Path: + if app_name in ("", ".", ".."): + raise ValueError(f"Invalid app name: {app_name!r}") + if Path(app_name).name != app_name or "\\" in app_name: + raise ValueError(f"Invalid app name: {app_name!r}") + app_root = (agents_base_path / app_name).resolve() + if not app_root.is_relative_to(agents_base_path): + raise ValueError(f"Invalid app name: {app_name!r}") + return app_root + + def _normalize_relative_path(path: str) -> str: + return path.replace("\\", "/").lstrip("/") + + def _has_parent_reference(path: str) -> bool: + return any(part == ".." for part in path.split("/")) + + _ALLOWED_UPLOAD_EXTENSIONS = frozenset({".yaml", ".yml"}) + + def _parse_upload_filename(filename: Optional[str]) -> tuple[str, str]: + if not filename: + raise ValueError("Upload filename is missing.") + filename = _normalize_relative_path(filename) + if "/" not in filename: + raise ValueError(f"Invalid upload filename: {filename!r}") + app_name, rel_path = filename.split("/", 1) + if not app_name or not rel_path: + raise ValueError(f"Invalid upload filename: {filename!r}") + if rel_path.startswith("/"): + raise ValueError(f"Absolute upload path rejected: {filename!r}") + if _has_parent_reference(rel_path): + raise ValueError(f"Path traversal rejected: {filename!r}") + ext = os.path.splitext(rel_path)[1].lower() + if ext not in _ALLOWED_UPLOAD_EXTENSIONS: + raise ValueError( + f"File type not allowed: {rel_path!r}" + f" (allowed: {', '.join(sorted(_ALLOWED_UPLOAD_EXTENSIONS))})" + ) + return app_name, rel_path + + def _parse_file_path(file_path: str) -> str: + file_path = _normalize_relative_path(file_path) + if not file_path: + raise ValueError("file_path is missing.") + if file_path.startswith("/"): + raise ValueError(f"Absolute file_path rejected: {file_path!r}") + if _has_parent_reference(file_path): + raise ValueError(f"Path traversal rejected: {file_path!r}") + return file_path + + def _resolve_under_dir(root_dir: Path, rel_path: str) -> Path: + file_path = root_dir / rel_path + resolved_root_dir = root_dir.resolve() + resolved_file_path = file_path.resolve() + if not resolved_file_path.is_relative_to(resolved_root_dir): + raise ValueError(f"Path escapes root_dir: {rel_path!r}") + return file_path + + def _get_tmp_agent_root(app_root: Path, app_name: str) -> Path: + tmp_agent_root = app_root / "tmp" / app_name + resolved_tmp_agent_root = tmp_agent_root.resolve() + if not resolved_tmp_agent_root.is_relative_to(app_root): + raise ValueError(f"Invalid tmp path for app: {app_name!r}") + return tmp_agent_root + + def copy_dir_contents(source_dir: Path, dest_dir: Path) -> None: + dest_dir.mkdir(parents=True, exist_ok=True) + for source_path in source_dir.iterdir(): + if source_path.name == "tmp": + continue - try: - tmp_dir.rmdir() - except OSError: - pass + dest_path = dest_dir / source_path.name + if source_path.is_dir(): + if dest_path.exists() and dest_path.is_file(): + dest_path.unlink() + shutil.copytree(source_path, dest_path, dirs_exist_ok=True) + elif source_path.is_file(): + if dest_path.exists() and dest_path.is_dir(): + shutil.rmtree(dest_path) + shutil.copy2(source_path, dest_path) + + def cleanup_tmp(app_name: str) -> bool: + try: + app_root = _get_app_root(app_name) + except ValueError as exc: + logger.exception("Error in cleanup_tmp: %s", exc) + return False - return True + try: + tmp_agent_root = _get_tmp_agent_root(app_root, app_name) + except ValueError as exc: + logger.exception("Error in cleanup_tmp: %s", exc) + return False - def ensure_tmp_exists(app_name: str) -> bool: - try: - app_root = _get_app_root(app_name) - except ValueError as exc: - logger.exception("Error in ensure_tmp_exists: %s", exc) - return False + try: + shutil.rmtree(tmp_agent_root) + except FileNotFoundError: + pass + except OSError as exc: + logger.exception("Error deleting tmp agent root: %s", exc) + return False - if not app_root.is_dir(): - return False + tmp_dir = app_root / "tmp" + resolved_tmp_dir = tmp_dir.resolve() + if not resolved_tmp_dir.is_relative_to(app_root): + logger.error( + "Refusing to delete tmp outside app_root: %s", resolved_tmp_dir + ) + return False - try: - tmp_agent_root = _get_tmp_agent_root(app_root, app_name) - except ValueError as exc: - logger.exception("Error in ensure_tmp_exists: %s", exc) - return False + try: + tmp_dir.rmdir() + except OSError: + pass - if tmp_agent_root.exists(): return True - try: - tmp_agent_root.mkdir(parents=True, exist_ok=True) - copy_dir_contents(app_root, tmp_agent_root) - except OSError as exc: - logger.exception("Error in ensure_tmp_exists: %s", exc) - return False + def ensure_tmp_exists(app_name: str) -> bool: + try: + app_root = _get_app_root(app_name) + except ValueError as exc: + logger.exception("Error in ensure_tmp_exists: %s", exc) + return False - return True + if not app_root.is_dir(): + return False + + try: + tmp_agent_root = _get_tmp_agent_root(app_root, app_name) + except ValueError as exc: + logger.exception("Error in ensure_tmp_exists: %s", exc) + return False + + if tmp_agent_root.exists(): + return True + + try: + tmp_agent_root.mkdir(parents=True, exist_ok=True) + copy_dir_contents(app_root, tmp_agent_root) + except OSError as exc: + logger.exception("Error in ensure_tmp_exists: %s", exc) + return False + + return True + + @app.post("/builder/save", response_model_exclude_none=True) + async def builder_build( + files: list[UploadFile], tmp: Optional[bool] = False + ) -> bool: + try: + if tmp: + app_names = set() + uploads = [] + for file in files: + app_name, rel_path = _parse_upload_filename(file.filename) + app_names.add(app_name) + uploads.append((rel_path, file)) + + if len(app_names) != 1: + logger.error( + "Exactly one app name is required, found: %s", + sorted(app_names), + ) + return False + + app_name = next(iter(app_names)) + app_root = _get_app_root(app_name) + tmp_agent_root = _get_tmp_agent_root(app_root, app_name) + tmp_agent_root.mkdir(parents=True, exist_ok=True) + + for rel_path, file in uploads: + destination_path = _resolve_under_dir(tmp_agent_root, rel_path) + destination_path.parent.mkdir(parents=True, exist_ok=True) + with destination_path.open("wb") as buffer: + shutil.copyfileobj(file.file, buffer) + + return True - @app.post("/builder/save", response_model_exclude_none=True) - async def builder_build( - files: list[UploadFile], tmp: Optional[bool] = False - ) -> bool: - try: - if tmp: app_names = set() uploads = [] for file in files: @@ -419,108 +461,85 @@ async def builder_build( if len(app_names) != 1: logger.error( - "Exactly one app name is required, found: %s", sorted(app_names) + "Exactly one app name is required, found: %s", + sorted(app_names), ) return False app_name = next(iter(app_names)) app_root = _get_app_root(app_name) + app_root.mkdir(parents=True, exist_ok=True) + tmp_agent_root = _get_tmp_agent_root(app_root, app_name) - tmp_agent_root.mkdir(parents=True, exist_ok=True) + if tmp_agent_root.is_dir(): + copy_dir_contents(tmp_agent_root, app_root) for rel_path, file in uploads: - destination_path = _resolve_under_dir(tmp_agent_root, rel_path) + destination_path = _resolve_under_dir(app_root, rel_path) destination_path.parent.mkdir(parents=True, exist_ok=True) with destination_path.open("wb") as buffer: shutil.copyfileobj(file.file, buffer) - return True - - app_names = set() - uploads = [] - for file in files: - app_name, rel_path = _parse_upload_filename(file.filename) - app_names.add(app_name) - uploads.append((rel_path, file)) - - if len(app_names) != 1: - logger.error( - "Exactly one app name is required, found: %s", sorted(app_names) - ) + return cleanup_tmp(app_name) + except ValueError as exc: + logger.exception("Error in builder_build: %s", exc) + return False + except OSError as exc: + logger.exception("Error in builder_build: %s", exc) return False - app_name = next(iter(app_names)) - app_root = _get_app_root(app_name) - app_root.mkdir(parents=True, exist_ok=True) + @app.post( + "/builder/app/{app_name}/cancel", response_model_exclude_none=True + ) + async def builder_cancel(app_name: str) -> bool: + return cleanup_tmp(app_name) - tmp_agent_root = _get_tmp_agent_root(app_root, app_name) - if tmp_agent_root.is_dir(): - copy_dir_contents(tmp_agent_root, app_root) + @app.get( + "/builder/app/{app_name}", + response_model_exclude_none=True, + response_class=PlainTextResponse, + ) + async def get_agent_builder( + app_name: str, + file_path: Optional[str] = None, + tmp: Optional[bool] = False, + ): + try: + app_root = _get_app_root(app_name) + except ValueError as exc: + logger.exception("Error in get_agent_builder: %s", exc) + return "" - for rel_path, file in uploads: - destination_path = _resolve_under_dir(app_root, rel_path) - destination_path.parent.mkdir(parents=True, exist_ok=True) - with destination_path.open("wb") as buffer: - shutil.copyfileobj(file.file, buffer) + agent_dir = app_root + if tmp: + if not ensure_tmp_exists(app_name): + return "" + agent_dir = app_root / "tmp" / app_name - return cleanup_tmp(app_name) - except ValueError as exc: - logger.exception("Error in builder_build: %s", exc) - return False - except OSError as exc: - logger.exception("Error in builder_build: %s", exc) - return False - - @app.post("/builder/app/{app_name}/cancel", response_model_exclude_none=True) - async def builder_cancel(app_name: str) -> bool: - return cleanup_tmp(app_name) - - @app.get( - "/builder/app/{app_name}", - response_model_exclude_none=True, - response_class=PlainTextResponse, - ) - async def get_agent_builder( - app_name: str, - file_path: Optional[str] = None, - tmp: Optional[bool] = False, - ): - try: - app_root = _get_app_root(app_name) - except ValueError as exc: - logger.exception("Error in get_agent_builder: %s", exc) - return "" - - agent_dir = app_root - if tmp: - if not ensure_tmp_exists(app_name): - return "" - agent_dir = app_root / "tmp" / app_name + if not file_path: + rel_path = "root_agent.yaml" + else: + try: + rel_path = _parse_file_path(file_path) + except ValueError as exc: + logger.exception("Error in get_agent_builder: %s", exc) + return "" - if not file_path: - rel_path = "root_agent.yaml" - else: try: - rel_path = _parse_file_path(file_path) + agent_file_path = _resolve_under_dir(agent_dir, rel_path) except ValueError as exc: logger.exception("Error in get_agent_builder: %s", exc) return "" - try: - agent_file_path = _resolve_under_dir(agent_dir, rel_path) - except ValueError as exc: - logger.exception("Error in get_agent_builder: %s", exc) - return "" - - if not agent_file_path.is_file(): - return "" + if not agent_file_path.is_file(): + return "" - return FileResponse( - path=agent_file_path, - media_type="application/x-yaml", - filename=file_path or f"{app_name}.yaml", - headers={"Cache-Control": "no-store"}, - ) + return FileResponse( + path=agent_file_path, + media_type="application/x-yaml", + filename=file_path or f"{app_name}.yaml", + headers={"Cache-Control": "no-store"}, + ) if a2a: from a2a.server.apps import A2AStarletteApplication diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index 13b5a670e9..e53d5a918c 100755 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -1560,16 +1560,18 @@ def test_patch_memory(test_app, create_test_session, mock_memory_service): logger.info("Add session to memory test completed successfully") -def test_builder_final_save_preserves_tools_and_cleans_tmp( +def test_builder_final_save_preserves_files_and_cleans_tmp( builder_test_client, tmp_path ): files = [ - ("files", ("app/__init__.py", b"from . import agent\n", "text/plain")), - ("files", ("app/tools.py", b"def tool():\n return 1\n", "text/plain")), ( "files", ("app/root_agent.yaml", b"name: app\n", "application/x-yaml"), ), + ( + "files", + ("app/sub_agent.yaml", b"name: sub\n", "application/x-yaml"), + ), ] response = builder_test_client.post("/builder/save?tmp=true", files=files) assert response.status_code == 200 @@ -1589,7 +1591,7 @@ def test_builder_final_save_preserves_tools_and_cleans_tmp( assert response.status_code == 200 assert response.json() is True - assert (tmp_path / "app" / "tools.py").is_file() + assert (tmp_path / "app" / "sub_agent.yaml").is_file() assert not (tmp_path / "app" / "tmp" / "app").exists() tmp_dir = tmp_path / "app" / "tmp" assert not tmp_dir.exists() or not any(tmp_dir.iterdir()) @@ -1698,6 +1700,112 @@ def test_builder_save_rejects_traversal(builder_test_client, tmp_path): assert not (tmp_path / "app" / "tmp" / "escape.yaml").exists() +def test_builder_save_rejects_py_files(builder_test_client, tmp_path): + """Uploading .py files via /builder/save is rejected.""" + response = builder_test_client.post( + "/builder/save?tmp=true", + files=[( + "files", + ("app/agent.py", b"import os\nos.system('id')\n", "text/plain"), + )], + ) + assert response.status_code == 200 + assert response.json() is False + assert not (tmp_path / "app" / "tmp" / "app" / "agent.py").exists() + + +def test_builder_save_rejects_non_yaml_extensions( + builder_test_client, tmp_path +): + """Uploading non-YAML files (.json, .txt, .sh, etc.) is rejected.""" + for ext, content in [ + (".py", b"print('hi')"), + (".json", b"{}"), + (".txt", b"hello"), + (".sh", b"#!/bin/bash"), + (".pth", b"import os"), + ]: + response = builder_test_client.post( + "/builder/save?tmp=true", + files=[( + "files", + (f"app/file{ext}", content, "application/octet-stream"), + )], + ) + assert response.status_code == 200, f"Expected 200 for {ext}" + assert response.json() is False, f"Expected False for {ext}" + + +def test_builder_save_allows_yaml_files(builder_test_client, tmp_path): + """Uploading .yaml and .yml files is allowed.""" + response = builder_test_client.post( + "/builder/save?tmp=true", + files=[( + "files", + ("app/root_agent.yaml", b"name: app\n", "application/x-yaml"), + )], + ) + assert response.status_code == 200 + assert response.json() is True + + response = builder_test_client.post( + "/builder/save?tmp=true", + files=[( + "files", + ("app/sub_agent.yml", b"name: sub\n", "application/x-yaml"), + )], + ) + assert response.status_code == 200 + assert response.json() is True + + +def test_builder_endpoints_not_registered_without_web( + mock_session_service, + mock_artifact_service, + mock_memory_service, + mock_agent_loader, + mock_eval_sets_manager, + mock_eval_set_results_manager, +): + """Builder endpoints must not be registered when web=False (e.g. deploy).""" + client = _create_test_client( + mock_session_service, + mock_artifact_service, + mock_memory_service, + mock_agent_loader, + mock_eval_sets_manager, + mock_eval_set_results_manager, + web=False, + ) + # /builder/save should return 404/405, not 200 + response = client.post( + "/builder/save", + files=[ + ("files", ("app/agent.yaml", b"name: test\n", "application/x-yaml")) + ], + ) + assert response.status_code in (404, 405) + + # /builder/app/{name}/cancel should also be absent + response = client.post("/builder/app/app/cancel") + assert response.status_code in (404, 405) + + # /builder/app/{name} GET should also be absent + response = client.get("/builder/app/app") + assert response.status_code in (404, 405) + + +def test_builder_endpoints_registered_with_web(builder_test_client): + """Builder endpoints are available when web=True.""" + response = builder_test_client.post( + "/builder/save?tmp=true", + files=[ + ("files", ("app/agent.yaml", b"name: test\n", "application/x-yaml")) + ], + ) + assert response.status_code == 200 + + def test_agent_run_resume_without_message_success( test_app, create_test_session ): From be094f7e50e3976cb69468c7a8d17f3385132c41 Mon Sep 17 00:00:00 2001 From: Sasha Sobran Date: Thu, 26 Mar 2026 09:37:23 -0700 Subject: [PATCH 4/7] fix: add agent name validation to prevent arbitrary module imports Co-authored-by: Sasha Sobran PiperOrigin-RevId: 889890969 --- src/google/adk/cli/utils/agent_loader.py | 29 ++++++ .../unittests/cli/utils/test_agent_loader.py | 97 +++++++++++-------- 2 files changed, 84 insertions(+), 42 deletions(-) diff --git a/src/google/adk/cli/utils/agent_loader.py b/src/google/adk/cli/utils/agent_loader.py index efd24648dc..72bc4bf535 100644 --- a/src/google/adk/cli/utils/agent_loader.py +++ b/src/google/adk/cli/utils/agent_loader.py @@ -19,6 +19,7 @@ import logging import os from pathlib import Path +import re import sys from typing import Any from typing import Literal @@ -187,8 +188,36 @@ def _load_from_yaml_config( ) + e.args[1:] raise e + _VALID_AGENT_NAME_RE = re.compile(r"^[a-zA-Z0-9_]+$") + + def _validate_agent_name(self, agent_name: str) -> None: + """Validate agent name to prevent arbitrary module imports.""" + # Strip the special agent prefix for validation + if agent_name.startswith("__"): + name_to_check = agent_name[2:] + check_dir = os.path.abspath(SPECIAL_AGENTS_DIR) + else: + name_to_check = agent_name + check_dir = self.agents_dir + + if not self._VALID_AGENT_NAME_RE.match(name_to_check): + raise ValueError( + f"Invalid agent name: {agent_name!r}. Agent names must be valid" + " Python identifiers (letters, digits, and underscores only)." + ) + + # Verify the agent exists on disk before allowing import + agent_path = Path(check_dir) / name_to_check + agent_file = Path(check_dir) / f"{name_to_check}.py" + if not (agent_path.is_dir() or agent_file.is_file()): + raise ValueError( + f"Agent not found: {agent_name!r}. No matching directory or module" + f" exists in '{os.path.join(check_dir, name_to_check)}'." + ) + def _perform_load(self, agent_name: str) -> Union[BaseAgent, App]: """Internal logic to load an agent""" + self._validate_agent_name(agent_name) # Determine the directory to use for loading if agent_name.startswith("__"): # Special agent: use special agents directory diff --git a/tests/unittests/cli/utils/test_agent_loader.py b/tests/unittests/cli/utils/test_agent_loader.py index 0a7f9fc04f..bcf306ad49 100644 --- a/tests/unittests/cli/utils/test_agent_loader.py +++ b/tests/unittests/cli/utils/test_agent_loader.py @@ -299,8 +299,6 @@ def test_error_messages_use_os_sep_consistently(self): loader.load_agent(agent_name) exc_info.match(re.escape(expected_path)) - exc_info.match(re.escape(f"{agent_name}{os.sep}root_agent.yaml")) - exc_info.match(re.escape(f"{os.sep}")) def test_agent_loader_with_mocked_windows_path(self, monkeypatch): """Mock Path() to simulate Windows behavior and catch regressions. @@ -333,19 +331,10 @@ def test_agent_not_found_error(self): with pytest.raises(ValueError) as exc_info: loader.load_agent("nonexistent_agent") - expected_msg_part_1 = "No root_agent found for 'nonexistent_agent'." - expected_msg_part_2 = ( - "Searched in 'nonexistent_agent.agent.root_agent'," - " 'nonexistent_agent.root_agent' and" - " 'nonexistent_agent/root_agent.yaml'." + assert "Agent not found: 'nonexistent_agent'" in str(exc_info.value) + assert os.path.join(agents_dir, "nonexistent_agent") in str( + exc_info.value ) - expected_msg_part_3 = ( - f"Ensure '{agents_dir}/nonexistent_agent' is structured correctly" - ) - - assert expected_msg_part_1 in str(exc_info.value) - assert expected_msg_part_2 in str(exc_info.value) - assert expected_msg_part_3 in str(exc_info.value) def test_agent_without_root_agent_error(self): """Test that appropriate error is raised when agent has no root_agent.""" @@ -567,26 +556,16 @@ def test_yaml_agent_not_found_error(self): """Test that appropriate error is raised when YAML agent is not found.""" with tempfile.TemporaryDirectory() as temp_dir: loader = AgentLoader(temp_dir) - agents_dir = temp_dir # For use in the expected message string + agents_dir = temp_dir # Try to load nonexistent YAML agent with pytest.raises(ValueError) as exc_info: loader.load_agent("nonexistent_yaml_agent") - expected_msg_part_1 = "No root_agent found for 'nonexistent_yaml_agent'." - expected_msg_part_2 = ( - "Searched in 'nonexistent_yaml_agent.agent.root_agent'," - " 'nonexistent_yaml_agent.root_agent' and" - " 'nonexistent_yaml_agent/root_agent.yaml'." + assert "Agent not found: 'nonexistent_yaml_agent'" in str(exc_info.value) + assert os.path.join(agents_dir, "nonexistent_yaml_agent") in str( + exc_info.value ) - expected_msg_part_3 = ( - f"Ensure '{agents_dir}/nonexistent_yaml_agent' is structured" - " correctly" - ) - - assert expected_msg_part_1 in str(exc_info.value) - assert expected_msg_part_2 in str(exc_info.value) - assert expected_msg_part_3 in str(exc_info.value) def test_yaml_agent_invalid_yaml_error(self): """Test that appropriate error is raised when YAML is invalid.""" @@ -778,20 +757,7 @@ def test_special_agent_not_found_error(self): with pytest.raises(ValueError) as exc_info: loader.load_agent("__nonexistent_special") - expected_msg_part_1 = "No root_agent found for '__nonexistent_special'." - expected_msg_part_2 = ( - "Searched in 'nonexistent_special.agent.root_agent'," - " 'nonexistent_special.root_agent' and" - " 'nonexistent_special/root_agent.yaml'." - ) - expected_msg_part_3 = ( - f"Ensure '{special_agents_dir}/nonexistent_special' is structured" - " correctly" - ) - - assert expected_msg_part_1 in str(exc_info.value) - assert expected_msg_part_2 in str(exc_info.value) - assert expected_msg_part_3 in str(exc_info.value) + assert "Agent not found: '__nonexistent_special'" in str(exc_info.value) finally: # Restore original SPECIAL_AGENTS_DIR @@ -1038,3 +1004,50 @@ def __init__(self): assert "random_folder" not in agents assert "data" not in agents assert "tmp" not in agents + + def test_validate_agent_name_rejects_dotted_paths(self): + """Agent names with dots are rejected to prevent arbitrary module imports.""" + with tempfile.TemporaryDirectory() as temp_dir: + loader = AgentLoader(temp_dir) + for name in ["os.path", "sys.modules", "subprocess.call"]: + with pytest.raises(ValueError, match="Invalid agent name"): + loader.load_agent(name) + + def test_validate_agent_name_rejects_relative_imports(self): + """Agent names starting with dots are rejected.""" + with tempfile.TemporaryDirectory() as temp_dir: + loader = AgentLoader(temp_dir) + for name in ["..foo", ".bar", "...baz"]: + with pytest.raises(ValueError, match="Invalid agent name"): + loader.load_agent(name) + + def test_validate_agent_name_rejects_path_separators(self): + """Agent names with slashes or special characters are rejected.""" + with tempfile.TemporaryDirectory() as temp_dir: + loader = AgentLoader(temp_dir) + for name in ["foo/bar", "foo\\bar", "foo-bar", "foo bar"]: + with pytest.raises(ValueError, match="Invalid agent name"): + loader.load_agent(name) + + def test_validate_agent_name_allows_valid_names(self): + """Valid Python identifiers that exist on disk pass validation.""" + with tempfile.TemporaryDirectory() as temp_dir: + temp_path = Path(temp_dir) + for name in ["my_agent", "Agent1", "_private"]: + (temp_path / name).mkdir(exist_ok=True) + loader = AgentLoader(temp_dir) + for name in ["my_agent", "Agent1", "_private"]: + # Should not raise ValueError for name validation; + # may raise other errors because the agent has no root_agent + with pytest.raises(Exception) as exc_info: + loader.load_agent(name) + assert "Invalid agent name" not in str(exc_info.value) + assert "Agent not found" not in str(exc_info.value) + + def test_validate_agent_name_rejects_nonexistent_agent(self): + """Valid identifiers that don't exist on disk are rejected before import.""" + with tempfile.TemporaryDirectory() as temp_dir: + loader = AgentLoader(temp_dir) + # 'subprocess' is a valid identifier but shouldn't be importable as an agent + with pytest.raises(ValueError, match="Agent not found"): + loader.load_agent("subprocess") From 27aed23edddcc8c69c4c4563198c8570aa40743e Mon Sep 17 00:00:00 2001 From: Sasha Sobran Date: Thu, 26 Mar 2026 13:42:57 -0700 Subject: [PATCH 5/7] fix: Default to ClusterIP so GKE deployment isn't publicly exposed by default Co-authored-by: Sasha Sobran PiperOrigin-RevId: 890025323 --- src/google/adk/cli/cli_deploy.py | 15 ++++++++++++++- src/google/adk/cli/cli_tools_click.py | 13 +++++++++++++ tests/unittests/cli/utils/test_cli_deploy.py | 2 +- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/google/adk/cli/cli_deploy.py b/src/google/adk/cli/cli_deploy.py index 465ca6c3e1..ed94a7cd6d 100644 --- a/src/google/adk/cli/cli_deploy.py +++ b/src/google/adk/cli/cli_deploy.py @@ -22,6 +22,7 @@ import sys import traceback from typing import Final +from typing import Literal from typing import Optional import warnings @@ -1162,6 +1163,9 @@ def to_gke( memory_service_uri: Optional[str] = None, use_local_storage: bool = False, a2a: bool = False, + service_type: Literal[ + 'ClusterIP', 'NodePort', 'LoadBalancer' + ] = 'ClusterIP', ): """Deploys an agent to Google Kubernetes Engine(GKE). @@ -1189,6 +1193,7 @@ def to_gke( artifact_service_uri: The URI of the artifact service. memory_service_uri: The URI of the memory service. use_local_storage: Whether to use local .adk storage in the container. + service_type: The Kubernetes Service type (default: ClusterIP). """ click.secho( '\nšŸš€ Starting ADK Agent Deployment to GKE...', fg='cyan', bold=True @@ -1326,7 +1331,7 @@ def to_gke( metadata: name: {service_name} spec: - type: LoadBalancer + type: {service_type} selector: app: {service_name} ports: @@ -1380,3 +1385,11 @@ def to_gke( click.secho( '\nšŸŽ‰ Deployment to GKE finished successfully!', fg='cyan', bold=True ) + if service_type == 'ClusterIP': + click.echo( + '\nThe service is only reachable from within the cluster.' + ' To access it locally, run:' + f'\n kubectl port-forward svc/{service_name} {port}:{port}' + '\n\nTo expose the service externally, add a Gateway or' + ' re-deploy with --service_type=LoadBalancer.' + ) diff --git a/src/google/adk/cli/cli_tools_click.py b/src/google/adk/cli/cli_tools_click.py index 3565714139..b2660cd8a1 100644 --- a/src/google/adk/cli/cli_tools_click.py +++ b/src/google/adk/cli/cli_tools_click.py @@ -2209,6 +2209,17 @@ def cli_deploy_agent_engine( default="INFO", help="Optional. Set the logging level", ) +@click.option( + "--service_type", + type=click.Choice(["ClusterIP", "LoadBalancer"], case_sensitive=True), + default="ClusterIP", + show_default=True, + help=( + "Optional. The Kubernetes Service type for the deployed agent." + " ClusterIP (default) keeps the service cluster-internal;" + " use LoadBalancer to expose a public IP." + ), +) @click.option( "--temp_folder", type=str, @@ -2252,6 +2263,7 @@ def cli_deploy_gke( otel_to_cloud: bool, with_ui: bool, adk_version: str, + service_type: str, log_level: Optional[str] = None, session_service_uri: Optional[str] = None, artifact_service_uri: Optional[str] = None, @@ -2283,6 +2295,7 @@ def cli_deploy_gke( with_ui=with_ui, log_level=log_level, adk_version=adk_version, + service_type=service_type, session_service_uri=session_service_uri, artifact_service_uri=artifact_service_uri, memory_service_uri=memory_service_uri, diff --git a/tests/unittests/cli/utils/test_cli_deploy.py b/tests/unittests/cli/utils/test_cli_deploy.py index e1829d2f98..22b029a976 100644 --- a/tests/unittests/cli/utils/test_cli_deploy.py +++ b/tests/unittests/cli/utils/test_cli_deploy.py @@ -508,7 +508,7 @@ def mock_subprocess_run(*args, **kwargs): assert "image: gcr.io/gke-proj/gke-svc" in yaml_content assert f"containerPort: 9090" in yaml_content assert f"targetPort: 9090" in yaml_content - assert "type: LoadBalancer" in yaml_content + assert "type: ClusterIP" in yaml_content # 4. Verify cleanup assert str(rmtree_recorder.get_last_call_args()[0]) == str(tmp_path) From a86d0aa7d70d1c70346eceef591d255ed96711c8 Mon Sep 17 00:00:00 2001 From: Sasha Sobran Date: Wed, 25 Mar 2026 14:54:39 -0700 Subject: [PATCH 6/7] fix: Update eval extras to Vertex SDK package version with constrained LiteLLM upperbound Co-authored-by: Sasha Sobran PiperOrigin-RevId: 889449878 --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index ddeb50bf21..4717166ddd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -110,7 +110,7 @@ eval = [ # go/keep-sorted start "Jinja2>=3.1.4,<4.0.0", # For eval template rendering "gepa>=0.1.0", - "google-cloud-aiplatform[evaluation]>=1.100.0", + "google-cloud-aiplatform[evaluation]>=1.143.0", "pandas>=2.2.3", "rouge-score>=0.1.2", "tabulate>=0.9.0", From 68ece4e04fb3829e61847dae123cf4bc79a0a4c2 Mon Sep 17 00:00:00 2001 From: Sasha Sobran Date: Thu, 26 Mar 2026 10:15:51 -0700 Subject: [PATCH 7/7] fix: enforce allowed file extensions for GET requests in the builder API Co-authored-by: Sasha Sobran PiperOrigin-RevId: 889912141 --- src/google/adk/cli/fast_api.py | 12 +++++++--- tests/unittests/cli/test_fast_api.py | 34 ++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/google/adk/cli/fast_api.py b/src/google/adk/cli/fast_api.py index 4d666f78d3..0b6f3fb6fe 100644 --- a/src/google/adk/cli/fast_api.py +++ b/src/google/adk/cli/fast_api.py @@ -291,7 +291,7 @@ def _normalize_relative_path(path: str) -> str: def _has_parent_reference(path: str) -> bool: return any(part == ".." for part in path.split("/")) - _ALLOWED_UPLOAD_EXTENSIONS = frozenset({".yaml", ".yml"}) + _ALLOWED_EXTENSIONS = frozenset({".yaml", ".yml"}) def _parse_upload_filename(filename: Optional[str]) -> tuple[str, str]: if not filename: @@ -307,10 +307,10 @@ def _parse_upload_filename(filename: Optional[str]) -> tuple[str, str]: if _has_parent_reference(rel_path): raise ValueError(f"Path traversal rejected: {filename!r}") ext = os.path.splitext(rel_path)[1].lower() - if ext not in _ALLOWED_UPLOAD_EXTENSIONS: + if ext not in _ALLOWED_EXTENSIONS: raise ValueError( f"File type not allowed: {rel_path!r}" - f" (allowed: {', '.join(sorted(_ALLOWED_UPLOAD_EXTENSIONS))})" + f" (allowed: {', '.join(sorted(_ALLOWED_EXTENSIONS))})" ) return app_name, rel_path @@ -322,6 +322,12 @@ def _parse_file_path(file_path: str) -> str: raise ValueError(f"Absolute file_path rejected: {file_path!r}") if _has_parent_reference(file_path): raise ValueError(f"Path traversal rejected: {file_path!r}") + ext = os.path.splitext(file_path)[1].lower() + if ext not in _ALLOWED_EXTENSIONS: + raise ValueError( + f"File type not allowed: {file_path!r}" + f" (allowed: {', '.join(sorted(_ALLOWED_EXTENSIONS))})" + ) return file_path def _resolve_under_dir(root_dir: Path, rel_path: str) -> Path: diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index e53d5a918c..15bc908ddb 100755 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -1759,6 +1759,40 @@ def test_builder_save_allows_yaml_files(builder_test_client, tmp_path): assert response.json() is True +def test_builder_get_rejects_non_yaml_file_paths(builder_test_client, tmp_path): + """GET /builder/app/{app_name}?file_path=... rejects non-YAML extensions.""" + app_root = tmp_path / "app" + app_root.mkdir(parents=True, exist_ok=True) + (app_root / ".env").write_text("SECRET=supersecret\n") + (app_root / "agent.py").write_text("root_agent = None\n") + (app_root / "config.json").write_text("{}\n") + + for file_path in [".env", "agent.py", "config.json"]: + response = builder_test_client.get( + f"/builder/app/app?file_path={file_path}" + ) + assert response.status_code == 200, f"Expected 200 for {file_path}" + assert response.text == "", f"Expected empty response for {file_path}" + + +def test_builder_get_allows_yaml_file_paths(builder_test_client, tmp_path): + """GET /builder/app/{app_name}?file_path=... allows YAML extensions.""" + app_root = tmp_path / "app" + app_root.mkdir(parents=True, exist_ok=True) + (app_root / "sub_agent.yaml").write_text("name: sub\n") + (app_root / "tool.yml").write_text("name: tool\n") + + response = builder_test_client.get( + "/builder/app/app?file_path=sub_agent.yaml" + ) + assert response.status_code == 200 + assert response.text == "name: sub\n" + + response = builder_test_client.get("/builder/app/app?file_path=tool.yml") + assert response.status_code == 200 + assert response.text == "name: tool\n" + + def test_builder_endpoints_not_registered_without_web( mock_session_service, mock_artifact_service,