From 8bfe06c62b52b463ca00eb1f0c00aaf8844f5299 Mon Sep 17 00:00:00 2001 From: Uwamari Goddey Date: Mon, 5 Oct 2026 14:01:36 -0400 Subject: [PATCH 1/2] fix(auth): block viewer workspace mutations Creating and deleting teams, deployments and infrastructure rows, the AWS cost sync, and POST /api/services/discover checked only that the caller was authenticated, so a viewer could do all of them. They now require owner, admin or member (requireMember, on the caller's current membership role). Nothing changes for those three roles. POST /api/services/discover also takes the existing discoveryRateLimiter, so it shares the per-organization discovery budget with POST /api/aws-resources/discover instead of being unlimited. POST /api/infrastructure/sync-aws gets a per-organization limit of its own: a sync whose result is not already cached makes a billed Cost Explorer call. The GitHub webhook and the scheduled discovery job carry no user and do not pass through these checks. Co-Authored-By: Claude Opus 5.5 --- backend/src/middleware/rateLimiter.ts | 24 + .../workspace-mutation-viewer-gate.test.ts | 481 ++++++++++++++++++ backend/src/routes/deployments.routes.ts | 5 +- backend/src/routes/infrastructure.routes.ts | 8 +- backend/src/routes/services.routes.ts | 4 +- backend/src/routes/teams.routes.ts | 5 +- 6 files changed, 519 insertions(+), 8 deletions(-) create mode 100644 backend/src/routes/__tests__/workspace-mutation-viewer-gate.test.ts diff --git a/backend/src/middleware/rateLimiter.ts b/backend/src/middleware/rateLimiter.ts index 505ebf12..b62a64fe 100644 --- a/backend/src/middleware/rateLimiter.ts +++ b/backend/src/middleware/rateLimiter.ts @@ -182,6 +182,30 @@ export const discoveryRateLimiter = rateLimit({ }, }); +/** + * Rate limiter for the manual AWS cost sync + * A sync whose result is not already cached makes a billed Cost Explorer + * call, so it is limited per organization, not per IP. + * In-memory: per backend process. + */ +export const costSyncRateLimiter = rateLimit({ + windowMs: 60 * 60 * 1000, // 1 hour window + max: 10, + + standardHeaders: true, + legacyHeaders: false, + + keyGenerator: (req: Request) => `cost-sync:${req.user?.organizationId ?? 'unauthenticated'}`, + + handler: (req: Request, res: Response) => { + res.status(429).json({ + success: false, + error: 'AWS cost sync rate limit reached. Maximum 10 syncs per hour per organization.', + retry_after: 3600, + }); + }, +}); + /** * Rate limiter for authentication endpoints (login, register) * Prevents brute force attacks diff --git a/backend/src/routes/__tests__/workspace-mutation-viewer-gate.test.ts b/backend/src/routes/__tests__/workspace-mutation-viewer-gate.test.ts new file mode 100644 index 00000000..a75e66b8 --- /dev/null +++ b/backend/src/routes/__tests__/workspace-mutation-viewer-gate.test.ts @@ -0,0 +1,481 @@ +/** + * Viewers cannot change teams, deployments or infrastructure, or start + * discovery. + * + * Policy under test: + * - POST and DELETE on /api/teams, /api/deployments and /api/infrastructure, + * POST /api/infrastructure/sync-aws and POST /api/services/discover refuse + * a viewer with 403 and do nothing. + * - Owners, admins and members are answered exactly as before. + * - The role is the caller's current membership, not the claim in the token. + * - POST /api/services/discover draws on the same per-organization discovery + * budget as POST /api/aws-resources/discover. A refused viewer spends none + * of it. + * - POST /api/infrastructure/sync-aws has its own per-organization budget. + * - The GitHub webhook carries no user and is not subject to any of this. + * + * Real routes over an in-process HTTP server against live Postgres. Stubbed: + * authService.verifyToken (to choose the caller), and the two calls that + * would otherwise reach AWS -- discovery itself and the Cost Explorer fetch. + */ +import crypto, { randomUUID } from 'crypto'; +import express from 'express'; +import http from 'http'; +import { Pool } from 'pg'; +import teamsRoutes from '../teams.routes'; +import deploymentsRoutes from '../deployments.routes'; +import infrastructureRoutes from '../infrastructure.routes'; +import servicesRoutes from '../services.routes'; +import awsResourcesRoutes from '../awsResources.routes'; +import { errorHandler } from '../../middleware/error-handler'; +import { authService } from '../../services/auth.service'; +import { AWSResourceDiscoveryService } from '../../services/awsResourceDiscovery'; +import awsCostService from '../../services/aws-cost.service'; +import { pool as appPool } from '../../config/database'; + +// Listeners issue their own fire-and-forget queries; irrelevant here. +jest.mock('../../services/onboardingEvents', () => ({ emitOnboardingEvent: jest.fn() })); + +function dbConfig() { + return { + host: process.env.DB_HOST || 'localhost', + port: parseInt(process.env.DB_PORT || '5432'), + database: process.env.DB_NAME || 'platform_portal', + user: process.env.DB_USER || 'postgres', + password: process.env.DB_PASSWORD || 'postgres', + }; +} + +const WEBHOOK_SECRET = 'workspace-viewer-gate-test-secret'; +// The webhook reads its organization once, at module load. +const WEBHOOK_ORG_ID = randomUUID(); +// syncAWS only checks that these are set; the Cost Explorer fetch is stubbed. +const AWS_ENV = ['AWS_ACCESS_KEY_ID', 'AWS_SECRET_ACCESS_KEY', 'AWS_REGION'] as const; +const DISCOVERY_BUDGET = 10; +const COST_SYNC_BUDGET = 10; + +const VIEWER_REFUSAL = { + success: false, + error: 'Insufficient permissions', + required: ['owner', 'admin', 'member'], + current: 'viewer', +}; + +const pool = new Pool(dbConfig()); +const createdOrgIds: string[] = []; +const createdUserIds: string[] = []; +const savedEnv: Record = {}; + +function uniqueSuffix(): string { + return `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; +} + +async function insertOrg(id?: string): Promise { + const suffix = uniqueSuffix(); + const { rows } = await pool.query( + `INSERT INTO organizations (id, name, slug, display_name, subscription_tier, subscription_status) + VALUES (COALESCE($1::uuid, gen_random_uuid()), $2, $3, $4, 'enterprise', 'active') RETURNING id`, + [id ?? null, `Viewer Gate ${suffix}`, `viewer-gate-${suffix}`, `Viewer Gate ${suffix}`] + ); + createdOrgIds.push(rows[0].id); + return rows[0].id as string; +} + +async function member(orgId: string, role: string): Promise { + const user = await pool.query( + `INSERT INTO users (email, password_hash, full_name) VALUES ($1, 'x', 'Viewer Gate User') RETURNING id`, + [`viewer-gate-${role}-${uniqueSuffix()}@example.com`] + ); + createdUserIds.push(user.rows[0].id); + await pool.query( + `INSERT INTO organization_memberships (organization_id, user_id, role, joined_at, is_active) + VALUES ($1, $2, $3, NOW(), true)`, + [orgId, user.rows[0].id, role] + ); + return user.rows[0].id as string; +} + +async function insertTeam(orgId: string): Promise { + const { rows } = await pool.query( + `INSERT INTO teams (name, owner, organization_id) VALUES ($1, 'owner@example.com', $2) RETURNING id`, + [`viewer-gate-team-${uniqueSuffix()}`, orgId] + ); + return rows[0].id as string; +} + +async function insertService(orgId: string): Promise { + const { rows } = await pool.query( + `INSERT INTO services (name, template, owner, status, organization_id) + VALUES ($1, 'api', 'owner@example.com', 'active', $2) RETURNING id`, + [`viewer-gate-svc-${uniqueSuffix()}`, orgId] + ); + return rows[0].id as string; +} + +async function insertDeployment(orgId: string, serviceId: string): Promise { + const { rows } = await pool.query( + `INSERT INTO deployments (service_id, environment, aws_region, status, deployed_by, organization_id) + VALUES ($1, 'production', 'us-east-1', 'running', 'viewer-gate', $2) RETURNING id`, + [serviceId, orgId] + ); + return rows[0].id as string; +} + +async function insertInfrastructure(orgId: string, serviceId: string): Promise { + const { rows } = await pool.query( + `INSERT INTO infrastructure_resources (service_id, resource_type, aws_id, aws_region, status, cost_per_month, organization_id) + VALUES ($1, 'ec2', $2, 'us-east-1', 'running', 1, $3) RETURNING id`, + [serviceId, `i-${uniqueSuffix()}`, orgId] + ); + return rows[0].id as string; +} + +async function buildOrg(id?: string) { + const orgId = await insertOrg(id); + return { + orgId, + serviceId: await insertService(orgId), + owner: await member(orgId, 'owner'), + admin: await member(orgId, 'admin'), + member: await member(orgId, 'member'), + viewer: await member(orgId, 'viewer'), + }; +} + +type Org = Awaited>; +type Role = 'owner' | 'admin' | 'member' | 'viewer'; + +async function count(table: string, orgId: string): Promise { + const { rows } = await pool.query(`SELECT COUNT(*)::int AS n FROM ${table} WHERE organization_id = $1`, [orgId]); + return rows[0].n; +} + +async function exists(table: string, id: string): Promise { + const { rows } = await pool.query(`SELECT 1 FROM ${table} WHERE id = $1`, [id]); + return rows.length === 1; +} + +let server: http.Server; +let baseUrl: string; +let org: Org; +let webhookOrg: Org; +/** What a sync answers once past the gate: this schema may refuse the cost row it writes. */ +let syncStatus: number; +let discoverSpy: jest.SpyInstance; +let costSpy: jest.SpyInstance; + +beforeAll(async () => { + process.env.GITHUB_WEBHOOK_ORG_ID = WEBHOOK_ORG_ID; + process.env.GITHUB_WEBHOOK_SECRET = WEBHOOK_SECRET; + for (const key of AWS_ENV) { + savedEnv[key] = process.env[key]; + process.env[key] = key === 'AWS_REGION' ? 'us-east-1' : 'not-a-credential'; + } + // Loaded after the environment is set: the router captures its + // organization when the module is first evaluated. + const githubWebhookRoutes = require('../github-webhook.routes').default; + + const nullable = await pool.query( + `SELECT is_nullable FROM information_schema.columns + WHERE table_schema = current_schema() AND table_name = 'infrastructure_resources' AND column_name = 'service_id'` + ); + // syncAWS writes its cost row with no service; where the column is NOT NULL + // that insert fails and the route has always answered 500. + syncStatus = nullable.rows[0].is_nullable === 'YES' ? 200 : 500; + + org = await buildOrg(); + webhookOrg = await buildOrg(WEBHOOK_ORG_ID); + + const app = express(); + // Same as server.ts: the webhook verifies its signature over the raw body. + app.use('/api/webhooks/github', express.raw({ type: 'application/json' })); + app.use('/api/webhooks/github', githubWebhookRoutes); + app.use(express.json()); + app.use('/api/teams', teamsRoutes); + app.use('/api/deployments', deploymentsRoutes); + app.use('/api/infrastructure', infrastructureRoutes); + app.use('/api/services', servicesRoutes); + app.use('/api/aws-resources', awsResourcesRoutes); + app.use(errorHandler); + server = http.createServer(app); + await new Promise((resolve) => server.listen(0, resolve)); + const address = server.address(); + const port = typeof address === 'object' && address ? address.port : 0; + baseUrl = `http://127.0.0.1:${port}/api`; +}); + +beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(() => {}); + jest.spyOn(console, 'log').mockImplementation(() => {}); + discoverSpy = jest + .spyOn(AWSResourceDiscoveryService.prototype, 'discoverAllResources') + .mockResolvedValue({ job_id: 'stubbed', resources_discovered: 0, resources_updated: 0, errors: [] } as never); + costSpy = jest.spyOn(awsCostService, 'fetchMonthlyCosts').mockResolvedValue({ + total: 12.5, + byService: [], + period: { start: '2026-10-01', end: '2026-10-05' }, + } as never); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + for (const key of AWS_ENV) { + if (savedEnv[key] === undefined) delete process.env[key]; + else process.env[key] = savedEnv[key]; + } + await pool.query('DELETE FROM infrastructure_resources WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM deployments WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM services WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM teams WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM audit_logs WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM analytics_events WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query( + 'DELETE FROM organization_memberships WHERE organization_id = ANY($1) OR user_id = ANY($2)', + [createdOrgIds, createdUserIds] + ); + await pool.query('DELETE FROM organizations WHERE id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM users WHERE id = ANY($1)', [createdUserIds]); + await pool.end(); + await appPool.end(); +}); + +/** + * A request authenticated as the `role` member of `target`. The token's role + * CLAIM defaults to 'owner', so every refusal below is proven to come from + * the caller's current membership rather than the claim. + */ +function send(target: Org, role: Role, method: string, path: string, body?: unknown, jwtRole = 'owner') { + jest.spyOn(authService, 'verifyToken').mockReturnValue({ + userId: target[role], + email: 'viewer-gate-caller@example.com', + organizationId: target.orgId, + role: jwtRole, + type: 'access', + } as unknown as ReturnType); + return fetch(`${baseUrl}${path}`, { + method, + headers: { Authorization: 'Bearer test-token', 'Content-Type': 'application/json' }, + body: body === undefined ? undefined : JSON.stringify(body), + }); +} + +interface Mutation { + name: string; + /** Anything the request needs to exist first; its result is handed to the other steps. */ + prepare: () => Promise; + request: (role: Role, prepared: string) => Promise; + /** Status for owner, admin and member: unchanged by the gate. */ + allowed: () => number; + /** True when the mutation happened. */ + happened: (prepared: string) => Promise; +} + +const mutations: Mutation[] = [ + { + name: 'POST /api/teams', + prepare: async () => `viewer-gate-new-team-${uniqueSuffix()}`, + request: (role, name) => send(org, role, 'POST', '/teams', { name, owner: 'owner@example.com' }), + allowed: () => 201, + happened: async (name) => (await pool.query('SELECT 1 FROM teams WHERE name = $1', [name])).rows.length === 1, + }, + { + name: 'DELETE /api/teams/:id', + prepare: () => insertTeam(org.orgId), + request: (role, id) => send(org, role, 'DELETE', `/teams/${id}`), + allowed: () => 200, + happened: async (id) => !(await exists('teams', id)), + }, + { + name: 'POST /api/deployments', + prepare: async () => String(await count('deployments', org.orgId)), + request: (role) => send(org, role, 'POST', '/deployments', { service_id: org.serviceId, environment: 'production' }), + allowed: () => 201, + happened: async (before) => (await count('deployments', org.orgId)) === Number(before) + 1, + }, + { + name: 'DELETE /api/deployments/:id', + prepare: () => insertDeployment(org.orgId, org.serviceId), + request: (role, id) => send(org, role, 'DELETE', `/deployments/${id}`), + allowed: () => 200, + happened: async (id) => !(await exists('deployments', id)), + }, + { + name: 'POST /api/infrastructure', + prepare: async () => `i-new-${uniqueSuffix()}`, + request: (role, awsId) => + send(org, role, 'POST', '/infrastructure', { + service_id: org.serviceId, + resource_type: 'ec2', + aws_id: awsId, + aws_region: 'us-east-1', + status: 'running', + cost_per_month: 3, + }), + allowed: () => 201, + happened: async (awsId) => + (await pool.query('SELECT 1 FROM infrastructure_resources WHERE aws_id = $1', [awsId])).rows.length === 1, + }, + { + name: 'DELETE /api/infrastructure/:id', + prepare: () => insertInfrastructure(org.orgId, org.serviceId), + request: (role, id) => send(org, role, 'DELETE', `/infrastructure/${id}`), + allowed: () => 200, + happened: async (id) => !(await exists('infrastructure_resources', id)), + }, + { + name: 'POST /api/infrastructure/sync-aws', + prepare: async () => '', + request: (role) => send(org, role, 'POST', '/infrastructure/sync-aws'), + allowed: () => syncStatus, + happened: async () => costSpy.mock.calls.length === 1, + }, + { + name: 'POST /api/services/discover', + prepare: async () => '', + request: (role) => send(org, role, 'POST', '/services/discover'), + allowed: () => 200, + happened: async () => discoverSpy.mock.calls.length === 1, + }, +]; + +describe.each(mutations)('$name', ({ prepare, request, allowed, happened }) => { + it('refuses a viewer with 403 and does nothing', async () => { + const prepared = await prepare(); + + const res = await request('viewer', prepared); + + expect(res.status).toBe(403); + expect(await res.json()).toEqual(VIEWER_REFUSAL); + expect(await happened(prepared)).toBe(false); + expect(discoverSpy).not.toHaveBeenCalled(); + expect(costSpy).not.toHaveBeenCalled(); + }); + + it.each(['owner', 'admin', 'member'] as const)('answers a %s as before', async (role) => { + const prepared = await prepare(); + + const res = await request(role, prepared); + + expect(res.status).toBe(allowed()); + expect(await happened(prepared)).toBe(true); + }); +}); + +describe('the decision follows the current membership, not the token', () => { + it('a member whose token claims viewer is allowed, and a viewer whose token claims owner is refused', async () => { + const name = `viewer-gate-claim-${uniqueSuffix()}`; + + const asViewer = await send(org, 'viewer', 'POST', '/teams', { name, owner: 'owner@example.com' }, 'owner'); + const asMember = await send(org, 'member', 'POST', '/teams', { name, owner: 'owner@example.com' }, 'viewer'); + + expect(asViewer.status).toBe(403); + expect(asMember.status).toBe(201); + }); + + it('without a token every gated mutation is 401', async () => { + for (const [method, path] of [ + ['POST', '/teams'], + ['DELETE', `/teams/${randomUUID()}`], + ['POST', '/deployments'], + ['DELETE', `/deployments/${randomUUID()}`], + ['POST', '/infrastructure'], + ['DELETE', `/infrastructure/${randomUUID()}`], + ['POST', '/infrastructure/sync-aws'], + ['POST', '/services/discover'], + ]) { + const res = await fetch(`${baseUrl}${path}`, { method }); + expect([method, path, res.status]).toEqual([method, path, 401]); + } + }); +}); + +describe('discovery budget', () => { + const discover = (target: Org, role: Role) => send(target, role, 'POST', '/services/discover'); + const adminDiscover = (target: Org, role: Role) => send(target, role, 'POST', '/aws-resources/discover'); + + it('is one budget per organization across both discover endpoints, and a refused viewer spends none of it', async () => { + const limited = await buildOrg(); + const other = await buildOrg(); + + for (let i = 0; i < 3; i++) expect((await discover(limited, 'viewer')).status).toBe(403); + + // Alternating endpoints: together they get the budget once, not once each. + for (let i = 0; i < DISCOVERY_BUDGET; i++) { + const res = i % 2 === 0 ? await discover(limited, 'member') : await adminDiscover(limited, 'admin'); + expect([i, res.status]).toEqual([i, 200]); + } + expect(discoverSpy).toHaveBeenCalledTimes(DISCOVERY_BUDGET); + + const overServices = await discover(limited, 'owner'); + const overAwsResources = await adminDiscover(limited, 'owner'); + + expect(overServices.status).toBe(429); + expect(await overServices.json()).toMatchObject({ success: false, retry_after: 3600 }); + expect(overAwsResources.status).toBe(429); + expect(discoverSpy).toHaveBeenCalledTimes(DISCOVERY_BUDGET); + + // Another organization is unaffected. + expect((await discover(other, 'member')).status).toBe(200); + expect(discoverSpy).toHaveBeenLastCalledWith(other.orgId); + }); +}); + +describe('cost sync budget', () => { + const sync = (target: Org, role: Role) => send(target, role, 'POST', '/infrastructure/sync-aws'); + + it('is its own budget per organization, apart from discovery', async () => { + const limited = await buildOrg(); + const other = await buildOrg(); + + for (let i = 0; i < 3; i++) expect((await sync(limited, 'viewer')).status).toBe(403); + for (let i = 0; i < COST_SYNC_BUDGET; i++) { + expect([i, (await sync(limited, 'member')).status]).toEqual([i, syncStatus]); + } + + const over = await sync(limited, 'owner'); + + expect(over.status).toBe(429); + expect(await over.json()).toMatchObject({ success: false, retry_after: 3600 }); + expect(costSpy).toHaveBeenCalledTimes(COST_SYNC_BUDGET); + // Neither the other organization nor this one's discovery budget is touched. + expect((await sync(other, 'member')).status).toBe(syncStatus); + expect((await send(limited, 'member', 'POST', '/services/discover')).status).toBe(200); + }); +}); + +describe('automation that carries no user is unaffected', () => { + it('a signed GitHub delivery still records a deployment', async () => { + const body = JSON.stringify({ + action: 'completed', + workflow_job: { + id: Math.floor(Math.random() * 1e12), + run_id: Math.floor(Math.random() * 1e12), + name: 'deploy-backend', + conclusion: 'success', + completed_at: new Date().toISOString(), + head_sha: 'abc123', + html_url: 'https://example.com/run', + }, + repository: { name: `viewer-gate-repo-${uniqueSuffix()}` }, + sender: { login: 'viewer-gate' }, + }); + const sign = (secret: string) => 'sha256=' + crypto.createHmac('sha256', secret).update(body).digest('hex'); + const deliver = (signature: string) => + fetch(`${baseUrl}/webhooks/github`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'X-GitHub-Event': 'workflow_job', 'X-Hub-Signature-256': signature }, + body, + }); + + const unsigned = await deliver(sign('wrong-secret')); + const signed = await deliver(sign(WEBHOOK_SECRET)); + + expect(unsigned.status).toBe(401); + expect(signed.status).toBe(201); + expect((await signed.json()).data).toMatchObject({ organization_id: webhookOrg.orgId, status: 'success' }); + }); +}); diff --git a/backend/src/routes/deployments.routes.ts b/backend/src/routes/deployments.routes.ts index b9fef01c..80f390a3 100644 --- a/backend/src/routes/deployments.routes.ts +++ b/backend/src/routes/deployments.routes.ts @@ -2,6 +2,7 @@ import { Router, Request, Response } from 'express'; import { pool } from '../config/database'; import { DeploymentsController } from '../controllers/deployments.controller'; import { authenticateToken } from '../middleware/auth.middleware'; +import { requireMember } from '../middleware/rbac.middleware'; import { checkResourceLimit } from '../middleware/subscription.middleware'; const router = Router(); @@ -69,7 +70,7 @@ router.get('/:id', authenticateToken, (req, res) => controller.getById(req, res) // ─── POST / — resolve serviceName → service_id, then delegate ──────────────── -router.post('/', authenticateToken, checkResourceLimit('deployments', 1), async (req: Request, res: Response): Promise => { +router.post('/', authenticateToken, requireMember, checkResourceLimit('deployments', 1), async (req: Request, res: Response): Promise => { try { const orgId = req.organizationId; let { service_id, service_name, serviceName, environment, aws_region, region, deployed_by, version } = req.body; @@ -129,6 +130,6 @@ router.post('/', authenticateToken, checkResourceLimit('deployments', 1), async } }); -router.delete('/:id', authenticateToken, (req, res) => controller.delete(req, res)); +router.delete('/:id', authenticateToken, requireMember, (req, res) => controller.delete(req, res)); export default router; diff --git a/backend/src/routes/infrastructure.routes.ts b/backend/src/routes/infrastructure.routes.ts index eeeb0858..45a1f392 100644 --- a/backend/src/routes/infrastructure.routes.ts +++ b/backend/src/routes/infrastructure.routes.ts @@ -1,6 +1,8 @@ import { Router } from 'express'; import { InfrastructureController } from '../controllers/infrastructure.controller'; import { authenticateToken } from '../middleware/auth.middleware'; +import { requireMember } from '../middleware/rbac.middleware'; +import { costSyncRateLimiter } from '../middleware/rateLimiter'; import { checkDiscoveryLimit } from '../middleware/subscription.middleware'; const router = Router(); @@ -10,9 +12,9 @@ router.use(authenticateToken); router.get('/', checkDiscoveryLimit, (req, res) => controller.getAll(req, res)); router.get('/costs', (req, res) => controller.getCosts(req, res)); -router.post('/sync-aws', (req, res) => controller.syncAWS(req, res)); +router.post('/sync-aws', requireMember, costSyncRateLimiter, (req, res) => controller.syncAWS(req, res)); router.get('/:id', (req, res) => controller.getById(req, res)); -router.post('/', (req, res) => controller.create(req, res)); -router.delete('/:id', (req, res) => controller.delete(req, res)); +router.post('/', requireMember, (req, res) => controller.create(req, res)); +router.delete('/:id', requireMember, (req, res) => controller.delete(req, res)); export default router; diff --git a/backend/src/routes/services.routes.ts b/backend/src/routes/services.routes.ts index 01b6ca36..a4fbc061 100644 --- a/backend/src/routes/services.routes.ts +++ b/backend/src/routes/services.routes.ts @@ -4,6 +4,8 @@ import { ServicesController } from '../controllers/services.controller'; import { validateBody, validateParams } from '../middleware/validation'; import { createServiceSchema, updateServiceSchema, uuidParamSchema } from '../validators/schemas'; import { authenticateToken } from '../middleware/auth.middleware'; +import { requireMember } from '../middleware/rbac.middleware'; +import { discoveryRateLimiter } from '../middleware/rateLimiter'; import { checkDiscoveryLimit, checkResourceLimit } from '../middleware/subscription.middleware'; import { AWSResourceDiscoveryService } from '../services/awsResourceDiscovery'; import { OrganizationAccessError, requireCurrentRole } from '../services/organization-authorization'; @@ -324,7 +326,7 @@ router.get('/stats', authenticateToken, async (req: Request, res: Response): Pro // ─── POST /api/services/discover ───────────────────────────────────────────── -router.post('/discover', authenticateToken, checkDiscoveryLimit, async (req: Request, res: Response): Promise => { +router.post('/discover', authenticateToken, requireMember, checkDiscoveryLimit, discoveryRateLimiter, async (req: Request, res: Response): Promise => { try { const orgId = req.organizationId; if (!orgId) { res.status(401).json({ success: false, error: 'Unauthorized' }); return; } diff --git a/backend/src/routes/teams.routes.ts b/backend/src/routes/teams.routes.ts index 12db0bc6..17388a9b 100644 --- a/backend/src/routes/teams.routes.ts +++ b/backend/src/routes/teams.routes.ts @@ -1,5 +1,6 @@ import { Router } from 'express'; import { authenticateToken } from '../middleware/auth.middleware'; +import { requireMember } from '../middleware/rbac.middleware'; import { TeamsController } from '../controllers/teams.controller'; const router = Router(); @@ -10,7 +11,7 @@ router.use(authenticateToken); router.get('/', (req, res) => controller.getAll(req, res)); router.get('/:id', (req, res) => controller.getById(req, res)); router.get('/:id/services', (req, res) => controller.getTeamServices(req, res)); -router.post('/', (req, res) => controller.create(req, res)); -router.delete('/:id', (req, res) => controller.delete(req, res)); +router.post('/', requireMember, (req, res) => controller.create(req, res)); +router.delete('/:id', requireMember, (req, res) => controller.delete(req, res)); export default router; From 8bdd8bde57ba27df1f38f6818f8303818a3fd18e Mon Sep 17 00:00:00 2001 From: Uwamari Goddey Date: Mon, 5 Oct 2026 14:04:27 -0400 Subject: [PATCH 2/2] fix(auth): enforce tenant ownership references POST /api/services stored whatever team_id it was given, and POST /api/infrastructure whatever service_id: the foreign key only proved the row existed somewhere, so a service could point at another organization's team and a resource at another organization's service. Both inserts now select from the referenced row with the caller's organization in the predicate, in the same statement as the insert (the shape the deployments insert already uses). An id that belongs to another organization is answered exactly like one that exists nowhere: 404, same body, nothing written. The organization still comes only from the authenticated request. An id that exists nowhere used to surface as a foreign-key 500; it is now the same 404. Co-Authored-By: Claude Opus 5.5 --- .../controllers/infrastructure.controller.ts | 11 + .../src/controllers/services.controller.ts | 7 + .../repositories/infrastructure.repository.ts | 16 +- .../src/repositories/services.repository.ts | 42 ++- .../workspace-reference-ownership.test.ts | 312 ++++++++++++++++++ 5 files changed, 376 insertions(+), 12 deletions(-) create mode 100644 backend/src/routes/__tests__/workspace-reference-ownership.test.ts diff --git a/backend/src/controllers/infrastructure.controller.ts b/backend/src/controllers/infrastructure.controller.ts index 4fcf6b13..a855e5f5 100644 --- a/backend/src/controllers/infrastructure.controller.ts +++ b/backend/src/controllers/infrastructure.controller.ts @@ -153,6 +153,17 @@ export class InfrastructureController { const organizationId = (req as any).user?.organizationId; const resource = await repository.create(resourceData, organizationId); + if (!resource) { + // Same answer whether the service doesn't exist or belongs to + // another organization. + const response: ApiResponse = { + success: false, + error: 'Service not found', + }; + res.status(404).json(response); + return; + } + const response: ApiResponse = { success: true, data: resource, diff --git a/backend/src/controllers/services.controller.ts b/backend/src/controllers/services.controller.ts index 3798cf5b..34438171 100644 --- a/backend/src/controllers/services.controller.ts +++ b/backend/src/controllers/services.controller.ts @@ -72,6 +72,13 @@ export class ServicesController { } const service = await repository.create(serviceData, organizationId); + if (!service) { + // Same answer whether the team doesn't exist or belongs to another + // organization. + next(new NotFoundError('Team')); + return; + } + // Emit onboarding event for service creation const user = (req as any).user; if (user && service) { diff --git a/backend/src/repositories/infrastructure.repository.ts b/backend/src/repositories/infrastructure.repository.ts index 318b6f11..7cc9b957 100644 --- a/backend/src/repositories/infrastructure.repository.ts +++ b/backend/src/repositories/infrastructure.repository.ts @@ -63,13 +63,23 @@ export class InfrastructureRepository { return result.rows[0] || null; } - async create(resource: CreateInfrastructureRequest, organizationId: string): Promise { + // Inserts only when the referenced service belongs to the resource's own + // organization -- checked and row-locked in the same statement as the + // insert, so the service can't be deleted or moved in between. Returns null + // when it doesn't (missing, or another organization's); callers must not + // distinguish those cases to the client. + async create(resource: CreateInfrastructureRequest, organizationId: string): Promise { const query = ` INSERT INTO infrastructure_resources ( service_id, resource_type, aws_id, aws_region, status, cost_per_month, metadata, organization_id ) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + SELECT + s.id, $2::varchar, $3::varchar, $4::varchar, + $5::varchar, $6::numeric, $7::jsonb, s.organization_id + FROM services s + WHERE s.id = $1::uuid AND s.organization_id = $8::uuid + FOR SHARE OF s RETURNING * `; const result = await pool.query(query, [ @@ -82,7 +92,7 @@ export class InfrastructureRepository { JSON.stringify(resource.metadata || {}), organizationId, ]); - return result.rows[0]; + return result.rows[0] || null; } async delete(id: string, organizationId: string): Promise { diff --git a/backend/src/repositories/services.repository.ts b/backend/src/repositories/services.repository.ts index 72d8567f..63a8c2ec 100644 --- a/backend/src/repositories/services.repository.ts +++ b/backend/src/repositories/services.repository.ts @@ -58,13 +58,13 @@ export class ServicesRepository { return result.rows[0] || null; } - async create(service: CreateServiceRequest, organizationId: string): Promise { - const query = ` - INSERT INTO services (name, template, owner, team_id, github_url, description, status, organization_id) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8) - RETURNING * - `; - const result = await pool.query(query, [ + // With a team: inserts only when that team belongs to the service's own + // organization -- checked and row-locked in the same statement as the + // insert, so the team can't be deleted in between. Returns null when it + // doesn't (missing, or another organization's); callers must not + // distinguish those cases to the client. + async create(service: CreateServiceRequest, organizationId: string): Promise { + const values = [ service.name, service.template, service.owner, @@ -73,8 +73,32 @@ export class ServicesRepository { service.description, 'active', // default status organizationId, - ]); - return result.rows[0]; + ]; + + if (service.team_id === undefined || service.team_id === null) { + const result = await pool.query( + ` + INSERT INTO services (name, template, owner, team_id, github_url, description, status, organization_id) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + RETURNING * + `, + values + ); + return result.rows[0]; + } + + const query = ` + INSERT INTO services (name, template, owner, team_id, github_url, description, status, organization_id) + SELECT + $1::varchar, $2::varchar, $3::varchar, t.id, + $5::text, $6::text, $7::varchar, t.organization_id + FROM teams t + WHERE t.id = $4::uuid AND t.organization_id = $8::uuid + FOR SHARE OF t + RETURNING * + `; + const result = await pool.query(query, values); + return result.rows[0] || null; } async update(id: string, updates: UpdateServiceRequest, organizationId: string): Promise { diff --git a/backend/src/routes/__tests__/workspace-reference-ownership.test.ts b/backend/src/routes/__tests__/workspace-reference-ownership.test.ts new file mode 100644 index 00000000..a2129cb0 --- /dev/null +++ b/backend/src/routes/__tests__/workspace-reference-ownership.test.ts @@ -0,0 +1,312 @@ +/** + * A new service can only reference a team, and a new infrastructure resource + * a service, of the caller's own organization. + * + * Policy under test: + * - POST /api/services creates the service only when `team_id` names a team + * of the caller's organization. + * - POST /api/infrastructure creates the resource only when `service_id` + * names a service of the caller's organization. + * - An id that belongs to another organization is answered exactly like an + * id that exists nowhere: the same status and the same body, so the + * response never reveals that the id exists elsewhere. + * - A refused request writes nothing, in either organization, and leaves + * the referenced row untouched. + * - The organization is the caller's own, whatever the body says. + * + * Real routes over an in-process HTTP server against live Postgres. The test + * role is not subject to RLS (see the precondition test), so every assertion + * passes only because of the organization predicate in the statement itself. + * Only authService.verifyToken (to choose the caller) is stubbed. + */ +import { randomUUID } from 'crypto'; +import express from 'express'; +import http from 'http'; +import { Pool } from 'pg'; +import servicesRoutes from '../services.routes'; +import infrastructureRoutes from '../infrastructure.routes'; +import { errorHandler } from '../../middleware/error-handler'; +import { authService } from '../../services/auth.service'; +import { pool as appPool } from '../../config/database'; + +// Listeners issue their own fire-and-forget queries; irrelevant here. +jest.mock('../../services/onboardingEvents', () => ({ emitOnboardingEvent: jest.fn() })); + +function dbConfig() { + return { + host: process.env.DB_HOST || 'localhost', + port: parseInt(process.env.DB_PORT || '5432'), + database: process.env.DB_NAME || 'platform_portal', + user: process.env.DB_USER || 'postgres', + password: process.env.DB_PASSWORD || 'postgres', + }; +} + +const pool = new Pool(dbConfig()); +const createdOrgIds: string[] = []; +const createdUserIds: string[] = []; + +function uniqueSuffix(): string { + return `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; +} + +async function member(orgId: string, role: string): Promise { + const user = await pool.query( + `INSERT INTO users (email, password_hash, full_name) VALUES ($1, 'x', 'Reference Ownership User') RETURNING id`, + [`reference-ownership-${role}-${uniqueSuffix()}@example.com`] + ); + createdUserIds.push(user.rows[0].id); + await pool.query( + `INSERT INTO organization_memberships (organization_id, user_id, role, joined_at, is_active) + VALUES ($1, $2, $3, NOW(), true)`, + [orgId, user.rows[0].id, role] + ); + return user.rows[0].id as string; +} + +async function buildOrg() { + const suffix = uniqueSuffix(); + const org = await pool.query( + `INSERT INTO organizations (name, slug, display_name, subscription_tier, subscription_status) + VALUES ($1, $2, $1, 'enterprise', 'active') RETURNING id`, + [`Reference Ownership ${suffix}`, `reference-ownership-${suffix}`] + ); + const orgId = org.rows[0].id as string; + createdOrgIds.push(orgId); + const team = await pool.query( + `INSERT INTO teams (name, owner, organization_id) VALUES ($1, 'owner@example.com', $2) RETURNING id`, + [`reference-ownership-team-${suffix}`, orgId] + ); + const service = await pool.query( + `INSERT INTO services (name, template, owner, status, team_id, organization_id) + VALUES ($1, 'api', 'owner@example.com', 'active', $2, $3) RETURNING id`, + [`reference-ownership-svc-${suffix}`, team.rows[0].id, orgId] + ); + return { + orgId, + teamId: team.rows[0].id as string, + serviceId: service.rows[0].id as string, + admin: await member(orgId, 'admin'), + member: await member(orgId, 'member'), + }; +} + +type Org = Awaited>; + +async function row(table: 'teams' | 'services', id: string) { + const { rows } = await pool.query(`SELECT * FROM ${table} WHERE id = $1`, [id]); + return rows[0] ?? null; +} + +/** Every row of `table` that either organization owns or that points at `referenceId`. */ +async function rowsTouching(table: 'services' | 'infrastructure_resources', column: string, referenceId: string) { + const { rows } = await pool.query( + `SELECT id FROM ${table} WHERE organization_id = ANY($1) OR ${column} = $2 ORDER BY id`, + [createdOrgIds, referenceId] + ); + return rows.map((r) => r.id as string); +} + +let server: http.Server; +let baseUrl: string; +let orgA: Org; +let orgB: Org; + +beforeAll(async () => { + orgA = await buildOrg(); + orgB = await buildOrg(); + + const app = express(); + app.use(express.json()); + app.use('/api/services', servicesRoutes); + app.use('/api/infrastructure', infrastructureRoutes); + app.use(errorHandler); + server = http.createServer(app); + await new Promise((resolve) => server.listen(0, resolve)); + const address = server.address(); + const port = typeof address === 'object' && address ? address.port : 0; + baseUrl = `http://127.0.0.1:${port}/api`; +}); + +beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(() => {}); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + await pool.query('DELETE FROM infrastructure_resources WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM services WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM teams WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM audit_logs WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM analytics_events WHERE organization_id = ANY($1)', [createdOrgIds]); + await pool.query( + 'DELETE FROM organization_memberships WHERE organization_id = ANY($1) OR user_id = ANY($2)', + [createdOrgIds, createdUserIds] + ); + await pool.query('DELETE FROM organizations WHERE id = ANY($1)', [createdOrgIds]); + await pool.query('DELETE FROM users WHERE id = ANY($1)', [createdUserIds]); + await pool.end(); + await appPool.end(); +}); + +function post(org: Org, userId: string, path: string, body: Record) { + jest.spyOn(authService, 'verifyToken').mockReturnValue({ + userId, + email: 'reference-ownership-caller@example.com', + organizationId: org.orgId, + role: 'owner', + type: 'access', + } as unknown as ReturnType); + return fetch(`${baseUrl}${path}`, { + method: 'POST', + headers: { Authorization: 'Bearer test-token', 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); +} + +/** As org A's admin: service creation is owner/admin only. */ +function createService(body: Record) { + return post(orgA, orgA.admin, '/services', { + name: `reference-ownership-new-${uniqueSuffix()}`, + template: 'api', + owner: 'owner@example.com', + ...body, + }); +} + +/** As org A's member. */ +function createInfrastructure(body: Record) { + return post(orgA, orgA.member, '/infrastructure', { + resource_type: 'ec2', + aws_id: `i-${uniqueSuffix()}`, + aws_region: 'us-east-1', + status: 'running', + cost_per_month: 4, + ...body, + }); +} + +describe('precondition: RLS cannot be what makes these tests pass', () => { + it('the connecting role is not subject to row-level security', async () => { + const { rows } = await pool.query( + 'SELECT rolsuper OR rolbypassrls AS bypasses FROM pg_roles WHERE rolname = current_user' + ); + expect(rows[0].bypasses).toBe(true); + }); +}); + +describe('POST /api/services checks that the team is the caller\'s own', () => { + it('creates the service for a team of the caller\'s organization', async () => { + const res = await createService({ team_id: orgA.teamId }); + + expect(res.status).toBe(201); + const body = await res.json(); + expect(body.message).toBe('Service created successfully'); + expect(body.data).toMatchObject({ team_id: orgA.teamId, organization_id: orgA.orgId, status: 'active' }); + expect(await row('services', body.data.id)).toMatchObject({ team_id: orgA.teamId, organization_id: orgA.orgId }); + }); + + it('answers another organization\'s team exactly like a team that exists nowhere, and writes nothing', async () => { + const teamBefore = await row('teams', orgB.teamId); + const servicesBefore = await rowsTouching('services', 'team_id', orgB.teamId); + + const foreign = await createService({ team_id: orgB.teamId }); + const unknown = await createService({ team_id: randomUUID() }); + + expect(foreign.status).toBe(404); + expect(unknown.status).toBe(foreign.status); + const foreignBody = await foreign.json(); + expect(foreignBody).toEqual({ success: false, error: 'Team not found', code: 'NOT_FOUND' }); + expect(await unknown.json()).toEqual(foreignBody); + // No service anywhere, and the other organization's team is as it was. + expect(await rowsTouching('services', 'team_id', orgB.teamId)).toEqual(servicesBefore); + expect(await row('teams', orgB.teamId)).toEqual(teamBefore); + }); + + it('takes the organization from the caller, never from the body', async () => { + const servicesBefore = await rowsTouching('services', 'team_id', orgB.teamId); + + // Naming the other organization does not make its team acceptable... + const foreign = await createService({ team_id: orgB.teamId, organization_id: orgB.orgId }); + // ...and does not move a valid request out of the caller's organization. + const own = await createService({ team_id: orgA.teamId, organization_id: orgB.orgId }); + + expect(foreign.status).toBe(404); + expect(own.status).toBe(201); + const created = (await own.json()).data; + expect(created).toMatchObject({ team_id: orgA.teamId, organization_id: orgA.orgId }); + expect(await rowsTouching('services', 'team_id', orgB.teamId)).toEqual([...servicesBefore, created.id].sort()); + }); + + it('still requires a team', async () => { + const res = await createService({}); + + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ success: false, error: 'Validation failed' }); + }); +}); + +describe('POST /api/infrastructure checks that the service is the caller\'s own', () => { + it('creates the resource for a service of the caller\'s organization', async () => { + const res = await createInfrastructure({ service_id: orgA.serviceId, metadata: { note: 'kept' } }); + + expect(res.status).toBe(201); + const body = await res.json(); + expect(body.message).toBe('Infrastructure resource created successfully'); + expect(body.data).toMatchObject({ + service_id: orgA.serviceId, + organization_id: orgA.orgId, + resource_type: 'ec2', + aws_region: 'us-east-1', + status: 'running', + cost_per_month: '4.00', + metadata: { note: 'kept' }, + }); + }); + + it('answers another organization\'s service exactly like a service that exists nowhere, and writes nothing', async () => { + const serviceBefore = await row('services', orgB.serviceId); + const resourcesBefore = await rowsTouching('infrastructure_resources', 'service_id', orgB.serviceId); + + const foreign = await createInfrastructure({ service_id: orgB.serviceId }); + const unknown = await createInfrastructure({ service_id: randomUUID() }); + + expect(foreign.status).toBe(404); + expect(unknown.status).toBe(foreign.status); + const foreignBody = await foreign.json(); + expect(foreignBody).toEqual({ success: false, error: 'Service not found' }); + expect(await unknown.json()).toEqual(foreignBody); + // No resource anywhere, and the other organization's service is as it was. + expect(await rowsTouching('infrastructure_resources', 'service_id', orgB.serviceId)).toEqual(resourcesBefore); + expect(await row('services', orgB.serviceId)).toEqual(serviceBefore); + }); + + it('takes the organization from the caller, never from the body', async () => { + const resourcesBefore = await rowsTouching('infrastructure_resources', 'service_id', orgB.serviceId); + + const foreign = await createInfrastructure({ service_id: orgB.serviceId, organization_id: orgB.orgId }); + const own = await createInfrastructure({ service_id: orgA.serviceId, organization_id: orgB.orgId }); + + expect(foreign.status).toBe(404); + expect(own.status).toBe(201); + const created = (await own.json()).data; + expect(created).toMatchObject({ service_id: orgA.serviceId, organization_id: orgA.orgId }); + expect(await rowsTouching('infrastructure_resources', 'service_id', orgB.serviceId)).toEqual( + [...resourcesBefore, created.id].sort() + ); + }); + + it('still requires a service', async () => { + const res = await createInfrastructure({}); + + expect(res.status).toBe(400); + expect(await res.json()).toEqual({ + success: false, + error: 'Missing required fields: service_id, resource_type, aws_id, aws_region, status, cost_per_month', + }); + }); +});