fix: prevent privilege escalation in project member role updates#8833
Conversation
…A-494h-3rcq-5g3c) Restrict role modification in ProjectMemberViewSet.partial_update to Admins only and enforce that requesters cannot modify or assign roles equal to or higher than their own. Previously, Guests could demote Admins by exploiting a missing lower-bound check on role changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Fixes a reported privilege-escalation path in the API endpoint that updates project member records by tightening authorization rules around project role changes in ProjectMemberViewSet.partial_update.
Changes:
- Gates role updates so only project Admins can modify roles (with intended workspace-admin bypass).
- Prevents updating roles of members with equal/higher roles and prevents assigning a role equal/higher than the requester’s.
- Keeps a workspace-role ceiling for workspace Guests (cannot be assigned Member/Admin at project level).
| if "role" in request.data: | ||
| # Only Admins can modify roles | ||
| if requested_project_member.role < ROLE.ADMIN.value and not is_workspace_admin: | ||
| return Response( | ||
| {"error": "You do not have permission to update roles"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
|
|
||
| if ( | ||
| "role" in request.data | ||
| and int(request.data.get("role", project_member.role)) > requested_project_member.role | ||
| and not is_workspace_admin | ||
| ): | ||
| return Response( | ||
| {"error": "You cannot update a role that is higher than your own role"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
| # Cannot modify a member whose role is equal to or higher than your own | ||
| if project_member.role >= requested_project_member.role and not is_workspace_admin: |
There was a problem hiding this comment.
is_workspace_admin is computed from the target member’s WorkspaceMember.role (the user being edited), but it’s used here as the bypass condition for the requester (e.g., ... and not is_workspace_admin). This lets a non-admin requester bypass all role checks when editing a workspace admin target, and it also fails to grant the intended bypass to actual workspace-admin requesters. Consider fetching the requester’s workspace role separately (e.g., requester_workspace_role) and using that for bypass decisions; keep the target’s workspace role only for validating what roles can be assigned to that target.
| new_role = int(request.data.get("role")) | ||
|
|
||
| # Cannot assign a role equal to or higher than your own | ||
| if new_role >= requested_project_member.role and not is_workspace_admin: | ||
| return Response( |
There was a problem hiding this comment.
new_role = int(request.data.get("role")) can raise ValueError/TypeError (e.g., role is "", null, or a non-numeric string), resulting in a 500 before serializer validation runs. Handle coercion defensively (catch conversion errors and return a 400 with a validation-style error), or rely on the serializer’s validated value instead of casting directly from request.data.
| {"error": "You cannot add a user with role higher than the workspace role"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
There was a problem hiding this comment.
The workspace-role constraint enforced during create() (workspace admins cannot be added with a lower project role) isn’t enforced here: a workspace admin target could be updated to Member/Guest, which appears to violate the invariant enforced at creation time. If that invariant is required, add the symmetric lower-bound check for workspace_role == ROLE.ADMIN.value during role updates as well (or document why updates are allowed to diverge).
| # Enforce invariant: workspace admins must not have a lower project role | |
| if is_workspace_admin and new_role < ROLE.ADMIN.value: | |
| return Response( | |
| {"error": "You cannot set a workspace admin's project role lower than admin"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) |
| if "role" in request.data: | ||
| # Only Admins can modify roles | ||
| if requested_project_member.role < ROLE.ADMIN.value and not is_workspace_admin: | ||
| return Response( | ||
| {"error": "You do not have permission to update roles"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
This change introduces security-critical authorization logic for role updates, but there doesn’t appear to be contract/unit coverage for the project member role-update endpoint. Given existing contract tests for other project/workspace endpoints under apps/api/plane/tests/contract/, add automated tests for the scenarios in the PR test plan (Guest/Member forbidden; Admin demotion only; workspace-admin bypass; non-role patch still allowed).
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/app/views/project/member.py (1)
210-213:⚠️ Potential issue | 🔴 CriticalCritical: Authorization bypass checks the wrong user's workspace role
The
is_workspace_adminvariable is derived from the target member's workspace role (line 210-213), but it's used to bypass authorization for the requester (lines 231, 238, 247). This inverts the security logic:Attack scenario: A Guest (requester) targeting a Workspace Admin (target):
workspace_role= 20 (target's WS role) →is_workspace_admin= True- Line 231:
5 < 20 and not True= False → check bypassed- Line 238:
20 >= 5 and not True= False → check bypassed- Line 247:
5 >= 5 and not True= False → check bypassed- Guest successfully demotes Workspace Admin
The fix must check the requester's workspace role for the authorization bypass.
🔒 Proposed fix
# Fetch the workspace role of the project member - workspace_role = WorkspaceMember.objects.get( + target_workspace_role = WorkspaceMember.objects.get( workspace__slug=slug, member=project_member.member, is_active=True ).role - is_workspace_admin = workspace_role == ROLE.ADMIN.value + + # Fetch the workspace role of the requester for authorization bypass + requester_workspace_role = WorkspaceMember.objects.get( + workspace__slug=slug, member=request.user, is_active=True + ).role + is_requester_workspace_admin = requester_workspace_role == ROLE.ADMIN.value # Check if the user is not editing their own role if they are not an admin - if request.user.id == project_member.member_id and not is_workspace_admin: + if request.user.id == project_member.member_id and not is_requester_workspace_admin: return Response( {"error": "You cannot update your own role"}, status=status.HTTP_400_BAD_REQUEST, )Then update lines 231, 238, and 247 to use
is_requester_workspace_admin, and line 254 to usetarget_workspace_role:if "role" in request.data: # Only Admins can modify roles - if requested_project_member.role < ROLE.ADMIN.value and not is_workspace_admin: + if requested_project_member.role < ROLE.ADMIN.value and not is_requester_workspace_admin: return Response( {"error": "You do not have permission to update roles"}, status=status.HTTP_403_FORBIDDEN, ) # Cannot modify a member whose role is equal to or higher than your own - if project_member.role >= requested_project_member.role and not is_workspace_admin: + if project_member.role >= requested_project_member.role and not is_requester_workspace_admin: return Response( {"error": "You cannot update the role of a member with a role equal to or higher than your own"}, status=status.HTTP_403_FORBIDDEN, ) new_role = int(request.data.get("role")) # Cannot assign a role equal to or higher than your own - if new_role >= requested_project_member.role and not is_workspace_admin: + if new_role >= requested_project_member.role and not is_requester_workspace_admin: return Response( {"error": "You cannot assign a role equal to or higher than your own"}, status=status.HTTP_403_FORBIDDEN, ) # Cannot assign a role higher than the target's workspace role - if workspace_role in [5] and new_role in [15, 20]: + if target_workspace_role in [5] and new_role in [15, 20]: return Response( {"error": "You cannot add a user with role higher than the workspace role"}, status=status.HTTP_400_BAD_REQUEST, )Also applies to: 229-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/views/project/member.py` around lines 210 - 213, The code currently reads the target member's workspace role into workspace_role and uses it for requester authorization checks; change this to read both roles: fetch target_workspace_role = WorkspaceMember.objects.get(workspace__slug=slug, member=project_member.member, is_active=True).role and fetch requester_workspace_role = WorkspaceMember.objects.get(workspace__slug=slug, member=request.user, is_active=True).role (or the equivalent requesting user variable), then set is_requester_workspace_admin = requester_workspace_role == ROLE.ADMIN.value and use is_requester_workspace_admin in the bypass checks (replace occurrences that used is_workspace_admin), and where the target's role is needed (e.g., the final comparison) use target_workspace_role.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/app/views/project/member.py`:
- Line 244: The code currently does new_role = int(request.data.get("role"))
which will raise ValueError/TypeError for non-numeric or missing values; wrap
this conversion in validation: check that request.data.get("role") is not None,
attempt int(...) inside a try/except catching ValueError and TypeError, and on
error return a proper 400 validation response (or raise
rest_framework.exceptions.ValidationError) with a clear message instead of
letting a 500 propagate; reference the new_role variable and the
request.data.get("role") conversion when applying the change.
---
Outside diff comments:
In `@apps/api/plane/app/views/project/member.py`:
- Around line 210-213: The code currently reads the target member's workspace
role into workspace_role and uses it for requester authorization checks; change
this to read both roles: fetch target_workspace_role =
WorkspaceMember.objects.get(workspace__slug=slug, member=project_member.member,
is_active=True).role and fetch requester_workspace_role =
WorkspaceMember.objects.get(workspace__slug=slug, member=request.user,
is_active=True).role (or the equivalent requesting user variable), then set
is_requester_workspace_admin = requester_workspace_role == ROLE.ADMIN.value and
use is_requester_workspace_admin in the bypass checks (replace occurrences that
used is_workspace_admin), and where the target's role is needed (e.g., the final
comparison) use target_workspace_role.
🪄 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: e0c2cca7-37ff-4ebc-8d5d-38fa4cf2cbd2
📒 Files selected for processing (1)
apps/api/plane/app/views/project/member.py
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
|
|
||
| new_role = int(request.data.get("role")) |
There was a problem hiding this comment.
Unhandled ValueError on non-numeric role input
If request.data.get("role") contains a non-integer value (e.g., "abc" or null), int() raises ValueError, resulting in a 500 error instead of a proper 400 validation response.
🛡️ Proposed fix
+ try:
+ new_role = int(request.data.get("role"))
+ except (ValueError, TypeError):
+ return Response(
+ {"error": "Invalid role value"},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
- new_role = int(request.data.get("role"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| new_role = int(request.data.get("role")) | |
| try: | |
| new_role = int(request.data.get("role")) | |
| except (ValueError, TypeError): | |
| return Response( | |
| {"error": "Invalid role value"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/app/views/project/member.py` at line 244, The code currently
does new_role = int(request.data.get("role")) which will raise
ValueError/TypeError for non-numeric or missing values; wrap this conversion in
validation: check that request.data.get("role") is not None, attempt int(...)
inside a try/except catching ValueError and TypeError, and on error return a
proper 400 validation response (or raise
rest_framework.exceptions.ValidationError) with a clear message instead of
letting a 500 propagate; reference the new_role variable and the
request.data.get("role") conversion when applying the change.
Summary
ProjectMemberViewSet.partial_updateTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes