diff --git a/apps/api/internal/handler/issue.go b/apps/api/internal/handler/issue.go index e241f967..822e87de 100644 --- a/apps/api/internal/handler/issue.go +++ b/apps/api/internal/handler/issue.go @@ -17,6 +17,14 @@ type IssueHandler struct { Issue *service.IssueService } +// invalidRelationError reports whether err is a rejected related-id error that +// should surface as a 400 (bad state/label/parent/assignee for the scope). +func invalidRelationError(err error) bool { + return err == service.ErrInvalidState || err == service.ErrInvalidLabel || + err == service.ErrInvalidParent || err == service.ErrInvalidAssignee || + err == service.ErrInvalidPriority +} + func issueID(c *gin.Context) (uuid.UUID, bool) { idStr := c.Param("pk") if idStr == "" { @@ -197,6 +205,10 @@ func (h *IssueHandler) Create(c *gin.Context) { c.JSON(http.StatusNotFound, gin.H{"error": "Not found"}) return } + if invalidRelationError(err) { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create issue"}) return } @@ -305,6 +317,10 @@ func (h *IssueHandler) Update(c *gin.Context) { c.JSON(http.StatusNotFound, gin.H{"error": "Issue not found"}) return } + if invalidRelationError(err) { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to update issue"}) return } diff --git a/apps/api/internal/handler/issue_relation_validation_test.go b/apps/api/internal/handler/issue_relation_validation_test.go new file mode 100644 index 00000000..5c49c499 --- /dev/null +++ b/apps/api/internal/handler/issue_relation_validation_test.go @@ -0,0 +1,83 @@ +package handler_test + +import ( + "net/http" + "testing" + + "github.com/Devlaner/devlane/api/internal/testutil" + "github.com/stretchr/testify/require" +) + +func TestIssue_CreateRejectsForeignRelations(t *testing.T) { + ts := testutil.NewTestServer(t) + w := testutil.SeedWorld(t, ts.DB) + + // A second project in the same workspace, and one in another workspace. + otherProject := testutil.CreateProject(t, ts.DB, w.Workspace.ID, w.User.ID) + foreignState := testutil.CreateState(t, ts.DB, otherProject.ID, w.Workspace.ID) + foreignLabel := testutil.CreateLabel(t, ts.DB, otherProject.ID, w.Workspace.ID) + foreignParent := testutil.CreateIssue(t, ts.DB, otherProject.ID, w.Workspace.ID, w.User.ID) + nonMember := testutil.CreateUser(t, ts.DB) + + base := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() + "/issues/" + + require.Equal(t, http.StatusBadRequest, + ts.POST(base, map[string]any{"name": "x", "state_id": foreignState.ID.String()}, w.Session).Code, + "a state from another project must be rejected") + require.Equal(t, http.StatusBadRequest, + ts.POST(base, map[string]any{"name": "x", "label_ids": []string{foreignLabel.ID.String()}}, w.Session).Code, + "a label from another project must be rejected") + require.Equal(t, http.StatusBadRequest, + ts.POST(base, map[string]any{"name": "x", "parent_id": foreignParent.ID.String()}, w.Session).Code, + "a parent from another project must be rejected") + require.Equal(t, http.StatusBadRequest, + ts.POST(base, map[string]any{"name": "x", "assignee_ids": []string{nonMember.ID.String()}}, w.Session).Code, + "an assignee who isn't a workspace member must be rejected") + + // A well-scoped create still works. + okState := testutil.CreateState(t, ts.DB, w.Project.ID, w.Workspace.ID) + okLabel := testutil.CreateLabel(t, ts.DB, w.Project.ID, w.Workspace.ID) + okParent := testutil.CreateIssue(t, ts.DB, w.Project.ID, w.Workspace.ID, w.User.ID) + rr := ts.POST(base, map[string]any{ + "name": "valid", + "state_id": okState.ID.String(), + "label_ids": []string{okLabel.ID.String()}, + "assignee_ids": []string{w.User.ID.String()}, + "parent_id": okParent.ID.String(), + }, w.Session) + require.Equal(t, http.StatusCreated, rr.Code, "body=%s", rr.Body.String()) +} + +func TestIssue_UpdateRejectsForeignRelations(t *testing.T) { + ts := testutil.NewTestServer(t) + w := testutil.SeedWorld(t, ts.DB) + otherProject := testutil.CreateProject(t, ts.DB, w.Workspace.ID, w.User.ID) + foreignState := testutil.CreateState(t, ts.DB, otherProject.ID, w.Workspace.ID) + foreignLabel := testutil.CreateLabel(t, ts.DB, otherProject.ID, w.Workspace.ID) + nonMember := testutil.CreateUser(t, ts.DB) + + issue := testutil.CreateIssue(t, ts.DB, w.Project.ID, w.Workspace.ID, w.User.ID) + base := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() + + "/issues/" + issue.ID.String() + "/" + + require.Equal(t, http.StatusBadRequest, + ts.PATCH(base, map[string]any{"state_id": foreignState.ID.String()}, w.Session).Code) + require.Equal(t, http.StatusBadRequest, + ts.PATCH(base, map[string]any{"label_ids": []string{foreignLabel.ID.String()}}, w.Session).Code) + require.Equal(t, http.StatusBadRequest, + ts.PATCH(base, map[string]any{"assignee_ids": []string{nonMember.ID.String()}}, w.Session).Code) + + // A parent from another project is rejected. + foreignParent := testutil.CreateIssue(t, ts.DB, otherProject.ID, w.Workspace.ID, w.User.ID) + require.Equal(t, http.StatusBadRequest, + ts.PATCH(base, map[string]any{"parent_id": foreignParent.ID.String()}, w.Session).Code) + + // An issue can't be made its own parent. + require.Equal(t, http.StatusBadRequest, + ts.PATCH(base, map[string]any{"parent_id": issue.ID.String()}, w.Session).Code) + + // A well-scoped update still works. + okState := testutil.CreateState(t, ts.DB, w.Project.ID, w.Workspace.ID) + require.Equal(t, http.StatusOK, + ts.PATCH(base, map[string]any{"state_id": okState.ID.String()}, w.Session).Code) +} diff --git a/apps/api/internal/router/router.go b/apps/api/internal/router/router.go index d99ce034..c76ad86c 100644 --- a/apps/api/internal/router/router.go +++ b/apps/api/internal/router/router.go @@ -161,6 +161,7 @@ func New(cfg Config) *gin.Engine { issueReactionStore := store.NewIssueReactionStore(cfg.DB) issueSvc.SetReactionStore(issueReactionStore) issueSvc.SetStateStore(stateStore) + issueSvc.SetLabelStore(labelStore) commentReactionStore := store.NewCommentReactionStore(cfg.DB) commentSvc := service.NewCommentService(commentStore, issueStore, projectStore, workspaceStore) commentSvc.SetReactionStore(commentReactionStore) diff --git a/apps/api/internal/service/issue.go b/apps/api/internal/service/issue.go index 2d405a8d..aa365d7e 100644 --- a/apps/api/internal/service/issue.go +++ b/apps/api/internal/service/issue.go @@ -25,6 +25,11 @@ var ( ErrEpicHasChildren = errors.New("epic has child work items") // ErrMoveSameProject is returned when a move targets the issue's current project. ErrMoveSameProject = errors.New("issue already in target project") + // ErrInvalidLabel / ErrInvalidParent / ErrInvalidAssignee are returned when a + // related id supplied on create/update doesn't belong to the allowed scope. + ErrInvalidLabel = errors.New("invalid label for project") + ErrInvalidParent = errors.New("invalid parent for project") + ErrInvalidAssignee = errors.New("assignee is not a workspace member") ) // validPriorities is the accepted set of work-item priority values. @@ -41,7 +46,8 @@ type IssueService struct { notify *NotificationService // optional — may be nil subs *store.IssueSubscriberStore // optional — auto-subscribe assignees/mentions reactions *store.IssueReactionStore // optional — per-issue emoji reactions - states *store.StateStore // optional — validates state ownership on bulk update + states *store.StateStore // optional — validates state ownership + labels *store.LabelStore // optional — validates label ownership } func NewIssueService(is *store.IssueStore, ps *store.ProjectStore, ws *store.WorkspaceStore) *IssueService { @@ -63,9 +69,74 @@ func (s *IssueService) SetSubscriberStore(subs *store.IssueSubscriberStore) { s. // SetReactionStore wires per-issue emoji reactions support. Optional. func (s *IssueService) SetReactionStore(r *store.IssueReactionStore) { s.reactions = r } -// SetStateStore wires state-ownership validation for bulk updates. Optional. +// SetStateStore wires state-ownership validation. Optional. func (s *IssueService) SetStateStore(st *store.StateStore) { s.states = st } +// SetLabelStore wires label-ownership validation. Optional. +func (s *IssueService) SetLabelStore(l *store.LabelStore) { s.labels = l } + +// validateRelations rejects related ids that fall outside the allowed scope: +// state and labels must belong to the same project, a parent must be another +// issue in the same project (and never the issue itself — pass selfID on +// update; uuid.Nil on create), and assignees must be members of the workspace. +// Only the provided (non-nil / non-empty) fields are checked. Ownership stores +// are optional; when one isn't wired the corresponding check is skipped. A +// genuine "not found" maps to the invalid-* sentinel, but any other datastore +// error is returned so query failures surface as 5xx, not 400. +func (s *IssueService) validateRelations(ctx context.Context, projectID, workspaceID, selfID uuid.UUID, stateID *uuid.UUID, labelIDs []uuid.UUID, assigneeIDs []uuid.UUID, parentID *uuid.UUID) error { + if stateID != nil && s.states != nil { + st, err := s.states.GetByID(ctx, *stateID) + if errors.Is(err, gorm.ErrRecordNotFound) { + return ErrInvalidState + } + if err != nil { + return err + } + if st == nil || st.ProjectID != projectID { + return ErrInvalidState + } + } + if len(labelIDs) > 0 && s.labels != nil { + for _, id := range labelIDs { + l, err := s.labels.GetByID(ctx, id) + if errors.Is(err, gorm.ErrRecordNotFound) { + return ErrInvalidLabel + } + if err != nil { + return err + } + if l == nil || l.ProjectID == nil || *l.ProjectID != projectID { + return ErrInvalidLabel + } + } + } + if parentID != nil { + if *parentID == selfID { + return ErrInvalidParent // an issue can't be its own parent + } + parent, err := s.is.GetByID(ctx, *parentID) + if errors.Is(err, gorm.ErrRecordNotFound) { + return ErrInvalidParent + } + if err != nil { + return err + } + if parent == nil || parent.ProjectID != projectID { + return ErrInvalidParent + } + } + for _, id := range assigneeIDs { + ok, err := s.ws.IsMember(ctx, workspaceID, id) + if err != nil { + return err + } + if !ok { + return ErrInvalidAssignee + } + } + return nil +} + // autoSubscribe is a fire-and-forget helper used by the assignee and mention // hooks. Errors are logged-and-ignored — the user's primary action must not // fail because of a subscription bookkeeping issue. @@ -474,6 +545,9 @@ func (s *IssueService) Create(ctx context.Context, workspaceSlug string, project return nil, err } wrk, _ := s.ws.GetBySlug(ctx, workspaceSlug) + if err := s.validateRelations(ctx, projectID, wrk.ID, uuid.Nil, stateID, labelIDs, assigneeIDs, parentID); err != nil { + return nil, err + } issue := &model.Issue{ Name: name, ProjectID: projectID, @@ -549,6 +623,18 @@ func (s *IssueService) Update(ctx context.Context, workspaceSlug string, project return nil, err } + // Reject related ids outside the allowed scope before touching anything. + var wantAssignees, wantLabels []uuid.UUID + if assigneeIDs != nil { + wantAssignees = *assigneeIDs + } + if labelIDs != nil { + wantLabels = *labelIDs + } + if err := s.validateRelations(ctx, issue.ProjectID, issue.WorkspaceID, issue.ID, stateID, wantLabels, wantAssignees, parentID); err != nil { + return nil, err + } + // Snapshot values before mutation so we can diff them for the activity log. prevName := issue.Name prevPriority := issue.Priority