fix(server): validate assignee_id existence on issue create/update (#1694)

* 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.
2026-06-17 03:38:32 +02:00 · 2026-04-26 10:35:47 +08:00
parent 9b55b2a9ce
commit 58547faf31
2 changed files with 290 additions and 39 deletions
--- 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()
--- 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
 			}
 		}