diff --git a/server/cmd/multica/cmd_issue.go b/server/cmd/multica/cmd_issue.go index bc721d27fc..4d8895996f 100644 --- a/server/cmd/multica/cmd_issue.go +++ b/server/cmd/multica/cmd_issue.go @@ -461,7 +461,7 @@ func init() { issueCreateCmd.Flags().String("description", "", "Issue description (decodes \\n, \\r, \\t, \\\\; pipe via --description-stdin to preserve literal backslashes)") issueCreateCmd.Flags().Bool("description-stdin", false, "Read issue description from stdin (preserves multi-line content verbatim)") issueCreateCmd.Flags().String("description-file", "", "Read issue description from a UTF-8 file (preserves multi-line content verbatim; use this on Windows when stdin piping mangles non-ASCII bytes). The path must be inside the current working directory unless --allow-external-file is set.") - issueCreateCmd.Flags().Bool("allow-external-file", false, "Allow --description-file to read a path outside the current working directory. Off by default so a stale temp file from another run/environment can't be picked up (MUL-4252).") + issueCreateCmd.Flags().Bool("allow-external-file", false, "Allow --description-file / --attachment to read a path outside the current working directory. Off by default so a stale file from another run/environment can't be picked up (MUL-4252).") issueCreateCmd.Flags().String("status", "", "Issue status") issueCreateCmd.Flags().String("priority", "", "Issue priority") issueCreateCmd.Flags().String("assignee", "", "Assignee name (member, agent, or squad; fuzzy match)") @@ -539,7 +539,7 @@ func init() { issueCommentAddCmd.Flags().String("content", "", "Comment content (decodes \\n, \\r, \\t, \\\\; pipe via --content-stdin for multi-line bodies or to preserve literal backslashes)") issueCommentAddCmd.Flags().Bool("content-stdin", false, "Read comment content from stdin (preserves multi-line content verbatim)") issueCommentAddCmd.Flags().String("content-file", "", "Read comment content from a UTF-8 file (preserves multi-line content verbatim; use this on Windows when stdin piping mangles non-ASCII bytes). The path must be inside the current working directory unless --allow-external-file is set.") - issueCommentAddCmd.Flags().Bool("allow-external-file", false, "Allow --content-file to read a path outside the current working directory. Off by default so a stale temp file from another run/environment can't be picked up (MUL-4252).") + issueCommentAddCmd.Flags().Bool("allow-external-file", false, "Allow --content-file / --attachment to read a path outside the current working directory. Off by default so a stale file from another run/environment can't be picked up (MUL-4252).") issueCommentAddCmd.Flags().String("parent", "", "Parent comment ID (reply to a specific comment)") issueCommentAddCmd.Flags().StringSlice("attachment", nil, "File path(s) to attach (can be specified multiple times)") issueCommentAddCmd.Flags().String("output", "json", "Output format: table or json") @@ -960,6 +960,66 @@ func isHTTPURL(path string) bool { return strings.HasPrefix(p, "http://") || strings.HasPrefix(p, "https://") } +// ensureAttachmentWithinWorkdir applies the same workdir containment guard as +// --description-file / --content-file (MUL-4252) to a local --attachment path. +// An agent that writes a chart/report to a machine-shared path like /tmp and +// then attaches it could otherwise pick up another run's — possibly another +// workspace's — stale file (the image version of the /tmp/desc.md leak). URL +// values are filtered by the caller and never reach here. --allow-external-file +// overrides, mirroring the text-flag escape hatch. +func ensureAttachmentWithinWorkdir(cmd *cobra.Command, filePath string) error { + if allow, _ := cmd.Flags().GetBool("allow-external-file"); allow { + return nil + } + within, err := fileWithinWorkingDir(filePath) + if err != nil { + return fmt.Errorf("resolve --attachment path %q: %w", filePath, err) + } + if !within { + return fmt.Errorf( + "--attachment path %q resolves outside the current working directory; "+ + "attach files generated inside the task workdir rather than machine-shared "+ + "paths like /tmp, where another run's stale file can be attached by mistake. "+ + "Pass --allow-external-file to override.", + filePath) + } + return nil +} + +// pendingAttachment is a local --attachment file that passed URL filtering and +// the workdir guard and has been read into memory, ready to upload. +type pendingAttachment struct { + path string + data []byte +} + +// collectLocalAttachments validates and reads ALL local --attachment paths up +// front, before any upload. URL-shaped values are warned and skipped (the API +// only accepts local paths). Each remaining path is run through the MUL-4252 +// workdir guard and read into memory; the first invalid or unreadable path +// returns an error with nothing uploaded. Both `issue create` and +// `comment add` share this so an invalid attachment can never leave an earlier +// one uploaded as an orphaned issue attachment while the issue/comment is never +// created (which would duplicate on retry). +func collectLocalAttachments(cmd *cobra.Command, attachments []string) ([]pendingAttachment, error) { + pending := make([]pendingAttachment, 0, len(attachments)) + for _, filePath := range attachments { + if isHTTPURL(filePath) { + fmt.Fprintf(os.Stderr, "Skipping --attachment %q: URLs are not supported here, only local file paths.\n", filePath) + continue + } + if err := ensureAttachmentWithinWorkdir(cmd, filePath); err != nil { + return nil, err + } + data, readErr := os.ReadFile(filePath) + if readErr != nil { + return nil, fmt.Errorf("read attachment %s: %w", filePath, readErr) + } + pending = append(pending, pendingAttachment{path: filePath, data: data}) + } + return pending, nil +} + func appendUniqueStrings(dst []string, values ...string) []string { seen := make(map[string]struct{}, len(dst)+len(values)) out := make([]string, 0, len(dst)+len(values)) @@ -1095,34 +1155,15 @@ func runIssueCreate(cmd *cobra.Command, _ []string) error { body["attachment_ids"] = attachmentIDs } - // Pre-validate attachments BEFORE creating the issue so a bad path - // can never produce a half-created issue (which would otherwise - // trigger callers — especially the agent doing quick-create — to - // retry the whole `issue create` and end up with duplicates). - // - // - http(s) URLs are not local files; the API only accepts local - // paths here. Warn and skip rather than fail — a markdown image - // URL embedded in the prompt should never be re-attached, and - // skipping is the safest outcome for that case. - // - Anything else is treated as a local path and read upfront. - // A read failure here is a real user/agent mistake (typo, - // missing file) and we surface it pre-create so the issue - // never lands. - type pendingAttachment struct { - path string - data []byte - } - pending := make([]pendingAttachment, 0, len(attachments)) - for _, filePath := range attachments { - if isHTTPURL(filePath) { - fmt.Fprintf(os.Stderr, "Skipping --attachment %q: URLs are not supported here, only local file paths.\n", filePath) - continue - } - data, readErr := os.ReadFile(filePath) - if readErr != nil { - return fmt.Errorf("read attachment %s: %w", filePath, readErr) - } - pending = append(pending, pendingAttachment{path: filePath, data: data}) + // Pre-validate attachments BEFORE creating the issue so a bad path can + // never produce a half-created issue (which would otherwise trigger + // callers — especially the agent doing quick-create — to retry the whole + // `issue create` and end up with duplicates). URLs are warned+skipped, the + // workdir guard is applied, and every local path is read upfront; a failure + // here surfaces pre-create so the issue never lands. + pending, err := collectLocalAttachments(cmd, attachments) + if err != nil { + return err } var result map[string]any @@ -1894,28 +1935,24 @@ func runIssueCommentAdd(cmd *cobra.Command, args []string) error { } issueID := issueRef.ID - // Upload attachments and collect their IDs. URLs are skipped with a - // warning — `--attachment` only accepts local file paths, and a - // markdown image URL embedded in agent-supplied content should never - // be re-uploaded as if it were a file. Unlike `issue create`, this - // path uploads BEFORE posting the comment, so a hard failure on a - // real (local) attachment correctly aborts the whole call. + // Validate and read ALL attachments before uploading any. URLs are skipped + // with a warning — `--attachment` only accepts local file paths. Reading + // everything up front means a later invalid path (external / symlink escape + // caught by the workdir guard) aborts the call with ZERO uploads, instead + // of leaving an earlier file uploaded as an orphaned issue attachment while + // the comment is never posted (which would duplicate on retry — MUL-4252). + pending, err := collectLocalAttachments(cmd, attachments) + if err != nil { + return err + } var attachmentIDs []string - for _, filePath := range attachments { - if isHTTPURL(filePath) { - fmt.Fprintf(os.Stderr, "Skipping --attachment %q: URLs are not supported here, only local file paths.\n", filePath) - continue - } - data, readErr := os.ReadFile(filePath) - if readErr != nil { - return fmt.Errorf("read attachment %s: %w", filePath, readErr) - } - id, uploadErr := client.UploadFile(ctx, data, filePath, issueID) + for _, att := range pending { + id, uploadErr := client.UploadFile(ctx, att.data, att.path, issueID) if uploadErr != nil { - return fmt.Errorf("upload attachment %s: %w", filePath, uploadErr) + return fmt.Errorf("upload attachment %s: %w", att.path, uploadErr) } attachmentIDs = append(attachmentIDs, id) - fmt.Fprintf(os.Stderr, "Uploaded %s\n", filePath) + fmt.Fprintf(os.Stderr, "Uploaded %s\n", att.path) } body := map[string]any{"content": content} diff --git a/server/cmd/multica/cmd_issue_test.go b/server/cmd/multica/cmd_issue_test.go index d3eca2c522..f39889cfc7 100644 --- a/server/cmd/multica/cmd_issue_test.go +++ b/server/cmd/multica/cmd_issue_test.go @@ -299,6 +299,129 @@ func TestResolveTextFlag(t *testing.T) { }) } +// TestEnsureAttachmentWithinWorkdir covers the MUL-4252 guardrail extended to +// --attachment: a local attachment path outside the task workdir is rejected +// (so an agent can't attach another run's stale /tmp file), with the same +// --allow-external-file escape hatch as the text flags. +func TestEnsureAttachmentWithinWorkdir(t *testing.T) { + newCmd := func() *cobra.Command { + c := &cobra.Command{Use: "test"} + c.Flags().Bool("allow-external-file", false, "") + return c + } + + t.Run("attachment inside the working directory is accepted", func(t *testing.T) { + t.Chdir(t.TempDir()) + if err := os.WriteFile("chart.png", []byte("png"), 0o644); err != nil { + t.Fatalf("write tempfile: %v", err) + } + if err := ensureAttachmentWithinWorkdir(newCmd(), "chart.png"); err != nil { + t.Fatalf("expected in-workdir attachment to be accepted, got %v", err) + } + }) + + t.Run("attachment outside the working directory is rejected", func(t *testing.T) { + t.Chdir(t.TempDir()) + outside := t.TempDir() + path := outside + string(os.PathSeparator) + "stale.png" + if err := os.WriteFile(path, []byte("png"), 0o644); err != nil { + t.Fatalf("write tempfile: %v", err) + } + err := ensureAttachmentWithinWorkdir(newCmd(), path) + if err == nil { + t.Fatalf("expected rejection for attachment outside the working directory") + } + if !strings.Contains(err.Error(), "allow-external-file") { + t.Errorf("error should point at --allow-external-file, got %v", err) + } + }) + + t.Run("--allow-external-file permits an attachment outside the working directory", func(t *testing.T) { + t.Chdir(t.TempDir()) + outside := t.TempDir() + path := outside + string(os.PathSeparator) + "external.png" + if err := os.WriteFile(path, []byte("png"), 0o644); err != nil { + t.Fatalf("write tempfile: %v", err) + } + c := newCmd() + _ = c.Flags().Set("allow-external-file", "true") + if err := ensureAttachmentWithinWorkdir(c, path); err != nil { + t.Fatalf("expected override to permit external attachment, got %v", err) + } + }) +} + +func newIssueCommentAddTestCmd() *cobra.Command { + cmd := &cobra.Command{Use: "add"} + cmd.Flags().String("content", "", "") + cmd.Flags().Bool("content-stdin", false, "") + cmd.Flags().String("content-file", "", "") + cmd.Flags().Bool("allow-external-file", false, "") + cmd.Flags().StringSlice("attachment", nil, "") + cmd.Flags().String("parent", "", "") + cmd.Flags().String("output", "json", "") + return cmd +} + +// TestRunIssueCommentAddRejectsExternalAttachmentWithZeroUploads is the MUL-4252 +// P2 guard: `comment add` must validate every --attachment BEFORE uploading any, +// so a valid attachment followed by an invalid (external) one aborts the call +// with ZERO upload requests and no comment — the pre-refactor loop uploaded the +// first file, orphaning it as an issue attachment, then failed on the second. +func TestRunIssueCommentAddRejectsExternalAttachmentWithZeroUploads(t *testing.T) { + const issueID = "11111111-1111-4111-8111-111111111111" + var uploads, comments int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/issues/"+issueID: + _ = json.NewEncoder(w).Encode(map[string]any{"id": issueID, "identifier": "TST-1"}) + case r.Method == http.MethodPost && r.URL.Path == "/api/upload-file": + uploads++ + _ = json.NewEncoder(w).Encode(map[string]any{"id": "att-1"}) + case r.Method == http.MethodPost && r.URL.Path == "/api/issues/"+issueID+"/comments": + comments++ + _ = json.NewEncoder(w).Encode(map[string]any{"id": "comment-1"}) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + setCLITestServerEnv(t, srv.URL) + // mat_ prefix clears the daemon-managed execution-context guard both in CI + // and when the suite runs inside an agent task (leftover daemon marker). + t.Setenv("MULTICA_TOKEN", "mat_test-token") + + // A valid attachment inside the workdir, FOLLOWED BY an external one. + t.Chdir(t.TempDir()) + if err := os.WriteFile("good.png", []byte("good"), 0o644); err != nil { + t.Fatalf("write good attachment: %v", err) + } + external := t.TempDir() + string(os.PathSeparator) + "bad.png" + if err := os.WriteFile(external, []byte("bad"), 0o644); err != nil { + t.Fatalf("write external attachment: %v", err) + } + + cmd := newIssueCommentAddTestCmd() + _ = cmd.Flags().Set("content", "hello") + _ = cmd.Flags().Set("attachment", "good.png") + _ = cmd.Flags().Set("attachment", external) + + err := runIssueCommentAdd(cmd, []string{issueID}) + if err == nil { + t.Fatalf("expected rejection for an external attachment") + } + if !strings.Contains(err.Error(), "allow-external-file") { + t.Errorf("error should point at --allow-external-file, got %v", err) + } + if uploads != 0 { + t.Errorf("expected ZERO upload requests when a later attachment is invalid, got %d", uploads) + } + if comments != 0 { + t.Errorf("expected no comment to be posted, got %d", comments) + } +} + func newIssueCreateTestCmd() *cobra.Command { cmd := &cobra.Command{Use: "create"} cmd.Flags().String("title", "", "") diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index 10534a1980..b9de6290cc 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -1474,14 +1474,14 @@ func (h *Handler) hasActiveTaskForIssueAndAgent(ctx context.Context, issueID, ag // one-pending-per-(issue,agent) unique index) and so silently dropped the // mismatched-originator comment. func (h *Handler) mergeCommentIntoPendingTask(ctx context.Context, issue db.Issue, trigger commentAgentTrigger, newTriggerCommentID pgtype.UUID) bool { - originator := h.TaskService.ResolveOriginatorFromTriggerComment(ctx, newTriggerCommentID) + originator := h.TaskService.ResolveOriginatorFromTriggerComment(ctx, issue.WorkspaceID, newTriggerCommentID) overlay, connectedApps := h.TaskService.BuildRuntimeMCPOverlayForMerge(ctx, originator, trigger.Agent) row, err := h.Queries.MergeCommentIntoPendingTask(ctx, db.MergeCommentIntoPendingTaskParams{ IssueID: issue.ID, AgentID: trigger.Agent.ID, NewTriggerCommentID: newTriggerCommentID, NewOriginatorUserID: originator, - NewTriggerSummary: h.TaskService.BuildCommentTriggerSummary(ctx, newTriggerCommentID), + NewTriggerSummary: h.TaskService.BuildCommentTriggerSummary(ctx, issue.WorkspaceID, newTriggerCommentID), NewRuntimeMcpOverlay: overlay, NewRuntimeConnectedApps: connectedApps, }) diff --git a/server/internal/handler/daemon.go b/server/internal/handler/daemon.go index ccc6501d21..30372b5ee4 100644 --- a/server/internal/handler/daemon.go +++ b/server/internal/handler/daemon.go @@ -1606,7 +1606,7 @@ func (h *Handler) ClaimTaskByRuntime(w http.ResponseWriter, r *http.Request) { // Embed the folded comments' full detail (thread/author/created_at/ // content) so the prompt can address each one without assuming they // share the triggering thread (MUL-4195 review should-fix #3). - resp.CoalescedComments = h.buildCoalescedCommentData(r.Context(), task.CoalescedCommentIds) + resp.CoalescedComments = h.buildCoalescedCommentData(r.Context(), runtime.WorkspaceID, task.CoalescedCommentIds) // Fetch the triggering comment content so the daemon can embed it // directly in the agent prompt (prevents the agent from ignoring comments @@ -1615,7 +1615,14 @@ func (h *Handler) ClaimTaskByRuntime(w http.ResponseWriter, r *http.Request) { // was triggered by a human or by another agent — a signal used by the // harness instructions to avoid mention loops between agents. if task.TriggerCommentID.Valid { - if comment, err := h.Queries.GetComment(r.Context(), task.TriggerCommentID); err == nil { + // Scope by the runtime's workspace so a task row carrying a foreign + // comment UUID can never pull another workspace's comment text into + // this agent's prompt. The task's issue workspace is asserted equal + // to runtime.WorkspaceID below, so this is the right tenant (MUL-4252). + if comment, err := h.Queries.GetCommentInWorkspace(r.Context(), db.GetCommentInWorkspaceParams{ + ID: task.TriggerCommentID, + WorkspaceID: runtime.WorkspaceID, + }); err == nil { resp.TriggerCommentContent = comment.Content resp.TriggerThreadID = uuidToString(comment.ID) if comment.ParentID.Valid { @@ -2523,7 +2530,13 @@ func (h *Handler) reconcileCommentsOnCompletion(ctx context.Context, task *db.Ag } var parentComment *db.Comment if c.ParentID.Valid { - if parent, err := h.Queries.GetComment(ctx, c.ParentID); err == nil { + // Scope to the issue's workspace; a comment's parent is always in the + // same workspace, so this only fails closed against a stray foreign + // UUID rather than changing behavior (MUL-4252). + if parent, err := h.Queries.GetCommentInWorkspace(ctx, db.GetCommentInWorkspaceParams{ + ID: c.ParentID, + WorkspaceID: issue.WorkspaceID, + }); err == nil { parentComment = &parent } } @@ -2540,7 +2553,7 @@ func (h *Handler) reconcileCommentsOnCompletion(ctx context.Context, task *db.Ag actorID := uuidToString(c.AuthorID) originatorUserID := actorID if actorType != "member" { - originatorUserID = uuidToString(h.TaskService.ResolveOriginatorFromTriggerComment(ctx, c.ID)) + originatorUserID = uuidToString(h.TaskService.ResolveOriginatorFromTriggerComment(ctx, issue.WorkspaceID, c.ID)) } triggers := h.computeCommentAgentTriggers(ctx, issue, c.Content, parentComment, actorType, actorID, commentTriggerComputeOptions{ ExcludeTriggerCommentID: c.ID, @@ -2612,7 +2625,7 @@ func keepExplicitMentionTriggers(triggers []commentAgentTrigger) []commentAgentT // comment's own id). Missing comments (deleted / wrong workspace) are skipped // rather than failing the claim. The set is bounded by how many comments a user // fires before a run starts, so the per-comment lookups stay cheap. -func (h *Handler) buildCoalescedCommentData(ctx context.Context, ids []pgtype.UUID) []CoalescedCommentData { +func (h *Handler) buildCoalescedCommentData(ctx context.Context, workspaceID pgtype.UUID, ids []pgtype.UUID) []CoalescedCommentData { if len(ids) == 0 { return nil } @@ -2621,7 +2634,13 @@ func (h *Handler) buildCoalescedCommentData(ctx context.Context, ids []pgtype.UU if !id.Valid { continue } - comment, err := h.Queries.GetComment(ctx, id) + // Workspace-scoped so a foreign comment UUID resolves to "missing" + // (skipped) instead of leaking another tenant's text into the prompt + // (MUL-4252). Matches this function's documented skip-on-missing rule. + comment, err := h.Queries.GetCommentInWorkspace(ctx, db.GetCommentInWorkspaceParams{ + ID: id, + WorkspaceID: workspaceID, + }) if err != nil { continue } diff --git a/server/internal/handler/daemon_comment_workspace_scope_test.go b/server/internal/handler/daemon_comment_workspace_scope_test.go new file mode 100644 index 0000000000..b92e0187f2 --- /dev/null +++ b/server/internal/handler/daemon_comment_workspace_scope_test.go @@ -0,0 +1,175 @@ +package handler + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + db "github.com/multica-ai/multica/server/pkg/db/generated" +) + +// insertCommentForScopeTest inserts a member comment on issueID in the given +// workspace and returns its id. +func insertCommentForScopeTest(t *testing.T, ctx context.Context, issueID, workspaceID, content string) string { + t.Helper() + var id string + if err := testPool.QueryRow(ctx, ` + INSERT INTO comment (issue_id, workspace_id, author_type, author_id, content, type) + VALUES ($1, $2, 'member', $3, $4, 'comment') + RETURNING id + `, issueID, workspaceID, testUserID, content).Scan(&id); err != nil { + t.Fatalf("insert comment: %v", err) + } + t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM comment WHERE id = $1`, id) }) + return id +} + +// enqueueIssueTaskWithTrigger enqueues a real task for (issueID, agentID) via the +// production TaskService.EnqueueTaskForIssue path — the path that snapshots +// trigger_summary and resolves the originator from the triggering comment — so +// the MUL-4252 workspace scoping is exercised end-to-end rather than bypassed +// with a hand-written row. Returns the created task id. +func enqueueIssueTaskWithTrigger(t *testing.T, ctx context.Context, agentID, issueID, triggerCommentID string) string { + t.Helper() + task, err := testHandler.TaskService.EnqueueTaskForIssue(ctx, db.Issue{ + ID: parseUUID(issueID), + WorkspaceID: parseUUID(testWorkspaceID), + AssigneeType: pgtype.Text{String: "agent", Valid: true}, + AssigneeID: parseUUID(agentID), + CreatorType: "member", + CreatorID: parseUUID(testUserID), + Priority: "none", + }, parseUUID(triggerCommentID)) + if err != nil { + t.Fatalf("EnqueueTaskForIssue: %v", err) + } + taskID := uuidToString(task.ID) + t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM agent_task_queue WHERE id = $1`, taskID) }) + return taskID +} + +// claimTriggerFieldsForTest claims the next task for runtimeID and returns the +// triggering comment content + summary the claim response carried (both empty +// when absent — omitempty on the wire) plus the raw response body. +func claimTriggerFieldsForTest(t *testing.T, runtimeID string) (taskID, triggerContent, triggerSummary, raw string) { + t.Helper() + w := httptest.NewRecorder() + req := newDaemonTokenRequest("POST", "/api/daemon/runtimes/"+runtimeID+"/tasks/claim", nil, + testWorkspaceID, "comment-workspace-scope") + req = withURLParam(req, "runtimeId", runtimeID) + testHandler.ClaimTaskByRuntime(w, req) + if w.Code != http.StatusOK { + t.Fatalf("ClaimTaskByRuntime: expected 200, got %d: %s", w.Code, w.Body.String()) + } + var resp struct { + Task *struct { + ID string `json:"id"` + TriggerCommentContent string `json:"trigger_comment_content"` + TriggerSummary string `json:"trigger_summary"` + } `json:"task"` + } + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode claim response: %v", err) + } + if resp.Task == nil { + return "", "", "", w.Body.String() + } + return resp.Task.ID, resp.Task.TriggerCommentContent, resp.Task.TriggerSummary, w.Body.String() +} + +// TestClaimDeliversSameWorkspaceTriggerComment is the positive path: a triggering +// comment in the task's own workspace must still be snapshotted into +// trigger_summary at enqueue and embedded (content + summary) in the claim +// response. The MUL-4252 workspace-scoped lookups must not regress delivery. +func TestClaimDeliversSameWorkspaceTriggerComment(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + ctx := context.Background() + runtimeID := createClaimReclaimRuntime(t, ctx, "same-ws trigger runtime") + agentID, issueID := createClaimReclaimAgentAndIssue(t, ctx, runtimeID, "same-ws trigger agent") + + const body = "same-workspace trigger body ABC123" + commentID := insertCommentForScopeTest(t, ctx, issueID, testWorkspaceID, body) + + want := enqueueIssueTaskWithTrigger(t, ctx, agentID, issueID, commentID) + got, content, summary, raw := claimTriggerFieldsForTest(t, runtimeID) + if got != want { + t.Fatalf("claimed task id = %q, want %q: %s", got, want, raw) + } + if content != body { + t.Fatalf("trigger_comment_content = %q, want same-workspace body %q", content, body) + } + if summary != body { + t.Fatalf("trigger_summary = %q, want same-workspace body %q", summary, body) + } +} + +// TestClaimDoesNotLeakForeignWorkspaceTriggerCommentOrSummary is the MUL-4252 +// guard, exercised through the real enqueue + claim path. A task whose +// trigger_comment_id points at a comment owned by a DIFFERENT workspace must +// leak neither the full body (claim-time fetch) nor the truncated summary +// (enqueue-time snapshot), and must not inherit an originator from it. +func TestClaimDoesNotLeakForeignWorkspaceTriggerCommentOrSummary(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + ctx := context.Background() + runtimeID := createClaimReclaimRuntime(t, ctx, "foreign trigger runtime") + agentID, issueID := createClaimReclaimAgentAndIssue(t, ctx, runtimeID, "foreign trigger agent") + + // A comment that lives entirely in a DIFFERENT workspace (its own issue). + otherWS := createOtherTestWorkspace(t) + var foreignIssueID string + if err := testPool.QueryRow(ctx, ` + INSERT INTO issue (workspace_id, title, status, priority, creator_id, creator_type, number, position) + VALUES ($1, 'foreign issue', 'in_progress', 'none', $2, 'member', + (SELECT COALESCE(MAX(number), 90000) + 1 FROM issue WHERE workspace_id = $1), 0) + RETURNING id + `, otherWS, testUserID).Scan(&foreignIssueID); err != nil { + t.Fatalf("insert foreign-workspace issue: %v", err) + } + t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, foreignIssueID) }) + + const secret = "FOREIGN-WORKSPACE-SECRET-DO-NOT-LEAK" + foreignCommentID := insertCommentForScopeTest(t, ctx, foreignIssueID, otherWS, secret) + + // Enqueue through the production path: this is where trigger_summary is + // snapshotted and the originator resolved. Both must fail closed. + taskID := enqueueIssueTaskWithTrigger(t, ctx, agentID, issueID, foreignCommentID) + + // Stored row: neither the summary nor the originator may come from the + // foreign comment. + var storedSummary pgtype.Text + var storedOriginator pgtype.UUID + if err := testPool.QueryRow(ctx, + `SELECT trigger_summary, originator_user_id FROM agent_task_queue WHERE id = $1`, taskID, + ).Scan(&storedSummary, &storedOriginator); err != nil { + t.Fatalf("read stored task: %v", err) + } + if storedSummary.Valid { + t.Fatalf("stored trigger_summary must be NULL for a foreign-workspace comment, got %q", storedSummary.String) + } + if storedOriginator.Valid { + t.Fatalf("stored originator_user_id must be NULL for a foreign-workspace comment, got %s", uuidToString(storedOriginator)) + } + + // Wire: the claim response must carry neither the body nor the summary. + got, content, summary, raw := claimTriggerFieldsForTest(t, runtimeID) + if got != taskID { + t.Fatalf("claimed task id = %q, want %q: %s", got, taskID, raw) + } + if content != "" { + t.Fatalf("trigger_comment_content must be empty for a foreign-workspace comment, got %q", content) + } + if summary != "" { + t.Fatalf("trigger_summary must be empty for a foreign-workspace comment, got %q", summary) + } + if strings.Contains(raw, secret) { + t.Fatalf("claim response leaked foreign-workspace comment text:\n%s", raw) + } +} diff --git a/server/internal/service/resolve_originator_test.go b/server/internal/service/resolve_originator_test.go index 3e71f43329..28e9d87415 100644 --- a/server/internal/service/resolve_originator_test.go +++ b/server/internal/service/resolve_originator_test.go @@ -112,7 +112,7 @@ func TestBuildRuntimeMCPOverlaySkipsBuilderWhenComposioFlagDisabled(t *testing.T // source_task_id=T_A), T_A's task id, and U as pgtype.UUID. T_A's // originator_user_id is U so the fanout / quick-create branches can prove the // inheritance. -func seedOriginatorFanout(t *testing.T, pool *pgxpool.Pool) (memberCommentID, agentCommentID, taskAID, userID pgtype.UUID) { +func seedOriginatorFanout(t *testing.T, pool *pgxpool.Pool) (memberCommentID, agentCommentID, taskAID, userID, workspaceUUID pgtype.UUID) { t.Helper() ctx := context.Background() @@ -227,6 +227,7 @@ func seedOriginatorFanout(t *testing.T, pool *pgxpool.Pool) (memberCommentID, ag agentCommentID = util.MustParseUUID(commentAgentID) taskAID = util.MustParseUUID(taskAIDStr) userID = util.MustParseUUID(userIDStr) + workspaceUUID = util.MustParseUUID(workspaceID) return } @@ -235,10 +236,10 @@ func seedOriginatorFanout(t *testing.T, pool *pgxpool.Pool) (memberCommentID, ag // originator is the comment's own author_id. func TestResolveOriginatorFromTriggerComment_MemberAuthored(t *testing.T) { pool := newResolveOriginatorPool(t) - memberCommentID, _, _, userID := seedOriginatorFanout(t, pool) + memberCommentID, _, _, userID, workspaceID := seedOriginatorFanout(t, pool) svc := &TaskService{Queries: db.New(pool)} - got := svc.resolveOriginatorFromTriggerComment(context.Background(), memberCommentID) + got := svc.resolveOriginatorFromTriggerComment(context.Background(), workspaceID, memberCommentID) if !got.Valid { t.Fatalf("expected valid originator for member-authored comment, got invalid") } @@ -255,10 +256,10 @@ func TestResolveOriginatorFromTriggerComment_MemberAuthored(t *testing.T) { // parent.originator_user_id, yielding U. func TestResolveOriginatorFromTriggerComment_AgentAuthoredInheritsFromParent(t *testing.T) { pool := newResolveOriginatorPool(t) - _, agentCommentID, _, userID := seedOriginatorFanout(t, pool) + _, agentCommentID, _, userID, workspaceID := seedOriginatorFanout(t, pool) svc := &TaskService{Queries: db.New(pool)} - got := svc.resolveOriginatorFromTriggerComment(context.Background(), agentCommentID) + got := svc.resolveOriginatorFromTriggerComment(context.Background(), workspaceID, agentCommentID) if !got.Valid { t.Fatalf("expected valid originator inherited from parent task, got invalid") } @@ -291,7 +292,7 @@ func TestResolveOriginatorForIssueTask_MemberCreatedNoComment(t *testing.T) { // parent quick-create task and must be inherited for downstream dispatch. func TestResolveOriginatorForIssueTask_QuickCreateIssueInheritsParentTask(t *testing.T) { pool := newResolveOriginatorPool(t) - _, _, parentTaskID, userID := seedOriginatorFanout(t, pool) + _, _, parentTaskID, userID, _ := seedOriginatorFanout(t, pool) svc := &TaskService{Queries: db.New(pool)} issue := db.Issue{ CreatorType: "agent", @@ -316,7 +317,7 @@ func TestResolveOriginatorForIssueTask_QuickCreateIssueInheritsParentTask(t *tes // squad-leader runs (and the A2A mentions they emit) keep the originator. func TestResolveOriginatorForIssueTask_AgentCreateIssueInheritsParentTask(t *testing.T) { pool := newResolveOriginatorPool(t) - _, _, parentTaskID, userID := seedOriginatorFanout(t, pool) + _, _, parentTaskID, userID, _ := seedOriginatorFanout(t, pool) svc := &TaskService{Queries: db.New(pool)} issue := db.Issue{ CreatorType: "agent", @@ -341,7 +342,7 @@ func TestResolveOriginatorForIssueTask_AgentCreateIssueInheritsParentTask(t *tes // by a gate that computed a different (empty) originator. func TestOriginatorForIssueTask_MatchesResolverForAgentCreate(t *testing.T) { pool := newResolveOriginatorPool(t) - _, _, parentTaskID, userID := seedOriginatorFanout(t, pool) + _, _, parentTaskID, userID, _ := seedOriginatorFanout(t, pool) svc := &TaskService{Queries: db.New(pool)} issue := db.Issue{ CreatorType: "agent", @@ -489,7 +490,7 @@ func TestEnqueueTaskForIssueStoresRuntimeMCPOverlayInQueuedRow(t *testing.T) { func TestResolveOriginatorFromTriggerComment_InvalidCommentID(t *testing.T) { pool := newResolveOriginatorPool(t) svc := &TaskService{Queries: db.New(pool)} - got := svc.resolveOriginatorFromTriggerComment(context.Background(), pgtype.UUID{}) + got := svc.resolveOriginatorFromTriggerComment(context.Background(), pgtype.UUID{}, pgtype.UUID{}) if got.Valid { t.Errorf("invalid comment id must yield invalid originator, got %s", util.UUIDToString(got)) } diff --git a/server/internal/service/task.go b/server/internal/service/task.go index 89ec59cf42..fa99b20dc0 100644 --- a/server/internal/service/task.go +++ b/server/internal/service/task.go @@ -129,11 +129,19 @@ const ( // it for storage on the task row. Returns an invalid pgtype.Text when // the comment is missing (deleted / wrong workspace / etc) so the column // stays NULL — front-end falls back to a structural label in that case. -func (s *TaskService) buildCommentTriggerSummary(ctx context.Context, commentID pgtype.UUID) pgtype.Text { +// +// workspaceID scopes the fetch to the task's own workspace: the summary is +// later returned in claim / task-history responses, so a foreign comment UUID +// reaching an enqueue/merge path must NOT leak another workspace's text even in +// truncated form (MUL-4252). +func (s *TaskService) buildCommentTriggerSummary(ctx context.Context, workspaceID, commentID pgtype.UUID) pgtype.Text { if !commentID.Valid { return pgtype.Text{} } - comment, err := s.Queries.GetComment(ctx, commentID) + comment, err := s.Queries.GetCommentInWorkspace(ctx, db.GetCommentInWorkspaceParams{ + ID: commentID, + WorkspaceID: workspaceID, + }) if err != nil { return pgtype.Text{} } @@ -147,16 +155,17 @@ func (s *TaskService) buildCommentTriggerSummary(ctx context.Context, commentID // ResolveOriginatorFromTriggerComment is the exported wrapper used by the // comment-merge path (MUL-4195) to compute the top-of-chain human originator // for a newly-arrived comment, so a merge can be gated on the originator being -// unchanged. See resolveOriginatorFromTriggerComment for the chain rules. -func (s *TaskService) ResolveOriginatorFromTriggerComment(ctx context.Context, commentID pgtype.UUID) pgtype.UUID { - return s.resolveOriginatorFromTriggerComment(ctx, commentID) +// unchanged. workspaceID scopes the comment lookup to the task's workspace +// (MUL-4252). See resolveOriginatorFromTriggerComment for the chain rules. +func (s *TaskService) ResolveOriginatorFromTriggerComment(ctx context.Context, workspaceID, commentID pgtype.UUID) pgtype.UUID { + return s.resolveOriginatorFromTriggerComment(ctx, workspaceID, commentID) } // BuildCommentTriggerSummary is the exported wrapper used by the comment-merge // path (MUL-4195) to refresh a coalesced task's trigger_summary to the newest -// trigger comment's snapshot. -func (s *TaskService) BuildCommentTriggerSummary(ctx context.Context, commentID pgtype.UUID) pgtype.Text { - return s.buildCommentTriggerSummary(ctx, commentID) +// trigger comment's snapshot. workspaceID scopes the lookup (MUL-4252). +func (s *TaskService) BuildCommentTriggerSummary(ctx context.Context, workspaceID, commentID pgtype.UUID) pgtype.Text { + return s.buildCommentTriggerSummary(ctx, workspaceID, commentID) } // BuildRuntimeMCPOverlayForMerge recomputes the Composio MCP overlay + @@ -270,15 +279,20 @@ func (s *TaskService) buildRuntimeMCPOverlay(ctx context.Context, originatorUser // (gate 1). // // A nil receiver / nil Queries falls through to invalid so unit-test -// setups that don't wire a DB stay safe. -func (s *TaskService) resolveOriginatorFromTriggerComment(ctx context.Context, commentID pgtype.UUID) pgtype.UUID { +// setups that don't wire a DB stay safe. workspaceID scopes the comment lookup +// to the task's workspace so a foreign comment UUID cannot resolve an +// originator from another tenant (MUL-4252). +func (s *TaskService) resolveOriginatorFromTriggerComment(ctx context.Context, workspaceID, commentID pgtype.UUID) pgtype.UUID { if s == nil || s.Queries == nil { return pgtype.UUID{} } if !commentID.Valid { return pgtype.UUID{} } - comment, err := s.Queries.GetComment(ctx, commentID) + comment, err := s.Queries.GetCommentInWorkspace(ctx, db.GetCommentInWorkspaceParams{ + ID: commentID, + WorkspaceID: workspaceID, + }) if err != nil { return pgtype.UUID{} } @@ -313,7 +327,7 @@ func (s *TaskService) resolveOriginatorFromTriggerComment(ctx context.Context, c // agent/system origins, including autopilot, deliberately remain unattributed. func (s *TaskService) resolveOriginatorForIssueTask(ctx context.Context, issue db.Issue, triggerCommentID pgtype.UUID) pgtype.UUID { if triggerCommentID.Valid { - return s.resolveOriginatorFromTriggerComment(ctx, triggerCommentID) + return s.resolveOriginatorFromTriggerComment(ctx, issue.WorkspaceID, triggerCommentID) } if issue.CreatorType == "member" && issue.CreatorID.Valid { return issue.CreatorID @@ -723,7 +737,7 @@ func (s *TaskService) enqueueIssueTask(ctx context.Context, issue db.Issue, trig IssueID: issue.ID, Priority: priorityToInt(issue.Priority), TriggerCommentID: triggerCommentID, - TriggerSummary: s.buildCommentTriggerSummary(ctx, triggerCommentID), + TriggerSummary: s.buildCommentTriggerSummary(ctx, issue.WorkspaceID, triggerCommentID), ForceFreshSession: pgtype.Bool{Bool: forceFreshSession, Valid: forceFreshSession}, HandoffNote: pgtype.Text{String: handoffNote, Valid: handoffNote != ""}, OriginatorUserID: originatorUserID, @@ -813,7 +827,7 @@ func (s *TaskService) enqueueMentionTask(ctx context.Context, issue db.Issue, ag IssueID: issue.ID, Priority: priorityToInt(issue.Priority), TriggerCommentID: triggerCommentID, - TriggerSummary: s.buildCommentTriggerSummary(ctx, triggerCommentID), + TriggerSummary: s.buildCommentTriggerSummary(ctx, issue.WorkspaceID, triggerCommentID), IsLeaderTask: pgtype.Bool{Bool: isLeader, Valid: isLeader}, ForceFreshSession: pgtype.Bool{Bool: forceFreshSession, Valid: forceFreshSession}, HandoffNote: pgtype.Text{String: handoffNote, Valid: handoffNote != ""}, @@ -861,7 +875,7 @@ func (s *TaskService) EnqueueDeferredAssigneeFallback(ctx context.Context, issue IssueID: issue.ID, Priority: priorityToInt(issue.Priority), TriggerCommentID: triggerCommentID, - TriggerSummary: s.buildCommentTriggerSummary(ctx, triggerCommentID), + TriggerSummary: s.buildCommentTriggerSummary(ctx, issue.WorkspaceID, triggerCommentID), IsLeaderTask: pgtype.Bool{Bool: isLeader, Valid: isLeader}, SquadID: squadID, EscalationForTaskID: escalationForTaskID,