🛡️ Sentinel: [HIGH] Add security headers & fix CORS

.jules/sentinel.md

@@ -0,0 +1,4 @@
+## 2025-12-22 - Manual CORS Implementation Pitfalls
+**Vulnerability:** Broken CORS configuration for allowed origins on non-preflight requests.
+**Learning:** Manual CORS implementation in Express middleware can be brittle if ordering is not carefully managed. Specifically, an early return for `localhost` checks bypassed the logic that sets `Access-Control-Allow-Origin` headers for other allowed origins.
+**Prevention:** When implementing manual CORS logic, ensure the header-setting logic runs for *all* valid requests before any early exit conditions, or use a standard middleware like `cors` which handles these edge cases robustly.

src/http-transport.ts

@@ -30,6 +30,7 @@ import { ExternalOAuthProvider } from './oauth-provider.js';
 import type { AuthInterceptor } from './types/profile.js';
 import { HTTP_STATUS, MIME_TYPES, OAUTH_PATHS, TIMEOUTS, OAUTH_RATE_LIMIT } from './constants.js';
 import { escapeHtmlSafe } from './validation-utils.js';
+import { addSecurityHeaders } from './security-headers.js';
 import type { OAuthTokens } from '@modelcontextprotocol/sdk/shared/auth.js';
 import {
   AuthenticationError,
@@ -106,6 +107,9 @@ export class HttpTransport {
       next();
     });
 
+    // Security: Add standard security headers (HSTS, CSP, etc.)
+    this.app.use(addSecurityHeaders);
+
     // DNS rebinding protection when binding to localhost
     // Deny requests with mismatched Host headers to prevent DNS rebinding attacks
     // Applies when server host is localhost/127.0.0.1, regardless of auth configuration
@@ -191,6 +195,13 @@ export class HttpTransport {
         this.hasWarnedAboutBinding = true;
       }
 
