mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-27 19:35:13 +01:00
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies, and a single server instance can host many companies. > - The auth middleware (`server/src/middleware/auth.ts`) supports a `cloud_tenant` mode where a trusted hosting proxy injects per-request identity headers, designed originally for one-deployment-per-tenant setups. > - In that original setup, granting every cloud tenant the `instance_admin` role was harmless; on a **shared, multi-tenant pool** it means any paying tenant is admin of the whole instance and can reach every other tenant's data. > - A tenant only needs to own its own company — which it already gets via the company membership the same code path upserts — so instance-level admin is never appropriate for `cloud_tenant` actors. > - This PR removes the `instance_admin` grant from the cloud-tenant path and pins `isInstanceAdmin: false` on the resolved actor. > - Greptile review then surfaced a follow-up gap: deployments that ran the pre-hardening build still have stale `instance_admin` rows in `instance_user_roles`, which other lookups (BetterAuth session path, board API keys, and the authorization service's own DB re-check) would still honor. > - The follow-up commit closes that gap by purging stale rows at the cloud-tenant auth boundary and by teaching the authorization service that `cloud_tenant` actors are never instance admins. > - The benefit is that shared-pool hosting becomes structurally safe: tenants are company-scoped owners, never instance admins — including on deployments upgrading from the older behavior. ## Linked Issues - Refs #966 — managed SaaS multi-tenant hosting is the deployment shape this hardening protects. - Refs #5015 — same problem space: instance-admin-scoped credentials are too broad for multi-company instances; tenants need company-scoped access. Neither issue is fully closed by this PR; it removes the instance-admin grant from the `cloud_tenant` trusted-header path specifically. ## What Changed - `server/src/middleware/auth.ts` - Removed the `instanceUserRoles` insert that granted every cloud tenant `instance_admin`; `resolveCloudTenantActor` now returns `isInstanceAdmin: false` (was `true`). - `resolveCloudTenantActor` now **deletes** any stale `instance_admin` row for the authenticated tenant user on every trusted-header request, so grants left behind by pre-hardening deployments are purged at the source (closes the Greptile P2: stale rows could otherwise re-elevate the user via the BetterAuth session path, board API keys, or the authorization service). - The function is `export`ed so it can be unit-tested directly. - `server/src/services/authorization.ts` - `authorizationService` previously re-checked `instanceUserRoles` from the DB regardless of the actor flag, which would have elevated even hardened `cloud_tenant` actors while a stale row lingered. Actors with `source === "cloud_tenant"` are now never elevated to instance admin; other board actors keep the existing lookup. - `server/src/services/authorization.ts` + `server/src/middleware/auth.ts` (follow-up commit `dc57a71c7`) - CI on the merge ref surfaced that elevation removal alone strands real cloud tenant users: board actors only ever reached `issue:read` / `issue:mutate` through instance-admin elevation (`permissionForAction` maps both to no grant key). `decide()` now grants `cloud_tenant` actors with an **active membership in the resource company** the same read surface as a same-company agent (`agent:read`, `company_scope:read`, `issue:read`, `project:read`) plus `issue:mutate` for non-viewer members — cross-company access stays denied (new `allow_company_member` reason). - `resolveCloudTenantActor` seeds the standard role-default permission grants (`ensureHumanRoleDefaultGrants`) so granted actions (e.g. `tasks:assign`, `agents:create` for owners) work without elevation. - Master-side route tests that stubbed cloud tenant actors with `isInstanceAdmin: true` now seed a real membership and assert under the hardened contract (`issue-identifier-routes`, `multilingual-issues-routes`, `issue-comment-redaction`). - Tests - `server/src/middleware/cloud-tenant-actor.test.ts` (new): cloud tenant is never instance-admin, is scoped to exactly the one company from its stack, still upserts user/company/membership, purges stale `instance_admin` rows, returns null without the server token, and maps non-owner stack roles without elevating. - `server/src/__tests__/auth-session-route.test.ts`: end-to-end middleware regression — a user with a stale `instance_admin` row stops being elevated via the session path once they authenticate through the cloud-tenant path (with a control assertion showing the pre-purge elevation). - `server/src/__tests__/authorization-service.test.ts` (embedded Postgres): a `cloud_tenant` actor with a stale `instance_admin` row in the real DB cannot cross company boundaries, while a `session` actor with the same row still resolves `allow_instance_admin`. ## Verification Run from the repo root after `pnpm install --frozen-lockfile`: ```bash cd server npx vitest run src/middleware/cloud-tenant-actor.test.ts src/__tests__/auth-session-route.test.ts # 9 tests passed npx vitest run src/__tests__/authorization-service.test.ts # 16 tests passed (embedded Postgres) pnpm typecheck # clean ``` Also ran the broader auth-related suites locally (`auth-routes`, `authz-company-access`, `better-auth`, `adapter-routes-authz`, `express5-auth-wildcard`): 8 files, 58 tests, all passing. ## Risks - **This touches authentication and authorization paths directly.** Mistakes here are security bugs in both directions; review accordingly. - **Behavioral change for existing `cloud_tenant` deployments:** tenants that previously (incorrectly) had instance-admin lose it — including the ability to see/manage other companies on the instance. This is the intended hardening, but any single-tenant deployment that relied on the cloud-tenant identity for instance administration must provision a separate admin identity. - **The purge is destructive by design:** if an operator's instance-admin identity is *also* provisioned through the cloud-tenant headers (same user id), its `instance_admin` row will be deleted on the next trusted-header request. Operators should hold admin through a non-cloud-tenant identity. - **Residual gap (documented, not fixed here):** a deployment that ran the old cloud_tenant build and then *disabled* cloud-tenant mode keeps stale rows until the affected user re-authenticates through the cloud path. A data migration was considered and deliberately avoided: there is no reliable SQL predicate for "cloud-tenant-provisioned user" (no source column), so a migration risks deleting legitimate admins. - No schema or migration changes; no UI changes. ## Model Used - Claude Fable 5 (claude-fable-5, 1M context), extended thinking + tool use, via Claude Code — this revision; original PR authored in an earlier Claude Code session. ## 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 - [x] I have searched GitHub for duplicate or related PRs and linked them above (none duplicate this; related issues Refs #966 / #5015 are linked in the issue section) - [x] 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 (no UI changes) - [x] I have updated relevant documentation to reflect my changes (no existing docs reference `cloud_tenant` mode) - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
261 lines
8.3 KiB
TypeScript
261 lines
8.3 KiB
TypeScript
import { randomUUID } from "node:crypto";
|
|
import express from "express";
|
|
import request from "supertest";
|
|
import { sql } from "drizzle-orm";
|
|
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
|
|
import {
|
|
companies,
|
|
companyMemberships,
|
|
createDb,
|
|
issueComments,
|
|
issueReferenceMentions,
|
|
issues,
|
|
} from "@paperclipai/db";
|
|
import { companySearchQuerySchema } from "@paperclipai/shared";
|
|
import {
|
|
getEmbeddedPostgresTestSupport,
|
|
startEmbeddedPostgresTestDatabase,
|
|
} from "./helpers/embedded-postgres.js";
|
|
import { errorHandler } from "../middleware/index.js";
|
|
import { issueRoutes } from "../routes/issues.js";
|
|
import { companySearchService } from "../services/company-search.js";
|
|
import { buildPaperclipWakePayload } from "../services/heartbeat.js";
|
|
import { issueReferenceService } from "../services/issue-references.js";
|
|
import { issueService } from "../services/issues.js";
|
|
import type { StorageService } from "../storage/types.js";
|
|
|
|
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
|
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe.sequential : describe.skip;
|
|
|
|
if (!embeddedPostgresSupport.supported) {
|
|
console.warn(
|
|
`Skipping embedded Postgres issue comment redaction tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`,
|
|
);
|
|
}
|
|
|
|
describeEmbeddedPostgres("deleted issue comment redaction", () => {
|
|
let db!: ReturnType<typeof createDb>;
|
|
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
|
|
|
beforeAll(async () => {
|
|
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-comment-redaction-");
|
|
db = createDb(tempDb.connectionString);
|
|
await db.execute(sql.raw("CREATE EXTENSION IF NOT EXISTS pg_trgm"));
|
|
}, 20_000);
|
|
|
|
afterEach(async () => {
|
|
await db.delete(issueReferenceMentions);
|
|
await db.delete(issueComments);
|
|
await db.delete(issues);
|
|
await db.delete(companyMemberships);
|
|
await db.delete(companies);
|
|
});
|
|
|
|
afterAll(async () => {
|
|
await tempDb?.cleanup();
|
|
});
|
|
|
|
async function seedIssue() {
|
|
const companyId = randomUUID();
|
|
const issueId = randomUUID();
|
|
await db.insert(companies).values({
|
|
id: companyId,
|
|
name: "Comment Redaction Co",
|
|
issuePrefix: `R${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
|
requireBoardApprovalForNewAgents: false,
|
|
});
|
|
await db.insert(issues).values({
|
|
id: issueId,
|
|
companyId,
|
|
identifier: "RED-1",
|
|
title: "Deleted comment redaction",
|
|
status: "todo",
|
|
priority: "medium",
|
|
});
|
|
await db.insert(companyMemberships).values({
|
|
companyId,
|
|
principalType: "user",
|
|
principalId: "board-user-1",
|
|
status: "active",
|
|
membershipRole: "owner",
|
|
updatedAt: new Date(),
|
|
});
|
|
return { companyId, issueId };
|
|
}
|
|
|
|
function createApp(companyId: string) {
|
|
const app = express();
|
|
app.use(express.json());
|
|
app.use((req, _res, next) => {
|
|
(req as any).actor = {
|
|
type: "board",
|
|
userId: "board-user-1",
|
|
companyIds: [companyId],
|
|
memberships: [{ companyId, membershipRole: "owner", status: "active" }],
|
|
source: "cloud_tenant",
|
|
// cloud_tenant actors are never instance admins — reads flow through
|
|
// the active company membership seeded in seedIssue().
|
|
isInstanceAdmin: false,
|
|
};
|
|
next();
|
|
});
|
|
const storage: StorageService = {
|
|
provider: "local_disk",
|
|
putFile: vi.fn(async () => {
|
|
throw new Error("Unexpected storage.putFile call");
|
|
}),
|
|
getObject: vi.fn(async () => {
|
|
throw new Error("Unexpected storage.getObject call");
|
|
}),
|
|
headObject: vi.fn(async () => ({ exists: false })),
|
|
deleteObject: vi.fn(async () => undefined),
|
|
};
|
|
app.use("/api", issueRoutes(db, storage));
|
|
app.use(errorHandler);
|
|
return app;
|
|
}
|
|
|
|
it("redacts deleted comment bodies from ordinary reads, heartbeat context, and wake payloads", async () => {
|
|
const { companyId, issueId } = await seedIssue();
|
|
const commentId = randomUUID();
|
|
const deletedAt = new Date("2026-06-03T12:00:00.000Z");
|
|
await db.insert(issueComments).values({
|
|
id: commentId,
|
|
companyId,
|
|
issueId,
|
|
authorUserId: "board-user-1",
|
|
body: "secret deleted body",
|
|
presentation: { kind: "system_notice", tone: "warning" },
|
|
metadata: { version: 1, sections: [{ rows: [{ type: "text", text: "secret metadata" }] }] },
|
|
deletedAt,
|
|
deletedByType: "user",
|
|
deletedByUserId: "board-user-1",
|
|
});
|
|
|
|
const comments = await issueService(db).listComments(issueId, { order: "asc" });
|
|
expect(comments).toHaveLength(1);
|
|
expect(comments[0]).toMatchObject({
|
|
id: commentId,
|
|
body: "",
|
|
presentation: null,
|
|
metadata: null,
|
|
deletedAt,
|
|
deletedByType: "user",
|
|
deletedByUserId: "board-user-1",
|
|
});
|
|
|
|
const exactComment = await issueService(db).getComment(commentId);
|
|
expect(exactComment?.body).toBe("");
|
|
expect(exactComment?.metadata).toBeNull();
|
|
|
|
const heartbeatContext = await request(createApp(companyId))
|
|
.get(`/api/issues/${issueId}/heartbeat-context`)
|
|
.query({ wakeCommentId: commentId });
|
|
expect(heartbeatContext.status, JSON.stringify(heartbeatContext.body)).toBe(200);
|
|
expect(heartbeatContext.body.wakeComment).toMatchObject({
|
|
id: commentId,
|
|
body: "",
|
|
metadata: null,
|
|
deletedByUserId: "board-user-1",
|
|
});
|
|
expect(JSON.stringify(heartbeatContext.body)).not.toContain("secret deleted body");
|
|
expect(JSON.stringify(heartbeatContext.body)).not.toContain("secret metadata");
|
|
|
|
const wakePayload = await buildPaperclipWakePayload({
|
|
db,
|
|
companyId,
|
|
contextSnapshot: {
|
|
issueId,
|
|
commentId,
|
|
wakeCommentIds: [commentId],
|
|
wakeReason: "issue_commented",
|
|
},
|
|
});
|
|
|
|
expect(wakePayload?.comments).toEqual([
|
|
expect.objectContaining({
|
|
id: commentId,
|
|
body: "",
|
|
bodyTruncated: false,
|
|
presentation: null,
|
|
metadata: null,
|
|
deletedAt: deletedAt.toISOString(),
|
|
deletedByUserId: "board-user-1",
|
|
}),
|
|
]);
|
|
expect(JSON.stringify(wakePayload)).not.toContain("secret deleted body");
|
|
expect(JSON.stringify(wakePayload)).not.toContain("secret metadata");
|
|
});
|
|
|
|
it("excludes deleted comment bodies from company search", async () => {
|
|
const { companyId, issueId } = await seedIssue();
|
|
await db.insert(issueComments).values({
|
|
companyId,
|
|
issueId,
|
|
body: "vanished-search-needle",
|
|
deletedAt: new Date("2026-06-03T12:00:00.000Z"),
|
|
deletedByType: "user",
|
|
deletedByUserId: "board-user-1",
|
|
});
|
|
|
|
const result = await companySearchService(db).search(
|
|
companyId,
|
|
companySearchQuerySchema.parse({ q: "vanished-search-needle", scope: "comments" }),
|
|
);
|
|
|
|
expect(result.results).toEqual([]);
|
|
});
|
|
|
|
it("clears issue references sourced from deleted comment bodies", async () => {
|
|
const companyId = randomUUID();
|
|
const sourceIssueId = randomUUID();
|
|
const targetIssueId = randomUUID();
|
|
const commentId = randomUUID();
|
|
await db.insert(companies).values({
|
|
id: companyId,
|
|
name: "Reference Redaction Co",
|
|
issuePrefix: "REF",
|
|
requireBoardApprovalForNewAgents: false,
|
|
});
|
|
await db.insert(issues).values([
|
|
{
|
|
id: sourceIssueId,
|
|
companyId,
|
|
identifier: "REF-1",
|
|
title: "Source issue",
|
|
status: "todo",
|
|
priority: "medium",
|
|
},
|
|
{
|
|
id: targetIssueId,
|
|
companyId,
|
|
identifier: "REF-2",
|
|
title: "Target issue",
|
|
status: "todo",
|
|
priority: "medium",
|
|
},
|
|
]);
|
|
await db.insert(issueComments).values({
|
|
id: commentId,
|
|
companyId,
|
|
issueId: sourceIssueId,
|
|
body: "Follow up in REF-2",
|
|
});
|
|
|
|
const refs = issueReferenceService(db);
|
|
await refs.syncComment(commentId);
|
|
expect((await refs.listIssueReferenceSummary(sourceIssueId)).outbound.map((item) => item.issue.id)).toEqual([
|
|
targetIssueId,
|
|
]);
|
|
|
|
await db.update(issueComments).set({
|
|
deletedAt: new Date("2026-06-03T12:00:00.000Z"),
|
|
deletedByType: "user",
|
|
deletedByUserId: "board-user-1",
|
|
});
|
|
await refs.syncComment(commentId);
|
|
|
|
expect((await refs.listIssueReferenceSummary(sourceIssueId)).outbound).toEqual([]);
|
|
});
|
|
});
|