feat(api): expose project pages in the public REST API#9020
Open
Ojajajajaja wants to merge 2 commits intomakeplane:previewfrom
Open
feat(api): expose project pages in the public REST API#9020Ojajajajaja wants to merge 2 commits intomakeplane:previewfrom
Ojajajajaja wants to merge 2 commits intomakeplane:previewfrom
Conversation
Adds Pages endpoints to the public API namespace (`/api/v1/`) so external clients (integrations, MCP servers, automation) can manage pages without the session-only app API. Routes (project-scoped, X-API-Key auth): - GET /workspaces/<slug>/projects/<id>/pages/ list - POST /workspaces/<slug>/projects/<id>/pages/ create - GET /workspaces/<slug>/projects/<id>/pages/<page_id>/ retrieve - PATCH /workspaces/<slug>/projects/<id>/pages/<page_id>/ update - DELETE /workspaces/<slug>/projects/<id>/pages/<page_id>/ delete - GET /workspaces/<slug>/projects/<id>/pages-summary/ stats - GET /workspaces/<slug>/projects/<id>/archived-pages/ list archived - POST /workspaces/<slug>/projects/<id>/pages/<page_id>/archive/ archive - DELETE /workspaces/<slug>/projects/<id>/pages/<page_id>/archive/ unarchive - POST /workspaces/<slug>/projects/<id>/pages/<page_id>/lock/ lock - DELETE /workspaces/<slug>/projects/<id>/pages/<page_id>/lock/ unlock - POST /workspaces/<slug>/projects/<id>/pages/<page_id>/access/ toggle public/private - POST /workspaces/<slug>/projects/<id>/pages/<page_id>/duplicate/ duplicate Reuses the existing Page / ProjectPage models and ProjectEntityPermission. Mirrors conventions from cycle / state public endpoints (BaseAPIView, APIKeyAuthentication, pagination, external_id idempotency). Refs makeplane#7319 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
|
Contributor
📝 WalkthroughWalkthroughAdds a full project-scoped Page API: serializer with parent validation and atomic create (Page + ProjectPage), URL routes, and multiple API endpoints for list/create, detail/update/delete, archive/unarchive (recursive), lock/unlock, access toggle, duplicate, and per-project page summary. ChangesProject Page API
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Page API
participant DB as Database
Client->>API: POST /workspaces/:w/projects/:p/pages (create)
API->>DB: SELECT Project (by project_id from context)
DB-->>API: Project row (workspace_id)
API->>DB: INSERT INTO pages (owned_by, workspace_id, ...) RETURNING id
DB-->>API: new Page id
API->>DB: INSERT INTO project_pages (workspace_id, project_id, page_id, created_by, updated_by)
DB-->>API: project_page row
API-->>Client: 201 Created (page payload)
alt Archive request
Client->>API: POST /.../pages/:id/archive
API->>DB: WITH RECURSIVE ... UPDATE pages SET archived_at = now() WHERE id IN (cte)
DB-->>API: rows updated
API-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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: 10
🤖 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/api/serializers/page.py`:
- Around line 53-73: Wrap the two writes in the serializer's create method
inside a database transaction to ensure atomicity: use transaction.atomic()
around Project.objects.get(...), Page.objects.create(...), and
ProjectPage.objects.create(...) so a failure creating the ProjectPage rolls back
the Page; also handle Project.DoesNotExist from Project.objects.get(...) and
raise a serializers.ValidationError (or appropriate API error) instead of
letting it bubble as a 500; finally verify and, if necessary, populate or assert
page.created_by_id / page.updated_by_id before inserting into ProjectPage to
avoid NULL audit fields.
- Around line 19-51: The Meta currently exposes parent as writable without
scoping; add a validate_parent(self, value) method on the Page serializer (the
serializer class that contains Meta) that checks value is either None or a Page
whose project_id matches the project_id provided in serializer context (e.g.,
self.context.get("project_id")) and raise serializers.ValidationError if it
doesn't match (also guard against setting parent to self by comparing
instance.pk when updating). Ensure this validator is invoked on PATCH/PUT by
keeping parent in fields and let DRF call validate_parent during standard
validation; include clear error messages for cross-project and self-parent
cases.
In `@apps/api/plane/api/urls/page.py`:
- Around line 19-28: Two URL patterns register the same name "project-pages",
causing ambiguous reverse/{% url %} lookups; update the name on the detail route
registered with ProjectPageDetailAPIEndpoint.as_view(...) to a distinct
identifier (for example "project-pages-detail") so the list/create route
(ProjectPageListCreateAPIEndpoint.as_view(...)) keeps "project-pages" and the
detail route uses the new unique name.
In `@apps/api/plane/api/views/page.py`:
- Around line 131-197: The handlers call Page.objects.get(...) and
self.get_queryset().get(...) without handling Page.DoesNotExist, causing 500s;
change these to use Django's get_object_or_404 or add try/except
Page.DoesNotExist to return a 404. Create/replace a helper (e.g.,
get_page_or_404) that accepts slug, project_id, page_id and uses
get_object_or_404(Page, ...) or catches Page.DoesNotExist and raises Http404,
then call that helper from ProjectPageDetailAPIEndpoint.get/patch/delete and
likewise update ProjectPageArchiveAPIEndpoint.post/delete,
ProjectPageLockAPIEndpoint.post/delete, ProjectPageAccessAPIEndpoint.post, and
ProjectPageDuplicateAPIEndpoint.post to use the helper instead of direct
Page.objects.get/self.get_queryset().get.
- Around line 379-409: The aggregates are over the joined queryset and will
over-count because JOINs multiply rows; replace the Count(Case(...)) pattern
with distinct-counts on the Page primary key and use the filter= parameter to
scope each metric. Specifically, in the stats aggregation (the variable stats
computed from queryset), change public_pages/private_pages/archived_pages to use
Count("id", distinct=True, filter=Q(...)) with the corresponding filters (e.g.,
Q(access=Page.PUBLIC_ACCESS, archived_at__isnull=True),
Q(access=Page.PRIVATE_ACCESS, archived_at__isnull=True), and
Q(archived_at__isnull=False)) so counts are computed distinct per Page rather
than over the joined rows.
- Line 54: The current .order_by(self.kwargs.get("order_by", "-created_at"))
reads from URL kwargs (dead) and is insecure; change it to read order_by from
the request query params (e.g. request.query_params.get("order_by")) and
validate it against a small allow-list (e.g.
{"created_at","-created_at","name","-name","sort_order","-sort_order"}) before
passing to queryset.order_by(); implement a small helper (e.g.
validate_order_by(value) or get_valid_order_by(request, default="-created_at"))
and use it in the affected view methods (the method(s) that call .order_by
currently—e.g. ProjectPageListAPIEndpoint.get_queryset—and also update
ProjectPageDetailAPIEndpoint.get_queryset and
ProjectPageArchiveAPIEndpoint.get_queryset for consistent behavior).
- Around line 21-32: The recursive CTE in _archive_page_and_descendants walks
parent_id across the entire pages table and can touch pages outside the intended
project/workspace and soft-deleted rows; change the CTE to constrain recursion
to the seed page's project/workspace and to only non-deleted pages by joining to
the project_pages (or filtering by project_id/workspace_id from the seed row)
and adding "deleted_at IS NULL" to the recursive WHERE clauses; ensure the
UPDATE limits to ids returned from that scoped CTE so only pages belonging to
the caller's project/workspace (and not soft-deleted) are (un)archived.
- Around line 332-369: The duplication flow in
ProjectPageDuplicateAPIEndpoint.post must be made atomic and must only link the
new page to projects the requesting user legitimately belongs to: wrap the Page
cloning and the subsequent ProjectPage.objects.create loop in a
transaction.atomic() block; change how project_ids is collected to filter
ProjectPage.objects.filter(page_id=page.id, deleted_at__isnull=True) and further
restrict to projects where request.user has an active membership (e.g., query
Project or ProjectMembership to only include projects the user belongs to);
finally, use the filtered project_ids when creating ProjectPage records instead
of blindly iterating over all linked projects, leaving Page.save() and
ProjectPage.objects.create inside the same transaction to ensure rollback on any
failure.
- Around line 264-289: The ProjectPageLockAPIEndpoint methods (post and delete)
are missing the ownership check so any project member can lock/unlock another
user's page; update the Page lookup in both ProjectPageLockAPIEndpoint.post and
.delete to include the ownership filter (owned_by_id=request.user.id) or
otherwise verify page.owned_by_id == request.user.id and return a
403/PermissionDenied when it doesn't match, mirroring the behavior in
ProjectPageAccessAPIEndpoint.post to ensure only the page owner can change lock
state.
- Around line 177-197: The delete handler currently uses Page.objects.get(...)
and then unconditionally calls page.delete(), which hard-deletes the Page across
all projects; instead, use self.get_queryset().get(...) to locate the page
(matching GET/PATCH behavior), soft-delete the ProjectPage row for the given
project (set deleted_at or call its soft-delete method) rather than .delete(),
then check ProjectPage.objects.filter(page_id=page.id, project_id=project_id,
workspace__slug=slug, project_pages__deleted_at__isnull=True).exists() (or
similar) and only call page.delete() when that query returns false (no remaining
active project links); keep the ownership check (page.owned_by_id) as-is and
preserve permission enforcement via ProjectEntityPermission.has_permission().
🪄 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: f878391d-623c-4d95-9a32-94f6b7a67c0f
📒 Files selected for processing (6)
apps/api/plane/api/serializers/__init__.pyapps/api/plane/api/serializers/page.pyapps/api/plane/api/urls/__init__.pyapps/api/plane/api/urls/page.pyapps/api/plane/api/views/__init__.pyapps/api/plane/api/views/page.py
| ) | ||
| .select_related("workspace") | ||
| .select_related("owned_by") | ||
| .order_by(self.kwargs.get("order_by", "-created_at")) |
Contributor
There was a problem hiding this comment.
order_by is read from URL kwargs, so it's effectively dead code.
self.kwargs only contains URL-captured params (slug, project_id). There is no order_by URL kwarg, so this always falls back to "-created_at". If you want to honour a client-supplied order, read from request.query_params and validate it against an allow-list (passing arbitrary user input straight into order_by opens up information-disclosure via ordering on unintended columns).
- .order_by(self.kwargs.get("order_by", "-created_at"))
+ .order_by(self._safe_order_by())with a small helper that whitelists e.g. {"created_at", "-created_at", "name", "-name", "sort_order", "-sort_order"}. The same applies to ProjectPageDetailAPIEndpoint.get_queryset and ProjectPageArchiveAPIEndpoint.get_queryset if you want consistent behaviour.
🤖 Prompt for 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.
In `@apps/api/plane/api/views/page.py` at line 54, The current
.order_by(self.kwargs.get("order_by", "-created_at")) reads from URL kwargs
(dead) and is insecure; change it to read order_by from the request query params
(e.g. request.query_params.get("order_by")) and validate it against a small
allow-list (e.g.
{"created_at","-created_at","name","-name","sort_order","-sort_order"}) before
passing to queryset.order_by(); implement a small helper (e.g.
validate_order_by(value) or get_valid_order_by(request, default="-created_at"))
and use it in the affected view methods (the method(s) that call .order_by
currently—e.g. ProjectPageListAPIEndpoint.get_queryset—and also update
ProjectPageDetailAPIEndpoint.get_queryset and
ProjectPageArchiveAPIEndpoint.get_queryset for consistent behavior).
Comment on lines
+177
to
+197
| def delete(self, request, slug, project_id, page_id): | ||
| page = Page.objects.get( | ||
| pk=page_id, | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| project_pages__deleted_at__isnull=True, | ||
| ) | ||
|
|
||
| if page.owned_by_id != request.user.id: | ||
| return Response( | ||
| {"error": "Only the page owner can delete the page"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
|
|
||
| ProjectPage.objects.filter( | ||
| page_id=page.id, | ||
| project_id=project_id, | ||
| workspace__slug=slug, | ||
| ).delete() | ||
| page.delete() | ||
| return Response(status=status.HTTP_204_NO_CONTENT) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm pages are routinely linked to multiple projects in the codebase.
rg -nP --type=py -C3 'ProjectPage\.objects\.create|projects\.add\('
# Inspect ProjectEntityPermission to confirm membership semantics on object-level.
fd -t f 'permissions' apps/api/plane/app | head
rg -nP --type=py -C3 'class\s+ProjectEntityPermission'Repository: makeplane/plane
Length of output: 3025
🏁 Script executed:
#!/bin/bash
# 1. Find and inspect the Page model to verify cascade behavior
fd -t f 'models.py' apps/api/plane --type f | xargs rg -l 'class Page'
# 2. Get the Page model definition and its relationships
rg -n 'class Page' -A 50 apps/api/plane/db/models.py | head -100Repository: makeplane/plane
Length of output: 560
🏁 Script executed:
#!/bin/bash
# 3. Check which ProjectEntityPermission is used in the API
rg -n 'from.*ProjectEntityPermission' apps/api/plane/api/views/page.py
# 4. Inspect both ProjectEntityPermission implementations
rg -n 'class ProjectEntityPermission' -A 30 apps/api/plane/utils/permissions/project.pyRepository: makeplane/plane
Length of output: 1531
🏁 Script executed:
#!/bin/bash
# 5. Check the full delete view method and its parent class
rg -n 'def delete' -B 5 -A 25 apps/api/plane/api/views/page.py | grep -A 25 'def delete.*slug.*project_id.*page_id'Repository: makeplane/plane
Length of output: 3074
🏁 Script executed:
#!/bin/bash
# 6. Verify if get_queryset is defined in this view and what filters it applies
rg -n 'get_queryset|class.*PageViewSet\|class.*PageView' apps/api/plane/api/views/page.py | head -20Repository: makeplane/plane
Length of output: 462
🏁 Script executed:
#!/bin/bash
# Get the Page model definition and FK relationships
cat -n apps/api/plane/db/models/page.py | head -150Repository: makeplane/plane
Length of output: 6983
🏁 Script executed:
#!/bin/bash
# Get the complete ProjectEntityPermission class with has_object_permission
rg -n 'class ProjectEntityPermission' -A 80 apps/api/plane/app/permissions/project.pyRepository: makeplane/plane
Length of output: 2377
🏁 Script executed:
#!/bin/bash
# Check the class definition for the delete endpoint around line 177
rg -n 'class.*Page.*APIEndpoint|permission_classes' apps/api/plane/api/views/page.py | grep -B 5 -A 2 '177'Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Find the view class around line 177
rg -n 'class.*APIEndpoint.*:' apps/api/plane/api/views/page.py | head -20Repository: makeplane/plane
Length of output: 436
🏁 Script executed:
#!/bin/bash
# Get the full context around line 177 with class definition and permission_classes
sed -n '140,180p' apps/api/plane/api/views/page.pyRepository: makeplane/plane
Length of output: 1739
🏁 Script executed:
#!/bin/bash
# Check if there are other places where pages are deleted to understand expected behavior
rg -n 'page\.delete\(\)|ProjectPage.*delete\(\)' apps/api/plane/api/views/page.py -B 3 -A 1Repository: makeplane/plane
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Get the complete ProjectPageDetailAPIEndpoint class with permission_classes
sed -n '109,200p' apps/api/plane/api/views/page.py | head -80Repository: makeplane/plane
Length of output: 3220
🏁 Script executed:
#!/bin/bash
# Also check if ProjectPage has any deleted_at logic
rg -n 'deleted_at' apps/api/plane/db/models/page.pyRepository: makeplane/plane
Length of output: 270
DELETE wipes the page from every linked project, not just this one.
Pages are M2M with projects through ProjectPage — and the codebase explicitly supports creating single Pages linked to multiple projects (confirmed by multiple ProjectPage.objects.create() calls in loops across different project IDs). This delete handler removes the ProjectPage row for the current project and then calls page.delete() unconditionally, which hard-deletes the entire Page entity and cascades to all other ProjectPage rows. The ProjectPage model uses soft deletes (deleted_at field), but Page does not—when page.delete() executes, it completely removes the page from every project it was linked to.
Fix: Only call page.delete() when no remaining ProjectPage rows exist:
Suggested fix
ProjectPage.objects.filter(
page_id=page.id,
project_id=project_id,
workspace__slug=slug,
).delete()
- page.delete()
+ if not ProjectPage.objects.filter(
+ page_id=page.id, deleted_at__isnull=True
+ ).exists():
+ page.delete()
return Response(status=status.HTTP_204_NO_CONTENT)Also note: the delete() method bypasses the queryset membership filter by using Page.objects.get() directly instead of self.get_queryset().get(). While ProjectEntityPermission.has_permission() still enforces that the user is a project member, and an additional ownership check restricts deletion to the page owner, this inconsistency with the GET/PATCH pattern (which use self.get_queryset()) should be addressed for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def delete(self, request, slug, project_id, page_id): | |
| page = Page.objects.get( | |
| pk=page_id, | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| project_pages__deleted_at__isnull=True, | |
| ) | |
| if page.owned_by_id != request.user.id: | |
| return Response( | |
| {"error": "Only the page owner can delete the page"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| ProjectPage.objects.filter( | |
| page_id=page.id, | |
| project_id=project_id, | |
| workspace__slug=slug, | |
| ).delete() | |
| page.delete() | |
| return Response(status=status.HTTP_204_NO_CONTENT) | |
| def delete(self, request, slug, project_id, page_id): | |
| page = Page.objects.get( | |
| pk=page_id, | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| project_pages__deleted_at__isnull=True, | |
| ) | |
| if page.owned_by_id != request.user.id: | |
| return Response( | |
| {"error": "Only the page owner can delete the page"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| ProjectPage.objects.filter( | |
| page_id=page.id, | |
| project_id=project_id, | |
| workspace__slug=slug, | |
| ).delete() | |
| if not ProjectPage.objects.filter( | |
| page_id=page.id, deleted_at__isnull=True | |
| ).exists(): | |
| page.delete() | |
| return Response(status=status.HTTP_204_NO_CONTENT) |
🤖 Prompt for 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.
In `@apps/api/plane/api/views/page.py` around lines 177 - 197, The delete handler
currently uses Page.objects.get(...) and then unconditionally calls
page.delete(), which hard-deletes the Page across all projects; instead, use
self.get_queryset().get(...) to locate the page (matching GET/PATCH behavior),
soft-delete the ProjectPage row for the given project (set deleted_at or call
its soft-delete method) rather than .delete(), then check
ProjectPage.objects.filter(page_id=page.id, project_id=project_id,
workspace__slug=slug, project_pages__deleted_at__isnull=True).exists() (or
similar) and only call page.delete() when that query returns false (no remaining
active project links); keep the ownership check (page.owned_by_id) as-is and
preserve permission enforcement via ProjectEntityPermission.has_permission().
Comment on lines
+264
to
+289
| class ProjectPageLockAPIEndpoint(BaseAPIView): | ||
| """Lock and unlock a project page via the public API.""" | ||
|
|
||
| permission_classes = [ProjectEntityPermission] | ||
|
|
||
| def post(self, request, slug, project_id, page_id): | ||
| page = Page.objects.get( | ||
| pk=page_id, | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| project_pages__deleted_at__isnull=True, | ||
| ) | ||
| page.is_locked = True | ||
| page.save(update_fields=["is_locked"]) | ||
| return Response(status=status.HTTP_204_NO_CONTENT) | ||
|
|
||
| def delete(self, request, slug, project_id, page_id): | ||
| page = Page.objects.get( | ||
| pk=page_id, | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| project_pages__deleted_at__isnull=True, | ||
| ) | ||
| page.is_locked = False | ||
| page.save(update_fields=["is_locked"]) | ||
| return Response(status=status.HTTP_204_NO_CONTENT) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the app-side lock implementation to compare ownership semantics.
rg -nP --type=py -C5 'is_locked\s*=\s*True' apps/api/plane/app
rg -nP --type=py -C5 '"lock"|/lock/' apps/api/plane/app/urls 2>/dev/nullRepository: makeplane/plane
Length of output: 1767
🏁 Script executed:
# Check the full PageViewSet lock/unlock implementation
head -300 apps/api/plane/app/views/page/base.py | tail -150Repository: makeplane/plane
Length of output: 5861
🏁 Script executed:
# Find the ProjectPageAccessAPIEndpoint to see the ownership check at line 322
rg -nP --type=py -B10 -A10 'ProjectPageAccessAPIEndpoint' apps/api/plane/api/views/page.py | head -100Repository: makeplane/plane
Length of output: 856
🏁 Script executed:
# Check if PageViewSet has permission_classes defined
rg -nP --type=py -B5 'class PageViewSet' apps/api/plane/app/views/page/base.pyRepository: makeplane/plane
Length of output: 231
🏁 Script executed:
# Read the ProjectPageAccessAPIEndpoint.post method in full
sed -n '292,330p' apps/api/plane/api/views/page.pyRepository: makeplane/plane
Length of output: 1590
🏁 Script executed:
# Check PageViewSet for permission_classes or decorators
rg -nP --type=py -A30 'class PageViewSet' apps/api/plane/app/views/page/base.py | head -50Repository: makeplane/plane
Length of output: 1395
Lock/unlock has no owner check; other ownership-sensitive operations do.
Both the app-side PageViewSet.lock/unlock and this API endpoint allow any project member to lock another user's page, blocking edits. However, ProjectPageAccessAPIEndpoint.post (line 325) restricts access changes to page.owned_by_id == request.user.id. Since lock gates PATCH requests (line 139), lock/unlock should apply the same ownership restriction to prevent members from blocking each other's edits.
🤖 Prompt for 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.
In `@apps/api/plane/api/views/page.py` around lines 264 - 289, The
ProjectPageLockAPIEndpoint methods (post and delete) are missing the ownership
check so any project member can lock/unlock another user's page; update the Page
lookup in both ProjectPageLockAPIEndpoint.post and .delete to include the
ownership filter (owned_by_id=request.user.id) or otherwise verify
page.owned_by_id == request.user.id and return a 403/PermissionDenied when it
doesn't match, mirroring the behavior in ProjectPageAccessAPIEndpoint.post to
ensure only the page owner can change lock state.
Address CodeRabbit findings on makeplane#9020: - Validate `parent` belongs to the same project (PageSerializer.validate_parent) - Wrap Page+ProjectPage create in transaction.atomic() and use get_object_or_404 for the project lookup so a stale project_id surfaces as 404, not 500 - Rename detail URL from `project-pages` to `project-pages-detail` so the two routes don't collide on `reverse()` lookups - Scope the recursive archive CTE by project_id (and exclude soft-deleted ProjectPage rows) so an (un)archive can never cascade across projects or workspaces - Replace the dead `self.kwargs.get("order_by")` with a `_safe_order_by` helper that reads from `request.query_params` and validates against an allow-list (defends against information-disclosure via arbitrary ordering columns) - Replace bare `Page.objects.get(...)` with `get_object_or_404` so missing/ out-of-scope page_ids return clean 404s - Wrap the duplicate handler in transaction.atomic() and link the clone only to the project the request targeted (linking to other projects the source page belongs to would bypass per-project membership checks) - Switch summary aggregation to `Count("id", distinct=True, filter=Q(...))` so counts are not multiplied by the number of project members in the JOIN Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Author
|
Pushed
Findings 7 and 8 were CodeRabbit analysis chains without a main message body — treating them as no-ops; happy to revisit if a finding was intended. Smoke-tested locally on top of
|
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/api/serializers/page.py`:
- Around line 60-72: validate_parent currently allows assigning a page as its
own parent; add a self-reference guard in validate_parent by checking if
self.instance exists and if value.id == getattr(self.instance, "pk", None) and,
if so, raise serializers.ValidationError("Page cannot be its own parent.")
before the project-membership check; ensure this runs only when updating
(self.instance is not None) so creating new pages still allows None parent.
- Around line 26-58: The serializer's Meta currently exposes is_locked and
archived_at as writable; update the Meta.read_only_fields to include "is_locked"
and "archived_at" so these attributes cannot be set via Page creation or update
(leave them listed in Meta.fields but add both "is_locked" and "archived_at" to
the read_only_fields list in the Page serializer's Meta class).
🪄 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: e82c02a4-7cc9-488b-9b9b-0d7971e712a4
📒 Files selected for processing (3)
apps/api/plane/api/serializers/page.pyapps/api/plane/api/urls/page.pyapps/api/plane/api/views/page.py
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/plane/api/urls/page.py
- apps/api/plane/api/views/page.py
Comment on lines
+26
to
+58
| class Meta: | ||
| model = Page | ||
| fields = [ | ||
| "id", | ||
| "name", | ||
| "description_html", | ||
| "description_json", | ||
| "owned_by", | ||
| "access", | ||
| "color", | ||
| "parent", | ||
| "is_locked", | ||
| "archived_at", | ||
| "view_props", | ||
| "logo_props", | ||
| "sort_order", | ||
| "external_id", | ||
| "external_source", | ||
| "workspace", | ||
| "created_at", | ||
| "updated_at", | ||
| "created_by", | ||
| "updated_by", | ||
| ] | ||
| read_only_fields = [ | ||
| "id", | ||
| "owned_by", | ||
| "workspace", | ||
| "created_at", | ||
| "updated_at", | ||
| "created_by", | ||
| "updated_by", | ||
| ] |
Contributor
There was a problem hiding this comment.
is_locked and archived_at should be read-only.
Both fields are managed exclusively by the dedicated /lock/ and /archive/ endpoints (which enforce ownership checks, recursive cascade, etc.). Exposing them as writable in the serializer lets a client silently set is_locked=true or archived_at=<timestamp> on a POST /pages/ or PATCH /pages/<id>/ call, completely bypassing that business logic.
🛡️ Proposed fix
read_only_fields = [
"id",
"owned_by",
"workspace",
"created_at",
"updated_at",
"created_by",
"updated_by",
+ "is_locked",
+ "archived_at",
]🤖 Prompt for 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.
In `@apps/api/plane/api/serializers/page.py` around lines 26 - 58, The
serializer's Meta currently exposes is_locked and archived_at as writable;
update the Meta.read_only_fields to include "is_locked" and "archived_at" so
these attributes cannot be set via Page creation or update (leave them listed in
Meta.fields but add both "is_locked" and "archived_at" to the read_only_fields
list in the Page serializer's Meta class).
Comment on lines
+60
to
+72
| def validate_parent(self, value): | ||
| if value is None: | ||
| return value | ||
| project_id = self.context.get("project_id") | ||
| if project_id and not ProjectPage.objects.filter( | ||
| page_id=value.id, | ||
| project_id=project_id, | ||
| deleted_at__isnull=True, | ||
| ).exists(): | ||
| raise serializers.ValidationError( | ||
| "Parent page must belong to the same project." | ||
| ) | ||
| return value |
Contributor
There was a problem hiding this comment.
validate_parent resolves the previous cross-project assignment concern — but is missing a self-reference guard.
The past review concern about cross-project parent assignment has been addressed. However, validate_parent does not check whether value is the page being updated (i.e., value.id == self.instance.pk on PATCH), which would create a circular parent chain.
🛡️ Proposed fix
def validate_parent(self, value):
if value is None:
return value
+ if self.instance and value.id == self.instance.pk:
+ raise serializers.ValidationError(
+ "A page cannot be its own parent."
+ )
project_id = self.context.get("project_id")
if project_id and not ProjectPage.objects.filter(
page_id=value.id,
project_id=project_id,
deleted_at__isnull=True,
).exists():
raise serializers.ValidationError(
"Parent page must belong to the same project."
)
return value🤖 Prompt for 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.
In `@apps/api/plane/api/serializers/page.py` around lines 60 - 72, validate_parent
currently allows assigning a page as its own parent; add a self-reference guard
in validate_parent by checking if self.instance exists and if value.id ==
getattr(self.instance, "pk", None) and, if so, raise
serializers.ValidationError("Page cannot be its own parent.") before the
project-membership check; ensure this runs only when updating (self.instance is
not None) so creating new pages still allows None parent.
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
2 participants
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.
Description
Exposes Pages in the public REST API namespace (
/api/v1/...) so external clients — integrations, MCP servers, automation scripts — can manage pages with anX-API-Key, instead of being limited to the session-authenticated app API (/api/...).The Page model, ProjectPage join, queries and permission classes already exist; this PR only adds the public-API surface (URLs, views, serializer) and wires it into the existing aggregator. Mirrors the conventions used by
cycleandstate(BaseAPIView,APIKeyAuthentication,ProjectEntityPermission, pagination,external_ididempotency).Closes the gap raised in #7319.
Routes added
All project-scoped, prefixed with
/api/v1/workspaces/<slug>/projects/<project_id>/:pages/pages/<page_id>/pages-summary/archived-pages/pages/<page_id>/archive/pages/<page_id>/lock/pages/<page_id>/access/pages/<page_id>/duplicate/Out of scope for this PR (can land as follow-ups): per-user favorite endpoint (UX-only), description binary endpoint and page versions (collaborative-editor specific).
Type of Change
Test Scenarios
Smoke-tested against
docker-compose-local.ymlon v1.3.0 + this patch. All routes verified end-to-end withcurlusing a Plane API key:200/201/204on list, create, retrieve, patch, deleteexternal_id+external_sourceduplicate →409 Conflictwith existing idlock→204thenPATCH→400 {"error":"Page is locked"}thenunlock→204thenPATCH→200accessaccepts0/1, rejects others with400, owner-only enforcementarchive→200 {archived_at}, double-archive →400, listpages/excludes it, listarchived-pages/includes it,unarchive→204duplicatereturns the new page withname = "<original> (Copy)"and a freshidsummaryreturns{public_pages, private_pages, archived_pages}consistent with create/archive/access flowReferences
Summary by CodeRabbit