Skip to content

improvement(permissions): confine workspace role changes to existing members - #6180

Merged
icecrasher321 merged 2 commits into
stagingfrom
staging-v22
Aug 2, 2026
Merged

improvement(permissions): confine workspace role changes to existing members#6180
icecrasher321 merged 2 commits into
stagingfrom
staging-v22

Conversation

@icecrasher321

Copy link
Copy Markdown
Collaborator

Summary

Fixes a workspace-access bypass. PATCH /api/workspaces/[id]/permissions accepted arbitrary userIds and inserted permission rows for them, letting any workspace admin attach an account — and, through the credential sync this route performs, hand it the workspace’s secrets — without the invitation flow’s plan check, seat provisioning, or invitee consent. The write is now a conditional UPDATE of an existing row so introducing a member is structurally unreachable, and every state-dependent guard (owner, organization admins, billing account, the caller’s own authority) is re-evaluated under lock, since an admin could otherwise drive an ownership transfer against their own in-flight batch and strand a user as workspace owner with read. Reviewer-visible changes: permissions.createdAt now means join time rather than last-role-change (documented on the admin members endpoint), the PATCH response is an acknowledgement only, and the owner/billing-account role controls are disabled in the members lists — all three were offering or reporting something the server never honored.

Type of Change

  • Other: Security

Testing

Tests added

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Aug 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 2, 2026 1:50am

Request Review

@cursor

cursor Bot commented Aug 2, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes authentication/authorization and workspace membership writes, including a security fix that alters how roles are granted and how concurrent updates behave; regressions could block legitimate admins or leave inconsistent credential sync (logged for reconciliation).

Overview
Closes a workspace-access bypass on PATCH /api/workspaces/[id]/permissions: the route no longer inserts permission rows for arbitrary userIds. Role changes are in-place UPDATEs for people who are already members (explicit row or org admin), so adding collaborators stays on the invitation flow and its plan/seat/consent gates.

The handler is hardened for concurrency: ordered FOR UPDATE locks, lock_timeout with withTransactionRetry, re-checks of caller admin standing, owner/billing/org-admin rules, and membership under lock. Stale or racing state returns 409 with actionable messages instead of silent success or 500s. PATCH now returns a simple acknowledgement (not a full members payload); createdAt on permission rows is join time because delete+insert was removed.

APIs and UI stay aligned: roster and workspace member payloads add roleSource and isBilledAccount; teammates and org roster disable role chips for owner, org admin, and billing account (matching server guards). Admin workspace member create/update uses conflict responses and upsert on concurrent adds. useUpdateWorkspacePermissions surfaces validation and conflict errors via toast.

Reviewed by Cursor Bugbot for commit 21820e0. Configure here.

Comment thread apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts
Comment thread apps/sim/app/api/workspaces/[id]/permissions/route.ts
@greptile-apps

greptile-apps Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR confines workspace role changes to existing members and hardens concurrent permission updates.

  • Replaces permission-row recreation with locked, conditional in-place updates.
  • Revalidates caller authority, inherited roles, ownership, billing identity, and membership inside the transaction.
  • Retries PostgreSQL contention failures and returns an actionable conflict after retries are exhausted.
  • Aligns API contracts and member-list controls with immutable roles.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/app/api/workspaces/[id]/permissions/route.ts Restricts role changes to existing permission rows and revalidates all state-dependent authorization constraints under ordered locks.
apps/sim/lib/db/transaction.ts Adds bounded transaction retries for serialization failures, deadlocks, and lock timeouts.
apps/sim/lib/api/contracts/workspaces.ts Updates the permission mutation contract to validate bounded, unique member-role changes and the acknowledgement response.
apps/sim/app/api/organizations/[id]/roster/route.ts Adds role-source and billing-account metadata needed to lock immutable role controls.
apps/sim/components/permissions/role-lock.tsx Centralizes UI role-lock behavior for owners, organization administrators, and billing accounts.
apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts Correctly distinguishes concurrent membership creation from conflict-driven role updates in responses and audit events.

Sequence Diagram

sequenceDiagram
  participant Admin
  participant Route as Permissions API
  participant DB as PostgreSQL
  Admin->>Route: PATCH existing member roles
  Route->>Route: Authenticate and validate request
  Route->>DB: Read membership and workspace context
  Route->>DB: Begin transaction and set lock timeout
  Route->>DB: Lock org member, workspace, and permission rows
  Route->>Route: Revalidate authority and protected roles
  alt Membership and authority remain valid
    Route->>DB: Update existing permission rows
    DB-->>Route: Commit
    Route-->>Admin: 200 acknowledgement
  else State changed concurrently
    DB-->>Route: Roll back
    Route-->>Admin: 409 conflict
  else Lock contention
    Route->>DB: Retry in a fresh transaction
    Route-->>Admin: 409 if retries are exhausted
  end
Loading

Reviews (2): Last reviewed commit: "improvement(permissions): retry lock tim..." | Re-trigger Greptile

Comment thread apps/sim/lib/db/transaction.ts Outdated
icecrasher321 and others added 2 commits August 1, 2026 18:48
… truthfully

Bounding the row-lock wait with `lock_timeout` made a competing writer raise
55P03, which the retry set excluded — so the bounded wait failed the request
where the unbounded one would have waited. 55P03 is now retried like the other
contention aborts, the timeout is shortened since each attempt releases its
pooled connection, and contention that outlives every attempt answers with a
busy conflict instead of a generic server error.

The admin member upsert recorded MEMBER_ADDED even when the conflict branch
amended an existing membership, putting a join that never happened in the
workspace audit trail. It now records a role change, matching the action the
response already reported.

Adds the missing test coverage for withTransactionRetry.

Co-Authored-By: Claude <noreply@anthropic.com>
@icecrasher321

Copy link
Copy Markdown
Collaborator Author

@greptile

@icecrasher321

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 21820e0. Configure here.

@icecrasher321
icecrasher321 merged commit e90378a into staging Aug 2, 2026
27 checks passed
@icecrasher321
icecrasher321 deleted the staging-v22 branch August 2, 2026 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant