Repository navigation
FirestoreSessionService rejects valid ids "__" and "___" as reserved #7855
Description
Describe the Bug
#7801 (fix for #7800) added FirestoreSessionService._validate_identity_component, which rejects ids matching Firestore's reserved __.*__ pattern with:
if value.startswith("__") and value.endswith("__"):This is broader than the documented rule. __.*__ needs at least four characters, but the check above also matches "__" and "___", because the prefix and suffix overlap. Those two ids are valid Firestore document ids and do not change the document path, yet they are now rejected.
Minimal Reproduction
import re
from google.adk.integrations.firestore.firestore_session_service import (
FirestoreSessionService,
)
for v in ["__", "___", "____", "__x__"]:
reserved = re.fullmatch(r"__.*__", v) is not None
try:
FirestoreSessionService._validate_identity_component("user_id", v)
rejected = False
except ValueError:
rejected = True
print(repr(v), "reserved:", reserved, "rejected:", rejected)On current main:
'__' reserved: False rejected: True <- wrong
'___' reserved: False rejected: True <- wrong
'____' reserved: True rejected: True
'__x__' reserved: True rejected: True
Expected Behavior
Only ids that fully match __.*__ are rejected, so "__" and "___" are accepted like any other id that does not alter the path.
Impact
Low. It only affects the ids "__" and "___". Sessions that were already stored under those ids can no longer be read or deleted through the service. I introduced this boundary in #7801. The path-injection fix itself is unaffected.
Proposed Fix
Require a minimum length of four characters, which makes the check equivalent to re.fullmatch(r"__.*__", value):
if len(value) >= 4 and value.startswith("__") and value.endswith("__"):Environment
- ADK main at commit
c501d8e
Activity
CorgiBoyG commentedon Oct 30, 2026
I'll take this, since the boundary came from my change in #7801. I have a one-line fix and regression tests ready (the check is verified to be equivalent to
re.fullmatch(r"__.*__", value)) and will open a PR shortly.surajksharma07 commentedon Oct 30, 2026
Thanks for catching this so quickly @CorgiBoyG. Reproduced it on current main: "" and "_" both get rejected by _validate_identity_component. It only affects main for now since ec9756e isn’t in any release yet (v2.13.0 and earlier aren’t affected).
The len(value) >= 4 guard you suggested worked well on my end. ""/"" pass and "___", "x", "/", "." and ".." are still rejected. Could you double-check it against your own setup too?
#7856 and #7858 both target this same fix so it might be worth checking with @ninglinLiu so we only keep one. Once you’ve confirmed the tests pass we’ll get it in front of the reviewers.
CorgiBoyG commentedon Nov 1, 2026
Thanks @surajksharma07 for reproducing it and for the extra cases.
I re-checked on current main (95d8610) with #7858 cherry-picked on top:
"__"/"___"raise in both_validate_identity_componentandcreate_session) and pass with the fixtests/unittests/integrations/firestore/: 88 passedtests/unittests/sessions/: 732 passed, 1 xfailedI also pulled the head of #7856 and verified it fixes the reported cases as well; the full firestore suite passes with its change applied. I'm happy for whichever PR you'd prefer to take. Two small differences I noticed while comparing, in case they help decide:
"__"and"___". I checked exhaustively over strings up to length 7 on the alphabet_,a,\n,\r(21,845 values): no other id changes classification, and the guard is equivalent tore.fullmatch(r"__.*__", v, re.DOTALL). fix(firestore): only reject ids that fully match __.*__ #7856 usesre.fullmatch(r"__.*__", value)withoutre.DOTALL, which additionally accepts ids such as"__\n__"that main rejects today (45 values in that set; none contain/, so the path-injection fix from fix(firestore): validate session identity components before path use #7801 is unaffected). An edge case either way — fix(firestore): only reject ids fully matching the reserved __.*__ pattern #7858 just keeps the behavior change strictly minimal.import reis added beforeimport logging/import os; isort with the repo's google profile would place it afteros. Trivial to fix if that PR is the one that goes in.@ninglinLiu no problem on my side either way — happy to close #7858 if you'd like to take this one forward.
fix(firestore): only reject ids fully matching the reserved __.*__ pa…
fix: only reject Firestore ids fully matching __.*__
surajksharma07 commentedon Nov 3, 2026
Thanks @CorgiBoyG, really appreciate the thorough comparison. The re.DOTALL point on "\n" was a good catch and a fair reason to keep the change minimal. #7858 has landed on main in e533f55 and I re-ran the validator from current main: ""/"" now pass while "___", "x", "\n", "/", "." and ".." are still rejected.
Both ec9756e and e533f55 came in after v2.13.0 so they’ll go out together in the next release and the regression never reaches a released version.
@ninglinLiu thanks for jumping on this as well. Since it’s fixed on main could you close #7856 as superseded? This issue is already closed by the merge so nothing else needed here.
CorgiBoyG commentedon Nov 3, 2026
Thanks @surajksharma07 for the quick reproduction, the careful review, and for confirming the release scope. And thanks @ninglinLiu for the parallel fix as well — glad this is settled on main.