mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 22:56:58 +01:00
fix(authz): require board authority for agent budget changes on every route (#20213)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Budgets stop agent spending. The budget hard stop is a control-plane invariant > - The budget routes (`PATCH /api/agents/:id/budgets`, `PATCH /api/companies/:id/budgets`) are board-only > - `PATCH /api/agents/:id` also accepts `budgetMonthlyCents`, and the configuration service writes the agent budget policy from it. That route had no board check for the field > - So an agent, such as a CEO, a manager or the agent itself, can change a monthly budget without a board user. This is a second route around a board-only control, like the pause bypass that #20164 fixed > - This pull request applies the budget route authority to every route that writes an agent budget, and gives agents the final board-only denial with a link to the budget page (#20151) > - The benefit is that only the board changes budgets, and an agent that asks gets a final answer that it can pass to the person ## Linked Issues or Issue Description Refs #17857 (earlier fix, now in conflict with master; this PR rebases it and adds the budget page link). Refs #20164 and #20151 (same pattern for PATCH status and board-only denials). **What happened** An agent can send `PATCH /api/agents/{id}` with `budgetMonthlyCents`. The route accepts it, and the configuration service writes the agent budget policy. This works for the agent's own budget and, with update permission, for other agents. The same field also reaches budget policy through a configuration rollback, direct agent creation, an immediate hire and a built-in agent provision. A chat eval (`budgets-costs.resolve-budget`) found the route. **Expected behavior** Only a board user can change an active budget, as on the dedicated budget routes. An agent gets `403` with code `board_access_required`. The denial says it is final and links the agent's budget tab (`/<prefix>/agents/<id>/budgets`) or the company budgets page (`/<prefix>/activity/budgets`). An agent can still propose a budget in a hire that waits for board approval. The approving board user activates it. **Steps to reproduce** 1. On master, authenticate as an agent of a company (for example the CEO). 2. Send `PATCH /api/agents/{agentId}` with body `{"budgetMonthlyCents": 5000}`, for its own id or for another agent. 3. See `200`. The agent budget policy changes with no board action. 4. The new route tests reproduce this: on master source they fail with `expected 200 to be 403`. ## What Changed - `PATCH /api/agents/:id`: a changed `budgetMonthlyCents` requires a board actor. An unchanged amount with other edits still works. - Configuration rollback: a rollback that changes the budget requires a board actor. - `POST /api/companies/:id/agents` and an immediate `agent-hires`: a positive budget requires a board actor. A hire that needs board approval keeps the proposed budget and does not activate it before approval. - Built-in agent provision: a budget override requires a board actor. - `prepareAgentConfiguration` rejects an agent-originated budget change again (defense in depth). An agent rollback with an unchanged amount does not rewrite the policy. - Board writes keep the actual operator identity. The initial hire budget is saved in the hire transaction. - `assertBoard` and `boardAccessRequired` take an optional board page. Budget denials link the agent's budget tab or the company budgets page, not the general agent profile. - Agent access map updated. - This includes the four commits of #17857 (f970f6b01f,8c06aba41c,e5682d22fa,b6cd49d4b6), rebased onto master with the conflicts resolved, then squashed. ## Verification - Red on master source, same tests: 12 budget tests fail. The PATCH tests fail with `expected 200 to be 403` for the agent's own budget and for a CEO on another agent. Built-in provision fails with `expected 200 to be 403`. Creation and immediate hire fail with `expected 201 to be 403`. - Green with the fix: `cd server && npx vitest run src/__tests__/agent-permissions-routes.test.ts src/__tests__/built-in-agent-routes.test.ts src/__tests__/built-in-agents.test.ts src/__tests__/agent-hire-idempotency-routes.test.ts src/__tests__/board-access-denial.test.ts src/__tests__/agent-budget-configuration.test.ts`: 222 passed. - `src/modules/agent-lifecycle/__tests__/lifecycle.test.ts` (embedded Postgres): 76 passed. - `agent-skills-routes`, `agent-adapter-validation-routes`, `openapi-routes`, `agent-cross-tenant-authz-routes`: 275 passed. - `node scripts/check-maps.mjs`: passed. `git diff --check`: passed. - Server `tsc --noEmit`: no errors in changed files. One error in `native-session-executor.ts` comes from a linked runner build in the local worktree, not from this change. CI runs the full typecheck. ## Risks - Agent clients that set an active budget directly now get `403`. They must propose the budget in an approval-gated hire, or ask a board user. - Agent creation with no budget or a zero budget is not changed. Pending hire budgets do not activate before approval. - Trusted internal plugin provisioning keeps its prior behavior and gets no board identity. - No schema or migration change. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), with tool use and code execution, in Claude Code. The rebased commits from #17857 were first written with OpenAI `gpt-6.1-sol` through Codex. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
b1049b174e
commit
6383eb8033
14 files changed
+226
-41
No files matched your search
@@ -0,0 +1,21 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import type { AgentConfigurationRecord } from "../lib/agent-records.js";
|
||||
import { prepareAgentConfiguration } from "../services/agent-configuration.js";
|
||||
|
||||
const existing = { id: "agent", companyId: "company", status: "idle", budgetMonthlyCents: 500 } as AgentConfigurationRecord;
|
||||
// Preparation queues retention work through the database before reading the patch.
|
||||
const fakeDb = () => ({ execute: async () => [] }) as unknown as Db;
|
||||
describe("budget authority in configuration preparation", () => {
|
||||
it("rejects agent-originated budget changes before persistence or policy audit", async () => {
|
||||
const db = fakeDb();
|
||||
await expect(prepareAgentConfiguration(db, existing, { budgetMonthlyCents: 5000 }, { recordRevision: { createdByAgentId: "ceo" } })).rejects.toMatchObject({ status: 403 });
|
||||
await expect(prepareAgentConfiguration(db, existing, { budgetMonthlyCents: 0 }, { recordRevision: { createdByAgentId: "ceo" } })).rejects.toMatchObject({ status: 403 });
|
||||
});
|
||||
it("removes an unchanged legacy budget from agent rollback so it cannot rewrite policy or invent a board actor", async () => {
|
||||
expect(await prepareAgentConfiguration(fakeDb(), existing, { budgetMonthlyCents: 500 }, { recordRevision: { createdByAgentId: "ceo", source: "rollback" } })).toEqual({});
|
||||
});
|
||||
it("preserves the actual board-authorized amount for the existing policy writer", async () => {
|
||||
expect(await prepareAgentConfiguration(fakeDb(), existing, { budgetMonthlyCents: 5000 }, { recordRevision: { createdByUserId: "operator-user" } })).toEqual({ budgetMonthlyCents: 5000 });
|
||||
});
|
||||
});
|
||||
@@ -185,11 +185,11 @@ describeEmbeddedPostgres("agent hire idempotency within a run", () => {
|
||||
// Same identity, corrected configuration: the agent meant a different hire.
|
||||
const corrected = await request(app)
|
||||
.post(`/api/companies/${company.id}/agent-hires`)
|
||||
.send({ name: "Sam", role: "engineer", adapterType: "process", budgetMonthlyCents: 5000 });
|
||||
.send({ name: "Sam", role: "engineer", adapterType: "process", title: "Store Builder" });
|
||||
expect(corrected.status, JSON.stringify(corrected.body)).toBe(201);
|
||||
expect(corrected.body.idempotent).toBeUndefined();
|
||||
expect(corrected.body.agent?.id).not.toBe(first.body.agent?.id);
|
||||
expect(corrected.body.agent?.budgetMonthlyCents).toBe(5000);
|
||||
expect(corrected.body.agent?.title).toBe("Store Builder");
|
||||
});
|
||||
|
||||
it("still creates a distinct agent for a different hire in the same run", async () => {
|
||||
|
||||
@@ -2642,6 +2642,80 @@ describe("agent permission routes", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it.each([
|
||||
["its own budget", agentId, 0],
|
||||
["its own budget", agentId, 5000],
|
||||
["another agent's budget as a CEO", "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", 5000],
|
||||
])("budget authority: PATCH /agents/:id never lets an agent change %s (%s, %i)", async (_case, actorAgentId, amount) => {
|
||||
mockAgentService.getById.mockImplementation(async (id: string) => id === agentId
|
||||
? { ...baseAgent, budgetMonthlyCents: 500 }
|
||||
: { ...baseAgent, id, role: "ceo", permissions: { canCreateAgents: true } });
|
||||
const app = createApp({ type: "agent", agentId: actorAgentId, companyId, source: "agent_key", runId: "run-1" });
|
||||
const response = await requestApp(app, url => request(url).patch(`/api/agents/${agentId}`).send({ budgetMonthlyCents: amount }));
|
||||
expect(response.status, JSON.stringify(response.body)).toBe(403);
|
||||
expect(response.body.code).toBe("board_access_required");
|
||||
expect(response.body.remediation).toContain("this denial is final");
|
||||
expect(response.body.remediation).toContain(`[Open the agent's budget](/<prefix>/agents/${agentId}/budgets)`);
|
||||
expect(mockAgentService.update).not.toHaveBeenCalled();
|
||||
expect(mockBudgetService.upsertPolicy).not.toHaveBeenCalled();
|
||||
expect(mockLogActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
it("budget authority: a configuration rollback never lets an agent restore a different budget", async () => {
|
||||
mockAgentService.getById.mockResolvedValue({ ...baseAgent, budgetMonthlyCents: 500 });
|
||||
mockAgentService.getConfigRevision.mockResolvedValue({
|
||||
id: "33333333-3333-4333-8333-333333333333",
|
||||
afterConfig: { adapterType: "process", adapterConfig: {}, runtimeConfig: {}, budgetMonthlyCents: 50000 },
|
||||
});
|
||||
const app = createApp({ type: "agent", agentId, companyId, source: "agent_key", runId: "run-1" });
|
||||
const response = await requestApp(app, url => request(url).post(`/api/agents/${agentId}/config-revisions/33333333-3333-4333-8333-333333333333/rollback`));
|
||||
expect(response.status, JSON.stringify(response.body)).toBe(403);
|
||||
expect(response.body.code).toBe("board_access_required");
|
||||
expect(response.body.remediation).toContain(`[Open the agent's budget](/<prefix>/agents/${agentId}/budgets)`);
|
||||
expect(mockAgentService.rollbackConfigRevision).not.toHaveBeenCalled();
|
||||
});
|
||||
it("budget authority: accepts an unchanged legacy amount with an unrelated title edit", async () => {
|
||||
const app = createApp({ type: "agent", agentId, companyId, source: "agent_key" });
|
||||
const response = await requestApp(app, url => request(url).patch(`/api/agents/${agentId}`).send({ budgetMonthlyCents: 0, title: "New title" }));
|
||||
expect(response.status).toBe(200);
|
||||
expect(mockAgentService.update).toHaveBeenCalled();
|
||||
});
|
||||
it.each(["agents", "agent-hires"])("budget authority: rejects agent immediate creation budgets through %s", async route => {
|
||||
mockAccessService.hasPermission.mockResolvedValue(true);
|
||||
const app = createApp({ type: "agent", agentId, companyId, source: "agent_key" });
|
||||
const response = await requestApp(app, url => request(url).post(`/api/companies/${companyId}/${route}`).send({ name: "New employee", role: "engineer", adapterType: "process", budgetMonthlyCents: 5000 }));
|
||||
expect(response.status, JSON.stringify(response.body)).toBe(403);
|
||||
expect(response.body.code).toBe("board_access_required");
|
||||
expect(response.body.remediation).toContain("[Open budgets](/<prefix>/activity/budgets)");
|
||||
expect(mockAgentService.create).not.toHaveBeenCalled();
|
||||
expect(mockBudgetService.upsertPolicy).not.toHaveBeenCalled();
|
||||
expect(mockLogActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
it("budget authority: preserves a proposed budget without applying a policy before approval", async () => {
|
||||
mockAccessService.hasPermission.mockResolvedValue(true);
|
||||
mockAgentService.create.mockResolvedValue({ ...baseAgent, status: "pending_approval", budgetMonthlyCents: 5000 });
|
||||
mockApprovalService.create.mockResolvedValue({ id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", status: "pending", type: "hire_agent", companyId });
|
||||
const app = createApp({ type: "agent", agentId, companyId, source: "agent_key" }, { requireBoardApprovalForNewAgents: true });
|
||||
const response = await requestApp(app, url => request(url).post(`/api/companies/${companyId}/agent-hires`).send({ name: "Proposed employee", role: "engineer", adapterType: "process", budgetMonthlyCents: 5000 }));
|
||||
expect(response.status).toBe(201);
|
||||
expect(response.body.approval.status).toBe("pending");
|
||||
expect(mockApprovalService.create).toHaveBeenCalledWith(companyId, expect.objectContaining({ payload: expect.objectContaining({ budgetMonthlyCents: 5000 }) }));
|
||||
expect(mockBudgetService.upsertPolicy).not.toHaveBeenCalled();
|
||||
});
|
||||
it("budget authority: forwards the actual board identity on legacy updates", async () => {
|
||||
const app = createApp({ type: "board", userId: "operator-user", source: "session", companyIds: [companyId] });
|
||||
const response = await requestApp(app, url => request(url).patch(`/api/agents/${agentId}`).send({ budgetMonthlyCents: 5000 }));
|
||||
expect(response.status).toBe(200);
|
||||
expect(mockAgentService.update).toHaveBeenCalledWith(agentId, expect.objectContaining({ budgetMonthlyCents: 5000 }), expect.objectContaining({ recordRevision: expect.objectContaining({ createdByUserId: "operator-user", createdByAgentId: null }) }));
|
||||
expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ actorType: "user", actorId: "operator-user", agentId: null }));
|
||||
});
|
||||
it.each(["agents", "agent-hires"])("budget authority: records the real board actor for immediate %s budgets", async route => {
|
||||
mockAgentService.create.mockResolvedValue({ ...baseAgent, budgetMonthlyCents: 5000 });
|
||||
const app = createApp({ type: "board", userId: "operator-user", source: "local_implicit", companyIds: [companyId] });
|
||||
const response = await requestApp(app, url => request(url).post(`/api/companies/${companyId}/${route}`).send({ name: "New employee", role: "engineer", adapterType: "process", budgetMonthlyCents: 5000 }));
|
||||
expect(response.status).toBe(201);
|
||||
expect(mockAgentService.create).toHaveBeenCalledWith(companyId, expect.objectContaining({ budgetMonthlyCents: 5000 }), expect.objectContaining({ createdByUserId: "operator-user" }));
|
||||
expect(mockBudgetService.upsertPolicy).not.toHaveBeenCalled();
|
||||
});
|
||||
it("rejects heartbeat cancellation outside the caller company scope", async () => {
|
||||
mockHeartbeatService.getRun.mockResolvedValue({
|
||||
id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa",
|
||||
|
||||
@@ -204,6 +204,16 @@ describe("built-in agent routes", () => {
|
||||
}));
|
||||
});
|
||||
|
||||
it.each([0, 5000])("rejects an agent budget override through built-in provision (%s)", async budgetMonthlyCents => {
|
||||
const app = await createApp({ type: "agent", agentId: "agent-1", companyId, source: "agent_key" });
|
||||
const response = await request(app).post(`/api/companies/${companyId}/built-in-agents/briefs/provision`).send({ budgetMonthlyCents });
|
||||
expect(response.status).toBe(403);
|
||||
expect(response.body.code).toBe("board_access_required");
|
||||
expect(response.body.remediation).toContain("[Open budgets](/<prefix>/activity/budgets)");
|
||||
expect(mockBuiltInAgentService.provision).not.toHaveBeenCalled();
|
||||
expect(mockLogActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("denies provision when agents:create is not allowed", async () => {
|
||||
mockAccessService.decide.mockResolvedValue(denyDecision());
|
||||
const app = await createApp({
|
||||
@@ -372,6 +382,7 @@ describe("built-in agent routes", () => {
|
||||
const app = await createApp({
|
||||
type: "agent",
|
||||
agentId: "manager-agent",
|
||||
runId: "native-request-run",
|
||||
companyId,
|
||||
source: "agent_key",
|
||||
});
|
||||
@@ -395,6 +406,7 @@ describe("built-in agent routes", () => {
|
||||
actorType: "agent",
|
||||
actorId: "manager-agent",
|
||||
action: "approval.created",
|
||||
runId: "native-request-run",
|
||||
entityId: "approval-1",
|
||||
}));
|
||||
});
|
||||
|
||||
@@ -266,6 +266,14 @@ describeEmbeddedPostgres("built-in agents", () => {
|
||||
])).toThrow("defaultAdapterType must be allowed");
|
||||
});
|
||||
|
||||
it("retains the actual operator on a provisioned existing agent budget", async () => {
|
||||
const companyId = await seedCompany({ requireApproval: false });
|
||||
const svc = builtInAgentService(db);
|
||||
const original = await svc.ensure(companyId, "briefs");
|
||||
await svc.provision(companyId, "briefs", { budgetMonthlyCents: 5000 }, { requestedByUserId: "budget-operator" });
|
||||
expect(await db.select().from(budgetPolicies).where(eq(budgetPolicies.scopeId, original.agentId!))).toMatchObject([{ amount: 5000, createdByUserId: "budget-operator" }]);
|
||||
});
|
||||
|
||||
it.each([{ platform: "linux", architecture: "x64" }, { platform: "linux", architecture: "arm64" }, { platform: "darwin", architecture: "arm64" }, { platform: "darwin", architecture: "x64" }])("lazily provisions one agent per company/key and updates the same row on setup ($platform/$architecture)", async ({ platform, architecture }) => {
|
||||
runnerTarget.platform = platform;
|
||||
runnerTarget.architecture = architecture;
|
||||
|
||||
Reference in new issue
Block a user