From e364e1692a9c8e94f1586f842e4fcd61cea9195f Mon Sep 17 00:00:00 2001 From: Sriram Veeraghanta Date: Thu, 12 Nov 2026 01:04:38 +0530 Subject: [PATCH] fix(security): block restricted guests from assets of issues hidden from them A guest of a project with guest_view_all_features off may only see issues they created, and the issue sub-resource endpoints enforce that. The asset routes that serve a file by id checked project membership only, so such a guest could still download attachments, description images and comment images of other issues through the download, asset, duplicate and check routes. - FileAsset.is_project_accessible_to now applies the restricted-guest rule to assets bound to an issue or to a comment on one. It already backs the workspace download, workspace asset, duplicate, check, legacy file and external API generic asset routes. - ProjectAssetEndpoint.get and ProjectAssetDownloadEndpoint.get now call it. - Contract tests cover the blocked routes, the guest's own issues, members and unrestricted guests. --- apps/api/plane/app/views/asset/v2.py | 13 ++ apps/api/plane/db/models/asset.py | 44 +++++-- .../test_guest_asset_download_scope_app.py | 123 ++++++++++++++++++ 3 files changed, 169 insertions(+), 11 deletions(-) create mode 100644 apps/api/plane/tests/contract/app/test_guest_asset_download_scope_app.py diff --git a/apps/api/plane/app/views/asset/v2.py b/apps/api/plane/app/views/asset/v2.py index a8db045876bd..8115e7c5b466 100644 --- a/apps/api/plane/app/views/asset/v2.py +++ b/apps/api/plane/app/views/asset/v2.py @@ -713,6 +713,12 @@ def delete(self, request, slug, project_id, pk): def get(self, request, slug, project_id, pk): # get the asset id asset = FileAsset.objects.get(workspace__slug=slug, project_id=project_id, pk=pk) + # A restricted guest only reaches the assets of issues they created. + if not asset.is_project_accessible_to(request.user): + return Response( + {"error": "You don't have access to this asset."}, + status=status.HTTP_403_FORBIDDEN, + ) # Check if the asset is uploaded if not asset.is_uploaded: @@ -994,6 +1000,13 @@ def get(self, request, slug, project_id, asset_id): status=status.HTTP_404_NOT_FOUND, ) + # A restricted guest only reaches the assets of issues they created. + if not asset.is_project_accessible_to(request.user): + return Response( + {"error": "You don't have access to this asset."}, + status=status.HTTP_403_FORBIDDEN, + ) + storage = S3Storage(request=request) signed_url = storage.generate_presigned_url( object_name=asset.asset.name, diff --git a/apps/api/plane/db/models/asset.py b/apps/api/plane/db/models/asset.py index ed3459d9205e..634cf81b9ad8 100644 --- a/apps/api/plane/db/models/asset.py +++ b/apps/api/plane/db/models/asset.py @@ -14,7 +14,7 @@ from plane.utils.path_validator import sanitize_filename from .base import BaseModel -from .project import ProjectMember +from .project import ROLE, ProjectMember def get_upload_path(instance, filename): @@ -104,11 +104,16 @@ def asset_url(self): return None def is_project_accessible_to(self, user): - """Return whether ``user`` is an active member of this asset's project. + """Return whether ``user`` may read this asset within its project. - Covers the project dimension only; callers still check workspace access - (the endpoint's permission class or ``allow_permission(..., level="WORKSPACE")``). - Lives on the model so the app and external-API asset views share one rule. + The user must be an active member of the asset's project. A restricted guest + (a GUEST of a project with ``guest_view_all_features`` off) also only reaches + the assets of issues they created, together with those issues' comments: the + rule ``issue_hidden_from_guest`` applies to the issue sub-resource endpoints. + + Callers still check workspace access (the endpoint's permission class or + ``allow_permission(..., level="WORKSPACE")``). Lives on the model so the app and + external-API asset views share one rule. Assets with no project (workspace logos, user avatars and covers) are workspace-level, so this returns True for them. @@ -116,9 +121,26 @@ def is_project_accessible_to(self, user): if self.project_id is None: return True # Scope the membership to the asset's workspace as well as its project. - return ProjectMember.objects.filter( - member=user, - workspace_id=self.workspace_id, - project_id=self.project_id, - is_active=True, - ).exists() + membership = ( + ProjectMember.objects.filter( + member=user, + workspace_id=self.workspace_id, + project_id=self.project_id, + is_active=True, + ) + .values("role", "project__guest_view_all_features") + .first() + ) + if membership is None: + return False + if membership["role"] != ROLE.GUEST.value or membership["project__guest_view_all_features"]: + return True + + issue_id = self.issue_id or (self.comment.issue_id if self.comment_id else None) + if issue_id is None: + return True + + from .issue import Issue + + # Issue.objects, as issue detail uses, so the guest's own archived and draft issues stay visible. + return Issue.objects.filter(pk=issue_id, project_id=self.project_id, created_by=user).exists() diff --git a/apps/api/plane/tests/contract/app/test_guest_asset_download_scope_app.py b/apps/api/plane/tests/contract/app/test_guest_asset_download_scope_app.py new file mode 100644 index 000000000000..2580399f404c --- /dev/null +++ b/apps/api/plane/tests/contract/app/test_guest_asset_download_scope_app.py @@ -0,0 +1,123 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Imported pytest fixtures are requested by parameter name, which F811 misreads as redefinition. +# ruff: noqa: F811 + +"""A restricted guest (``guest_view_all_features=False``) must not read, by asset id, +the files of issues they did not create: attachments, description images and +comment images. ``FileAsset.is_project_accessible_to`` applies the rule for every +asset read route. Own issues, members and unrestricted guests are unaffected. + +Reuses the fixtures of the sibling sub-resource test module. +""" + +from unittest import mock +from uuid import uuid4 + +import pytest +from rest_framework import status + +from plane.db.models import FileAsset +from plane.tests.contract.app.test_guest_issue_subresource_scope_app import ( # noqa: F401 + _make_comment, + foreign_issue, + guest, + guest_client, + own_issue, + project, +) + +S3_STORAGE_PATH = "plane.app.views.asset.v2.S3Storage" + +READ_URLS = [ + "/api/assets/v2/workspaces/{slug}/download/{asset_id}/", + "/api/assets/v2/workspaces/{slug}/projects/{project_id}/download/{asset_id}/", + "/api/assets/v2/workspaces/{slug}/{asset_id}/", + "/api/assets/v2/workspaces/{slug}/projects/{project_id}/{asset_id}/", +] +CHECK_URL = "/api/assets/v2/workspaces/{slug}/check/{asset_id}/" +DUPLICATE_URL = "/api/assets/v2/workspaces/{slug}/duplicate-assets/{asset_id}/" + + +def _url(tpl, workspace, project, asset): + return tpl.format(slug=workspace.slug, project_id=project.id, asset_id=asset.id) + + +def _attachment(issue): + return FileAsset.objects.get(issue=issue, entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT) + + +def _get_all(client, workspace, project, asset): + """Status codes of every read route for ``asset``; storage is mocked.""" + with mock.patch(S3_STORAGE_PATH) as storage: + storage.return_value.generate_presigned_url.return_value = "https://example.com/s" + return [client.get(_url(tpl, workspace, project, asset)).status_code for tpl in READ_URLS] + + +@pytest.fixture +def foreign_description_asset(workspace, project, foreign_issue): + return FileAsset.objects.create( + attributes={"name": "diagram.png", "type": "image/png", "size": 100}, + asset=f"{workspace.id}/{uuid4().hex}-diagram.png", + workspace=workspace, + project=project, + issue_id=foreign_issue.id, + entity_type=FileAsset.EntityTypeContext.ISSUE_DESCRIPTION, + is_uploaded=True, + ) + + +@pytest.fixture +def foreign_comment_asset(workspace, project, foreign_issue, create_user): + # Comment images carry the comment, not the issue (ProjectBulkAssetEndpoint). + comment = _make_comment(foreign_issue, project, workspace, create_user) + return FileAsset.objects.create( + attributes={"name": "screenshot.png", "type": "image/png", "size": 100}, + asset=f"{workspace.id}/{uuid4().hex}-screenshot.png", + workspace=workspace, + project=project, + comment_id=comment.id, + entity_type=FileAsset.EntityTypeContext.COMMENT_DESCRIPTION, + is_uploaded=True, + ) + + +@pytest.mark.contract +@pytest.mark.django_db +class TestGuestHiddenIssueAssets: + def test_guest_blocked_from_foreign_issue_assets( + self, guest_client, workspace, project, foreign_issue, foreign_description_asset, foreign_comment_asset + ): + for asset in (_attachment(foreign_issue), foreign_description_asset, foreign_comment_asset): + assert _get_all(guest_client, workspace, project, asset) == [status.HTTP_403_FORBIDDEN] * 4, ( + asset.entity_type + ) + + def test_guest_check_and_duplicate_blocked(self, guest_client, workspace, project, foreign_issue): + asset = _attachment(foreign_issue) + assert guest_client.get(_url(CHECK_URL, workspace, project, asset)).data == {"exists": False} + + before = FileAsset.objects.count() + with mock.patch(S3_STORAGE_PATH) as storage: + response = guest_client.post( + _url(DUPLICATE_URL, workspace, project, asset), + {"entity_type": FileAsset.EntityTypeContext.ISSUE_ATTACHMENT, "project_id": str(project.id)}, + format="json", + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + storage.return_value.copy_object.assert_not_called() + assert FileAsset.objects.count() == before + + def test_guest_allowed_own_issue_assets(self, guest_client, workspace, project, own_issue): + assert _get_all(guest_client, workspace, project, _attachment(own_issue)) == [status.HTTP_302_FOUND] * 4 + + def test_member_unaffected(self, session_client, workspace, project, foreign_issue, foreign_comment_asset): + for asset in (_attachment(foreign_issue), foreign_comment_asset): + assert _get_all(session_client, workspace, project, asset) == [status.HTTP_302_FOUND] * 4 + + def test_unrestricted_guest_unaffected(self, guest_client, workspace, project, foreign_issue): + project.guest_view_all_features = True + project.save(update_fields=["guest_view_all_features"]) + assert _get_all(guest_client, workspace, project, _attachment(foreign_issue)) == [status.HTTP_302_FOUND] * 4