chore: improvements to security

[dev] [Marfuen] mariano/chore-improve-api-middleware-hardening

Author: github-actions[bot]

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);

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,

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'");
+  });
+});

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());

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<string, unknown> {
+  return {
+    method,
+    path,
+    headers: origin ? { origin } : {},
+  };
+}
+
+function createMockRes(): Record<string, unknown> & { statusCode?: number; body?: unknown } {
+  const res: Record<string, unknown> & { 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();
+  });
+});

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',
+  });
+}