From 9d4db6da014118c923c695ba06bb6dc7c5d87146 Mon Sep 17 00:00:00 2001 From: Nicola Pesavento Date: Sun, 3 May 2026 13:15:27 +0200 Subject: [PATCH] fix(sessions): validate session_id and enforce ownership in delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VertexAiSessionService interpolated session_id directly into the Vertex AI REST URL path with no validation, and delete_session ignored its user_id parameter. This allowed two flaws when a backend forwarded a frontend- supplied session_id to delete_session(): 1. Path traversal via session_id (e.g. "..", "..?force=true", "../../../datasets/"): httpx resolves ../ before sending the request, so the Vertex AI API receives a clean path pointing to a sibling resource — including the parent reasoning engine itself. 2. Cross-user deletion (delete-IDOR): delete_session accepted user_id but never used it, so any caller knowing a session_id could delete a session belonging to another user. This change adds: - _validate_session_id(): strict regex (^[A-Za-z0-9_-]+$) applied at every interpolation site (create_session, get_session, delete_session, append_event). Matches the format of server-generated session IDs and client-supplied UUIDs while rejecting '/', '..', '?', etc. - delete_session: fetches the session first and verifies user_id matches, mirroring the existing check in get_session. Tests added for both fixes. --- .../adk/sessions/vertex_ai_session_service.py | 44 +++++++++++++++++-- .../test_vertex_ai_session_service.py | 39 ++++++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/src/google/adk/sessions/vertex_ai_session_service.py b/src/google/adk/sessions/vertex_ai_session_service.py index 8c1fdc134e..24a9a1dcd3 100644 --- a/src/google/adk/sessions/vertex_ai_session_service.py +++ b/src/google/adk/sessions/vertex_ai_session_service.py @@ -47,6 +47,23 @@ _COMPACTION_CUSTOM_METADATA_KEY = '_compaction' _USAGE_METADATA_CUSTOM_METADATA_KEY = '_usage_metadata' +_SESSION_ID_PATTERN = re.compile(r'^[A-Za-z0-9_-]+$') + + +def _validate_session_id(session_id: str) -> None: + """Rejects session IDs that could escape the URL path segment. + + Vertex AI session IDs are interpolated into the REST URL path. Without + validation, values like '..' or '../foo' resolve to sibling resources at + HTTP-client level (path traversal), allowing cross-resource reads/deletes. + """ + if not isinstance(session_id, str) or not _SESSION_ID_PATTERN.fullmatch( + session_id + ): + raise ValueError( + f'Invalid session_id {session_id!r}: must match {_SESSION_ID_PATTERN.pattern}.' + ) + def _quote_filter_literal(value: str) -> str: """Quotes filter values so embedded metacharacters stay inside the literal.""" @@ -127,6 +144,7 @@ async def create_session( config = {'session_state': state} if state else {} if session_id: + _validate_session_id(session_id) config['session_id'] = session_id config.update(kwargs) async with self._get_api_client() as api_client: @@ -157,6 +175,7 @@ async def get_session( session_id: str, config: Optional[GetSessionConfig] = None, ) -> Optional[Session]: + _validate_session_id(session_id) reasoning_engine_id = self._get_reasoning_engine_id(app_name) session_resource_name = ( f'reasoningEngines/{reasoning_engine_id}/sessions/{session_id}' @@ -256,14 +275,32 @@ async def list_sessions( async def delete_session( self, *, app_name: str, user_id: str, session_id: str ) -> None: + _validate_session_id(session_id) reasoning_engine_id = self._get_reasoning_engine_id(app_name) + session_resource_name = ( + f'reasoningEngines/{reasoning_engine_id}/sessions/{session_id}' + ) async with self._get_api_client() as api_client: + # Ownership check: ensure the session belongs to the caller before + # deleting. Without this, delete_session ignores user_id entirely and + # any caller who knows a session_id can delete it. + try: + existing = await api_client.agent_engines.sessions.get( + name=session_resource_name + ) + except ClientError as e: + if e.code == 404: + return + raise + if existing.user_id != user_id: + raise ValueError( + f'Session {session_id} does not belong to user {user_id}.' + ) + try: await api_client.agent_engines.sessions.delete( - name=( - f'reasoningEngines/{reasoning_engine_id}/sessions/{session_id}' - ), + name=session_resource_name, ) except Exception as e: logger.error('Error deleting session %s: %s', session_id, e) @@ -274,6 +311,7 @@ async def append_event(self, session: Session, event: Event) -> Event: # Update the in-memory session. await super().append_event(session=session, event=event) + _validate_session_id(session.id) reasoning_engine_id = self._get_reasoning_engine_id(session.app_name) # Build config (Monolithic approach) diff --git a/tests/unittests/sessions/test_vertex_ai_session_service.py b/tests/unittests/sessions/test_vertex_ai_session_service.py index 0b265c3cfb..e9ae881858 100644 --- a/tests/unittests/sessions/test_vertex_ai_session_service.py +++ b/tests/unittests/sessions/test_vertex_ai_session_service.py @@ -725,6 +725,45 @@ async def test_get_and_delete_session(): assert str(excinfo.value) == '404 Session not found: 1' +@pytest.mark.asyncio +@pytest.mark.usefixtures('mock_get_api_client') +async def test_delete_session_rejects_other_users_session(): + """delete_session must not delete a session owned by a different user.""" + session_service = mock_vertex_ai_session_service() + + # session '1' belongs to 'user'; 'user2' must not be allowed to delete it. + with pytest.raises(ValueError) as excinfo: + await session_service.delete_session( + app_name='123', user_id='user2', session_id='1' + ) + assert 'does not belong to user user2' in str(excinfo.value) + + # Session must still exist. + assert ( + await session_service.get_session( + app_name='123', user_id='user', session_id='1' + ) + == MOCK_SESSION + ) + + +@pytest.mark.asyncio +@pytest.mark.usefixtures('mock_get_api_client') +async def test_session_id_path_traversal_rejected(): + """Session IDs containing path-traversal characters must be rejected.""" + session_service = mock_vertex_ai_session_service() + + for bad_id in ['..', '../foo', '..?force=true', 'a/b', '']: + with pytest.raises(ValueError): + await session_service.delete_session( + app_name='123', user_id='user', session_id=bad_id + ) + with pytest.raises(ValueError): + await session_service.get_session( + app_name='123', user_id='user', session_id=bad_id + ) + + @pytest.mark.asyncio @pytest.mark.usefixtures('mock_get_api_client') async def test_get_session_with_page_token():