Repository navigation
Redis session keys can collide for distinct identity tuples #7464
Description
Describe the Bug
RedisSessionService builds session keys by joining app_name, user_id, and session_id with colons, without constraining those values to exclude colons. Distinct identity tuples can therefore address the same Redis key:
("billing", "tenant:victim", "fixed")("billing", "tenant", "victim:fixed")
Both produce {key_prefix}billing:tenant:victim:fixed.
I reproduced cross-user session retrieval, a Runner turn being appended to the other session, and deletion through the colliding tuple against published google-adk==2.8.0 with Redis 7.0.15. The current v2.11.0 source and current main still use the same key construction; I have not rerun the runtime proof against v2.11.0.
Google Bug Hunters reviewed the same behavior, closed the report as Infeasible (P4/S4), and explicitly permitted public GitHub disclosure. This issue is about upstream identity isolation and storage compatibility, not an appeal of that decision or a bounty request.
Steps to Reproduce
- Configure
RedisSessionServiceagainst a local Redis instance. - Create a session with
app_name="billing",user_id="tenant:victim", andsession_id="fixed". - Request that session with
app_name="billing",user_id="tenant", andsession_id="victim:fixed". - Observe that the second tuple resolves to the first session. Deleting with the second tuple also removes the stored session.
Expected Behavior
Different (app_name, user_id, session_id) tuples should always map to distinct session records.
Observed Behavior
The two tuples map to the same Redis key, so reads and deletes through one identity can reach the other's record. The Runner mutation path was also reproduced.
Environment Details
- ADK Library Version:
google-adk==2.8.0for the runtime reproduction; v2.11.0 and currentmainsource still show the same key construction - Redis: 7.0.15
- Desktop OS: not recorded in the original runtime reproduction
- Python Version: not recorded in the original runtime reproduction
- LiteLLM: No
- Model: N/A
Additional Context
A complete tuple- and type-separated key format changes persisted session, user-state, and app-state key names. The local candidate does not automatically read legacy keys because fallback could preserve identity aliases. Before I send a code change, what compatibility approach would maintainers prefer: a documented cutover/new key prefix, or a migration plan? I have not opened a pull request.
Activity
Li-john1021 commentedon Oct 9, 2026
I'd like to work on this. Reproduced the key collision on the current
_session_key/_user_state_keyconstruction insrc/google/adk/integrations/redis/_redis_session_service.py(colon-joinedapp_name:user_id:session_id). Distinct tuples like("billing","tenant:victim","fixed")and("billing","tenant","victim:fixed")map to one Redis key, so get/delete/Runner writes can cross identities.Proposed direction: switch session and user-state keys to a tuple-safe encoding (no raw colons), with a documented cutover via a new key prefix rather than a read-fallback that would preserve aliases. Happy to follow a migration plan instead if you'd rather keep old keys readable for one release.
Plan: unit tests for colliding tuples (create/get/delete isolation + no cross-session event append), then the key-format change. I've signed / can sign the Google CLA. Could a maintainer confirm the compatibility approach and assign this to me before I open a PR?
surajksharma07 commentedon Oct 9, 2026
Reproduced this on 2.11.0/main too @srkyn and noticed a related case: a session in an app named "user_state" lands on the same key as another app's user-state record.
A workaround that worked locally: a small RedisSessionService subclass that URL-encodes each key segment and adds a type tag (s:/u:/a:), with list_sessions adjusted to match. Note it won't see sessions stored under the old keys. Could you try that against your Redis 7 setup and confirm it holds up for get/append/delete and listing? Please test it thoroughly before raising a PR.
Flagged the cutover vs migration question to the maintainers. @Li-john1021 it'd be good to sync with @srkyn so only one PR goes up.
Li-john1021 commentedon Oct 9, 2026
Thank you @surajksharma07 — really appreciate you confirming the repro on 2.11.0/main, and for catching the
"user_state"app-name collision as well. That extra case is very helpful.I'd love to stay coordinated with @srkyn so we only land one PR. I'm happy to follow whatever works best for you both: I can wait and carefully review/test a PR if @srkyn is already driving it, or I can take a first pass at the implementation if that would be more helpful. Either way I'm glad to learn from the maintainers' preferred approach here.
Two gentle questions, if you have a moment:
s:/u:/a:prefixes)?I'll hold off on opening a competing PR until that's clear. Thanks again for the guidance.
srkyn commentedon Oct 10, 2026
Thanks for checking in. I’m driving this from a local branch, but I haven’t opened a PR yet. I’m rechecking the candidate against current main and waiting for maintainer guidance on whether existing Redis keys should be cut over or migrated. Please hold off on a parallel implementation for now. I’ll update the thread once the approach is clear and the tests pass on the current base.
Li-john1021 commentedon Oct 11, 2026
Thanks @srkyn — understood. I'll hold off on a parallel PR and watch this thread for your update once the approach is clear.
surajksharma07 commentedon Oct 12, 2026
Rechecked against current main and the key construction is unchanged so the collision still reproduces @srkyn. No maintainer call on cutover vs migration yet and pinged again.
One idea while that's pending: the format change could be split from a smaller backward-compatible piece. get/delete/append_event could reject a record whose stored app_name/user_id/id don't match the request, the same check list_sessions already does. That stops cross-identity access without touching existing keys though colliding tuples still can't coexist and the user_state/app_state clash needs the new format.
If it does end up being a migration deriving the new keys from the ids stored inside each session JSON rather than parsing the old key would sidestep the ambiguous ones, and agreed on no read-fallback.
Could you see whether that split works on your branch? Please run the get/append/delete/list cases against Redis before opening the PR.
srkyn commentedon Oct 12, 2026
Thanks for the suggestion. I tested the full typed-key candidate against current main at
a334bac. Session, user-state, and app-state keys now have distinct type tags and unpadded Base64URL identity components. Stored-identity checks remain as defense in depth, and delete plus session/state updates use watched Redis transactions.All 41 Redis integration tests pass. Direct Redis 7.0.15 checks also passed for tuple and type-collision isolation, state updates, and concurrent identity replacement.
The README documents a cutover only. Legacy keys remain untouched and unread because legacy user-state keys do not reliably identify their original app/user tuple. I have not opened a PR. Would maintainers prefer this documented cutover, or do they have a specific migration approach in mind?
Li-john1021 commentedon Oct 13, 2026
Thanks @srkyn for the typed-key write-up and the Redis 7 verification notes — that cutover framing (type tags + unpadded Base64URL, no read-fallback on ambiguous legacy user-state keys) looks solid from here.
I'll keep holding off on a parallel PR and watch for maintainer guidance on cutover vs migration. Happy to help review or re-test once a PR is up.
surajksharma07 commentedon Oct 13, 2026
That's a solid candidate @srkyn and having the stored-identity checks plus watched transactions in there is a nice bonus since the app/user state writes are plain GET+SET today. Still no maintainer call on cutover vs migration. Suggest opening it as a draft PR with "Fixes #7464" so they can weigh in on the actual diff.
One thing to look at before then: if the new keys stay under the default "adk:session:" prefix a legacy key from an app named "s" (or "u"/"a") can end up with the same shape as a new typed key. Your identity check catches that but a versioned segment like "adk:session:v2:" would keep the two namespaces apart and let people clean up old keys with a simple SCAN on the prefix.
Could you try that on your branch and add a test that seeds a legacy-format key and confirms get/list/delete ignore it? It would also help to add a short README note on expiring or deleting the old keys.
Please rerun the 41 Redis tests and the Redis 7 checks after that change before you open the PR. @Li-john1021 thanks for holding off, reviewing the draft once it's up would be a big help.
Li-john1021 commentedon Oct 13, 2026
Thanks @surajksharma07 — happy to review the draft once @srkyn opens it (especially the
adk:session:v2:namespace split and the legacy-key ignore test). I'll keep holding off on a parallel PR in the meantime.surajksharma07 commentedon Oct 15, 2026
Quick status: rechecked against 2.12.0 and today’s main (156f007) and nothing has changed @Li-john1021. _session_key/_user_state_key/_app_state_key still join raw values with ":" and list_sessions is still the only path that checks the stored identity so both the tuple and the user_state/app_state collisions still reproduce.
@srkyn whenever the "adk:session:v2:" change and the legacy-key test are in please open the draft PR with “Fixes #7464”. No maintainer call on cutover vs migration yet and having the actual diff up is the easiest way to get one.
Li-john1021 commentedon Oct 15, 2026
Thanks @surajksharma07 for the recheck against 2.12.0 / current main — noted that both collision cases still reproduce.
Still holding off on a parallel PR as discussed. Happy to review the draft once @srkyn opens it with the
adk:session:v2:namespace and legacy-key ignore test.srkyn commentedon Oct 15, 2026
Draft PR #7563 is open with the v2 typed-key namespace, legacy-key isolation test, and README cutover note. I addressed the review comments in the latest commit and reran the focused Redis suite: 43 passed. The draft remains open for maintainer guidance on cutover versus migration.
Li-john1021 commentedon Oct 15, 2026
Thanks @srkyn for opening draft PR #7563 — I took a pass over the current diff as promised.
A few notes from my side (non-blocking; happy for maintainers to weigh in on cutover vs migration):
v2:+ type tags (s2_/u2_/a2_) + unpadded Base64URL components cleanly separates tuple collisions and the olduser_state/app_statetype clash. The README cutover note is clear about not reading or migrating legacy keys.DEL/ pre-txEXISTSwere already addressed.No competing PR from me. Happy to re-check if anything else lands on the draft. Thanks again for driving this.
surajksharma07 commentedon Oct 19, 2026
Applied #7563 on today’s main (57e8bc6, still the same key format as 2.12.0) and all 45 Redis tests pass @srkyn. The 9 new ones fail on main as they should. Also ran a probe: colon aliases, the user_state/app_state names, "." inside ids, non-ASCII ids and a key_prefix with glob characters. Every tuple got its own key and an alias append or delete left the original session untouched.
One thing to look at: append_event now WATCHes the session, app-state and user-state keys together. On Redis Cluster those keys land in different hash slots so a multi-key transaction would fail with CROSSSLOT. Could you either confirm cluster mode isn’t in scope and say so in the README or cover it with a test? A smaller pre-existing point: create_session still writes app/user state before the NX check so a duplicate session_id changes state before AlreadyExistsError is raised. That’s fine to leave for a follow-up.
With the diff up passed the cutover-vs-migration question to the maintainers again. The cutover note in the README reads clearly. Once you’re happy with the cluster point feel free to mark the PR ready for review so the pending workflows can be approved.
Li-john1021 commentedon Oct 19, 2026
Thanks @surajksharma07 for re-applying #7563 on current main and for the thorough probe (aliases, dotted/non-ASCII ids, glob in key_prefix) — good to hear the 45 Redis tests pass and the 9 new ones correctly fail on main.
The Redis Cluster CROSSSLOT note on the multi-key WATCH in
append_eventlooks important; I’ll leave that to @srkyn on the draft. Happy to take another look once that point is settled and the PR is marked ready for review.srkyn commentedon Oct 20, 2026
Good catch. The service documents
redis.asyncio.Redis, notRedisCluster. This PR makes a state-bearing append watch session, app, and user keys together, which may cross Cluster hash slots. I added the native Cluster limitation to both Redis guides in b4dcdf5. The 45 Redis unit tests pass locally. A caller-injected Cluster client may have worked for some paths before this PR, so I am flagging that compatibility tradeoff for maintainer review rather than claiming a Cluster fix. I will mark the PR ready.Li-john1021 commentedon Oct 20, 2026
Thanks @srkyn for documenting the native Redis Cluster limitation on the multi-key WATCH path and for marking #7563 ready for review — that tradeoff note is clear for maintainers.
I'll take another look at the ready diff shortly; happy to leave the Cluster / cutover calls to the ADK maintainers.
surajksharma07 commentedon Oct 23, 2026
The Cluster note in b4dcdf5 reads well and the tradeoff is described accurately @srkyn. Appends without app:/user: deltas still only watch the session key so only state-bearing appends become multi-key. If maintainers want Cluster support later a shared hash tag on the app segment would keep all three keys in one slot. Fine as a follow-up rather than something for this PR.
Merged #7563 onto today’s main (7f3517e) and it merges cleanly including next to the recent mypy change in the same file. All 45 Redis tests pass and the wider sessions suite has no new failures compared with main.
Nothing else needed from your side for now. Asked the maintainers to approve the 2 pending workflows and review the cutover. @Li-john1021 thanks for the second look.
Li-john1021 commentedon Oct 23, 2026
Thanks @surajksharma07 for the recheck against current main and for confirming the Cluster note — glad to hear #7563 still merges cleanly and the Redis / sessions suites look good.
I'll leave the pending workflows and cutover review to the maintainers as you suggested. Happy to take another look if anything else comes up.
surajksharma07 commentedon Oct 27, 2026
Quick status for everyone @Li-john1021. 2.13.0 and today’s main (2084efb) still build keys as raw "{prefix}{app}:{user}:{session}" so the collision isn’t fixed in any release yet. #7563 is unchanged at b4dcdf5, nothing has touched the Redis service on main since my last check and it still merges cleanly.
The red “agent-triage-pull-request” check is the triage bot’s own job not the Redis tests. What’s blocking is still maintainer approval of the 2 pending workflows and a review with write access which I’ve chased again along with the cutover decision.
@srkyn nothing needed from you for now. If main moves under the Redis files before review a quick rebase would help.
chore(sessions): keep Redis identity guard current with main