Repository navigation
tech-debt(security): two different validate_session_ownership functions — same name, different signature and semantics #14016
Description
Found during review of PR #14013 (#14011). Not a live bug — every current caller imports the right one — but it is a name collision on a security-critical helper, which is a poor thing to leave to luck.
The two
| module | signature | semantics |
|---|---|---|
autobot-backend/security/session_ownership.py |
validate_session_ownership(session_id: str, request: Request) -> Dict |
the canonical validator; FastAPI-dependency shaped, integrates feature flags, metrics and audit |
autobot-backend/api/conversation_files.py |
validate_session_ownership(request: Request, session_id: str, user_data: Dict) -> bool |
a different helper: different parameter order, extra required argument, returns a bool |
They differ in arity, parameter order, and return type, so a wrong import is not a subtle behaviour change — but the two most plausible mistakes both fail in the wrong direction:
- used as a FastAPI dependency, the
conversation_filesone would have FastAPI try to resolveuser_dataas a query parameter - called positionally with the canonical argument order,
requestandsession_idare swapped, so the id being validated is not the id being used
The second is the shape that produces a check that runs, passes, and guards nothing — the exact failure mode #13982 and #14011 were about.
Why it is worth fixing rather than noting
grep -rn "def validate_session_ownership" returns two hits and no indication which is canonical. Someone adding an ownership check to a new endpoint — which is now a recurring task, see #13982, #14011, #14012 — has a 50% chance of importing the wrong one from a bare grep.
This is the same class as the same-name/different-concept enum forks (#13845/#13846): two implementations of one idea that only a reader can tell apart.
Suggested fix
Rename the conversation_files one to what it actually is (it takes pre-fetched user_data and returns a bool — something like _verify_session_owner_for_user, and make it private if it has no external callers), leaving exactly one public validate_session_ownership in the codebase.
Consolidating them is the alternative but likely wrong: they answer different questions and one is dependency-shaped. One canonical name plus one clearly-scoped private helper is the smaller, safer change.
Acceptance criteria
-
grep -rn "def validate_session_ownership"returns exactly one definition - Every caller of the renamed helper updated; no behaviour change
- The canonical validator's docstring states it is the one to use for endpoint ownership checks
Related
- security(chat): send_direct_chat_response has no ownership check — any user can inject into any chat session #13982 / PR security(chat): require chat ownership on /chat/direct (#13982) #14009, security(chat): export_session has no ownership check — any authenticated user can export another user's full transcript #14011 / PR security(chat): require ownership on export_session and reset_chat (#14011) #14013, security(chat): create_session accepts a client-supplied id with no collision guard — wipes another user's transcript and reassigns ownership #14012 — the ownership-check work that surfaced this
- security(chat): ownership enforcement fails OPEN — an unavailable feature-flag service disables every check platform-wide #14010 — ownership enforcement fails open
- canonical(enums): CommandRisk and CommandRiskLevel model the same concept with divergent tails — FORBIDDEN has no wire representation #13845 / canonical(enums): SecretType and SecretRequirement duplicate the secret taxonomy — agents cannot express an OAuth requirement #13846 — same-name/different-concept forks, same class of hazard
Activity
mrveiss commentedon Oct 10, 2026
Decision needed (owner), security. The de-fork stopped before merging, because a merge would widen access. The two
validate_session_ownershipfunctions are genuinely different security postures. Decision table (read from code) in short: if the conversation-files routes moved onto the canonicalsecurity/session_ownership.pyvalidator, they would ALLOW what they deny today in 7 cases: auth disabled, enforcementdisabled,log_onlynon-owner, flags unavailable (degrades to log_only), user with no role, session shared with user (#689), and owner unreadable. Conversely, the canonical validator denies a global admin outside the session's org, which the file routes allow.Options:
7d4c136354onissue-14016-session-ownership, NOT queued): the file routes' check becomes private_authorize_session_for_user, with one public name and a guard. It widens nothing but contradicts the standing rule "merge, never rename apart".strict=Trueposture for the file routes. No flag or fast paths, role required, unreadable owner denies, no shared or org grants, global admin allowed. One function, two named postures, and a sign-off that the strict posture belongs in the canonical module.security/session_ownership.pyas a separately named helper. Same behaviour as 1, in one module.Separate security finding in the canonical validator (it applies whatever is chosen):
SessionOwnerUnreadableis swallowed and treated as "unowned", so the caller is allowed and claims the session. The file routes deny in that case (#14033). That is fail-open on a read error. The docstring calls the legacy-claim fallback deliberate, but an unreadable owner is not an unowned session.mrveiss commentedon Oct 10, 2026
Correction to the comment above: the fail-open finding (
SessionOwnerUnreadabletreated as unowned in the canonical validator) is already tracked as #17481. It is not a new finding, and its fix lives there.mrveiss commentedon Oct 10, 2026
Owner decision (2026-10-10): option 2, a real merge. The canonical validator gains an explicit
strictposture for the file routes: no flag or fast paths, role required, an unreadable owner denies, no shared or org grants, global admin allowed. One function with two named postures, plus a guard. Also fix the fail-open:SessionOwnerUnreadablemust deny, never count as "unowned" and let the caller claim the session (#14033).test(security): the file-route guard requires the helper's own STRICT…
test(security): the session-posture lookup-failure rows are selected …
docs(security): THREAT_MODEL anchors point at today's lines, and the …
chore(train): merge train 2026-10-11 #2 — 4 approved PRs + reach_scop…
mrveiss commentedon Oct 11, 2026
Closure evidence (merged in merge train #18314, 5efe902; constituent #18258). Verified against
main.def validate_session_ownership:autobot-backend/security/session_ownership.py:745, guarded byrepo_tests/one_session_ownership_validator_14016_test.py(an AST check with a reach floor).SessionPosture.STRICT(autobot-backend/api/conversation_files.py:108,513). The only verdict changes are deliberate: an owner-lookup failure now denies (security(chat): get_session_owner cannot distinguish 'no owner' from 'unreadable', and callers now WRITE on that answer #14033, security(chat): the ownership gate still claims a session whose owner record is unreadable — #14033's contract is not honoured by SessionOwnershipValidator #17481), andautobot-backend/security/session_posture_equivalence_14016_test.py:210-217pins exactly those rows.session_ownership.py:749, "The ONE endpoint ownership dependency (tech-debt(security): two different validate_session_ownership functions — same name, different signature and semantics #14016)".