From b6a6b577ee55e33bf602cac65ff425e06837ad99 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Thu, 27 Aug 2026 10:35:38 +0530 Subject: [PATCH] [INFRA-501] fix(security): require project scoping on asset create/duplicate Both new create-asset and duplicate-asset paths accepted a request that simply omitted project_id and silently stored project_id=None on the row. is_project_accessible_to() treats project_id=None as workspace-accessible-by-default (the correct behavior for genuinely workspace-level assets like logos), so an attacker could reuse that fallback to erase the project-membership check this PR just added, defeating its own goal for the exact entity types it targets. GenericAssetEndpoint.post always creates an ISSUE_ATTACHMENT, a project-scoped entity type, so project_id is now required before the row is created (400 if missing); the dedup-echo short-circuit above it is unaffected since it never creates a row. DuplicateAssetEndpoint.post now defaults project_id to the source asset's own project when the request omits it (or sends it empty/null) instead of defaulting to None -- the caller's access to the source was only ever established through that project, and a duplicate belongs to the same project as its source by construction rather than by client-supplied opinion. Explicitly naming a different destination project is unaffected and still goes through the existing membership check. Also aligned the destination-project error response shape in DuplicateAssetEndpoint with GenericAssetEndpoint's ({"error": ..., "status": False}), which had drifted between the two hand-duplicated checks. Co-authored-by: Plane AI --- apps/api/plane/api/views/asset.py | 13 +++++ apps/api/plane/app/views/asset/v2.py | 50 ++++++++++------- .../api/test_generic_asset_project_scope.py | 18 +++++++ ...orkspace_asset_routes_project_scope_app.py | 54 +++++++++++++++++++ 4 files changed, 117 insertions(+), 18 deletions(-) diff --git a/apps/api/plane/api/views/asset.py b/apps/api/plane/api/views/asset.py index 81451acd88..e5e8db019e 100644 --- a/apps/api/plane/api/views/asset.py +++ b/apps/api/plane/api/views/asset.py @@ -618,6 +618,19 @@ class GenericAssetEndpoint(BaseAPIView): status=status.HTTP_409_CONFLICT, ) + # This endpoint always creates an ISSUE_ATTACHMENT (below), a + # project-scoped entity type. The block above only validates project_id + # when one is supplied, so a caller who omits it entirely reaches this + # point unchecked -- creating the row here would leave project_id=None, + # and is_project_accessible_to() treats project_id=None as + # workspace-accessible-by-default, silently bypassing the membership + # check above for every caller who just leaves the field out. + if not project_id: + return Response( + {"error": "Project id is required.", "status": False}, + status=status.HTTP_400_BAD_REQUEST, + ) + # Create a File Asset asset = FileAsset.objects.create( attributes={"name": name, "type": type, "size": size_limit}, diff --git a/apps/api/plane/app/views/asset/v2.py b/apps/api/plane/app/views/asset/v2.py index 277d1d3bf0..786ef01752 100644 --- a/apps/api/plane/app/views/asset/v2.py +++ b/apps/api/plane/app/views/asset/v2.py @@ -813,24 +813,6 @@ class DuplicateAssetEndpoint(BaseAPIView): ) workspace = Workspace.objects.get(slug=slug) - if project_id: - # check if project exists in the workspace - if not Project.objects.filter(id=project_id, workspace=workspace).exists(): - return Response({"error": "Project not found"}, status=status.HTTP_404_NOT_FOUND) - # project_id is the *destination* and comes from the request body. - # Existence in the workspace is not authorization: require the caller - # to be an active member of the project the copy will land in, or a - # workspace member could deposit assets into any project. - if not ProjectMember.objects.filter( - member=request.user, - workspace=workspace, - project_id=project_id, - is_active=True, - ).exists(): - return Response( - {"error": "You don't have access to this project."}, - status=status.HTTP_403_FORBIDDEN, - ) storage = S3Storage(request=request) # Restrict the source asset to the same destination workspace to prevent cross-workspace asset copying @@ -852,6 +834,38 @@ class DuplicateAssetEndpoint(BaseAPIView): status=status.HTTP_403_FORBIDDEN, ) + # A caller may redirect the copy to a different project than the source + # (e.g. duplicating an attachment onto an issue that lives in another + # project) by naming project_id explicitly -- that's still validated + # below. But leaving it out (or sending it empty/null) must not be read + # as "make this workspace-level": the caller's access to the source + # only ever came through its project, and defaulting to None here + # would strip that scoping and expose the copy to the entire + # workspace. This isn't something the client should be able to unset + # at all -- default to the source's own project instead. + if project_id: + # check if project exists in the workspace + if not Project.objects.filter(id=project_id, workspace=workspace).exists(): + return Response( + {"error": "Project not found", "status": False}, status=status.HTTP_404_NOT_FOUND + ) + # project_id is the *destination* and comes from the request body. + # Existence in the workspace is not authorization: require the caller + # to be an active member of the project the copy will land in, or a + # workspace member could deposit assets into any project. + if not ProjectMember.objects.filter( + member=request.user, + workspace=workspace, + project_id=project_id, + is_active=True, + ).exists(): + return Response( + {"error": "You don't have access to this project.", "status": False}, + status=status.HTTP_403_FORBIDDEN, + ) + else: + project_id = original_asset.project_id + sanitized_name = sanitize_filename(original_asset.attributes.get("name")) or "unnamed" destination_key = f"{workspace.id}/{uuid.uuid4().hex}-{sanitized_name}" duplicated_asset = FileAsset.objects.create( diff --git a/apps/api/plane/tests/contract/api/test_generic_asset_project_scope.py b/apps/api/plane/tests/contract/api/test_generic_asset_project_scope.py index 54ce292859..2ca258e3cf 100644 --- a/apps/api/plane/tests/contract/api/test_generic_asset_project_scope.py +++ b/apps/api/plane/tests/contract/api/test_generic_asset_project_scope.py @@ -296,6 +296,24 @@ class TestGenericAssetPostProjectScope: ) assert FileAsset.objects.count() == before + 1 + @pytest.mark.django_db + def test_post_denied_when_project_id_omitted(self, api_key_client, workspace): + """The endpoint only ever creates ISSUE_ATTACHMENT rows, a project-scoped + entity type. Omitting project_id must not create a project_id=None row -- + is_project_accessible_to() treats that as workspace-accessible-by-default, + which would hand every workspace member access regardless of role. + """ + before = FileAsset.objects.count() + payload = {"name": "planted.png", "type": "image/png", "size": 16} + response = api_key_client.post(asset_list_url(workspace.slug), payload, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert FileAsset.objects.count() == before, ( + "an asset row was created with project_id=None despite the omitted field" + ) + @pytest.mark.contract class TestGenericAssetExternalIdDedupDisclosure: diff --git a/apps/api/plane/tests/contract/app/test_workspace_asset_routes_project_scope_app.py b/apps/api/plane/tests/contract/app/test_workspace_asset_routes_project_scope_app.py index 995d3ba917..ee972e095d 100644 --- a/apps/api/plane/tests/contract/app/test_workspace_asset_routes_project_scope_app.py +++ b/apps/api/plane/tests/contract/app/test_workspace_asset_routes_project_scope_app.py @@ -300,3 +300,57 @@ class TestDuplicateAssetProjectScope: f"Got {response.status_code}: {getattr(response, 'data', None)!r}" ) assert FileAsset.objects.count() == before + 1 + + @pytest.mark.django_db + def test_duplicate_omitted_project_id_inherits_source_project( + self, session_client, workspace, project, project_asset + ): + """Omitting project_id must not strip the copy's project scoping. + + The caller's access to ``project_asset`` is only established through + ``project`` membership; defaulting the copy to project_id=None would + make is_project_accessible_to() treat it as workspace-accessible by + default, exposing it to every workspace member regardless of role. + """ + before = FileAsset.objects.count() + with mock.patch(S3_STORAGE_PATH): + response = session_client.post( + duplicate_url(workspace.slug, project_asset.id), + {"entity_type": FileAsset.EntityTypeContext.ISSUE_ATTACHMENT}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert FileAsset.objects.count() == before + 1 + duplicated_asset = FileAsset.objects.get(id=response.data["asset_id"]) + assert duplicated_asset.project_id == project.id, ( + "the duplicate did not inherit the source asset's project and was " + f"created with project_id={duplicated_asset.project_id!r} instead" + ) + + @pytest.mark.django_db + def test_duplicate_null_project_id_still_inherits_source_project( + self, session_client, workspace, project, project_asset + ): + """An explicit ``project_id: null`` must not override inheritance either -- + the field isn't meant to be client-settable to "no project" at all when + the source asset has one.""" + before = FileAsset.objects.count() + with mock.patch(S3_STORAGE_PATH): + response = session_client.post( + duplicate_url(workspace.slug, project_asset.id), + { + "entity_type": FileAsset.EntityTypeContext.ISSUE_ATTACHMENT, + "project_id": None, + }, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert FileAsset.objects.count() == before + 1 + duplicated_asset = FileAsset.objects.get(id=response.data["asset_id"]) + assert duplicated_asset.project_id == project.id