+      // Security: Enable CORS for allowed origins on actual requests (not just preflight)
+      if (origin && this.isAllowedOrigin(origin)) {
+        res.setHeader('Access-Control-Allow-Origin', origin);
+        res.setHeader('Access-Control-Allow-Credentials', 'false');
+        res.setHeader('Vary', 'Origin');
+      }
+
       // Skip Origin check for localhost
       if (req.hostname === 'localhost' || req.hostname === '127.0.0.1') {
         return next();

src/security-headers.test.ts

@@ -0,0 +1,67 @@
+import { describe, it, expect, vi } from 'vitest';
+import { addSecurityHeaders } from './security-headers.js';
+import { Request, Response } from 'express';
+
+function createMockResponse() {
+  const res: any = {
+    headers: {} as Record<string, string>,
+    setHeader: vi.fn((key: string, value: string) => {
+      res.headers[key] = value;
+    }),
+  };
+  return res as Response;
+}
+
+function createMockRequest(headers: Record<string, string> = {}, secure = false) {
+  return {
+    headers,
+    secure,
+  } as unknown as Request;
+}
+
+describe('Security Headers Middleware', () => {
+  it('sets standard security headers', () => {
+    const req = createMockRequest();
+    const res = createMockResponse();
+    const next = vi.fn();
+
+    addSecurityHeaders(req, res, next);
+
+    expect(next).toHaveBeenCalled();
+    expect(res.headers['X-Content-Type-Options']).toBe('nosniff');
+    expect(res.headers['X-Frame-Options']).toBe('DENY');
+    expect(res.headers['Content-Security-Policy']).toContain("default-src 'none'");
+    expect(res.headers['Permissions-Policy']).toBeDefined();
+    expect(res.headers['Referrer-Policy']).toBe('no-referrer');
+  });
+
+  it('sets HSTS when request is secure', () => {
+    const req = createMockRequest({}, true); // secure=true
+    const res = createMockResponse();
+    const next = vi.fn();
+
+    addSecurityHeaders(req, res, next);
+
+    expect(res.headers['Strict-Transport-Security']).toBe('max-age=31536000; includeSubDomains');
+  });
+
+  it('sets HSTS when X-Forwarded-Proto is https', () => {
+    const req = createMockRequest({ 'x-forwarded-proto': 'https' }, false);
+    const res = createMockResponse();
+    const next = vi.fn();
+
+    addSecurityHeaders(req, res, next);
+
+    expect(res.headers['Strict-Transport-Security']).toBe('max-age=31536000; includeSubDomains');
+  });
+
+  it('does NOT set HSTS when insecure', () => {
+    const req = createMockRequest({}, false);
+    const res = createMockResponse();
+    const next = vi.fn();
+
+    addSecurityHeaders(req, res, next);
+
+    expect(res.headers['Strict-Transport-Security']).toBeUndefined();
+  });
+});

src/security-headers.ts

@@ -0,0 +1,40 @@
+import { Request, Response, NextFunction } from 'express';
+
+/**
+ * Middleware to add security headers to all responses
+ *
+ * Why: Provides defense-in-depth against common web attacks
+ * - X-Content-Type-Options: Prevents MIME-sniffing
+ * - X-Frame-Options: Prevents clickjacking
+ * - Strict-Transport-Security: Enforces HTTPS
+ * - Content-Security-Policy: Restricts loaded resources
+ */
+export const addSecurityHeaders = (req: Request, res: Response, next: NextFunction) => {
+  // Prevent sniffing of content type
+  res.setHeader('X-Content-Type-Options', 'nosniff');
+
+  // Prevent clickjacking (not strictly needed for API, but good practice)
+  res.setHeader('X-Frame-Options', 'DENY');
+
+  // Strict Transport Security (HSTS) - 1 year
+  // Only applicable if HTTPS, but good practice to set generally for security scanning
+  // We check secure flag or forwarded proto to determine if HSTS should be sent
+  if (req.secure || req.headers['x-forwarded-proto'] === 'https') {
+    res.setHeader('Strict-Transport-Security', 'max-age=31536000; includeSubDomains');
+  }
+
+  // Content Security Policy
+  // Default to none for maximum security, as this is an API server
+  // We don't serve HTML/scripts, so we can be very strict
+  res.setHeader('Content-Security-Policy', "default-src 'none'; frame-ancestors 'none'");
+
+  // Permisison Policy
+  // Disable powerful features not needed by an API
+  res.setHeader('Permissions-Policy', 'accelerometer=(), camera=(), geolocation=(), gyroscope=(), magnetometer=(), microphone=(), payment=(), usb=()');
+
+  // Referrer Policy
+  // Don't leak referrer information
+  res.setHeader('Referrer-Policy', 'no-referrer');
+
+  next();
+};

src/testing/http-integration.test.ts

@@ -428,4 +428,39 @@ describeIfListen('HTTP Transport Integration', () => {
       expect(typeof response.body.sessions).toBe('number');
     });
   });
+
+  describe('Security Headers', () => {
+    it('should include security headers in responses', async () => {
+      const response = await request(app)
+        .post('/mcp')
+        .set('Accept', 'application/json')
+        .send({
+          jsonrpc: '2.0',
+          id: 1,
+          method: 'initialize',
+          params: {},
+        });
+
+      expect(response.headers['x-content-type-options']).toBe('nosniff');
+      expect(response.headers['x-frame-options']).toBe('DENY');
+      expect(response.headers['content-security-policy']).toContain("default-src 'none'");
+    });
+
+    it('should include CORS headers for allowed origin', async () => {
+      const response = await request(app)
+        .post('/mcp')
+        .set('Origin', 'http://localhost:3000') // Localhost is allowed by default
+        .set('Accept', 'application/json')
+        .send({
+          jsonrpc: '2.0',
+          id: 1,
+          method: 'initialize',
+          params: {},
+        });
+
+      expect(response.headers['access-control-allow-origin']).toBe('http://localhost:3000');
+      expect(response.headers['access-control-allow-credentials']).toBe('false');
+      expect(response.headers['vary']).toBe('Origin');
+    });
+  });
 });