Skip to content

fix(groups): guard member mutations against stale roles - #626

Merged
junhoyeo merged 1 commit into
junhoyeo:mainfrom
IvGolovach:codex/group-member-race-safe-20260528
May 30, 2026
Merged

fix(groups): guard member mutations against stale roles#626
junhoyeo merged 1 commit into
junhoyeo:mainfrom
IvGolovach:codex/group-member-race-safe-20260528

Conversation

@IvGolovach

@IvGolovach IvGolovach commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Recheck and lock actor/target group membership rows inside the DELETE member transaction before removing a member.
  • Recheck and lock actor/target roles inside the PATCH role transaction before changing a member role.
  • Preserve private-group not-found behavior and best-effort cache revalidation while making stale authorization fail safely.
  • Add focused regression tests for stale actor state, stale target state, owner protection, and last-owner role safety.

Why

Group member mutations previously authorized from membership state loaded before the write. A concurrent demotion, removal, or target role change could make that authorization stale by the time the mutation executed. This PR moves the decisive actor/target role checks into the mutation boundary so group admins cannot accidentally act with stale privileges and owner invariants are preserved under concurrent changes.

Diff scope

  • packages/frontend/src/app/api/groups/[slug]/members/route.ts
  • packages/frontend/src/app/api/groups/[slug]/members/[userId]/role/route.ts
  • packages/frontend/__tests__/api/groupMemberDeleteRoute.test.ts
  • packages/frontend/__tests__/api/groupMemberRoleRoute.test.ts

Branch integrity

  • Base branch: main
  • Validated base SHA: c21f0f533205ad63e9154666c294bb789d2415c9
  • Head branch: IvGolovach:codex/group-member-race-safe-20260528
  • Head SHA: 657514bf3bc979826d24ce9607d8a2b4c8e3585a
  • Ahead/behind vs fetched origin/main: 1 ahead / 0 behind
  • Commit: 657514bf3bc979826d24ce9607d8a2b4c8e3585a fix(groups): guard member mutations against stale roles

Validation

  • bun run test __tests__/api/group*.test.ts __tests__/lib/groupHelpers.test.ts from packages/frontend: PASS, 49 tests passed.
  • bun x eslint 'src/app/api/groups/[slug]/members/route.ts' 'src/app/api/groups/[slug]/members/[userId]/role/route.ts' __tests__/api/groupMemberDeleteRoute.test.ts __tests__/api/groupMemberRoleRoute.test.ts from packages/frontend: PASS, no output.
  • git diff --check origin/main...HEAD: PASS, no output.
  • git status --short --untracked-files=all: PASS, clean worktree.

Runtime safety

  • No new external services, queues, timers, or background jobs.
  • Mutations now perform the decisive role checks inside the transaction before delete/update.
  • Cache invalidation remains best-effort and still cannot mask a successful database mutation.
  • No invariant regression introduced.

Migration notes

Not applicable - no database migration or schema change.

Rollback plan

Rollback: revert this PR. DB downgrade: not applicable. Data repair: not applicable. Operational caveats: none known.

Known residual risks

  • Remote CI should provide the final full-suite proof for the PR SHA.
  • The transaction-level behavior depends on the production database honoring row locks for the selected membership rows, matching the existing Postgres-backed route patterns.

Summary by cubic

Prevents stale-role race conditions in group member delete and role update by rechecking and row-locking actor/target memberships inside the transaction with a deterministic lock order. This blocks actions with outdated privileges and preserves owner safety.

  • Bug Fixes
    • DELETE members: lock actor/target rows, authorize via canManageGroupRole, return 403 on stale roles and 404 when missing; preserve private-group 404 behavior.
    • PATCH role: lock actor/target rows in sorted user-id order, forbid updates if target is now owner, reject demoting the last owner (400) after locking owners, guard updates by matching the current target role; return 403/404 as appropriate.
    • Keep cache revalidation best-effort post-mutation.
    • Add regression tests for stale actor/target state, deterministic lock ordering, owner protection, and last-owner safety.

Written for commit 899a602. Summary will update on new commits.

Review in cubic

@vercel

vercel Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

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

Project Deployment Actions Updated (UTC)
tokscale Ready Ready Preview, Comment May 28, 2026 6:41pm

Request Review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 4 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Validation
* Validation tier: Tier 3 - High-Risk Runtime / security-sensitive group membership permission mutation.
* bun run --cwd packages/frontend test __tests__/api/groupMemberDeleteRoute.test.ts __tests__/api/groupMemberRoleRoute.test.ts __tests__/api/groupsPayloadGuard.test.ts: PASS, 16/16 tests.
* cd packages/frontend && bun run test __tests__/api/group*.test.ts __tests__/lib/groupHelpers.test.ts: PASS, 50/50 tests.
* bun run --cwd packages/frontend lint -- 'src/app/api/groups/[slug]/members/route.ts' 'src/app/api/groups/[slug]/members/[userId]/role/route.ts' __tests__/api/groupMemberDeleteRoute.test.ts __tests__/api/groupMemberRoleRoute.test.ts __tests__/api/groupsPayloadGuard.test.ts: PASS, no output.
* git diff --check: PASS.
* git diff --cached --check: PASS.
* Ledger: not applicable - not required for selected validation tier/change family.
* Version: not applicable - not required for selected validation tier/change family.
* Not run: full frontend build/coverage - not required for selected validation tier; targeted group route tests and touched-file lint covered this change.

Rollback
* git revert HEAD
@IvGolovach
IvGolovach force-pushed the codex/group-member-race-safe-20260528 branch from 657514b to 899a602 Compare May 28, 2026 18:40
@junhoyeo
junhoyeo merged commit eb3e6dc into junhoyeo:main May 30, 2026
6 checks passed
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.

2 participants