mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-16 02:40:39 +09:00
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies. > - The REST API is the control-plane boundary for companies, agents, plugins, adapters, costs, invites, and issue mutations. > - Several routes still relied on broad board or company access checks without consistently enforcing the narrower actor, company, and active-checkout boundaries those operations require. > - That can allow agents or non-admin users to mutate sensitive resources outside the intended governance path. > - This pull request hardens the route authorization layer and adds regression coverage for the audited API surfaces. > - The benefit is tighter multi-company isolation, safer plugin and adapter administration, and stronger enforcement of active issue ownership. ## What Changed - Added route-level authorization checks for budgets, plugin administration/scoped routes, adapter management, company import/export, direct agent creation, invite test resolution, and issue mutation/write surfaces. - Enforced active checkout ownership for agent-authenticated issue mutations, while preserving explicit management overrides for permitted managers. - Restricted sensitive adapter and plugin management operations to instance-admin or properly scoped actors. - Tightened company portability and invite probing routes so agents cannot cross company boundaries. - Updated access constants and the Company Access UI copy for the new active-checkout management grant. - Added focused regression tests covering cross-company denial, agent self-mutation denial, admin-only operations, and active checkout ownership. - Rebased the branch onto `public-gh/master` and fixed validation fallout from the rebase: heartbeat-context route ordering and a company import/export e2e fixture that now opts out of direct-hire approval before using direct agent creation. - Updated onboarding and signoff e2e setup to create seed agents through `/agent-hires` plus board approval, so they remain compatible with the approval-gated new-agent default. - Addressed Greptile feedback by removing a duplicate company export API alias, avoiding N+1 reporting-chain lookups in active-checkout override checks, allowing agent mutations on unassigned `in_progress` issues, and blocking NAT64 invite-probe targets. ## Verification - `pnpm exec vitest run server/src/__tests__/issues-goal-context-routes.test.ts cli/src/__tests__/company-import-export-e2e.test.ts` - `pnpm exec vitest run server/src/__tests__/plugin-routes-authz.test.ts server/src/__tests__/adapter-routes-authz.test.ts server/src/__tests__/agent-permissions-routes.test.ts server/src/__tests__/company-portability-routes.test.ts server/src/__tests__/costs-service.test.ts server/src/__tests__/invite-test-resolution-route.test.ts server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts server/src/__tests__/agent-adapter-validation-routes.test.ts` - `pnpm exec vitest run server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts` - `pnpm exec vitest run server/src/__tests__/invite-test-resolution-route.test.ts` - `pnpm -r typecheck` - `pnpm --filter server typecheck` - `pnpm --filter ui typecheck` - `pnpm build` - `pnpm test:e2e -- tests/e2e/onboarding.spec.ts tests/e2e/signoff-policy.spec.ts` - `pnpm test:e2e -- tests/e2e/signoff-policy.spec.ts` - `pnpm test:run` was also run. It failed under default full-suite parallelism with two order-dependent failures in `plugin-routes-authz.test.ts` and `routines-e2e.test.ts`; both files passed when rerun directly together with `pnpm exec vitest run server/src/__tests__/plugin-routes-authz.test.ts server/src/__tests__/routines-e2e.test.ts`. ## Risks - Medium risk: this changes authorization behavior across multiple sensitive API surfaces, so callers that depended on broad board/company access may now receive `403` or `409` until they use the correct governance path. - Direct agent creation now respects the company-level board-approval requirement; integrations that need pending hires should use `/api/companies/:companyId/agent-hires`. - Active in-progress issue mutations now require checkout ownership or an explicit management override, which may reveal workflow assumptions in older automation. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex, GPT-5 coding agent, tool-using workflow with local shell, Git, GitHub CLI, and repository tests. ## 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 - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
233 lines
7.5 KiB
TypeScript
233 lines
7.5 KiB
TypeScript
import express from "express";
|
|
import request from "supertest";
|
|
import { beforeEach, afterEach, describe, expect, it, vi } from "vitest";
|
|
import type { ServerAdapterModule } from "../adapters/index.js";
|
|
|
|
const mockAgentService = vi.hoisted(() => ({
|
|
create: vi.fn(),
|
|
getById: vi.fn(),
|
|
}));
|
|
|
|
const mockAccessService = vi.hoisted(() => ({
|
|
canUser: vi.fn(),
|
|
hasPermission: vi.fn(),
|
|
ensureMembership: vi.fn(),
|
|
setPrincipalPermission: vi.fn(),
|
|
}));
|
|
|
|
const mockCompanySkillService = vi.hoisted(() => ({
|
|
listRuntimeSkillEntries: vi.fn(),
|
|
resolveRequestedSkillKeys: vi.fn(),
|
|
}));
|
|
|
|
const mockSecretService = vi.hoisted(() => ({
|
|
normalizeAdapterConfigForPersistence: vi.fn(async (_companyId: string, config: Record<string, unknown>) => config),
|
|
resolveAdapterConfigForRuntime: vi.fn(async (_companyId: string, config: Record<string, unknown>) => ({ config })),
|
|
}));
|
|
|
|
const mockAgentInstructionsService = vi.hoisted(() => ({
|
|
materializeManagedBundle: vi.fn(),
|
|
getBundle: vi.fn(),
|
|
readFile: vi.fn(),
|
|
updateBundle: vi.fn(),
|
|
writeFile: vi.fn(),
|
|
deleteFile: vi.fn(),
|
|
exportFiles: vi.fn(),
|
|
ensureManagedBundle: vi.fn(),
|
|
}));
|
|
|
|
const mockBudgetService = vi.hoisted(() => ({
|
|
upsertPolicy: vi.fn(),
|
|
}));
|
|
|
|
const mockHeartbeatService = vi.hoisted(() => ({
|
|
cancelActiveForAgent: vi.fn(),
|
|
}));
|
|
|
|
const mockIssueApprovalService = vi.hoisted(() => ({
|
|
linkManyForApproval: vi.fn(),
|
|
}));
|
|
|
|
const mockApprovalService = vi.hoisted(() => ({
|
|
create: vi.fn(),
|
|
getById: vi.fn(),
|
|
}));
|
|
|
|
const mockInstanceSettingsService = vi.hoisted(() => ({
|
|
getGeneral: vi.fn(async () => ({ censorUsernameInLogs: false })),
|
|
}));
|
|
|
|
const mockLogActivity = vi.hoisted(() => vi.fn());
|
|
|
|
vi.mock("../services/index.js", () => ({
|
|
agentService: () => mockAgentService,
|
|
agentInstructionsService: () => mockAgentInstructionsService,
|
|
accessService: () => mockAccessService,
|
|
approvalService: () => mockApprovalService,
|
|
companySkillService: () => mockCompanySkillService,
|
|
budgetService: () => mockBudgetService,
|
|
heartbeatService: () => mockHeartbeatService,
|
|
issueApprovalService: () => mockIssueApprovalService,
|
|
issueService: () => ({}),
|
|
logActivity: mockLogActivity,
|
|
secretService: () => mockSecretService,
|
|
syncInstructionsBundleConfigFromFilePath: vi.fn((_agent, config) => config),
|
|
workspaceOperationService: () => ({}),
|
|
}));
|
|
|
|
vi.mock("../services/instance-settings.js", () => ({
|
|
instanceSettingsService: () => mockInstanceSettingsService,
|
|
}));
|
|
|
|
function registerModuleMocks() {
|
|
vi.doMock("../services/index.js", () => ({
|
|
agentService: () => mockAgentService,
|
|
agentInstructionsService: () => mockAgentInstructionsService,
|
|
accessService: () => mockAccessService,
|
|
approvalService: () => mockApprovalService,
|
|
companySkillService: () => mockCompanySkillService,
|
|
budgetService: () => mockBudgetService,
|
|
heartbeatService: () => mockHeartbeatService,
|
|
issueApprovalService: () => mockIssueApprovalService,
|
|
issueService: () => ({}),
|
|
logActivity: mockLogActivity,
|
|
secretService: () => mockSecretService,
|
|
syncInstructionsBundleConfigFromFilePath: vi.fn((_agent, config) => config),
|
|
workspaceOperationService: () => ({}),
|
|
}));
|
|
|
|
vi.doMock("../services/instance-settings.js", () => ({
|
|
instanceSettingsService: () => mockInstanceSettingsService,
|
|
}));
|
|
}
|
|
|
|
const externalAdapter: ServerAdapterModule = {
|
|
type: "external_test",
|
|
execute: async () => ({ exitCode: 0, signal: null, timedOut: false }),
|
|
testEnvironment: async () => ({
|
|
adapterType: "external_test",
|
|
status: "pass",
|
|
checks: [],
|
|
testedAt: new Date(0).toISOString(),
|
|
}),
|
|
};
|
|
|
|
const missingAdapterType = "missing_adapter_validation_test";
|
|
|
|
async function createApp() {
|
|
const [{ agentRoutes }, { errorHandler }] = await Promise.all([
|
|
vi.importActual<typeof import("../routes/agents.js")>("../routes/agents.js"),
|
|
vi.importActual<typeof import("../middleware/index.js")>("../middleware/index.js"),
|
|
]);
|
|
const app = express();
|
|
app.use(express.json());
|
|
app.use((req, _res, next) => {
|
|
(req as any).actor = {
|
|
type: "board",
|
|
userId: "local-board",
|
|
companyIds: ["company-1"],
|
|
source: "local_implicit",
|
|
isInstanceAdmin: false,
|
|
};
|
|
next();
|
|
});
|
|
const db = {
|
|
select: vi.fn(() => ({
|
|
from: vi.fn(() => ({
|
|
where: vi.fn(async () => [
|
|
{
|
|
id: "company-1",
|
|
requireBoardApprovalForNewAgents: false,
|
|
},
|
|
]),
|
|
})),
|
|
})),
|
|
};
|
|
app.use("/api", agentRoutes(db as any));
|
|
app.use(errorHandler);
|
|
return app;
|
|
}
|
|
|
|
async function unregisterTestAdapter(type: string) {
|
|
const { unregisterServerAdapter } = await import("../adapters/index.js");
|
|
unregisterServerAdapter(type);
|
|
}
|
|
|
|
describe("agent routes adapter validation", () => {
|
|
beforeEach(async () => {
|
|
vi.resetModules();
|
|
vi.doUnmock("../routes/agents.js");
|
|
vi.doUnmock("../routes/authz.js");
|
|
vi.doUnmock("../middleware/index.js");
|
|
vi.doUnmock("../routes/agents.js");
|
|
registerModuleMocks();
|
|
vi.resetAllMocks();
|
|
mockCompanySkillService.listRuntimeSkillEntries.mockResolvedValue([]);
|
|
mockCompanySkillService.resolveRequestedSkillKeys.mockResolvedValue([]);
|
|
mockAccessService.canUser.mockResolvedValue(true);
|
|
mockAccessService.hasPermission.mockResolvedValue(true);
|
|
mockAccessService.ensureMembership.mockResolvedValue(undefined);
|
|
mockAccessService.setPrincipalPermission.mockResolvedValue(undefined);
|
|
mockLogActivity.mockResolvedValue(undefined);
|
|
mockAgentService.create.mockImplementation(async (_companyId: string, input: Record<string, unknown>) => ({
|
|
id: "11111111-1111-4111-8111-111111111111",
|
|
companyId: "company-1",
|
|
name: String(input.name ?? "Agent"),
|
|
urlKey: "agent",
|
|
role: String(input.role ?? "general"),
|
|
title: null,
|
|
icon: null,
|
|
status: "idle",
|
|
reportsTo: null,
|
|
capabilities: null,
|
|
adapterType: String(input.adapterType ?? "process"),
|
|
adapterConfig: (input.adapterConfig as Record<string, unknown> | undefined) ?? {},
|
|
runtimeConfig: (input.runtimeConfig as Record<string, unknown> | undefined) ?? {},
|
|
budgetMonthlyCents: 0,
|
|
spentMonthlyCents: 0,
|
|
pauseReason: null,
|
|
pausedAt: null,
|
|
permissions: { canCreateAgents: false },
|
|
lastHeartbeatAt: null,
|
|
metadata: null,
|
|
createdAt: new Date(),
|
|
updatedAt: new Date(),
|
|
}));
|
|
await unregisterTestAdapter("external_test");
|
|
await unregisterTestAdapter(missingAdapterType);
|
|
});
|
|
|
|
afterEach(async () => {
|
|
await unregisterTestAdapter("external_test");
|
|
await unregisterTestAdapter(missingAdapterType);
|
|
});
|
|
|
|
it("creates agents for dynamically registered external adapter types", async () => {
|
|
const { registerServerAdapter } = await import("../adapters/index.js");
|
|
registerServerAdapter(externalAdapter);
|
|
|
|
const app = await createApp();
|
|
const res = await request(app)
|
|
.post("/api/companies/company-1/agents")
|
|
.send({
|
|
name: "External Agent",
|
|
adapterType: "external_test",
|
|
});
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(201);
|
|
expect(res.body.adapterType).toBe("external_test");
|
|
});
|
|
|
|
it("rejects unknown adapter types even when schema accepts arbitrary strings", async () => {
|
|
const app = await createApp();
|
|
const res = await request(app)
|
|
.post("/api/companies/company-1/agents")
|
|
.send({
|
|
name: "Missing Adapter",
|
|
adapterType: missingAdapterType,
|
|
});
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(422);
|
|
expect(String(res.body.error ?? res.body.message ?? "")).toContain(`Unknown adapter type: ${missingAdapterType}`);
|
|
});
|
|
});
|