From 3701be76fafd9c973f180c2949f870e8c409b987 Mon Sep 17 00:00:00 2001 From: Jannes Stubbemann Date: Fri, 12 Jun 2026 19:36:49 +0200 Subject: [PATCH] fix: read-only agent config/skill endpoints should not require agents:create (#3725) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Access to agents, skills, and configurations is governed by a per-company permission system > - `agents:create` is a mutation-tier permission that controls who can create or modify agents > - `assertCanReadConfigurations` delegates to `assertCanCreateAgentsForCompany`, effectively requiring `agents:create` just to *read* agent configs, skills, and config revisions > - That's a permission regression: any company member without `agents:create` hits 403 on the Skills tab, agent config pages, and revision history — but those responses are already secret-redacted > - This pull request loosens the read gate to company membership only, while keeping every mutation-adjacent gate at `agents:create` ## Linked Issues or Issue Description No existing issue covers this directly — problem described in-PR following the bug-report template: **What happened** `assertCanReadConfigurations` delegates to `assertCanCreateAgentsForCompany`, effectively requiring the mutation-tier `agents:create` permission just to *read* agent configs, skills, and config revisions. Any company member without `agents:create` hits 403 on the Skills tab, agent config pages, and revision history — even though those responses are already secret-redacted (`redactAgentConfiguration`, `redactConfigRevision`). **Expected behavior** Read-only configuration/skill/revision endpoints are readable by any company member; only mutation-adjacent endpoints require `agents:create`. **Steps to reproduce** As a company member without `agents:create`, open the Skills tab or an agent config page (or `GET` the config/skill/revision endpoints) — the request fails with 403. ## What Changed - `server/src/routes/agents.ts`: - `assertCanReadConfigurations` now requires company membership only (plus the existing agent-key cross-company check). Previously it required `agents:create`. - `actorCanReadConfigurationsForCompany` (the boolean twin, used by `GET /agents/:id` to decide whether to return a restricted detail) now uses the standard try/catch-around-`assertCompanyAccess` pattern. - `POST /companies/:companyId/adapters/:type/test-environment` is not a pure read (it exercises adapter secrets) and now calls `assertCanCreateAgentsForCompany` directly instead of going through `assertCanReadConfigurations`. Behavior for this endpoint is unchanged. ## Verification - Existing tests pass. - Manual: log in as a company member without an `agents:create` grant. Visit the Skills tab on an agent and the agent configuration panel — both load. Try to edit the agent — blocked, as before. - Manual: POST to `/companies/:companyId/adapters/:type/test-environment` as the same user — still 403. ## Risks Low. The only behavior change is on read endpoints whose responses were already redacted (\`redactAgentConfiguration\`, \`redactConfigRevision\`). No secret escapes anywhere. ## Model Used Claude Opus 4.6 (1M context), extended thinking mode. ## Checklist - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] Thinking path traces from project context to this change - [x] Model used specified - [x] Tests run locally and pass - [x] CI green - [x] Greptile review addressed Co-authored-by: Claude Opus 4.7 (1M context) --- .../agent-permissions-routes.test.ts | 82 +++++++++++++++++++ server/src/routes/agents.ts | 61 ++++++++++++-- 2 files changed, 134 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index 3b479ec7..445aae08 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -1492,6 +1492,88 @@ describe.sequential("agent permission routes", () => { }); }); + describe("agent configuration read gate", () => { + it("allows a board member without agents:create to read agent configuration", async () => { + // Board (human) users with company membership but no agents:create + // grant should still be able to view agent configuration — this is + // the read-only permission loosening introduced by this PR. + mockAccessService.canUser.mockResolvedValue(false); + mockAccessService.hasPermission.mockResolvedValue(false); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "session", + isInstanceAdmin: false, + companyIds: [companyId], + }); + + const res = await request(app).get(`/api/agents/${agentId}/configuration`); + + expect(res.status).toBe(200); + }); + + it("denies an agent actor without agents:create when reading peer config", async () => { + // Agent actors must still pass the agents:create gate (explicit + // grant OR canCreateAgents permission on the agent record). A peer + // agent in the same company without that permission must not be + // able to read another agent's configuration. + const peerAgentId = "33333333-3333-4333-8333-333333333333"; + const peerAgent = { ...baseAgent, id: peerAgentId }; + mockAgentService.getById.mockImplementation(async (id: string) => { + if (id === peerAgentId) return peerAgent; + if (id === agentId) { + return { ...baseAgent, permissions: { canCreateAgents: false } }; + } + return null; + }); + mockAccessService.hasPermission.mockResolvedValue(false); + + const app = await createApp({ + type: "agent", + agentId, + companyId, + runId: "run-1", + source: "agent_key", + }); + + const res = await request(app).get(`/api/agents/${peerAgentId}/configuration`); + + expect(res.status).toBe(403); + }); + + it("allows an agent actor with agents:create grant to read peer config", async () => { + // When an agent actor has an explicit agents:create grant in the + // access service, the read gate must let them through. + const peerAgentId = "44444444-4444-4444-8444-444444444444"; + const peerAgent = { ...baseAgent, id: peerAgentId }; + mockAgentService.getById.mockImplementation(async (id: string) => { + if (id === peerAgentId) return peerAgent; + if (id === agentId) { + return { ...baseAgent, permissions: { canCreateAgents: false } }; + } + return null; + }); + mockAccessService.hasPermission.mockImplementation( + async (_companyId: string, _principalType: string, principalId: string, key: string) => { + return principalId === agentId && key === "agents:create"; + }, + ); + + const app = await createApp({ + type: "agent", + agentId, + companyId, + runId: "run-1", + source: "agent_key", + }); + + const res = await request(app).get(`/api/agents/${peerAgentId}/configuration`); + + expect(res.status).toBe(200); + }); + }); + it("rejects heartbeat cancellation outside the caller company scope", async () => { mockHeartbeatService.getRun.mockResolvedValue({ id: "run-1", diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index fb52036d..0a9688ac 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -684,7 +684,36 @@ export function agentRoutes( } async function assertCanReadConfigurations(req: Request, companyId: string) { - return assertCanCreateAgentsForCompany(req, companyId); + // Reading agent configurations, skills, and config revisions is a + // read-only operation available to any board (human) member of the + // company. Responses go through `redactAgentConfiguration` so secrets + // are never exposed. Mutations and environment probes still gate on + // agents:create via assertCanCreateAgentsForCompany / assertCanUpdateAgent. + // + // For AGENT actors we keep the previous, stricter gate: an agent must + // either have an explicit `agents:create` grant or the legacy + // `canCreateAgents` permission on its own record. Agents are + // non-human principals — they should not be able to introspect peer + // agents' configurations just by virtue of being in the same company. + assertCompanyAccess(req, companyId); + if (req.actor.type === "agent") { + if (!req.actor.agentId) throw forbidden("Agent authentication required"); + const actorAgent = await svc.getById(req.actor.agentId); + if (!actorAgent || actorAgent.companyId !== companyId) { + throw forbidden("Agent key cannot access another company"); + } + const allowedByGrant = await access.hasPermission( + companyId, + "agent", + actorAgent.id, + "agents:create", + ); + if (!allowedByGrant && !canCreateAgents(actorAgent)) { + throw forbidden("Missing permission: can create agents"); + } + return actorAgent; + } + return null; } async function getAccessibleAgent(req: Request, res: Response, id: string) { @@ -701,13 +730,27 @@ export function agentRoutes( } async function actorCanReadConfigurationsForCompany(req: Request, companyId: string) { - assertCompanyAccess(req, companyId); - const decision = await access.decide({ - actor: req.actor, - action: "agent_config:read", - resource: { type: "company", companyId }, - }); - return decision.allowed; + // Mirrors assertCanReadConfigurations but returns a boolean instead of + // throwing. Board actors only need company access; agent actors must + // still pass the agents:create gate (explicit grant or canCreateAgents + // on their own record) so peer agents cannot snoop each others' + // configurations. + try { + assertCompanyAccess(req, companyId); + } catch { + return false; + } + if (req.actor.type === "board") return true; + if (!req.actor.agentId) return false; + const actorAgent = await svc.getById(req.actor.agentId); + if (!actorAgent || actorAgent.companyId !== companyId) return false; + const allowedByGrant = await access.hasPermission( + companyId, + "agent", + actorAgent.id, + "agents:create", + ); + return allowedByGrant || canCreateAgents(actorAgent); } async function buildSkippedWakeupResponse( @@ -1519,7 +1562,7 @@ export function agentRoutes( async (req, res) => { const companyId = req.params.companyId as string; const type = assertKnownAdapterType(req.params.type as string); - await assertCanReadConfigurations(req, companyId); + await assertCanCreateAgentsForCompany(req, companyId); const adapter = requireServerAdapter(type);