Skip to content

Commit 6afa8d9

Browse files
Nicola Pesaventonicola-pesavento
authored andcommitted
fix(sessions): validate session_id and enforce ownership in delete
session_id was interpolated raw into the Vertex AI REST URL, and delete_session ignored user_id. A frontend-supplied session_id could therefore (1) traverse paths via ".." / "..?force=true" to sibling resources, or (2) delete another user's session. - Add _validate_session_id() (^[A-Za-z0-9_-]+$) at every interpolation site: create_session, get_session, delete_session, append_event. - delete_session now verifies user_id, mirroring get_session.
1 parent f8b4c59 commit 6afa8d9

3 files changed

Lines changed: 76 additions & 4 deletions

File tree

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
{
22
"backendUrl": ""
3-
}
3+
}

src/google/adk/sessions/vertex_ai_session_service.py

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,18 @@
4747
_COMPACTION_CUSTOM_METADATA_KEY = '_compaction'
4848
_USAGE_METADATA_CUSTOM_METADATA_KEY = '_usage_metadata'
4949

50+
_SESSION_ID_PATTERN = re.compile(r'^[A-Za-z0-9_-]+$')
51+
52+
53+
def _validate_session_id(session_id: str) -> None:
54+
"""Rejects session IDs that could escape the URL path segment."""
55+
if not isinstance(session_id, str) or not _SESSION_ID_PATTERN.fullmatch(
56+
session_id
57+
):
58+
raise ValueError(
59+
f'Invalid session_id {session_id!r}: must match {_SESSION_ID_PATTERN.pattern}.'
60+
)
61+
5062

5163
def _quote_filter_literal(value: str) -> str:
5264
"""Quotes filter values so embedded metacharacters stay inside the literal."""
@@ -127,6 +139,7 @@ async def create_session(
127139

128140
config = {'session_state': state} if state else {}
129141
if session_id:
142+
_validate_session_id(session_id)
130143
config['session_id'] = session_id
131144
config.update(kwargs)
132145
async with self._get_api_client() as api_client:
@@ -157,6 +170,7 @@ async def get_session(
157170
session_id: str,
158171
config: Optional[GetSessionConfig] = None,
159172
) -> Optional[Session]:
173+
_validate_session_id(session_id)
160174
reasoning_engine_id = self._get_reasoning_engine_id(app_name)
161175
session_resource_name = (
162176
f'reasoningEngines/{reasoning_engine_id}/sessions/{session_id}'
@@ -256,14 +270,32 @@ async def list_sessions(
256270
async def delete_session(
257271
self, *, app_name: str, user_id: str, session_id: str
258272
) -> None:
273+
_validate_session_id(session_id)
259274
reasoning_engine_id = self._get_reasoning_engine_id(app_name)
275+
session_resource_name = (
276+
f'reasoningEngines/{reasoning_engine_id}/sessions/{session_id}'
277+
)
260278

261279
async with self._get_api_client() as api_client:
280+
# Ownership check: ensure the session belongs to the caller before
281+
# deleting. Without this, delete_session ignores user_id entirely and
282+
# any caller who knows a session_id can delete it.
283+
try:
284+
existing = await api_client.agent_engines.sessions.get(
285+
name=session_resource_name
286+
)
287+
except ClientError as e:
288+
if e.code == 404:
289+
return
290+
raise
291+
if existing.user_id != user_id:
292+
raise ValueError(
293+
f'Session {session_id} does not belong to user {user_id}.'
294+
)
295+
262296
try:
263297
await api_client.agent_engines.sessions.delete(
264-
name=(
265-
f'reasoningEngines/{reasoning_engine_id}/sessions/{session_id}'
266-
),
298+
name=session_resource_name,
267299
)
268300
except Exception as e:
269301
logger.error('Error deleting session %s: %s', session_id, e)
@@ -274,6 +306,7 @@ async def append_event(self, session: Session, event: Event) -> Event:
274306
# Update the in-memory session.
275307
await super().append_event(session=session, event=event)
276308

309+
_validate_session_id(session.id)
277310
reasoning_engine_id = self._get_reasoning_engine_id(session.app_name)
278311

279312
# Build config (Monolithic approach)

tests/unittests/sessions/test_vertex_ai_session_service.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -725,6 +725,45 @@ async def test_get_and_delete_session():
725725
assert str(excinfo.value) == '404 Session not found: 1'
726726

727727

728+
@pytest.mark.asyncio
729+
@pytest.mark.usefixtures('mock_get_api_client')
730+
async def test_delete_session_rejects_other_users_session():
731+
"""delete_session must not delete a session owned by a different user."""
732+
session_service = mock_vertex_ai_session_service()
733+
734+
# session '1' belongs to 'user'; 'user2' must not be allowed to delete it.
735+
with pytest.raises(ValueError) as excinfo:
736+
await session_service.delete_session(
737+
app_name='123', user_id='user2', session_id='1'
738+
)
739+
assert 'does not belong to user user2' in str(excinfo.value)
740+
741+
# Session must still exist.
742+
assert (
743+
await session_service.get_session(
744+
app_name='123', user_id='user', session_id='1'
745+
)
746+
== MOCK_SESSION
747+
)
748+
749+
750+
@pytest.mark.asyncio
751+
@pytest.mark.usefixtures('mock_get_api_client')
752+
async def test_session_id_path_traversal_rejected():
753+
"""Session IDs containing path-traversal characters must be rejected."""
754+
session_service = mock_vertex_ai_session_service()
755+
756+
for bad_id in ['..', '../foo', '..?force=true', 'a/b', '']:
757+
with pytest.raises(ValueError):
758+
await session_service.delete_session(
759+
app_name='123', user_id='user', session_id=bad_id
760+
)
761+
with pytest.raises(ValueError):
762+
await session_service.get_session(
763+
app_name='123', user_id='user', session_id=bad_id
764+
)
765+
766+
728767
@pytest.mark.asyncio
729768
@pytest.mark.usefixtures('mock_get_api_client')
730769
async def test_get_session_with_page_token():

0 commit comments

Comments
 (0)