mirror of
https://github.com/makeplane/plane.git
synced 2026-09-01 19:48:42 +02:00
[WEB-8095] fix: scope page-version reads to the URL project (GHSA-g49r/ghcr) (#9380)
* [WEB-8095] fix: scope page-version reads to the URL project (GHSA-g49r/ghcr) ProjectPagePermission verified the caller was a member of the URL project_id but then resolved the page by workspace + page_id only, and PageVersionEndpoint filtered versions the same way. A member of one project could read the page versions of a public page belonging to a different project in the same workspace via that project's URL (GHSA-g49r-p85q-qq2w / GHSA-ghcr-frqr-6pqr). - Scope the page lookup in ProjectPagePermission to projects__id via the ProjectPage M2M (both app/ and utils/ copies); deny when the page does not belong to the URL project. - Scope PageVersionEndpoint list/detail querysets to page__projects__id=project_id (defense in depth); distinct() on the list guards against active + soft-deleted ProjectPage duplicates. - Add contract regression tests (fail-before verified). Co-authored-by: Plane AI <noreply@plane.so> * [WEB-8095] fix: require active ProjectPage link when scoping pages to a project Address CodeRabbit + Copilot review on #9380: projects__id=project_id matched even soft-deleted ProjectPage links, so a page removed from the project (link revoked) would still pass, and the version detail get() could raise MultipleObjectsReturned on active + soft-deleted rows. Put both conditions on the same project_pages relation in one filter so they match a single ProjectPage row that is active: project_pages__project_id=project_id + project_pages__deleted_at__isnull =True. The partial-unique constraint (project, page WHERE deleted_at IS NULL) then guarantees at most one row, so get() stays unambiguous and the list needs no distinct(). Add a revoked-link regression test. Co-authored-by: Plane AI <noreply@plane.so> * [WEB-8095] fix: distinct() on page-version detail lookup as a MultipleObjectsReturned guard Address CodeRabbit review on #9380. The active-link filter already keeps the page__project_pages join to a single row via the partial-unique constraint, but add distinct() to the detail get() as defense in depth so the join can never surface MultipleObjectsReturned (a 500) even if that invariant were ever violated. Co-authored-by: Plane AI <noreply@plane.so> --------- Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -39,7 +39,20 @@ class ProjectPagePermission(BasePermission):
|
||||
return False
|
||||
|
||||
if page_id:
|
||||
page = Page.objects.get(id=page_id, workspace__slug=slug)
|
||||
# Scope the page to the project in the URL. Resolving the page by
|
||||
# workspace + page_id alone allowed a member of one project to read
|
||||
# pages belonging to another project in the same workspace
|
||||
# (GHSA-g49r / GHSA-ghcr). Require an *active* ProjectPage link (both
|
||||
# conditions on the same relation so they match one row) so a page
|
||||
# removed from the project (soft-deleted link) is also denied.
|
||||
page = Page.objects.filter(
|
||||
id=page_id,
|
||||
workspace__slug=slug,
|
||||
project_pages__project_id=project_id,
|
||||
project_pages__deleted_at__isnull=True,
|
||||
).first()
|
||||
if page is None:
|
||||
return False
|
||||
|
||||
# Allow access if the user is the owner of the page
|
||||
if page.owned_by_id == user_id:
|
||||
|
||||
@@ -19,13 +19,35 @@ class PageVersionEndpoint(BaseAPIView):
|
||||
def get(self, request, slug, project_id, page_id, pk=None):
|
||||
# Check if pk is provided
|
||||
if pk:
|
||||
# Return a single page version
|
||||
page_version = PageVersion.objects.get(workspace__slug=slug, page_id=page_id, pk=pk)
|
||||
# Return a single page version. Scope to an *active* ProjectPage link
|
||||
# for the URL project so a page belonging to (or removed from)
|
||||
# another project cannot be read via this endpoint (GHSA-g49r /
|
||||
# GHSA-ghcr). The active-link partial-unique constraint keeps the
|
||||
# join to a single row; distinct() is a defensive guard so the
|
||||
# page__project_pages join can never make get() raise
|
||||
# MultipleObjectsReturned (a 500).
|
||||
page_version = (
|
||||
PageVersion.objects.filter(
|
||||
workspace__slug=slug,
|
||||
page__project_pages__project_id=project_id,
|
||||
page__project_pages__deleted_at__isnull=True,
|
||||
page_id=page_id,
|
||||
pk=pk,
|
||||
)
|
||||
.distinct()
|
||||
.get()
|
||||
)
|
||||
# Serialize the page version
|
||||
serializer = PageVersionDetailSerializer(page_version)
|
||||
return Response(serializer.data, status=status.HTTP_200_OK)
|
||||
# Return all page versions
|
||||
page_versions = PageVersion.objects.filter(workspace__slug=slug, page_id=page_id)
|
||||
# Return all page versions scoped to an active ProjectPage link for the
|
||||
# URL project (defense in depth).
|
||||
page_versions = PageVersion.objects.filter(
|
||||
workspace__slug=slug,
|
||||
page__project_pages__project_id=project_id,
|
||||
page__project_pages__deleted_at__isnull=True,
|
||||
page_id=page_id,
|
||||
)
|
||||
# Serialize the page versions
|
||||
serializer = PageVersionSerializer(page_versions, many=True)
|
||||
return Response(serializer.data, status=status.HTTP_200_OK)
|
||||
|
||||
@@ -0,0 +1,149 @@
|
||||
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
||||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# See the LICENSE file for details.
|
||||
|
||||
"""
|
||||
Regression tests for GHSA-g49r-p85q-qq2w / GHSA-ghcr-frqr-6pqr.
|
||||
|
||||
ProjectPagePermission verified that the caller was a member of the URL
|
||||
project_id, but PageVersionEndpoint resolved the page (and its versions) by
|
||||
workspace + page_id only. A member of one project could therefore read the
|
||||
page versions of a public page belonging to a *different* project in the same
|
||||
workspace via that project's URL.
|
||||
"""
|
||||
|
||||
import uuid
|
||||
|
||||
import pytest
|
||||
from django.utils import timezone
|
||||
from rest_framework import status
|
||||
|
||||
from plane.db.models import (
|
||||
Page,
|
||||
PageVersion,
|
||||
Project,
|
||||
ProjectMember,
|
||||
ProjectPage,
|
||||
User,
|
||||
)
|
||||
|
||||
|
||||
def _page_versions_url(slug, project_id, page_id, pk=None):
|
||||
base = f"/api/workspaces/{slug}/projects/{project_id}/pages/{page_id}/versions/"
|
||||
return f"{base}{pk}/" if pk else base
|
||||
|
||||
|
||||
def _make_project(workspace, identifier):
|
||||
return Project.objects.create(
|
||||
name=f"Project {identifier}",
|
||||
identifier=identifier,
|
||||
workspace=workspace,
|
||||
)
|
||||
|
||||
|
||||
def _make_page(workspace, project, owner, access=Page.PUBLIC_ACCESS):
|
||||
page = Page.objects.create(
|
||||
workspace=workspace,
|
||||
owned_by=owner,
|
||||
access=access,
|
||||
name="Secret page",
|
||||
)
|
||||
ProjectPage.objects.create(workspace=workspace, project=project, page=page)
|
||||
return page
|
||||
|
||||
|
||||
def _make_version(workspace, page, owner):
|
||||
return PageVersion.objects.create(
|
||||
workspace=workspace,
|
||||
page=page,
|
||||
owned_by=owner,
|
||||
description_html="<p>secret</p>",
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.contract
|
||||
class TestPageVersionProjectScope:
|
||||
"""The attacker (create_user / session_client) is an active member of
|
||||
project_a only. Victim owns a public page in project_b."""
|
||||
|
||||
def _setup(self, workspace, attacker):
|
||||
victim = User.objects.create(email="victim@plane.so", username=f"victim_{uuid.uuid4().hex[:8]}")
|
||||
|
||||
project_a = _make_project(workspace, "PRJA")
|
||||
project_b = _make_project(workspace, "PRJB")
|
||||
|
||||
# Attacker is an active member of project A only.
|
||||
ProjectMember.objects.create(workspace=workspace, project=project_a, member=attacker, role=20)
|
||||
|
||||
# Public page + version living in project B (attacker is NOT a member).
|
||||
page_b = _make_page(workspace, project_b, victim)
|
||||
version_b = _make_version(workspace, page_b, victim)
|
||||
|
||||
return victim, project_a, project_b, page_b, version_b
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_cross_project_version_list_denied(self, session_client, workspace, create_user):
|
||||
"""Listing another project's page versions via a project the attacker
|
||||
belongs to must be denied (was a 200 leak)."""
|
||||
_, project_a, _, page_b, _ = self._setup(workspace, create_user)
|
||||
|
||||
response = session_client.get(_page_versions_url(workspace.slug, project_a.id, page_b.id))
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_cross_project_version_detail_denied(self, session_client, workspace, create_user):
|
||||
"""Reading a single cross-project page version must be denied."""
|
||||
_, project_a, _, page_b, version_b = self._setup(workspace, create_user)
|
||||
|
||||
response = session_client.get(
|
||||
_page_versions_url(workspace.slug, project_a.id, page_b.id, pk=version_b.id)
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_same_project_public_page_versions_allowed(self, session_client, workspace, create_user):
|
||||
"""A public page that genuinely belongs to the attacker's project is
|
||||
still readable, and its versions are returned."""
|
||||
victim, project_a, _, _, _ = self._setup(workspace, create_user)
|
||||
|
||||
# Public page owned by the victim but linked to project A (attacker is a
|
||||
# member of A). Exercises the public-page access branch (not owner).
|
||||
page_a = _make_page(workspace, project_a, victim)
|
||||
version_a = _make_version(workspace, page_a, victim)
|
||||
|
||||
response = session_client.get(_page_versions_url(workspace.slug, project_a.id, page_a.id))
|
||||
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
returned_ids = {str(item["id"]) for item in response.json()}
|
||||
assert str(version_a.id) in returned_ids
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_revoked_project_link_denied(self, session_client, workspace, create_user):
|
||||
"""A page whose ProjectPage link to the attacker's project was
|
||||
soft-deleted (page removed from the project) must be denied, even
|
||||
though the attacker is a member of that project."""
|
||||
victim, project_a, _, _, _ = self._setup(workspace, create_user)
|
||||
|
||||
page = Page.objects.create(
|
||||
workspace=workspace, owned_by=victim, access=Page.PUBLIC_ACCESS, name="Removed page"
|
||||
)
|
||||
# Link exists but is soft-deleted → the page no longer belongs to A.
|
||||
ProjectPage.objects.create(
|
||||
workspace=workspace, project=project_a, page=page, deleted_at=timezone.now()
|
||||
)
|
||||
_make_version(workspace, page, victim)
|
||||
|
||||
response = session_client.get(_page_versions_url(workspace.slug, project_a.id, page.id))
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_cross_project_version_list_not_a_member_anywhere(self, session_client, workspace, create_user):
|
||||
"""Sanity: a project the attacker is not a member of is denied outright."""
|
||||
_, _, project_b, page_b, _ = self._setup(workspace, create_user)
|
||||
|
||||
response = session_client.get(_page_versions_url(workspace.slug, project_b.id, page_b.id))
|
||||
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN
|
||||
@@ -39,7 +39,20 @@ class ProjectPagePermission(BasePermission):
|
||||
return False
|
||||
|
||||
if page_id:
|
||||
page = Page.objects.get(id=page_id, workspace__slug=slug)
|
||||
# Scope the page to the project in the URL. Resolving the page by
|
||||
# workspace + page_id alone allowed a member of one project to read
|
||||
# pages belonging to another project in the same workspace
|
||||
# (GHSA-g49r / GHSA-ghcr). Require an *active* ProjectPage link (both
|
||||
# conditions on the same relation so they match one row) so a page
|
||||
# removed from the project (soft-deleted link) is also denied.
|
||||
page = Page.objects.filter(
|
||||
id=page_id,
|
||||
workspace__slug=slug,
|
||||
project_pages__project_id=project_id,
|
||||
project_pages__deleted_at__isnull=True,
|
||||
).first()
|
||||
if page is None:
|
||||
return False
|
||||
|
||||
# Allow access if the user is the owner of the page
|
||||
if page.owned_by_id == user_id:
|
||||
|
||||
Reference in New Issue
Block a user