mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(tool-access): an unscoped tools:use grant does not turn on actions a connection left off (#20160)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - People connect apps and choose which actions of each app agents may
call; the choice is saved as the connection's tool profile
> - Every new standard agent also gets an unscoped `tools:use` grant,
which means "this agent may use tools"
> - The access policy checked explicit grants before profiles, so this
default grant allowed every action of every installed connection,
including actions the person left off
> - The agent's effective profile showed the action off, but the gateway
and the test-as-agent summary allowed it
> - This pull request makes an unscoped grant stop at a connection
profile: when a profile governs the tool's connection, only a grant
scoped to the action, connection or application allows an action the
profile leaves off
> - The benefit is that the actions a person turns off for a connection
stay off for new agents
## Linked Issues or Issue Description
**What happened**
A person connected an app with one read action and one destructive write
action. When they finished the connection, they enabled only the read
action. They installed the connection for a new agent, Morgan. Morgan's
effective profile (`GET
/api/companies/{companyId}/tools/profiles/effective/agents/{agentId}`)
allowed only the read action. But `GET
/api/tool-connections/{connectionId}/test-agents/{agentId}/access`
reported both actions as `allowed` with reason `allow_explicit_grant`,
and a chat agent told the person that Morgan could also run the
destructive write. The same policy decision admits gateway calls, so
Morgan could call the write.
**Expected behavior**
An action that the connection's profile leaves off is off for the agent.
The access summary, the effective profile and the gateway agree. A grant
that a person scopes to that action, connection or application still
allows it.
**Steps to reproduce**
1. Connect an MCP app that has a read tool and a destructive tool. In
the finish step, enable only the read tool.
2. Create a new agent through the API (it gets the default unscoped
`tools:use` grant) and install the connection for it.
3. Call `GET
/api/tool-connections/{connectionId}/test-agents/{agentId}/access`: the
destructive tool shows `allowed`, reason `allow_explicit_grant`.
## What Changed
- `server/src/services/tool-access-policy.ts`: for an agent, a tool's
connection is governed by a connection profile when the agent's
effective profiles include the connection's own access profile (the app
wizard's `app:<connectionId>` profile or the agent's connection-intent
profile, even with no entries), or any entry that names this connection
or selects this action (including application, tool-name and risk-level
selectors). For a governed connection, `explicitGrant` counts only
grants whose scope names an action, connection or application (`allow`,
`toolName`, `catalogEntryId`, `connectionId`, `applicationId`,
`applicationKey`). A grant scoped only by context (for example an agent
or project) still covers every tool, so it does not override the
profile. Board users keep their operator grant.
- `server/src/services/tool-profile-binding-precedence.ts`: export the
existing connection-profile identity check as
`isConnectionAccessProfile`, so the policy and binding precedence use
one definition.
- `server/src/__tests__/generic-mcp-connection.test.ts`: regression
tests through the gateway's access summary and the direct policy check:
- only the read action enabled: the write stays off with the unscoped
grant, and a grant scoped to the write allows it
- every action turned off (both lists empty): both stay off; a
context-only scope stays off; a connection-scoped or application-scoped
grant allows them
- an agent the connection's profile does not cover keeps the unscoped
grant
- a general profile with an application-selector exclude (no connection
id) is not overridden by the unscoped grant
- with company-wide access, the direct policy check denies the off
action for an agent and still allows it for a board user with the
unscoped grant
## Verification
- New tests: all pass with the change. On `master`, 4 of 5 fail. On the
first revision of this PR, the all-off, application-exclude and
board-user tests fail.
- Regression suites, all pass: `generic-mcp-connection`,
`tool-access-policy-service`, `tool-profile-matching`,
`tool-profile-entry-order`, `connection-management.integration` (238
tests), and `tool-access-service`, `tool-gateway-service`,
`tool-gateway`, `tool-profile-review-service` (799 tests).
- Every enforcement path uses the one `decide` function in
`tool-access-policy.ts`, and `explicitGrant` is called only there. That
covers the gateway tool listing and calls, the access summary, delegated
and Test tab calls, the connection-intent access check, the GitHub
review bot tool, and the example smoke check.
- Chat eval `app-permissions.test-as-agent` (runner-opencode). Before
the change (GitHub Actions run 42550085273), the access summary for
Morgan reported the write as `allowed` by `allow_explicit_grant`. With
the first commit cherry-picked onto the eval branch, GitHub Actions run
42552819589 passed. The summary reported `launch_read` allowed
(`allow_profile`) and `launch_update` off (`deny_default`), which
matches Morgan's effective profile, and the agent's reply said the same.
## Risks
- Medium risk. This changes an access decision. An agent that used a
connection action only through its default unscoped grant, while the
connection's profile or an explicit exclude left that action off, loses
that action. That is the intended fix. An operator who relied on it must
enable the action in the connection or give the agent a grant that names
the action, connection or application.
- A grant scoped only by context (for example `{ projectId }`) no longer
overrides a connection profile. Risk-level and provider-type scopes are
not treated as naming an action.
- Tools that no connection profile governs (for example Paperclip's own
tools), agents without a connection profile, and board users keep the
current behavior.
> For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and
discuss it in `#dev` before opening the PR. Feature PRs that overlap
with planned core work may need to be redirected — check the roadmap
first. See `CONTRIBUTING.md`.
## Model Used
- Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code
execution in Claude Code.
## 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
- [x] I will address all Greptile and reviewer comments before
requesting merge
Related: #17884 (scoped `tools:use` grants at hire time).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
28e070f8dc
commit
3f12046576
3 files changed
+140
-5
No files matched your search
@@ -1053,6 +1053,106 @@ describeEmbeddedPostgres("generic remote MCP connections", () => {
|
||||
expect(JSON.stringify(response.body)).not.toContain("PAPERCLIP_PUBLIC_URL");
|
||||
});
|
||||
|
||||
it("keeps an action the connection left off from agents that hold only the default unscoped tools:use grant", async () => {
|
||||
installMcpOAuthFixture({ auth: "header", requiredHeader: { name: "Authorization", value: "Bearer fixture-key-123" }, tools: [
|
||||
{ name: "list_insights", description: "List insights", annotations: { readOnlyHint: true, destructiveHint: false } },
|
||||
{ name: "create_insight", description: "Create an insight", annotations: { readOnlyHint: false, destructiveHint: true } },
|
||||
] });
|
||||
const company = await createCompany(db);
|
||||
const service = toolAccessService(db);
|
||||
const actor = { actorType: "user" as const, actorId: "board-user", userId: "board-user" } as never;
|
||||
const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture bearer", authMode: "bearer",
|
||||
credentialValues: { "credentials.authorization": "fixture-key-123" }, grantKind: "organization" } as never, actor);
|
||||
const readOnly = connected.actions.readOnly.map((row: { catalogEntryId: string }) => row.catalogEntryId);
|
||||
const [agent] = await db.insert(agents).values({ companyId: company.id, name: "Morgan", role: "engineer", status: "active", adapterType: "process", adapterConfig: {}, runtimeConfig: {} }).returning();
|
||||
await service.finishGalleryAppConnection(company.id, connected.connectionId, { enabledCatalogEntryIds: readOnly, askFirstCatalogEntryIds: [], access: { agentIds: [] } }, actor);
|
||||
await service.putConnectionInstalls(connected.connectionId, { installs: [{ targetType: "agent", targetId: agent!.id }] }, actor);
|
||||
// Every new standard agent receives this unscoped grant.
|
||||
await db.insert(principalPermissionGrants).values({ companyId: company.id, principalType: "agent", principalId: agent!.id, permissionKey: "tools:use", scope: null });
|
||||
const gateway = createToolGatewayService(db, { toolActionSigningSecret: "fixture-secret" });
|
||||
const summary = async () => (await gateway.summarizeConnectionAccessForAgent({ companyId: company.id, connectionId: connected.connectionId, agentId: agent!.id }))
|
||||
.tools.map(tool => [tool.toolName, tool.decision]).sort();
|
||||
expect(await summary()).toEqual([["create_insight", "off"], ["list_insights", "allowed"]]);
|
||||
// A grant scoped to the action is an explicit choice and still applies.
|
||||
await db.update(principalPermissionGrants).set({ scope: { allow: ["tool:create_insight"] } })
|
||||
.where(and(eq(principalPermissionGrants.principalId, agent!.id), eq(principalPermissionGrants.permissionKey, "tools:use")));
|
||||
expect(await summary()).toEqual([["create_insight", "allowed"], ["list_insights", "allowed"]]);
|
||||
});
|
||||
|
||||
async function setUpGovernedTwoActionConnection(options: { enabled: "read" | "none"; access?: "all_agents" }) {
|
||||
installMcpOAuthFixture({ auth: "header", requiredHeader: { name: "Authorization", value: "Bearer fixture-key-123" }, tools: [
|
||||
{ name: "list_insights", description: "List insights", annotations: { readOnlyHint: true, destructiveHint: false } },
|
||||
{ name: "create_insight", description: "Create an insight", annotations: { readOnlyHint: false, destructiveHint: true } },
|
||||
] });
|
||||
const company = await createCompany(db);
|
||||
const service = toolAccessService(db);
|
||||
const actor = { actorType: "user" as const, actorId: "board-user", userId: "board-user" } as never;
|
||||
const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture bearer", authMode: "bearer",
|
||||
credentialValues: { "credentials.authorization": "fixture-key-123" }, grantKind: "organization" } as never, actor);
|
||||
const readOnly = connected.actions.readOnly.map((row: { catalogEntryId: string }) => row.catalogEntryId);
|
||||
const [agent] = await db.insert(agents).values({ companyId: company.id, name: "Morgan", role: "engineer", status: "active", adapterType: "process", adapterConfig: {}, runtimeConfig: {} }).returning();
|
||||
await service.finishGalleryAppConnection(company.id, connected.connectionId, {
|
||||
enabledCatalogEntryIds: options.enabled === "read" ? readOnly : [], askFirstCatalogEntryIds: [],
|
||||
access: options.access ?? { agentIds: [] },
|
||||
}, actor);
|
||||
if (!options.access) await service.putConnectionInstalls(connected.connectionId, { installs: [{ targetType: "agent", targetId: agent!.id }] }, actor);
|
||||
// Every new standard agent receives this unscoped grant.
|
||||
await db.insert(principalPermissionGrants).values({ companyId: company.id, principalType: "agent", principalId: agent!.id, permissionKey: "tools:use", scope: null });
|
||||
const gateway = createToolGatewayService(db, { toolActionSigningSecret: "fixture-secret" });
|
||||
const summary = async (agentId = agent!.id) => (await gateway.summarizeConnectionAccessForAgent({ companyId: company.id, connectionId: connected.connectionId, agentId }))
|
||||
.tools.map(tool => [tool.toolName, tool.decision]).sort();
|
||||
const setGrantScope = (scope: Record<string, unknown> | null) => db.update(principalPermissionGrants).set({ scope })
|
||||
.where(and(eq(principalPermissionGrants.principalId, agent!.id), eq(principalPermissionGrants.permissionKey, "tools:use")));
|
||||
const [connection] = await db.select().from(toolConnections).where(eq(toolConnections.id, connected.connectionId));
|
||||
return { company, agent: agent!, connection: connection!, summary, setGrantScope };
|
||||
}
|
||||
|
||||
it("keeps every action off when the connection turns all of them off, even for the default unscoped tools:use grant", async () => {
|
||||
const { connection, summary, setGrantScope } = await setUpGovernedTwoActionConnection({ enabled: "none" });
|
||||
expect(await summary()).toEqual([["create_insight", "off"], ["list_insights", "off"]]);
|
||||
// A grant that only narrows when it applies (agent, project) still covers every tool, so it is not a choice about this connection.
|
||||
await setGrantScope({ actorType: "agent" });
|
||||
expect(await summary()).toEqual([["create_insight", "off"], ["list_insights", "off"]]);
|
||||
// Grants that name the connection or the application are explicit choices and still apply.
|
||||
await setGrantScope({ connectionId: connection.id });
|
||||
expect(await summary()).toEqual([["create_insight", "allowed"], ["list_insights", "allowed"]]);
|
||||
await setGrantScope({ allow: [`application:${connection.applicationId}`] });
|
||||
expect(await summary()).toEqual([["create_insight", "allowed"], ["list_insights", "allowed"]]);
|
||||
});
|
||||
|
||||
it("keeps the unscoped tools:use grant for agents the connection's profile does not cover", async () => {
|
||||
const { company, summary } = await setUpGovernedTwoActionConnection({ enabled: "read" });
|
||||
const [other] = await db.insert(agents).values({ companyId: company.id, name: "Riley", role: "engineer", status: "active", adapterType: "process", adapterConfig: {}, runtimeConfig: {} }).returning();
|
||||
await db.insert(principalPermissionGrants).values({ companyId: company.id, principalType: "agent", principalId: other!.id, permissionKey: "tools:use", scope: null });
|
||||
expect(await summary(other!.id)).toEqual([["create_insight", "allowed"], ["list_insights", "allowed"]]);
|
||||
});
|
||||
|
||||
it("does not let the unscoped tools:use grant override a profile entry that excludes the application", async () => {
|
||||
const { company, summary } = await setUpGovernedTwoActionConnection({ enabled: "read" });
|
||||
const [other] = await db.insert(agents).values({ companyId: company.id, name: "Riley", role: "engineer", status: "active", adapterType: "process", adapterConfig: {}, runtimeConfig: {} }).returning();
|
||||
await db.insert(principalPermissionGrants).values({ companyId: company.id, principalType: "agent", principalId: other!.id, permissionKey: "tools:use", scope: null });
|
||||
const [connection] = await db.select().from(toolConnections).where(eq(toolConnections.companyId, company.id));
|
||||
const [profile] = await db.insert(toolProfiles).values({ companyId: company.id, profileKey: `no-insights-${randomUUID()}`, name: "No insights", defaultAction: "allow" }).returning();
|
||||
// An application selector carries no connection id.
|
||||
await db.insert(toolProfileEntries).values({ companyId: company.id, profileId: profile!.id, selectorType: "application", effect: "exclude", applicationId: connection!.applicationId });
|
||||
await db.insert(toolProfileBindings).values({ companyId: company.id, profileId: profile!.id, targetType: "agent", targetId: other!.id });
|
||||
expect(await summary(other!.id)).toEqual([["create_insight", "off"], ["list_insights", "off"]]);
|
||||
});
|
||||
|
||||
it("applies the connection profile on the direct policy check and keeps a board user's unscoped grant", async () => {
|
||||
const { company, agent, connection, summary } = await setUpGovernedTwoActionConnection({ enabled: "read", access: "all_agents" });
|
||||
expect(await summary()).toEqual([["create_insight", "off"], ["list_insights", "allowed"]]);
|
||||
const [create] = await db.select().from(toolCatalogEntries).where(and(eq(toolCatalogEntries.connectionId, connection.id), eq(toolCatalogEntries.toolName, "create_insight")));
|
||||
const request = { connectionId: connection.id, catalogEntryId: create!.id, toolName: create!.toolName, arguments: {} };
|
||||
const policy = toolAccessPolicyService(db);
|
||||
const agentDecision = await policy.decide({ companyId: company.id, actor: { actorType: "agent", actorId: agent.id, agentId: agent.id }, request });
|
||||
expect([agentDecision.decision, agentDecision.reasonCode]).toEqual(["deny", "deny_default"]);
|
||||
// The connection's profile is bound to the company, which also matches board users; their operator grant still applies.
|
||||
await db.insert(principalPermissionGrants).values({ companyId: company.id, principalType: "user", principalId: "board-user", permissionKey: "tools:use", scope: null });
|
||||
const userDecision = await policy.decide({ companyId: company.id, actor: { actorType: "user", actorId: "board-user" }, request });
|
||||
expect([userDecision.decision, userDecision.reasonCode]).toEqual(["allow", "allow_explicit_grant"]);
|
||||
});
|
||||
|
||||
it("stores a bearer key as a secret and never reads it back", async () => {
|
||||
installMcpOAuthFixture({
|
||||
auth: "header",
|
||||
|
||||
@@ -49,6 +49,7 @@ import { toolPolicyConditionsSchema } from "@paperclipai/shared";
|
||||
import { badRequest, conflict, notFound, unprocessable } from "../errors.js";
|
||||
import {
|
||||
effectiveToolProfileBindings,
|
||||
isConnectionAccessProfile,
|
||||
profileIdsInBindingOrder,
|
||||
} from "./tool-profile-binding-precedence.js";
|
||||
import { recordToolRuntimeAuditWriteFailure } from "./tool-runtime-metrics.js";
|
||||
@@ -760,6 +761,23 @@ function rateBucket(rule: ToolRateLimitRule, ctx: ToolAccessContext): string {
|
||||
}).join("|");
|
||||
}
|
||||
|
||||
// Grant scope keys that name which tools the grant covers: an action, a
|
||||
// connection or an application. Context keys such as agentId or projectId only
|
||||
// narrow when the grant applies, so a grant with only those still covers every
|
||||
// tool and does not count as a choice about one connection's actions.
|
||||
const TOOL_GRANT_TARGET_SCOPE_KEYS = new Set([
|
||||
"allow",
|
||||
"toolName", "toolNames",
|
||||
"catalogEntryId", "catalogEntryIds",
|
||||
"connectionId", "connectionIds",
|
||||
"applicationId", "applicationIds",
|
||||
"applicationKey", "applicationKeys",
|
||||
]);
|
||||
|
||||
function isToolTargetedGrant(scope: unknown) {
|
||||
return isRecord(scope) && Object.keys(scope).some((key) => TOOL_GRANT_TARGET_SCOPE_KEYS.has(key));
|
||||
}
|
||||
|
||||
function scopeAllowsTool(scope: Record<string, unknown> | null, ctx: ToolAccessContext) {
|
||||
if (scope === null || scope === undefined) return true;
|
||||
if (!isRecord(scope)) return false;
|
||||
@@ -1246,7 +1264,7 @@ export function toolAccessPolicyService(db: Pick<Db, "select" | "insert" | "upda
|
||||
return { profiles: activeProfiles, entries };
|
||||
}
|
||||
|
||||
async function explicitGrant(ctx: ToolAccessContext, cache?: ToolAccessDecisionCache): Promise<boolean> {
|
||||
async function explicitGrant(ctx: ToolAccessContext, cache?: ToolAccessDecisionCache, options: { toolTargetedOnly?: boolean } = {}): Promise<boolean> {
|
||||
const principalType = ctx.actorType === "agent" ? "agent" : ctx.actorType === "user" ? "user" : null;
|
||||
const principalId = ctx.actorType === "agent" ? ctx.agentId : ctx.actorId;
|
||||
if (!principalType || !principalId) return false;
|
||||
@@ -1260,7 +1278,7 @@ export function toolAccessPolicyService(db: Pick<Db, "select" | "insert" | "upda
|
||||
eq(principalPermissionGrants.principalId, principalId),
|
||||
eq(principalPermissionGrants.permissionKey, "tools:use"),
|
||||
)));
|
||||
return grants.some((grant) => scopeAllowsTool(grant.scope, ctx));
|
||||
return grants.some((grant) => (!options.toolTargetedOnly || isToolTargetedGrant(grant.scope)) && scopeAllowsTool(grant.scope, ctx));
|
||||
}
|
||||
|
||||
async function enforceRateLimit(policy: typeof toolPolicies.$inferSelect, ctx: ToolAccessContext, consume: boolean) {
|
||||
@@ -1650,7 +1668,19 @@ export function toolAccessPolicyService(db: Pick<Db, "select" | "insert" | "upda
|
||||
return decision("allow", "allow_policy", "Tool access allowed by policy.", effectiveProfileIds, [policy.id], { redactionPlan: redaction.redactionPlan, policyExplanation });
|
||||
}
|
||||
}
|
||||
if (await explicitGrant(ctx, cache)) {
|
||||
// A connection's profile decides which of its actions agents may call. An
|
||||
// unscoped tools:use grant, which every new standard agent receives, is
|
||||
// permission to use tools at all; it must not turn on an action that a
|
||||
// profile governing this connection leaves off. The connection's own
|
||||
// access profile governs it even with no entries (every action turned
|
||||
// off), and so does any entry that selects this action, connection or
|
||||
// application. A grant that names the action, connection or application
|
||||
// still applies. Board users keep their operator grants.
|
||||
const governedByConnectionProfile = ctx.actorType === "agent" && !!ctx.connectionId && (
|
||||
profileState.profiles.some((profile) => isConnectionAccessProfile(profile, ctx.connectionId))
|
||||
|| profileState.entries.some((entry) => entry.connectionId === ctx.connectionId || profileEntryMatches(entry, ctx))
|
||||
);
|
||||
if (await explicitGrant(ctx, cache, { toolTargetedOnly: governedByConnectionProfile })) {
|
||||
return decision("allow", "allow_explicit_grant", "Tool access allowed by explicit grant.", effectiveProfileIds, [], { redactionPlan: redaction.redactionPlan });
|
||||
}
|
||||
|
||||
|
||||
@@ -56,7 +56,12 @@ export function profileIdsInBindingOrder<T extends Pick<BindingLike, "profileId"
|
||||
return ordered;
|
||||
}
|
||||
|
||||
function isWizardAppProfile(profile: ProfileLike, connectionId?: string | null): boolean {
|
||||
/**
|
||||
* A profile that a connection's own access settings own: the app wizard's
|
||||
* `app:<connectionId>` profile or an agent's `connection-intent` profile.
|
||||
* With a connection id, only that connection's profiles match.
|
||||
*/
|
||||
export function isConnectionAccessProfile(profile: ProfileLike, connectionId?: string | null): boolean {
|
||||
if (!profile.metadata || typeof profile.metadata !== "object" || Array.isArray(profile.metadata)) return false;
|
||||
const metadata = profile.metadata as Record<string, unknown>;
|
||||
if (metadata.source === "connection_intent" && typeof metadata.connectionId === "string" && typeof metadata.agentId === "string") {
|
||||
@@ -85,7 +90,7 @@ export function effectiveToolProfileBindings<T extends BindingLike>(
|
||||
return narrowestScopeBindings(bindings);
|
||||
}
|
||||
const appProfileIds = new Set(
|
||||
profiles.filter((profile) => isWizardAppProfile(profile, connectionId)).map((profile) => profile.id),
|
||||
profiles.filter((profile) => isConnectionAccessProfile(profile, connectionId)).map((profile) => profile.id),
|
||||
);
|
||||
const accessProfileIds = new Set(profiles.filter(profile => {
|
||||
const metadata = profile.metadata as Record<string, unknown> | null;
|
||||
|
||||
Reference in new issue
Block a user