fix: scope draft publish to the draft's owner - #9811
Conversation
Drafts are personal, and list, retrieve and partial_update all look them up with created_by=request.user. create_draft_to_issue looked up by id alone, so any workspace member with a draft's UUID could publish someone else's draft, which also deletes the original as part of the conversion. The decorator's creator check cannot cover this route because it reads kwargs["pk"] and this one receives draft_id, so scope the query instead, the way the neighbouring actions already do. A draft that does not resolve now returns 404 rather than raising AttributeError on None. Fixes makeplane#9363
◈ PR Lens
Architecture 1 component touched across 5 lanes. Inside the changed components — 1 viewComponent view — Draft issue conversion The workspace draft viewset enforces user isolation by scoping draft lookups to the creator before issue creation. Data flow
View
Tip The CLI's 🪧 More tips
Thanks for using PR Lens! It's built by Coldtea, free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe draft-to-issue endpoint now limits publication to drafts created by the requesting user. It returns 404 for unauthorized or missing drafts. New unit tests validate query scoping, serializer exclusion, and missing-draft handling. ChangesDraft publication security
Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to Draft publication now requires ownership, preventing other workspace members from publishing or deleting someone else’s draft. Unauthorized and missing drafts return 404, with regression coverage for these cases and no concrete merge-blocking risk remaining. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Description
Draft work items are personal.
list,retrieveandpartial_updateinWorkspaceDraftIssueViewSetall resolve them withcreated_by=request.user, butcreate_draft_to_issuelooked the draft up by id alone:get_queryset()only filters by workspace, so any workspace member holding a draft's UUID could publish another user's draft. Publishing also deletes the original, so the author loses the draft with nothing in the activity trail naming who did it.Worth noting why the decorator does not already cover this:
allow_permission(..., creator=True, model=...)checkskwargs["pk"], and this route receivesdraft_id. That is whydestroyis protected and this action is not, and why the fix belongs in the query rather than on the decorator.The unresolved case now returns 404 instead of raising
AttributeErroronNone.project_id, matching howretrievehandles a draft that is not there.Test
plane/tests/unit/views/test_workspace_draft.pyasserts the lookup carriescreated_by, that a draft the caller does not own never reachesIssueCreateSerializer, and that a missing draft returns 404. Reverting the fix fails all three, the last with theAttributeErrorthe guard removes.ruff checkandruff format --checkare clean on both files.Fixes #9363
Summary by CodeRabbit
Bug Fixes
Tests