mirror of
https://github.com/makeplane/plane.git
synced 2026-06-13 19:19:54 +00:00
fix(api): enforce workspace membership on GenericAssetEndpoint (#9212)
The public REST API GenericAssetEndpoint (/api/v1/workspaces/<slug>/assets/) declared no permission class, inheriting only IsAuthenticated. Since APIKeyAuthentication does not bind a token to a workspace and the workspace is read straight from the URL slug, any valid Personal Access Token could read (GET), create (POST), and modify (PATCH) assets in a workspace the caller is not a member of — a cross-workspace IDOR, the public-API sibling of the CVE-2026-46558 dashboard asset fix. Add permission_classes = [WorkspaceUserPermission] so every method requires active workspace membership, matching the dashboard fix semantics. Also add contract regression tests covering cross-workspace GET/POST/PATCH (now 403) and a positive control confirming members retain access. Also ignore the local /security/ advisory notes folder.
This commit is contained in:
committed by
GitHub
parent
b6e47ccdae
commit
9a30a07cf5
+2
-2
@@ -115,5 +115,5 @@ scripts/
|
||||
# i18n auto-generated types (regenerated on every build)
|
||||
packages/i18n/src/types/keys.generated.ts
|
||||
|
||||
# Local security advisory notes (not for version control)
|
||||
/advisories.md
|
||||
# Local security notes (not for version control)
|
||||
/security/
|
||||
|
||||
@@ -19,6 +19,7 @@ from plane.bgtasks.storage_metadata_task import get_asset_object_metadata
|
||||
from plane.settings.storage import S3Storage
|
||||
from plane.utils.path_validator import sanitize_filename
|
||||
from plane.db.models import FileAsset, User, Workspace
|
||||
from plane.app.permissions import WorkspaceUserPermission
|
||||
from plane.api.views.base import BaseAPIView
|
||||
from plane.api.serializers import (
|
||||
UserAssetUploadSerializer,
|
||||
@@ -404,6 +405,12 @@ class UserServerAssetEndpoint(BaseAPIView):
|
||||
class GenericAssetEndpoint(BaseAPIView):
|
||||
"""This endpoint is used to upload generic assets that can be later bound to entities."""
|
||||
|
||||
# The workspace is taken straight from the URL slug, so every method must
|
||||
# verify the caller is an active member of that workspace. Without this the
|
||||
# endpoint is a cross-workspace IDOR (the public-API sibling of the
|
||||
# CVE-2026-46558 dashboard fix).
|
||||
permission_classes = [WorkspaceUserPermission]
|
||||
|
||||
use_read_replica = True
|
||||
|
||||
@asset_docs(
|
||||
|
||||
@@ -0,0 +1,143 @@
|
||||
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
||||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# See the LICENSE file for details.
|
||||
|
||||
"""Contract tests for the public REST API ``GenericAssetEndpoint``.
|
||||
|
||||
Regression coverage for the cross-workspace asset IDOR (the unfixed
|
||||
external-API sibling of CVE-2026-46558 / GHSA-qw87-v5w3-6vxx). The endpoint
|
||||
must reject any caller that is not an active member of the workspace named in
|
||||
the URL slug, regardless of the workspace their Personal Access Token came
|
||||
from.
|
||||
"""
|
||||
|
||||
from unittest import mock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from rest_framework import status
|
||||
|
||||
from plane.db.models import FileAsset, User, Workspace, WorkspaceMember
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def victim_user(db):
|
||||
"""A user that owns a separate workspace the attacker is not part of."""
|
||||
unique_id = uuid4().hex[:8]
|
||||
user = User.objects.create(
|
||||
email=f"victim-{unique_id}@plane.so",
|
||||
username=f"victim_{unique_id}",
|
||||
first_name="Victim",
|
||||
last_name="User",
|
||||
)
|
||||
user.set_password("test-password")
|
||||
user.save()
|
||||
return user
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def victim_workspace(db, victim_user):
|
||||
"""A workspace whose only active member is ``victim_user``.
|
||||
|
||||
The attacker (``create_user``, who authenticates ``api_key_client``) is
|
||||
deliberately NOT a member here.
|
||||
"""
|
||||
workspace = Workspace.objects.create(
|
||||
name="Victim Workspace",
|
||||
owner=victim_user,
|
||||
slug="victim-workspace",
|
||||
)
|
||||
WorkspaceMember.objects.create(workspace=workspace, member=victim_user, role=20)
|
||||
return workspace
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def victim_asset(db, victim_workspace, victim_user):
|
||||
"""An uploaded attachment that lives inside the victim workspace.
|
||||
|
||||
``storage_metadata`` is pre-populated so the PATCH handler does not enqueue
|
||||
the metadata Celery task during the test.
|
||||
"""
|
||||
return FileAsset.objects.create(
|
||||
attributes={"name": "secret.pdf", "type": "application/pdf", "size": 1024},
|
||||
asset=f"{victim_workspace.id}/secret.pdf",
|
||||
size=1024,
|
||||
workspace=victim_workspace,
|
||||
created_by=victim_user,
|
||||
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
|
||||
is_uploaded=True,
|
||||
storage_metadata={"size": 1024},
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.contract
|
||||
class TestGenericAssetCrossWorkspaceIDOR:
|
||||
"""A PAT holder must not reach assets in a workspace they don't belong to."""
|
||||
|
||||
def detail_url(self, slug, asset_id):
|
||||
return f"/api/v1/workspaces/{slug}/assets/{asset_id}/"
|
||||
|
||||
def list_url(self, slug):
|
||||
return f"/api/v1/workspaces/{slug}/assets/"
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_get_cross_workspace_asset_returns_403(self, api_key_client, victim_workspace, victim_asset):
|
||||
"""GET on another workspace's asset must be forbidden, not return a
|
||||
presigned download URL."""
|
||||
url = self.detail_url(victim_workspace.slug, victim_asset.id)
|
||||
|
||||
with mock.patch("plane.api.views.asset.S3Storage") as mock_storage:
|
||||
mock_storage.return_value.generate_presigned_url.return_value = "https://signed.example/download"
|
||||
response = api_key_client.get(url)
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, f"Got {response.status_code}: {response.data!r}"
|
||||
# The S3 download URL must never be minted for a non-member.
|
||||
mock_storage.return_value.generate_presigned_url.assert_not_called()
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_post_cross_workspace_asset_returns_403(self, api_key_client, victim_workspace):
|
||||
"""POST (upload) into another workspace must be forbidden and must not
|
||||
plant an asset row in the victim workspace."""
|
||||
url = self.list_url(victim_workspace.slug)
|
||||
payload = {"name": "evil.pdf", "type": "application/pdf", "size": 1024}
|
||||
|
||||
with mock.patch("plane.api.views.asset.S3Storage") as mock_storage:
|
||||
mock_storage.return_value.generate_presigned_post.return_value = {"url": "x", "fields": {}}
|
||||
response = api_key_client.post(url, payload, format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, f"Got {response.status_code}: {response.data!r}"
|
||||
assert FileAsset.objects.filter(workspace=victim_workspace).count() == 0
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_patch_cross_workspace_asset_returns_403(self, api_key_client, victim_workspace, victim_asset):
|
||||
"""PATCH on another workspace's asset must be forbidden and must leave
|
||||
the asset untouched."""
|
||||
url = self.detail_url(victim_workspace.slug, victim_asset.id)
|
||||
|
||||
response = api_key_client.patch(url, {"is_uploaded": False}, format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN, f"Got {response.status_code}: {response.data!r}"
|
||||
victim_asset.refresh_from_db()
|
||||
assert victim_asset.is_uploaded is True
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_member_can_patch_own_workspace_asset(self, api_key_client, workspace, create_user):
|
||||
"""Positive control: an active member of the workspace can still update
|
||||
their own asset, so the fix does not over-block legitimate callers."""
|
||||
asset = FileAsset.objects.create(
|
||||
attributes={"name": "mine.pdf", "type": "application/pdf", "size": 10},
|
||||
asset=f"{workspace.id}/mine.pdf",
|
||||
size=10,
|
||||
workspace=workspace,
|
||||
created_by=create_user,
|
||||
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
|
||||
is_uploaded=False,
|
||||
storage_metadata={"size": 10},
|
||||
)
|
||||
url = self.detail_url(workspace.slug, asset.id)
|
||||
|
||||
response = api_key_client.patch(url, {"is_uploaded": True}, format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_204_NO_CONTENT, f"Got {response.status_code}: {response.data!r}"
|
||||
asset.refresh_from_db()
|
||||
assert asset.is_uploaded is True
|
||||
Reference in New Issue
Block a user