fix(auth): fail fast on missing CAPTCHA secret and harden rate-limit IP trust

.env.example

@@ -64,6 +64,14 @@ GEOCODING_TIMEOUT_MS=5000
 # CAPTCHA Configuration
 # Set to 'true' only in development environments to bypass CAPTCHA verification
 CAPTCHA_BYPASS=false
+# REQUIRED when CAPTCHA_BYPASS != true (default). Boot fails fast if absent.
+# RECAPTCHA_SECRET=
+
+# Reverse proxy / load balancer (Issue #1195)
+# Set to the number of trusted proxy hops (e.g. '1' for nginx in front of the
+# app) when running behind a proxy, so x-forwarded-for is trusted for rate
+# limiting. Leave unset when exposed directly to the internet.
+# TRUST_PROXY=1
 
 # Redis (used by Docker Compose and cache module)
 REDIS_HOST=localhost

src/auth/auth.service.captcha.spec.ts

@@ -1,4 +1,4 @@
-import { UnauthorizedException } from '@nestjs/common';
+import { ServiceUnavailableException, UnauthorizedException } from '@nestjs/common';
 import { Test, TestingModule } from '@nestjs/testing';
 import { ConfigService } from '@nestjs/config';
 import { AuthService } from './auth.service';
@@ -56,7 +56,9 @@ describe('AuthService – CAPTCHA failure lockout', () => {
         JWT_REFRESH_EXPIRES_IN: '7d',
         BCRYPT_ROUNDS: '10',
         CAPTCHA_THRESHOLD: '3',
-        // No RECAPTCHA_SECRET → verifyCaptcha bypassed (returns true) unless we mock fetch
+        // No RECAPTCHA_SECRET and no CAPTCHA_BYPASS in the default harness →
+        // verifyCaptcha throws ServiceUnavailableException (#1194). Tests that
+        // need it to return false set a secret and mock fetch below.
       };
       return config[key];
     }),
@@ -103,6 +105,19 @@ describe('AuthService – CAPTCHA failure lockout', () => {
     }) as unknown as typeof fetch;
   }
 
