From 4e00210da004376945b85bd3855a36df8f3b5a4d Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 22 Sep 2026 12:46:05 -0500 Subject: [PATCH] fix: qualify Slack transport and task tool initialization Co-Authored-By: Paperclip --- packages/shared/src/slack-tools.ts | 9 +++++- .../chat-channels.integration.test.ts | 32 +++++++++++++++++-- server/src/__tests__/openapi-routes.test.ts | 1 + server/src/routes/openapi.ts | 32 ++++++++++++++++++- server/src/routes/slack-tools.ts | 9 +++--- server/src/services/chat-channels.ts | 8 +++++ server/src/services/connector-runtime.ts | 12 ++++++- .../services/connectors/slack-access.test.ts | 12 +++++++ .../src/services/connectors/slack-client.ts | 11 +++++-- .../connectors/slack-native-search.test.ts | 11 +++---- .../services/connectors/slack-search-oauth.ts | 7 +--- 11 files changed, 119 insertions(+), 25 deletions(-) diff --git a/packages/shared/src/slack-tools.ts b/packages/shared/src/slack-tools.ts index b71f4ce14f9..94348a4718b 100644 --- a/packages/shared/src/slack-tools.ts +++ b/packages/shared/src/slack-tools.ts @@ -1,6 +1,13 @@ import { z } from "zod"; -const channel = z.string().regex(/^[CGD][A-Z0-9]+$/); +export const slackSearchConfigSchema = z + .object({ + clientId: z.string().regex(/^\d+\.\d+$/), + clientSecret: z.string().min(10).max(512), + }) + .strict(); + +const channel = z.string().regex(/^[CGD][A-Z0-9]+$/).describe("Slack channel ID (for example C012AB3CD), not a channel name or URL. Use the assigned source channel or slack_channels results."); const user = z.string().regex(/^[UW][A-Z0-9]+$/); const timestamp = z.string().regex(/^\d+\.\d+$/); const text = z.string().min(1).max(12000); diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index e255664fa54..72a09589ea3 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -3937,7 +3937,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { }); it("binds Slack tools to the admitted linked request, recovers its provenance and revokes retained access", async () => { - const { resolveConnectorAssignments, executeConnectorTool } = await import("../services/connector-runtime.js"); + const { resolveConnectorAssignments, executeConnectorTool, applyConnectorSkills, prepareConnectorSkillDelivery } = await import("../services/connector-runtime.js"); const { executeSlackTool } = await import("../services/connectors/slack.js"); const { resolveSlackTaskAuthority } = await import("../services/connectors/slack-authority.js"); await instanceSettingsService(db).updateExperimental({ enableChatConnectors: true }); @@ -3955,16 +3955,44 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { await db.insert(heartbeatRuns).values({ id: runId, companyId: fixture.companyId, agentId: fixture.assignedAgentId, status: "running", invocationSource: "assignment", responsibleUserId: "owner-user", contextSnapshot: { issueId: conversation.issueId } }); const origin = await initializeRunIdentity(db, { companyId: fixture.companyId, runId, issueId: conversation.issueId, responsibleUserId: "owner-user", messageIds: [String(action.payload.commentId)], cause: "instruction" }); const binding = { companyId: fixture.companyId, agentId: fixture.assignedAgentId, runId, issueId: conversation.issueId }; + // An ingress transaction may briefly own the endpoint while a retained + // run resolves its tools. Resolution waits, then rechecks authorization. + let unlockEndpoint!: () => void; + let lockedEndpoint!: () => void; + const endpointLocked = new Promise(resolve => { lockedEndpoint = resolve; }); + const endpointRelease = new Promise(resolve => { unlockEndpoint = resolve; }); + const ingressLock = db.transaction(async tx => { + await tx.select().from(chatEndpoints).where(eq(chatEndpoints.id, endpoint.id)).for("no key update"); + lockedEndpoint(); + await endpointRelease; + }); + await endpointLocked; + const resolvingAuthority = resolveSlackTaskAuthority(db, binding); + // Attach the rejection handler before waiting so NOWAIT regressions fail + // as an assertion, not an unhandled rejection. + const resolvedAuthority = resolvingAuthority.then(value => ({ value }), error => ({ error })); + try { + await new Promise(resolve => setTimeout(resolve, 100)); + } finally { + unlockEndpoint(); + await ingressLock; + } + expect(await resolvedAuthority).toMatchObject({ value: { slackUserId: "UTOOLS" } }); const assignments = await resolveConnectorAssignments(db, binding); expect(assignments.map(a => a.key)).toEqual(["slack"]); expect(assignments[0].resources[0].metadata?.channelId).toBe("CTOOLS"); + const skillConfig = await applyConnectorSkills({}, [], assignments); + const runnerDelivery = await prepareConnectorSkillDelivery(skillConfig, "paperclip_runner"); + expect(runnerDelivery.instructions).toContain('"channelId": "CTOOLS"'); + expect(runnerDelivery.instructions).toContain("untrusted source material"); + expect(runnerDelivery.instructions).not.toContain("xoxb-test-token"); expect(JSON.stringify(assignments)).not.toContain("xoxb-test-token"); await expect(resolveConnectorAssignments(db, { ...binding, agentId: fixture.replacementAgentId })).resolves.toEqual([]); await expect(resolveConnectorAssignments(db, { ...binding, issueId: randomUUID() })).resolves.toEqual([]); await expect(resolveConnectorAssignments(db, { ...binding, companyId: randomUUID() })).resolves.toEqual([]); const fetched = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { const method = String(input).split("/").at(-1); - const args = JSON.parse(String(init?.body ?? "{}")); + const args = Object.fromEntries(new URLSearchParams(String(init?.body ?? ""))); if (method === "users.info") return Response.json({ ok: true, user: { id: "UTOOLS", team_id: "T-PAPERCLIP" } }); if (method === "conversations.info") return Response.json({ ok: true, channel: { id: args.channel, is_member: true, is_private: args.channel === "GPRIVATE" } }); if (method === "conversations.members") return Response.json({ ok: true, members: ["UTOOLS"] }); diff --git a/server/src/__tests__/openapi-routes.test.ts b/server/src/__tests__/openapi-routes.test.ts index 88e88fdc208..22f40613943 100644 --- a/server/src/__tests__/openapi-routes.test.ts +++ b/server/src/__tests__/openapi-routes.test.ts @@ -29,6 +29,7 @@ const apiPrefixes: Record = { "board-chat.ts": "/api", "built-in-agents.ts": "/api", "chat-channels.ts": "/api", + "slack-tools.ts": "/api", "email.ts": "/api", "cloud.ts": "/api/cloud", "companies.ts": "/api/companies", diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 256cc4fc283..54e72f1d959 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -13,6 +13,8 @@ import { emailEndpointSetupSchema, emailConnectionSchema, emailSendSchema, + slackToolCallSchema, + slackSearchConfigSchema, // Agent AGENT_PALETTE_IDS, AGENT_AVATAR_SIZES, @@ -1483,6 +1485,12 @@ const BOARD_ONLY_OPERATIONS = new Set([ // Chat endpoints expose provider credentials, identity mappings, access // policy, and replay controls. Every mounted handler asserts a board actor; // keep the generated security contract equally restrictive. + "GET /api/slack/search/callback", + "GET /api/companies/{companyId}/slack/endpoints/{endpointId}/capabilities", + "GET /api/companies/{companyId}/slack/endpoints/{endpointId}/search", + "PUT /api/companies/{companyId}/slack/endpoints/{endpointId}/search", + "POST /api/companies/{companyId}/slack/endpoints/{endpointId}/search/connect", + "DELETE /api/companies/{companyId}/slack/endpoints/{endpointId}/search", "GET /api/companies/{companyId}/chat-endpoints", "POST /api/companies/{companyId}/chat-endpoints", "GET /api/chat-endpoints/{endpointId}", @@ -1622,7 +1630,7 @@ function resolveOperationAuthLevel( ): OpenApiAuthLevel { const key = operationKey(method, path); if (PUBLIC_OPERATIONS.has(key)) return "public"; - if (key === "POST /api/mcp/project-tools") return "agent_run"; + if (key === "POST /api/mcp/project-tools" || key === "POST /api/companies/{companyId}/slack/tasks/{issueId}/tools") return "agent_run"; if (RUNTIME_TOOLS_OPERATIONS.has(key)) return "runtime_tools"; if (INSTANCE_ADMIN_OPERATIONS.has(key)) return "instance_admin"; if ( @@ -2101,6 +2109,28 @@ for (const [method, path, summary, body, success] of [ }); } +// Slack bot tools use the verified task/run identity; setup and search grants +// remain board-only and company/endpoint scoped. +for (const [method, path, summary, body] of [ + ["get", "/api/companies/{companyId}/slack/endpoints/{endpointId}/capabilities", "Inspect Slack bot capabilities and missing scopes", undefined], + ["get", "/api/companies/{companyId}/slack/endpoints/{endpointId}/search", "Read personal Slack search authorization status", undefined], + ["put", "/api/companies/{companyId}/slack/endpoints/{endpointId}/search", "Configure optional Slack search OAuth credentials", slackSearchConfigSchema], + ["post", "/api/companies/{companyId}/slack/endpoints/{endpointId}/search/connect", "Begin personal Slack search authorization", undefined], + ["delete", "/api/companies/{companyId}/slack/endpoints/{endpointId}/search", "Disconnect personal Slack search and invalidate pending authorization", undefined], + ["post", "/api/companies/{companyId}/slack/tasks/{issueId}/tools", "Execute a task-bound Slack bot tool", slackToolCallSchema], +] as const) { + registry.registerPath({ method, path, tags: ["chat-channels"], summary, + description: "Experimental Slack task tools. Current company, endpoint, linked requester, task/run authority and action permissions are revalidated. Bot credentials remain server-side. Search grants never authorize writes or expand bot membership. Native search is unavailable until the runtime qualifies transient result handling.", + request: { params: z.object(Object.fromEntries([...path.matchAll(/\{([^}]+)\}/g)].map(match => [match[1], z.string().uuid()]))), ...(body ? { body: jsonBody(body) } : {}) }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden, 404: r.notFound, 409: r.conflict }, + }); +} +registry.registerPath({ method: "get", path: "/api/slack/search/callback", tags: ["chat-channels"], summary: "Complete personal Slack search OAuth", + description: "Requires the same signed-in user, single-use state, linked Slack identity and workspace; redirects to connector Access. Never accepts model-supplied identity.", + request: { query: z.object({ state: z.string(), code: z.string() }) }, + responses: { 302: { description: "Redirect to connector Access" }, 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden }, +}); + // ─── Chat Channels ───────────────────────────────────────────────────────── registry.registerPath({ diff --git a/server/src/routes/slack-tools.ts b/server/src/routes/slack-tools.ts index 9cf6feb963f..ffa86c7448a 100644 --- a/server/src/routes/slack-tools.ts +++ b/server/src/routes/slack-tools.ts @@ -85,8 +85,7 @@ export function slackToolRoutes(db: Db, publicBaseUrl?: string) { )) ); } - const searchPath = "/companies/:companyId/slack/endpoints/:endpointId/search"; - router.get(searchPath, async (req, res) => { + router.get("/companies/:companyId/slack/endpoints/:endpointId/search", async (req, res) => { const userId = user(req); const companyId = String(req.params.companyId); assertCompanyAccess(req, companyId); @@ -95,7 +94,7 @@ export function slackToolRoutes(db: Db, publicBaseUrl?: string) { canConfigure: await canConfigure(req, companyId, userId), }); }); - router.put(searchPath, async (req, res) => { + router.put("/companies/:companyId/slack/endpoints/:endpointId/search", async (req, res) => { const userId = user(req); const companyId = String(req.params.companyId); assertCompanyAccess(req, companyId); @@ -119,7 +118,7 @@ export function slackToolRoutes(db: Db, publicBaseUrl?: string) { ), ); }); - router.post(`${searchPath}/connect`, async (req, res) => { + router.post("/companies/:companyId/slack/endpoints/:endpointId/search/connect", async (req, res) => { const userId = user(req); const companyId = String(req.params.companyId); assertCompanyAccess(req, companyId); @@ -129,7 +128,7 @@ export function slackToolRoutes(db: Db, publicBaseUrl?: string) { await oauth.start(companyId, String(req.params.endpointId), userId), ); }); - router.delete(searchPath, async (req, res) => { + router.delete("/companies/:companyId/slack/endpoints/:endpointId/search", async (req, res) => { const userId = user(req); const companyId = String(req.params.companyId); assertCompanyAccess(req, companyId); diff --git a/server/src/services/chat-channels.ts b/server/src/services/chat-channels.ts index 51dbfef9031..49d413006fd 100644 --- a/server/src/services/chat-channels.ts +++ b/server/src/services/chat-channels.ts @@ -38168,6 +38168,14 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { )).limit(1); if (!action || action.payload.requestedByActorType !== "user" || action.payload.requestedByActorId !== run.responsibleUserId) throw forbidden("Slack tools require a verified linked Slack request"); + // This standalone resolver does not own the scheduler's issue lock. Use + // the ingress lock order (endpoint, then issue) so a normal webhook or + // endpoint update cannot turn tool setup into a NOWAIT failure. The + // scheduler retains its nonblocking check when it owns the issue first. + await tx.select({ id: chatEndpoints.id }).from(chatEndpoints).where(and( + eq(chatEndpoints.companyId, binding.companyId), + eq(chatEndpoints.id, action.endpointId), + )).for("no key update"); const source = await authorizeInboundWakeup(tx, action); if (source.endpoint.provider !== "slack") throw forbidden("This is not a Slack task"); const [principal] = await tx.select().from(chatExternalPrincipals).where(and( diff --git a/server/src/services/connector-runtime.ts b/server/src/services/connector-runtime.ts index 94abb50b3a7..46bfb3f6af5 100644 --- a/server/src/services/connector-runtime.ts +++ b/server/src/services/connector-runtime.ts @@ -216,7 +216,17 @@ export async function prepareConnectorSkillDelivery( adapterType === "paperclip_runner" || (config.engine === "cli" && ["codex_local", "claude_local", "kimi_local"].includes(adapterType)); - if (scopedFiles) return { config, instructions: "" }; + if (scopedFiles) { + // Runner models with semantic tools cannot necessarily read staged skill + // files. Supply the Slack contract and verified source IDs in their input; + // keep the staged bundle for CLI-capable engines and compatibility hashing. + const slack = adapterType === "paperclip_runner" + ? config.paperclipRuntimeSkills.filter(entry => entry.key === "paperclipai/paperclip/slack") + : []; + const instructions = (await Promise.all(slack.map(entry => + fs.readFile(path.join(entry.source, "SKILL.md"), "utf8")))).join("\n\n"); + return { config, instructions }; + } const assigned = config.paperclipRuntimeSkills.filter((entry) => isConnectorSkill(entry.key), ); diff --git a/server/src/services/connectors/slack-access.test.ts b/server/src/services/connectors/slack-access.test.ts index 1749a332e01..070f177f593 100644 --- a/server/src/services/connectors/slack-access.test.ts +++ b/server/src/services/connectors/slack-access.test.ts @@ -171,6 +171,18 @@ describe("Slack contracts and transport", () => { ).rejects.toThrow("Unsupported"); expect(fetcher).not.toHaveBeenCalled(); }); + it("encodes Slack arguments like the official SDK, with nested values as JSON fields", async () => { + const fetcher = vi.fn().mockImplementation(async () => Response.json({ ok: true })); + await slackClient("bot-secret", fetcher)("conversations.list", { + types: "public_channel,private_channel,mpim,im", limit: 50, exclude_archived: true, cursor: undefined, + }); + expect(fetcher.mock.calls[0][1].headers["content-type"]).toContain("application/x-www-form-urlencoded"); + expect(Object.fromEntries(new URLSearchParams(fetcher.mock.calls[0][1].body))).toEqual({ + types: "public_channel,private_channel,mpim,im", limit: "50", exclude_archived: "true", + }); + await slackClient("bot-secret", fetcher)("files.completeUploadExternal", { files: [{ id: "F123", title: "Test & review" }] }); + expect(JSON.parse(new URLSearchParams(fetcher.mock.calls[1][1].body).get("files")!)).toEqual([{ id: "F123", title: "Test & review" }]); + }); it("reports missing scopes without exposing raw provider errors or credentials", async () => { const fetcher = vi.fn().mockResolvedValue( new Response( diff --git a/server/src/services/connectors/slack-client.ts b/server/src/services/connectors/slack-client.ts index aec180de2a6..0f48e58fa9f 100644 --- a/server/src/services/connectors/slack-client.ts +++ b/server/src/services/connectors/slack-client.ts @@ -39,9 +39,14 @@ export function slackClient(token: string, fetchImpl = fetch) { signal: AbortSignal.timeout(20_000), headers: { authorization: `Bearer ${token}`, - "content-type": "application/json; charset=utf-8", + "content-type": "application/x-www-form-urlencoded; charset=utf-8", }, - body: JSON.stringify(args), + // Match Slack's official WebClient encoding, including nested JSON + // arguments. Some Web API methods reject a JSON request body. + body: new URLSearchParams(Object.entries(args) + .filter(([, value]) => value !== undefined && value !== null) + .map(([key, value]) => [key, typeof value === "object" + ? JSON.stringify(value) : String(value)])).toString(), }); } catch { throw unprocessable( @@ -104,7 +109,7 @@ export function slackClient(token: string, fetchImpl = fetch) { ? result.needed : undefined; throw unprocessable( - `Slack could not perform this operation: ${code}${scopes ? `. Reinstall the app with ${scopes}.` : ". Check app permissions and Slack feature availability."}`, + `Slack could not perform this operation: ${code}${scopes ? `. Reinstall the app with ${scopes}.` : code === "invalid_arguments" ? ". Check the tool arguments; installing another connector will not fix this request." : ". Check app permissions and Slack feature availability."}`, { code: `slack_${code}`, missingScopes: scopes }, ); } diff --git a/server/src/services/connectors/slack-native-search.test.ts b/server/src/services/connectors/slack-native-search.test.ts index a9c66a6e093..bf8646d0c74 100644 --- a/server/src/services/connectors/slack-native-search.test.ts +++ b/server/src/services/connectors/slack-native-search.test.ts @@ -62,11 +62,10 @@ describe("Slack transient search provider (not exposed to retaining runtimes)", const init = ( fetched.mock.calls as unknown as [unknown, RequestInit][] )[0][1]; - expect(JSON.parse(String(init.body))).toMatchObject({ - action_token: "verified-event-token", - term_clauses: ["in:<#C1>", "from:<@U1>"], - include_context_messages: false, - }); + const sent = new URLSearchParams(String(init.body)); + expect(sent.get("action_token")).toBe("verified-event-token"); + expect(JSON.parse(sent.get("term_clauses")!)).toEqual(["in:<#C1>", "from:<@U1>"]); + expect(sent.get("include_context_messages")).toBe("false"); expect(JSON.stringify(result)).not.toContain("verified-event-token"); }); it("uses a search-only personal grant for private search and rejects revoked access before delivery", async () => { @@ -97,7 +96,7 @@ describe("Slack transient search provider (not exposed to retaining runtimes)", expect(init.headers).toMatchObject({ authorization: "Bearer personal-token", }); - expect(JSON.parse(String(init.body))).not.toHaveProperty("action_token"); + expect(new URLSearchParams(String(init.body)).has("action_token")).toBe(false); }); it("does not expose file hits outside bot-verified channel shares", async () => { const fetched = vi.fn(async (url: RequestInfo | URL) => diff --git a/server/src/services/connectors/slack-search-oauth.ts b/server/src/services/connectors/slack-search-oauth.ts index 207e31a20a2..934977dda1b 100644 --- a/server/src/services/connectors/slack-search-oauth.ts +++ b/server/src/services/connectors/slack-search-oauth.ts @@ -1,3 +1,4 @@ +import { slackSearchConfigSchema } from "@paperclipai/shared"; import { SLACK_NATIVE_SEARCH_LIMITATION } from "./slack-native-search.js"; import { syncConnectionCredentialBindings } from "../connection-credential-bindings.js"; import { randomBytes } from "node:crypto"; @@ -29,12 +30,6 @@ export const SLACK_SEARCH_USER_SCOPES = [ "search:read.private", "search:read.files", ]; -export const slackSearchConfigSchema = z - .object({ - clientId: z.string().regex(/^\d+\.\d+$/), - clientSecret: z.string().min(10).max(512), - }) - .strict(); const configSchema = z.object({ clientId: z.string(), clientSecretId: z.string().uuid(),