From 58547faf312faa8ac827947e368f433cc7201c2c Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Sun, 26 Apr 2026 10:35:47 +0800 Subject: [PATCH] fix(server): validate assignee_id existence on issue create/update (#1694) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(server): validate assignee_id existence on issue create/update POST /api/issues and PUT /api/issues/:id silently accepted any well-formed UUID as assignee_id (#1662). The new validateAssigneePair helper consolidates the existing canAssignAgent check and adds: - existence lookup against workspace members for assignee_type=member - existence lookup against workspace agents for assignee_type=agent - pair consistency: type and id must be both set or both null - whitelist for assignee_type values (member|agent) UpdateIssue and BatchUpdateIssues now run the same validator on the post-merge assignee pair whenever the caller touches either field, closing the parallel gap on the update path. * fix(server): reject malformed assignee_id at handler entry parseUUID silently returns an invalid pgtype.UUID for unparseable input and validateAssigneePair treats (type unset + id invalid) as "no assignee". Together they let `POST /api/issues` and `PUT /api/issues/:id` silently drop a malformed assignee_id and return a successful response. Reject the parse failure inline at every entry point — Create, Update, and BatchUpdateIssues — so the validator never sees an unparseable id. Adds two regression tests covering the create and update paths. --- server/internal/handler/handler_test.go | 212 ++++++++++++++++++++++++ server/internal/handler/issue.go | 117 ++++++++----- 2 files changed, 290 insertions(+), 39 deletions(-) diff --git a/server/internal/handler/handler_test.go b/server/internal/handler/handler_test.go index 216c04317..0c6205ab5 100644 --- a/server/internal/handler/handler_test.go +++ b/server/internal/handler/handler_test.go @@ -530,6 +530,218 @@ func TestCreateSubIssueUsesExplicitProjectOverParentProject(t *testing.T) { } } +// TestCreateIssueRejectsNonexistentMemberAssignee covers the bug where any +// well-formed UUID was accepted as assignee_id without checking workspace +// membership. +func TestCreateIssueRejectsNonexistentMemberAssignee(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Ghost member assignee", + "assignee_type": "member", + "assignee_id": "00000000-0000-0000-0000-000000000000", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400 for nonexistent member, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestCreateIssueRejectsNonexistentAgentAssignee verifies the same check on +// the agent branch — previously rejected with 403 "agent not found"; we want a +// consistent 400 from the new validator. +func TestCreateIssueRejectsNonexistentAgentAssignee(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Ghost agent assignee", + "assignee_type": "agent", + "assignee_id": "00000000-0000-0000-0000-000000000000", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400 for nonexistent agent, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestCreateIssueRejectsAssigneeTypeWithoutID rejects requests where only one +// of the two fields was supplied — historically this would create an issue +// with an inconsistent state. +func TestCreateIssueRejectsAssigneeTypeWithoutID(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Lone assignee_type", + "assignee_type": "member", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400 when only assignee_type is set, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestCreateIssueRejectsAssigneeIDWithoutType is the symmetric case. +func TestCreateIssueRejectsAssigneeIDWithoutType(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Lone assignee_id", + "assignee_id": testUserID, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400 when only assignee_id is set, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestCreateIssueRejectsUnknownAssigneeType guards against typos like +// "members" or "user" that previously sneaked through. +func TestCreateIssueRejectsUnknownAssigneeType(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Bogus assignee_type", + "assignee_type": "user", + "assignee_id": testUserID, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400 for unknown assignee_type, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestCreateIssueAcceptsValidMemberAssignee is the positive control — the +// validator must not block legitimate workspace members. +func TestCreateIssueAcceptsValidMemberAssignee(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Valid member assignee", + "assignee_type": "member", + "assignee_id": testUserID, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201 for valid member assignee, got %d: %s", w.Code, w.Body.String()) + } + + var created IssueResponse + json.NewDecoder(w.Body).Decode(&created) + cleanupReq := newRequest("DELETE", "/api/issues/"+created.ID, nil) + cleanupReq = withURLParam(cleanupReq, "id", created.ID) + testHandler.DeleteIssue(httptest.NewRecorder(), cleanupReq) +} + +// TestCreateIssueRejectsMalformedAssigneeID covers the case where parseUUID +// silently produces an invalid pgtype.UUID and the validator would otherwise +// treat (no type + unparseable id) as "no assignee" and accept the request. +func TestCreateIssueRejectsMalformedAssigneeID(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Malformed assignee_id only", + "assignee_id": "not-a-uuid", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("CreateIssue: expected 400 for malformed assignee_id, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestUpdateIssueRejectsMalformedAssigneeID is the equivalent for the update +// path, where the same parseUUID-shaped gap existed on a previously-unassigned +// issue. +func TestUpdateIssueRejectsMalformedAssigneeID(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Update malformed assignee target", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var created IssueResponse + json.NewDecoder(w.Body).Decode(&created) + defer func() { + cleanupReq := newRequest("DELETE", "/api/issues/"+created.ID, nil) + cleanupReq = withURLParam(cleanupReq, "id", created.ID) + testHandler.DeleteIssue(httptest.NewRecorder(), cleanupReq) + }() + + w = httptest.NewRecorder() + req = newRequest("PUT", "/api/issues/"+created.ID, map[string]any{ + "assignee_id": "not-a-uuid", + }) + req = withURLParam(req, "id", created.ID) + testHandler.UpdateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("UpdateIssue: expected 400 for malformed assignee_id, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestUpdateIssueRejectsNonexistentMemberAssignee verifies the same gap is +// closed on the update path — UpdateIssue previously only validated agents. +func TestUpdateIssueRejectsNonexistentMemberAssignee(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Update assignee target", + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var created IssueResponse + json.NewDecoder(w.Body).Decode(&created) + defer func() { + cleanupReq := newRequest("DELETE", "/api/issues/"+created.ID, nil) + cleanupReq = withURLParam(cleanupReq, "id", created.ID) + testHandler.DeleteIssue(httptest.NewRecorder(), cleanupReq) + }() + + w = httptest.NewRecorder() + req = newRequest("PUT", "/api/issues/"+created.ID, map[string]any{ + "assignee_type": "member", + "assignee_id": "00000000-0000-0000-0000-000000000000", + }) + req = withURLParam(req, "id", created.ID) + testHandler.UpdateIssue(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("UpdateIssue: expected 400 for nonexistent member, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestUpdateIssueAllowsExplicitUnassign verifies that sending null for both +// fields still works after the new validator landed — clearing the assignee +// must not be misclassified as a mismatched pair. +func TestUpdateIssueAllowsExplicitUnassign(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{ + "title": "Issue to unassign", + "assignee_type": "member", + "assignee_id": testUserID, + }) + testHandler.CreateIssue(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue: expected 201, got %d: %s", w.Code, w.Body.String()) + } + var created IssueResponse + json.NewDecoder(w.Body).Decode(&created) + defer func() { + cleanupReq := newRequest("DELETE", "/api/issues/"+created.ID, nil) + cleanupReq = withURLParam(cleanupReq, "id", created.ID) + testHandler.DeleteIssue(httptest.NewRecorder(), cleanupReq) + }() + + w = httptest.NewRecorder() + req = newRequest("PUT", "/api/issues/"+created.ID, map[string]any{ + "assignee_type": nil, + "assignee_id": nil, + }) + req = withURLParam(req, "id", created.ID) + testHandler.UpdateIssue(w, req) + if w.Code != http.StatusOK { + t.Fatalf("UpdateIssue: expected 200 for unassign, got %d: %s", w.Code, w.Body.String()) + } + var updated IssueResponse + json.NewDecoder(w.Body).Decode(&updated) + if updated.AssigneeType != nil || updated.AssigneeID != nil { + t.Fatalf("UpdateIssue: expected assignee cleared, got type=%v id=%v", updated.AssigneeType, updated.AssigneeID) + } +} + func TestCommentCRUD(t *testing.T) { // Create an issue first w := httptest.NewRecorder() diff --git a/server/internal/handler/issue.go b/server/internal/handler/issue.go index e1f53b691..cee39fddb 100644 --- a/server/internal/handler/issue.go +++ b/server/internal/handler/issue.go @@ -806,16 +806,20 @@ func (h *Handler) CreateIssue(w http.ResponseWriter, r *http.Request) { } if req.AssigneeID != nil { assigneeID = parseUUID(*req.AssigneeID) - } - - // Enforce agent visibility: private agents can only be assigned by owner/admin. - if req.AssigneeType != nil && *req.AssigneeType == "agent" && req.AssigneeID != nil { - if ok, msg := h.canAssignAgent(r.Context(), r, *req.AssigneeID, workspaceID); !ok { - writeError(w, http.StatusForbidden, msg) + // parseUUID silently returns an invalid pgtype.UUID for malformed input. + // Reject explicitly so the validator below cannot mistake "type unset + // + id unparseable" for "no assignee" and accept the request. + if !assigneeID.Valid { + writeError(w, http.StatusBadRequest, "assignee_id is not a valid UUID") return } } + if status, msg := h.validateAssigneePair(r.Context(), r, workspaceID, assigneeType, assigneeID); status != 0 { + writeError(w, status, msg) + return + } + var parentIssueID pgtype.UUID var projectID pgtype.UUID if req.ProjectID != nil { @@ -1005,6 +1009,10 @@ func (h *Handler) UpdateIssue(w http.ResponseWriter, r *http.Request) { if _, ok := rawFields["assignee_id"]; ok { if req.AssigneeID != nil { params.AssigneeID = parseUUID(*req.AssigneeID) + if !params.AssigneeID.Valid { + writeError(w, http.StatusBadRequest, "assignee_id is not a valid UUID") + return + } } else { params.AssigneeID = pgtype.UUID{Valid: false} // explicit null = unassign } @@ -1063,10 +1071,14 @@ func (h *Handler) UpdateIssue(w http.ResponseWriter, r *http.Request) { } } - // Enforce agent visibility: private agents can only be assigned by owner/admin. - if req.AssigneeType != nil && *req.AssigneeType == "agent" && req.AssigneeID != nil { - if ok, msg := h.canAssignAgent(r.Context(), r, *req.AssigneeID, workspaceID); !ok { - writeError(w, http.StatusForbidden, msg) + // Validate the resulting (assignee_type, assignee_id) pair when the caller + // touches either field. Existing data on the issue is left alone if the + // caller is not changing it. + _, touchedType := rawFields["assignee_type"] + _, touchedID := rawFields["assignee_id"] + if touchedType || touchedID { + if status, msg := h.validateAssigneePair(r.Context(), r, workspaceID, params.AssigneeType, params.AssigneeID); status != 0 { + writeError(w, status, msg) return } } @@ -1143,35 +1155,56 @@ func (h *Handler) UpdateIssue(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, resp) } -// canAssignAgent checks whether the requesting user is allowed to assign issues -// to the given agent. Private agents can only be assigned by their owner or -// workspace admins/owners. -func (h *Handler) canAssignAgent(ctx context.Context, r *http.Request, agentID, workspaceID string) (bool, string) { - agent, err := h.Queries.GetAgentInWorkspace(ctx, db.GetAgentInWorkspaceParams{ - ID: parseUUID(agentID), - WorkspaceID: parseUUID(workspaceID), - }) - if err != nil { - return false, "agent not found" +// validateAssigneePair verifies the (assignee_type, assignee_id) pair refers +// to an existing entity in the workspace. For agent assignees it also enforces +// visibility (private agents are only assignable by their owner or by +// workspace admins/owners) and rejects archived agents. +// +// Returns (statusCode, errorMessage). statusCode == 0 means the pair is valid; +// callers should treat any non-zero status as a rejection and surface it back +// to the client. +func (h *Handler) validateAssigneePair(ctx context.Context, r *http.Request, workspaceID string, assigneeType pgtype.Text, assigneeID pgtype.UUID) (int, string) { + // Both unset → unassigned issue, valid. + if !assigneeType.Valid && !assigneeID.Valid { + return 0, "" } - if agent.ArchivedAt.Valid { - return false, "cannot assign to archived agent" + // Exactly one of type/id provided → callers must always pair them. + if assigneeType.Valid != assigneeID.Valid { + return http.StatusBadRequest, "assignee_type and assignee_id must be provided together" } - if agent.Visibility != "private" { - return true, "" + switch assigneeType.String { + case "member": + if _, err := h.Queries.GetMemberByUserAndWorkspace(ctx, db.GetMemberByUserAndWorkspaceParams{ + UserID: assigneeID, + WorkspaceID: parseUUID(workspaceID), + }); err != nil { + return http.StatusBadRequest, "assignee_id does not refer to a member of this workspace" + } + return 0, "" + case "agent": + agent, err := h.Queries.GetAgentInWorkspace(ctx, db.GetAgentInWorkspaceParams{ + ID: assigneeID, + WorkspaceID: parseUUID(workspaceID), + }) + if err != nil { + return http.StatusBadRequest, "assignee_id does not refer to an agent of this workspace" + } + if agent.ArchivedAt.Valid { + return http.StatusBadRequest, "cannot assign to archived agent" + } + if agent.Visibility == "private" { + userID := requestUserID(r) + if uuidToString(agent.OwnerID) != userID { + member, err := h.getWorkspaceMember(ctx, userID, workspaceID) + if err != nil || !roleAllowed(member.Role, "owner", "admin") { + return http.StatusForbidden, "cannot assign to private agent" + } + } + } + return 0, "" + default: + return http.StatusBadRequest, "assignee_type must be 'member' or 'agent'" } - userID := requestUserID(r) - if uuidToString(agent.OwnerID) == userID { - return true, "" - } - member, err := h.getWorkspaceMember(ctx, userID, workspaceID) - if err != nil { - return false, "cannot assign to private agent" - } - if roleAllowed(member.Role, "owner", "admin") { - return true, "" - } - return false, "cannot assign to private agent" } // shouldEnqueueAgentTask returns true when an issue creation or assignment @@ -1335,6 +1368,9 @@ func (h *Handler) BatchUpdateIssues(w http.ResponseWriter, r *http.Request) { if _, ok := rawUpdates["assignee_id"]; ok { if req.Updates.AssigneeID != nil { params.AssigneeID = parseUUID(*req.Updates.AssigneeID) + if !params.AssigneeID.Valid { + continue + } } else { params.AssigneeID = pgtype.UUID{Valid: false} } @@ -1395,9 +1431,12 @@ func (h *Handler) BatchUpdateIssues(w http.ResponseWriter, r *http.Request) { } } - // Enforce agent visibility for batch assignment. - if req.Updates.AssigneeType != nil && *req.Updates.AssigneeType == "agent" && req.Updates.AssigneeID != nil { - if ok, _ := h.canAssignAgent(r.Context(), r, *req.Updates.AssigneeID, workspaceID); !ok { + // Validate the resulting assignee pair when this batch update touches + // either assignee field. Skip the issue silently on failure. + _, batchTouchedType := rawUpdates["assignee_type"] + _, batchTouchedID := rawUpdates["assignee_id"] + if batchTouchedType || batchTouchedID { + if status, _ := h.validateAssigneePair(r.Context(), r, workspaceID, params.AssigneeType, params.AssigneeID); status != 0 { continue } }