fix(firestore): validate session identity components before path use

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

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",
+        "[email protected]",  # 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