[INFRA-499] fix(security): require workspace membership to read a global view - #9653
[INFRA-499] fix(security): require workspace membership to read a global view#9653mguptahub wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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.
Pull request overview
This PR closes an authorization gap in the app API by ensuring that reading a global workspace view (WorkspaceViewViewSet.retrieve) requires workspace membership, aligning the access boundary with the existing list() behavior. It also adds contract/regression coverage for authorization logic that is implemented via inline checks rather than @allow_permission decorators.
Changes:
- Require workspace membership on
WorkspaceViewViewSet.retrieveby adding an@allow_permission(..., level="WORKSPACE")guard. - Add contract tests covering (1) the workspace-view retrieve membership requirement and (2) regression protection for
IssueDetailIdentifierEndpointguest scoping.
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/view/base.py | Adds workspace-membership authorization to WorkspaceViewViewSet.retrieve to prevent cross-workspace reads of global views. |
| apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py | Introduces contract tests for the fixed endpoint and regression tests for the identifier-based issue detail endpoint. |
Suppressed comments (1)
apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py:264
- The file ends mid-test:
test_workspace_guest_can_read_a_global_viewis missing its final assertions/closing lines, leaving a dangling assert block and causing a syntax error.
assert response.status_code == status.HTTP_200_OK, (
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
WorkspaceViewViewSet.retrieve was the only action on the class with no authorization check, and its queryset supplied none either. It filters on workspace__slug and then `Q(owned_by=request.user) | Q(access=1)`. That second clause reads as a visibility predicate but is vacuous: `access` sits in IssueViewSerializer.read_only_fields so the API never sets it, and the model defaults it to 1 (Public), so every row matches. Any authenticated account holding a view id could therefore read any global view in any workspace, including one it had no membership in. Requires workspace membership, matching the role set on list(). Also adds regression coverage for IssueDetailIdentifierEndpoint. That endpoint was reported as missing the guest restriction; it is not — the membership check at the top of get() is followed, after the issue is fetched, by an explicit role-5 / guest_view_all_features / created_by check. It had no test, so a guard preventing a guest from walking PROJ-1..PROJ-N and reading every work item's description_html was one refactor from being lost silently. Verified non-vacuous: neutering that check makes the test fail with 200 and the foreign work item's full payload. Co-authored-by: Plane AI <noreply@plane.so>
037254f to
3cec6c8
Compare
Review catch, verified before fixing. WorkspaceViewViewSet.retrieve resolves the
view with .first() and serialized the result unconditionally, so a member asking
for an id that does not exist got 200 with every field null or empty
(`{"name": "", "description": "", "filters": null, ...}`) and a recent-visit
enqueued for a nonexistent entity. Because get_queryset() is scoped to the URL
workspace, the same happened for a real view id belonging to a different
workspace.
Returns 404, matching the other retrieve endpoints.
Noted while confirming this, not fixed here: the project-level sibling
IssueViewViewSet.retrieve has the same .first() pattern and then dereferences
`issue_view.owned_by`, which raises AttributeError on None rather than answering
404 — a 500 instead of a hollow 200. Different method, so it gets its own ticket
rather than widening this one.
Co-authored-by: Plane AI <noreply@plane.so>
Second layer of the stack, on top of #9652. Same bug family, one layer over: #9652 closed routed verbs that fall through to a DRF generic mixin. This closes an action that is defined on the viewset but carries no authorization check — which no fall-through scan can see, because a method we wrote with no check on it looks identical to a correctly guarded one.
The fix —
WorkspaceViewViewSet.retrieveIt was the only action on the class with no check at all, and
get_queryset()supplied none either:That last clause reads as a visibility predicate and is vacuous.
accesssits inIssueViewSerializer.read_only_fields, so the API never sets it, and the model defaults it to1(Public) — so every row matches. A filter that matches everything looks like scoping and provides none.Result: any authenticated account holding a view id could read any global view in any workspace, including one it had no membership in. Now requires workspace membership, matching the role set on
list().Verified by reverting the fix: the test fails with
200and the full view payload.Also here — regression coverage for a guard that had none
IssueDetailIdentifierEndpointwas reported to me as missing the guest restriction. It is not. The membership check at the top ofget()is followed, after the issue is fetched, by an explicit role-5 /guest_view_all_features/created_bycheck returning 403. I wrote a fix for it, discovered during fail-before verification that the endpoint was already guarded, and reverted the change — this PR contains no code change for it.What it did have was no test. That endpoint resolves work items by human-readable sequence number, so it's an enumeration surface: losing the guard lets a guest walk
PROJ-1..PROJ-Nand read every work item's full detail includingdescription_html, plus the project and issue UUIDs every other endpoint keys on. Those five tests pin it.They are verified non-vacuous: neutering the existing check makes
test_guest_cannot_read_foreign_work_itemfail with200and the foreign work item's full payload. Worth stating explicitly, because my first attempt at that verification patched a different guard with the same shape earlier in the same file and the tests stayed green — a vacuous pass I'd otherwise have shipped.Verification
guest_view_all_features=Truestill permissive, workspace member and workspace guest both still read the view).ruff checkandruff formatclean on both changed files.Note for reviewers
The role set on
retrievedeliberately matcheslist()— admin, member, guest. The defect is that non-members could read at all, not which member roles may. Tightening beyond that is a product decision, not a security one, so it is out of scope here.The broader audit this came from is still open: enumerating every action across
app/,api/andspace/that has neither a decorator nor an inline check. #9652's count of 27 is a floor, not a ceiling, precisely because of the class this PR is in.Refs INFRA-499.