fix(session-ingest): enforce current organization access#4408
fix(session-ingest): enforce current organization access#4408eshurakov wants to merge 2 commits into
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryReviewed the full PR at HEAD Files Reviewed (26 files)
Previous Review Summary (commit 00d7f96)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 00d7f96)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the organization-membership access-revocation changes across the Session Ingest Worker, its Durable Object cache, the shared session-access query helper, and the web app's invalidation call and member-removal flow with high confidence; the new authorization checks, TTL-bounded cache, and best-effort invalidation paths are logically consistent with the stated design and no security, correctness, or reliability issues were found in the changed lines. Files Reviewed (25 files)
Reviewed by claude-sonnet-5 · Input: 64 · Output: 13.1K · Cached: 3.2M Review guidance: REVIEW.md from base branch |
eshurakov
force-pushed
the
fix/session-ingest-access-revocation
branch
from
00d7f96 to
ee0841e
Compare
- Move session-access invalidation into removeUserFromOrganization so every removal path (including child-organization sync) purges cached access, not just the members.remove procedure - Purge expired rows in SessionAccessCacheDO.putValidated so per-user DO storage stays bounded - Document the timing-equalization branch in hasValidInternalSecret - Introduce producer-neutral AccessibleSession type; keep AccessibleCloudAgentSession as an alias for existing consumers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Why
Session Ingest operations (ingest, share, unshare, delete, and export) previously verified only that a session row existed for the requesting user. They did not check whether the user still has access to the owning organization, so a removed member could continue to publish events or manage sessions until their token expired. This change makes current organization membership part of the authorization decision.
What was done
resolveAccessibleKiloSessionto the Session Ingest Worker, which joinscli_sessions_v2with currentorganization_membershipsand caches positive results in the per-userSessionAccessCacheDO.organization_idand a fixedauthorization_expires_atTTL; legacy cache rows expire immediately via a generated SQLite migration.POST /internal/session-access/invalidateendpoint and call it from the web app when an organization member is removed.has/addDurable Object RPCs as no-ops for mixed-version rollout compatibility.High-level architecture
sequenceDiagram participant Client as CLI / Extension participant Web as Web App (tRPC) participant SIW as Session Ingest Worker participant DO as SessionAccessCacheDO participant PG as Postgres (Hyperdrive) Client->>SIW: ingest / share / unshare / delete SIW->>DO: getAccess(sessionId) alt cache hit DO-->>SIW: cached organizationId else cache miss SIW->>PG: query session + current membership PG-->>SIW: accessible session SIW->>DO: putValidated(sessionId, organizationId) end SIW-->>Client: result Web->>Web: remove organization member Web->>SIW: POST /internal/session-access/invalidate SIW->>DO: invalidateOrganization(organizationId)Architecture decision
Decision: Cache positive authorization results in a per-user Durable Object with a fixed 60-second TTL and best-effort invalidation.
Context: Session Ingest is on the hot path for every CLI/extension event. A Postgres lookup on every request adds latency and load, but checking only the session row would miss organization membership revocation.
Rationale: The Durable Object is already part of the Session Ingest architecture and provides strongly consistent storage per user. A fixed TTL bounds the worst-case staleness, while Postgres remains authoritative on cache misses.
Alternatives considered:
Consequences: Ingest paths avoid most Postgres authorization lookups; removed members lose access within the 60-second TTL window (or immediately on successful invalidation). Best-effort invalidation means membership removal does not fail if the cache is unreachable.
Verification
No manual verification was performed. Behavior is covered by Session Ingest unit tests, Worker integration tests, and targeted web tests.
Visual Changes
N/A
Reviewer Notes
0003_free_valkyrie.sql) expires legacy cache rows immediately by adding a non-nullauthorization_expires_atdefault of0.has/addRPCs are kept for mixed-version deployment compatibility and should be removed once all Workers are updated.