Repository navigation
[WEB-8332] fix(security): block workspace-member mass-assignment - #9460
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the security fix and the affected workspace-member mass-assignment issue. Description check The description provides detailed context, implementation changes, behavior changes, test scenarios, results, and the related issue reference. It does not use the template headings or explicit change-…
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
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 @coderabbitai help to get the list of available commands.
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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>
mguptahub
changed the title
Explanations kept unchanged; only the IDs are removed. Co-authored-by: Plane AI <noreply@plane.so>
|
React Doctor skipped this pull request — it changed no React files. Reviewed by React Doctor for commit |
…-member-mass-assignment
- Reject any partial_update payload other than {"role": ...} with 400,
including non-object JSON bodies (previously 500).
- Mark workspace/is_active/audit columns read-only on the workspace
member serializers so the fix doesn't rely on the view alone.
- Save only role/updated_at/updated_by so a concurrent removal or
view_props write isn't reverted by a stale full-row save.
- Block demotion to guest when the member is the sole admin of a
project, matching leave()/destroy().
- Fix destroy()'s sole-admin guard, which compared a user id with a
WorkspaceMember id and never fired; share one helper across all three.
- Bump ProjectMember.updated_at on the guest downgrade; use ROLE.GUEST.
- Tests: cover the downgrade, sole-admin guards, non-object bodies and
an invalid role that passed the old int() check; use shared fixtures.
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:partial_updaterejects (400) any payload other than{"role": ...}, including non-object JSON bodies (previously a 500).workspace,is_activeand the audit/soft-delete columns are read-only onWorkSpaceMemberSerializer;WorkspaceMemberAdminSerializernow subclasses it, so the protection doesn't depend on this one view.role/updated_at/updated_byare saved, so a concurrent removal orview_propssave isn't reverted by a stale full-row write.ProjectMember.updated_at, and is refused (400) when the member is the only member and admin of a project — same rule asleave()/destroy().destroy()guard: the sole-project-admin check compared a user id with aWorkspaceMemberid and never fired. All three call sites now shareis_sole_admin_of_any_project().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.Behaviour changes: extra fields now return 400 instead of being ignored (the web app only sends
{role}, and shows theerrormessage in a toast). Removing or demoting a project's only member/admin now returns 400.Tests
tests/contract/app/test_workspace_member_mass_assignment_app.py— 12 cases: cross-workspace move andis_activerejected with no write; non-object bodies → 400; legitimate role update; demotion downgrades project roles; sole-project-admin demotion and removal rejected; self-update still 400; invalid roles (incl.5.0, which passed the oldint()check) → 400 with no downgrade.Fail-before: 8 of 12 fail against the previous branch head. Full backend suite on the CE docker test stack: 712 passed.
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
rolechange; requests without a role leave the member unchanged.Tests