From ec6c1d6ec55db42fc3dd40cc0109116a09efa4ea Mon Sep 17 00:00:00 2001 From: CorgiBoyG <111257566+CorgiBoyG@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:02:21 +0800 Subject: [PATCH] fix(firestore): validate session identity components before path use FirestoreSessionService used app_name, user_id and session_id as raw Firestore document path segments. The client treats "/" as a path separator and only checks segment parity, not content, so an identity component with an even number of "/" silently changed the document path and could nest one identity's session document under another's. The core VertexAiSessionService already guards session ids; the Firestore backend did not. Reject identity components that would alter the path (containing "/", equal to "." / "..", empty, or matching the Firestore-reserved __.*__ pattern) across all CRUD paths, while still accepting ordinary ids such as email addresses. The empty/None user_id list_sessions behavior from #7662 is unchanged: it returns before reaching the validated reference. Fixes #7800 --- .../firestore/firestore_session_service.py | 50 ++++++++-- .../test_firestore_session_service.py | 96 +++++++++++++++++++ 2 files changed, 140 insertions(+), 6 deletions(-) diff --git a/src/google/adk/integrations/firestore/firestore_session_service.py b/src/google/adk/integrations/firestore/firestore_session_service.py index 2066178f67b..072698e2710 100644 --- a/src/google/adk/integrations/firestore/firestore_session_service.py +++ b/src/google/adk/integrations/firestore/firestore_session_service.py @@ -173,6 +173,8 @@ def _merge_state( def _get_sessions_ref( self, app_name: str, user_id: str ) -> firestore.AsyncCollectionReference: + self._validate_identity_component("app_name", app_name) + self._validate_identity_component("user_id", user_id) return cast( "firestore.AsyncCollectionReference", self.client.collection(self.root_collection) @@ -182,6 +184,42 @@ def _get_sessions_ref( .collection(self.sessions_collection), ) + def _get_session_doc_ref( + self, app_name: str, user_id: str, session_id: str + ) -> firestore.AsyncDocumentReference: + self._validate_identity_component("session_id", session_id) + return cast( + "firestore.AsyncDocumentReference", + self._get_sessions_ref(app_name, user_id).document(session_id), + ) + + @staticmethod + def _validate_identity_component(name: str, value: str) -> None: + """Rejects identity components that would alter the document path. + + ``app_name``, ``user_id`` and ``session_id`` are used as raw Firestore + document path segments. The client treats ``/`` as a path separator and + only checks segment parity, not content, so a value containing ``/`` (or a + reserved segment) silently changes the path structure and can nest one + identity's session document under another's. This keeps distinct identity + tuples on distinct, non-nesting paths without adopting an allow-list strict + enough to reject legitimate ids such as email addresses. + """ + if not isinstance(value, str) or not value: + raise ValueError(f"{name} must be a non-empty string, got {value!r}.") + if "/" in value: + raise ValueError( + f"{name} must not contain '/': {value!r} would change the Firestore" + " document path." + ) + if value in (".", ".."): + raise ValueError(f"{name} must not be '.' or '..', got {value!r}.") + if value.startswith("__") and value.endswith("__"): + raise ValueError( + f"{name} must not match the Firestore-reserved __.*__ pattern, got" + f" {value!r}." + ) + async def create_session( self, *, @@ -197,7 +235,7 @@ async def create_session( initial_state = state or {} now = firestore.SERVER_TIMESTAMP - session_ref = self._get_sessions_ref(app_name, user_id).document(session_id) + session_ref = self._get_session_doc_ref(app_name, user_id, session_id) # Extract state deltas. App and user state are written to Firestore # natively, which keeps datetime and other rich types, so only the session @@ -288,7 +326,7 @@ async def get_session( config: Optional[GetSessionConfig] = None, ) -> Optional[Session]: """Gets a session from Firestore.""" - session_ref = self._get_sessions_ref(app_name, user_id).document(session_id) + session_ref = self._get_session_doc_ref(app_name, user_id, session_id) doc = await session_ref.get() if not doc.exists: @@ -465,7 +503,7 @@ async def delete_session( self, *, app_name: str, user_id: str, session_id: str ) -> None: """Deletes a session and its events from Firestore.""" - session_ref = self._get_sessions_ref(app_name, user_id).document(session_id) + session_ref = self._get_session_doc_ref(app_name, user_id, session_id) @firestore.async_transactional # type: ignore[untyped-decorator] async def _mark_deleting_txn( @@ -505,9 +543,9 @@ async def append_event(self, session: Session, event: Event) -> Event: self._apply_temp_state(session, event) event = self._trim_temp_delta_state(event) - session_ref = self._get_sessions_ref( - session.app_name, session.user_id - ).document(session.id) + session_ref = self._get_session_doc_ref( + session.app_name, session.user_id, session.id + ) state_delta = ( event.actions.state_delta diff --git a/tests/unittests/integrations/firestore/test_firestore_session_service.py b/tests/unittests/integrations/firestore/test_firestore_session_service.py index 382432f35a8..e0ef8e1ef59 100644 --- a/tests/unittests/integrations/firestore/test_firestore_session_service.py +++ b/tests/unittests/integrations/firestore/test_firestore_session_service.py @@ -1329,3 +1329,99 @@ def collection_side_effect(name): assert session.state["session_key"] == "session_val" assert "_app_app_key" not in session.state assert "_user_user_key" not in session.state + + +@pytest.mark.parametrize( + "bad_user_id", + [ + "alice/sessions/s", # even number of '/': passes verify_path, injects + "a/b/c/d", + "..", + "", + "__reserved__", + ], +) +@pytest.mark.asyncio +async def test_create_session_rejects_path_breaking_user_id( + mock_firestore_client, bad_user_id +): + """A user_id that would alter the Firestore document path is rejected.""" + service = FirestoreSessionService(client=mock_firestore_client) + + with mock.patch("google.cloud.firestore.async_transactional", lambda x: x): + with pytest.raises(ValueError): + await service.create_session(app_name="test_app", user_id=bad_user_id) + + +@pytest.mark.parametrize( + "bad_session_id", + [ + "x/sessions/y", # even number of '/': passes verify_path, injects + "s/users/other/sessions/s", + "..", + "__reserved__", + ], +) +@pytest.mark.asyncio +async def test_create_session_rejects_path_breaking_session_id( + mock_firestore_client, bad_session_id +): + """A caller-supplied session_id that alters the document path is rejected.""" + service = FirestoreSessionService(client=mock_firestore_client) + + with mock.patch("google.cloud.firestore.async_transactional", lambda x: x): + with pytest.raises(ValueError): + await service.create_session( + app_name="test_app", user_id="alice", session_id=bad_session_id + ) + + +@pytest.mark.asyncio +async def test_get_session_rejects_path_breaking_identity( + mock_firestore_client, +): + """Read paths validate identity components too.""" + service = FirestoreSessionService(client=mock_firestore_client) + + with pytest.raises(ValueError): + await service.get_session( + app_name="test_app", user_id="alice/sessions/s", session_id="s" + ) + + +@pytest.mark.asyncio +async def test_delete_session_rejects_path_breaking_identity( + mock_firestore_client, +): + """Delete paths validate identity components too.""" + service = FirestoreSessionService(client=mock_firestore_client) + + with pytest.raises(ValueError): + await service.delete_session( + app_name="test_app", user_id="alice", session_id="x/sessions/y" + ) + + +@pytest.mark.parametrize( + "ok_identity", + [ + "alice", + "alice@example.com", # email user_id (contains '.') must still work + "user-123_abc", + "a.b.c", + ], +) +@pytest.mark.asyncio +async def test_create_session_accepts_normal_identity( + mock_firestore_client, ok_identity +): + """Legitimate identity components (including emails) are not rejected.""" + service = FirestoreSessionService(client=mock_firestore_client) + + with mock.patch("google.cloud.firestore.async_transactional", lambda x: x): + session = await service.create_session( + app_name="test_app", user_id=ok_identity + ) + + assert session.user_id == ok_identity + assert session.id