From 296f231b7d0c64fb60adfe47a8facd528693126b Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 30 Jul 2026 08:10:03 -0400 Subject: [PATCH 1/2] Harden tenant admin API key boundaries --- server/src/middleware/auth.ts | 213 +++++++++-------- server/src/routes/billing.ts | 9 +- server/src/routes/member-profiles.ts | 14 -- .../unit/admin-stripe-customer-link.test.ts | 1 + .../unit/api-key-issuance-permissions.test.ts | 3 +- .../billing-admin-tenant-boundary.test.ts | 225 ++++++++++++++++++ ...mber-profile-admin-tenant-boundary.test.ts | 191 +++++++++++++++ .../unit/require-admin-cross-tenant.test.ts | 191 +++------------ 8 files changed, 573 insertions(+), 274 deletions(-) create mode 100644 server/tests/unit/billing-admin-tenant-boundary.test.ts create mode 100644 server/tests/unit/member-profile-admin-tenant-boundary.test.ts diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index a2e83c4e9b..f72e2760ef 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -1341,11 +1341,91 @@ export function requireRole(...allowedRoles: Array<'owner' | 'admin' | 'member'> }; } +/** + * Verify that a tenant-issued API key has the admin permission required by the + * request method. Organization binding is enforced separately by the one + * explicit tenant-admin middleware below. + */ +function tenantApiKeyHasAdminPermission( + req: Request, + res: Response, + apiKey: ValidatedApiKey, +): boolean { + const isReadOnlyRequest = req.method === 'GET' || req.method === 'HEAD' || req.method === 'OPTIONS'; + + if (apiKeyHasPermission(apiKey, 'admin:*')) { + return true; + } + + if (apiKeyHasPermission(apiKey, 'admin:read') && isReadOnlyRequest) { + return true; + } + + res.status(403).json({ + error: 'Insufficient permissions', + message: isReadOnlyRequest + ? 'This API key does not have admin access. Required permission: admin:* or admin:read' + : 'This API key does not have write access. Required permission: admin:*', + api_key_permissions: apiKey.permissions, + }); + return false; +} + +/** + * Bind a tenant-issued API key to a server-resolved target organization. + */ +function tenantApiKeyMatchesOrganization( + req: Request, + res: Response, + apiKey: ValidatedApiKey, + targetOrgId: string, +): boolean { + if (apiKey.organizationId !== targetOrgId) { + logger.warn( + { path: req.path, method: req.method, apiKeyId: apiKey.id, apiKeyOrgId: apiKey.organizationId, targetOrgId }, + 'Refused cross-tenant admin API key', + ); + res.status(403).json({ + error: 'cross_tenant_api_key', + message: `API key issued by ${apiKey.organizationId} cannot operate on ${targetOrgId}`, + }); + return false; + } + + return true; +} + +/** + * Authorize a tenant-issued API key against a server-resolved organization. + * WorkOS `admin:*` and `admin:read` permissions never grant platform-global + * authority; they are valid only when the target organization exactly matches + * the organization that issued the key. + */ +function authorizeTenantAdminApiKey( + req: Request, + res: Response, + apiKey: ValidatedApiKey, + targetOrgId: string, +): boolean { + if (!tenantApiKeyMatchesOrganization(req, res, apiKey, targetOrgId)) { + return false; + } + + if (!tenantApiKeyHasAdminPermission(req, res, apiKey)) { + return false; + } + + logger.debug({ path: req.path, method: req.method, apiKeyId: apiKey.id }, 'Tenant admin access via WorkOS API key'); + return true; +} + /** * Middleware to require admin access * Must be used after requireAuth * Accepts static admin API key (ADMIN_API_KEY env var) for internal tooling - * Accepts WorkOS API keys with 'admin:*' permission for programmatic access + * Rejects tenant-issued WorkOS API keys: this middleware grants platform-wide + * administration. Audited tenant routes must use the explicit tenant-admin + * middleware below. * Or checks if user's email is in ADMIN_EMAILS list */ export async function requireAdmin(req: Request, res: Response, next: NextFunction) { @@ -1357,61 +1437,12 @@ export async function requireAdmin(req: Request, res: Response, next: NextFuncti return next(); } - // Check for WorkOS API key with admin permission + // WorkOS API keys are tenant-scoped and must never inherit platform-admin + // authority merely because a route happens to contain an organization ID. const apiKey = (req as Request & { apiKey?: ValidatedApiKey }).apiKey; if (apiKey) { - // Cross-tenant defense for routes whose path resolves a specific - // target org via `:orgId`. The `admin:*` permission is tenant-scoped - // by issuance: it grants admin access *within* the org that minted - // the key, not across orgs. Without this gate, any org holding an - // `admin:*` key could mutate any other org's data exposed by a - // cross-org admin route. Surfaced by security review on #4498. - // - // KNOWN GAP: routes that target an org via a differently-named param - // (`:id`, `:userId`, profile UUIDs) silently skip this default gate. - // Member-profile admin PUT/DELETE uses `refuseCrossTenantAdminApiKey` - // after a profile-id → org lookup; `/api/admin/users/*` uses the - // `requireGlobalAdmin` chain (which composes a global-state refusal). - // Cousin routes that operate on global state via `:id` (notably - // `admin/feeds.ts` and `admin/notification-channels.ts`) are tracked - // in #4501 and should adopt `requireGlobalAdmin` once cross-tenant - // exposure is escalated. Pushing this default catches every admin - // route that uses `:orgId` for free, while leaving the higher-risk - // routes explicit. - const targetOrgId = req.params.orgId; - if (targetOrgId && apiKey.organizationId !== targetOrgId) { - logger.warn( - { path: req.path, method: req.method, apiKeyId: apiKey.id, apiKeyOrgId: apiKey.organizationId, targetOrgId }, - 'Refused cross-tenant admin API key', - ); - return res.status(403).json({ - error: 'cross_tenant_api_key', - message: `API key issued by ${apiKey.organizationId} cannot operate on ${targetOrgId}`, - }); - } - - const isReadOnlyRequest = req.method === 'GET' || req.method === 'HEAD' || req.method === 'OPTIONS'; - - // admin:* grants full access (read and write) - if (apiKeyHasPermission(apiKey, 'admin:*')) { - logger.debug({ path: req.path, method: req.method, apiKeyId: apiKey.id }, 'Full admin access via WorkOS API key'); - return next(); - } - - // admin:read only grants access to read operations - if (apiKeyHasPermission(apiKey, 'admin:read') && isReadOnlyRequest) { - logger.debug({ path: req.path, method: req.method, apiKeyId: apiKey.id }, 'Read-only admin access via WorkOS API key'); - return next(); - } - - // API key exists but doesn't have sufficient permission - return res.status(403).json({ - error: 'Insufficient permissions', - message: isReadOnlyRequest - ? 'This API key does not have admin access. Required permission: admin:* or admin:read' - : 'This API key does not have write access. Required permission: admin:*', - api_key_permissions: apiKey.permissions, - }); + refuseAnyApiKeyOnGlobalAdmin(req, res); + return; } // Dev mode: check if dev user has admin flag @@ -1538,45 +1569,31 @@ export async function requireAdmin(req: Request, res: Response, next: NextFuncti } /** - * Reject the request when the caller is using a WorkOS API key whose - * `organizationId` does not match `targetOrgId`. Returns true if the - * request was refused (response sent), false if the caller may proceed. - * - * Use this on admin routes whose target org is NOT resolvable from - * `req.params.orgId` — e.g. routes keyed by a profile UUID (`:id`) or - * a global user id (`:userId`). The default cross-tenant gate in - * `requireAdmin` keys off `req.params.orgId` and silently skips on those - * routes; the security review on #4498 flagged this as a real bypass - * for `/api/admin/member-profiles/:id`. Resolve the target org from the - * resource (the profile's `workos_organization_id`, the user's primary - * org, etc.) and pass it here. - * - * Static `ADMIN_API_KEY` and SSO admin users do not set `req.apiKey` - * and pass through unchanged. + * Admin middleware for an audited tenant route whose target is the literal + * `:orgId` path parameter. Tenant WorkOS keys are bound to their issuing + * organization; SSO and static platform admins delegate to `requireAdmin`. + * Must be used after `requireAuth`. */ -export function refuseCrossTenantAdminApiKey( +export async function requireTenantAdminForOrganization( req: Request, res: Response, - targetOrgId: string, -): boolean { + next: NextFunction, +): Promise { const apiKey = (req as Request & { apiKey?: ValidatedApiKey }).apiKey; - if (!apiKey) return false; - if (apiKey.organizationId === targetOrgId) return false; - logger.warn( - { - path: req.path, - method: req.method, - apiKeyId: apiKey.id, - apiKeyOrgId: apiKey.organizationId, - targetOrgId, - }, - 'Refused cross-tenant admin API key (per-route gate)', - ); - res.status(403).json({ - error: 'cross_tenant_api_key', - message: `API key issued by ${apiKey.organizationId} cannot operate on ${targetOrgId}`, - }); - return true; + if (!apiKey) { + await requireAdmin(req, res, next); + return; + } + + const targetOrgId = req.params.orgId; + if (!targetOrgId) { + refuseAnyApiKeyOnGlobalAdmin(req, res); + return; + } + + if (authorizeTenantAdminApiKey(req, res, apiKey, targetOrgId)) { + next(); + } } /** @@ -1607,25 +1624,11 @@ export function refuseAnyApiKeyOnGlobalAdmin( } /** - * Composite middleware chain for admin routes that operate on cross- - * org / global state. Wraps `requireAuth` + a global-admin gate + - * `requireAdmin` so every route mounted under it inherits the cross- - * tenant-API-key refusal by default — preventing the regression class - * where a new admin route is added but the per-handler call is - * forgotten. Use as `router.('/path', ...requireGlobalAdmin, handler)`. - * - * Surfaced by both code-review and security-review on PR #4646: 7 of - * the 13 routes on `/api/admin/users` originally relied on - * `requireAdmin` alone and were exposed to cross-tenant `admin:*` keys - * despite operating on global state. + * Composite chain for platform-admin routes. `requireAdmin` centrally rejects + * tenant WorkOS API keys, so global routes need no separate API-key guard. */ -const refuseAnyApiKeyMiddleware: import('express').RequestHandler = (req, res, next) => { - if (refuseAnyApiKeyOnGlobalAdmin(req, res)) return; - next(); -}; export const requireGlobalAdmin: import('express').RequestHandler[] = [ requireAuth, - refuseAnyApiKeyMiddleware, requireAdmin, ]; diff --git a/server/src/routes/billing.ts b/server/src/routes/billing.ts index ad5d55477c..ea19e4a676 100644 --- a/server/src/routes/billing.ts +++ b/server/src/routes/billing.ts @@ -9,7 +9,12 @@ import { Router } from "express"; import Stripe from "stripe"; import type { PoolClient } from "pg"; import { createLogger } from "../logger.js"; -import { requireAuth, requireAdmin, requireGlobalAdmin } from "../middleware/auth.js"; +import { + requireAuth, + requireAdmin, + requireGlobalAdmin, + requireTenantAdminForOrganization, +} from "../middleware/auth.js"; import { serveHtmlWithConfig } from "../utils/html-config.js"; import { getPool } from "../db/client.js"; import { @@ -188,7 +193,7 @@ export function createBillingRouter(): { pageRouter: Router; apiRouter: Router } apiRouter.get( "/orgs/:orgId/invite-products", requireAuth, - requireAdmin, + requireTenantAdminForOrganization, async (req, res) => { try { const { orgId } = req.params; diff --git a/server/src/routes/member-profiles.ts b/server/src/routes/member-profiles.ts index 8dc1d63435..0cc01a9a0b 100644 --- a/server/src/routes/member-profiles.ts +++ b/server/src/routes/member-profiles.ts @@ -11,7 +11,6 @@ import { createLogger } from "../logger.js"; import { requireAuth, requireAdmin, - refuseCrossTenantAdminApiKey, isDevModeEnabled, DEV_USERS, } from "../middleware/auth.js"; @@ -2637,12 +2636,6 @@ export function createAdminMemberProfileRouter(config: MemberProfileRoutesConfig const { id } = req.params; const updates = req.body; - // Cross-tenant gate. `requireAdmin` keys off `:orgId` in the path, - // but this route uses `:id` (a profile UUID) — resolve the profile's - // org and apply the gate here. Without this, any WorkOS API key - // holding `admin:*` (issued by any org) could mutate any - // member_profiles row by guessing the UUID. Surfaced by security - // review on #4498. const existingProfile = await memberDb.getProfileById(id); if (!existingProfile) { return res.status(404).json({ @@ -2650,9 +2643,6 @@ export function createAdminMemberProfileRouter(config: MemberProfileRoutesConfig message: `No member profile found with ID: ${id}`, }); } - if (refuseCrossTenantAdminApiKey(req, res, existingProfile.workos_organization_id)) { - return; - } // Validate offerings if provided if (updates.offerings && Array.isArray(updates.offerings)) { @@ -2717,7 +2707,6 @@ export function createAdminMemberProfileRouter(config: MemberProfileRoutesConfig try { const { id } = req.params; - // Cross-tenant gate — see PUT above for context. const existingProfile = await memberDb.getProfileById(id); if (!existingProfile) { return res.status(404).json({ @@ -2725,9 +2714,6 @@ export function createAdminMemberProfileRouter(config: MemberProfileRoutesConfig message: `No member profile found with ID: ${id}`, }); } - if (refuseCrossTenantAdminApiKey(req, res, existingProfile.workos_organization_id)) { - return; - } const deleted = await memberDb.deleteProfile(id); diff --git a/server/tests/unit/admin-stripe-customer-link.test.ts b/server/tests/unit/admin-stripe-customer-link.test.ts index 04853e895d..f8de654e24 100644 --- a/server/tests/unit/admin-stripe-customer-link.test.ts +++ b/server/tests/unit/admin-stripe-customer-link.test.ts @@ -32,6 +32,7 @@ vi.mock('../../src/middleware/auth.js', () => ({ next(); }, requireAdmin: (_req: any, _res: any, next: any) => next(), + requireTenantAdminForOrganization: (_req: any, _res: any, next: any) => next(), requireGlobalAdmin: [ (req: any, _res: any, next: any) => { req.user = { id: 'user_admin_01', email: 'admin@test', is_admin: true }; diff --git a/server/tests/unit/api-key-issuance-permissions.test.ts b/server/tests/unit/api-key-issuance-permissions.test.ts index 76fd4ae192..4e992ff1f5 100644 --- a/server/tests/unit/api-key-issuance-permissions.test.ts +++ b/server/tests/unit/api-key-issuance-permissions.test.ts @@ -130,7 +130,8 @@ describe('tenant API key issuance permissions', () => { .send({ name: 'Automation key', permissions: [permission] }); expect(response.status).toBe(201); - const [, options] = mocks.fetch.mock.calls[0] as [string, RequestInit]; + const [url, options] = mocks.fetch.mock.calls[0] as [string, RequestInit]; + expect(new URL(url).pathname).toBe('/organizations/org_target/api_keys'); expect(JSON.parse(options.body as string)).toEqual({ name: 'Automation key', permissions: [permission], diff --git a/server/tests/unit/billing-admin-tenant-boundary.test.ts b/server/tests/unit/billing-admin-tenant-boundary.test.ts new file mode 100644 index 0000000000..1c35c4e28b --- /dev/null +++ b/server/tests/unit/billing-admin-tenant-boundary.test.ts @@ -0,0 +1,225 @@ +import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import express, { type NextFunction, type Request, type Response } from 'express'; +import request from 'supertest'; + +const mocks = vi.hoisted(() => ({ + createProduct: vi.fn(), + getProductsForCustomer: vi.fn(), + getOrganization: vi.fn(), + findStripeCustomerConflicts: vi.fn(), + getOrganizationByStripeCustomerId: vi.fn(), + setStripeCustomerId: vi.fn(), + unlinkStripeCustomer: vi.fn(), + customersRetrieve: vi.fn(), + customersUpdate: vi.fn(), + isWebUserAAOAdmin: vi.fn(), +})); + +vi.hoisted(() => { + process.env.WORKOS_API_KEY = 'sk_test_billing_tenant_boundary'; + process.env.WORKOS_CLIENT_ID = 'client_test_billing_tenant_boundary'; + process.env.WORKOS_COOKIE_PASSWORD = + 'test-cookie-password-at-least-32-characters'; +}); + +vi.mock('../../src/addie/mcp/admin-tools.js', () => ({ + isWebUserAAOAdmin: (...args: unknown[]) => mocks.isWebUserAAOAdmin(...args), +})); + +vi.mock('../../src/middleware/auth.js', async (importOriginal) => { + const actual = await importOriginal(); + + return { + ...actual, + requireAuth: (req: Request, _res: Response, next: NextFunction) => { + req.user = { + id: 'user_billing_boundary', + email: 'billing-boundary@example.test', + emailVerified: true, + createdAt: new Date(0).toISOString(), + updatedAt: new Date(0).toISOString(), + }; + + const apiKeyOrgId = req.header('x-test-api-key-org-id'); + if (apiKeyOrgId) { + (req as Request & { + apiKey?: { + id: string; + organizationId: string; + name: string; + permissions: string[]; + }; + }).apiKey = { + id: 'apikey_owner_automation', + organizationId: apiKeyOrgId, + name: 'Owner automation', + permissions: ['admin:*'], + }; + } + + if (req.header('x-test-static-admin') === '1') { + (req as Request & { isStaticAdminApiKey?: boolean }).isStaticAdminApiKey = true; + } + next(); + }, + }; +}); + +vi.mock('../../src/billing/stripe-client.js', () => ({ + stripe: { + customers: { + retrieve: (...args: unknown[]) => mocks.customersRetrieve(...args), + update: (...args: unknown[]) => mocks.customersUpdate(...args), + }, + invoices: {}, + products: {}, + subscriptions: {}, + }, + getBillingProducts: vi.fn(), + getProductsForCustomer: (...args: unknown[]) => mocks.getProductsForCustomer(...args), + createProduct: (...args: unknown[]) => mocks.createProduct(...args), + updateProductMetadata: vi.fn(), + archiveProduct: vi.fn(), + clearProductsCache: vi.fn(), + getPendingInvoices: vi.fn(), + voidInvoice: vi.fn(), + deleteDraftInvoice: vi.fn(), +})); + +vi.mock('../../src/db/organization-db.js', () => ({ + OrganizationDatabase: class OrganizationDatabase { + getOrganization = (...args: unknown[]) => mocks.getOrganization(...args); + findStripeCustomerConflicts = (...args: unknown[]) => + mocks.findStripeCustomerConflicts(...args); + getOrganizationByStripeCustomerId = (...args: unknown[]) => + mocks.getOrganizationByStripeCustomerId(...args); + setStripeCustomerId = (...args: unknown[]) => mocks.setStripeCustomerId(...args); + unlinkStripeCustomer = (...args: unknown[]) => mocks.unlinkStripeCustomer(...args); + }, + TIER_PRESERVING_STATUSES: new Set(), + buildSubscriptionUpdate: vi.fn(), +})); + +const { createBillingRouter } = await import('../../src/routes/billing.js'); +const { stopAuthTimers } = await import('../../src/middleware/auth.js'); + +const app = express(); +app.use(express.json()); +app.use('/api/admin', createBillingRouter().apiRouter); + +afterAll(() => { + stopAuthTimers(); +}); + +beforeEach(() => { + vi.clearAllMocks(); + mocks.getOrganization.mockResolvedValue({ + workos_organization_id: 'org_owner', + name: 'Acme Corp', + discount_percent: null, + discount_amount_cents: null, + stripe_coupon_id: null, + discount_reason: null, + }); + mocks.getProductsForCustomer.mockResolvedValue([]); + mocks.findStripeCustomerConflicts.mockResolvedValue([]); + mocks.createProduct.mockResolvedValue({ + product_id: 'prod_platform', + price_id: 'price_platform', + lookup_key: 'aao_platform_product', + }); + mocks.isWebUserAAOAdmin.mockResolvedValue(true); +}); + +describe('billing admin tenant boundary', () => { + it('allows owner automation only on an explicitly scoped route for its own organization', async () => { + const response = await request(app) + .get('/api/admin/orgs/org_owner/invite-products') + .set('x-test-api-key-org-id', 'org_owner'); + + expect(response.status).toBe(200); + expect(mocks.getOrganization).toHaveBeenCalledWith('org_owner'); + expect(mocks.getProductsForCustomer).toHaveBeenCalledWith({ invoiceableOnly: true }); + }); + + it('refuses the same owner automation key on a sibling organization', async () => { + const response = await request(app) + .get('/api/admin/orgs/org_sibling/invite-products') + .set('x-test-api-key-org-id', 'org_owner'); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('cross_tenant_api_key'); + expect(mocks.getOrganization).not.toHaveBeenCalled(); + expect(mocks.getProductsForCustomer).not.toHaveBeenCalled(); + }); + + it('refuses owner automation before platform-global product creation', async () => { + const response = await request(app) + .post('/api/admin/products') + .set('x-test-api-key-org-id', 'org_owner') + .send({ + name: 'Platform product', + lookupKey: 'aao_platform_product', + amountCents: 1000, + billingType: 'one_time', + category: 'other', + }); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('global_admin_required'); + expect(mocks.createProduct).not.toHaveBeenCalled(); + }); + + it('refuses owner automation before Stripe-conflict resolution side effects', async () => { + const response = await request(app) + .post('/api/admin/stripe-conflicts/resolve') + .set('x-test-api-key-org-id', 'org_owner') + .send({ + stripe_customer_id: 'cus_conflict', + keep_org_id: 'org_sibling', + action: 'update_stripe_metadata', + }); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('global_admin_required'); + expect(mocks.getOrganizationByStripeCustomerId).not.toHaveBeenCalled(); + expect(mocks.customersUpdate).not.toHaveBeenCalled(); + }); + + it('keeps platform-global billing routes available to the static admin key', async () => { + const productResponse = await request(app) + .post('/api/admin/products') + .set('x-test-static-admin', '1') + .send({ + name: 'Platform product', + lookupKey: 'aao_platform_product', + amountCents: 1000, + billingType: 'one_time', + category: 'other', + }); + const conflictsResponse = await request(app) + .get('/api/admin/stripe-conflicts') + .set('x-test-static-admin', '1'); + + expect(productResponse.status).toBe(200); + expect(mocks.createProduct).toHaveBeenCalledOnce(); + expect(conflictsResponse.status).toBe(200); + expect(mocks.findStripeCustomerConflicts).toHaveBeenCalledOnce(); + }); + + it('keeps platform-global billing routes available to an SSO platform admin', async () => { + const productResponse = await request(app) + .post('/api/admin/products') + .send({ + name: 'Platform product', + lookupKey: 'aao_platform_product', + amountCents: 1000, + billingType: 'one_time', + category: 'other', + }); + + expect(productResponse.status).toBe(200); + expect(mocks.isWebUserAAOAdmin).toHaveBeenCalledWith('user_billing_boundary'); + expect(mocks.createProduct).toHaveBeenCalledOnce(); + }); +}); diff --git a/server/tests/unit/member-profile-admin-tenant-boundary.test.ts b/server/tests/unit/member-profile-admin-tenant-boundary.test.ts new file mode 100644 index 0000000000..5f25154c7d --- /dev/null +++ b/server/tests/unit/member-profile-admin-tenant-boundary.test.ts @@ -0,0 +1,191 @@ +import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import express, { type NextFunction, type Request, type Response } from 'express'; +import request from 'supertest'; + +const mocks = vi.hoisted(() => ({ + getProfileById: vi.fn(), + updateProfile: vi.fn(), + deleteProfile: vi.fn(), + invalidateMemberContextCache: vi.fn(), +})); + +vi.hoisted(() => { + process.env.WORKOS_API_KEY = 'sk_test_profile_tenant_boundary'; + process.env.WORKOS_CLIENT_ID = 'client_test_profile_tenant_boundary'; + process.env.WORKOS_COOKIE_PASSWORD = + 'test-cookie-password-at-least-32-characters'; +}); + +vi.mock('../../src/middleware/auth.js', async (importOriginal) => { + const actual = await importOriginal(); + + return { + ...actual, + requireAuth: (req: Request, _res: Response, next: NextFunction) => { + req.user = { + id: 'user_profile_boundary', + email: 'profile-boundary@example.test', + emailVerified: true, + createdAt: new Date(0).toISOString(), + updatedAt: new Date(0).toISOString(), + }; + + const apiKeyOrgId = req.header('x-test-api-key-org-id'); + if (apiKeyOrgId) { + (req as Request & { + apiKey?: { + id: string; + organizationId: string; + name: string; + permissions: string[]; + }; + }).apiKey = { + id: 'apikey_profile_automation', + organizationId: apiKeyOrgId, + name: 'Profile automation', + permissions: [req.header('x-test-api-key-permission') ?? 'admin:*'], + }; + } + if (req.header('x-test-static-admin') === '1') { + (req as Request & { isStaticAdminApiKey?: boolean }).isStaticAdminApiKey = true; + } + next(); + }, + }; +}); + +const { createAdminMemberProfileRouter } = await import('../../src/routes/member-profiles.js'); +const { stopAuthTimers } = await import('../../src/middleware/auth.js'); + +const profile = { + id: 'profile_target', + workos_organization_id: 'org_owner', + display_name: 'Acme Corp', +}; + +const memberDb = { + getProfileById: (...args: unknown[]) => mocks.getProfileById(...args), + updateProfile: (...args: unknown[]) => mocks.updateProfile(...args), + deleteProfile: (...args: unknown[]) => mocks.deleteProfile(...args), +}; + +const app = express(); +app.use(express.json()); +app.use( + '/api/admin/member-profiles', + createAdminMemberProfileRouter({ + workos: null, + memberDb, + brandDb: {}, + orgDb: {}, + invalidateMemberContextCache: mocks.invalidateMemberContextCache, + } as unknown as Parameters[0]), +); + +afterAll(() => { + stopAuthTimers(); +}); + +beforeEach(() => { + vi.clearAllMocks(); + mocks.getProfileById.mockResolvedValue(profile); + mocks.updateProfile.mockResolvedValue({ ...profile, tagline: 'Updated' }); + mocks.deleteProfile.mockResolvedValue(true); +}); + +function mutateProfile( + method: 'put' | 'delete', + apiKeyOrgId: string, + permission = 'admin:*', +) { + const pendingRequest = method === 'put' + ? request(app) + .put(`/api/admin/member-profiles/${profile.id}`) + .send({ tagline: 'Updated' }) + : request(app).delete(`/api/admin/member-profiles/${profile.id}`); + + return pendingRequest + .set('x-test-api-key-org-id', apiKeyOrgId) + .set('x-test-api-key-permission', permission); +} + +describe('member profile platform-admin boundary', () => { + it.each(['put', 'delete'] as const)( + 'refuses same-tenant admin:* automation before it can %s its own profile', + async (method) => { + const response = await mutateProfile(method, 'org_owner'); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('global_admin_required'); + expect(mocks.getProfileById).not.toHaveBeenCalled(); + expect(mocks.updateProfile).not.toHaveBeenCalled(); + expect(mocks.deleteProfile).not.toHaveBeenCalled(); + expect(mocks.invalidateMemberContextCache).not.toHaveBeenCalled(); + }, + ); + + it.each(['put', 'delete'] as const)( + 'keeps the %s route available to the static platform-admin key', + async (method) => { + const pendingRequest = method === 'put' + ? request(app) + .put(`/api/admin/member-profiles/${profile.id}`) + .send({ tagline: 'Updated' }) + : request(app).delete(`/api/admin/member-profiles/${profile.id}`); + const response = await pendingRequest.set('x-test-static-admin', '1'); + + expect(response.status).toBe(200); + expect(mocks.getProfileById).toHaveBeenCalledOnce(); + if (method === 'put') { + expect(mocks.updateProfile).toHaveBeenCalledWith(profile.id, { tagline: 'Updated' }); + } else { + expect(mocks.deleteProfile).toHaveBeenCalledWith(profile.id); + } + expect(mocks.invalidateMemberContextCache).toHaveBeenCalledOnce(); + }, + ); + + it('returns not found without mutation for a platform admin when the profile does not exist', async () => { + mocks.getProfileById.mockResolvedValueOnce(null); + + const response = await request(app) + .put(`/api/admin/member-profiles/${profile.id}`) + .set('x-test-static-admin', '1') + .send({ tagline: 'Updated' }); + + expect(response.status).toBe(404); + expect(response.body.error).toBe('Profile not found'); + expect(mocks.getProfileById).toHaveBeenCalledOnce(); + expect(mocks.updateProfile).not.toHaveBeenCalled(); + expect(mocks.deleteProfile).not.toHaveBeenCalled(); + expect(mocks.invalidateMemberContextCache).not.toHaveBeenCalled(); + }); + + it.each(['put', 'delete'] as const)( + 'refuses cross-tenant admin:* automation before it can %s a sibling profile', + async (method) => { + const response = await mutateProfile(method, 'org_sibling'); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('global_admin_required'); + expect(mocks.getProfileById).not.toHaveBeenCalled(); + expect(mocks.updateProfile).not.toHaveBeenCalled(); + expect(mocks.deleteProfile).not.toHaveBeenCalled(); + expect(mocks.invalidateMemberContextCache).not.toHaveBeenCalled(); + }, + ); + + it.each(['put', 'delete'] as const)( + 'refuses same-tenant admin:read automation before a %s write', + async (method) => { + const response = await mutateProfile(method, 'org_owner', 'admin:read'); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('global_admin_required'); + expect(mocks.getProfileById).not.toHaveBeenCalled(); + expect(mocks.updateProfile).not.toHaveBeenCalled(); + expect(mocks.deleteProfile).not.toHaveBeenCalled(); + expect(mocks.invalidateMemberContextCache).not.toHaveBeenCalled(); + }, + ); +}); diff --git a/server/tests/unit/require-admin-cross-tenant.test.ts b/server/tests/unit/require-admin-cross-tenant.test.ts index 176826660a..70bd516d89 100644 --- a/server/tests/unit/require-admin-cross-tenant.test.ts +++ b/server/tests/unit/require-admin-cross-tenant.test.ts @@ -1,15 +1,8 @@ /** - * Unit tests for the cross-tenant defense added to `requireAdmin`. - * - * Surfaced by the security review on PR #4609 / issue #4501: a WorkOS - * API key carrying `admin:*` is tenant-scoped by issuance (the - * permission grants admin within the issuing org, not across orgs). - * Before this gate, any org holding such a key could mutate any other - * org's data via admin routes whose path resolves a target org. - * - * Exercises the REAL middleware (not a mock) by stubbing the upstream - * `req.apiKey`/`req.params` shape that auth-and-routing would have set - * by the time control reaches `requireAdmin`. + * Unit tests for platform-admin and explicit tenant-admin boundaries. + * `requireAdmin` rejects every tenant WorkOS key; the audited + * `requireTenantAdminForOrganization` middleware grants only permission- and + * issuer-bound access to a literal `:orgId` target. */ import { describe, it, expect, afterAll, beforeAll } from 'vitest'; import express from 'express'; @@ -25,10 +18,9 @@ process.env.WORKOS_COOKIE_PASSWORD = const { requireAdmin, + requireTenantAdminForOrganization, requireAuth, requireGlobalAdmin, - refuseAnyApiKeyOnGlobalAdmin, - refuseCrossTenantAdminApiKey, stopAuthTimers, } = await import('../../src/middleware/auth.js'); @@ -62,13 +54,13 @@ describe('requireAdmin cross-tenant API key defense', () => { next(); }); - app.get('/api/admin/accounts/:orgId/agents', requireAdmin, (_req, res) => { + app.get('/api/admin/accounts/:orgId/agents', requireTenantAdminForOrganization, (_req, res) => { res.json({ ok: true }); }); app.delete( '/api/admin/accounts/:orgId/agents/:url', - requireAdmin, + requireTenantAdminForOrganization, (_req, res) => { res.json({ ok: true }); }, @@ -76,18 +68,23 @@ describe('requireAdmin cross-tenant API key defense', () => { app.post( '/api/admin/accounts/:orgId/agents', - requireAdmin, + requireTenantAdminForOrganization, (_req, res) => { res.json({ ok: true }); }, ); - // Route without :orgId — gate should NOT engage even with a - // tenant-scoped API key; this proves the check is opt-in by path - // convention rather than blanket-deny. + // Route without :orgId — tenant-scoped keys must fail closed because + // there is no organization target to bind to the key's issuer. app.get('/api/admin/stats', requireAdmin, (_req, res) => { res.json({ ok: true }); }); + + // A matching org parameter does not make a platform financial route safe + // for tenant keys. Only the explicit tenant middleware above may do that. + app.post('/api/admin/organizations/:orgId/discount', requireAdmin, (_req, res) => { + res.json({ ok: true }); + }); }); it('refuses an admin:* API key when its issuing org does not match :orgId', async () => { @@ -127,15 +124,22 @@ describe('requireAdmin cross-tenant API key defense', () => { expect(res.body.error).toBe('cross_tenant_api_key'); }); - it('does NOT engage on routes without a :orgId path param', async () => { - // A tenant-scoped key with admin:* hitting a non-tenant-scoped admin - // route should still pass — the check is convention-based on the - // route shape, not a blanket allow-list. + it('refuses a tenant-scoped admin:* key on routes without a :orgId path param', async () => { const res = await request(app) .get('/api/admin/stats') .set('x-test-api-key-org-id', 'org_caller'); - expect(res.status).toBe(200); + expect(res.status).toBe(403); + expect(res.body.error).toBe('global_admin_required'); + }); + + it('refuses a tenant key on a platform-admin route even when :orgId matches', async () => { + const res = await request(app) + .post('/api/admin/organizations/org_same/discount') + .set('x-test-api-key-org-id', 'org_same'); + + expect(res.status).toBe(403); + expect(res.body.error).toBe('global_admin_required'); }); it('lets the static admin_api_key through cross-tenant routes (not tenant-scoped)', async () => { @@ -146,6 +150,14 @@ describe('requireAdmin cross-tenant API key defense', () => { expect(res.status).toBe(200); }); + it('lets the static admin_api_key through platform-global routes', async () => { + const res = await request(app) + .get('/api/admin/stats') + .set('x-test-static-admin', '1'); + + expect(res.status).toBe(200); + }); + it('still rejects keys with admin:read for write operations on the matched org', async () => { // Same-tenant admin:read key — the cross-tenant gate passes, then the // existing permission check rejects the DELETE because admin:read is @@ -189,135 +201,10 @@ describe('requireAdmin cross-tenant API key defense', () => { }); }); -describe('refuseCrossTenantAdminApiKey + refuseAnyApiKeyOnGlobalAdmin helpers', () => { - let app: express.Application; - - beforeAll(() => { - app = express(); - - app.use((req, _res, next) => { - const apiKeyHeader = req.headers['x-test-api-key-org-id']; - if (typeof apiKeyHeader === 'string') { - (req as any).apiKey = { - id: 'apikey_test', - organizationId: apiKeyHeader, - permissions: ['admin:*'], - }; - } - next(); - }); - - // Route keyed on a UUID-style :id with the target org resolved - // dynamically (mimics /api/admin/member-profiles/:id PUT/DELETE). - // The handler invokes refuseCrossTenantAdminApiKey after the lookup. - app.put('/profiles/:id', (req, res) => { - const targetOrgId = req.headers['x-test-resolved-org'] as string; - if (refuseCrossTenantAdminApiKey(req, res, targetOrgId)) return; - res.json({ ok: true }); - }); - - // Route operating on global state (mimics /api/admin/users/:userId/*). - app.put('/users/:userId/name', (req, res) => { - if (refuseAnyApiKeyOnGlobalAdmin(req, res)) return; - res.json({ ok: true }); - }); - }); - - it('refuses cross-tenant API key on a profile UUID route', async () => { - const res = await request(app) - .put('/profiles/profile-xyz') - .set('x-test-api-key-org-id', 'org_caller') - .set('x-test-resolved-org', 'org_target'); - expect(res.status).toBe(403); - expect(res.body.error).toBe('cross_tenant_api_key'); - }); - - it('allows same-tenant API key on a profile UUID route', async () => { - const res = await request(app) - .put('/profiles/profile-xyz') - .set('x-test-api-key-org-id', 'org_same') - .set('x-test-resolved-org', 'org_same'); - expect(res.status).toBe(200); - }); - - it('allows the route when no api key is present (SSO admin / static admin)', async () => { - const res = await request(app) - .put('/profiles/profile-xyz') - .set('x-test-resolved-org', 'org_target'); - expect(res.status).toBe(200); - }); - - it('refuses ANY tenant-scoped API key on a global-admin route', async () => { - const res = await request(app) - .put('/users/userid_abc/name') - .set('x-test-api-key-org-id', 'org_caller'); - expect(res.status).toBe(403); - expect(res.body.error).toBe('global_admin_required'); - }); - - it('allows the global-admin route when no api key is present', async () => { - const res = await request(app).put('/users/userid_abc/name'); - expect(res.status).toBe(200); - }); -}); - describe('requireGlobalAdmin composite middleware', () => { - // The composite chain wraps `requireAuth` + the cross-tenant refusal - // + `requireAdmin` so a router can opt into "this whole surface is - // global-state admin only" via `...requireGlobalAdmin` instead of - // remembering to add a per-handler gate. The 7 originally-unprotected - // `/api/admin/users` routes are the motivating case (security review - // on #4646: per-handler enforcement risked silent regression every - // time a new route was added). Full end-to-end coverage of the - // chain belongs in the admin-users integration tests where real - // requireAuth has a real session to validate; here we pin the - // composition shape so a future refactor doesn't silently re-order - // or drop a middleware from the chain. - it('is a 3-element middleware array in the documented order', () => { - expect(requireGlobalAdmin).toHaveLength(3); - // First and last are the existing requireAuth / requireAdmin - // exports; the middle is the chain's new contribution. Pinning - // identities here means a future "I'll just swap in a different - // requireAuth" refactor has to update the test, surfacing the - // change explicitly. + it('composes authentication before the platform-admin boundary', () => { + expect(requireGlobalAdmin).toHaveLength(2); expect(requireGlobalAdmin[0]).toBe(requireAuth); - expect(requireGlobalAdmin[2]).toBe(requireAdmin); - expect(typeof requireGlobalAdmin[1]).toBe('function'); - }); - - it('the middle middleware refuses an apiKey-bearing request and short-circuits next()', async () => { - const middle = requireGlobalAdmin[1]; - - let nextCalled = false; - const calls: { status?: number; body?: unknown } = {}; - const req = { apiKey: { id: 'k', organizationId: 'org_caller', permissions: ['admin:*'] }, path: '/x', method: 'GET' } as any; - const res = { - status(code: number) { - calls.status = code; - return this; - }, - json(body: unknown) { - calls.body = body; - return this; - }, - } as any; - await middle(req, res, () => { - nextCalled = true; - }); - expect(nextCalled).toBe(false); - expect(calls.status).toBe(403); - expect((calls.body as { error?: string })?.error).toBe('global_admin_required'); - }); - - it('the middle middleware calls next() when no apiKey is present', async () => { - const middle = requireGlobalAdmin[1]; - - let nextCalled = false; - const req = { path: '/x', method: 'GET' } as any; - const res = {} as any; - await middle(req, res, () => { - nextCalled = true; - }); - expect(nextCalled).toBe(true); + expect(requireGlobalAdmin[1]).toBe(requireAdmin); }); }); From d9a6fd22a65a620a49a5ee654a51cbc8bf9a664e Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 30 Jul 2026 08:23:21 -0400 Subject: [PATCH 2/2] Fix integration auth mocks --- server/tests/integration/content-my-content.test.ts | 4 ++-- server/tests/integration/event-speakers.test.ts | 4 ++-- server/tests/integration/events-draft-preview.test.ts | 4 ++-- server/tests/integration/perspective-assets.test.ts | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/tests/integration/content-my-content.test.ts b/server/tests/integration/content-my-content.test.ts index b9315fc262..d260dab8c5 100644 --- a/server/tests/integration/content-my-content.test.ts +++ b/server/tests/integration/content-my-content.test.ts @@ -33,6 +33,7 @@ vi.mock('../../src/middleware/auth.js', () => { return { requireAuth: requireAuthMock, requireAdmin: passthrough, + requireTenantAdminForOrganization: passthrough, optionalAuth: (req: any, _res: any, next: any) => { setTestUser(req); next(); }, requireCompanyAccess: passthrough, requireActiveSubscription: passthrough, @@ -40,12 +41,11 @@ vi.mock('../../src/middleware/auth.js', () => { requireRole: () => passthrough, createRequireWorkingGroupLeader: () => passthrough, createRequireWorkingGroupMember: () => passthrough, - refuseCrossTenantAdminApiKey: () => false, refuseAnyApiKeyOnGlobalAdmin: () => false, // Composite chain for /api/admin/users routes — see auth.ts. // Captured-at-load-time references mean the per-export mocks above // can't propagate into the production array, so re-build it here. - requireGlobalAdmin: [requireAuthMock, passthrough, passthrough], + requireGlobalAdmin: [requireAuthMock, passthrough], invalidateSessionCache: vi.fn(), invalidateBanCache: vi.fn(), invalidateSessionsForUsers: vi.fn(), diff --git a/server/tests/integration/event-speakers.test.ts b/server/tests/integration/event-speakers.test.ts index 9d26e042b4..bee3288fd9 100644 --- a/server/tests/integration/event-speakers.test.ts +++ b/server/tests/integration/event-speakers.test.ts @@ -28,6 +28,7 @@ vi.mock('../../src/middleware/auth.js', () => { return { requireAuth: requireAuthMock, requireAdmin: passthrough, + requireTenantAdminForOrganization: passthrough, optionalAuth: (req: any, _res: any, next: any) => { setTestUser(req); next(); }, requireCompanyAccess: passthrough, requireActiveSubscription: passthrough, @@ -35,9 +36,8 @@ vi.mock('../../src/middleware/auth.js', () => { requireRole: () => passthrough, createRequireWorkingGroupLeader: () => passthrough, createRequireWorkingGroupMember: () => passthrough, - refuseCrossTenantAdminApiKey: () => false, refuseAnyApiKeyOnGlobalAdmin: () => false, - requireGlobalAdmin: [requireAuthMock, passthrough, passthrough], + requireGlobalAdmin: [requireAuthMock, passthrough], invalidateSessionCache: vi.fn(), invalidateBanCache: vi.fn(), invalidateSessionsForUsers: vi.fn(), diff --git a/server/tests/integration/events-draft-preview.test.ts b/server/tests/integration/events-draft-preview.test.ts index 798b63ec14..c34217daac 100644 --- a/server/tests/integration/events-draft-preview.test.ts +++ b/server/tests/integration/events-draft-preview.test.ts @@ -29,6 +29,7 @@ vi.mock('../../src/middleware/auth.js', () => { return { requireAuth: requireAuthMock, requireAdmin: passthrough, + requireTenantAdminForOrganization: passthrough, optionalAuth: (req: any, _res: any, next: any) => { setTestUser(req); next(); }, requireCompanyAccess: passthrough, requireActiveSubscription: passthrough, @@ -36,9 +37,8 @@ vi.mock('../../src/middleware/auth.js', () => { requireRole: () => passthrough, createRequireWorkingGroupLeader: () => passthrough, createRequireWorkingGroupMember: () => passthrough, - refuseCrossTenantAdminApiKey: () => false, refuseAnyApiKeyOnGlobalAdmin: () => false, - requireGlobalAdmin: [requireAuthMock, passthrough, passthrough], + requireGlobalAdmin: [requireAuthMock, passthrough], invalidateSessionCache: vi.fn(), invalidateBanCache: vi.fn(), invalidateSessionsForUsers: vi.fn(), diff --git a/server/tests/integration/perspective-assets.test.ts b/server/tests/integration/perspective-assets.test.ts index 8b2d3ebd1d..03a896ec36 100644 --- a/server/tests/integration/perspective-assets.test.ts +++ b/server/tests/integration/perspective-assets.test.ts @@ -34,6 +34,7 @@ vi.mock('../../src/middleware/auth.js', () => { return { requireAuth: requireAuthMock, requireAdmin: passthrough, + requireTenantAdminForOrganization: passthrough, optionalAuth: (req: any, _res: any, next: any) => { setTestUser(req); next(); }, requireCompanyAccess: passthrough, requireActiveSubscription: passthrough, @@ -41,9 +42,8 @@ vi.mock('../../src/middleware/auth.js', () => { requireRole: () => passthrough, createRequireWorkingGroupLeader: () => passthrough, createRequireWorkingGroupMember: () => passthrough, - refuseCrossTenantAdminApiKey: () => false, refuseAnyApiKeyOnGlobalAdmin: () => false, - requireGlobalAdmin: [requireAuthMock, passthrough, passthrough], + requireGlobalAdmin: [requireAuthMock, passthrough], invalidateSessionCache: vi.fn(), invalidateBanCache: vi.fn(), invalidateSessionsForUsers: vi.fn(),