From 95cbc82d9ffda33bac829fe383bbe7fe93dbba1f Mon Sep 17 00:00:00 2001 From: Semih702 Date: Wed, 29 Jul 2026 11:55:08 +0300 Subject: [PATCH 1/2] fix: reject unassignable assignees before Plane silently clears the field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plane filters ids it will not accept out of `assignees` during validation instead of rejecting them, and an update deletes the work item's existing assignees before writing that filtered list. A single unassignable id therefore clears the assignees and still answers 200 — the caller sees a successful write that both failed and destroyed data. Three ways to trip it, none of them visible in the response: the user is a workspace member but not a member of this project, their project role is guest (below the member floor), or the project membership is inactive. `manage_work_item_assignee` is the worst case, because it reads the current assignees, appends one id and writes the whole list back — so a single bad id discards the list it just read, which is exactly what that tool exists to avoid. Check the project's members before the write and raise instead, naming both the rejected ids and the ids that would work so the caller can recover. Verified against self-managed Plane 1.2.0; the filtering lives in the shared issue serializer, so Cloud may behave the same way, but that is unverified. The check degrades safely. Deployments whose member payload omits `role` and `is_active` fall back to a membership-only check, so an unreported role does not disqualify every member. If the member lookup fails or answers in an unexpected shape, the write proceeds exactly as it did before — a pre-check must not become a new failure mode. Writes that carry no assignees issue no extra request. --- plane_mcp/tools/work_items.py | 90 ++++++++++++++++++++++ tests/test_work_items.py | 139 ++++++++++++++++++++++++++++++++++ 2 files changed, 229 insertions(+) diff --git a/plane_mcp/tools/work_items.py b/plane_mcp/tools/work_items.py index ecba3875..e25e4157 100644 --- a/plane_mcp/tools/work_items.py +++ b/plane_mcp/tools/work_items.py @@ -49,6 +49,86 @@ def _ids(items: Any) -> list[str]: return ids +# Plane's project role ladder: guest=5, member=15, admin=20. Only member and +# above can hold a work item assignment. +_MEMBER_ROLE = 15 + + +def _is_assignable(member: Any) -> bool: + """Whether a project member can hold a work item assignment. + + `role` and `is_active` are missing from some deployments' member payloads. + Only reject on a field the server actually reported — an unreported role + must not disqualify every member. + """ + if getattr(member, "is_active", None) is False: + return False + role = getattr(member, "role", None) + return role is None or role >= _MEMBER_ROLE + + +def _assert_assignable(client: Any, workspace_slug: str, project_id: str, assignees: list[str]) -> None: + """Reject assignees Plane would drop, before a write that would clear the field. + + Plane filters unassignable ids out of `assignees` during validation rather + than rejecting them, and an update deletes the work item's existing + assignees before writing that filtered list. A single unassignable id + therefore clears the assignees and still answers 200, so the caller sees a + successful write that both failed and destroyed data. Checking first turns + it into an error naming the ids that would work. + + Args: + client: Plane client. + workspace_slug: The workspace slug identifier. + project_id: UUID of the project the work item belongs to. + assignees: User IDs the caller asked to assign. + + Raises: + ValueError: If any id is not an assignable member of the project. + """ + if not assignees: + return + + try: + members = client.projects.get_members(workspace_slug=workspace_slug, project_id=project_id) + known = {str(m.id): m for m in members} + assignable = {uid: m for uid, m in known.items() if _is_assignable(m)} + # A pre-check must not become a new failure mode: if the lookup is + # unavailable or answers in an unexpected shape, let the write proceed + # exactly as it did before. + except Exception as e: + logger.warning("Could not verify assignees against project %s members: %s", project_id, e) + return + + rejected = [uid for uid in assignees if uid not in assignable] + if not rejected: + return + + details = [] + for uid in rejected: + member = known.get(uid) + if member is None: + details.append(f"{uid} (not a member of this project)") + elif getattr(member, "is_active", None) is False: + details.append(f"{uid} ({member.email}, project membership is inactive)") + else: + details.append(f"{uid} ({member.email}, project role {member.role} is below member)") + + roster = sorted(f"{m.email}={uid}" for uid, m in assignable.items()) + shown = ", ".join(roster[:25]) or "(none)" + if len(roster) > 25: + shown += f", … and {len(roster) - 25} more" + + raise ValueError( + "Plane silently drops assignees it will not accept, and an update clears the work item's " + "existing assignees in the process. Rejected: " + + "; ".join(details) + + ". Only active project members at member role or above can be assigned. " + + f"Assignable members of this project: {shown}. " + + "Add the user to the project first, or use one of the IDs above." + ) + + def _resolve_description_html(description_html: str | None, description_stripped: str | None) -> str | None: """Resolve the description_html to persist. @@ -262,6 +342,8 @@ def create_work_item( priority if priority in get_args(PriorityEnum) else None # type: ignore[assignment] ) + _assert_assignable(client, workspace_slug, project_id, assignees or []) + data = CreateWorkItem( name=name, assignees=assignees, @@ -445,6 +527,8 @@ def update_work_item( priority if priority in get_args(PriorityEnum) else None # type: ignore[assignment] ) + _assert_assignable(client, workspace_slug, project_id, assignees or []) + data = UpdateWorkItem( name=name, assignees=assignees, @@ -508,6 +592,12 @@ def manage_work_item_assignee( Updated WorkItem object """ client, workspace_slug = get_plane_client_context() + # The update below rewrites the whole list, so an unassignable add_user_id + # would clear the very assignees this tool exists to preserve. Only the + # incoming id is checked: an already-assigned member who has since lost + # access must not block a removal. + _assert_assignable(client, workspace_slug, project_id, [add_user_id] if add_user_id else []) + current = client.work_items.retrieve( workspace_slug=workspace_slug, project_id=project_id, work_item_id=work_item_id ) diff --git a/tests/test_work_items.py b/tests/test_work_items.py index b370424e..b2f0876d 100644 --- a/tests/test_work_items.py +++ b/tests/test_work_items.py @@ -2,7 +2,10 @@ import asyncio +import pytest from fastmcp import Client, FastMCP +from fastmcp.exceptions import ToolError +from plane.models.projects import ProjectMember from plane.models.work_items import WorkItem, WorkItemDetail from plane_mcp.tools import work_items as work_item_tools @@ -14,6 +17,12 @@ class FakeWorkItems: def __init__(self): self.updated = None + self.created = None + + def create(self, workspace_slug, project_id, data): + """Record the create so a test can assert it was never issued.""" + self.created = data + return WorkItem.model_validate({"id": "w", "name": data.name}) def retrieve(self, workspace_slug, project_id, work_item_id): return WorkItemDetail.model_validate( @@ -95,3 +104,133 @@ def test_manage_assignee_remove_bare_string(monkeypatch): {"project_id": "p", "work_item_id": "w", "remove_user_id": "existing-user"}, ) assert client.work_items.updated.assignees == [] + + +# --- assignee assignability guard ------------------------------------------- +# +# Plane filters unassignable ids out of `assignees` during validation instead of +# rejecting them, then deletes the work item's existing assignees before writing +# that filtered list. One bad id clears the field and still answers 200, so the +# guard has to run before the write. + +MEMBER = {"id": "member-1", "email": "member@example.com", "role": 20, "is_active": True} +GUEST = {"id": "guest-1", "email": "guest@example.com", "role": 5, "is_active": True} +INACTIVE = {"id": "inactive-1", "email": "inactive@example.com", "role": 20, "is_active": False} +BARE = {"id": "bare-1", "email": "bare@example.com"} # CE omits role/is_active + + +class FakeProjects: + """`get_members` returns a bare list, the shape the non-lite endpoint serves.""" + + def __init__(self, members, error=None): + self.members = members + self.error = error + self.calls = 0 + + def get_members(self, workspace_slug, project_id): + """Return the configured members, or raise to simulate the lookup being unavailable.""" + self.calls += 1 + if self.error: + raise self.error + return [ProjectMember.model_validate(m) for m in self.members] + + +class MemberAwareClient(FakeClient): + """A FakeClient that can also answer the project-member lookup the guard makes.""" + + def __init__(self, members, error=None): + super().__init__() + self.projects = FakeProjects(members, error) + + +def _assign(monkeypatch, client, assignees, tool="update_work_item"): + """Call a write tool with the given assignees, defaulting to the destructive update path.""" + args = {"project_id": "p", "work_item_id": "w", "assignees": assignees} + if tool == "create_work_item": + args = {"project_id": "p", "name": "X", "assignees": assignees} + return _call(monkeypatch, client, tool, args) + + +@pytest.mark.parametrize( + "members,bad_id,reason", + [ + ([MEMBER], "stranger-1", "stranger-1 (not a member of this project)"), + ([MEMBER, GUEST], "guest-1", "project role 5 is below member"), + ([MEMBER, INACTIVE], "inactive-1", "project membership is inactive"), + ], + ids=["not-a-member", "below-member-role", "inactive-membership"], +) +def test_unassignable_assignee_is_rejected_before_the_write(monkeypatch, members, bad_id, reason): + """Each way Plane filters an id out must raise, name why, and leave the write unsent. + + The error also has to name an id that would work — a caller cannot guess one. + """ + client = MemberAwareClient(members) + with pytest.raises(ToolError) as exc: + _assign(monkeypatch, client, [bad_id]) + assert reason in str(exc.value) + assert "member@example.com=member-1" in str(exc.value) + assert client.work_items.updated is None, "the destructive update must not be issued" + + +def test_assignable_member_passes_through(monkeypatch): + """The happy path is unchanged: a real member is written as before.""" + client = MemberAwareClient([MEMBER]) + _assign(monkeypatch, client, ["member-1"]) + assert client.work_items.updated.assignees == ["member-1"] + + +def test_members_without_role_are_accepted_on_membership_alone(monkeypatch): + """Some deployments omit role/is_active; an unreported role must not fail everyone.""" + client = MemberAwareClient([BARE]) + _assign(monkeypatch, client, ["bare-1"]) + assert client.work_items.updated.assignees == ["bare-1"] + + +def test_member_lookup_failure_does_not_block_the_write(monkeypatch): + """A pre-check must not become a new failure mode when the lookup itself breaks.""" + client = MemberAwareClient([], error=RuntimeError("members endpoint unavailable")) + _assign(monkeypatch, client, ["member-1"]) + assert client.work_items.updated.assignees == ["member-1"] + + +def test_empty_assignees_skips_the_member_lookup(monkeypatch): + """Writes that carry no assignees must not pay for an extra request.""" + client = MemberAwareClient([MEMBER]) + _call(monkeypatch, client, "update_work_item", {"project_id": "p", "work_item_id": "w", "name": "Y"}) + assert client.projects.calls == 0 + + +def test_create_work_item_rejects_unassignable_assignee(monkeypatch): + """Create cannot wipe anything, but the requested assignment still silently fails.""" + client = MemberAwareClient([MEMBER]) + with pytest.raises(ToolError) as exc: + _assign(monkeypatch, client, ["stranger-1"], tool="create_work_item") + assert "not a member of this project" in str(exc.value) + assert client.work_items.created is None + + +def test_manage_assignee_rejects_unassignable_add(monkeypatch): + """The read-modify-write in manage_work_item_assignee is what makes this destructive.""" + client = MemberAwareClient([MEMBER]) + with pytest.raises(ToolError): + _call( + monkeypatch, + client, + "manage_work_item_assignee", + {"project_id": "p", "work_item_id": "w", "add_user_id": "stranger-1"}, + ) + assert client.work_items.updated is None, "existing-user must survive a rejected add" + + +def test_manage_assignee_removal_is_not_blocked_by_a_stale_assignee(monkeypatch): + """Only the incoming id is checked, so losing access does not trap the existing list.""" + client = MemberAwareClient([MEMBER]) # "existing-user" is no longer a member + _call( + monkeypatch, + client, + "manage_work_item_assignee", + {"project_id": "p", "work_item_id": "w", "remove_user_id": "existing-user"}, + ) + assert client.work_items.updated.assignees == [] + assert client.projects.calls == 0, "a pure removal introduces no id to check" From 81ea6db5788f3a6cf1671f0d1eabe056878f2d15 Mon Sep 17 00:00:00 2001 From: Semih702 Date: Sat, 1 Aug 2026 22:52:08 +0300 Subject: [PATCH 2/2] test: cover the member-role boundary exactly at the threshold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plane's cutoff is `role >= 15`, but the fixtures only carried role 20 and role 5 — nothing sat on 15 itself. An off-by-one in that comparison would have locked out every plain member while the suite stayed green. Parametrizes the pass-through test over both an above-threshold role and one exactly at it. Flipping `>=` to `>` now fails the boundary case and nothing else. --- tests/test_work_items.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/test_work_items.py b/tests/test_work_items.py index b2f0876d..f7720161 100644 --- a/tests/test_work_items.py +++ b/tests/test_work_items.py @@ -114,6 +114,7 @@ def test_manage_assignee_remove_bare_string(monkeypatch): # guard has to run before the write. MEMBER = {"id": "member-1", "email": "member@example.com", "role": 20, "is_active": True} +AT_THRESHOLD = {"id": "member-2", "email": "member2@example.com", "role": 15, "is_active": True} GUEST = {"id": "guest-1", "email": "guest@example.com", "role": 5, "is_active": True} INACTIVE = {"id": "inactive-1", "email": "inactive@example.com", "role": 20, "is_active": False} BARE = {"id": "bare-1", "email": "bare@example.com"} # CE omits role/is_active @@ -173,11 +174,16 @@ def test_unassignable_assignee_is_rejected_before_the_write(monkeypatch, members assert client.work_items.updated is None, "the destructive update must not be issued" -def test_assignable_member_passes_through(monkeypatch): - """The happy path is unchanged: a real member is written as before.""" - client = MemberAwareClient([MEMBER]) - _assign(monkeypatch, client, ["member-1"]) - assert client.work_items.updated.assignees == ["member-1"] +@pytest.mark.parametrize("member", [MEMBER, AT_THRESHOLD], ids=["above-threshold", "exactly-at-threshold"]) +def test_assignable_member_passes_through(monkeypatch, member): + """The happy path is unchanged, including a role sitting exactly on the member floor. + + Plane's cutoff is `role >= 15`, so role 15 is the value that decides whether the + comparison is inclusive — an off-by-one there would lock out every plain member. + """ + client = MemberAwareClient([member]) + _assign(monkeypatch, client, [member["id"]]) + assert client.work_items.updated.assignees == [member["id"]] def test_members_without_role_are_accepted_on_membership_alone(monkeypatch):