mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 10:08:51 +02:00
[INFRA-501] fix(security): stop the external-id dedup echo disclosing foreign asset ids
Review catch. GenericAssetEndpoint.post deduplicates on workspace + external_source + external_id with no project scoping, and answers a match with 409 carrying asset_id and asset_url. For an attachment, asset_url also embeds the owning project and issue ids. When the body omits project_id the create-path validation is skipped entirely, so this branch is the only gate. Knowing the asset UUID is the precondition for every asset-scoped attack on this surface, so the preceding commit closed the routes that consume a foreign id while leaving the path that hands it out -- in the same handler. Two of the reports this branch addresses name this echo as their id-recovery step. Answer 404 when the matched asset's project is not accessible, and keep the 409 echo for a match the caller can reach. 404 rather than 403 deliberately: a 403 would still confirm that some asset holds this external id pair in this workspace, turning the pair into an existence oracle. The 403 used elsewhere in this branch is fine on routes where the caller already named an asset id; here they named only an external id, so a match is new information. The cost is that a caller who guesses a pair held by a project they cannot see cannot create their own asset under it -- the right trade, since real integrations mint ids per source and run as a member of the target project. Contract tests: disclosure with project_id supplied, disclosure with project_id omitted, and two controls proving dedup still echoes for a project member and for a workspace-level asset whose project_id is NULL. Verified fail-before against the previous commit -- the negative case returned 409 with the foreign asset id and its project id in asset_url. Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -587,6 +587,28 @@ class GenericAssetEndpoint(BaseAPIView):
|
||||
).first()
|
||||
|
||||
if existing_asset:
|
||||
# The dedup lookup is scoped to the workspace only -- and when the
|
||||
# body omits project_id the validation above is skipped entirely --
|
||||
# so the match may belong to a project the caller cannot see.
|
||||
# Echoing it would hand over that asset's id, and asset_url
|
||||
# additionally embeds the owning project and issue ids. Knowing the
|
||||
# asset UUID is the precondition for every asset-scoped attack on
|
||||
# this surface, so this branch must not supply it.
|
||||
#
|
||||
# 404 rather than 403 on purpose: a 403 would still confirm that
|
||||
# some asset carries this external id pair in this workspace, which
|
||||
# turns the pair into an existence oracle. The elsewhere-consistent
|
||||
# 403 is fine on routes where the caller already named the asset id;
|
||||
# here they named only an external id, so a match is new
|
||||
# information. The cost is that a caller who guesses a pair held by
|
||||
# a project they cannot see cannot create their own asset under it,
|
||||
# which is the right trade -- real integrations mint ids per source
|
||||
# and run as a member of the target project.
|
||||
if not existing_asset.is_project_accessible_to(request.user):
|
||||
return Response(
|
||||
{"error": "Asset not found.", "status": False},
|
||||
status=status.HTTP_404_NOT_FOUND,
|
||||
)
|
||||
return Response(
|
||||
{
|
||||
"message": "Asset with same external id and source already exists",
|
||||
|
||||
@@ -295,3 +295,94 @@ class TestGenericAssetPostProjectScope:
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
assert FileAsset.objects.count() == before + 1
|
||||
|
||||
|
||||
@pytest.mark.contract
|
||||
class TestGenericAssetExternalIdDedupDisclosure:
|
||||
"""The 409 dedup echo must not hand out a foreign project's asset identifiers.
|
||||
|
||||
The dedup lookup matches on workspace + external_source + external_id, with no
|
||||
project scoping, and the 409 body carries ``asset_id`` and ``asset_url``. That
|
||||
url embeds the owning project and issue ids for an attachment. Since knowing
|
||||
the asset UUID is the precondition for every asset-scoped attack on this
|
||||
surface, echoing a match the caller cannot access supplies exactly what the
|
||||
other guards in this module exist to make useless.
|
||||
"""
|
||||
|
||||
EXTERNAL = {"external_id": "EXT-1", "external_source": "jira"}
|
||||
|
||||
def _payload(self, project_id=None):
|
||||
payload = {"name": "dedup.png", "type": "image/png", "size": 16, **self.EXTERNAL}
|
||||
if project_id is not None:
|
||||
payload["project_id"] = str(project_id)
|
||||
return payload
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_dedup_does_not_disclose_foreign_asset_identifiers(
|
||||
self, api_key_client, workspace, foreign_asset, joined_project
|
||||
):
|
||||
"""404, not 403: a 403 would still confirm the external id pair is taken."""
|
||||
foreign_asset.external_id = self.EXTERNAL["external_id"]
|
||||
foreign_asset.external_source = self.EXTERNAL["external_source"]
|
||||
foreign_asset.save(update_fields=["external_id", "external_source"])
|
||||
|
||||
response = api_key_client.post(
|
||||
asset_list_url(workspace.slug), self._payload(joined_project.id), format="json"
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
body = str(getattr(response, "data", ""))
|
||||
assert str(foreign_asset.id) not in body, "the foreign asset id was disclosed"
|
||||
assert str(foreign_asset.project_id) not in body, "the foreign project id was disclosed"
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_dedup_does_not_disclose_when_project_id_is_omitted(
|
||||
self, api_key_client, workspace, foreign_asset
|
||||
):
|
||||
"""Omitting project_id skips the create-path validation, so this branch is the only gate."""
|
||||
foreign_asset.external_id = self.EXTERNAL["external_id"]
|
||||
foreign_asset.external_source = self.EXTERNAL["external_source"]
|
||||
foreign_asset.save(update_fields=["external_id", "external_source"])
|
||||
|
||||
response = api_key_client.post(asset_list_url(workspace.slug), self._payload(), format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_404_NOT_FOUND, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
assert str(foreign_asset.id) not in str(getattr(response, "data", ""))
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_dedup_still_echoes_an_accessible_asset(
|
||||
self, api_key_client, workspace, joined_asset, joined_project
|
||||
):
|
||||
"""Dedup must keep working for a caller who is a member of the match's project."""
|
||||
joined_asset.external_id = self.EXTERNAL["external_id"]
|
||||
joined_asset.external_source = self.EXTERNAL["external_source"]
|
||||
joined_asset.save(update_fields=["external_id", "external_source"])
|
||||
|
||||
response = api_key_client.post(
|
||||
asset_list_url(workspace.slug), self._payload(joined_project.id), format="json"
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_409_CONFLICT, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
assert response.data["asset_id"] == str(joined_asset.id)
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_dedup_still_echoes_a_workspace_level_asset(
|
||||
self, api_key_client, workspace, workspace_level_asset
|
||||
):
|
||||
"""project_id is NULL, so there is no project dimension to gate on."""
|
||||
workspace_level_asset.external_id = self.EXTERNAL["external_id"]
|
||||
workspace_level_asset.external_source = self.EXTERNAL["external_source"]
|
||||
workspace_level_asset.save(update_fields=["external_id", "external_source"])
|
||||
|
||||
response = api_key_client.post(asset_list_url(workspace.slug), self._payload(), format="json")
|
||||
|
||||
assert response.status_code == status.HTTP_409_CONFLICT, (
|
||||
f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
|
||||
)
|
||||
assert response.data["asset_id"] == str(workspace_level_asset.id)
|
||||
|
||||
Reference in New Issue
Block a user