+  describe('missing RECAPTCHA_SECRET (#1194)', () => {
+    it('throws a stable ServiceUnavailableException when the secret is absent and CAPTCHA is not bypassed', async () => {
+      // Default harness config has no RECAPTCHA_SECRET and no CAPTCHA_BYPASS,
+      // so CAPTCHA is required.
+      await expect(service['verifyCaptcha']('token')).rejects.toThrow(
+        ServiceUnavailableException,
+      );
+      await expect(service['verifyCaptcha']('token')).rejects.toMatchObject({
+        response: { error: 'CAPTCHA_SERVICE_UNAVAILABLE' },
+      });
+    });
+  });
+
   describe('CAPTCHA failure', () => {
     beforeEach(() => {
       rateLimitService.isAccountLocked.mockResolvedValue(false);

src/auth/auth.service.ts

@@ -4,6 +4,7 @@ import {
   Logger,
   NotFoundException,
   Optional,
+  ServiceUnavailableException,
   UnauthorizedException,
 } from '@nestjs/common';
 import { ConfigService } from '@nestjs/config';
@@ -1668,8 +1669,12 @@ export class AuthService {
     }
 
     if (!secret) {
-      throw new Error(
-        'RECAPTCHA_SECRET is not configured. Set CAPTCHA_BYPASS=true for development environments.',
+      // Defensive: boot-time validation catches this in validateEnvironment().
+      // At request time we must never surface a bare 500 for a configuration
+      // problem – return a stable 503 instead.
+      throw new ServiceUnavailableException(
+        'CAPTCHA verification is temporarily unavailable.',
+        'CAPTCHA_SERVICE_UNAVAILABLE',
       );
     }
 

src/auth/guards/rate-limit.guard.ts

@@ -10,12 +10,29 @@ import {
 import { Reflector } from '@nestjs/core';
 import { Request } from 'express';
 import { RateLimitService } from '../rate-limit.service';
+import { createSha256 } from '../security.utils';
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 import { RATE_LIMIT_HEADERS } from '../rate-limit.config';
 
 export const RATE_LIMIT_SKIP_KEY = 'rate-limit-skip';
 export const RATE_LIMIT_CUSTOM_KEY = 'rate-limit-custom';
 
+/**
+ * Trim and validate a single `x-forwarded-for` entry. Rejects anything that is
+ * not a plausible IP literal (v4 or v6) so header junk can never be embedded
+ * in rate-limit keys.
+ */
+export function sanitizeIpEntry(entry: string): string | null {
+  if (!entry) return null;
+  const trimmed = entry.trim();
+  if (!trimmed) return null;
+  return /^((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}|[0-9a-fA-F:]+)$/.test(
+    trimmed,
+  )
+    ? trimmed
+    : null;
+}
+
 /**
  * Decorator to skip rate limiting for a route
  */
@@ -156,17 +173,56 @@ export class RateLimitGuard implements CanActivate {
   }
 
   /**
-   * Extract client IP from request
+   * Extract client IP from request.
+   *
+   * #1195 – A raw `x-forwarded-for` header is NEVER trusted directly: without a
+   * configured reverse proxy any client can send `X-Forwarded-For: <victimIP>`
+   * and rotate the per-IP rate-limit buckets. The header is only consulted
+   * when TRUST_PROXY is enabled (app.set('trust proxy', …) in main.ts), and the
+   * returned value is a SHA-256 hash so attacker-controlled header bytes never
+   * reach the rate-limit key store verbatim.
    */
   private getClientIp(request: Request): string {
+    const rawIp = this.resolveRawClientIp(request);
+    return rawIp === 'unknown' ? rawIp : createSha256(rawIp);
+  }
+
+  private resolveRawClientIp(request: Request): string {
+    const trustProxy = this.isTrustProxyEnabled();
+
+    if (!trustProxy) {
+      // Ignore x-forwarded-for entirely. Express's request.ip is the direct
+      // connection address from the socket when trust proxy is not set.
+      return this.fromSocket(request);
+    }
+
     const forwardedFor = request.headers['x-forwarded-for'];
     const forwardedForStr = Array.isArray(forwardedFor) ? forwardedFor[0] : forwardedFor;
+
+    if (forwardedForStr) {
+      // Right-most entry is the hop closest to us when behind a trusted proxy.
+      const candidates = forwardedForStr.split(',').map((s) => sanitizeIpEntry(s)).filter(Boolean);
+      const nearest = candidates[candidates.length - 1];
+      if (nearest) {
+        return nearest;
+      }
+    }
+
+    return this.fromSocket(request);
+  }
+
+  private fromSocket(request: Request): string {
     return (
-      forwardedForStr?.split(',')[0].trim() ||
+      request.ip ||
       request.connection?.remoteAddress ||
       request.socket?.remoteAddress ||
-      request.ip ||
       'unknown'
     );
   }
+
+  private isTrustProxyEnabled(): boolean {
+    const value = process.env.TRUST_PROXY;
+    if (value === undefined || value === '') return false;
+    return !['false', '0', 'none'].includes(value.trim().toLowerCase());
+  }
 }

src/auth/rate-limit.guard.spec.ts

@@ -1,5 +1,6 @@
 import { RateLimitGuard } from './guards/rate-limit.guard';
 import { RateLimitService } from './rate-limit.service';
+import { createSha256 } from './security.utils';
 import { ExecutionContext, HttpException } from '@nestjs/common';
 import { Reflector } from '@nestjs/core';
 
@@ -95,6 +96,49 @@ describe('RateLimitGuard - auth/signup endpoints', () => {
   });
 });
 
+describe('RateLimitGuard – IPv4 header spoofing (#1195)', () => {
+  let guard: RateLimitGuard;
+  let rateLimitService: jest.Mocked<RateLimitService>;
+
+  const notExceeded = { limit: 5, remaining: 4, reset: 9999999999, isExceeded: false };
+
+  beforeEach(() => {
+    delete process.env.TRUST_PROXY;
+    rateLimitService = {
+      checkIpRateLimit: jest.fn().mockResolvedValue(notExceeded),
+      checkUserRateLimit: jest.fn().mockResolvedValue(notExceeded),
+      checkUserIpRateLimit: jest.fn().mockResolvedValue(notExceeded),
+      checkEndpointRateLimit: jest.fn().mockResolvedValue({ ...notExceeded, limit: 0 }),
+      getHeaders: jest.fn().mockReturnValue({}),
+    } as unknown as jest.Mocked<RateLimitService>;
+    const reflector = new Reflector();
+    jest.spyOn(reflector, 'getAllAndOverride').mockReturnValue(false);
+    guard = new RateLimitGuard(reflector, rateLimitService);
+  });
+
+  it('ignores a spoofed x-forwarded-for header when trust proxy is not set', async () => {
+    // Client sends X-Forwarded-For: <victim IP> but TRUST_PROXY is off →
+    // the guard must bucket by the actual connection address.
+    const ctx = makeContext({
+      headers: { 'x-forwarded-for': '203.0.113.200' },
+      ip: '1.2.3.4',
+    });
+    await expect(guard.canActivate(ctx)).resolves.toBe(true);
+    expect(rateLimitService.checkIpRateLimit).toHaveBeenCalledWith(createSha256('1.2.3.4'));
+  });
+
+  it('ignores a malformed x-forwarded-for entry when trust proxy is on', async () => {
+    process.env.TRUST_PROXY = '1';
+    const ctx = makeContext({
+      headers: { 'x-forwarded-for': '<script>evil()</script>, 1.2.3.4 ' },
+      ip: '1.2.3.4',
+    });
+    await expect(guard.canActivate(ctx)).resolves.toBe(true);
+    // Only the valid right-most entry survives sanitisation.
+    expect(rateLimitService.checkIpRateLimit).toHaveBeenCalledWith(createSha256('1.2.3.4'));
+  });
+});
+
 describe('RateLimitGuard - authenticated tier-based limiting', () => {
   let guard: RateLimitGuard;
   let rateLimitService: jest.Mocked<RateLimitService>;
@@ -124,7 +168,7 @@ describe('RateLimitGuard - authenticated tier-based limiting', () => {
     });
     await expect(guard.canActivate(ctx)).resolves.toBe(true);
     expect(rateLimitService.checkUserRateLimit).toHaveBeenCalledWith('user-1', 'premium');
-    expect(rateLimitService.checkUserIpRateLimit).toHaveBeenCalledWith('user-1', '1.2.3.4');
+    expect(rateLimitService.checkUserIpRateLimit).toHaveBeenCalledWith('user-1', createSha256('1.2.3.4'));
     expect(rateLimitService.checkIpRateLimit).not.toHaveBeenCalled();
   });
 
@@ -136,7 +180,7 @@ describe('RateLimitGuard - authenticated tier-based limiting', () => {
     });
     await expect(guard.canActivate(ctx)).resolves.toBe(true);
     expect(rateLimitService.checkUserRateLimit).not.toHaveBeenCalled();
-    expect(rateLimitService.checkIpRateLimit).toHaveBeenCalledWith('1.2.3.4');
+    expect(rateLimitService.checkIpRateLimit).toHaveBeenCalledWith(createSha256('1.2.3.4'));
   });
 
   it('defaults to the free tier when authUser.tier is missing', async () => {

src/main.ts

@@ -1,6 +1,7 @@
 import { NestFactory } from '@nestjs/core';
 import { ValidationPipe, BadRequestException } from '@nestjs/common';
 import { Reflector } from '@nestjs/core';
+import { NestExpressApplication } from '@nestjs/platform-express';
 import { AppModule } from './app.module';
 import { VersionHeaderInterceptor } from './versioning/version-header.interceptor';
 import { DeprecationWarningInterceptor } from './versioning/deprecation-warning.interceptor';
@@ -37,11 +38,21 @@ async function bootstrap() {
     process.exit(1);
   }
 
-  const app = await NestFactory.create(AppModule, {
+  const app = await NestFactory.create<NestExpressApplication>(AppModule, {
     // Issue #914 – replace NestJS default ConsoleLogger with our structured logger
     logger: new AppLogger('NestApplication'),
   });
 
+  // Issue #1195 – only trust forwarded headers behind a configured reverse
+  // proxy. Without this, RateLimitGuard ignores x-forwarded-for entirely so a
+  // spoofed header cannot rotate the per-IP rate-limit buckets.
+  const trustProxy = process.env.TRUST_PROXY;
+  if (trustProxy) {
+    app.set('trust proxy', trustProxy);
+  } else {
+    app.set('trust proxy', false);
+  }
+
   // CORS configuration
   const corsOrigins = process.env.CORS_ORIGINS
     ? process.env.CORS_ORIGINS.split(',').map((origin) => origin.trim())

src/utils/validate-env.ts

@@ -6,6 +6,19 @@ const REQUIRED_ENV_VARS = ['DATABASE_URL', 'JWT_SECRET', 'JWT_REFRESH_SECRET'] a
 const JWT_SECRET_VARS = ['JWT_SECRET', 'JWT_REFRESH_SECRET'] as const;
 const MIN_JWT_SECRET_LENGTH = 32;
 
+/**
+ * Whether CAPTCHA verification is required at runtime (#1194).
+ *
+ * CAPTCHA is required by default (docs recommend keeping it on in production)
+ * and is only disabled when CAPTCHA_BYPASS=true is explicitly set for
+ * development. When required, a missing RECAPTCHA_SECRET is a fatal
+ * configuration error that must abort boot instead of surfacing as an opaque
+ * 500 during login.
+ */
+export function isCaptchaRequired(): boolean {
+  return process.env.CAPTCHA_BYPASS !== 'true';
+}
+
 export function validateEnvironment(): void {
   const MISSING: string[] = [];
   const WEAK: string[] = [];
@@ -16,6 +29,15 @@ export function validateEnvironment(): void {
     }
   }
 
+  // #1194 – fail fast at boot when CAPTCHA is required but the secret is
+  // absent. This prevents the request-time 500 caused by a missing secret.
+  if (isCaptchaRequired() && !process.env.RECAPTCHA_SECRET) {
+    MISSING.push(
+      'RECAPTCHA_SECRET (required because CAPTCHA_BYPASS != true; ' +
+        'set CAPTCHA_BYPASS=true for development environments without reCAPTCHA)',
+    );
+  }
+
   for (const key of JWT_SECRET_VARS) {
     const value = process.env[key];
     if (value && value.length < MIN_JWT_SECRET_LENGTH) {

test/unit/validate-env.spec.ts

@@ -29,6 +29,7 @@ describe('validateEnvironment', () => {
     process.env.DATABASE_URL = 'postgresql://user:pass@localhost:5432/db';
     process.env.JWT_SECRET = 'thisisalongenoughsecretkeythatis32charsmin';
     process.env.JWT_REFRESH_SECRET = 'thisisanotherlongenoughsecretkeythatis32charsmin';
+    process.env.RECAPTCHA_SECRET = 'test-recaptcha-secret';
 
     validateEnvironment();
 
@@ -155,6 +156,35 @@ describe('validateEnvironment', () => {
     process.env.DATABASE_URL = 'postgresql://user:pass@localhost:5432/db';
     process.env.JWT_SECRET = 'a'.repeat(32);
     process.env.JWT_REFRESH_SECRET = 'b'.repeat(32);
+    process.env.RECAPTCHA_SECRET = 'test-recaptcha-secret';
+
+    validateEnvironment();
+
+    expect(mockLoggerError).not.toHaveBeenCalled();
+    expect(mockExit).not.toHaveBeenCalled();
+  });
+
+  it('should log an error and exit when RECAPTCHA_SECRET is missing and CAPTCHA is required', () => {
+    process.env.DATABASE_URL = 'postgresql://user:pass@localhost:5432/db';
+    process.env.JWT_SECRET = 'thisisalongenoughsecretkeythatis32charsmin';
+    process.env.JWT_REFRESH_SECRET = 'thisisanotherlongenoughsecretkeythatis32charsmin';
+    delete process.env.RECAPTCHA_SECRET;
+    delete process.env.CAPTCHA_BYPASS;
+
+    validateEnvironment();
+
+    expect(mockLoggerError).toHaveBeenCalled();
+    expect(mockExit).toHaveBeenCalledWith(1);
+    const errorMessage = mockLoggerError.mock.calls[0][0];
+    expect(errorMessage).toContain('RECAPTCHA_SECRET');
+  });
+
+  it('should not require RECAPTCHA_SECRET when CAPTCHA_BYPASS=true', () => {
+    process.env.DATABASE_URL = 'postgresql://user:pass@localhost:5432/db';
+    process.env.JWT_SECRET = 'thisisalongenoughsecretkeythatis32charsmin';
+    process.env.JWT_REFRESH_SECRET = 'thisisanotherlongenoughsecretkeythatis32charsmin';
+    delete process.env.RECAPTCHA_SECRET;
+    process.env.CAPTCHA_BYPASS = 'true';
 
     validateEnvironment();