Skip to content

fix: enforce appName ownership check in VertexAiSessionService.listEvents - #1435

Open
prasanna8585 wants to merge 1 commit into
google:mainfrom
prasanna8585:fix/listevents-ownership-check
Open

prasanna8585 wants to merge 1 commit into
google:mainfrom
prasanna8585:fix/listevents-ownership-check

Conversation

@prasanna8585

Copy link
Copy Markdown
Contributor

Summary

Commit d1b1d92 (#1323) added an ownership check to getSession and deleteSession - fetch the session, compare the backend-reported owner to the caller's userId, deny as not-found on mismatch. The same check was not applied to listEvents.

Impact

listEvents returns a session's full event stream (conversation content, tool calls, state deltas) addressed solely by sessionId, with no ownership check. The Vertex backend session-events sub-resource does not enforce per-caller ownership on its own (per the existing comments on the getSession/deleteSession checks in this same file), so any authenticated caller who knows or guesses another user's sessionId can read that user's full conversation history via listEvents - the same disclosure class d1b1d92 fixed for getSession/deleteSession, left open here. listEvents is public API (BaseSessionService interface), not an internal helper.

Fix

listEvents now fetches the session and applies the same ownership check getSession/deleteSession already use before returning events, denying as an empty response (not an error) on mismatch - consistent with getSession's not-found-style denial rather than deleteSession's exception-based one, since listEvents' existing contract already returns an empty response for a nonexistent/eventless session.

Testing

Added listEvents_wrongUser_returnsEmpty, mirroring the existing getSession_wrongUser_returnsEmpty test in the same file. Verified locally: the new test fails against the unfixed code (confirming it genuinely exercises the gap) and passes with the fix; full VertexAiSessionServiceTest suite passes with no regressions.

@hemasekhar-p hemasekhar-p self-assigned this Aug 17, 2026
@hemasekhar-p

Copy link
Copy Markdown
Contributor

Hi @prasanna8585, thank you for your contribution! We appreciate you taking the time to submit this pull request. our team is currently reviewing your changes, and we will keep you posted if any additional information is required. Thank you!

@prasanna8585

Copy link
Copy Markdown
Contributor Author

Hi @hemasekhar-p, is there any update or code changes needed from my side or are we ready to merge this PR?

@prasanna8585

Copy link
Copy Markdown
Contributor Author

Hi Team , is there any update or code changes needed from my side or are we ready to merge this PR?

@prasanna8585

Copy link
Copy Markdown
Contributor Author

Hi @hemasekhar-p, any update required from my side to merge this PR?

@prasanna8585
prasanna8585 force-pushed the fix/listevents-ownership-check branch from 198b177 to 0be9ca1 Compare September 24, 2026 11:22
@damianmomotgoogle

Copy link
Copy Markdown
Contributor

Hi, thank your for PR, please:

  • squash all changes to a single commit
  • mark commit message with "fix:"

@damianmomotgoogle damianmomotgoogle added waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. and removed needs review labels Oct 15, 2026
@prasanna8585
prasanna8585 force-pushed the fix/listevents-ownership-check branch from f9f3220 to af65945 Compare October 15, 2026 11:45
@prasanna8585
prasanna8585 force-pushed the fix/listevents-ownership-check branch from af65945 to 6cb6557 Compare October 15, 2026 11:46
@prasanna8585

Copy link
Copy Markdown
Contributor Author

Hi @damianmomotgoogle, thanks for the review :

Squashed all commits into a single commit
Commit message now prefixed with fix:

Ready for another look.

@damianmomotgoogle damianmomotgoogle added needs review and removed waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. labels Oct 15, 2026
@damianmomotgoogle damianmomotgoogle changed the title Enforce session ownership in VertexAiSessionService.listEvents fix: enforce appName ownership check in VertexAiSessionService.listEvents Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants