Skip to content

[INFRA-499] fix(security): require workspace membership to read a global view - #9653

Open
mguptahub wants to merge 2 commits into
infra-496/baseviewset-routed-verb-guardfrom
infra-499/undecorated-action-authz
Open

[INFRA-499] fix(security): require workspace membership to read a global view#9653
mguptahub wants to merge 2 commits into
infra-496/baseviewset-routed-verb-guardfrom
infra-499/undecorated-action-authz

Conversation

@mguptahub

@mguptahub mguptahub commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

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.retrieve

It was the only action on the class with no check at all, and get_queryset() supplied none either:

.filter(workspace__slug=self.kwargs.get("slug"))
.filter(project__isnull=True)
.filter(Q(owned_by=self.request.user) | Q(access=1))

That last clause reads as a visibility predicate and 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. 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 200 and the full view payload.

Also here — regression coverage for a guard that had none

IssueDetailIdentifierEndpoint was reported to me 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 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-N and read every work item's full detail including description_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_item fail with 200 and 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

  • 8 contract tests, each property with a positive control (own item still readable, full member unaffected, guest_view_all_features=True still permissive, workspace member and workspace guest both still read the view).
  • Fail-before verified per property, targeting each guard individually by line rather than by pattern.
  • Full suite green against a real database: 527 passed, contract + unit.
  • ruff check and ruff format clean on both changed files.

Note for reviewers

The role set on retrieve deliberately matches list() — 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/ and space/ 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.

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: da05a20d-b441-4ca1-95f6-e608a49c2ea7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mguptahub mguptahub changed the title fix(security): require workspace membership to read a global view [INFRA-499] fix(security): require workspace membership to read a global view Aug 20, 2026
@mguptahub
mguptahub marked this pull request as ready for review August 20, 2026 12:20
Copilot AI lite review requested due to automatic review settings August 20, 2026 12:20
@makeplane

makeplane Bot commented Aug 20, 2026

Copy link
Copy Markdown

Linked to Plane Work Item(s)

This comment was auto-generated by Plane

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.retrieve by adding an @allow_permission(..., level="WORKSPACE") guard.
  • Add contract tests covering (1) the workspace-view retrieve membership requirement and (2) regression protection for IssueDetailIdentifierEndpoint guest 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_view is 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.

Comment thread apps/api/plane/app/views/view/base.py
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>
@mguptahub
mguptahub force-pushed the infra-499/undecorated-action-authz branch from 037254f to 3cec6c8 Compare August 20, 2026 12:31
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants