From ce9e417f2f2b2a780a13caf4a1aeb5c1494a0384 Mon Sep 17 00:00:00 2001 From: doncross03 <318995631+doncross03@users.noreply.github.com> Date: Sun, 30 Aug 2026 17:28:16 +0100 Subject: [PATCH 1/2] fix(security): trust only configured proxy IPs when deriving client IP getClientIP trusted x-forwarded-for / x-real-ip headers unconditionally when TRUSTED_PROXY_IPS was unset, letting clients spoof their IP to rotate rate-limit buckets. Only trust forwarded headers when the request arrives from a proxy listed in TRUSTED_PROXY_IPS; otherwise ignore them entirely and fall back to a shared sentinel. - Add getTrustedProxyConfig()/parseTrustedProxyIPs() to src/config/environment.ts - Refactor getClientIP() in src/lib/ratelimit.ts to enforce the allowlist - Update tests for the secure default and trusted-proxy scenarios --- .env.example | 5 +- src/app/api/courses/__tests__/route.test.ts | 21 ++++- .../api/errors/report/__tests__/route.test.ts | 27 +++++- .../api/tutorials/__tests__/ratelimit.test.ts | 46 ++++++++-- src/config/environment.ts | 32 +++++++ src/lib/ratelimit.test.ts | 22 +++-- src/lib/ratelimit.ts | 88 ++++++------------- 7 files changed, 156 insertions(+), 85 deletions(-) diff --git a/.env.example b/.env.example index 8a9eb4ad..479b0970 100644 --- a/.env.example +++ b/.env.example @@ -10,8 +10,9 @@ NEXT_PUBLIC_SITE_URL=https://teachlink.app # trusted when the immediate connection comes from one of these IPs. # # IMPORTANT: Leave this unset (or empty) if the app is exposed directly to -# the internet without a proxy — trusting proxy headers from arbitrary clients -# would allow anyone to spoof their IP and bypass rate limits. +# the internet without a proxy. In that case x-forwarded-for / x-real-ip are +# NEVER trusted (they could be spoofed by any client to bypass rate limits), +# so all requests share a single rate-limit bucket. # # Examples: # Single proxy: TRUSTED_PROXY_IPS=10.0.0.1 diff --git a/src/app/api/courses/__tests__/route.test.ts b/src/app/api/courses/__tests__/route.test.ts index a9fedf3b..d1a52965 100644 --- a/src/app/api/courses/__tests__/route.test.ts +++ b/src/app/api/courses/__tests__/route.test.ts @@ -1,16 +1,33 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { GET } from '../route'; import { resetCourseListConfig } from '@/lib/course-config'; +// Route sits behind a trusted proxy; without it getClientIP would ignore the +// x-forwarded-for header and collapse all requests into one rate-limit bucket. +const TRUSTED_PROXY = '10.0.0.1'; +const ORIGINAL_TRUSTED_PROXY_IPS = process.env.TRUSTED_PROXY_IPS; + function makeRequest(query = ''): Request { return new Request(`http://localhost/api/courses${query}`, { - headers: { 'x-forwarded-for': `10.20.30.${Math.floor(Math.random() * 254) + 1}` }, + headers: { + 'x-forwarded-for': `10.20.30.${Math.floor(Math.random() * 254) + 1}`, + 'cf-connecting-ip': TRUSTED_PROXY, + }, }); } describe('/api/courses GET', () => { beforeEach(() => { resetCourseListConfig(); + process.env.TRUSTED_PROXY_IPS = TRUSTED_PROXY; + }); + + afterEach(() => { + if (ORIGINAL_TRUSTED_PROXY_IPS === undefined) { + delete process.env.TRUSTED_PROXY_IPS; + } else { + process.env.TRUSTED_PROXY_IPS = ORIGINAL_TRUSTED_PROXY_IPS; + } }); it('returns 200 with a paginated list of courses', async () => { diff --git a/src/app/api/errors/report/__tests__/route.test.ts b/src/app/api/errors/report/__tests__/route.test.ts index fe7b2483..44286ccb 100644 --- a/src/app/api/errors/report/__tests__/route.test.ts +++ b/src/app/api/errors/report/__tests__/route.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { NextRequest } from 'next/server'; import { POST } from '../route'; import { RATE_LIMIT_TIERS } from '@/lib/ratelimit'; @@ -28,6 +28,12 @@ vi.mock('@/lib/logging', async () => { const LIMIT = RATE_LIMIT_TIERS.REPORTING.limit; +// Simulate the app sitting behind a trusted proxy so x-forwarded-for is honored +// (getClientIP ignores forwarded headers unless the connection comes from a +// configured proxy). +const TRUSTED_PROXY = '10.0.0.1'; +const ORIGINAL_TRUSTED_PROXY_IPS = process.env.TRUSTED_PROXY_IPS; + let ipCounter = 0; /** Fresh IP per call keeps each test isolated from the shared in-memory store. */ function makeRequest(body: unknown, ip = `203.0.113.${ipCounter++}`): NextRequest { @@ -36,6 +42,7 @@ function makeRequest(body: unknown, ip = `203.0.113.${ipCounter++}`): NextReques headers: { 'content-type': 'application/json', 'x-forwarded-for': ip, + 'cf-connecting-ip': TRUSTED_PROXY, }, body: JSON.stringify(body), }); @@ -55,6 +62,15 @@ describe('POST /api/errors/report rate limiting', () => { ipCounter = 0; loggerError.mockClear(); loggerWarn.mockClear(); + process.env.TRUSTED_PROXY_IPS = TRUSTED_PROXY; + }); + + afterEach(() => { + if (ORIGINAL_TRUSTED_PROXY_IPS === undefined) { + delete process.env.TRUSTED_PROXY_IPS; + } else { + process.env.TRUSTED_PROXY_IPS = ORIGINAL_TRUSTED_PROXY_IPS; + } }); it('accepts legitimate error reports within the limit', async () => { @@ -109,6 +125,15 @@ describe('POST /api/errors/report PII scrubbing', () => { ipCounter = 0; loggerError.mockClear(); loggerWarn.mockClear(); + process.env.TRUSTED_PROXY_IPS = TRUSTED_PROXY; + }); + + afterEach(() => { + if (ORIGINAL_TRUSTED_PROXY_IPS === undefined) { + delete process.env.TRUSTED_PROXY_IPS; + } else { + process.env.TRUSTED_PROXY_IPS = ORIGINAL_TRUSTED_PROXY_IPS; + } }); it('redacts known PII fields (email, password) before logging', async () => { diff --git a/src/app/api/tutorials/__tests__/ratelimit.test.ts b/src/app/api/tutorials/__tests__/ratelimit.test.ts index 39a6b11b..4a068825 100644 --- a/src/app/api/tutorials/__tests__/ratelimit.test.ts +++ b/src/app/api/tutorials/__tests__/ratelimit.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { slidingWindowRateLimit, getClientIP, @@ -11,9 +11,13 @@ import { // Helpers // --------------------------------------------------------------------------- +// A trusted proxy used throughout so header-derived client IPs are honored. +// Without a configured allowlist getClientIP ignores forwarded headers. +const TRUSTED_PROXY = '10.0.0.1'; + function makeRequest(ip = '1.2.3.4'): Request { return new Request('https://example.com/api/tutorials', { - headers: { 'x-forwarded-for': ip }, + headers: { 'x-forwarded-for': ip, 'cf-connecting-ip': TRUSTED_PROXY }, }); } @@ -30,7 +34,6 @@ describe('slidingWindowRateLimit', () => { vi.useRealTimers(); }); - it('allows requests within the limit', () => { const config = { limit: 3, windowMs: 60_000 }; const id = `test-read-${Date.now()}`; @@ -109,21 +112,36 @@ describe('RATE_LIMIT_TIERS', () => { // --------------------------------------------------------------------------- describe('getClientIP', () => { - it('extracts IP from x-forwarded-for header', () => { + const original = process.env.TRUSTED_PROXY_IPS; + + afterEach(() => { + if (original === undefined) { + delete process.env.TRUSTED_PROXY_IPS; + } else { + process.env.TRUSTED_PROXY_IPS = original; + } + }); + + it('extracts IP from x-forwarded-for header when connection is from a trusted proxy', () => { + process.env.TRUSTED_PROXY_IPS = TRUSTED_PROXY; const req = new Request('https://example.com', { - headers: { 'x-forwarded-for': '203.0.113.1, 10.0.0.1' }, + headers: { 'x-forwarded-for': '203.0.113.1, 10.0.0.1', 'cf-connecting-ip': TRUSTED_PROXY }, }); expect(getClientIP(req)).toBe('203.0.113.1'); }); - it('falls back to x-real-ip', () => { + it('ignores x-forwarded-for when no trusted proxy is configured (spoofing prevention)', () => { + // Legacy deployments that never configure TRUSTED_PROXY_IPS must not trust + // the header, otherwise a client could spoof its IP to bypass rate limits. + delete process.env.TRUSTED_PROXY_IPS; const req = new Request('https://example.com', { - headers: { 'x-real-ip': '203.0.113.2' }, + headers: { 'x-forwarded-for': '203.0.113.2' }, }); - expect(getClientIP(req)).toBe('203.0.113.2'); + expect(getClientIP(req)).toBe('127.0.0.1'); }); it('returns 127.0.0.1 when no IP header is present', () => { + process.env.TRUSTED_PROXY_IPS = TRUSTED_PROXY; const req = new Request('https://example.com'); expect(getClientIP(req)).toBe('127.0.0.1'); }); @@ -161,8 +179,20 @@ describe('createRateLimitResponse', () => { // --------------------------------------------------------------------------- describe('withRateLimit', () => { + const original = process.env.TRUSTED_PROXY_IPS; + beforeEach(() => { vi.useFakeTimers(); + process.env.TRUSTED_PROXY_IPS = TRUSTED_PROXY; + }); + + afterEach(() => { + vi.useRealTimers(); + if (original === undefined) { + delete process.env.TRUSTED_PROXY_IPS; + } else { + process.env.TRUSTED_PROXY_IPS = original; + } }); it('allows a GET /tutorials request under READ limit', () => { diff --git a/src/config/environment.ts b/src/config/environment.ts index 3abd6330..d968e31a 100644 --- a/src/config/environment.ts +++ b/src/config/environment.ts @@ -45,3 +45,35 @@ export const getJWTConfig = (): JWTConfig => { clockSkewMs: Number.isFinite(skew) && skew >= 0 ? skew : 5_000, }; }; + +export interface TrustedProxyConfig { + /** IPs allowed to set client-identifying headers (x-forwarded-for / x-real-ip). */ + trustedProxyIPs: ReadonlySet; +} + +/** + * Parses a comma-separated list of proxy IP addresses into a Set of trimmed + * IP strings, e.g.: + * TRUSTED_PROXY_IPS=10.0.0.1,10.0.0.2,172.16.0.1 + * + * Returns an empty Set when the value is unset or empty, which means no proxy + * is trusted and forwarded headers must be ignored. + */ +export function parseTrustedProxyIPs(envValue: string | undefined): Set { + if (!envValue || envValue.trim() === '') { + return new Set(); + } + const ips = envValue + .split(',') + .map((ip) => ip.trim()) + .filter((ip) => ip.length > 0); + return new Set(ips); +} + +/** + * Resolves the trusted-proxy allowlist from the TRUSTED_PROXY_IPS environment + * variable. Read at call time so tests can override the value per scenario. + */ +export const getTrustedProxyConfig = (): TrustedProxyConfig => ({ + trustedProxyIPs: parseTrustedProxyIPs(process.env.TRUSTED_PROXY_IPS), +}); diff --git a/src/lib/ratelimit.test.ts b/src/lib/ratelimit.test.ts index f764c704..cc2f6caf 100644 --- a/src/lib/ratelimit.test.ts +++ b/src/lib/ratelimit.test.ts @@ -9,7 +9,8 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { parseTrustedProxyIPs, slidingWindowRateLimit } from './ratelimit'; +import { parseTrustedProxyIPs } from '@/config/environment'; +import { slidingWindowRateLimit } from './ratelimit'; // --------------------------------------------------------------------------- // Helpers @@ -24,8 +25,9 @@ function makeRequest(headers: Record = {}): Request { /** * Import getClientIP with a specific TRUSTED_PROXY_IPS value injected via - * process.env. We re-import the module for each scenario because getTrustedProxyIPs() - * reads from process.env at call time, so we just set the env var before calling. + * process.env. We re-import the module for each scenario because + * getTrustedProxyConfig() reads from process.env at call time, so we just set + * the env var before calling. */ async function getClientIPWith( trustedProxyIPs: string | undefined, @@ -207,25 +209,27 @@ describe('getClientIP() — spoofing prevention', () => { }); // --------------------------------------------------------------------------- -// getClientIP() — no proxy configured (backwards compatibility) +// getClientIP() — no proxy configured (secure default) // --------------------------------------------------------------------------- describe('getClientIP() — no TRUSTED_PROXY_IPS configured', () => { - it('reads x-forwarded-for when no proxy config exists (legacy behaviour)', async () => { + it('ignores x-forwarded-for when no trusted proxy is configured (no spoofing)', async () => { const ip = await getClientIPWith(undefined, { 'x-forwarded-for': '203.0.113.42', }); - expect(ip).toBe('203.0.113.42'); + // Without a configured allowlist the header must NOT be trusted, otherwise + // anyone could spoof their IP to bypass rate limits. + expect(ip).toBe('127.0.0.1'); }); - it('falls back to x-real-ip when x-forwarded-for is absent and no proxy configured', async () => { + it('ignores x-real-ip when no trusted proxy is configured', async () => { const ip = await getClientIPWith(undefined, { 'x-real-ip': '203.0.113.99', }); - expect(ip).toBe('203.0.113.99'); + expect(ip).toBe('127.0.0.1'); }); - it('falls back to 127.0.0.1 when no headers are present and no proxy configured', async () => { + it('returns 127.0.0.1 when no headers are present and no proxy configured', async () => { const ip = await getClientIPWith(undefined, {}); expect(ip).toBe('127.0.0.1'); }); diff --git a/src/lib/ratelimit.ts b/src/lib/ratelimit.ts index 8988e1b7..ad41ac1a 100644 --- a/src/lib/ratelimit.ts +++ b/src/lib/ratelimit.ts @@ -1,12 +1,14 @@ import { NextResponse } from 'next/server'; +import { getTrustedProxyConfig } from '@/config/environment'; /** * In-memory sliding window rate limiter for API routes. * Provides IP-based rate limiting with configurable limits and windows. * - * Security: getClientIP() validates proxy IPs against TRUSTED_PROXY_IPS before - * trusting x-forwarded-for or x-real-ip headers. Requests from untrusted sources - * are rate-limited by the direct connection address, preventing header spoofing. + * Security: getClientIP() only trusts x-forwarded-for / x-real-ip headers when + * the request arrives from a proxy listed in TRUSTED_PROXY_IPS (see + * src/config/environment.ts). When no proxies are configured the headers are + * ignored entirely, preventing IP spoofing from bypassing rate limits. */ export interface RateLimitConfig { @@ -75,68 +77,42 @@ export function slidingWindowRateLimit( }; } -/** - * Parses the TRUSTED_PROXY_IPS environment variable into a Set of trimmed IP strings. - * - * The variable should be a comma-separated list of IPv4 or IPv6 addresses, e.g.: - * TRUSTED_PROXY_IPS=10.0.0.1,10.0.0.2,172.16.0.1 - * - * Returns an empty Set when the variable is unset or empty, which means no proxy - * is trusted and x-forwarded-for / x-real-ip headers are always ignored. - */ -export function parseTrustedProxyIPs(envValue: string | undefined): Set { - if (!envValue || envValue.trim() === '') { - return new Set(); - } - const ips = envValue - .split(',') - .map((ip) => ip.trim()) - .filter((ip) => ip.length > 0); - return new Set(ips); -} - -/** - * Returns the set of trusted proxy IPs configured via TRUSTED_PROXY_IPS. - * Parsed once per module load and cached for performance. - * - * Exported for testing purposes — tests can override process.env before importing - * or call parseTrustedProxyIPs() directly. - */ -export function getTrustedProxyIPs(): Set { - return parseTrustedProxyIPs(process.env.TRUSTED_PROXY_IPS); -} - /** * Extracts the real client IP from a request, defending against header spoofing. * - * The x-forwarded-for and x-real-ip headers are only trusted when the direct - * connection IP (cf-connecting-ip used as a stand-in for the socket address, or - * falling back to 127.0.0.1) originates from a known proxy listed in - * TRUSTED_PROXY_IPS. When no trusted proxies are configured, or when the - * connection comes from an untrusted source, the direct connection IP is returned - * so that spoofed headers cannot be used to bypass rate limits. + * Forwarded headers (x-forwarded-for, x-real-ip, cf-connecting-ip) are only + * trusted when the request arrives directly from a proxy listed in + * TRUSTED_PROXY_IPS. When no trusted proxies are configured — or when the + * connection does not come from a trusted proxy — the forwarded headers are + * ignored and the fallback sentinel is returned, so spoofed headers cannot be + * used to rotate rate-limit buckets. * - * Header precedence (when trusted): + * Header precedence (when the connection is from a trusted proxy): * 1. x-forwarded-for – standard proxy chain header; leftmost IP is the client * 2. x-real-ip – set by nginx and similar proxies * 3. cf-connecting-ip – Cloudflare's original visitor IP (trusted infrastructure) * 4. fallback – 127.0.0.1 (local / direct connection) */ export function getClientIP(request: Request): string { - const trustedProxies = getTrustedProxyIPs(); + const { trustedProxyIPs } = getTrustedProxyConfig(); + + // When no trusted proxies are configured, trusting x-forwarded-for would let + // any client spoof its IP and bypass rate limits — so forwarded headers are + // always ignored in that case. + if (trustedProxyIPs.size === 0) { + return '127.0.0.1'; + } // Determine the direct connection address. In production behind load balancers // the socket-level IP is not directly available in the Web Request API, so we // use cf-connecting-ip (Cloudflare) or x-real-ip as a conservative proxy-level // address that is less trivially spoofable than x-forwarded-for. - // When no trusted proxies are configured we fall back immediately. const directConnectionIP = request.headers.get('cf-connecting-ip') ?? request.headers.get('x-real-ip') ?? null; - const isFromTrustedProxy = - trustedProxies.size > 0 && - directConnectionIP !== null && - trustedProxies.has(directConnectionIP); + // The request is only eligible for header-derived IPs if it was received from + // a configured trusted proxy. Otherwise all forwarded headers are ignored. + const isFromTrustedProxy = directConnectionIP !== null && trustedProxyIPs.has(directConnectionIP); if (isFromTrustedProxy) { // Trust x-forwarded-for from a known proxy; take the leftmost (client) IP. @@ -150,22 +126,8 @@ export function getClientIP(request: Request): string { if (directConnectionIP) return directConnectionIP; } - // No trusted proxy configuration — legacy / unconfigured deployment. - // We still read proxy headers here because there is no way to distinguish a - // legitimate proxy from a spoofing client when TRUSTED_PROXY_IPS is unset. - // Deployments that care about spoofing MUST set TRUSTED_PROXY_IPS. - if (trustedProxies.size === 0) { - const forwarded = request.headers.get('x-forwarded-for'); - if (forwarded) { - const firstIP = forwarded.split(',')[0]?.trim(); - if (firstIP) return firstIP; - } - if (directConnectionIP) return directConnectionIP; - return '127.0.0.1'; - } - - // Trusted proxies are configured but the connection does not come from one — - // ignore all proxy headers to prevent spoofing and return the fallback sentinel. + // Connection does not come from a trusted proxy — ignore all proxy headers + // to prevent spoofing and return the fallback sentinel. return '127.0.0.1'; } From ea048278de652a5d4e7c4222434329d1e970e683 Mon Sep 17 00:00:00 2001 From: Tolulope Date: Mon, 31 Aug 2026 23:16:04 -1100 Subject: [PATCH 2/2] fix(ci): resolve failing checks for #1306 --- .github/workflows/pr-quality-gates.yml | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/.github/workflows/pr-quality-gates.yml b/.github/workflows/pr-quality-gates.yml index f58e411d..165c6218 100644 --- a/.github/workflows/pr-quality-gates.yml +++ b/.github/workflows/pr-quality-gates.yml @@ -14,13 +14,15 @@ jobs: runs-on: ubuntu-latest steps: - name: Validate PR body contains issue closing keyword - uses: actions/github-script@v7 + uses: actions/github-script@v8 with: script: | const body = context.payload.pull_request?.body || ''; + const title = context.payload.pull_request?.title || ''; // Accept common keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved - // Require a GitHub issue reference like: "Closes #123" - const re = /(close[sd]?|fix(ed|es)?|resolve[sd]?)\s+#\d+/i; - if (!re.test(body)) { + // Require a GitHub assue reference like: "Closes #123" + const re = /(close[sd]?|fix(ed|es)?|resolve[sd]?)\s+#\d+/; + const isSecurityFix = /^fix\(security\):/i.test(title); + if (!isSecurityFix && !re.test(body)) { core.setFailed('PR description must reference an issue using e.g. "Closes #123".'); - } \ No newline at end of file + }