diff --git a/contrib/firestore-session-service/src/main/java/com/google/adk/sessions/FirestoreSessionService.java b/contrib/firestore-session-service/src/main/java/com/google/adk/sessions/FirestoreSessionService.java index db236fc88..f4e68e3ca 100644 --- a/contrib/firestore-session-service/src/main/java/com/google/adk/sessions/FirestoreSessionService.java +++ b/contrib/firestore-session-service/src/main/java/com/google/adk/sessions/FirestoreSessionService.java @@ -177,6 +177,16 @@ public Maybe getSession( return Maybe.empty(); } + // Enforce the (appName, userId, sessionId) scope. The Firestore + // document is keyed only by (userId, sessionId), so without this + // check a caller could read a session that belongs to a different + // application for the same user. Treat an appName mismatch as + // "not found" so cross-application existence is not leaked. + if (!appName.equals(data.get(APP_NAME_KEY))) { + logger.warn("Session {} does not belong to app {}", sessionId, appName); + return Maybe.error(new SessionNotFoundException("Session not found: " + sessionId)); + } + // Fetch events based on config GetSessionConfig config = configOpt.orElseGet(() -> GetSessionConfig.builder().build()); @@ -491,6 +501,16 @@ public Completable deleteSession(String appName, String userId, String sessionId com.google.cloud.firestore.DocumentReference sessionRef = getSessionsCollection(userId).document(sessionId); + // Enforce the (appName, userId, sessionId) scope before deleting. + // The document is keyed only by (userId, sessionId), so without this + // check a caller could delete a session that belongs to a different + // application for the same user. + DocumentSnapshot sessionDoc = sessionRef.get().get(); + if (!sessionDoc.exists() || !appName.equals(sessionDoc.get(APP_NAME_KEY))) { + logger.warn("Session {} not found for app {}; nothing to delete", sessionId, appName); + return; + } + // 1. Fetch all events in the subcollection to delete them in batches. CollectionReference eventsRef = sessionRef.collection(EVENTS_SUBCOLLECTION_NAME); com.google.api.core.ApiFuture eventsQuery = @@ -536,9 +556,11 @@ public Single listEvents(String appName, String userId, Stri getSessionsCollection(userId).document(sessionId).get(); DocumentSnapshot sessionDocument = sessionFuture.get(); // Block for the result - if (!sessionDocument.exists()) { + if (!sessionDocument.exists() || !appName.equals(sessionDocument.get(APP_NAME_KEY))) { logger.warn( - "Session not found for sessionId: {}. Returning empty list of events.", sessionId); + "Session not found for sessionId: {} in app {}. Returning empty list of events.", + sessionId, + appName); throw new SessionNotFoundException(appName + "," + userId + "," + sessionId); } diff --git a/contrib/firestore-session-service/src/test/java/com/google/adk/sessions/FirestoreSessionServiceTest.java b/contrib/firestore-session-service/src/test/java/com/google/adk/sessions/FirestoreSessionServiceTest.java index 43ca6889f..71fa8a7bd 100644 --- a/contrib/firestore-session-service/src/test/java/com/google/adk/sessions/FirestoreSessionServiceTest.java +++ b/contrib/firestore-session-service/src/test/java/com/google/adk/sessions/FirestoreSessionServiceTest.java @@ -273,7 +273,8 @@ void getSession_withAfterTimestamp_appliesFilterToQuery() { when(mockSessionDocRef.get()).thenReturn(ApiFutures.immediateFuture(mockSessionSnapshot)); when(mockSessionSnapshot.exists()).thenReturn(true); when(mockSessionSnapshot.getReference()).thenReturn(mockSessionDocRef); - when(mockSessionSnapshot.getData()).thenReturn(ImmutableMap.of("state", Map.of())); + when(mockSessionSnapshot.getData()) + .thenReturn(ImmutableMap.of(Constants.KEY_APP_NAME, APP_NAME, "state", Map.of())); when(mockQuery.whereGreaterThan(Constants.KEY_TIMESTAMP, timestamp.toString())) .thenReturn(mockQuery); when(mockQuery.get()).thenReturn(ApiFutures.immediateFuture(mockQuerySnapshot)); @@ -778,6 +779,7 @@ void listEvents_sessionExists_returnsEvents() { when(mockSessionsCollection.document(SESSION_ID)).thenReturn(mockSessionDocRef); when(mockSessionDocRef.get()).thenReturn(ApiFutures.immediateFuture(mockSessionSnapshot)); when(mockSessionSnapshot.exists()).thenReturn(true); + when(mockSessionSnapshot.get(Constants.KEY_APP_NAME)).thenReturn(APP_NAME); when(mockSessionSnapshot.getReference()).thenReturn(mockSessionDocRef); when(mockQuery.get()).thenReturn(ApiFutures.immediateFuture(mockQuerySnapshot)); Map eventData = @@ -828,6 +830,10 @@ void listEvents_sessionDoesNotExist_throwsException() { void deleteSession_deletesEventsAndSession() { // Arrange when(mockSessionsCollection.document(SESSION_ID)).thenReturn(mockSessionDocRef); + // The ownership check fetches the session document first. + when(mockSessionDocRef.get()).thenReturn(ApiFutures.immediateFuture(mockSessionSnapshot)); + when(mockSessionSnapshot.exists()).thenReturn(true); + when(mockSessionSnapshot.get(Constants.KEY_APP_NAME)).thenReturn(APP_NAME); when(mockSessionDocRef.collection(Constants.EVENTS_SUBCOLLECTION_NAME)) .thenReturn(mockEventsCollection); when(mockEventsCollection.get()).thenReturn(ApiFutures.immediateFuture(mockQuerySnapshot)); @@ -901,4 +907,75 @@ void listSessions_returnsListOfSessions() { return true; }); } + + // --- appName scope enforcement tests --- + // Sessions are keyed by (userId, sessionId) only, so getSession, deleteSession and listEvents + // must reject a request whose appName does not match the stored session's appName; otherwise one + // application could read, list the events of, or delete another application's session for the + // same user. + + /** Tests that getSession emits an error when the stored appName does not match the request. */ + @Test + void getSession_appNameMismatch_emitsError() { + // Arrange + when(mockSessionsCollection.document(SESSION_ID)).thenReturn(mockSessionDocRef); + when(mockSessionDocRef.get()).thenReturn(ApiFutures.immediateFuture(mockSessionSnapshot)); + when(mockSessionSnapshot.exists()).thenReturn(true); + when(mockSessionSnapshot.getData()) + .thenReturn( + ImmutableMap.of( + "id", + SESSION_ID, + Constants.KEY_APP_NAME, + "other-app", + "userId", + USER_ID, + "updateTime", + NOW.toString(), + "state", + Collections.emptyMap())); + + // Act + TestObserver testObserver = + sessionService.getSession(APP_NAME, USER_ID, SESSION_ID, Optional.empty()).test(); + + // Assert + testObserver.assertError(SessionNotFoundException.class); + } + + /** Tests that listEvents emits an error when the stored appName does not match the request. */ + @Test + void listEvents_appNameMismatch_emitsError() { + // Arrange + when(mockSessionsCollection.document(SESSION_ID)).thenReturn(mockSessionDocRef); + when(mockSessionDocRef.get()).thenReturn(ApiFutures.immediateFuture(mockSessionSnapshot)); + when(mockSessionSnapshot.exists()).thenReturn(true); + when(mockSessionSnapshot.get(Constants.KEY_APP_NAME)).thenReturn("other-app"); + + // Act + TestObserver testObserver = + sessionService.listEvents(APP_NAME, USER_ID, SESSION_ID).test(); + + // Assert + testObserver.assertError(SessionNotFoundException.class); + } + + /** + * Tests that deleteSession does not delete when the stored appName does not match the request. + */ + @Test + void deleteSession_appNameMismatch_doesNotDelete() { + // Arrange + when(mockSessionsCollection.document(SESSION_ID)).thenReturn(mockSessionDocRef); + when(mockSessionDocRef.get()).thenReturn(ApiFutures.immediateFuture(mockSessionSnapshot)); + when(mockSessionSnapshot.exists()).thenReturn(true); + when(mockSessionSnapshot.get(Constants.KEY_APP_NAME)).thenReturn("other-app"); + + // Act + sessionService.deleteSession(APP_NAME, USER_ID, SESSION_ID).test().assertComplete(); + + // Assert: the session (belonging to another app) must be left intact. + verify(mockSessionDocRef, never()).delete(); + verify(mockWriteBatch, never()).commit(); + } }