mirror of
https://github.com/makeplane/plane.git
synced 2026-09-02 12:09:14 +02:00
[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 <noreply@plane.so>
This commit is contained in:
@@ -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},
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user