mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 10:08:51 +02:00
[INFRA-772] fix(security): stop trusting body-supplied created_by/created_at on external API
Three duplicate/related reports, one mechanism: POST/PATCH handlers for work items, comments and issue links let a caller set created_by and created_at directly from the request body, even though the model already auto-populates created_by correctly on create (BaseModel.save() reads the authenticated user via crum). The escalation: forge created_by via PATCH on an existing issue, then pass IssueDetailAPIEndpoint.delete's "admin OR creator" gate as a plain project member — deleting arbitrary work items. Root cause had two independent halves: - Four view-level blocks (issue POST, issue PUT's external-id upsert create branch, comment POST, issue-link POST) re-fetched the row a serializer.save() had just correctly created — turning it into an UPDATE, where BaseModel.save()'s auto-created_by protection does not apply — then overwrote created_by/created_at straight from request.data. All four removed entirely; the initial serializer.save() already produces the right created_by unaided. - IssueSerializer never marked created_by read-only (unlike updated_by right beside it, and unlike created_at, which Django's auto_now_add protects independently). That let a plain PATCH on an *existing* issue set created_by directly via the serializer — this is what actually makes the delete-escalation chain possible, since PATCH never touched the view-level override blocks at all. Added to IssueSerializer.Meta.read_only_fields. Comment and issue-link PATCH already used properly-protected serializers (created_by already read-only, or absent from fields entirely) — their only gap was the POST-path override, now removed. Verified via fail-before: reverted the fix, 5 of 6 new tests correctly failed (forged attacker-controlled created_by landing in the DB); the 2 that passed regardless are intentional (a same-value no-op and a positive control). Confirmed IssueDetailAPIEndpoint.put has no route mounted anywhere (only ["get", "post"] on IssueListCreateAPIEndpoint) — cleaned up the same override there for consistency, but it's unreachable dead code, not a live vector. Tests: 6 new (apps/api/plane/tests/contract/api/test_created_by_forgery.py). Broader regression sweep (issue/comment/link/notification suites, 49 tests) green. ruff clean. Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -69,7 +69,22 @@ class IssueSerializer(BaseSerializer):
|
|||||||
|
|
||||||
class Meta:
|
class Meta:
|
||||||
model = Issue
|
model = Issue
|
||||||
read_only_fields = ["id", "workspace", "project", "updated_by", "updated_at", "completed_at"]
|
# SECURITY: created_by must stay read-only, matching updated_by right beside it.
|
||||||
|
# created_by is a plain FK with no auto_now_add-style protection, so leaving it
|
||||||
|
# out of this list let any project member set an issue's creator to an arbitrary
|
||||||
|
# user via PATCH — serializer.update() setattrs it before BaseModel.save() runs,
|
||||||
|
# and BaseModel.save() only auto-protects created_by on create (self._state.adding),
|
||||||
|
# never on update. That forged created_by then passed IssueDetailAPIEndpoint.delete's
|
||||||
|
# "creator can delete" check, letting a member delete work items only an admin should.
|
||||||
|
read_only_fields = [
|
||||||
|
"id",
|
||||||
|
"workspace",
|
||||||
|
"project",
|
||||||
|
"created_by",
|
||||||
|
"updated_by",
|
||||||
|
"updated_at",
|
||||||
|
"completed_at",
|
||||||
|
]
|
||||||
exclude = ["description_json", "description_stripped"]
|
exclude = ["description_json", "description_stripped"]
|
||||||
|
|
||||||
def validate(self, data):
|
def validate(self, data):
|
||||||
|
|||||||
@@ -489,11 +489,6 @@ class IssueListCreateAPIEndpoint(BaseAPIView):
|
|||||||
)
|
)
|
||||||
|
|
||||||
serializer.save()
|
serializer.save()
|
||||||
# Refetch the issue
|
|
||||||
issue = Issue.objects.filter(workspace__slug=slug, project_id=project_id, pk=serializer.data["id"]).first()
|
|
||||||
issue.created_at = request.data.get("created_at", timezone.now())
|
|
||||||
issue.created_by_id = request.data.get("created_by", request.user.id)
|
|
||||||
issue.save(update_fields=["created_at", "created_by"])
|
|
||||||
|
|
||||||
# Track the issue
|
# Track the issue
|
||||||
issue_activity.delay(
|
issue_activity.delay(
|
||||||
@@ -703,19 +698,6 @@ class IssueDetailAPIEndpoint(BaseAPIView):
|
|||||||
# issue activity worker event as created
|
# issue activity worker event as created
|
||||||
if serializer.is_valid():
|
if serializer.is_valid():
|
||||||
serializer.save()
|
serializer.save()
|
||||||
# Refetch the issue
|
|
||||||
issue = Issue.objects.filter(
|
|
||||||
workspace__slug=slug,
|
|
||||||
project_id=project_id,
|
|
||||||
pk=serializer.data["id"],
|
|
||||||
).first()
|
|
||||||
|
|
||||||
# If any of the created_at or created_by is present, update
|
|
||||||
# the issue with the provided data, else return with the
|
|
||||||
# default states given.
|
|
||||||
issue.created_at = request.data.get("created_at", timezone.now())
|
|
||||||
issue.created_by_id = request.data.get("created_by", request.user.id)
|
|
||||||
issue.save(update_fields=["created_at", "created_by"])
|
|
||||||
|
|
||||||
issue_activity.delay(
|
issue_activity.delay(
|
||||||
type="issue.activity.created",
|
type="issue.activity.created",
|
||||||
@@ -1198,8 +1180,6 @@ class IssueLinkListCreateAPIEndpoint(BaseAPIView):
|
|||||||
serializer.save(project_id=project_id, issue_id=issue_id)
|
serializer.save(project_id=project_id, issue_id=issue_id)
|
||||||
crawl_work_item_link_title.delay(serializer.instance.id, serializer.instance.url)
|
crawl_work_item_link_title.delay(serializer.instance.id, serializer.instance.url)
|
||||||
link = IssueLink.objects.get(pk=serializer.instance.id)
|
link = IssueLink.objects.get(pk=serializer.instance.id)
|
||||||
link.created_by_id = request.data.get("created_by", request.user.id)
|
|
||||||
link.save(update_fields=["created_by"])
|
|
||||||
issue_activity.delay(
|
issue_activity.delay(
|
||||||
type="link.activity.created",
|
type="link.activity.created",
|
||||||
requested_data=json.dumps(serializer.data, cls=DjangoJSONEncoder),
|
requested_data=json.dumps(serializer.data, cls=DjangoJSONEncoder),
|
||||||
@@ -1481,11 +1461,6 @@ class IssueCommentListCreateAPIEndpoint(BaseAPIView):
|
|||||||
if serializer.is_valid():
|
if serializer.is_valid():
|
||||||
serializer.save(project_id=project_id, issue_id=issue_id, actor=request.user)
|
serializer.save(project_id=project_id, issue_id=issue_id, actor=request.user)
|
||||||
issue_comment = IssueComment.objects.get(pk=serializer.instance.id)
|
issue_comment = IssueComment.objects.get(pk=serializer.instance.id)
|
||||||
# Update the created_at and the created_by and save the comment
|
|
||||||
issue_comment.created_at = request.data.get("created_at", timezone.now())
|
|
||||||
issue_comment.created_by_id = request.data.get("created_by", request.user.id)
|
|
||||||
issue_comment.actor_id = request.data.get("created_by", request.user.id)
|
|
||||||
issue_comment.save(update_fields=["created_at", "created_by"])
|
|
||||||
|
|
||||||
issue_activity.delay(
|
issue_activity.delay(
|
||||||
type="comment.activity.created",
|
type="comment.activity.created",
|
||||||
|
|||||||
240
apps/api/plane/tests/contract/api/test_created_by_forgery.py
Normal file
240
apps/api/plane/tests/contract/api/test_created_by_forgery.py
Normal file
@@ -0,0 +1,240 @@
|
|||||||
|
# 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 the created_by/created_at forgery class on the
|
||||||
|
external API — authorship and audit-trail forgery via body-controlled fields
|
||||||
|
on issue, comment and issue-link create/update endpoints.
|
||||||
|
|
||||||
|
Root cause: view-level code re-fetched a just-created row (turning it into an
|
||||||
|
update, where BaseModel.save()'s auto-created_by protection does not apply)
|
||||||
|
and then wrote created_by/created_at straight from request.data. Separately,
|
||||||
|
IssueSerializer never marked created_by read-only, so a plain PATCH on an
|
||||||
|
existing issue could set it directly via the serializer.
|
||||||
|
|
||||||
|
Fixed by: removing the four view-level override blocks (issue POST, issue PUT
|
||||||
|
upsert-create branch, comment POST, issue-link POST) entirely — the initial
|
||||||
|
serializer.save() already produces the correct created_by via BaseModel's
|
||||||
|
crum-based auto-set on create, which needs no help — and adding created_by to
|
||||||
|
IssueSerializer.Meta.read_only_fields, closing the PATCH vector directly.
|
||||||
|
|
||||||
|
The escalation chain: forge created_by via PATCH, then pass
|
||||||
|
IssueDetailAPIEndpoint.delete's "admin OR creator" gate as a plain member.
|
||||||
|
Test `test_patch_then_delete_forgery_chain_is_blocked` exercises the whole
|
||||||
|
chain end to end, not just the individual forgery.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from rest_framework import status
|
||||||
|
|
||||||
|
from plane.db.models import APIToken, Issue, IssueComment, IssueLink, Project, ProjectMember, State
|
||||||
|
|
||||||
|
|
||||||
|
def _create_issue_as(creator, **kwargs):
|
||||||
|
"""BaseModel.save() sets created_by from crum's current request/user, and
|
||||||
|
there is no active request in a fixture — get_current_user() returns None
|
||||||
|
there, and the model then *nulls* created_by regardless of what the
|
||||||
|
constructor was given. save(created_by_id=...) is the documented escape
|
||||||
|
hatch for exactly this case (see BaseModel.save's signature)."""
|
||||||
|
obj = Issue(**kwargs)
|
||||||
|
obj.save(created_by_id=creator.id)
|
||||||
|
return obj
|
||||||
|
|
||||||
|
pytestmark = pytest.mark.contract
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def admin_user(db, create_user):
|
||||||
|
"""The project admin and original creator of everything in these tests."""
|
||||||
|
return create_user
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def member_user(db):
|
||||||
|
"""A second, lower-privileged account — the attacker in every test here."""
|
||||||
|
from plane.db.models import User
|
||||||
|
|
||||||
|
unique = uuid4().hex[:8]
|
||||||
|
user = User.objects.create(email=f"member-{unique}@plane.so", username=f"member_{unique}")
|
||||||
|
user.set_password("member-password")
|
||||||
|
user.save()
|
||||||
|
return user
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def project(db, workspace, admin_user, member_user):
|
||||||
|
project = Project.objects.create(
|
||||||
|
name="Test Project",
|
||||||
|
identifier="TPF",
|
||||||
|
workspace=workspace,
|
||||||
|
created_by=admin_user,
|
||||||
|
)
|
||||||
|
ProjectMember.objects.create(project=project, member=admin_user, role=20, is_active=True)
|
||||||
|
ProjectMember.objects.create(project=project, member=member_user, role=15, is_active=True)
|
||||||
|
return project
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def state(db, workspace, project):
|
||||||
|
return State.objects.create(name="Todo", project=project, workspace=workspace, group="backlog", default=True)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def issue(db, workspace, project, state, admin_user):
|
||||||
|
"""An issue created by the admin — member_user must never be able to
|
||||||
|
reattribute this to themselves."""
|
||||||
|
return _create_issue_as(
|
||||||
|
admin_user,
|
||||||
|
name="Admin's issue",
|
||||||
|
workspace=workspace,
|
||||||
|
project=project,
|
||||||
|
state=state,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def member_api_client(api_client, member_user):
|
||||||
|
"""External-API client authenticated as the low-privilege member."""
|
||||||
|
token = APIToken.objects.create(user=member_user, label="Member Token", token=f"member-token-{uuid4().hex}")
|
||||||
|
api_client.credentials(HTTP_X_API_KEY=token.token)
|
||||||
|
return api_client
|
||||||
|
|
||||||
|
|
||||||
|
def _issues_url(slug, project_id):
|
||||||
|
return f"/api/v1/workspaces/{slug}/projects/{project_id}/issues/"
|
||||||
|
|
||||||
|
|
||||||
|
def _issue_detail_url(slug, project_id, pk):
|
||||||
|
return f"/api/v1/workspaces/{slug}/projects/{project_id}/issues/{pk}/"
|
||||||
|
|
||||||
|
|
||||||
|
def _links_url(slug, project_id, issue_id):
|
||||||
|
return f"/api/v1/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/links/"
|
||||||
|
|
||||||
|
|
||||||
|
def _comments_url(slug, project_id, issue_id):
|
||||||
|
return f"/api/v1/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/comments/"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
class TestIssueCreateIgnoresBodyCreatedBy:
|
||||||
|
def test_post_sets_created_by_to_the_caller_not_the_body(
|
||||||
|
self, member_api_client, workspace, project, state, admin_user, member_user
|
||||||
|
):
|
||||||
|
url = _issues_url(workspace.slug, project.id)
|
||||||
|
response = member_api_client.post(
|
||||||
|
url,
|
||||||
|
{"name": "Spoofed issue", "state": str(state.id), "created_by": str(admin_user.id)},
|
||||||
|
format="json",
|
||||||
|
)
|
||||||
|
assert response.status_code == status.HTTP_201_CREATED, f"got {response.status_code}: {response.data!r}"
|
||||||
|
|
||||||
|
created = Issue.objects.get(pk=response.data["id"])
|
||||||
|
assert created.created_by_id == member_user.id, (
|
||||||
|
"created_by must be the authenticated caller regardless of what the body requested"
|
||||||
|
)
|
||||||
|
assert created.created_by_id != admin_user.id
|
||||||
|
|
||||||
|
# No test for IssueDetailAPIEndpoint.put's external_id-upsert create branch:
|
||||||
|
# confirmed against apps/api/plane/api/urls/work_item.py that
|
||||||
|
# IssueListCreateAPIEndpoint is only ever mounted with
|
||||||
|
# http_method_names=["get", "post"] (both old_url_patterns and
|
||||||
|
# new_url_patterns) — put() is unreachable dead code. Cleaned up the same
|
||||||
|
# override there anyway for consistency, but there's no route to test it
|
||||||
|
# through.
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
class TestIssuePatchCannotForgeCreatedBy:
|
||||||
|
def test_patch_cannot_reattribute_to_the_caller(
|
||||||
|
self, member_api_client, workspace, project, issue, admin_user, member_user
|
||||||
|
):
|
||||||
|
"""A project member may edit fields on an issue they don't own, but
|
||||||
|
created_by must remain the original creator (admin_user here) — it is
|
||||||
|
not a field this endpoint's contract lets anyone change. Targets
|
||||||
|
member_user specifically (not admin_user, the value already in place)
|
||||||
|
so a no-op forgery attempt can't masquerade as a passing test."""
|
||||||
|
url = _issue_detail_url(workspace.slug, project.id, issue.id)
|
||||||
|
response = member_api_client.patch(
|
||||||
|
url, {"name": "Edited by member", "created_by": str(member_user.id)}, format="json"
|
||||||
|
)
|
||||||
|
assert response.status_code == status.HTTP_200_OK, f"got {response.status_code}: {response.data!r}"
|
||||||
|
|
||||||
|
issue.refresh_from_db()
|
||||||
|
assert issue.created_by_id == admin_user.id, (
|
||||||
|
"created_by must be immutable after creation — a PATCH must never reattribute an issue"
|
||||||
|
)
|
||||||
|
assert issue.name == "Edited by member", "the legitimate field in the same request must still apply"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
class TestForgeryThenDeleteChainIsBlocked:
|
||||||
|
def test_patch_then_delete_is_still_403_for_a_plain_member(
|
||||||
|
self, member_api_client, workspace, project, issue, admin_user, member_user
|
||||||
|
):
|
||||||
|
"""The full escalation chain: forge created_by via PATCH, then rely on
|
||||||
|
the DELETE handler's "creator can delete" gate. Since the PATCH no
|
||||||
|
longer forges anything, the DELETE must still refuse a plain member.
|
||||||
|
"""
|
||||||
|
patch_url = _issue_detail_url(workspace.slug, project.id, issue.id)
|
||||||
|
member_api_client.patch(patch_url, {"created_by": str(member_user.id)}, format="json")
|
||||||
|
issue.refresh_from_db()
|
||||||
|
assert issue.created_by_id != member_user.id, "forgery must not have landed before we even try the delete"
|
||||||
|
|
||||||
|
delete_url = _issue_detail_url(workspace.slug, project.id, issue.id)
|
||||||
|
response = member_api_client.delete(delete_url)
|
||||||
|
|
||||||
|
assert response.status_code == status.HTTP_403_FORBIDDEN, (
|
||||||
|
f"a plain member who is not the real creator must not be able to delete; got {response.status_code}"
|
||||||
|
)
|
||||||
|
assert Issue.objects.filter(pk=issue.id).exists(), "the issue must not have been deleted"
|
||||||
|
|
||||||
|
def test_real_creator_can_still_delete_their_own_issue(
|
||||||
|
self, member_api_client, workspace, project, state, member_user
|
||||||
|
):
|
||||||
|
"""Positive control: the fix must not break the legitimate creator-delete path."""
|
||||||
|
own_issue = _create_issue_as(
|
||||||
|
member_user, name="Member's own issue", workspace=workspace, project=project, state=state
|
||||||
|
)
|
||||||
|
url = _issue_detail_url(workspace.slug, project.id, own_issue.id)
|
||||||
|
response = member_api_client.delete(url)
|
||||||
|
assert response.status_code == status.HTTP_204_NO_CONTENT, f"got {response.status_code}: {response.data!r}"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
class TestCommentCreateIgnoresBodyCreatedBy:
|
||||||
|
def test_post_comment_sets_created_by_to_the_caller(
|
||||||
|
self, member_api_client, workspace, project, issue, admin_user, member_user
|
||||||
|
):
|
||||||
|
url = _comments_url(workspace.slug, project.id, issue.id)
|
||||||
|
response = member_api_client.post(
|
||||||
|
url,
|
||||||
|
{"comment_html": "<p>please approve the payment</p>", "created_by": str(admin_user.id)},
|
||||||
|
format="json",
|
||||||
|
)
|
||||||
|
assert response.status_code == status.HTTP_201_CREATED, f"got {response.status_code}: {response.data!r}"
|
||||||
|
|
||||||
|
comment = IssueComment.objects.get(pk=response.data["id"])
|
||||||
|
assert comment.created_by_id == member_user.id
|
||||||
|
assert comment.created_by_id != admin_user.id
|
||||||
|
assert comment.actor_id == member_user.id, "actor (the audit-log identity) must also be the real caller"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
class TestIssueLinkCreateIgnoresBodyCreatedBy:
|
||||||
|
def test_post_link_sets_created_by_to_the_caller_not_null_or_forged(
|
||||||
|
self, member_api_client, workspace, project, issue, admin_user, member_user
|
||||||
|
):
|
||||||
|
url = _links_url(workspace.slug, project.id, issue.id)
|
||||||
|
response = member_api_client.post(
|
||||||
|
url,
|
||||||
|
{"title": "Spec doc", "url": "https://example.com/spec", "created_by": str(admin_user.id)},
|
||||||
|
format="json",
|
||||||
|
)
|
||||||
|
assert response.status_code == status.HTTP_201_CREATED, f"got {response.status_code}: {response.data!r}"
|
||||||
|
|
||||||
|
link = IssueLink.objects.get(pk=response.data["id"])
|
||||||
|
assert link.created_by_id == member_user.id, "must be the real caller, not forged and not NULL"
|
||||||
|
assert link.created_by_id != admin_user.id
|
||||||
Reference in New Issue
Block a user