From bdd3bf19a3fcc997c56ebccc0f3dbb4cdbfe5029 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 19 Mar 2026 15:49:06 -0400 Subject: [PATCH 1/3] chore: improvements to security [dev] [Marfuen] mariano/chore-improve-api-middleware-hardening --- .../src/attachments/attachments.service.ts | 4 + apps/api/src/auth/api-key.service.ts | 25 +-- apps/api/src/auth/auth-server-origins.spec.ts | 61 +++++++ apps/api/src/auth/auth.server.ts | 2 +- .../src/auth/origin-check.middleware.spec.ts | 151 ++++++++++++++++++ apps/api/src/auth/origin-check.middleware.ts | 65 ++++++++ apps/api/src/auth/permission.guard.spec.ts | 105 +++++++++++- apps/api/src/auth/permission.guard.ts | 14 +- .../src/browserbase/dto/browserbase.dto.ts | 11 +- .../validators/url-safety.validator.spec.ts | 62 +++++++ .../validators/url-safety.validator.ts | 98 ++++++++++++ apps/api/src/main.ts | 13 +- apps/api/src/policies/policies.service.ts | 7 + apps/api/src/tasks/attachments.service.ts | 4 + apps/api/src/tasks/tasks.service.ts | 18 +++ .../src/utils/file-type-validation.spec.ts | 49 ++++++ apps/api/src/utils/file-type-validation.ts | 62 +++++++ apps/app/next.config.ts | 19 +++ apps/app/package.json | 1 + .../components/table/ApiKeysTable.tsx | 39 ++++- .../components/table/CreateApiKeySheet.tsx | 6 +- .../settings/api-keys/lib/scope-presets.ts | 6 + .../markdown-renderer/markdown-renderer.tsx | 3 +- bun.lock | 1 + 24 files changed, 799 insertions(+), 27 deletions(-) create mode 100644 apps/api/src/auth/auth-server-origins.spec.ts create mode 100644 apps/api/src/auth/origin-check.middleware.spec.ts create mode 100644 apps/api/src/auth/origin-check.middleware.ts create mode 100644 apps/api/src/browserbase/validators/url-safety.validator.spec.ts create mode 100644 apps/api/src/browserbase/validators/url-safety.validator.ts create mode 100644 apps/api/src/utils/file-type-validation.spec.ts create mode 100644 apps/api/src/utils/file-type-validation.ts diff --git a/apps/api/src/attachments/attachments.service.ts b/apps/api/src/attachments/attachments.service.ts index f38b51ad00..fa818e2622 100644 --- a/apps/api/src/attachments/attachments.service.ts +++ b/apps/api/src/attachments/attachments.service.ts @@ -16,6 +16,7 @@ import { randomBytes } from 'crypto'; import { AttachmentResponseDto } from '../tasks/dto/task-responses.dto'; import { UploadAttachmentDto } from './upload-attachment.dto'; import { s3Client } from '@/app/s3'; +import { validateFileContent } from '../utils/file-type-validation'; @Injectable() export class AttachmentsService { @@ -123,6 +124,9 @@ export class AttachmentsService { ); } + // Validate file content matches declared MIME type + validateFileContent(fileBuffer, uploadDto.fileType, uploadDto.fileName); + // Generate unique file key const fileId = randomBytes(16).toString('hex'); const sanitizedFileName = this.sanitizeFileName(uploadDto.fileName); diff --git a/apps/api/src/auth/api-key.service.ts b/apps/api/src/auth/api-key.service.ts index 876155364a..01551f33f4 100644 --- a/apps/api/src/auth/api-key.service.ts +++ b/apps/api/src/auth/api-key.service.ts @@ -55,16 +55,19 @@ export class ApiKeyService { expiresAt?: string, scopes?: string[], ) { - // Validate scopes if provided - const validatedScopes = scopes?.length ? scopes : []; - if (validatedScopes.length > 0) { - const availableScopes = this.getAvailableScopes(); - const invalid = validatedScopes.filter((s) => !availableScopes.includes(s)); - if (invalid.length > 0) { - throw new BadRequestException( - `Invalid scopes: ${invalid.join(', ')}`, - ); - } + // New keys must have explicit scopes — no more legacy empty-scope keys + if (!scopes || scopes.length === 0) { + throw new BadRequestException( + 'API keys must have at least one scope. Use the "Full Access" preset to grant all permissions.', + ); + } + // Validate all scopes against the allowlist + const availableScopes = this.getAvailableScopes(); + const invalid = scopes.filter((s) => !availableScopes.includes(s)); + if (invalid.length > 0) { + throw new BadRequestException( + `Invalid scopes: ${invalid.join(', ')}`, + ); } const apiKey = this.generateApiKey(); @@ -103,7 +106,7 @@ export class ApiKeyService { salt, expiresAt: expirationDate, organizationId, - scopes: validatedScopes, + scopes, }, select: { id: true, diff --git a/apps/api/src/auth/auth-server-origins.spec.ts b/apps/api/src/auth/auth-server-origins.spec.ts new file mode 100644 index 0000000000..8c781ff1b6 --- /dev/null +++ b/apps/api/src/auth/auth-server-origins.spec.ts @@ -0,0 +1,61 @@ +/** + * Tests for the getTrustedOrigins logic. + * + * Because auth.server.ts has side effects at module load time (better-auth + * initialization, DB connections, validateSecurityConfig), we test the logic + * in isolation rather than importing the module directly. + */ + +function getTrustedOriginsLogic(authTrustedOrigins: string | undefined): string[] { + if (authTrustedOrigins) { + return authTrustedOrigins.split(',').map((o) => o.trim()); + } + + return [ + 'http://localhost:3000', + 'http://localhost:3002', + 'http://localhost:3333', + 'https://app.trycomp.ai', + 'https://portal.trycomp.ai', + 'https://api.trycomp.ai', + 'https://app.staging.trycomp.ai', + 'https://portal.staging.trycomp.ai', + 'https://api.staging.trycomp.ai', + 'https://dev.trycomp.ai', + ]; +} + +describe('getTrustedOrigins', () => { + it('should return env-configured origins when AUTH_TRUSTED_ORIGINS is set', () => { + const origins = getTrustedOriginsLogic('https://a.com, https://b.com'); + expect(origins).toEqual(['https://a.com', 'https://b.com']); + }); + + it('should return hardcoded origins when AUTH_TRUSTED_ORIGINS is not set', () => { + const origins = getTrustedOriginsLogic(undefined); + expect(origins).toContain('https://app.trycomp.ai'); + }); + + it('should never include wildcard origin', () => { + const origins = getTrustedOriginsLogic(undefined); + expect(origins.every((o: string) => o !== '*' && o !== 'true')).toBe(true); + }); + + it('should trim whitespace from comma-separated origins', () => { + const origins = getTrustedOriginsLogic(' https://a.com , https://b.com '); + expect(origins).toEqual(['https://a.com', 'https://b.com']); + }); + + it('main.ts should use getTrustedOrigins instead of origin: true', () => { + // Validate the CORS config change was made correctly by checking file content + const fs = require('fs'); + const path = require('path'); + const mainTs = fs.readFileSync( + path.join(__dirname, '..', 'main.ts'), + 'utf-8', + ) as string; + expect(mainTs).not.toContain('origin: true'); + expect(mainTs).toContain('origin: getTrustedOrigins()'); + expect(mainTs).toContain("import { getTrustedOrigins } from './auth/auth.server'"); + }); +}); diff --git a/apps/api/src/auth/auth.server.ts b/apps/api/src/auth/auth.server.ts index 55c551d071..c46789ede1 100644 --- a/apps/api/src/auth/auth.server.ts +++ b/apps/api/src/auth/auth.server.ts @@ -36,7 +36,7 @@ function getCookieDomain(): string | undefined { /** * Get trusted origins for CORS/auth */ -function getTrustedOrigins(): string[] { +export function getTrustedOrigins(): string[] { const origins = process.env.AUTH_TRUSTED_ORIGINS; if (origins) { return origins.split(',').map((o) => o.trim()); diff --git a/apps/api/src/auth/origin-check.middleware.spec.ts b/apps/api/src/auth/origin-check.middleware.spec.ts new file mode 100644 index 0000000000..5659f84ef2 --- /dev/null +++ b/apps/api/src/auth/origin-check.middleware.spec.ts @@ -0,0 +1,151 @@ +import { originCheckMiddleware } from './origin-check.middleware'; + +// Mock getTrustedOrigins +jest.mock('./auth.server', () => ({ + getTrustedOrigins: () => [ + 'http://localhost:3000', + 'http://localhost:3002', + 'https://app.trycomp.ai', + 'https://portal.trycomp.ai', + ], +})); + +function createMockReq( + method: string, + path: string, + origin?: string, +): Record { + return { + method, + path, + headers: origin ? { origin } : {}, + }; +} + +function createMockRes(): Record & { statusCode?: number; body?: unknown } { + const res: Record & { statusCode?: number; body?: unknown } = {}; + res.status = jest.fn().mockImplementation((code: number) => { + res.statusCode = code; + return res; + }); + res.json = jest.fn().mockImplementation((body: unknown) => { + res.body = body; + return res; + }); + return res; +} + +describe('originCheckMiddleware', () => { + it('should allow GET requests regardless of origin', () => { + const req = createMockReq('GET', '/v1/controls', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); + + it('should allow HEAD requests regardless of origin', () => { + const req = createMockReq('HEAD', '/v1/health', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); + + it('should allow OPTIONS requests regardless of origin', () => { + const req = createMockReq('OPTIONS', '/v1/controls', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); + + it('should allow POST from trusted origin', () => { + const req = createMockReq('POST', '/v1/organization/api-keys', 'http://localhost:3000'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); + + it('should block POST from untrusted origin', () => { + const req = createMockReq('POST', '/v1/organization/transfer-ownership', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('should block DELETE from untrusted origin', () => { + const req = createMockReq('DELETE', '/v1/organization', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('should block PATCH from untrusted origin', () => { + const req = createMockReq('PATCH', '/v1/members/123/role', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('should allow POST without Origin header (API key / service token)', () => { + const req = createMockReq('POST', '/v1/controls', undefined); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); + + it('should allow POST to /api/auth routes (better-auth exempt)', () => { + const req = createMockReq('POST', '/api/auth/sign-in', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); + + it('should allow POST to health check', () => { + const req = createMockReq('POST', '/v1/health', 'http://evil.com'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); + + it('should allow production origins', () => { + const req = createMockReq('POST', '/v1/organization/api-keys', 'https://app.trycomp.ai'); + const res = createMockRes(); + const next = jest.fn(); + + originCheckMiddleware(req as any, res as any, next); + + expect(next).toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/auth/origin-check.middleware.ts b/apps/api/src/auth/origin-check.middleware.ts new file mode 100644 index 0000000000..ecfa92ab3a --- /dev/null +++ b/apps/api/src/auth/origin-check.middleware.ts @@ -0,0 +1,65 @@ +import type { Request, Response, NextFunction } from 'express'; +import { getTrustedOrigins } from './auth.server'; + +const SAFE_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']); + +/** + * Paths exempt from Origin validation (webhooks, public endpoints). + * These are called by external services that don't send browser Origin headers. + */ +const EXEMPT_PATH_PREFIXES = [ + '/api/auth', // better-auth handles its own CSRF + '/v1/health', // health check + '/api/docs', // swagger +]; + +/** + * Express middleware that validates the Origin header on state-changing requests. + * + * This is defense-in-depth against CSRF attacks that bypass CORS: + * - HTML form submissions (Content-Type: application/x-www-form-urlencoded) + * don't trigger CORS preflight, so CORS alone doesn't block them. + * - This middleware rejects any state-changing request whose Origin header + * doesn't match a trusted origin. + * + * API keys and service tokens (which don't come from browsers) typically + * don't send an Origin header, so requests without an Origin are allowed + * — they'll be authenticated by HybridAuthGuard instead. + */ +export function originCheckMiddleware( + req: Request, + res: Response, + next: NextFunction, +): void { + // Allow safe (read-only) methods + if (SAFE_METHODS.has(req.method)) { + return next(); + } + + // Allow exempt paths (webhooks, auth, etc.) + const isExempt = EXEMPT_PATH_PREFIXES.some((prefix) => + req.path.startsWith(prefix), + ); + if (isExempt) { + return next(); + } + + const origin = req.headers['origin'] as string | undefined; + + // No Origin header = not a browser request (API key, service token, curl, etc.) + // These are authenticated via HybridAuthGuard, not cookies, so no CSRF risk. + if (!origin) { + return next(); + } + + // Validate Origin against trusted origins + const trustedOrigins = getTrustedOrigins(); + if (trustedOrigins.includes(origin)) { + return next(); + } + + res.status(403).json({ + statusCode: 403, + message: 'Forbidden', + }); +} diff --git a/apps/api/src/auth/permission.guard.spec.ts b/apps/api/src/auth/permission.guard.spec.ts index 9926e2eb6f..d96c010f9e 100644 --- a/apps/api/src/auth/permission.guard.spec.ts +++ b/apps/api/src/auth/permission.guard.spec.ts @@ -8,11 +8,17 @@ const mockHasPermission = jest.fn(); jest.mock('./auth.server', () => ({ auth: { api: { - hasPermission: (...args) => mockHasPermission(...args), + hasPermission: (...args: unknown[]) => mockHasPermission(...args), }, }, })); +// Mock @trycompai/auth to avoid ESM issues with better-auth +jest.mock('@trycompai/auth', () => ({ + RESTRICTED_ROLES: ['employee', 'contractor'], + PRIVILEGED_ROLES: ['owner', 'admin', 'auditor'], +})); + describe('PermissionGuard', () => { let guard: PermissionGuard; let reflector: Reflector; @@ -20,18 +26,24 @@ describe('PermissionGuard', () => { const createMockExecutionContext = ( request: Partial<{ isApiKey: boolean; + apiKeyScopes: string[] | undefined; userRoles: string[] | null; headers: Record; organizationId: string; + method: string; + url: string; }>, ): ExecutionContext => { return { switchToHttp: () => ({ getRequest: () => ({ isApiKey: false, + apiKeyScopes: undefined, userRoles: null, headers: {}, organizationId: 'org_123', + method: 'GET', + url: '/v1/test', ...request, }), }), @@ -60,17 +72,104 @@ describe('PermissionGuard', () => { expect(result).toBe(true); }); - it('should allow access for API keys (with warning)', async () => { + it('should allow access for legacy API keys with empty scopes before deprecation date', async () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2026-04-19T23:59:59Z')); + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue([ { resource: 'control', actions: ['delete'] }, ]); - const context = createMockExecutionContext({ isApiKey: true }); + const context = createMockExecutionContext({ + isApiKey: true, + apiKeyScopes: [], + method: 'GET', + url: '/v1/controls', + }); + const result = await guard.canActivate(context); + expect(result).toBe(true); + jest.useRealTimers(); + }); + + it('should deny access for legacy API keys with empty scopes after deprecation date', async () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2026-04-20T00:00:00Z')); + + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue([ + { resource: 'control', actions: ['read'] }, + ]); + + const context = createMockExecutionContext({ + isApiKey: true, + apiKeyScopes: [], + method: 'GET', + url: '/v1/controls', + }); + + await expect(guard.canActivate(context)).rejects.toThrow( + ForbiddenException, + ); + + jest.useRealTimers(); + }); + + it('should deny access for legacy API keys with undefined scopes after deprecation date', async () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2026-05-01T00:00:00Z')); + + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue([ + { resource: 'control', actions: ['read'] }, + ]); + + const context = createMockExecutionContext({ + isApiKey: true, + apiKeyScopes: undefined, + method: 'GET', + url: '/v1/controls', + }); + + await expect(guard.canActivate(context)).rejects.toThrow( + ForbiddenException, + ); + + jest.useRealTimers(); + }); + + it('should allow access for API keys with matching scopes', async () => { + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue([ + { resource: 'control', actions: ['read'] }, + ]); + + const context = createMockExecutionContext({ + isApiKey: true, + apiKeyScopes: ['control:read'], + method: 'GET', + url: '/v1/controls', + }); + + const result = await guard.canActivate(context); expect(result).toBe(true); }); + it('should deny access for API keys with non-matching scopes', async () => { + jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue([ + { resource: 'control', actions: ['read'] }, + ]); + + const context = createMockExecutionContext({ + isApiKey: true, + apiKeyScopes: ['risk:read'], + method: 'GET', + url: '/v1/controls', + }); + + await expect(guard.canActivate(context)).rejects.toThrow( + ForbiddenException, + ); + }); + it('should deny access when no authorization or cookie header present', async () => { jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue([ { resource: 'control', actions: ['delete'] }, diff --git a/apps/api/src/auth/permission.guard.ts b/apps/api/src/auth/permission.guard.ts index bde84f7a49..0f19f79498 100644 --- a/apps/api/src/auth/permission.guard.ts +++ b/apps/api/src/auth/permission.guard.ts @@ -62,8 +62,20 @@ export class PermissionGuard implements CanActivate { if (request.isApiKey) { const scopes = request.apiKeyScopes; - // Legacy keys (empty scopes) = full access for backward compatibility + // Legacy keys (empty scopes): full access until April 20, 2026, then blocked if (!scopes || scopes.length === 0) { + const deprecationDate = new Date('2026-04-20T00:00:00Z'); + if (new Date() >= deprecationDate) { + this.logger.warn( + `[PermissionGuard] Legacy API key with empty scopes BLOCKED after deprecation date on ${request.method} ${request.url}.`, + ); + throw new ForbiddenException( + 'This API key is no longer supported. Please regenerate your API key with explicit scopes.', + ); + } + this.logger.warn( + `[PermissionGuard] Legacy API key with empty scopes used on ${request.method} ${request.url}. This key will stop working after April 20, 2026.`, + ); return true; } diff --git a/apps/api/src/browserbase/dto/browserbase.dto.ts b/apps/api/src/browserbase/dto/browserbase.dto.ts index 8a50c5f491..842d96d0f2 100644 --- a/apps/api/src/browserbase/dto/browserbase.dto.ts +++ b/apps/api/src/browserbase/dto/browserbase.dto.ts @@ -1,5 +1,6 @@ import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; -import { IsNotEmpty, IsOptional, IsString, IsBoolean } from 'class-validator'; +import { IsNotEmpty, IsOptional, IsString, IsBoolean, IsUrl } from 'class-validator'; +import { IsSafeUrl } from '../validators/url-safety.validator'; // ===== Session DTOs ===== @@ -17,6 +18,8 @@ export class NavigateToUrlDto { sessionId: string; @ApiProperty({ description: 'URL to navigate to' }) + @IsUrl({}, { message: 'url must be a valid URL' }) + @IsSafeUrl({ message: 'The provided URL is not allowed.' }) @IsString() @IsNotEmpty() url: string; @@ -29,6 +32,8 @@ export class CheckAuthDto { sessionId: string; @ApiProperty({ description: 'URL to check auth status on' }) + @IsUrl({}, { message: 'url must be a valid URL' }) + @IsSafeUrl({ message: 'The provided URL is not allowed.' }) @IsString() @IsNotEmpty() url: string; @@ -60,6 +65,8 @@ export class CreateBrowserAutomationDto { description?: string; @ApiProperty({ description: 'Target URL to start from' }) + @IsUrl({}, { message: 'url must be a valid URL' }) + @IsSafeUrl({ message: 'The provided URL is not allowed.' }) @IsString() @IsNotEmpty() targetUrl: string; @@ -87,6 +94,8 @@ export class UpdateBrowserAutomationDto { description?: string; @ApiPropertyOptional({ description: 'Target URL to start from' }) + @IsUrl({}, { message: 'url must be a valid URL' }) + @IsSafeUrl({ message: 'The provided URL is not allowed.' }) @IsString() @IsOptional() targetUrl?: string; diff --git a/apps/api/src/browserbase/validators/url-safety.validator.spec.ts b/apps/api/src/browserbase/validators/url-safety.validator.spec.ts new file mode 100644 index 0000000000..29fbe3dd0d --- /dev/null +++ b/apps/api/src/browserbase/validators/url-safety.validator.spec.ts @@ -0,0 +1,62 @@ +import { isSafeUrl } from './url-safety.validator'; + +describe('isSafeUrl', () => { + it('should allow normal HTTPS URLs', () => { + expect(isSafeUrl('https://example.com')).toBe(true); + expect(isSafeUrl('https://app.trycomp.ai/dashboard')).toBe(true); + }); + + it('should allow HTTP URLs', () => { + expect(isSafeUrl('http://example.com')).toBe(true); + }); + + it('should block AWS metadata endpoint', () => { + expect(isSafeUrl('http://169.254.169.254/latest/meta-data/')).toBe(false); + }); + + it('should block link-local range', () => { + expect(isSafeUrl('http://169.254.0.1/')).toBe(false); + }); + + it('should block private IP ranges', () => { + expect(isSafeUrl('http://10.0.0.1/')).toBe(false); + expect(isSafeUrl('http://172.16.0.1/')).toBe(false); + expect(isSafeUrl('http://192.168.1.1/')).toBe(false); + }); + + it('should block localhost', () => { + expect(isSafeUrl('http://localhost/')).toBe(false); + expect(isSafeUrl('http://127.0.0.1/')).toBe(false); + expect(isSafeUrl('http://[::1]/')).toBe(false); + }); + + it('should block non-http protocols', () => { + expect(isSafeUrl('file:///etc/passwd')).toBe(false); + expect(isSafeUrl('ftp://internal/')).toBe(false); + }); + + it('should reject invalid URLs', () => { + expect(isSafeUrl('not-a-url')).toBe(false); + expect(isSafeUrl('')).toBe(false); + }); + + it('should block IPv4-mapped IPv6 addresses', () => { + expect(isSafeUrl('http://[::ffff:169.254.169.254]/')).toBe(false); + expect(isSafeUrl('http://[::ffff:10.0.0.1]/')).toBe(false); + expect(isSafeUrl('http://[::ffff:127.0.0.1]/')).toBe(false); + expect(isSafeUrl('http://[::ffff:192.168.1.1]/')).toBe(false); + }); + + it('should block IPv4-mapped IPv6 in hex form', () => { + // ::ffff:a9fe:a9fe = 169.254.169.254 + expect(isSafeUrl('http://[::ffff:a9fe:a9fe]/')).toBe(false); + // ::ffff:a00:1 = 10.0.0.1 + expect(isSafeUrl('http://[::ffff:a00:1]/')).toBe(false); + // ::ffff:7f00:1 = 127.0.0.1 + expect(isSafeUrl('http://[::ffff:7f00:1]/')).toBe(false); + }); + + it('should allow valid IPv6 public addresses', () => { + expect(isSafeUrl('http://[2607:f8b0:4004:800::200e]/')).toBe(true); + }); +}); diff --git a/apps/api/src/browserbase/validators/url-safety.validator.ts b/apps/api/src/browserbase/validators/url-safety.validator.ts new file mode 100644 index 0000000000..9d191e6bfd --- /dev/null +++ b/apps/api/src/browserbase/validators/url-safety.validator.ts @@ -0,0 +1,98 @@ +import { + registerDecorator, + ValidationOptions, + ValidatorConstraint, + ValidatorConstraintInterface, +} from 'class-validator'; + +const BLOCKED_HOSTNAMES = ['localhost', '127.0.0.1', '[::1]', '0.0.0.0']; + +/** + * Check if an IPv4 address falls within private/reserved ranges. + */ +function isPrivateIpv4(hostname: string): boolean { + const parts = hostname.split('.').map(Number); + if (parts.length !== 4 || parts.some((p) => isNaN(p))) return false; + if (parts[0] === 10) return true; + if (parts[0] === 172 && parts[1] >= 16 && parts[1] <= 31) return true; + if (parts[0] === 192 && parts[1] === 168) return true; + if (parts[0] === 127) return true; + if (parts[0] === 169 && parts[1] === 254) return true; + if (parts.every((p) => p === 0)) return true; + return false; +} + +/** + * Check if a hostname is an IPv4-mapped IPv6 address pointing to a private IP. + * Handles formats like ::ffff:10.0.0.1 and the hex form ::ffff:a0a:1. + */ +function isPrivateIpv6(hostname: string): boolean { + // URL.hostname keeps brackets for IPv6: [::ffff:a9fe:a9fe] + const stripped = hostname.replace(/^\[|\]$/g, '').toLowerCase(); + + // IPv4-mapped IPv6 with dotted notation: ::ffff:169.254.169.254 + const v4MappedMatch = stripped.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/); + if (v4MappedMatch) { + return isPrivateIpv4(v4MappedMatch[1]); + } + + // IPv4-mapped IPv6 in hex form: ::ffff:a9fe:a9fe (169.254.169.254) + const hexMappedMatch = stripped.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); + if (hexMappedMatch) { + const hi = parseInt(hexMappedMatch[1], 16); + const lo = parseInt(hexMappedMatch[2], 16); + const ip = `${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`; + return isPrivateIpv4(ip); + } + + // Loopback [::1] is already in BLOCKED_HOSTNAMES + // Block all-zeros (::) + if (stripped === '::' || stripped === '0:0:0:0:0:0:0:0') return true; + + return false; +} + +export function isSafeUrl(value: string): boolean { + if (!value || typeof value !== 'string') return false; + + let parsed: URL; + try { + parsed = new URL(value); + } catch { + return false; + } + + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + return false; + } + + const hostname = parsed.hostname.toLowerCase(); + if (BLOCKED_HOSTNAMES.includes(hostname)) return false; + if (isPrivateIpv4(hostname)) return false; + if (isPrivateIpv6(hostname)) return false; + + return true; +} + +@ValidatorConstraint({ name: 'isSafeUrl', async: false }) +export class IsSafeUrlConstraint implements ValidatorConstraintInterface { + validate(value: string): boolean { + return isSafeUrl(value); + } + + defaultMessage(): string { + return 'The provided URL is not allowed.'; + } +} + +export function IsSafeUrl(validationOptions?: ValidationOptions) { + return function (object: object, propertyName: string) { + registerDecorator({ + target: object.constructor, + propertyName, + options: validationOptions, + constraints: [], + validator: IsSafeUrlConstraint, + }); + }; +} diff --git a/apps/api/src/main.ts b/apps/api/src/main.ts index 0f19b1dbba..89b3ea5e34 100644 --- a/apps/api/src/main.ts +++ b/apps/api/src/main.ts @@ -8,7 +8,9 @@ import * as express from 'express'; import helmet from 'helmet'; import path from 'path'; import { AppModule } from './app.module'; +import { getTrustedOrigins } from './auth/auth.server'; import { adminAuthRateLimiter } from './auth/admin-rate-limit.middleware'; +import { originCheckMiddleware } from './auth/origin-check.middleware'; import { mkdirSync, writeFileSync, existsSync } from 'fs'; let app: INestApplication | null = null; @@ -20,9 +22,9 @@ async function bootstrap(): Promise { bodyParser: false, }); - // Enable CORS for all origins - security is handled by authentication + // Enable CORS with explicit origin allowlist app.enableCors({ - origin: true, + origin: getTrustedOrigins(), credentials: true, exposedHeaders: ['Content-Disposition'], }); @@ -43,7 +45,12 @@ async function bootstrap(): Promise { }), ); - // STEP 3: Rate-limit better-auth admin routes (impersonation, ban, set-role, etc.) + // STEP 3a: Origin header validation for CSRF protection + // Rejects state-changing requests from untrusted origins. + // Defense-in-depth: CORS blocks fetch-based CSRF, this blocks form-based CSRF. + app.use(originCheckMiddleware); + + // STEP 3b: Rate-limit better-auth admin routes (impersonation, ban, set-role, etc.) // These bypass NestJS controllers so the global ThrottlerGuard doesn't apply. app.use(adminAuthRateLimiter); diff --git a/apps/api/src/policies/policies.service.ts b/apps/api/src/policies/policies.service.ts index 69ef173d26..47263ed59c 100644 --- a/apps/api/src/policies/policies.service.ts +++ b/apps/api/src/policies/policies.service.ts @@ -817,6 +817,13 @@ export class PoliciesService { throw new NotFoundException(`Policy with ID ${policyId} not found`); } + // Prevent direct publishing when approval workflow is active + if (policy.pendingVersionId && policy.approverId) { + throw new BadRequestException( + 'Cannot publish directly while an approval is pending. Either accept or reject the pending changes first.', + ); + } + const contentToPublish = ( policy.draftContent && policy.draftContent.length > 0 ? policy.draftContent diff --git a/apps/api/src/tasks/attachments.service.ts b/apps/api/src/tasks/attachments.service.ts index c33d884ba8..b6933d6aa2 100644 --- a/apps/api/src/tasks/attachments.service.ts +++ b/apps/api/src/tasks/attachments.service.ts @@ -16,6 +16,7 @@ import { randomBytes } from 'crypto'; import { AttachmentResponseDto } from './dto/task-responses.dto'; import { UploadAttachmentDto } from './dto/upload-attachment.dto'; import { s3Client } from '@/app/s3'; +import { validateFileContent } from '../utils/file-type-validation'; @Injectable() export class AttachmentsService { @@ -123,6 +124,9 @@ export class AttachmentsService { ); } + // Validate file content matches declared MIME type + validateFileContent(fileBuffer, uploadDto.fileType, uploadDto.fileName); + // Generate unique file key const fileId = randomBytes(16).toString('hex'); const sanitizedFileName = this.sanitizeFileName(uploadDto.fileName); diff --git a/apps/api/src/tasks/tasks.service.ts b/apps/api/src/tasks/tasks.service.ts index 9dcb28f517..e864a4fe7c 100644 --- a/apps/api/src/tasks/tasks.service.ts +++ b/apps/api/src/tasks/tasks.service.ts @@ -474,6 +474,7 @@ export class TasksService { title: true, status: true, assigneeId: true, + approverId: true, }, }); @@ -500,6 +501,23 @@ export class TasksService { dataToUpdate.description = updateData.description; } if (updateData.status !== undefined) { + // Prevent bypassing the approval workflow via direct status change + if (existingTask.status === 'in_review' && updateData.status !== 'in_review') { + throw new BadRequestException( + 'Cannot change status directly while task is in review. Use the approve or reject actions instead.', + ); + } + // Prevent directly setting status to 'done' when an approver is assigned + // (must go through submitForReview → approveTask workflow) + if ( + updateData.status === 'done' && + existingTask.status !== 'done' && + existingTask.approverId + ) { + throw new BadRequestException( + 'Cannot mark task as done directly when an approver is assigned. Submit for review instead.', + ); + } dataToUpdate.status = updateData.status; } if (updateData.assigneeId !== undefined) { diff --git a/apps/api/src/utils/file-type-validation.spec.ts b/apps/api/src/utils/file-type-validation.spec.ts new file mode 100644 index 0000000000..8c5be9daef --- /dev/null +++ b/apps/api/src/utils/file-type-validation.spec.ts @@ -0,0 +1,49 @@ +import { BadRequestException } from '@nestjs/common'; +import { validateFileContent } from './file-type-validation'; + +describe('validateFileContent', () => { + it('should accept a valid PNG file', () => { + const pngBuffer = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); + expect(() => validateFileContent(pngBuffer, 'image/png', 'test.png')).not.toThrow(); + }); + + it('should accept a valid PDF file', () => { + const pdfBuffer = Buffer.from('%PDF-1.4 some content'); + expect(() => validateFileContent(pdfBuffer, 'application/pdf', 'test.pdf')).not.toThrow(); + }); + + it('should accept a valid JPEG file', () => { + const jpegBuffer = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10]); + expect(() => validateFileContent(jpegBuffer, 'image/jpeg', 'test.jpg')).not.toThrow(); + }); + + it('should reject HTML content disguised as PNG', () => { + const htmlBuffer = Buffer.from(''); + expect(() => validateFileContent(htmlBuffer, 'image/png', 'test.png')).toThrow(BadRequestException); + }); + + it('should reject PNG with wrong magic bytes', () => { + const fakeBuffer = Buffer.from([0x00, 0x00, 0x00, 0x00]); + expect(() => validateFileContent(fakeBuffer, 'image/png', 'test.png')).toThrow(BadRequestException); + }); + + it('should reject files containing script tags regardless of type', () => { + const malicious = Buffer.from(''); + expect(() => validateFileContent(malicious, 'text/plain', 'readme.txt')).toThrow(BadRequestException); + }); + + it('should reject files with event handlers', () => { + const malicious = Buffer.from(''); + expect(() => validateFileContent(malicious, 'text/plain', 'readme.txt')).toThrow(BadRequestException); + }); + + it('should allow text files that are actually text', () => { + const textBuffer = Buffer.from('Hello, this is a normal text file.'); + expect(() => validateFileContent(textBuffer, 'text/plain', 'readme.txt')).not.toThrow(); + }); + + it('should allow unknown MIME types without magic byte check', () => { + const csvBuffer = Buffer.from('name,email\njohn,john@example.com'); + expect(() => validateFileContent(csvBuffer, 'text/csv', 'data.csv')).not.toThrow(); + }); +}); diff --git a/apps/api/src/utils/file-type-validation.ts b/apps/api/src/utils/file-type-validation.ts new file mode 100644 index 0000000000..65a9dafc1b --- /dev/null +++ b/apps/api/src/utils/file-type-validation.ts @@ -0,0 +1,62 @@ +import { BadRequestException } from '@nestjs/common'; + +const MAGIC_BYTES: Record = { + 'image/png': [Buffer.from([0x89, 0x50, 0x4e, 0x47])], + 'image/jpeg': [Buffer.from([0xff, 0xd8, 0xff])], + 'image/gif': [Buffer.from('GIF87a'), Buffer.from('GIF89a')], + 'image/webp': [Buffer.from('RIFF')], + 'application/pdf': [Buffer.from('%PDF')], + 'application/zip': [Buffer.from([0x50, 0x4b, 0x03, 0x04])], +}; + +/** MIME types that are verified binary — skip text pattern scanning for these. */ +const BINARY_MIME_TYPES = new Set(Object.keys(MAGIC_BYTES)); + +/** + * Patterns that indicate potentially dangerous HTML/script content. + * Only applied to text-based files (not binary files that passed magic byte check). + * Uses specific event handler names to avoid false positives on words like "online". + */ +const DANGEROUS_CONTENT_PATTERNS = [ + /]/i, + /]/i, + /]/i, + /]/i, + /javascript:/i, + /vbscript:/i, + /\bon(?:click|load|error|mouseover|focus|blur|submit|change|input|keydown|keyup|mousedown|mouseup|dblclick|contextmenu|drag|drop|touchstart|touchend|pointerdown|pointerup|animationend|abort|beforeunload|unload)\s*=/i, +]; + +export function validateFileContent( + fileBuffer: Buffer, + declaredMimeType: string, + fileName: string, +): void { + const lowerMime = declaredMimeType.toLowerCase(); + + // Check magic bytes for known binary types + const expectedSignatures = MAGIC_BYTES[lowerMime]; + if (expectedSignatures) { + const matchesSignature = expectedSignatures.some((sig) => + fileBuffer.subarray(0, sig.length).equals(sig), + ); + if (!matchesSignature) { + throw new BadRequestException( + 'The uploaded file is invalid or corrupted. Please try again with a valid file.', + ); + } + // Binary file passed magic byte check — skip text pattern scanning + // to avoid false positives from binary data matching text patterns + return; + } + + // For non-binary files: scan first 8KB for dangerous HTML/script content + const headStr = fileBuffer.subarray(0, 8192).toString('utf-8'); + for (const pattern of DANGEROUS_CONTENT_PATTERNS) { + if (pattern.test(headStr)) { + throw new BadRequestException( + 'The uploaded file is invalid or corrupted. Please try again with a valid file.', + ); + } + } +} diff --git a/apps/app/next.config.ts b/apps/app/next.config.ts index 223327801e..df53597e00 100644 --- a/apps/app/next.config.ts +++ b/apps/app/next.config.ts @@ -92,6 +92,25 @@ const config: NextConfig = { } : {}), + // Security headers + async headers() { + return [ + { + source: '/:path*', + headers: [ + { key: 'X-Frame-Options', value: 'DENY' }, + { key: 'X-Content-Type-Options', value: 'nosniff' }, + { key: 'Referrer-Policy', value: 'strict-origin-when-cross-origin' }, + { key: 'X-DNS-Prefetch-Control', value: 'on' }, + { + key: 'Content-Security-Policy', + value: "frame-ancestors 'none'", + }, + ], + }, + ]; + }, + // PostHog proxy for better tracking async rewrites() { return [ diff --git a/apps/app/package.json b/apps/app/package.json index 4a8e8e4b19..ab6a39c525 100644 --- a/apps/app/package.json +++ b/apps/app/package.json @@ -123,6 +123,7 @@ "react-use-draggable-scroll": "^0.4.7", "react-wrap-balancer": "^1.1.1", "rehype-raw": "^7.0.0", + "rehype-sanitize": "^6.0.0", "remark-gfm": "^4.0.1", "remark-parse": "^11.0.0", "resend": "^4.4.1", diff --git a/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/ApiKeysTable.tsx b/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/ApiKeysTable.tsx index 894df7536d..71cdd608b3 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/ApiKeysTable.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/ApiKeysTable.tsx @@ -22,6 +22,11 @@ import { InputGroup, InputGroupAddon, InputGroupInput, + Popover, + PopoverContent, + PopoverHeader, + PopoverTitle, + PopoverTrigger, Stack, Table, TableBody, @@ -32,6 +37,7 @@ import { Text, } from '@trycompai/design-system'; import { Add, OverflowMenuVertical, Search, TrashCan } from '@trycompai/design-system/icons'; +import { groupScopesByResource, RESOURCE_LABELS, ACTION_LABELS } from '../../lib/scope-presets'; import { useMemo, useState } from 'react'; import { toast } from 'sonner'; import { CreateApiKeySheet } from './CreateApiKeySheet'; @@ -54,10 +60,37 @@ function ScopeBadge({ apiKey }: { apiKey: ApiKey }) { return Full Access (Legacy); } + const groups = groupScopesByResource(apiKey.scopes); + return ( - - {apiKey.scopes.length} {apiKey.scopes.length === 1 ? 'scope' : 'scopes'} - + + + + {apiKey.scopes.length} {apiKey.scopes.length === 1 ? 'scope' : 'scopes'} + + + + + Permissions + +
+ {groups.map((group) => ( +
+ + {group.label} + +
+ {group.scopes.map((s) => ( + + {s.label} + + ))} +
+
+ ))} +
+ + ); } diff --git a/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/CreateApiKeySheet.tsx b/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/CreateApiKeySheet.tsx index bbc0901e32..15859f7574 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/CreateApiKeySheet.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/api-keys/components/table/CreateApiKeySheet.tsx @@ -58,8 +58,8 @@ export function CreateApiKeySheet({ open, onOpenChange }: CreateApiKeySheetProps const handleSubmit = async () => { setIsCreating(true); try { - // Full access preset sends empty scopes (legacy behavior) - const scopes = preset === 'full' ? [] : selectedScopes; + // Full access sends all scopes explicitly (no more empty-scope legacy keys) + const scopes = selectedScopes; const result = await createApiKey({ name, expiresAt: expiration, scopes }); if (result.key) { setCreatedApiKey(result.key); @@ -142,7 +142,7 @@ export function CreateApiKeySheet({ open, onOpenChange }: CreateApiKeySheetProps