Skip to content

fix(firestore): honor appName in getSession/deleteSession/listEvents#1361

Merged
copybara-service[bot] merged 1 commit into
google:mainfrom
petrmarinec:fix/firestore-session-appname-scope
Jul 22, 2026
Merged

fix(firestore): honor appName in getSession/deleteSession/listEvents#1361
copybara-service[bot] merged 1 commit into
google:mainfrom
petrmarinec:fix/firestore-session-appname-scope

Conversation

@petrmarinec

Copy link
Copy Markdown
Contributor

Description of Change

FirestoreSessionService stores each session at /{root}/{userId}/sessions/{sessionId} — keyed only by (userId, sessionId). appName is saved as a field (APP_NAME_KEY) but is not part of the key, and three methods fetch the document by (userId, sessionId) without checking it:

  • getSession(appName, userId, sessionId, ...) — returns the document regardless of appName.
  • listEvents(appName, userId, sessionId) — checks only that the document exists.
  • deleteSession(appName, userId, sessionId) — deletes regardless of appName.

listSessions(appName, userId) already scopes correctly with getSessionsCollection(userId).whereEqualTo(APP_NAME_KEY, appName), so the omission in the other three is an oversight.

Because appName is accepted but ignored, when several ADK applications share one Firestore for the same users, application B can read, list the events of, or delete application A's session by using A's session ID with any appName — e.g. getSession("appB", U, S) returns app A's session. Session state can hold OAuth tokens (via SessionStateCredentialService) and conversation history, so this is a cross-application read (confidentiality) and delete (integrity) issue. InMemorySessionService keys on appName -> userId -> sessionId, so this also brings the Firestore backend in line with the in-memory contract.

This change adds the appName check to getSession, deleteSession, and listEvents, treating a mismatch as "session not found", mirroring listSessions.

Testing Plan

Scope check only; matching appName behaves exactly as before. Suggested regression test (Firestore emulator): create a session under (appA, U, S), then assert getSession(appB, U, S) and listEvents(appB, U, S) raise SessionNotFoundException and deleteSession(appB, U, S) leaves it intact, while all three succeed for appA.

@hemasekhar-p hemasekhar-p self-assigned this Jul 20, 2026
@hemasekhar-p

Copy link
Copy Markdown
Contributor

Hi @petrmarinec, thank you for your contribution! We appreciate you taking the time to submit this pull request. To proceed with the review, could you please address the following,

  1. Please include corresponding unit tests to verify your changes.
  2. As per our contribution policy, please squash your changes into a single commit.
  3. Please resolve the current Maven build failures.

@hemasekhar-p hemasekhar-p added the waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. label Jul 20, 2026
@petrmarinec
petrmarinec force-pushed the fix/firestore-session-appname-scope branch from 4d0a80a to 87471cc Compare July 20, 2026 10:00
…sion/listEvents

FirestoreSessionService stores each session at /{root}/{userId}/sessions/{sessionId},
so the document is keyed only by (userId, sessionId). getSession, deleteSession, and
listEvents fetched the document by (userId, sessionId) and never checked the stored
appName against the requested one, so a caller could read, list the events of, or
delete another application's session for the same user by passing any appName.
listSessions already scopes by appName (whereEqualTo(APP_NAME_KEY, appName)).

Add the same appName check to getSession, deleteSession, and listEvents, treating a
mismatch as "session not found", and add unit tests covering the appName-mismatch
cases for all three methods.
@petrmarinec
petrmarinec force-pushed the fix/firestore-session-appname-scope branch from 87471cc to a370034 Compare July 20, 2026 10:03
@petrmarinec

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've addressed all three points:

  • Unit tests: added getSession / deleteSession / listEvents tests for the appName-mismatch case (a mismatched appName is treated as "not found" and does not delete), and updated the existing tests that now need to stub the stored appName.
  • Squashed into a single commit.
  • Build failures resolved — they came from the existing tests not stubbing appName; mvn -pl contrib/firestore-session-service -am test now passes (47 tests in FirestoreSessionServiceTest) and fmt:check is clean.

@hemasekhar-p

Copy link
Copy Markdown
Contributor

@petrmarinec, thank you for addressing those points and providing the updated tests. Your changes are now under review by our team and we will keep you posted if we require any further updates. Thanks you.

@hemasekhar-p hemasekhar-p added needs review and removed waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. labels Jul 20, 2026
@kvmilos kvmilos changed the title fix(firestore-session-service): honor appName in getSession/deleteSession/listEvents fix(firestore): honor appName in getSession/deleteSession/listEvents Jul 21, 2026
@kvmilos
kvmilos force-pushed the fix/firestore-session-appname-scope branch from c6654f4 to a370034 Compare July 21, 2026 09:09
@copybara-service
copybara-service Bot merged commit 2b2f25e into google:main Jul 22, 2026
18 of 19 checks passed
@kvmilos kvmilos self-assigned this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants