[WEB-8372] fix(security): restrict StateViewSet.partial_update to project admins - #9473
[WEB-8372] fix(security): restrict StateViewSet.partial_update to project admins#9473mguptahub wants to merge 3 commits into
Conversation
…ject admins (GHSA-4jpp-964m-27cr) Editing a workflow state is project configuration; its sibling writes (create, destroy, mark_as_default) are all @allow_permission([ROLE.ADMIN]). partial_update was the outlier at [ADMIN, MEMBER, GUEST], so any project Guest could rewrite any state's name/color/group/description and set it as the project default — a functional bypass of the admin-only mark_as_default. Tighten partial_update to [ROLE.ADMIN]. Adds 3 contract tests (guest 403, member 403, admin 200); fail-before verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Linked to Plane Work Item(s) References This comment was auto-generated by Plane |
|
React Doctor found 4 issues in 4 files · 1 error & 3 warnings · score 82 / 100 (Needs work) · vs Errors
3 warnings
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWorkflow state partial updates now require ChangesWorkflow state update authorization
Estimated code review effort: 2 (Simple) | ~10 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes an authorization bypass in the API by restricting StateViewSet.partial_update so that only project admins can edit workflow states, aligning it with other state-mutation endpoints and preventing guests/members from changing project configuration.
Changes:
- Tighten
StateViewSet.partial_updatepermission to@allow_permission([ROLE.ADMIN]). - Add a regression-focused contract test verifying guest/member are forbidden (403) and admin is allowed (200).
- Add inline security context commentary referencing GHSA-4jpp-964m-27cr.
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/state/base.py | Restricts state PATCH authorization to admins for security hardening. |
| apps/api/plane/tests/contract/app/test_state_partial_update_admin_scope_app.py | Adds contract coverage to prevent reintroducing the permission regression. |
![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_state_partial_update_admin_scope_app.py`:
- Around line 75-85: Update test_member_cannot_patch_state to include
default=True alongside the name change in the rejected PATCH request, then
assert after state.refresh_from_db() that state.default remains False, matching
the guest regression coverage while preserving the existing forbidden-status and
name assertions.
🪄 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 Plus
Run ID: 1096bac5-fc28-44e8-b8a7-ae624cf80990
📒 Files selected for processing (2)
apps/api/plane/app/views/state/base.pyapps/api/plane/tests/contract/app/test_state_partial_update_admin_scope_app.py
…n member test (CodeRabbit/Copilot #9473) - Copilot: partial_update mutates state data but (unlike create/destroy/ mark_as_default) did not invalidate the workspaces/:slug/states/ cache, so clients could see stale state after a PATCH. Add the same @invalidate_cache decorator as the sibling writes. - CodeRabbit: member test now also sends default=True and asserts it stays False, matching the guest regression coverage. 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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/api/plane/app/views/state/base.py:66
- Decorator order means
invalidate_cache(...)runs before@allow_permission(...). Becauseinvalidate_cachedeletes the sharedworkspaces/:slug/states/key unconditionally (even when the permission wrapper returns 403), any unauthorized user can repeatedly evict this cache by spamming PATCH requests, increasing load and reducing cache effectiveness.
Swapping the decorator order ensures permission checks short-circuit before invalidation on 403 responses.
@invalidate_cache(path="workspaces/:slug/states/", url_params=True, user=False)
@allow_permission([ROLE.ADMIN])
Summary
StateViewSet.partial_update(apps/api/plane/app/views/state/base.py) was decorated@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]), while every sibling write on the viewset —create,destroy,mark_as_default— is@allow_permission([ROLE.ADMIN]).StateSerializerexposesname,color,group,default,description,sequence,orderas writable, so any project Guest could rewrite any workflow state and set it as the project default — a functional bypass of the admin-onlymark_as_default.Fixes WEB-8372 (CWE-862). Confirmed vulnerable against
origin/preview@a8e53b6ac7.Fix
Tighten
partial_updateto@allow_permission([ROLE.ADMIN]), matching the other state writes.Tests
test_state_partial_update_admin_scope_app.py— guest PATCH → 403 (state + default unchanged), member PATCH → 403, admin PATCH → 200. Fail-before verified (guest & member could edit without the fix); ruff clean.EE
plane-ee vendors its own copy; EE port to be assessed under Epic WEB-8293 (EE StateViewSet uses the
@canengine — likely already gated, will confirm separately).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests