mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-23 00:53:38 +02: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>
305 lines
10 KiB
TypeScript
305 lines
10 KiB
TypeScript
import express from "express";
|
|
import request from "supertest";
|
|
import { beforeEach, describe, expect, it, vi } from "vitest";
|
|
|
|
const routineId = "11111111-1111-4111-8111-111111111111";
|
|
const companyId = "22222222-2222-4222-8222-222222222222";
|
|
const otherCompanyId = "33333333-3333-4333-8333-333333333333";
|
|
const agentId = "77777777-7777-4777-8777-777777777777";
|
|
const firstRevisionId = "44444444-4444-4444-8444-444444444444";
|
|
const secondRevisionId = "99999999-9999-4999-8999-999999999999";
|
|
|
|
const mockRoutineService = vi.hoisted(() => ({
|
|
get: vi.fn(),
|
|
getDetail: vi.fn(),
|
|
getDescriptionDocument: vi.fn(),
|
|
update: vi.fn(),
|
|
}));
|
|
const mockAnnotationService = vi.hoisted(() => ({
|
|
listThreadsForRoutineDocument: vi.fn(),
|
|
getThreadForRoutineDocument: vi.fn(),
|
|
createRoutineThread: vi.fn(),
|
|
addRoutineComment: vi.fn(),
|
|
updateRoutineThread: vi.fn(),
|
|
remapOpenThreadsForRoutineDocument: vi.fn(),
|
|
}));
|
|
const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined));
|
|
|
|
const routine = {
|
|
id: routineId,
|
|
companyId,
|
|
title: "Daily summary",
|
|
description: "Alpha selected text omega",
|
|
status: "active",
|
|
assigneeAgentId: agentId,
|
|
latestRevisionId: firstRevisionId,
|
|
latestRevisionNumber: 1,
|
|
};
|
|
|
|
const descriptionDocument = {
|
|
id: "document-1",
|
|
companyId,
|
|
routineId,
|
|
key: "description",
|
|
title: "Routine instructions",
|
|
format: "markdown",
|
|
body: "Alpha selected text omega",
|
|
latestRevisionId: firstRevisionId,
|
|
latestRevisionNumber: 1,
|
|
createdByAgentId: null,
|
|
createdByUserId: "board-user",
|
|
updatedByAgentId: null,
|
|
updatedByUserId: "board-user",
|
|
createdAt: new Date("2026-06-16T12:00:00.000Z"),
|
|
updatedAt: new Date("2026-06-16T12:00:00.000Z"),
|
|
};
|
|
|
|
const updatedDescriptionDocument = {
|
|
...descriptionDocument,
|
|
body: "Alpha updated selected text omega",
|
|
latestRevisionId: secondRevisionId,
|
|
latestRevisionNumber: 2,
|
|
updatedAt: new Date("2026-06-16T12:02:00.000Z"),
|
|
};
|
|
|
|
const selector = {
|
|
quote: { exact: "selected text", prefix: "Alpha ", suffix: " omega" },
|
|
position: { normalizedStart: 6, normalizedEnd: 19, markdownStart: 6, markdownEnd: 19 },
|
|
};
|
|
|
|
const annotationThread = {
|
|
id: "55555555-5555-4555-8555-555555555555",
|
|
companyId,
|
|
issueId: null,
|
|
routineId,
|
|
documentId: descriptionDocument.id,
|
|
documentKey: "description",
|
|
status: "open",
|
|
anchorState: "active",
|
|
anchorConfidence: "exact",
|
|
originalRevisionId: firstRevisionId,
|
|
originalRevisionNumber: 1,
|
|
currentRevisionId: firstRevisionId,
|
|
currentRevisionNumber: 1,
|
|
selectedText: "selected text",
|
|
prefixText: "Alpha ",
|
|
suffixText: " omega",
|
|
normalizedStart: 6,
|
|
normalizedEnd: 19,
|
|
markdownStart: 6,
|
|
markdownEnd: 19,
|
|
anchorSelector: selector,
|
|
createdByAgentId: null,
|
|
createdByUserId: "board-user",
|
|
resolvedByAgentId: null,
|
|
resolvedByUserId: null,
|
|
resolvedAt: null,
|
|
createdAt: new Date("2026-06-16T12:01:00.000Z"),
|
|
updatedAt: new Date("2026-06-16T12:01:00.000Z"),
|
|
};
|
|
|
|
const annotationComment = {
|
|
id: "66666666-6666-4666-8666-666666666666",
|
|
companyId,
|
|
threadId: annotationThread.id,
|
|
issueId: null,
|
|
routineId,
|
|
documentId: descriptionDocument.id,
|
|
body: "Please tighten this",
|
|
authorType: "user",
|
|
authorAgentId: null,
|
|
authorUserId: "board-user",
|
|
createdByRunId: null,
|
|
createdAt: new Date("2026-06-16T12:01:00.000Z"),
|
|
updatedAt: new Date("2026-06-16T12:01:00.000Z"),
|
|
};
|
|
|
|
function registerModuleMocks() {
|
|
vi.doMock("../services/index.js", () => ({
|
|
accessService: () => ({
|
|
canUser: vi.fn(async () => true),
|
|
}),
|
|
documentAnnotationService: () => mockAnnotationService,
|
|
logActivity: mockLogActivity,
|
|
routineService: () => mockRoutineService,
|
|
}));
|
|
}
|
|
|
|
async function createApp(actor: "board" | "agent" = "board", actorCompanyId = companyId) {
|
|
const [{ routineRoutes }, { errorHandler }] = await Promise.all([
|
|
vi.importActual<typeof import("../routes/routines.js")>("../routes/routines.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 = actor === "agent"
|
|
? {
|
|
type: "agent",
|
|
agentId,
|
|
companyId: actorCompanyId,
|
|
runId: "88888888-8888-4888-8888-888888888888",
|
|
}
|
|
: {
|
|
type: "board",
|
|
userId: "board-user",
|
|
companyIds: [actorCompanyId],
|
|
source: "local_implicit",
|
|
isInstanceAdmin: false,
|
|
};
|
|
next();
|
|
});
|
|
app.use("/api", routineRoutes({} as any));
|
|
app.use(errorHandler);
|
|
return app;
|
|
}
|
|
|
|
describe("routine description annotation routes", () => {
|
|
beforeEach(() => {
|
|
vi.resetModules();
|
|
vi.doUnmock("../routes/routines.js");
|
|
vi.doUnmock("../middleware/index.js");
|
|
registerModuleMocks();
|
|
vi.clearAllMocks();
|
|
|
|
mockRoutineService.get.mockResolvedValue(routine);
|
|
mockRoutineService.getDescriptionDocument.mockResolvedValue(updatedDescriptionDocument);
|
|
mockRoutineService.update.mockResolvedValue({
|
|
...routine,
|
|
description: "Alpha updated selected text omega",
|
|
latestRevisionId: secondRevisionId,
|
|
latestRevisionNumber: 2,
|
|
});
|
|
mockAnnotationService.listThreadsForRoutineDocument.mockImplementation(async (
|
|
_routineId: string,
|
|
_key: string,
|
|
options?: { includeComments?: boolean },
|
|
) => (
|
|
options?.includeComments
|
|
? [{ ...annotationThread, comments: [annotationComment] }]
|
|
: [annotationThread]
|
|
));
|
|
mockAnnotationService.getThreadForRoutineDocument.mockResolvedValue({
|
|
...annotationThread,
|
|
comments: [annotationComment],
|
|
});
|
|
mockAnnotationService.createRoutineThread.mockResolvedValue({
|
|
...annotationThread,
|
|
comments: [annotationComment],
|
|
});
|
|
mockAnnotationService.addRoutineComment.mockResolvedValue(annotationComment);
|
|
mockAnnotationService.updateRoutineThread.mockResolvedValue({ ...annotationThread, status: "resolved" });
|
|
mockAnnotationService.remapOpenThreadsForRoutineDocument.mockResolvedValue([
|
|
{
|
|
thread: {
|
|
...annotationThread,
|
|
currentRevisionId: secondRevisionId,
|
|
currentRevisionNumber: 2,
|
|
},
|
|
snapshot: { id: "snapshot-1" },
|
|
},
|
|
]);
|
|
});
|
|
|
|
it("creates, replies to, resolves, and reopens routine description annotation threads", async () => {
|
|
const app = await createApp();
|
|
|
|
const created = await request(app)
|
|
.post(`/api/routines/${routineId}/description/annotations`)
|
|
.send({
|
|
baseRevisionId: firstRevisionId,
|
|
baseRevisionNumber: 1,
|
|
selector,
|
|
body: "Please tighten this",
|
|
})
|
|
.expect(201);
|
|
|
|
expect(created.body.id).toBe(annotationThread.id);
|
|
expect(mockAnnotationService.createRoutineThread).toHaveBeenCalledWith(
|
|
routineId,
|
|
"description",
|
|
expect.objectContaining({ baseRevisionId: firstRevisionId, body: "Please tighten this" }),
|
|
expect.objectContaining({ actorType: "user", userId: "board-user" }),
|
|
);
|
|
expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({
|
|
action: "routine.document_annotation_thread_created",
|
|
entityType: "routine",
|
|
entityId: routineId,
|
|
details: expect.objectContaining({ documentKey: "description", threadId: annotationThread.id }),
|
|
}));
|
|
|
|
await request(app)
|
|
.post(`/api/routines/${routineId}/description/annotations/${annotationThread.id}/comments`)
|
|
.send({ body: "Reply on the same thread" })
|
|
.expect(201);
|
|
expect(mockAnnotationService.addRoutineComment).toHaveBeenCalledWith(
|
|
routineId,
|
|
"description",
|
|
annotationThread.id,
|
|
expect.objectContaining({ body: "Reply on the same thread" }),
|
|
expect.objectContaining({ actorType: "user", userId: "board-user" }),
|
|
);
|
|
expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({
|
|
action: "routine.document_annotation_comment_added",
|
|
details: expect.objectContaining({ documentKey: "description", threadId: annotationThread.id }),
|
|
}));
|
|
|
|
await request(app)
|
|
.patch(`/api/routines/${routineId}/description/annotations/${annotationThread.id}`)
|
|
.send({ status: "resolved" })
|
|
.expect(200);
|
|
expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({
|
|
action: "routine.document_annotation_thread_resolved",
|
|
details: expect.objectContaining({ documentKey: "description", threadId: annotationThread.id }),
|
|
}));
|
|
|
|
mockAnnotationService.updateRoutineThread.mockResolvedValueOnce({ ...annotationThread, status: "open" });
|
|
await request(app)
|
|
.patch(`/api/routines/${routineId}/description/annotations/${annotationThread.id}`)
|
|
.send({ status: "open" })
|
|
.expect(200);
|
|
expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({
|
|
action: "routine.document_annotation_thread_reopened",
|
|
details: expect.objectContaining({ documentKey: "description", threadId: annotationThread.id }),
|
|
}));
|
|
});
|
|
|
|
it("remaps open routine description annotations after routine description revisions", async () => {
|
|
const updated = await request(await createApp())
|
|
.patch(`/api/routines/${routineId}`)
|
|
.send({
|
|
description: "Alpha updated selected text omega",
|
|
baseRevisionId: firstRevisionId,
|
|
})
|
|
.expect(200);
|
|
|
|
expect(updated.body.latestRevisionNumber).toBe(2);
|
|
expect(mockRoutineService.getDescriptionDocument).toHaveBeenCalledWith(routineId);
|
|
expect(mockAnnotationService.remapOpenThreadsForRoutineDocument).toHaveBeenCalledWith({
|
|
routineId,
|
|
key: "description",
|
|
documentId: updatedDescriptionDocument.id,
|
|
nextRevisionId: updatedDescriptionDocument.latestRevisionId,
|
|
nextRevisionNumber: updatedDescriptionDocument.latestRevisionNumber,
|
|
nextBody: updatedDescriptionDocument.body,
|
|
});
|
|
expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({
|
|
action: "routine.document_annotation_remapped",
|
|
entityType: "routine",
|
|
entityId: routineId,
|
|
details: expect.objectContaining({
|
|
documentKey: "description",
|
|
threadId: annotationThread.id,
|
|
}),
|
|
}));
|
|
});
|
|
|
|
it("rejects agent cross-company routine annotation reads", async () => {
|
|
// Cross-tenant requests return 404 (not 403) so the status code cannot be
|
|
// used as an existence oracle for other tenants' routine ids.
|
|
await request(await createApp("agent", otherCompanyId))
|
|
.get(`/api/routines/${routineId}/description/annotations`)
|
|
.expect(404);
|
|
});
|
|
});
|