From cc3daaf3b4e1a0f00a80b898545c446eec9724c8 Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 10 Jul 2026 13:43:45 +0800 Subject: [PATCH] fix: scope claim-time comment fetch to workspace + guard --attachment paths (MUL-4252) (#5190) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(daemon): scope claim-time comment fetches to the task's workspace (MUL-4252) The daemon claim path embeds the triggering comment and every coalesced comment's full text into the agent prompt, but fetched them with an unscoped `GetComment(id)` — a task row carrying a foreign comment UUID would pull another workspace's comment text into the prompt. On a shared SaaS backend (tens of thousands of workspaces in one DB) that is a tenant boundary hole, latent today only because task rows are server-written. Switch all three claim/reconcile GetComment calls to GetCommentInWorkspace, scoped by the runtime's workspace (claim path) or the issue's workspace (completion reconcile). The task's issue workspace is already asserted equal to the runtime workspace, so same-workspace delivery is unchanged; a foreign UUID now resolves to "missing" and is skipped — matching buildCoalescedCommentData's documented behavior. Adds DB-backed claim tests: same-workspace trigger comment is still delivered; a foreign-workspace comment's content never surfaces. Co-authored-by: multica-agent * fix(cli): extend the workdir guardrail to --attachment paths (MUL-4252) #5167 fenced --description-file/--content-file to the working directory but left --attachment uncovered — the same /tmp stale-file leak in image form: an agent that writes chart.png to a machine-shared path and attaches it could upload another run's (possibly another workspace's) stale file. Apply ensureAttachmentWithinWorkdir to each local --attachment path in `issue create` and `comment add` (URL values are still skipped upstream), reusing #5167's symlink-resolving fileWithinWorkingDir and the existing --allow-external-file escape hatch. Rejection happens before the issue is created, so a bad path never yields a half-created issue. Co-authored-by: multica-agent * fix(service): scope trigger-summary + originator resolution to the task's workspace (MUL-4252) PR review P1: the claim-time full-comment fetch was already scoped, but the trigger_summary snapshot (first ~200 chars) still leaked. On the real enqueue/merge paths a foreign comment UUID flowed through buildCommentTriggerSummary / resolveOriginatorFromTriggerComment, which used an unscoped GetComment; the truncated text was stored on the task row and later returned in the claim / task-history response (handler/agent.go trigger_summary). Thread the issue's workspace through both helpers (and their exported merge-path wrappers) and switch to GetCommentInWorkspace, so a cross-workspace comment resolves to "missing": trigger_summary stays NULL and no foreign originator is inherited. Every caller already has the issue's WorkspaceID in scope (enqueue, mention/leader, deferred fallback, merge, completion reconcile). Rework the claim test to drive the REAL TaskService.EnqueueTaskForIssue path (which snapshots the summary) and assert the stored row's trigger_summary + originator_user_id stay NULL and the claim response carries neither the foreign body nor the foreign summary. Verified the test fails when the summary fetch is left unscoped. Co-authored-by: multica-agent * fix(cli): validate all --attachment paths before uploading any in comment add (MUL-4252) PR review P2: `issue comment add` checked-read-uploaded each attachment in one loop, so a valid workdir attachment followed by an invalid (external / symlink-escaping) one uploaded the first file — orphaning it as an issue-level attachment — then aborted before posting the comment, and a retry duplicated it. Extract the URL-filter + workdir-guard + read step `issue create` already used into a shared collectLocalAttachments helper and have comment add use it: every attachment is validated and read up front, and nothing is uploaded unless all pass. Adds a command-level test asserting a valid-then-external attachment pair aborts with ZERO upload requests and no comment (fails against the old interleaved loop). Co-authored-by: multica-agent --------- Co-authored-by: J Co-authored-by: multica-agent --- server/cmd/multica/cmd_issue.go | 133 ++++++++----- server/cmd/multica/cmd_issue_test.go | 123 ++++++++++++ server/internal/handler/comment.go | 4 +- server/internal/handler/daemon.go | 31 +++- .../daemon_comment_workspace_scope_test.go | 175 ++++++++++++++++++ .../service/resolve_originator_test.go | 19 +- server/internal/service/task.go | 44 +++-- 7 files changed, 449 insertions(+), 80 deletions(-) create mode 100644 server/internal/handler/daemon_comment_workspace_scope_test.go 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,