diff --git a/packages/db/src/migrations/0278_natural_nehzno.sql b/packages/db/src/migrations/0278_natural_nehzno.sql index 49fc39a78b..aa40e8e32d 100644 --- a/packages/db/src/migrations/0278_natural_nehzno.sql +++ b/packages/db/src/migrations/0278_natural_nehzno.sql @@ -1,73 +1,2 @@ ALTER TABLE "principal_permission_grants" ADD COLUMN "grant_origin" text DEFAULT 'explicit' NOT NULL;--> statement-breakpoint -UPDATE "principal_permission_grants" grants -SET "grant_origin" = 'role_default' -FROM "company_memberships" memberships -WHERE grants."company_id" = memberships."company_id" - AND grants."principal_type" = 'user' - AND grants."principal_id" = memberships."principal_id" - AND memberships."principal_type" = 'user' - AND memberships."status" = 'active' - AND grants."scope" IS NULL - AND grants."granted_by_user_id" IS NULL - AND NOT EXISTS ( - SELECT 1 - FROM "activity_log" explicit_activity - WHERE explicit_activity."company_id" = grants."company_id" - AND ( - ( - explicit_activity."action" = 'authorization.grants_updated_by_plugin' - AND explicit_activity."entity_type" = 'principal_permission_grants' - AND explicit_activity."entity_id" = grants."principal_type" || ':' || grants."principal_id" - ) - OR ( - explicit_activity."action" = 'company_member.permissions_updated' - AND explicit_activity."entity_type" = 'company_membership' - AND explicit_activity."entity_id" = memberships."id"::text - ) - ) - ) - AND NOT EXISTS ( - SELECT 1 - FROM "join_requests" approved_human_join - WHERE approved_human_join."company_id" = grants."company_id" - AND approved_human_join."request_type" = 'human' - AND approved_human_join."requesting_user_id" = grants."principal_id" - AND approved_human_join."status" = 'approved' - ) - AND ( - (memberships."membership_role" = 'owner' AND grants."permission_key" IN ( - 'agents:create', - 'agents:configure', - 'skills:create', - 'environments:manage', - 'users:invite', - 'users:manage_permissions', - 'tasks:assign', - 'tasks:manage_active_checkouts', - 'joins:approve', - 'pipelines:write', - 'audit:view_agent_actions', - 'tools:manage_connections', - 'tools:manage_runtime', - 'tools:use', - 'tools:admin' - )) - OR (memberships."membership_role" = 'admin' AND grants."permission_key" IN ( - 'agents:create', - 'agents:configure', - 'skills:create', - 'environments:manage', - 'users:invite', - 'tasks:assign', - 'tasks:manage_active_checkouts', - 'joins:approve', - 'pipelines:write', - 'audit:view_agent_actions', - 'tools:manage_connections', - 'tools:manage_runtime', - 'tools:use', - 'tools:admin' - )) - OR (memberships."membership_role" IN ('member', 'operator') AND grants."permission_key" = 'tasks:assign') - );--> statement-breakpoint ALTER TABLE "principal_permission_grants" ADD CONSTRAINT "principal_permission_grants_origin_check" CHECK ("principal_permission_grants"."grant_origin" in ('explicit', 'role_default')); diff --git a/packages/db/src/principal-permission-grant-origin-migration.test.ts b/packages/db/src/principal-permission-grant-origin-migration.test.ts index b1831f07ca..dab1243f29 100644 --- a/packages/db/src/principal-permission-grant-origin-migration.test.ts +++ b/packages/db/src/principal-permission-grant-origin-migration.test.ts @@ -27,7 +27,7 @@ async function migrationStatements() { } describeEmbeddedPostgres("principal permission grant origin migration", () => { - it("marks historical role defaults without consuming explicit grants", async () => { + it("preserves ambiguous historical grants as explicit", async () => { const database = await startEmbeddedPostgresTestDatabase("paperclip-grant-origin-"); cleanups.push(database.cleanup); const sql = postgres(database.connectionString, { max: 1, onnotice: () => {} }); @@ -102,9 +102,9 @@ describeEmbeddedPostgres("principal permission grant origin migration", () => { ORDER BY id `); expect(rows).toEqual([ - { id: "00000000-0000-4000-8000-000000000010", grant_origin: "role_default" }, - { id: "00000000-0000-4000-8000-000000000011", grant_origin: "role_default" }, - { id: "00000000-0000-4000-8000-000000000012", grant_origin: "role_default" }, + { id: "00000000-0000-4000-8000-000000000010", grant_origin: "explicit" }, + { id: "00000000-0000-4000-8000-000000000011", grant_origin: "explicit" }, + { id: "00000000-0000-4000-8000-000000000012", grant_origin: "explicit" }, { id: "00000000-0000-4000-8000-000000000013", grant_origin: "explicit" }, { id: "00000000-0000-4000-8000-000000000014", grant_origin: "explicit" }, { id: "00000000-0000-4000-8000-000000000015", grant_origin: "explicit" }, diff --git a/server/src/__tests__/access-routes-permissions-upgrade.test.ts b/server/src/__tests__/access-routes-permissions-upgrade.test.ts index 0001d1d8cf..4947b9def9 100644 --- a/server/src/__tests__/access-routes-permissions-upgrade.test.ts +++ b/server/src/__tests__/access-routes-permissions-upgrade.test.ts @@ -231,6 +231,36 @@ describeEmbeddedPostgres("access routes permissions upgrade compatibility", () = )).resolves.toBe(false); }); + it("rejects grant-backed board-key authority after membership suspension", async () => { + const { company, owner } = await createCompanyWithOwner(db); + await db.insert(principalPermissionGrants).values({ + companyId: company.id, + principalType: "user", + principalId: owner.principalId, + permissionKey: "tools:admin", + grantOrigin: "explicit", + }); + + await expect(ownerHasRequiredGrant( + db, + owner.principalId, + [company.id], + "tools:manage", + )).resolves.toBe(true); + + await db + .update(companyMemberships) + .set({ status: "suspended" }) + .where(eq(companyMemberships.id, owner.id)); + + await expect(ownerHasRequiredGrant( + db, + owner.principalId, + [company.id], + "tools:manage", + )).resolves.toBe(false); + }); + it("sweeps personal connection access when the member route suspends a user", async () => { const { company, owner } = await createCompanyWithOwner(db); const member = await db.insert(companyMemberships).values({ diff --git a/server/src/__tests__/board-key-auth-middleware.test.ts b/server/src/__tests__/board-key-auth-middleware.test.ts index 97696959f6..e64296e737 100644 --- a/server/src/__tests__/board-key-auth-middleware.test.ts +++ b/server/src/__tests__/board-key-auth-middleware.test.ts @@ -122,9 +122,10 @@ function createDbState() { function createApp( db: any, resolveSession = vi.fn(async () => null), - options: { sourceFromHeader?: boolean } = {}, + options: { sourceFromHeader?: boolean; trustProxy?: boolean } = {}, ) { const app = express(); + if (options.trustProxy) app.set("trust proxy", true); if (options.sourceFromHeader) { app.use((req, _res, next) => { Object.defineProperty(req.socket, "remoteAddress", { @@ -222,6 +223,27 @@ describe("board-key authentication middleware", () => { expect(db.select.mock.calls.length).toBe(lookupsBeforeThrottle); }); + it("isolates failure buckets for clients behind a trusted shared proxy", async () => { + const { db, state } = createDbState(); + state.keyExists = false; + const { app } = createApp(db, undefined, { trustProxy: true }); + const sourceLimit = BOARD_KEY_AUTH_FAILURE_RATE_LIMIT_DEFAULTS.sourceLimit; + + for (let index = 0; index < sourceLimit; index += 1) { + const response = await request(app) + .get("/actor") + .set("x-forwarded-for", "198.51.100.10") + .set("Authorization", `Bearer pcp_board_proxy_a_${index}`); + expect(response.status).toBe(401); + } + + const unrelatedClient = await request(app) + .get("/actor") + .set("x-forwarded-for", "198.51.100.11") + .set("Authorization", "Bearer pcp_board_proxy_b"); + expect(unrelatedClient.status).toBe(401); + }); + it("strictly bounds credential and source failure storage during identity floods", () => { const limiter = createBoardKeyAuthFailureRateLimiter({ windowMs: 60_000, diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index 9faed0ed2a..327d0c6b4c 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -68,7 +68,11 @@ function hashToken(token: string) { const boardKeyAuthFailureRateLimiter = createBoardKeyAuthFailureRateLimiter(); function boardKeyAuthFailureSource(req: Request) { - return req.socket.remoteAddress || req.ip || "unknown"; + // Express derives req.ip from the socket unless the immediate peer matches + // the operator's TRUST_PROXY policy. In the latter case it is the validated + // forwarded client address, so clients behind one proxy do not share a + // failure bucket merely because they share the proxy socket. + return req.ip || req.socket.remoteAddress || "unknown"; } export function resetBoardKeyAuthFailureRateLimitForTests() { diff --git a/server/src/security/board-key-owner-authority.ts b/server/src/security/board-key-owner-authority.ts index 9927f7d730..fd43a6b976 100644 --- a/server/src/security/board-key-owner-authority.ts +++ b/server/src/security/board-key-owner-authority.ts @@ -137,6 +137,12 @@ export async function ownerHasRequiredGrant( permissionKey: principalPermissionGrants.permissionKey, }) .from(principalPermissionGrants) + .innerJoin(companyMemberships, and( + eq(companyMemberships.companyId, principalPermissionGrants.companyId), + eq(companyMemberships.principalType, "user"), + eq(companyMemberships.principalId, ownerUserId), + eq(companyMemberships.status, "active"), + )) .where(and( inArray(principalPermissionGrants.companyId, [...companyIds]), eq(principalPermissionGrants.principalType, "user"),