fix: read-only agent config/skill endpoints should not require agents:create (#3725)
## 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) <noreply@anthropic.com>
This commit is contained in:
Jannes Stubbemann
committed by
GitHub
GitHub
parent
c21f70ef1c
commit
3701be76fa
@@ -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",
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user