Files
PaperClipAI/server/src/__tests__/issue-subtree-diagnostics-routes.test.ts
T
Jannes StubbemannandClaude Opus 4.7 7f2ed0ad90 security(server): close cross-tenant existence oracle (404 instead of 403) (#3967)
## 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>
2026-07-14 15:53:09 -07:00

432 lines
14 KiB
TypeScript

import { randomUUID } from "node:crypto";
import express from "express";
import request from "supertest";
import { eq } from "drizzle-orm";
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
import {
activityLog,
agentWakeupRequests,
agents,
companies,
createDb,
heartbeatRuns,
issueComments,
issueRelations,
issues,
projects,
} from "@paperclipai/db";
import { LOW_TRUST_REVIEW_PRESET } from "@paperclipai/shared";
import {
getEmbeddedPostgresTestSupport,
startEmbeddedPostgresTestDatabase,
} from "./helpers/embedded-postgres.js";
import { errorHandler } from "../middleware/index.js";
import { issueRoutes } from "../routes/issues.js";
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
if (!embeddedPostgresSupport.supported) {
console.warn(
`Skipping embedded Postgres subtree diagnostic route tests on this host: ${
embeddedPostgresSupport.reason ?? "unsupported environment"
}`,
);
}
type Db = ReturnType<typeof createDb>;
type CompanyRow = typeof companies.$inferSelect;
type AgentRow = typeof agents.$inferSelect;
type ProjectRow = typeof projects.$inferSelect;
type IssueRow = typeof issues.$inferSelect;
function createApp(db: Db, actor: Express.Request["actor"]) {
const app = express();
app.use(express.json());
app.use((req, _res, next) => {
req.actor = actor;
next();
});
app.use("/api", issueRoutes(db, {} as any));
app.use(errorHandler);
return app;
}
function boardActor(company: CompanyRow): Express.Request["actor"] {
return {
type: "board",
userId: "board-user",
companyIds: [company.id],
memberships: [{ companyId: company.id, membershipRole: "operator", status: "active" }],
isInstanceAdmin: true,
source: "local_implicit",
};
}
function agentActor(company: CompanyRow, agent: AgentRow, runId: string): Express.Request["actor"] {
return {
type: "agent",
agentId: agent.id,
companyId: company.id,
runId,
source: "agent_jwt",
};
}
async function seedCompany(db: Db, label = "Subtree Diagnostics") {
const nonce = randomUUID().slice(0, 8);
const [company] = await db.insert(companies).values({
name: `${label} ${nonce}`,
issuePrefix: `SD${nonce.slice(0, 4).toUpperCase()}`,
defaultResponsibleUserId: "board-user",
}).returning();
return company!;
}
async function seedAgent(db: Db, companyId: string, permissions: Record<string, unknown> = {}) {
const [agent] = await db.insert(agents).values({
companyId,
name: `Agent ${randomUUID().slice(0, 6)}`,
role: "engineer",
adapterType: "process",
adapterConfig: {},
runtimeConfig: {},
permissions,
}).returning();
return agent!;
}
async function seedProject(db: Db, companyId: string, name: string) {
const [project] = await db.insert(projects).values({
companyId,
name,
status: "in_progress",
}).returning();
return project!;
}
async function seedIssue(
db: Db,
input: {
companyId: string;
projectId?: string | null;
title: string;
status?: string;
assigneeAgentId?: string | null;
parentId?: string | null;
},
) {
const [issue] = await db.insert(issues).values({
companyId: input.companyId,
projectId: input.projectId ?? null,
parentId: input.parentId ?? null,
title: input.title,
status: input.status ?? "todo",
priority: "medium",
assigneeAgentId: input.assigneeAgentId ?? null,
responsibleUserId: "board-user",
}).returning();
return issue!;
}
async function blockIssue(db: Db, companyId: string, blockerIssueId: string, blockedIssueId: string) {
await db.insert(issueRelations).values({
companyId,
issueId: blockerIssueId,
relatedIssueId: blockedIssueId,
type: "blocks",
});
}
async function attachMentionScopedLowTrustRun(db: Db, fixture: {
company: CompanyRow;
ownerAgent: AgentRow;
mentionedAgent: AgentRow;
allowedProject: ProjectRow;
root: IssueRow;
}) {
const executionPolicy = {
authorizationPolicy: {
trustBoundary: {
mode: LOW_TRUST_REVIEW_PRESET,
companyId: fixture.company.id,
projectIds: [fixture.allowedProject.id],
issueIds: [],
allowedAgentIds: [],
},
},
};
await db.update(agents).set({
permissions: {
trustPreset: LOW_TRUST_REVIEW_PRESET,
authorizationPolicy: executionPolicy.authorizationPolicy,
},
}).where(eq(agents.id, fixture.mentionedAgent.id));
fixture.mentionedAgent.permissions = {
trustPreset: LOW_TRUST_REVIEW_PRESET,
authorizationPolicy: executionPolicy.authorizationPolicy,
};
await db.insert(issueComments).values({
companyId: fixture.company.id,
issueId: fixture.root.id,
authorAgentId: fixture.ownerAgent.id,
body: `[@Mentioned Agent](agent://${fixture.mentionedAgent.id}) please inspect this root.`,
});
const [run] = await db.insert(heartbeatRuns).values({
companyId: fixture.company.id,
agentId: fixture.mentionedAgent.id,
status: "running",
contextSnapshot: {
issueId: fixture.root.id,
executionPolicy,
},
}).returning();
return run!;
}
describeEmbeddedPostgres("issue subtree diagnostics route", () => {
let db!: Db;
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
beforeAll(async () => {
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-issue-subtree-diagnostics-");
db = createDb(tempDb.connectionString);
}, 20_000);
afterEach(async () => {
await db.delete(activityLog);
await db.delete(agentWakeupRequests);
await db.delete(issueComments);
await db.delete(issueRelations);
await db.delete(heartbeatRuns);
await db.delete(issues);
await db.delete(agents);
await db.delete(projects);
await db.delete(companies);
});
afterAll(async () => {
await tempDb?.cleanup();
});
it("returns visible subtree nodes, blocker edges, wake edges, and a deterministic stall diagnosis", async () => {
const company = await seedCompany(db);
const agent = await seedAgent(db, company.id);
const project = await seedProject(db, company.id, "Core");
const root = await seedIssue(db, {
companyId: company.id,
projectId: project.id,
title: "Blocked root",
status: "blocked",
assigneeAgentId: agent.id,
});
const child = await seedIssue(db, {
companyId: company.id,
projectId: project.id,
parentId: root.id,
title: "Unfinished child blocker",
status: "in_progress",
});
const rawMarker = `RAW-SUBTREE-${randomUUID()}`;
await blockIssue(db, company.id, child.id, root.id);
await db.insert(agentWakeupRequests).values({
companyId: company.id,
agentId: agent.id,
source: "automation",
reason: "issue_commented",
status: "completed",
payload: { issueId: child.id, privateValue: rawMarker },
error: `secret ${rawMarker}`,
requestedAt: new Date(Date.now() - 5_000),
finishedAt: new Date(Date.now() - 1_000),
});
const res = await request(createApp(db, boardActor(company)))
.get(`/api/issues/${root.id}/diagnostics/subtree`);
expect(res.status, JSON.stringify(res.body)).toBe(200);
expect(res.body).toMatchObject({
diagnosis: expect.stringContaining("Blocked root appears to be the subtree stall point"),
likelyReason: expect.stringContaining("Blocked root appears to be the subtree stall point"),
nodeCount: 2,
omittedUnauthorizedNodeCount: 0,
truncated: false,
caps: {
maxDepth: 8,
maxNodes: 100,
maxBlockersPerNode: 20,
maxWakeRequestsPerNode: 5,
maxActivityRecordsPerNode: 5,
lookbackDays: 14,
},
});
expect(res.body.nodes).toHaveLength(2);
const rootNode = res.body.nodes.find((node: { issue: { id: string } }) => node.issue.id === root.id);
const childNode = res.body.nodes.find((node: { issue: { id: string } }) => node.issue.id === child.id);
expect(rootNode).toMatchObject({
diagnosis: expect.stringContaining("blocked by Unfinished child blocker"),
blockers: [expect.objectContaining({ id: child.id, status: "in_progress", isUnresolved: true })],
});
expect(childNode).toMatchObject({
parentId: root.id,
depth: 1,
wakeRequestCount: 1,
wakeEvents: [expect.objectContaining({ kind: "wake_request", reason: "issue_commented", status: "completed" })],
});
expect(res.body.edges).toEqual(expect.arrayContaining([
expect.objectContaining({ kind: "parent", fromIssueId: root.id, toIssueId: child.id }),
expect.objectContaining({ kind: "blocks", fromIssueId: child.id, toIssueId: root.id }),
expect.objectContaining({ kind: "wake_request", issueId: child.id, reason: "issue_commented" }),
]));
const serialized = JSON.stringify(res.body);
expect(serialized).not.toContain(rawMarker);
expect(serialized).not.toContain("\"payload\"");
expect(serialized).not.toContain("\"details\"");
expect(serialized).not.toContain("\"triggerDetail\"");
expect(serialized).not.toContain("\"error\"");
});
it("returns null diagnosis for a quiet unblocked singleton subtree", async () => {
const company = await seedCompany(db);
const project = await seedProject(db, company.id, "Core");
const issue = await seedIssue(db, {
companyId: company.id,
projectId: project.id,
title: "Quiet root",
status: "todo",
});
const res = await request(createApp(db, boardActor(company)))
.get(`/api/issues/${issue.id}/diagnostics/subtree`);
expect(res.status, JSON.stringify(res.body)).toBe(200);
expect(res.body.diagnosis).toBeNull();
expect(res.body.likelyReason).toBeNull();
expect(res.body.nodes).toHaveLength(1);
expect(res.body.nodes[0]).toMatchObject({
diagnosis: null,
likelyReason: null,
blockers: [],
wakeEvents: [],
truncated: false,
});
expect(res.body.edges).toEqual([]);
expect(res.body.truncated).toBe(false);
});
it("caps subtree nodes and reports truncation explicitly", async () => {
const company = await seedCompany(db);
const project = await seedProject(db, company.id, "Core");
const root = await seedIssue(db, {
companyId: company.id,
projectId: project.id,
title: "Wide root",
status: "todo",
});
const childRows = [];
for (let index = 0; index < 100; index += 1) {
childRows.push({
companyId: company.id,
projectId: project.id,
parentId: root.id,
title: `Child ${String(index).padStart(3, "0")}`,
status: "todo",
priority: "medium",
responsibleUserId: "board-user",
});
}
await db.insert(issues).values(childRows);
const res = await request(createApp(db, boardActor(company)))
.get(`/api/issues/${root.id}/diagnostics/subtree`);
expect(res.status, JSON.stringify(res.body)).toBe(200);
expect(res.body.nodes).toHaveLength(100);
expect(res.body.nodeCount).toBe(100);
expect(res.body.truncated).toBe(true);
expect(res.body.truncatedSections).toMatchObject({ nodes: true });
expect(res.body.omittedUnauthorizedNodeCount).toBeNull();
expect(res.body.diagnosis).toContain("bounded to depth 8 and 100 nodes");
});
it("omits subtree nodes outside a mention-scoped actor's issue-read grant", async () => {
const company = await seedCompany(db);
const ownerAgent = await seedAgent(db, company.id);
const mentionedAgent = await seedAgent(db, company.id);
const allowedProject = await seedProject(db, company.id, "Allowed");
const targetProject = await seedProject(db, company.id, "Target");
const hiddenMarker = `HIDDEN-SUBTREE-${randomUUID()}`;
const root = await seedIssue(db, {
companyId: company.id,
projectId: targetProject.id,
title: "Mention-visible root",
status: "todo",
assigneeAgentId: ownerAgent.id,
});
const hiddenChild = await seedIssue(db, {
companyId: company.id,
projectId: targetProject.id,
parentId: root.id,
title: hiddenMarker,
status: "blocked",
});
await db.insert(agentWakeupRequests).values({
companyId: company.id,
agentId: ownerAgent.id,
source: "automation",
reason: "issue_commented",
status: "failed",
payload: { issueId: hiddenChild.id, privateValue: hiddenMarker },
error: `secret ${hiddenMarker}`,
requestedAt: new Date(Date.now() - 5_000),
});
const run = await attachMentionScopedLowTrustRun(db, {
company,
ownerAgent,
mentionedAgent,
allowedProject,
root,
});
const res = await request(createApp(db, agentActor(company, mentionedAgent, run.id)))
.get(`/api/issues/${root.id}/diagnostics/subtree`);
expect(res.status, JSON.stringify(res.body)).toBe(200);
expect(res.body.nodes).toHaveLength(1);
expect(res.body.nodes[0].issue.id).toBe(root.id);
expect(res.body.omittedUnauthorizedNodeCount).toBe(1);
expect(res.body.diagnosis).toContain("authorization boundary");
const serialized = JSON.stringify(res.body);
expect(serialized).not.toContain(hiddenChild.id);
expect(serialized).not.toContain(hiddenMarker);
expect(serialized).not.toContain("\"payload\"");
expect(serialized).not.toContain("\"error\"");
});
it("denies cross-company issue reads", async () => {
const companyA = await seedCompany(db, "Company A");
const companyB = await seedCompany(db, "Company B");
const agentB = await seedAgent(db, companyB.id);
const projectA = await seedProject(db, companyA.id, "A");
const issueA = await seedIssue(db, {
companyId: companyA.id,
projectId: projectA.id,
title: "Company A issue",
status: "blocked",
});
const [runB] = await db.insert(heartbeatRuns).values({
companyId: companyB.id,
agentId: agentB.id,
status: "running",
contextSnapshot: { issueId: issueA.id },
}).returning();
const res = await request(createApp(db, agentActor(companyB, agentB, runB!.id)))
.get(`/api/issues/${issueA.id}/diagnostics/subtree`);
// Uniform 404 so cross-tenant ids are indistinguishable from missing ones.
expect(res.status, JSON.stringify(res.body)).toBe(404);
expect(res.body.error).toBe("Issue not found");
});
});