mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-15 10:30:37 +09:00
Harden API route authorization boundaries (#4122)
## 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>
This commit is contained in:
parent
549ef11c14
commit
7a329fb8bb
22 changed files with 1903 additions and 138 deletions
|
|
@ -26,7 +26,14 @@ import { Router } from "express";
|
|||
import type { Request, Response } from "express";
|
||||
import { and, desc, eq, gte } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { companies, pluginLogs, pluginWebhookDeliveries } from "@paperclipai/db";
|
||||
import {
|
||||
agents,
|
||||
companies,
|
||||
heartbeatRuns,
|
||||
pluginLogs,
|
||||
pluginWebhookDeliveries,
|
||||
projects,
|
||||
} from "@paperclipai/db";
|
||||
import type {
|
||||
PluginApiRouteDeclaration,
|
||||
PluginStatus,
|
||||
|
|
@ -59,7 +66,7 @@ import {
|
|||
getActorInfo,
|
||||
} from "./authz.js";
|
||||
import { validateInstanceConfig } from "../services/plugin-config-validator.js";
|
||||
import { forbidden, notFound, unauthorized, unprocessable } from "../errors.js";
|
||||
import { badRequest, forbidden, notFound, unauthorized, unprocessable } from "../errors.js";
|
||||
|
||||
/** UI slot declaration extracted from plugin manifest */
|
||||
type PluginUiSlotDeclaration = NonNullable<NonNullable<PaperclipPluginManifestV1["ui"]>["slots"]>[number];
|
||||
|
|
@ -548,6 +555,52 @@ export function pluginRoutes(
|
|||
})));
|
||||
}
|
||||
|
||||
function assertPluginBridgeScope(req: Request, companyId: unknown): string | undefined {
|
||||
if (companyId === undefined || companyId === null) {
|
||||
assertInstanceAdmin(req);
|
||||
return undefined;
|
||||
}
|
||||
if (typeof companyId !== "string" || companyId.trim().length === 0) {
|
||||
throw badRequest('"companyId" must be a non-empty string when provided');
|
||||
}
|
||||
assertCompanyAccess(req, companyId);
|
||||
return companyId;
|
||||
}
|
||||
|
||||
async function validateToolRunContextScope(runContext: ToolRunContext): Promise<string | null> {
|
||||
const [agent] = await db
|
||||
.select({ companyId: agents.companyId })
|
||||
.from(agents)
|
||||
.where(eq(agents.id, runContext.agentId))
|
||||
.limit(1);
|
||||
if (!agent || agent.companyId !== runContext.companyId) {
|
||||
return '"runContext.agentId" does not belong to "runContext.companyId"';
|
||||
}
|
||||
|
||||
const [run] = await db
|
||||
.select({ companyId: heartbeatRuns.companyId, agentId: heartbeatRuns.agentId })
|
||||
.from(heartbeatRuns)
|
||||
.where(eq(heartbeatRuns.id, runContext.runId))
|
||||
.limit(1);
|
||||
if (!run || run.companyId !== runContext.companyId) {
|
||||
return '"runContext.runId" does not belong to "runContext.companyId"';
|
||||
}
|
||||
if (run.agentId !== runContext.agentId) {
|
||||
return '"runContext.runId" does not belong to "runContext.agentId"';
|
||||
}
|
||||
|
||||
const [project] = await db
|
||||
.select({ companyId: projects.companyId })
|
||||
.from(projects)
|
||||
.where(eq(projects.id, runContext.projectId))
|
||||
.limit(1);
|
||||
if (!project || project.companyId !== runContext.companyId) {
|
||||
return '"runContext.projectId" does not belong to "runContext.companyId"';
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* GET /api/plugins
|
||||
*
|
||||
|
|
@ -742,6 +795,11 @@ export function pluginRoutes(
|
|||
}
|
||||
|
||||
assertCompanyAccess(req, runContext.companyId);
|
||||
const scopeError = await validateToolRunContextScope(runContext);
|
||||
if (scopeError) {
|
||||
res.status(403).json({ error: scopeError });
|
||||
return;
|
||||
}
|
||||
|
||||
// Verify the tool exists
|
||||
const registeredTool = toolDeps.toolDispatcher.getTool(tool);
|
||||
|
|
@ -1020,9 +1078,7 @@ export function pluginRoutes(
|
|||
return;
|
||||
}
|
||||
|
||||
if (body.companyId) {
|
||||
assertCompanyAccess(req, body.companyId);
|
||||
}
|
||||
assertPluginBridgeScope(req, body.companyId);
|
||||
|
||||
try {
|
||||
const result = await bridgeDeps.workerManager.call(
|
||||
|
|
@ -1103,9 +1159,7 @@ export function pluginRoutes(
|
|||
return;
|
||||
}
|
||||
|
||||
if (body.companyId) {
|
||||
assertCompanyAccess(req, body.companyId);
|
||||
}
|
||||
assertPluginBridgeScope(req, body.companyId);
|
||||
|
||||
try {
|
||||
const result = await bridgeDeps.workerManager.call(
|
||||
|
|
@ -1186,9 +1240,7 @@ export function pluginRoutes(
|
|||
renderEnvironment?: PluginLauncherRenderContextSnapshot | null;
|
||||
} | undefined;
|
||||
|
||||
if (body?.companyId) {
|
||||
assertCompanyAccess(req, body.companyId);
|
||||
}
|
||||
assertPluginBridgeScope(req, body?.companyId);
|
||||
|
||||
try {
|
||||
const result = await bridgeDeps.workerManager.call(
|
||||
|
|
@ -1265,9 +1317,7 @@ export function pluginRoutes(
|
|||
renderEnvironment?: PluginLauncherRenderContextSnapshot | null;
|
||||
} | undefined;
|
||||
|
||||
if (body?.companyId) {
|
||||
assertCompanyAccess(req, body.companyId);
|
||||
}
|
||||
assertPluginBridgeScope(req, body?.companyId);
|
||||
|
||||
try {
|
||||
const result = await bridgeDeps.workerManager.call(
|
||||
|
|
@ -2140,7 +2190,7 @@ export function pluginRoutes(
|
|||
* - 400 if job not found, not active, already running, or worker unavailable
|
||||
*/
|
||||
router.post("/plugins/:pluginId/jobs/:jobId/trigger", async (req, res) => {
|
||||
assertBoardOrgAccess(req);
|
||||
assertInstanceAdmin(req);
|
||||
if (!jobDeps) {
|
||||
res.status(501).json({ error: "Job scheduling is not enabled" });
|
||||
return;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue