From fcf5dc2395ffc9eaa53b25807cd7877d50b37e59 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 6 Nov 2026 08:28:25 -0800 Subject: [PATCH] fix(ai-connections): reject unknown binding modes before grant checks (#19494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents run on AI accounts that people connect as AI connections > - Before a run, `aiConnectionService.select` resolves the agent's binding to one account grant and checks who may use it > - The grant-kind checks cover only the `responsible_user`, `shared` and `delegated` modes > - Every caller parses the binding first, so no other mode reaches `select` today. But if one did, it would skip the grant-kind checks > - This pull request rejects any other mode before `select` reads any data > - The benefit is that an authorization check cannot be skipped through a bad binding in the future ## Linked Issues or Issue Description No issue exists. The new test typecheck in #18739 showed that a test passed `mode: "connection"`, which is not a valid mode. #18739 fixed the test and listed the production question. **What existing behavior does this improve?** `select` in `server/src/services/ai-connections.ts` reads `binding.grantId` for every mode except `responsible_user`. It checks the grant kind only when the mode is `shared` or `delegated`. **Current behavior** All runtime callers parse the binding with `aiRuntimeConnectionBindingSchema` or `aiConnectionBindingSchema` first (`heartbeat.ts`, `agent-harness-verification.ts`, `routes/agents.ts`). So no live path sends an unknown mode. If a future caller skipped parsing, an unknown mode would pass with no grant-kind check. **Proposed behavior** `select` rejects an unknown mode with the existing `ai_connection_incompatible` failure (422), before it reads any data. **Reason and benefit** A guard in the owner of the check is cheaper than trusting every future caller. **Breaking changes** None. Valid bindings behave the same. ## What Changed - `server/src/services/ai-connections.ts`: `select` throws `ai_connection_incompatible` for any mode other than `responsible_user`, `shared` or `delegated`. - `server/src/__tests__/ai-connection-binding-mode.test.ts`: new test. It sends `mode: "connection"` and expects a 422 with no database read. ## Verification - `pnpm exec vitest run server/src/__tests__/ai-connection-binding-mode.test.ts`: passes. Without the service change, the same test fails because `select` reads the database. - `pnpm exec vitest run server/src/__tests__/ai-connections.test.ts server/src/__tests__/agent-hire-ai-connections.test.ts server/src/__tests__/ai-connection-binding-mode.test.ts`: 3 files, 243 tests pass. - `cd server && pnpm typecheck`: passes. This includes the source layers and the three test-tree shards. - Not run locally: the full `pnpm test:run` and the root `pnpm build`. Required CI covers them. ## Risks - Low risk. The new check only rejects bindings that the schemas already reject. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use (shell, file edits) and extended thinking. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- .../ai-connection-binding-mode.test.ts | 34 +++++++++++++++++++ server/src/services/ai-connections.ts | 4 +++ 2 files changed, 38 insertions(+) create mode 100644 server/src/__tests__/ai-connection-binding-mode.test.ts diff --git a/server/src/__tests__/ai-connection-binding-mode.test.ts b/server/src/__tests__/ai-connection-binding-mode.test.ts new file mode 100644 index 00000000000..e3b4a6d0ea6 --- /dev/null +++ b/server/src/__tests__/ai-connection-binding-mode.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it, vi } from "vitest"; +import type { Db } from "@paperclipai/db"; +import type { AiConnectionBinding } from "@paperclipai/shared"; +import { readAiConnectionConfigurationFailure } from "../services/ai-connection-configuration-failure.js"; +import { aiConnectionService } from "../services/ai-connections.js"; + +describe("AI connection binding mode guard", () => { + it("rejects an unknown binding mode before it reads any connection", async () => { + // Any database read fails the test: the guard must run first. + const select = vi.fn(() => { + throw new Error("unexpected database read"); + }); + const service = aiConnectionService({ select } as unknown as Db); + const binding = { + provider: "openai", + method: "api_key", + mode: "connection", + connectionId: "11111111-1111-4111-8111-111111111111", + grantId: "22222222-2222-4222-8222-222222222222", + } as unknown as AiConnectionBinding; + + const error = await service.select({ + companyId: "33333333-3333-4333-8333-333333333333", + userId: "user-1", + agentId: "44444444-4444-4444-8444-444444444444", + adapterType: "codex_local", + binding, + }).catch((caught: unknown) => caught); + + expect(error).toMatchObject({ status: 422 }); + expect(readAiConnectionConfigurationFailure(error)).toBe("ai_connection_incompatible"); + expect(select).not.toHaveBeenCalled(); + }); +}); diff --git a/server/src/services/ai-connections.ts b/server/src/services/ai-connections.ts index 3622fc2c1c8..a4081a4b4d7 100644 --- a/server/src/services/ai-connections.ts +++ b/server/src/services/ai-connections.ts @@ -281,6 +281,10 @@ export function aiConnectionService(db: Db) { binding: AiConnectionBinding; }) { const { companyId, userId, agentId, binding } = input; + // The grant-kind checks below cover only these three modes. Callers parse + // the binding first, but an unknown mode must never skip those checks. + if (binding.mode !== "responsible_user" && binding.mode !== "shared" && binding.mode !== "delegated") + throw aiConnectionConfigurationFailure("ai_connection_incompatible", "Select a supported AI connection mode"); if ( !isAiConnectionCompatible( binding,