mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-12 18:06:11 +02:00
fix(tool-access): enforce stored grant restrictions (#14915)
## Thinking Path > - Paperclip governs the tools that agents can discover and call. > - Stored grants can limit access to a tool, connection, or application. > - The grant matcher must enforce every restriction in that scope. > - A nonmatching allow list fell through to a policy selector matcher that ignores allow. > - This change requires an explicit allow match and validates additional selectors. > - Malformed and unknown restrictions deny access. > - Discovery and execution now enforce the same stored grant limits. ## Linked Issues or Issue Description Related: #14864 adds the shared database and HTTP discovery fixture used here. Searched existing public PRs for tool grant scope fixes. No duplicate scope-validation fix was found. **What happened?** A stored grant with a nonmatching `scope.allow` could authorize a tool. Empty or malformed allow lists, unknown selectors, and combined mismatching selectors could also authorize access. The fallback policy matcher does not validate stored grant JSON. **Expected behavior** An explicit allow list must match the requested tool, connection, or application. Every additional selector must also match. Unknown or malformed restrictions must deny access. Existing null and empty-object scopes keep their broad grant behavior. **Steps to reproduce** 1. Run `tool-grant-scope.test.ts` on the baseline. 2. Create a deny profile and a grant that names another tool. 3. Attempt discovery or a call for the tool outside the grant. 4. The baseline authorizes access. The fix denies it. **Paperclip version or commit** The red baseline is `f2e0f1963`. This PR is based on `cad26c6bf`, which includes the merged discovery fix. **Deployment mode** The company-scoped MCP gateway. Reproduction uses isolated database fixtures and a deterministic HTTP provider. ## What Changed - Require an explicit allow entry to match the gateway or upstream tool name, connection, or application. - Apply all additional selectors after the allow match. - Reject unknown selectors, invalid value types, empty restrictions, and non-object scopes. - Preserve null and empty-object scope compatibility. - Add sixteen regressions, including discovery, successful execution, and revocation through the HTTP gateway. - Document stored grant scope behavior. ## Verification - Red baseline: seven restricted-scope cases and three malformed-root cases fail. HTTP discovery also exposes tools outside the grant. - All 16 grant regressions and 35 adjacent policy tests pass locally. The HTTP test excludes an ungranted tool from discovery, returns 403 for its call, and verifies that no provider call occurs. It also checks successful execution and later revocation. - Server typecheck passes. The final ownership and grant patches also pass 42 combined database and HTTP regressions. - [Full CI](https://github.com/paperclipai/paperclip/actions/runs/37011177989) passes for `803fa9440111742672c94c4471e5b98f15dd3b97`: all 54 checks succeed; two optional Storybook checks skip. This includes full typecheck, build, all test shards, all eight E2E shards, runner verification, and the canary dry run. - Greptile scores that exact head 5/5. No review threads remain unresolved. ## Risks Stored scopes with unknown keys or malformed restrictions now deny access. Operators must correct those grants before they can authorize tools. Null and empty-object scopes keep their previous broad behavior. There are no schema, dependency, or API changes. ## Model Used OpenAI Codex (GPT-6), with reasoning, repository inspection, code execution, database regressions, and HTTP tests. This session does not expose the exact serving model identifier or context window. ## 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 - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
cad26c6bfb
commit
483dbc8890
3 files changed
+141
-6
No files matched your search
@@ -13,6 +13,7 @@ Operator guide for Paperclip's MCP tool access surface. Audience: board users an
|
||||
- [Managed connections](#managed-connections)
|
||||
- [Catalog and risk classification](#catalog-and-risk-classification)
|
||||
- [Profiles and bindings](#profiles-and-bindings)
|
||||
- [Scoped tool grants](#scoped-tool-grants)
|
||||
- [Policies](#policies)
|
||||
- [Approval flow and trust rules](#approval-flow-and-trust-rules)
|
||||
- [Runtime slots](#runtime-slots)
|
||||
@@ -243,6 +244,28 @@ curl -fsS -H "Authorization: Bearer $BOARD_API_KEY" \
|
||||
| jq '{profileIds, allowedToolNames}'
|
||||
```
|
||||
|
||||
## Scoped tool grants
|
||||
|
||||
A stored tool grant can restrict access with `scope.allow` and tool selectors.
|
||||
An explicit `allow` must contain a matching `tool:<name>`, `connection:<id>`, or
|
||||
`application:<id>`. Tool names can use the gateway name or the upstream name.
|
||||
If the scope also includes selectors, those selectors must match as well.
|
||||
|
||||
For example, `{"allow":["tool:read_item"],"connectionId":"<connection-id>"}`
|
||||
allows that tool only on the selected connection. A matching allow entry does
|
||||
not override a mismatched connection selector.
|
||||
|
||||
Unknown selector names, malformed values, empty restrictions, and non-object
|
||||
scopes deny access. Singular selectors require a nonempty string. Plural
|
||||
selectors require a nonempty array of nonempty strings. The supported selector
|
||||
names are `actorType`, `agentId`, `projectId`, `routineId`, `issueId`, `gatewayId`,
|
||||
`applicationId`, `connectionId`, `catalogEntryId`, `applicationKey`,
|
||||
`providerType`, `toolName`, and `riskLevel`, plus their plural forms.
|
||||
|
||||
A null scope or an empty object preserves the existing unrestricted grant
|
||||
behavior. Discovery and execution evaluate the current grant. A revoked grant
|
||||
cannot authorize a later call merely because discovery previously listed it.
|
||||
|
||||
## Policies
|
||||
|
||||
Policies run after profile selection. A profile decides *can this agent see the tool*; a policy decides *is this exact call allowed right now*. Policy types:
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
import { eq } from "drizzle-orm";
|
||||
import express from "express";
|
||||
import request from "supertest";
|
||||
import { beforeAll, afterAll, describe, expect, it } from "vitest";
|
||||
import { connectionGrants, createDb, principalPermissionGrants, toolPolicies, toolProfiles, startEmbeddedPostgresTestDatabase, getEmbeddedPostgresTestSupport } from "@paperclipai/db";
|
||||
import { toolAccessPolicyService } from "../services/tool-access-policy.js";
|
||||
import { createToolGatewayService } from "../services/tool-gateway.js";
|
||||
import { mcpGatewayProtocolRoutes } from "../routes/tool-gateway.js";
|
||||
import { createListingFixture } from "./helpers/tool-gateway-listing-fixture.js";
|
||||
|
||||
const support = await getEmbeddedPostgresTestSupport();
|
||||
const suite = support.supported ? describe : describe.skip;
|
||||
suite("tool grant restrictions", () => {
|
||||
let db!: ReturnType<typeof createDb>;
|
||||
let temp!: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>>;
|
||||
beforeAll(async () => {
|
||||
temp = await startEmbeddedPostgresTestDatabase("paperclip-grant-scope-");
|
||||
db = createDb(temp.connectionString);
|
||||
});
|
||||
afterAll(async () => { await temp?.cleanup(); });
|
||||
|
||||
it.each([
|
||||
{ name: "exact tool", scope: { allow: ["tool:tool_0004"] }, allowed: true },
|
||||
{ name: "other tool", scope: { allow: ["tool:tool_0005"] }, allowed: false },
|
||||
{ name: "empty allow list", scope: { allow: [] }, allowed: false },
|
||||
{ name: "malformed allow list", scope: { allow: [42] }, allowed: false },
|
||||
{ name: "unknown selector", scope: { unsupported: true }, allowed: false },
|
||||
{ name: "empty selector list", scope: { toolNames: [] }, allowed: false },
|
||||
{ name: "exact selector", scope: { toolName: "tool_0004" }, allowed: true },
|
||||
{ name: "other selector", scope: { toolName: "tool_0005" }, allowed: false },
|
||||
{ name: "combined mismatch", scope: { allow: ["tool:tool_0004"], toolName: "tool_0005" }, allowed: false },
|
||||
{ name: "unknown combined selector", scope: { allow: ["tool:tool_0004"], unsupported: true }, allowed: false },
|
||||
{ name: "unrestricted empty scope", scope: {}, allowed: true },
|
||||
{ name: "unrestricted null scope", scope: null, allowed: true },
|
||||
{ name: "invalid scope array", scope: [] as unknown as Record<string, unknown>, allowed: false },
|
||||
{ name: "invalid scope number", scope: 42 as unknown as Record<string, unknown>, allowed: false },
|
||||
{ name: "invalid scope boolean", scope: false as unknown as Record<string, unknown>, allowed: false },
|
||||
])("enforces $name", async ({ scope, allowed }) => {
|
||||
const fixture = await createListingFixture(db, 6);
|
||||
await db.update(principalPermissionGrants).set({ scope }).where(eq(principalPermissionGrants.principalId, fixture.agent.id));
|
||||
const result = await toolAccessPolicyService(db).decide({
|
||||
companyId: fixture.company.id,
|
||||
actor: { actorType: "agent", actorId: fixture.agent.id, agentId: fixture.agent.id },
|
||||
runContext: { heartbeatRunId: fixture.run.id },
|
||||
request: { connectionId: fixture.connection.id, catalogEntryId: fixture.entries[4]!.id, toolName: "tool_0004" },
|
||||
});
|
||||
expect(result.allowed).toBe(allowed);
|
||||
});
|
||||
|
||||
it("lists and calls only granted tools through the HTTP gateway, then observes revocation", async () => {
|
||||
const fixture = await createListingFixture(db, 6);
|
||||
await db.update(toolProfiles).set({ defaultAction: "deny" }).where(eq(toolProfiles.id, fixture.namedGateway.profileId));
|
||||
await db.delete(toolPolicies).where(eq(toolPolicies.companyId, fixture.company.id));
|
||||
await db.update(principalPermissionGrants).set({ scope: {} })
|
||||
.where(eq(principalPermissionGrants.principalId, fixture.agent.id));
|
||||
await db.insert(connectionGrants).values({ companyId: fixture.company.id, connectionId: fixture.connection.id,
|
||||
kind: "organization", status: "active", isDefault: true });
|
||||
const calls: string[] = [];
|
||||
const service = createToolGatewayService(db, { remoteHttpRequest: async (_url, init) => {
|
||||
const body = JSON.parse(String(init.body));
|
||||
if (body.method === "tools/call") calls.push(body.params.name);
|
||||
return new Response(JSON.stringify({ jsonrpc: "2.0", id: body.id, result: body.method === "initialize"
|
||||
? { protocolVersion: "2025-03-26", capabilities: {}, serverInfo: { name: "fixture", version: "1" } }
|
||||
: { content: [{ type: "text", text: "fixture result" }] } }), { headers: { "content-type": "application/json" } });
|
||||
} });
|
||||
const app = express().use(express.json()).use(mcpGatewayProtocolRoutes(service));
|
||||
const post = (method: string, params?: unknown) => request(app).post(`/mcp/gateways/${fixture.namedGateway.gatewayPublicId}`)
|
||||
.set("Authorization", `Bearer ${fixture.token.token}`).send({ jsonrpc: "2.0", id: 1, method, params });
|
||||
const allTools = await service.listToolsForNamedGateway({ gatewayId: fixture.namedGateway.id, bearerToken: fixture.token.token });
|
||||
const ungrantedTool = allTools.find((entry) => entry.catalogEntryId === fixture.entries[5]!.id)!;
|
||||
expect(ungrantedTool).toBeDefined();
|
||||
await db.update(principalPermissionGrants).set({ scope: { allow: ["tool:tool_0004"] } })
|
||||
.where(eq(principalPermissionGrants.principalId, fixture.agent.id));
|
||||
const names = await service.listToolsForNamedGateway({ gatewayId: fixture.namedGateway.id, bearerToken: fixture.token.token });
|
||||
const granted = names.filter((entry) => entry.connectionId === fixture.connection.id);
|
||||
expect(granted.map((entry) => entry.catalogEntryId)).toEqual([fixture.entries[4]!.id]);
|
||||
const listing = await post("tools/list").expect(200);
|
||||
expect(listing.body.result.tools.some((entry: { name: string }) => entry.name === granted[0]!.name)).toBe(true);
|
||||
expect(listing.body.result.tools.some((entry: { name: string }) => entry.name === ungrantedTool.name)).toBe(false);
|
||||
await post("tools/call", { name: ungrantedTool.name, arguments: {} }).expect(403);
|
||||
expect(calls).toEqual([]);
|
||||
await post("tools/call", { name: granted[0]!.name, arguments: {} }).expect(200);
|
||||
expect(calls).toEqual(["tool_0004"]);
|
||||
await db.update(principalPermissionGrants).set({ scope: { allow: ["tool:tool_0005"] } })
|
||||
.where(eq(principalPermissionGrants.principalId, fixture.agent.id));
|
||||
await post("tools/call", { name: granted[0]!.name, arguments: {} }).expect(403);
|
||||
expect(calls).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
@@ -729,12 +729,35 @@ function rateBucket(rule: ToolRateLimitRule, ctx: ToolAccessContext): string {
|
||||
}
|
||||
|
||||
function scopeAllowsTool(scope: Record<string, unknown> | null, ctx: ToolAccessContext) {
|
||||
if (!scope || Object.keys(scope).length === 0) return true;
|
||||
const allowed = listValues(scope.allow);
|
||||
if (allowed.includes(`tool:${ctx.toolName}`)) return true;
|
||||
if (ctx.connectionId && allowed.includes(`connection:${ctx.connectionId}`)) return true;
|
||||
if (ctx.applicationId && allowed.includes(`application:${ctx.applicationId}`)) return true;
|
||||
return selectorMatches(scope, ctx);
|
||||
if (scope === null || scope === undefined) return true;
|
||||
if (!isRecord(scope)) return false;
|
||||
if (Object.keys(scope).length === 0) return true;
|
||||
const { allow, ...selectors } = scope;
|
||||
if (Object.hasOwn(scope, "allow")) {
|
||||
const values = typeof allow === "string" ? [allow] : allow;
|
||||
if (!Array.isArray(values) || values.length === 0 ||
|
||||
!values.every((value) => typeof value === "string" && value.trim().length > 0)) return false;
|
||||
if (!values.includes(`tool:${ctx.toolName}`) &&
|
||||
!(ctx.upstreamToolName && values.includes(`tool:${ctx.upstreamToolName}`)) &&
|
||||
!(ctx.connectionId && values.includes(`connection:${ctx.connectionId}`)) &&
|
||||
!(ctx.applicationId && values.includes(`application:${ctx.applicationId}`))) return false;
|
||||
}
|
||||
// Grant restrictions must all be understood and match. The permissive policy
|
||||
// selector matcher cannot validate untrusted or historical grant JSON itself.
|
||||
const keys = new Set([
|
||||
"actorType", "agentId", "projectId", "routineId", "issueId", "gatewayId",
|
||||
"applicationId", "connectionId", "catalogEntryId", "applicationKey",
|
||||
"providerType", "toolName", "riskLevel",
|
||||
]);
|
||||
for (const [key, value] of Object.entries(selectors)) {
|
||||
if (keys.has(key)) {
|
||||
if (typeof value !== "string" || value.trim().length === 0) return false;
|
||||
} else if (key.endsWith("s") && keys.has(key.slice(0, -1))) {
|
||||
if (!Array.isArray(value) || value.length === 0 ||
|
||||
!value.every((entry) => typeof entry === "string" && entry.trim().length > 0)) return false;
|
||||
} else return false;
|
||||
}
|
||||
return selectorMatches(selectors, ctx);
|
||||
}
|
||||
|
||||
export function toolAccessPolicyService(db: Db) {
|
||||
|
||||
Reference in new issue
Block a user