[WEB-8332] fix(security): block workspace-member mass-assignment - #9460
[WEB-8332] fix(security): block workspace-member mass-assignment#9460mguptahub wants to merge 5 commits into
Conversation
…A-f739-39g5-jj49)
WorkSpaceMemberViewSet.partial_update passed request.data verbatim into
WorkSpaceMemberSerializer (fields="__all__"), making workspace, role, and
is_active mass-assignable. A workspace admin could PATCH a member row setting
workspace=<victim UUID> and role=20, relocating a controlled account into the
victim workspace as admin — full cross-tenant takeover.
The endpoint's only legitimate mutation is `role`, so restrict the writable
payload to {"role": ...}. Note: passing fields=("id","member","role") does NOT
work — DynamicBaseSerializer discards the fields= kwarg (base.py) — so the
allowlist is enforced in the view instead. Preserves the self-role-update guard
and the guest role-cascade. is_active is only changed via destroy(), not here.
Adds 4 contract tests; fail-before verified (2 attack tests failed unpatched → 4 pass).
Co-authored-by: Plane AI <noreply@plane.so>
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWorkspace member partial updates now whitelist ChangesWorkspace member update security
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/plane/tests/contract/app/test_workspace_member_mass_assignment_app.py`:
- Around line 99-107: Extend the mixed-payload assertions in the workspace
member update test to verify that the permitted role change is applied by
asserting puppet_member.role equals 20 after refresh_from_db(), while retaining
the existing assertions that the workspace remains unchanged and no victim
member is created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65e256f5-7f4d-4580-8509-f4bf08174ff0
📒 Files selected for processing (2)
apps/api/plane/app/views/workspace/member.pyapps/api/plane/tests/contract/app/test_workspace_member_mass_assignment_app.py
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical security vulnerability (GHSA-f739-39g5-jj49 / WEB-8332) where WorkSpaceMemberViewSet.partial_update allowed workspace admins to mass-assign sensitive WorkspaceMember fields (e.g. workspace, is_active) and perform cross-tenant workspace takeover by relocating a controlled member row into another workspace.
Changes:
- Restricts
WorkSpaceMemberViewSet.partial_updateto only acceptrolein the writable payload (view-level allowlist). - Adds contract regression tests covering the mass-assignment takeover vector and ensuring intended role updates still work.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/api/plane/app/views/workspace/member.py | Adds a view-level allowlist to prevent mass-assignment of WorkspaceMember fields via PATCH. |
| apps/api/plane/tests/contract/app/test_workspace_member_mass_assignment_app.py | Adds contract regression tests to prevent cross-workspace moves and verify allowed role updates. |
…opilot #9460) partial_update cascaded the guest project-role downgrade BEFORE the serializer was validated/saved, using a manual int() cast that 500s on non-integer input. Reorder: validate the serializer first, then cascade using serializer.validated_data["role"] inside a transaction.atomic() with save(), so a bad role returns 400 (not 500) and project roles can't be downgraded when the member update doesn't persist. Adds a non-integer-role 400 test. Co-authored-by: Plane AI <noreply@plane.so>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/plane/tests/contract/app/test_workspace_member_mass_assignment_app.py`:
- Around line 162-179: Extend test_non_integer_role_is_400_not_500 by creating a
related ProjectMember for target_member with a non-guest role before the invalid
PATCH, then refresh it and assert its role remains unchanged alongside
target_member.role. Use the existing project-member creation helpers and role
symbols visible in the test module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1339de50-6ecc-41a5-857f-0a45edaba3bb
📒 Files selected for processing (2)
apps/api/plane/app/views/workspace/member.pyapps/api/plane/tests/contract/app/test_workspace_member_mass_assignment_app.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/app/views/workspace/member.py
…id update (CodeRabbit #9460) Address two CodeRabbit review comments: - Mixed-payload takeover test now also asserts the permitted field (role) in the same payload WAS applied (role == 20), proving the fix filters the payload rather than rejecting it wholesale. - test_non_integer_role_is_400_not_500 now seeds a non-guest ProjectMember and asserts it stays role 15 after the invalid PATCH, so a regression that let the guest project-role cascade run on a failed update would be caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ld (Copilot #9460) When a PATCH carries only forbidden keys (e.g. {"is_active": false}), allowed_data is empty and the previous code still called serializer.save() — a no-op write that bumped updated_at/updated_by and ran through the save path for nothing. Early-return the current serialized member (200) when there are no allowed fields, so a forbidden-only payload has no side effects. Strengthened the is_active test to assert updated_at is unchanged (fail-before verified). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Explanations kept unchanged; only the IDs are removed. Co-authored-by: Plane AI <noreply@plane.so>
|
React Doctor found 4 issues in 4 files · 1 error & 3 warnings · score 82 / 100 (Needs work) · vs Errors
3 warnings
|
Summary
CRITICAL — fixes a cross-tenant workspace takeover (WEB-8332). Confirmed-vulnerable on
origin/preview.WorkSpaceMemberViewSet.partial_updatepassedrequest.datastraight intoWorkSpaceMemberSerializer(fields = "__all__", only nestedmemberread-only).workspace,role, andis_activewere therefore mass-assignable. An admin of an attacker-owned workspace could PATCH a member row settingworkspace=victim-workspace UUID androle=20 → relocate a controlled account into the victim workspace as an admin with no invitation. Since all authorization derives fromWorkspaceMember(workspace, member, role, is_active)rows, this is a full cross-tenant takeover.@allow_permission([ADMIN], level="WORKSPACE")authorizes against the URL slug but does not stop the write from moving the row elsewhere.Fix
The endpoint's only legitimate mutation is
role, so the writable payload is restricted to{"role": ...}in the view:Why not
fields=("id","member","role")?DynamicBaseSerializer.__init__pops thefields=kwarg and immediately overwrites it withself.expand(serializers/base.py), sofields=never restricts writes or output. The view-level allowlist is the reliable fix and is the only write path through this serializer (list/retrieve are read-only), so no other consumer is affected. The self-role-update guard and guest role-cascade are preserved;is_activeis only changed viadestroy().Tests
New
tests/contract/app/test_workspace_member_mass_assignment_app.py— 4 cases (cross-workspace move blocked;is_active=falseno-ops; legitimate role update works; self-update still 400). Fail-before verified on the CE docker test stack: 2 attack tests failed unpatched → 4 pass patched; 12/12 in a regression run with sibling member/authz suites.Related finding (separate ticket — NOT in this PR)
The root cause exposed a latent bug:
DynamicBaseSerializer'sfields=kwarg is discarded (base.py:fields = self.expand), so every caller that passesfields=(...)expecting to limit output (e.g.list/retrievehere) actually returns all model columns. That's an app-wide over-disclosure surface needing its own scoped audit + FE-compat check — filed separately; deliberately not touched here (changingbase.pywould enforce output filtering app-wide and likely break the frontend).Summary by CodeRabbit
Bug Fixes
PATCHis now restricted torole-only updates, preventing unauthorized changes to other fields (including workspace association and deactivation).PATCHare blocked, and related role effects no longer impact unrelated membership.roleinputs now return a client error without cascading guest-role behavior.Tests