mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 10:26:26 +01:00
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - In a multi-tenant deployment, route handlers that take a resource id (`issue`, `goal`, `project`, `approval`, etc.) look the resource up by id and then call `assertCompanyAccess` on its `companyId` — 404 if it doesn't exist, 403 if it exists in another tenant > - The split status codes are a classic *existence oracle*: any authenticated user can enumerate ids across tenants by probing for the 403/404 boundary, mapping out which issues, labels, approvals, etc. exist in other customers' tenants even when they cannot read the contents > - The right fix is a single uniform 404 for both "not found" and "found but cross-tenant", which collapses the oracle but still preserves write-path checks (active membership, viewer-readonly) for *authorized* tenants > - This pull request adds a non-throwing `hasCompanyAccess(req, companyId)` helper plus a `getAccessibleResource` wrapper that ~130 handlers across 14 route files now use, folding the access check into the existence check while still running `assertCompanyAccess` for authorized tenants so viewer-readonly / inactive-membership rejections fire unchanged on write paths > - The benefit is closing a multi-tenant information leak without breaking write-path security or single-tenant local-first behavior ## Linked Issues or Issue Description Refs #709 — asks for company-scope regression coverage across approval/activity/access routes, because a subtle route refactor could leak cross-tenant data; this PR hardens exactly those surfaces (uniform 404 across 14 route files including `approvals`, `activity`, `secrets`) and updates cross-tenant expectations in test files. It does not add the full coverage matrix #709 asks for — hence Refs, not Closes. No existing issue covers the oracle itself — described in-PR: - Route handlers returned 404 for "not found" but 403 for "exists in another tenant", a classic *existence oracle*: any authenticated user could enumerate ids across tenants by probing the 403/404 boundary. - That maps out which issues, labels, approvals, etc. exist in other customers' tenants even when their contents are unreadable. - Fix: a uniform 404 for both cases, while keeping write-path checks (active membership, viewer-readonly) for authorized tenants. ## What Changed - **`server/src/routes/authz.ts`** — new `hasCompanyAccess(req, companyId): boolean` helper alongside the existing `assertCompanyAccess`. Docstring spells out the two-step pattern (404 gate, then `assertCompanyAccess` for write-path checks). The helper mirrors `assertCompanyAccess`'s company-scope semantics exactly — in particular, signed-in instance admins do **not** get blanket access to companies they are not a member of (the repo's `authz-company-access` tests pin that behavior for `assertCompanyAccess`; an earlier draft of the helper accidentally widened it for reads). - **`getAccessibleResource(req, res, lookup, notFoundMessage)`** — the safe thing is now the easy thing. One helper wraps the whole pattern (uniform 404 for missing/cross-tenant, then `assertCompanyAccess` for write-path membership checks) and ~130 handlers across 14 route files use it: ```ts const goal = await getAccessibleResource(req, res, svc.getById(id), "Goal not found"); if (!goal) return; ``` Files: `activity`, `agents`, `approvals`, `assets`, `costs`, `environments`, `execution-workspaces`, `file-resources`, `goals`, `issue-tree-control`, `issues`, `projects`, `routines`, `secrets`. Handlers with bespoke not-found behavior (the legacy `200 []` contract, audit-logged denials in `file-resources`, null-returning authz helpers) compose `hasCompanyAccess` directly using the documented two-step pattern: ```ts // step 1: close the oracle (uniform 404 for both not-found and cross-tenant) if (!existing || !hasCompanyAccess(req, existing.companyId)) { res.status(404).json({ error: "Goal not found" }); return; } // step 2: enforce write-path membership checks for authorised tenants (no-op on GET) assertCompanyAccess(req, existing.companyId); ``` Routes where `companyId` comes from *request input* (`req.params.companyId`, `req.body.companyId`, e.g. in `companies.ts` and `plugins.ts`) deliberately retain plain `assertCompanyAccess` — there's no existence oracle to close because the companyId is an input, not a discovered value. - **Full-sweep coverage** — a scripted audit of every `assertCompanyAccess(req, <resource>.companyId)` call site in `server/src/routes/` found ~55 lookup-then-assert pairs the first pass missed; all are now gated. Notable ones: the `/secret-provider-configs/:id` CRUD routes, the agents instructions-bundle/config-revision/skills-sync routes (which check access via the `assertCanUpdateAgent` / `assertCanReadAgent` / `assertCanManageInstructionsPath` helpers), `POST /heartbeat-runs/:runId/watchdog-decisions`, `GET /issues/:id/cost-summary`, the environment + environment-lease GET routes, all six issue-tree-control routes, ~24 issue sub-resource routes (document annotations, interactions, approvals links, recovery actions, plan decompositions, lock/unlock), and the three workspace file-resource routes (these throw `notFound` instead of `forbidden` inside their audit-logging wrappers, so denied attempts are still activity-logged server-side while the client sees a uniform 404). - **Helpers made self-defending** — `assertCanUpdateAgent` / `assertCanReadAgent` / `assertCanManageInstructionsPath` (agents) and `assertCanManage{Project,Execution}WorkspaceRuntimeServices` throw `notFound` for cross-tenant resources before their `assertCompanyAccess` step, so a future caller that forgets the route-level gate still can't reopen the oracle. - **Pattern enforcement** — new `authz-existence-oracle-guard.test.ts` statically scans `server/src/routes/*.ts` and fails CI on any `assertCompanyAccess(req, <resource>.companyId)` call that is not preceded by a `hasCompanyAccess` gate, with an explicit allowlist (plus staleness check) for the request-input cases. New routes that regress to the 403/404 split fail the suite with a message pointing at the documented pattern. - **Tests** — cross-tenant expectations updated from 403→404 where routes are now gated; new `hasCompanyAccess` unit tests in `authz-company-access.test.ts` pin the instance-admin/local-implicit/agent/none semantics in lockstep with `assertCompanyAccess`; `write-path-membership.test.ts` (added in an earlier round) confirms viewer/inactive users are still rejected on writes. - **One legacy-contract preserve** — `GET /heartbeat-runs/:runId/issues` still returns `200 []` for both "doesn't exist" and "cross-tenant" so the legacy contract is preserved while the oracle stays closed. ## Verification - `pnpm run typecheck` — PASS. - `pnpm -F @paperclipai/server exec vitest run` — full server suite green locally apart from 4 pre-existing local-environment failures (`paperclip-skill-utils` ×2 and `workspace-runtime` ×1 are cwd/git-environment dependent — verified identical on a clean checkout of the base; `heartbeat-process-recovery` is the known macOS flake). - The new `authz-existence-oracle-guard` test sweeps `server/src/routes/*.ts` and confirms no remaining `assertCompanyAccess(resource.companyId)` site without a `hasCompanyAccess` gate; the only allowlisted holdouts take `companyId` from request input. ## Risks - **API contract narrowing.** Any client that specifically checked for `403` on cross-tenant access now sees `404`. This is a strict narrowing (one status instead of two for the same negative outcome) and matches what a client should expect for any id it can't access. - **Write-path checks preserved.** `assertCompanyAccess` still runs after the 404 gate on write routes, so viewer-readonly / inactive-membership rejections fire unchanged for legitimate users. - **Instance-admin scope unchanged.** `hasCompanyAccess` denies signed-in instance admins without an explicit membership, exactly like `assertCompanyAccess` (pinned by unit tests) — so the gate introduces no new read access for admins. - **Single-tenant local-first deploys** behave identically — the helper short-circuits to `true` for `local_implicit` sessions. - No new env vars, no deployment-mode switch. ## Model Used Claude Opus 4.7 (1M context), extended thinking mode; completeness sweep + instance-admin parity fix by Claude Fable 5 (1M context). ## Checklist - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] Thinking path traces from project context to this change - [x] Model used specified - [x] Checked ROADMAP.md — part of the multi-tenant hardening initiative - [x] Tests run locally and pass - [x] Added/updated cross-tenant 404 expectations across test files - [x] No UI changes - [x] Documented risks above - [x] Will address all Greptile and reviewer comments before merge Part of the multi-tenant hardening initiative — see also #5864 (per-company JWT keys) and #5865 (plugin tables `company_id`). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
318 lines
8.9 KiB
TypeScript
318 lines
8.9 KiB
TypeScript
import { describe, expect, it } from "vitest";
|
|
import { HttpError } from "../errors.js";
|
|
import { assertBoardOrgAccess, assertCompanyAccess, hasBoardOrgAccess, hasCompanyAccess } from "../routes/authz.js";
|
|
|
|
function makeReq(input: {
|
|
method?: string;
|
|
actor: Express.Request["actor"];
|
|
}) {
|
|
return {
|
|
method: input.method ?? "GET",
|
|
actor: input.actor,
|
|
} as Express.Request;
|
|
}
|
|
|
|
describe("assertCompanyAccess", () => {
|
|
it("allows viewer memberships to read", () => {
|
|
const req = makeReq({
|
|
method: "GET",
|
|
actor: {
|
|
type: "board",
|
|
userId: "user-1",
|
|
source: "session",
|
|
companyIds: ["company-1"],
|
|
memberships: [
|
|
{ companyId: "company-1", membershipRole: "viewer", status: "active" },
|
|
],
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).not.toThrow();
|
|
});
|
|
|
|
it("rejects viewer memberships for writes", () => {
|
|
const req = makeReq({
|
|
method: "PATCH",
|
|
actor: {
|
|
type: "board",
|
|
userId: "user-1",
|
|
source: "session",
|
|
companyIds: ["company-1"],
|
|
memberships: [
|
|
{ companyId: "company-1", membershipRole: "viewer", status: "active" },
|
|
],
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).toThrow("Viewer access is read-only");
|
|
});
|
|
|
|
it("rejects writes when membership details are present but omit the target company", () => {
|
|
const req = makeReq({
|
|
method: "POST",
|
|
actor: {
|
|
type: "board",
|
|
userId: "user-1",
|
|
source: "session",
|
|
companyIds: ["company-1"],
|
|
memberships: [],
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).toThrow("User does not have active company access");
|
|
});
|
|
|
|
it("allows legacy board actors that only provide company ids", () => {
|
|
const req = makeReq({
|
|
method: "POST",
|
|
actor: {
|
|
type: "board",
|
|
userId: "user-1",
|
|
source: "session",
|
|
companyIds: ["company-1"],
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).not.toThrow();
|
|
});
|
|
|
|
it("rejects signed-in instance admins without explicit company access", () => {
|
|
const req = makeReq({
|
|
method: "GET",
|
|
actor: {
|
|
type: "board",
|
|
userId: "admin-1",
|
|
source: "session",
|
|
isInstanceAdmin: true,
|
|
companyIds: [],
|
|
memberships: [],
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).toThrow("User does not have access to this company");
|
|
});
|
|
|
|
it("allows local trusted board access without explicit membership", () => {
|
|
const req = makeReq({
|
|
method: "GET",
|
|
actor: {
|
|
type: "board",
|
|
userId: "local-board",
|
|
source: "local_implicit",
|
|
isInstanceAdmin: true,
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).not.toThrow();
|
|
});
|
|
|
|
it("fails closed when an on-behalf-of agent lacks a responsible user membership snapshot", () => {
|
|
const req = makeReq({
|
|
method: "GET",
|
|
actor: {
|
|
type: "agent",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
onBehalfOfUserId: "user-1",
|
|
onBehalfOfMemberships: [],
|
|
source: "agent_jwt",
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).toThrow(HttpError);
|
|
try {
|
|
assertCompanyAccess(req, "company-1");
|
|
} catch (err) {
|
|
expect((err as HttpError).details).toMatchObject({ code: "RESPONSIBLE_USER_UNAVAILABLE" });
|
|
}
|
|
});
|
|
|
|
it("rejects on-behalf-of agent writes when the responsible user is read-only", () => {
|
|
const req = makeReq({
|
|
method: "PATCH",
|
|
actor: {
|
|
type: "agent",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
onBehalfOfUserId: "user-1",
|
|
onBehalfOfMemberships: [
|
|
{ companyId: "company-1", membershipRole: "viewer", status: "active" },
|
|
],
|
|
source: "agent_jwt",
|
|
},
|
|
});
|
|
|
|
try {
|
|
assertCompanyAccess(req, "company-1");
|
|
} catch (err) {
|
|
expect((err as HttpError).status).toBe(403);
|
|
expect((err as HttpError).details).toMatchObject({ code: "RESPONSIBLE_USER_UNAUTHORIZED" });
|
|
return;
|
|
}
|
|
throw new Error("Expected responsible-user company access denial");
|
|
});
|
|
|
|
it("logs only in shadow mode for responsible-user company access denials", () => {
|
|
const previous = process.env.PAPERCLIP_RESPONSIBLE_USER_AUTHZ_SHADOW;
|
|
process.env.PAPERCLIP_RESPONSIBLE_USER_AUTHZ_SHADOW = "true";
|
|
try {
|
|
const req = makeReq({
|
|
method: "PATCH",
|
|
actor: {
|
|
type: "agent",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
onBehalfOfUserId: "user-1",
|
|
onBehalfOfMemberships: [],
|
|
source: "agent_jwt",
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).not.toThrow();
|
|
} finally {
|
|
if (previous === undefined) delete process.env.PAPERCLIP_RESPONSIBLE_USER_AUTHZ_SHADOW;
|
|
else process.env.PAPERCLIP_RESPONSIBLE_USER_AUTHZ_SHADOW = previous;
|
|
}
|
|
});
|
|
|
|
it("allows on-behalf-of agent writes for active non-viewer responsible users", () => {
|
|
const req = makeReq({
|
|
method: "PATCH",
|
|
actor: {
|
|
type: "agent",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
onBehalfOfUserId: "user-1",
|
|
onBehalfOfMemberships: [
|
|
{ companyId: "company-1", membershipRole: "operator", status: "active" },
|
|
],
|
|
source: "agent_jwt",
|
|
},
|
|
});
|
|
|
|
expect(() => assertCompanyAccess(req, "company-1")).not.toThrow();
|
|
});
|
|
});
|
|
|
|
describe("hasCompanyAccess", () => {
|
|
it("allows members of the company", () => {
|
|
const req = makeReq({
|
|
actor: {
|
|
type: "board",
|
|
userId: "user-1",
|
|
source: "session",
|
|
companyIds: ["company-1"],
|
|
memberships: [{ companyId: "company-1", membershipRole: "viewer", status: "active" }],
|
|
},
|
|
});
|
|
|
|
expect(hasCompanyAccess(req, "company-1")).toBe(true);
|
|
});
|
|
|
|
it("denies users from other companies", () => {
|
|
const req = makeReq({
|
|
actor: {
|
|
type: "board",
|
|
userId: "user-1",
|
|
source: "session",
|
|
companyIds: ["company-1"],
|
|
memberships: [{ companyId: "company-1", membershipRole: "operator", status: "active" }],
|
|
},
|
|
});
|
|
|
|
expect(hasCompanyAccess(req, "company-2")).toBe(false);
|
|
});
|
|
|
|
it("denies signed-in instance admins without explicit company access, matching assertCompanyAccess", () => {
|
|
const req = makeReq({
|
|
actor: {
|
|
type: "board",
|
|
userId: "admin-1",
|
|
source: "session",
|
|
isInstanceAdmin: true,
|
|
companyIds: [],
|
|
memberships: [],
|
|
},
|
|
});
|
|
|
|
expect(hasCompanyAccess(req, "company-1")).toBe(false);
|
|
expect(() => assertCompanyAccess(req, "company-1")).toThrow("User does not have access to this company");
|
|
});
|
|
|
|
it("allows local trusted board access without explicit membership", () => {
|
|
const req = makeReq({
|
|
actor: {
|
|
type: "board",
|
|
userId: "local-board",
|
|
source: "local_implicit",
|
|
isInstanceAdmin: true,
|
|
},
|
|
});
|
|
|
|
expect(hasCompanyAccess(req, "company-1")).toBe(true);
|
|
});
|
|
|
|
it("scopes agent actors to their own company", () => {
|
|
const agent = { type: "agent", agentId: "agent-1", companyId: "company-1" } as const;
|
|
|
|
expect(hasCompanyAccess(makeReq({ actor: agent }), "company-1")).toBe(true);
|
|
expect(hasCompanyAccess(makeReq({ actor: agent }), "company-2")).toBe(false);
|
|
});
|
|
|
|
it("denies unauthenticated actors", () => {
|
|
const req = makeReq({ actor: { type: "none" } });
|
|
|
|
expect(hasCompanyAccess(req, "company-1")).toBe(false);
|
|
});
|
|
});
|
|
|
|
describe("assertBoardOrgAccess", () => {
|
|
it("allows signed-in board users with active company access", () => {
|
|
const req = makeReq({
|
|
actor: {
|
|
type: "board",
|
|
userId: "user-1",
|
|
source: "session",
|
|
companyIds: ["company-1"],
|
|
memberships: [{ companyId: "company-1", membershipRole: "operator", status: "active" }],
|
|
isInstanceAdmin: false,
|
|
},
|
|
});
|
|
|
|
expect(hasBoardOrgAccess(req)).toBe(true);
|
|
expect(() => assertBoardOrgAccess(req)).not.toThrow();
|
|
});
|
|
|
|
it("allows instance admins without company memberships", () => {
|
|
const req = makeReq({
|
|
actor: {
|
|
type: "board",
|
|
userId: "admin-1",
|
|
source: "session",
|
|
companyIds: [],
|
|
memberships: [],
|
|
isInstanceAdmin: true,
|
|
},
|
|
});
|
|
|
|
expect(hasBoardOrgAccess(req)).toBe(true);
|
|
expect(() => assertBoardOrgAccess(req)).not.toThrow();
|
|
});
|
|
|
|
it("rejects signed-in users without company access or instance admin rights", () => {
|
|
const req = makeReq({
|
|
actor: {
|
|
type: "board",
|
|
userId: "outsider-1",
|
|
source: "session",
|
|
companyIds: [],
|
|
memberships: [],
|
|
isInstanceAdmin: false,
|
|
},
|
|
});
|
|
|
|
expect(hasBoardOrgAccess(req)).toBe(false);
|
|
expect(() => assertBoardOrgAccess(req)).toThrow("Company membership or instance admin access required");
|
|
});
|
|
});
|