Skip to content

Commit 077d97b

Browse files
viktormarinhoclaude
andcommitted
fix(auth): authorize API keys solely by their own allowlist
A key's owner role no longer widens it: an API key is authorized strictly by its stored `permissions`. A key an admin scopes to e.g. ORGANIZATION_GET cannot call any other tool — the privilege escalation reported by an agent run (a read-only key was minting admin keys via API_KEY_CREATE). - Remove the "default permissions" concept entirely. `API_KEY_CREATE` now requires `permissions`, and the three internal minters that relied on the default pass an explicit scope: the per-run agent MCP key and the dev-link key act as the user (`{ "*": ["*"] }`), the org-fs mount uses `{ self: ["*"] }` (its file ops come from basic-usage). So "a key without a scope" no longer exists. - `createBoundAuthClient`: an API-key principal (apiKeyId present) is checked via `checkApiKeyPermission(permissions, …)` BEFORE the admin/owner role bypass, and exposes `isApiKeyPrincipal` on `BoundAuthClient`. - `AccessControl.checkResource` gates its admin/owner bypass on `!boundAuth?.isApiKeyPrincipal`, so REST (`/api/:org/tools`) and MCP (`/mcp`) both enforce from one signal. Browser sessions, MCP OAuth, and mesh JWTs keep the role-based path unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f1dacab commit 077d97b

12 files changed

Lines changed: 297 additions & 68 deletions

File tree

apps/mesh/src/api/routes/decopilot/dispatch-run.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,12 @@ async function mintMcpEndpoint(
397397
}> {
398398
const apiKey = await ctx.boundAuth.apiKey.create({
399399
name: apiKeyName,
400+
// The per-run key is the agent's own callback credential — it proxies to
401+
// `/mcp/virtual-mcp/<agentId>` (a `vir_*` resource) and acts on behalf of
402+
// the user for the duration of the run, so it needs full access. With no
403+
// implicit default (auth/index.ts), the scope must be explicit; wildcard
404+
// matches the prior behavior (full access via the admin bypass).
405+
permissions: { "*": ["*"] },
400406
expiresIn: MCP_KEY_TTL_SECONDS,
401407
metadata: {
402408
organization: {
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { describe, expect, it } from "bun:test";
2+
import { checkApiKeyPermission } from "./api-key-permissions";
3+
4+
describe("checkApiKeyPermission", () => {
5+
it("grants when the exact (resource, tool) is in the allowlist", () => {
6+
expect(
7+
checkApiKeyPermission(
8+
{ self: ["ORGANIZATION_GET"] },
9+
{ self: ["ORGANIZATION_GET"] },
10+
),
11+
).toBe(true);
12+
});
13+
14+
it("denies a tool outside the allowlist (the escalation we block)", () => {
15+
expect(
16+
checkApiKeyPermission(
17+
{ self: ["ORGANIZATION_GET"] },
18+
{ self: ["API_KEY_CREATE"] },
19+
),
20+
).toBe(false);
21+
});
22+
23+
it("denies when the resource is not granted at all", () => {
24+
expect(
25+
checkApiKeyPermission(
26+
{ self: ["ORGANIZATION_GET"] },
27+
{ conn_123: ["SEND_MESSAGE"] },
28+
),
29+
).toBe(false);
30+
});
31+
32+
it("grants every tool for a resource with a wildcard", () => {
33+
expect(checkApiKeyPermission({ self: ["*"] }, { self: ["ANYTHING"] })).toBe(
34+
true,
35+
);
36+
});
37+
38+
it("honors a wildcard `*` resource granting all tools (full key)", () => {
39+
expect(
40+
checkApiKeyPermission({ "*": ["*"] }, { conn_9: ["SEND_MESSAGE"] }),
41+
).toBe(true);
42+
expect(checkApiKeyPermission({ "*": ["*"] }, { self: ["ANYTHING"] })).toBe(
43+
true,
44+
);
45+
});
46+
47+
it("denies everything for an empty / absent allowlist (fail-closed)", () => {
48+
expect(checkApiKeyPermission({}, { self: ["ORGANIZATION_GET"] })).toBe(
49+
false,
50+
);
51+
});
52+
53+
it("requires ALL requested tools to be covered", () => {
54+
expect(
55+
checkApiKeyPermission(
56+
{ self: ["ORGANIZATION_GET"] },
57+
{ self: ["ORGANIZATION_GET", "API_KEY_CREATE"] },
58+
),
59+
).toBe(false);
60+
});
61+
});
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* API-key authorization.
3+
*
4+
* An API key is authorized SOLELY by its own stored `permissions` allowlist
5+
* ({ resource: [tools] }) — the owner's org role never widens it. A full-access
6+
* key carries an explicit wildcard (`{ "*": ["*"] }` or `{ self: ["*"] }`); a
7+
* key with no allowlist grants nothing (fail-closed). There is intentionally no
8+
* "default permission" fallback: every key is created with an explicit scope
9+
* (see API_KEY_CREATE and the internal minters), so "a key without a scope"
10+
* does not exist.
11+
*/
12+
13+
import type { Permission } from "../storage/types";
14+
15+
/**
16+
* Does an API key's stored allowlist grant the requested permission?
17+
*
18+
* Both are `{ resource: [tools] }` maps. Every (resource, tool) in `requested`
19+
* must be covered by `granted` — directly, by a per-resource `"*"`, or by a
20+
* wildcard `"*"` resource (itself optionally `"*"` for all tools). Returns false
21+
* on the first uncovered entry (fail-closed); an empty/absent allowlist grants
22+
* nothing.
23+
*/
24+
export function checkApiKeyPermission(
25+
granted: Permission,
26+
requested: Permission,
27+
): boolean {
28+
for (const [resource, tools] of Object.entries(requested)) {
29+
const grantedTools = granted[resource];
30+
31+
if (!grantedTools || grantedTools.length === 0) {
32+
// No grant for this resource — fall back to a wildcard `"*"` resource.
33+
const wildcardTools = granted["*"];
34+
if (!wildcardTools || wildcardTools.length === 0) {
35+
return false;
36+
}
37+
if (wildcardTools.includes("*")) {
38+
continue; // wildcard resource grants every tool
39+
}
40+
const wildcardSet = new Set(wildcardTools);
41+
for (const tool of tools) {
42+
if (!wildcardSet.has(tool)) return false;
43+
}
44+
continue;
45+
}
46+
47+
if (grantedTools.includes("*")) {
48+
continue; // wildcard grants all tools for this resource
49+
}
50+
51+
const grantedSet = new Set(grantedTools);
52+
for (const tool of tools) {
53+
if (!grantedSet.has(tool)) return false;
54+
}
55+
}
56+
57+
return true;
58+
}

apps/mesh/src/auth/dev-link-session.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,9 @@ export async function bootstrapDevLinkSession(
115115
body: {
116116
name: "dev-link (auto-minted by bun run dev)",
117117
userId: user.id,
118+
// Dev loopback acting as the user — full access. With no implicit
119+
// default (auth/index.ts), the scope must be explicit.
120+
permissions: { "*": ["*"] },
118121
// 30 days — re-minted on file deletion, far longer than any
119122
// single dev session.
120123
expiresIn: 60 * 60 * 24 * 30,

apps/mesh/src/auth/index.ts

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -311,20 +311,10 @@ const plugins = [
311311
}
312312
return null;
313313
},
314-
permissions: {
315-
defaultPermissions: {
316-
self: [
317-
"ORGANIZATION_LIST",
318-
"ORGANIZATION_GET", // Organization read access
319-
"ORGANIZATION_MEMBER_LIST", // Member read access
320-
"COLLECTION_CONNECTIONS_LIST",
321-
"COLLECTION_CONNECTIONS_GET", // Connection read access
322-
"API_KEY_CREATE", // API key creation
323-
"API_KEY_LIST", // API key listing (metadata only)
324-
// Note: API_KEY_UPDATE and API_KEY_DELETE are not default - users must explicitly request
325-
],
326-
},
327-
},
314+
// No `defaultPermissions`: every key is created with an explicit scope (the
315+
// API_KEY_CREATE tool requires `permissions`, and the internal minters pass
316+
// their own). A key is authorized solely by its allowlist — see
317+
// auth/api-key-permissions.ts. A key with no allowlist grants nothing.
328318
rateLimit: {
329319
enabled: false,
330320
},

apps/mesh/src/core/access-control.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,51 @@ describe("AccessControl", () => {
143143
expect(ac.granted()).toBe(true);
144144
});
145145

146+
it("does NOT bypass the admin role for an API-key principal", async () => {
147+
// A key scoped to ORGANIZATION_GET is authorized solely by that allowlist —
148+
// the owner's admin/owner role must not widen it. The flag lives on
149+
// boundAuth, which enforces the key's permissions.
150+
const keyBoundAuth = {
151+
...createMockBoundAuth({ self: ["ORGANIZATION_GET"] }),
152+
isApiKeyPrincipal: true,
153+
} as BoundAuthClient;
154+
155+
const allowed = new AccessControl(
156+
"user_1",
157+
"ORGANIZATION_GET",
158+
keyBoundAuth,
159+
"admin", // owner is an admin — must NOT grant beyond the allowlist
160+
);
161+
await allowed.check();
162+
expect(allowed.granted()).toBe(true);
163+
164+
const denied = new AccessControl(
165+
"user_1",
166+
"API_KEY_CREATE", // out of scope — the exact escalation we are blocking
167+
keyBoundAuth,
168+
"admin",
169+
);
170+
await expect(denied.check()).rejects.toThrow(ForbiddenError);
171+
expect(denied.granted()).toBe(false);
172+
});
173+
174+
it("still bypasses the admin role for a non-API-key principal (session)", async () => {
175+
// isApiKeyPrincipal unset (browser session / MCP OAuth) → admin bypass.
176+
const boundAuth = {
177+
...createMockBoundAuth({ self: ["ORGANIZATION_GET"] }),
178+
isApiKeyPrincipal: false,
179+
} as BoundAuthClient;
180+
181+
const ac = new AccessControl(
182+
"user_1",
183+
"API_KEY_CREATE",
184+
boundAuth,
185+
"owner",
186+
);
187+
await ac.check();
188+
expect(ac.granted()).toBe(true);
189+
});
190+
146191
it("should check connection-specific permissions", async () => {
147192
const ac = new AccessControl(
148193
"user_1",

apps/mesh/src/core/access-control.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,17 @@ export class AccessControl {
199199
return true;
200200
}
201201

202-
// Admin and owner roles bypass all checks (they have full access)
203-
if (this.role === "admin" || this.role === "owner") {
202+
// Admin and owner roles bypass all checks (they have full access) — EXCEPT
203+
// for an API-key principal, which is authorized SOLELY by the key's stored
204+
// allowlist and must never inherit the owner's role. Otherwise a "read-only"
205+
// key minted by an admin could mint admin keys. An API-key principal falls
206+
// through to `boundAuth.hasPermission`, which enforces the allowlist. The
207+
// flag lives on `boundAuth` so every construction site (REST + MCP) reads
208+
// one source; it is unset for sessions / MCP OAuth / mesh JWTs.
209+
if (
210+
!this.boundAuth?.isApiKeyPrincipal &&
211+
(this.role === "admin" || this.role === "owner")
212+
) {
204213
return true;
205214
}
206215

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
/**
2+
* Unit tests for the API-key authorization wired into `createBoundAuthClient`.
3+
* These exercise only the in-memory decision branches (API-key allowlist / role
4+
* bypass), none of which call Better Auth — so a minimal stubbed `auth` is enough.
5+
*/
6+
import { describe, expect, it } from "bun:test";
7+
import { createBoundAuthClient } from "./context-factory";
8+
import type { BetterAuthInstance } from "./studio-context";
9+
10+
const stubAuth = { api: {} } as unknown as BetterAuthInstance;
11+
12+
describe("createBoundAuthClient — API-key authorization", () => {
13+
it("authorizes an API key SOLELY by its allowlist, ignoring the owner role", async () => {
14+
const client = createBoundAuthClient({
15+
auth: stubAuth,
16+
headers: new Headers(),
17+
role: "admin", // owner is an admin — must NOT widen the key
18+
permissions: { self: ["ORGANIZATION_GET"] },
19+
apiKeyId: "key_1",
20+
});
21+
22+
expect(client.isApiKeyPrincipal).toBe(true);
23+
expect(await client.hasPermission({ self: ["ORGANIZATION_GET"] })).toBe(
24+
true,
25+
);
26+
// The exact escalation the report found — denied even for an admin owner.
27+
expect(await client.hasPermission({ self: ["API_KEY_CREATE"] })).toBe(
28+
false,
29+
);
30+
});
31+
32+
it("treats a wildcard key as full access (explicit full key)", async () => {
33+
const client = createBoundAuthClient({
34+
auth: stubAuth,
35+
headers: new Headers(),
36+
role: "user",
37+
permissions: { "*": ["*"] },
38+
apiKeyId: "key_2",
39+
});
40+
41+
expect(client.isApiKeyPrincipal).toBe(true);
42+
expect(await client.hasPermission({ self: ["API_KEY_CREATE"] })).toBe(true);
43+
expect(await client.hasPermission({ vir_x: ["SEND_MESSAGE"] })).toBe(true);
44+
});
45+
46+
it("denies an API key with no allowlist (fail-closed)", async () => {
47+
const client = createBoundAuthClient({
48+
auth: stubAuth,
49+
headers: new Headers(),
50+
role: "owner",
51+
permissions: undefined,
52+
apiKeyId: "key_3",
53+
});
54+
55+
expect(client.isApiKeyPrincipal).toBe(true);
56+
expect(await client.hasPermission({ self: ["ORGANIZATION_GET"] })).toBe(
57+
false,
58+
);
59+
});
60+
61+
it("keeps the admin/owner role bypass for a non-API-key principal", async () => {
62+
const client = createBoundAuthClient({
63+
auth: stubAuth,
64+
headers: new Headers(),
65+
role: "admin",
66+
permissions: { self: ["ORGANIZATION_GET"] }, // e.g. a mesh JWT payload
67+
// no apiKeyId — not an API key principal
68+
});
69+
70+
expect(client.isApiKeyPrincipal).toBe(false);
71+
expect(await client.hasPermission({ self: ["API_KEY_CREATE"] })).toBe(true);
72+
});
73+
});

0 commit comments

Comments
 (0)