diff --git a/doc/agent-instruction-revisions.md b/doc/agent-instruction-revisions.md index 247a1549cda..a920eaf2bfb 100644 --- a/doc/agent-instruction-revisions.md +++ b/doc/agent-instruction-revisions.md @@ -99,6 +99,13 @@ confirmation inside the commit transaction. Low-trust, task-bridge, and skill-te containment remains enforced. This path enforces the responsible-user ceiling even if the general responsible-user policy is configured in shadow mode. +Peer instruction reads require the existing `agent_config:read` grant or the +same strict delegated target content-edit authority. General same-company agent +visibility is insufficient for bundle files, history, diffs, and candidates. +Read checks do not bypass or consume protected-change consent. Self and board +reads retain their existing company visibility rules; external host bundles keep +their instance-admin and configuration-read restrictions. + ## HTTP and editor Existing company-scoped agent URLs now support: diff --git a/server/src/__tests__/agent-instruction-revisions.test.ts b/server/src/__tests__/agent-instruction-revisions.test.ts index 1588597eea7..77f3592d7ce 100644 --- a/server/src/__tests__/agent-instruction-revisions.test.ts +++ b/server/src/__tests__/agent-instruction-revisions.test.ts @@ -8,6 +8,7 @@ import { agents, agentApiKeys, companies, authUsers, companyMemberships, princip agentInstructionRevisions, agentInstructionHeads, issueThreadInteractions, issues, createDb } from "@paperclipai/db"; import { startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; import { agentInstructionRevisionService } from "../services/agent-instruction-revisions.js"; +import { agentInstructionWorkingCopyService } from "../services/agent-instruction-working-copies.js"; import { instructionBytes, instructionPath, readInstructionBytes } from "../services/agent-instruction-files.js"; import { agentInstructionsService, resolveManagedInstructionsRoot } from "../services/agent-instructions.js"; import type { AuthorizationActor } from "../services/authorization.js"; @@ -60,6 +61,34 @@ describe("canonical instruction revisions", () => { async function save(content: string, baseRevisionId: string | null) { return service.commit({ ...target(), entryFile, content, baseRevisionId, source: "cleanup" }, actor); } + it.each(["current", "history", "revision", "diff", "candidates"])("denies peer %s reads without a configuration grant or responsible-user target access", async (operation) => { + const first = await base(); + await db.delete(principalPermissionGrants).where(eq(principalPermissionGrants.principalId, userId)); + const read = () => { + if (operation === "current") return service.readCurrent(target(), actor); + if (operation === "history") return service.history({ ...target(), entryFile }, actor); + if (operation === "revision") return service.readRevision({ ...target(), entryFile, revisionId: first.revision.id }, actor); + if (operation === "diff") return service.diff({ ...target(), entryFile, fromRevisionId: first.revision.id, toRevisionId: first.revision.id }, actor); + return agentInstructionWorkingCopyService(db).list(companyId, agentId, actor); + }; + await expect(read()).rejects.toMatchObject({ status: 403 }); + await db.insert(principalPermissionGrants).values({ companyId, principalType: "user", principalId: userId, permissionKey: "agents:configure", scope: { agentIds: [actorId] } }); + await expect(read()).rejects.toMatchObject({ status: 403 }); + }); + it("allows delegated peer content reads but preserves explicit agent target restrictions", async () => { + expect(await db.select().from(principalPermissionGrants).where(eq(principalPermissionGrants.principalId, actorId))).toHaveLength(0); + expect((await base()).content).toBe(initial); + await db.insert(principalPermissionGrants).values({ companyId, principalType: "agent", principalId: actorId, permissionKey: "agents:configure", scope: { agentIds: [actorId] } }); + await expect(service.readCurrent(target(), actor)).rejects.toMatchObject({ status: 403 }); + }); + it("allows self and board reads without requiring instruction edit permission", async () => { + await db.delete(principalPermissionGrants).where(eq(principalPermissionGrants.principalId, userId)); + const selfRoot = resolveManagedInstructionsRoot({ id: actorId, companyId, name: "Writer", adapterConfig: {} }); + await fs.mkdir(selfRoot, { recursive: true }); + await fs.writeFile(path.join(selfRoot, "AGENTS.md"), "Self instructions"); + expect((await service.readCurrent({ companyId, agentId: actorId }, actor))?.content).toBe("Self instructions"); + expect((await service.readCurrent(target(), { type: "board", userId, source: "session" }))?.content).toBe(initial); + }); it("seeds configured bytes, preserves immutable history, deduplicates, diffs and restores as a new revision", async () => { const first = await base(); expect(first.content).toBe(initial); @@ -171,6 +200,8 @@ describe("canonical instruction revisions", () => { payload: { version: 1, prompt: "Apply the instruction change?", detailsMarkdown: "```diff\n+approved\n```", target: { type: "custom", key: `agent:${agentId}:instructions`, revisionId: "proposal" } }, result: { version: 1, outcome: "accepted" }, resolvedByUserId: userId, resolvedAt: new Date(), }); + expect((await service.readCurrent(target(), actor))?.revision.id).toBe(first.revision.id); + expect((await db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.id, interactionId)))[0].result).not.toHaveProperty("consumedAt"); await expect(save("approved", randomUUID())).rejects.toMatchObject({ status: 409 }); expect((await db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.id, interactionId)))[0].result).not.toHaveProperty("consumedAt"); const receipt = await save("approved", first.revision.id); diff --git a/server/src/__tests__/agent-instructions-routes.test.ts b/server/src/__tests__/agent-instructions-routes.test.ts index e783d68625b..38c83e3803b 100644 --- a/server/src/__tests__/agent-instructions-routes.test.ts +++ b/server/src/__tests__/agent-instructions-routes.test.ts @@ -30,6 +30,9 @@ vi.mock("../services/agent-instruction-working-copies.js", () => ({ agentInstruc const mockInstructionRevisions = vi.hoisted(() => ({ readCurrent: vi.fn(), commit: vi.fn(), restore: vi.fn(), history: vi.fn(), readRevision: vi.fn(), diff: vi.fn(), materializeCurrent: vi.fn() })); vi.mock("../services/agent-instruction-revisions.js", () => ({ agentInstructionRevisionService: () => mockInstructionRevisions })); +const mockAuthorizeInstructionRead = vi.hoisted(() => vi.fn()); +vi.mock("../services/agent-instruction-authorization.js", () => ({ authorizeInstructionRead: mockAuthorizeInstructionRead })); + const mockAccessService = vi.hoisted(() => ({ canUser: vi.fn(), decide: vi.fn(), @@ -208,6 +211,7 @@ describe("agent instructions bundle routes", () => { vi.doUnmock("../middleware/index.js"); registerModuleMocks(); vi.clearAllMocks(); + mockAuthorizeInstructionRead.mockImplementation(async (_db, actor) => actor); mockInstructionRevisions.readCurrent.mockResolvedValue(null); mockInstructionRevisions.commit.mockResolvedValue({ revision: { id: "33333333-3333-4333-8333-333333333333", entryFile: "AGENTS.md", byteLength: 18 }, content: "# Updated Agent\n", changed: true, materialization: "current" }); mockBuiltInAgentService.ensureCompanyDefaultAgentGrants.mockResolvedValue(0); @@ -294,6 +298,8 @@ describe("agent instructions bundle routes", () => { entryFile: "AGENTS.md", }); expect(mockAgentInstructionsService.getBundle).toHaveBeenCalled(); + expect(mockAuthorizeInstructionRead).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ type: "board" }), + expect.objectContaining({ companyId: "company-1", id: "11111111-1111-4111-8111-111111111111" })); }); it("requires instance-admin access for every external instruction entry point", async () => { @@ -339,6 +345,7 @@ describe("agent instructions bundle routes", () => { expect(mockAgentInstructionsService.updateBundle).not.toHaveBeenCalled(); expect(mockAgentInstructionsService.writeFile).not.toHaveBeenCalled(); expect(mockAgentInstructionsService.deleteFile).not.toHaveBeenCalled(); + expect(mockAuthorizeInstructionRead).not.toHaveBeenCalled(); }); it("treats a host root mislabeled as managed as external", async () => { @@ -391,6 +398,7 @@ describe("agent instructions bundle routes", () => { expect(res.status, JSON.stringify(res.body)).toBe(200); expect(mockAgentInstructionsService.getBundle).toHaveBeenCalled(); + expect(mockAuthorizeInstructionRead).not.toHaveBeenCalled(); }); it("rejects a company admin that requests a new external instruction root", async () => { @@ -492,7 +500,8 @@ describe("agent instructions bundle routes", () => { expect(mockAgentService.create).not.toHaveBeenCalled(); }); - it("denies instruction reads when the visibility policy denies access", async () => { + it.each(["", "/file?path=AGENTS.md", "/file?path=private-support.md"])( + "denies peer instruction reads before any data dispatch: %s", async (suffix) => { mockAgentService.getById.mockImplementation(async (id: string) => { if (id === "agent-reader") { return { @@ -504,11 +513,10 @@ describe("agent instructions bundle routes", () => { } return makeAgent(); }); - mockAccessService.decide.mockResolvedValue({ - allowed: false, - reason: "deny_no_grant", - explanation: "Missing permission: agents:configure or agents:suggest-changes.", - }); + // General agent visibility is allowed. It must not authorize instruction + // metadata, canonical entry bytes, or supporting files from a peer. + const { forbidden } = await vi.importActual("../errors.js"); + mockAuthorizeInstructionRead.mockRejectedValue(forbidden("Missing permission to read peer instructions")); const res = await requestApp( await createApp({ @@ -518,20 +526,18 @@ describe("agent instructions bundle routes", () => { source: "agent_key", }), (baseUrl) => request(baseUrl) - .get("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle"), + .get(`/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle${suffix}`), ); expect(res.status, JSON.stringify(res.body)).toBe(403); expect(res.body.error).toContain("Missing permission"); - expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ - action: "agent:read", - resource: { - type: "agent", - companyId: "company-1", - agentId: "11111111-1111-4111-8111-111111111111", - }, - })); + expect(mockAuthorizeInstructionRead).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + type: "agent", agentId: "agent-reader", + }), { companyId: "company-1", id: "11111111-1111-4111-8111-111111111111" }); expect(mockAgentInstructionsService.getBundle).not.toHaveBeenCalled(); + expect(mockAgentInstructionsService.readFile).not.toHaveBeenCalled(); + expect(mockInstructionRevisions.readCurrent).not.toHaveBeenCalled(); + expect(mockInstructionRevisions.materializeCurrent).not.toHaveBeenCalled(); }); it("allows agents to read their own instructions bundles", async () => { @@ -548,9 +554,12 @@ describe("agent instructions bundle routes", () => { expect(res.status, JSON.stringify(res.body)).toBe(200); expect(mockAgentInstructionsService.getBundle).toHaveBeenCalled(); + expect(mockAuthorizeInstructionRead).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + type: "agent", agentId: "11111111-1111-4111-8111-111111111111", + }), { companyId: "company-1", id: "11111111-1111-4111-8111-111111111111" }); }); - it("allows agents with suggest grants to read peer instructions bundles", async () => { + it.each(["AGENTS.md", "private-support.md"])("allows an authorized peer to read instruction file %s", async (filePath) => { mockAccessService.decide.mockResolvedValue({ allowed: true, reason: "allow_explicit_grant", @@ -578,21 +587,16 @@ describe("agent instructions bundle routes", () => { }), (baseUrl) => request(baseUrl) .get("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle/file") - .query({ path: "AGENTS.md" }), + .query({ path: filePath }), ); expect(res.status, JSON.stringify(res.body)).toBe(200); - expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ - action: "agent:read", - resource: { - type: "agent", - companyId: "company-1", - agentId: "11111111-1111-4111-8111-111111111111", - }, - })); + expect(mockAuthorizeInstructionRead).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + type: "agent", agentId: "coach-agent", + }), { companyId: "company-1", id: "11111111-1111-4111-8111-111111111111" }); expect(mockAgentInstructionsService.readFile).toHaveBeenCalledWith( expect.objectContaining({ id: "11111111-1111-4111-8111-111111111111" }), - "AGENTS.md", + filePath, ); }); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 4fca0a16543..2ac06541672 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -67,6 +67,7 @@ import { validate } from "../middleware/validate.js"; import { inheritNativeRunnerAdapterConfig } from "../services/native-runtime/native-agent-runtime-inheritance.js"; import { agentInstructionRevisionService } from "../services/agent-instruction-revisions.js"; import { agentInstructionWorkingCopyService } from "../services/agent-instruction-working-copies.js"; +import { authorizeInstructionRead } from "../services/agent-instruction-authorization.js"; import { instructionPath } from "../services/agent-instruction-files.js"; import { agentInstructionsBundleMode, deriveBundleState } from "../services/agent-instructions.js"; import { @@ -5081,9 +5082,12 @@ export function agentRoutes( const id = req.params.id as string; const existing = await getAccessibleResource(req, res, svc.getById(id), "Agent not found"); if (!existing) return; - const readDecision = await access.decide({ actor: req.actor, action: "agent:read", resource: { type: "agent", companyId: existing.companyId, agentId: existing.id } }); - if (!readDecision.allowed) throw forbidden(readDecision.explanation, authorizationDeniedDetails(readDecision)); assertExternalInstructionsAdmin(req, existing); + if (agentInstructionsBundleMode(existing) === "external") { + await assertCanReadAgent(req, existing); + } else { + await authorizeInstructionRead(db, req.actor, { companyId: existing.companyId, id: existing.id }); + } if (agentInstructionsBundleMode(existing) !== "external") { const target = { companyId: existing.companyId, agentId: existing.id }; const current = await instructionRevisions.readCurrent(target, req.actor); @@ -5151,9 +5155,12 @@ export function agentRoutes( const id = req.params.id as string; const existing = await getAccessibleResource(req, res, svc.getById(id), "Agent not found"); if (!existing) return; - const readDecision = await access.decide({ actor: req.actor, action: "agent:read", resource: { type: "agent", companyId: existing.companyId, agentId: existing.id } }); - if (!readDecision.allowed) throw forbidden(readDecision.explanation, authorizationDeniedDetails(readDecision)); assertExternalInstructionsAdmin(req, existing); + if (agentInstructionsBundleMode(existing) === "external") { + await assertCanReadAgent(req, existing); + } else { + await authorizeInstructionRead(db, req.actor, { companyId: existing.companyId, id: existing.id }); + } const relativePath = typeof req.query.path === "string" ? req.query.path : ""; if (!relativePath.trim()) { diff --git a/server/src/services/agent-instruction-authorization.ts b/server/src/services/agent-instruction-authorization.ts index 21798941f3f..bc2f4f254dc 100644 --- a/server/src/services/agent-instruction-authorization.ts +++ b/server/src/services/agent-instruction-authorization.ts @@ -134,6 +134,36 @@ export async function resolveInstructionActor( }; } +/** Instruction content is narrower than general same-company agent visibility. */ +export async function authorizeInstructionRead( + db: Connection, + actor: AuthorizationActor, + target: { companyId: string; id: string }, +) { + const bound = await resolveInstructionActor(db, actor); + const access = authorizationService(db); + const resource = { type: "agent" as const, companyId: target.companyId, agentId: target.id }; + const peer = bound.type === "agent" && bound.agentId !== target.id; + let decision = await access.decide({ + actor: bound, + action: peer ? "agent_config:read" : "agent:read", + resource, + scope: { targetAgentId: target.id }, + }); + if (!decision.allowed && peer) { + // Ordinary delegated instruction edits must remain possible without a new + // blanket agent-admin grant. The existing content-edit decision enforces + // current responsible-user target access and explicit agent containment. + // This is a read-only decision: no write, consent bypass, or consumption. + decision = await access.decide({ + actor: bound, action: "agent_instructions:update", resource, + scope: { targetAgentId: target.id, requiresChangeGrant: true }, + }); + } + if (!decision.allowed) throw forbidden(decision.explanation, authorizationDeniedDetails(decision)); + return bound; +} + /** Current target edit access + agent containment + protected-change consent, shared by all writers. */ export async function authorizeInstructionCommit( db: Connection, diff --git a/server/src/services/agent-instruction-revisions.ts b/server/src/services/agent-instruction-revisions.ts index 8ceea0e8f20..2b059bd579e 100644 --- a/server/src/services/agent-instruction-revisions.ts +++ b/server/src/services/agent-instruction-revisions.ts @@ -31,13 +31,9 @@ import { } from "./agent-instruction-files.js"; import { authorizeInstructionCommit, - resolveInstructionActor, + authorizeInstructionRead, } from "./agent-instruction-authorization.js"; -import { - authorizationService, - authorizationDeniedDetails, - type AuthorizationActor, -} from "./authorization.js"; +import type { AuthorizationActor } from "./authorization.js"; type Tx = Parameters[0]>[0]; type Revision = typeof revisions.$inferSelect; @@ -116,22 +112,7 @@ export function agentInstructionRevisionService(db: Db) { actor: AuthorizationActor, target: InstructionTarget, ) { - const bound = await resolveInstructionActor(tx, actor); - const decision = await authorizationService(tx).decide({ - actor: bound, - action: "agent:read", - resource: { - type: "agent", - companyId: target.companyId, - agentId: target.agentId, - }, - }); - if (!decision.allowed) - throw forbidden( - decision.explanation, - authorizationDeniedDetails(decision), - ); - return bound; + return authorizeInstructionRead(tx, actor, { companyId: target.companyId, id: target.agentId }); } // Trusted host-only reset authority. No HTTP or agent tool accepts this actor. async function authorizePluginReset(tx: Tx, actor: PluginResetActor, target: InstructionTarget, agent: typeof agents.$inferSelect) {