mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-06 10:48:12 +02:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
d54b750111
commit
6d03428682
7 files changed
+214
-8
No files matched your search
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
|
||||
@@ -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 &&
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -110,14 +110,15 @@ const filePhaseLabels: Record<ChatFileTransferPhase, string> = {
|
||||
|
||||
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,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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 <>
|
||||
<output>{binding ? "bound" : isLoading ? "loading" : "unbound"}</output>
|
||||
<EmailTaskActivity companyId={companyId} issueId={issueId} />
|
||||
</>;
|
||||
}
|
||||
|
||||
describe("task-only connector queries", () => {
|
||||
let container: HTMLDivElement;
|
||||
let root: Root;
|
||||
let client: QueryClient;
|
||||
|
||||
async function render(issueId: string) {
|
||||
flushSync(() => root.render(
|
||||
<QueryClientProvider client={client}>
|
||||
<ConnectorQueries issueId={issueId} />
|
||||
</QueryClientProvider>,
|
||||
));
|
||||
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();
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user