From cfe1eaa1766f3cadda96b4a354cfe79cdacffd9e Mon Sep 17 00:00:00 2001 From: Dotta Date: Sun, 27 Sep 2026 22:03:15 -0500 Subject: [PATCH] fix(instructions): enforce scoped authority for peer reads Require configuration-read grants or strict responsible-user target edit authority before exposing peer instruction content, history, and candidates. Preserve self, board, authorized delegated edits, and external host restrictions. Read decisions do not consume or bypass write consent. Co-Authored-By: Paperclip --- doc/agent-instruction-revisions.md | 7 +++ .../agent-instruction-revisions.test.ts | 31 ++++++++++ .../agent-instructions-routes.test.ts | 56 ++++++++++--------- server/src/routes/agents.ts | 15 +++-- .../agent-instruction-authorization.ts | 30 ++++++++++ .../services/agent-instruction-revisions.ts | 25 +-------- 6 files changed, 112 insertions(+), 52 deletions(-) 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) {