Repository navigation
[Cherry-pick] Cherry-pick to release-v0.182.0 - #1217
truefoundry-saas merged 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 2f76b29 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| export async function up(db: Kysely<unknown>): Promise<void> { | ||
| await sql`SET LOCAL lock_timeout = '5s'`.execute(db); | ||
| await sql` | ||
| INSERT INTO access_control (tenant_id, session_id, subject_type, subject_id, granted_by_subject, created_at) |
There was a problem hiding this comment.
Medium: Sharing revocations are lost during rolling upgrades
With the chart's default rolling update, this backfill runs while old replicas still serve requests: an owner's subsequent PATCH /api/v1/sessions/{id} with {"shared":false} handled by an old replica clears only session.shared, leaving the copied tenant grant intact. Any tenant member who knows the session ID can then continue reading its conversation and events through new replicas, even after the rollout finishes.
Synchronize legacy sharing writes with grants until all old writers are drained, or stop old writers before running the backfill.
PR overviewThis PR cherry-picks changes into release-v0.182.0, including a PostgreSQL migration that backfills tenant access-control grants from existing session-sharing state. One security issue remains open, with none addressed so far. During a default rolling upgrade, revoking session sharing through an old replica can leave a backfilled tenant grant intact. Tenant members who know the session ID can consequently continue reading its conversation and events through new replicas, even after the rollout finishes. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
![qodex-ai[bot]](https://avatars.githubusercontent.com/u/8568200?s=60&v=4){kind=small}
| sessionId, | ||
| subjects: users.map(user => ({ subjectType: 'user', subjectId: user.id })), | ||
| }); | ||
| setCollaborators(granted.filter(grant => grant.subjectType === 'user')); |
There was a problem hiding this comment.
🟠 Major Invitation completion restores removed collaborators · bug
invite replaces collaborators with the grantAccess response even when removeCollaborator concurrently removes a collaborator. Successful revokeAccess completion never reconciles that replacement. A delayed invitation response restores the removed collaborator's row until another refresh. Refresh collaborators after all mutations finish, or serialize invitations and removals.
| return false; | ||
| } finally { | ||
| collaboratorMutationsRef.current--; | ||
| collaboratorListEpochRef.current++; |
There was a problem hiding this comment.
🟠 Major Failed invitation loses pending collaborator refresh · bug
invite invalidates pending refreshCollaborators calls before grantAccess completes. Its failure path advances the epoch without fetching replacement collaborators. Existing collaborators can remain hidden until the user reopens the dialog. Retry refreshCollaborators when the last mutation finishes, including after grantAccess failure.
| INSERT INTO access_control (tenant_id, session_id, subject_type, subject_id, granted_by_subject, created_at) | ||
| SELECT tenant_id, session_id, 'tenant', tenant_id, created_by_subject, now() | ||
| FROM session | ||
| WHERE shared |
There was a problem hiding this comment.
🟠 Major Sharing divergence during rolling upgrades · security
The migration copies session.shared into tenant grants without ongoing synchronization. Older pods update session.shared, while setSharedWithTenant updates only grants. Revocations leave the other version's permission intact. Tenant members retain access after revocation through the other version. New pods also miss sharing changes older pods make after backfill. Synchronize session.shared and tenant grants throughout mixed-version deployment. Remove synchronization only after all older pods stop.
| throw error; | ||
| } | ||
| if (body.shared !== undefined) { | ||
| await setSharedWithTenant(deps.accessControlStore, { |
There was a problem hiding this comment.
🟠 Major Sharing revocations diverge during rolling upgrades · security
setSharedWithTenant changes tenant grants without updating session.shared. The migration backfills grants once, so older writers also leave grants stale. Older servers can authorize revoked sharing during rolling upgrades. Newer servers can retain access after older servers revoke sharing. Synchronize session.shared and tenant grants throughout the compatibility window, including writes from older servers.
| @classmethod | ||
| def _missing_(cls, value: typing.Any) -> "SessionGrantSubjectType": | ||
| unknown = cls._UNKNOWN | ||
| unknown._value_ = value |
There was a problem hiding this comment.
🟡 Minor Shared unknown enum value mutation · bug
missing overwrites the shared _UNKNOWN member's value for every unrecognized value. Parsing a second unknown subject type changes the first result's .value and loses its original subject type. Create a separate enum instance for each unknown value instead of mutating _UNKNOWN.
Qodex reviewReviewed units u1, u2, u3, u4, u5, u6, u7, u8. The changeset announces tenant collaborator filtering. Effort: ●●●●● (5/5) · 5 findings: 4 major, 1 minor Scope: the whole pull request (no previous review: the whole change is reviewed). Qodex review threads: 5 open, 0 resolved by Qodex. 5 inline comments attached to specific lines below. WalkthroughNotes
What Qodex checkedLenses (openqodex review with checked answers, all dimensions)
Scanners
Quality filters
Verification (M1b probes against PR preview)
Cross-review dedup
Output
Reviewer: shown 303 of 303 changed hunks, opened 42 files (43 changed files never opened), 188 tool calls served, 106 model calls, 2 correction rounds, 314.7s. The library does not report tool calls asked apart from those it served, or a step cap. Every dimension is reviewed. Bugs, security, and performance post inline; low-severity style and convention notes are collected in the group above rather than dropped. Mark a finding resolved or flag a false positive in the Qodex Pull Requests view. |
Automated cherry-pick for release request
909501aa-b10d-410b-9680-498ca73a0fff.Source PRs (merged to main):
Do not merge manually — this PR is merged automatically when the release request is approved. If you do merge it yourself, use "Sync status" on the request so it stops showing as pending.
Note
High Risk
Changes authorization for session reads and migrates sharing state; incorrect grant or migration logic could expose or hide session content.
Overview
Session sharing moves from a persisted
sharedflag on session rows toaccess_controlgrants: tenant-wide access is atenantsubject grant, and specific users getusergrants with read access to the session, turns, and events. API responses still exposeSession.shared, computed from the tenant grant; deprecatedPATCH … { shared }now creates or deletes that tenant grant. A DB migration backfills existing shared sessions into tenant grants, andcanReadSessionauthorizes reads via grant lookup (plus existing creator/agent-manager rules).OpenAPI/SDK adds
session_sharing.enabledon capabilities (true when the host has a user directory), extends grant subjects to includetenant, and documents grant semantics.UI/runtime introduces an optional
sessionAccessserver port (list/grant/revoke + user search). The share dialog can invite collaborators when capabilities allow, lists owner and user grants (tenant grants stay on the tenant/private selector), and the Harness adapter wires grants and user APIs—filtering tenant grants out of the collaborator list.Reviewed by Cursor Bugbot for commit 2f76b29. Bugbot is set up for automated code reviews on this repo. Configure here.