From 6d03428682d9c0bc75f620e74c7075b6d9d0d12e Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 18 Sep 2026 20:14:03 -0700 Subject: [PATCH] Skip task-only connector reads for agent chat views (#13654) Agent chat views reuse the task surface with synthetic chat-prefixed IDs. Skip their task-only email and external chat-binding queries, and reject invalid UUIDs after existing authentication checks on both read routes. Preserve normal task reads and company isolation. Verified failing regressions before the fix, all 6,377 UI tests, route and OpenAPI regressions, server/UI TypeScript checks, all Linux PR CI gates, and Greptile 5/5 with no unresolved comments. Co-Authored-By: Paperclip --- .../task-connector-read-routes.test.ts | 94 ++++++++++++++++++ server/src/routes/chat-channels.ts | 6 +- server/src/routes/email.ts | 11 ++- server/src/routes/openapi.ts | 3 +- ui/src/components/EmailTaskActivity.tsx | 3 + .../chat/ExternallyConnectedTaskBanner.tsx | 7 +- .../task-only-connector-queries.test.tsx | 98 +++++++++++++++++++ 7 files changed, 214 insertions(+), 8 deletions(-) create mode 100644 server/src/__tests__/task-connector-read-routes.test.ts create mode 100644 ui/src/components/task-only-connector-queries.test.tsx diff --git a/server/src/__tests__/task-connector-read-routes.test.ts b/server/src/__tests__/task-connector-read-routes.test.ts new file mode 100644 index 0000000000..f903e4693c --- /dev/null +++ b/server/src/__tests__/task-connector-read-routes.test.ts @@ -0,0 +1,94 @@ +import express from "express"; +import request from "supertest"; +import type { Db } from "@paperclipai/db"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { errorHandler } from "../middleware/error-handler.js"; +import { chatChannelRoutes } from "../routes/chat-channels.js"; +import { emailRoutes } from "../routes/email.js"; +import type { ChatChannelService } from "../services/chat-channels.js"; +import type { EmailChannelService } from "../services/email-channels.js"; + +const companyId = "11111111-1111-4111-8111-111111111111"; +const issueId = "22222222-2222-4222-8222-222222222222"; +const boardActor: Express.Request["actor"] = { + type: "board", + source: "session", + userId: "test-board-user", + companyIds: [companyId], +}; +const chat = { getIssueBinding: vi.fn(), get: vi.fn() }; +const email = { authorizeRead: vi.fn(), thread: vi.fn() }; + +function app(actor = boardActor) { + const instance = express(); + instance.use((req, _res, next) => { + req.actor = actor; + next(); + }); + instance.use("/api", chatChannelRoutes({} as Db, { + service: chat as unknown as ChatChannelService, + heartbeat: { wakeup: vi.fn() }, + })); + instance.use("/api", emailRoutes({} as Db, email as unknown as EmailChannelService)); + instance.use(errorHandler); + return instance; +} + +beforeEach(() => { + vi.resetAllMocks(); + chat.getIssueBinding.mockResolvedValue(null); + email.authorizeRead.mockResolvedValue(undefined); + email.thread.mockResolvedValue(null); +}); + +describe("task connector read routes", () => { + it.each([`chat:${issueId}`, "undefined", "not-a-uuid", ` ${issueId} `])( + "rejects invalid task ID %s before reading either connector", + async (invalidId) => { + const server = app(); + const encodedId = encodeURIComponent(invalidId); + await request(server).get(`/api/issues/${encodedId}/chat-binding`).expect(400); + await request(server).get(`/api/companies/${companyId}/email/tasks/${encodedId}`).expect(400); + expect(chat.getIssueBinding).not.toHaveBeenCalled(); + expect(email.authorizeRead).not.toHaveBeenCalled(); + expect(email.thread).not.toHaveBeenCalled(); + }, + ); + + it("retains normal task reads and email authorization", async () => { + const server = app(); + await request(server).get(`/api/issues/${issueId}/chat-binding`).expect(200, "null"); + await request(server).get(`/api/companies/${companyId}/email/tasks/${issueId}`).expect(200, "null"); + expect(chat.getIssueBinding).toHaveBeenCalledWith(issueId); + expect(email.authorizeRead).toHaveBeenCalledWith(companyId, issueId, { + userId: boardActor.userId, + localImplicit: false, + }); + expect(email.thread).toHaveBeenCalledWith(companyId, issueId); + }); + + it.each([issueId, `chat:${issueId}`])("retains authentication checks for %s", async (id) => { + const server = app({ type: "none", source: "none" }); + await request(server).get(`/api/issues/${id}/chat-binding`).expect(403); + await request(server).get(`/api/companies/${companyId}/email/tasks/${id}`).expect(401); + expect(chat.getIssueBinding).not.toHaveBeenCalled(); + expect(email.authorizeRead).not.toHaveBeenCalled(); + expect(email.thread).not.toHaveBeenCalled(); + }); + + it("retains company isolation for task bindings", async () => { + const binding = { endpointId: "test-endpoint" }; + chat.getIssueBinding.mockResolvedValue(binding); + chat.get.mockResolvedValue({ companyId }); + await request(app()).get(`/api/issues/${issueId}/chat-binding`).expect(200, binding); + chat.get.mockResolvedValue({ companyId: "other-company" }); + await request(app()).get(`/api/issues/${issueId}/chat-binding`).expect(404); + }); + + it.each([issueId, `chat:${issueId}`])("retains company isolation for email task %s", async (id) => { + await request(app({ ...boardActor, companyIds: [] })) + .get(`/api/companies/${companyId}/email/tasks/${id}`).expect(403); + expect(email.authorizeRead).not.toHaveBeenCalled(); + expect(email.thread).not.toHaveBeenCalled(); + }); +}); diff --git a/server/src/routes/chat-channels.ts b/server/src/routes/chat-channels.ts index 260caa4c56..8883be250e 100644 --- a/server/src/routes/chat-channels.ts +++ b/server/src/routes/chat-channels.ts @@ -436,7 +436,11 @@ export function chatChannelRoutes(db: Db, options: ChatChannelRouteOptions) { router.get("/issues/:issueId/chat-binding", async (req, res) => { assertBoard(req); - const binding = await service.getIssueBinding(req.params.issueId as string); + const issueId = req.params.issueId as string; + if (issueId !== issueId.trim() || !isUuidLike(issueId)) { + throw badRequest("Task ID must be a UUID"); + } + const binding = await service.getIssueBinding(issueId); if (binding) { const endpoint = await getAccessibleResource( req, diff --git a/server/src/routes/email.ts b/server/src/routes/email.ts index efbe970f43..937f3bc07e 100644 --- a/server/src/routes/email.ts +++ b/server/src/routes/email.ts @@ -4,13 +4,14 @@ import { emailConnectionSchema, emailEndpointSetupSchema, emailSendSchema, + isUuidLike, } from "@paperclipai/shared"; import type { Db } from "@paperclipai/db"; import { validate } from "../middleware/validate.js"; import { assertBoard, assertCompanyAccess, hasCompanyAccess } from "./authz.js"; import { emailConnectionService } from "../services/email-connections.js"; import { accessService } from "../services/access.js"; -import { forbidden, notFound } from "../errors.js"; +import { badRequest, forbidden, notFound } from "../errors.js"; import type { EmailChannelService, EmailActor, @@ -188,14 +189,18 @@ export function emailRoutes(db: Db, service: EmailChannelService) { router.get("/companies/:companyId/email/tasks/:issueId", async (req, res) => { const companyId = req.params.companyId as string; assertCompanyAccess(req, companyId); + const issueId = req.params.issueId as string; + if (issueId !== issueId.trim() || !isUuidLike(issueId)) { + throw badRequest("Task ID must be a UUID"); + } await service.authorizeRead( companyId, - req.params.issueId as string, + issueId, actor(req), ); const thread = await service.thread( companyId, - req.params.issueId as string, + issueId, ); if ( thread && diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 2b1a21d03a..5eac1bc5e3 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -2561,10 +2561,11 @@ registry.registerPath({ tags: ["chat-channels", "issues"], summary: "Get a task's external chat binding", description: - "Returns the task's current external conversation binding, or `null` when it has none. A binding in another company is reported as not found.", + "Returns the task's current external conversation binding, or `null` when it has none. Requires a task UUID; synthetic agent-chat view IDs are invalid. A binding in another company is reported as not found.", request: { params: z.object({ issueId: z.string().uuid() }) }, responses: { 200: r.ok(externalChannelBindingResponseSchema.nullable()), + 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden, 404: r.notFound, diff --git a/ui/src/components/EmailTaskActivity.tsx b/ui/src/components/EmailTaskActivity.tsx index cef0c38a90..428a953085 100644 --- a/ui/src/components/EmailTaskActivity.tsx +++ b/ui/src/components/EmailTaskActivity.tsx @@ -17,11 +17,14 @@ export function EmailTaskActivity({ }) { const cache = useQueryClient(); const threadKey = ["email-thread", companyId, issueId]; + const queryEnabled = Boolean(companyId && issueId) && !issueId.startsWith("chat:"); const thread = useQuery({ queryKey: threadKey, queryFn: () => emailApi.thread(companyId, issueId), + enabled: queryEnabled, refetchInterval: 3000, }); + if (!queryEnabled) return null; const data = thread.data; const messages = data?.messages.filter((m) => !m.commentId) ?? []; const publications = data?.publications.filter( diff --git a/ui/src/components/chat/ExternallyConnectedTaskBanner.tsx b/ui/src/components/chat/ExternallyConnectedTaskBanner.tsx index 9a5eee6b8d..67b2fe2218 100644 --- a/ui/src/components/chat/ExternallyConnectedTaskBanner.tsx +++ b/ui/src/components/chat/ExternallyConnectedTaskBanner.tsx @@ -110,14 +110,15 @@ const filePhaseLabels: Record = { export function useIssueChatBinding(companyId: string, issueId: string) { const { enabled } = useChatConnectorsEnabled(); + const queryEnabled = enabled && Boolean(companyId && issueId) && !issueId.startsWith("chat:"); const query = useQuery({ queryKey: ["issue-chat-binding", companyId, issueId], queryFn: () => chatEndpointsApi.getIssueBinding(issueId), - enabled: enabled && Boolean(companyId && issueId), + enabled: queryEnabled, }); return { - binding: enabled ? (query.data ?? null) : null, - isLoading: enabled && query.isLoading, + binding: queryEnabled ? (query.data ?? null) : null, + isLoading: queryEnabled && query.isLoading, }; } diff --git a/ui/src/components/task-only-connector-queries.test.tsx b/ui/src/components/task-only-connector-queries.test.tsx new file mode 100644 index 0000000000..ec9a021f6c --- /dev/null +++ b/ui/src/components/task-only-connector-queries.test.tsx @@ -0,0 +1,98 @@ +// @vitest-environment jsdom + +import { flushSync } from "react-dom"; +import { createRoot, type Root } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { EmailTaskActivity } from "./EmailTaskActivity"; +import { useIssueChatBinding } from "./chat/ExternallyConnectedTaskBanner"; + +const api = vi.hoisted(() => ({ getIssueBinding: vi.fn(), thread: vi.fn() })); +vi.mock("@/api/chatEndpoints", () => ({ chatEndpointsApi: api })); +vi.mock("@/api/email", () => ({ emailApi: api })); +vi.mock("@/hooks/useChatConnectorsEnabled", () => ({ + useChatConnectorsEnabled: () => ({ enabled: true, loaded: true }), +})); + +const companyId = "test-company"; +const taskId = "22222222-2222-4222-8222-222222222222"; +const chatId = `chat:${taskId}`; + +function ConnectorQueries({ issueId }: { issueId: string }) { + const { binding, isLoading } = useIssueChatBinding(companyId, issueId); + return <> + {binding ? "bound" : isLoading ? "loading" : "unbound"} + + ; +} + +describe("task-only connector queries", () => { + let container: HTMLDivElement; + let root: Root; + let client: QueryClient; + + async function render(issueId: string) { + flushSync(() => root.render( + + + , + )); + for (let i = 0; i < 5; i += 1) { + await new Promise((resolve) => window.setTimeout(resolve, 0)); + } + flushSync(() => {}); + } + + beforeEach(() => { + vi.resetAllMocks(); + api.getIssueBinding.mockResolvedValue({ endpointId: "test-endpoint" }); + api.thread.mockResolvedValue(null); + client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + flushSync(() => root.unmount()); + client.clear(); + container.remove(); + }); + + it.each([chatId, ""])("does not request task connectors for %s", async (id) => { + await render(id); + expect(api.getIssueBinding).not.toHaveBeenCalled(); + expect(api.thread).not.toHaveBeenCalled(); + expect(container.textContent).toBe("unbound"); + }); + + it("resumes task queries when navigating from an agent chat to a task", async () => { + await render(taskId); + expect(api.getIssueBinding).toHaveBeenCalledWith(taskId); + expect(api.thread).toHaveBeenCalledWith(companyId, taskId); + expect(container.textContent).toBe("bound"); + + await render(chatId); + expect(api.getIssueBinding).toHaveBeenCalledTimes(1); + expect(api.thread).toHaveBeenCalledTimes(1); + expect(container.textContent).toBe("unbound"); + + const nextTaskId = "33333333-3333-4333-8333-333333333333"; + await render(nextTaskId); + expect(api.getIssueBinding).toHaveBeenLastCalledWith(nextTaskId); + expect(api.thread).toHaveBeenLastCalledWith(companyId, nextTaskId); + expect(container.textContent).toBe("bound"); + }); + + it("does not display cached task connector data for an agent chat", async () => { + client.setQueryData(["issue-chat-binding", companyId, chatId], { endpointId: "stale-endpoint" }); + client.setQueryData(["email-thread", companyId, chatId], { + messages: [], + publications: [{ id: "stale-publication", outcome: "failed", error: "Stale task delivery" }], + }); + await render(chatId); + expect(container.textContent).toBe("unbound"); + expect(api.getIssueBinding).not.toHaveBeenCalled(); + expect(api.thread).not.toHaveBeenCalled(); + }); +});