[WEB-8095] fix: scope page-version reads to the URL project (GHSA-g49r/ghcr)#9380
Open
mguptahub wants to merge 3 commits into
Open
[WEB-8095] fix: scope page-version reads to the URL project (GHSA-g49r/ghcr)#9380mguptahub wants to merge 3 commits into
mguptahub wants to merge 3 commits into
Conversation
…r/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>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPage permission and page-version lookups now require an active link to the requested ChangesProject-scoped page version access control
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Linked to Plane Work Item(s) References This comment was auto-generated by Plane |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a cross-project information disclosure in the Pages API by ensuring page and page-version reads are scoped to the project_id from the URL, preventing a user who is a member of Project A from reading versions of a (public) page that belongs to Project B in the same workspace.
Changes:
- Scope
ProjectPagePermissionpage resolution to the URL project (instead of workspace + page id only). - Scope
PageVersionEndpointlist/detail queries toproject_idfor defense in depth. - Add contract regression tests covering cross-project deny + same-project allow behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/api/plane/utils/permissions/page.py | Scopes permission-time page lookup to the URL project to prevent cross-project access. |
| apps/api/plane/app/permissions/page.py | Same permission scoping change in the app permissions module. |
| apps/api/plane/app/views/page/version.py | Scopes page-version list/detail queries to the URL project to close the disclosure path. |
| apps/api/plane/tests/contract/app/test_page_version_project_scope_app.py | Adds regression contract tests for cross-project page-version access. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/app/permissions/page.py`:
- Around line 46-50: The Page.objects.filter lookup in the permission check is
still matching soft-deleted project-page links, which can keep access open after
revocation. Update the query in the page permission logic to also require the
related ProjectPage link to be active by adding a deleted_at is null condition
alongside projects__id=project_id, and keep the change within the
permission-check path that returns False when no page is found.
In `@apps/api/plane/app/views/page/version.py`:
- Around line 25-27: The PageVersion detail lookup in PageVersionView/page
version retrieval can be duplicated by the page__projects join, causing .get()
to raise MultipleObjectsReturned. Update the queryset used in the page version
lookup to add .distinct() before .get(), keeping the existing filters on
workspace__slug, page__projects__id, page_id, and pk so the lookup remains
unique even when ProjectPage has both active and soft-deleted rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21872407-7ad5-4d04-bef7-46dcd5609447
📒 Files selected for processing (4)
apps/api/plane/app/permissions/page.pyapps/api/plane/app/views/page/version.pyapps/api/plane/tests/contract/app/test_page_version_project_scope_app.pyapps/api/plane/utils/permissions/page.py
… 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>
…eObjectsReturned 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the cross-project page-version disclosure described in GHSA-g49r-p85q-qq2w (dup GHSA-ghcr-frqr-6pqr).
Problem
ProjectPagePermission.has_permissionverified the caller was an active member of the URLproject_id, but then resolved the page withPage.objects.get(id=page_id, workspace__slug=slug)— never confirming the page belongs to that project.PageVersionEndpoint.getfiltered versions the same way. A member of Project A could callGET /workspaces/<slug>/projects/<A>/pages/<pageB>/versions/, wherepageBis a public page in Project B (attacker not a member of B), pass the membership check on A, and read Bs page versions. Leak is limited to non-private pages (private pages are owner-gated on CE).Fix
Page-to-Project is an M2M through
ProjectPage.app/permissions/page.py+utils/permissions/page.py: scope the page lookup toprojects__id=project_id; deny (403) when the page does not belong to the URL project.PageVersionEndpoint.get: scope list/detail querysets topage__projects__id=project_id(defense in depth).distinct()on the list guards against duplicate rows when a page has both an active and a soft-deletedProjectPagelink to the same project.Sibling page endpoints (
PageViewSet,PagesDescriptionViewSet,PageDuplicateEndpoint) already scope their querysets byprojects__id, so the permission gap was latent for them.Tests
tests/contract/app/test_page_version_project_scope_app.py— 4 contract tests (cross-project list/detail denied, same-project public allowed, non-member denied). Fail-before verified (the two denied tests return 200 before the fix).ruff+manage.py checkgreen.EE counterpart: WEB-8096 (makeplane/plane-ee).
Summary by CodeRabbit