diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index f31f47b..d0e612c 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -5,10 +5,10 @@ jobs: testing: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Setup Bun - uses: oven-sh/setup-bun@v1 + uses: oven-sh/setup-bun@v2 with: bun-version: latest @@ -16,7 +16,7 @@ jobs: run: bun install - name: Linting - run: bun run format + run: bun run lint - name: Run tests run: bun test diff --git a/README.md b/README.md index f958cf8..4aabe0e 100644 --- a/README.md +++ b/README.md @@ -363,7 +363,7 @@ router.get('/api/risky', async (req: ZeroRequest) => { - **Algorithm Confusion Prevention**: JWT middleware rejects mixed symmetric/asymmetric algorithm configurations - **Cache Exhaustion Prevention**: LRU-style route cache with configurable `cacheSize` limit (default: 1000) - **Memory Exhaustion Prevention**: Strict size limits, sliding window rate limiter with `maxKeys` eviction, and automatic cleanup intervals -- **Route Filter Bypass Prevention**: URL path normalization (double-slash collapse, URI decoding, `%2F` preservation) +- **Route Filter Bypass Prevention**: URL path normalization (double-slash collapse, URI decoding, `%2F` preservation, `.` / `..` resolution) - **Frozen Route Params**: Parameterless routes receive an immutable `Object.freeze({})` to prevent cross-request data leakage #### **Error Handling** @@ -499,7 +499,7 @@ router.use( - **Fast parameter parsing**: Optimized URL parameter extraction with caching - **Query string parsing**: Uses `fast-querystring` for optimal performance - **Memory efficient**: LRU-style route caching with configurable `cacheSize` limit, immutable shared objects, and minimal allocations -- **URL normalization**: Single-pass URL parsing with path normalization (double-slash collapse, URI decoding) +- **URL normalization**: Single-pass URL parsing with path normalization (double-slash collapse, URI decoding, `.` / `..` resolution) ### Benchmark Results @@ -607,6 +607,39 @@ _Benchmarks run on Bun v1.2.2 with simple JSON response routes. Results may vary ## Changelog +### v1.3.1 — Follow-up Hardening + +Adversarial review of the v1.3.0 hardening pass. Remaining gaps in path handling, middleware consistency, and hot-path cost are addressed. + +#### Security + +- **Dot-segment resolution** — `%2e%2e` / `..` / `.` are now resolved after URI decoding so routing and `excludePaths` cannot disagree (the previous pass collapsed slashes and decoded, but left `..` in the path). +- **Shared canonical path** — JWT, rate-limit, logger, and Prometheus `excludePaths` now use the same `req.path` the router computed, instead of `new URL(req.url).pathname`. +- **Logger / Prometheus prefix matching** — `excludePaths` now uses exact-or-boundary matching (same as JWT and rate-limit). `/health` no longer skips `/healthcheck`. +- **CORS preflight `Vary: Origin`** — set for static string origins as well as function/array origins. +- **CORS `null` origin** — rejected for all non-wildcard configs, including `origin: 'null'`. +- **Logger request IDs** — header-supplied IDs are stripped of control characters and capped at 128 chars. +- **Logger response headers** — `Set-Cookie`, `Authorization`, `Cookie`, and `Proxy-Authorization` are redacted in the default serializer. +- **JWT optional mode** — `req.ctx.authError` is a generic message, not the raw jose error. +- **MemoryStore bounds** — `maxKeys` (default 10,000) plus amortized cleanup. When full, **new keys fail closed** (429) instead of evicting live counters. +- **Write-once canonical path** — security middleware reads `Symbol.for('0http.canonicalPath')` or re-parses `req.url`; mutable `req.path` is ignored. + +#### Performance + +- Router skips decode / slash-collapse / dot-resolution when the path does not need them. +- Reused frozen empty query object; single-pass param copy. +- Middleware no longer allocates `new URL()` on every request when `req.path` is set. +- JSON nesting scan short-circuits at the depth limit; body reader uses tracked byte length. +- Custom `jsonTypes` parsers are cached instead of being created per request. + +#### Ergonomics + +- Types now include `req.path`, `req.body`, `req.files`, logger options (`level`, `requestIdHeader`, `generateRequestId`), `errorHandler(err, req)`, `extended` on body parser, and `maxKeys` on rate limit. +- `ParsedFile.data` is `Uint8Array` (matches the implementation). `req.jwt.token` removed from types (removed in 1.3.0). +- CORS origin validators receive `(origin, req)` as documented. +- CORS preflight methods are compared case-insensitively. +- CI runs `bun run lint` (check) instead of `format` (write). Bench script points at `bench.ts`. + ### v1.3.0 — Security Hardening Release This release addresses **43 vulnerabilities** (6 Critical, 13 High, 13 Medium, 7 Low, 4 Info) identified in a comprehensive penetration test. All 43 issues have been resolved. diff --git a/SECURITY_REVIEWS.md b/SECURITY_REVIEWS.md index ac39794..95ac7d4 100644 --- a/SECURITY_REVIEWS.md +++ b/SECURITY_REVIEWS.md @@ -3,10 +3,98 @@ > Penetration test conducted on 2025-02-07 against `0http-bun@1.2.2`. > **43 vulnerabilities found** — 6 Critical, 13 High, 13 Medium, 7 Low, 4 Info. > **Overall Security Grade: D → B+ — All identified vulnerabilities resolved.** +> +> Follow-up adversarial review (2026-09-05) against `0http-bun@1.3.0` found additional +> gaps that the first pass claimed to have closed. Those are tracked and fixed below +> as **R2-*** (review 2). ## Remediation Progress -> **Fixed:** 6/6 Critical, 13/13 High, 13/13 Medium, 7/7 Low, 4/4 Info = **43/43 vulnerabilities resolved** ✅ +> **Review 1 (v1.3.0):** 6/6 Critical, 13/13 High, 13/13 Medium, 7/7 Low, 4/4 Info = **43/43 vulnerabilities resolved** ✅ +> +> **Review 2 (v1.3.1):** 1/1 High, 5/5 Medium, 3/3 Low = **9/9 follow-up findings resolved** ✅ +> +> **Review 2 adversarial pass:** FIFO `maxKeys` eviction and trusted `req.path` were rejected and replaced with fail-closed admission and a write-once canonical-path symbol. + +--- + +## REVIEW 2 — Adversarial follow-up (v1.3.0 → v1.3.1) + +The v1.3.0 pass fixed the issues it named, but several fixes were incomplete or inconsistent across modules. These were found by re-reading the claimed remediations against the actual code. + +### ✅ R2-H1: Path normalization did not resolve `.` / `..` (auth / route filter bypass) + +- **Status:** FIXED +- **Files:** `lib/path.js`, `lib/router/sequential.js`, JWT / rate-limit / logger / prometheus +- **Issue:** M-2 claimed to prevent bypass via `%2e%2e`, but after `decodeURIComponent` the path still contained literal `..`. Middleware `excludePaths` used `new URL(req.url).pathname` (which *does* resolve dots) while the router did not. A request to `/admin/../health` could skip JWT (`pathname === '/health'`) while routing on `/admin/../health`. +- **Fix applied:** Shared `normalizePathname()` collapses slashes, decodes (preserving `%2F`), then resolves `.` / `..`. All middleware reads `req.path` when present. + +### ✅ R2-M1: Logger and Prometheus `excludePaths` still used prefix matching + +- **Status:** FIXED +- **Files:** `lib/middleware/logger.js`, `lib/middleware/prometheus.js` +- **Issue:** H-7 / M-11 were fixed for JWT and rate-limit. Logger and Prometheus still used `pathname.startsWith(path)`, so `/health` skipped `/healthcheck`. +- **Fix applied:** Shared `isExcludedPath()` — exact or `path + '/'` boundary. + +### ✅ R2-M2: CORS preflight omitted `Vary: Origin` for static string origins + +- **Status:** FIXED +- **File:** `lib/middleware/cors.js` +- **Issue:** L-7 said Vary is set for all non-wildcard origins. Actual requests did; preflight only set Vary for function/array origins. CDNs could cache a preflight for the wrong origin. +- **Fix applied:** `applyVaryOrigin()` on every non-wildcard preflight and response. + +### ✅ R2-M3: `origin: 'null'` string config allowed sandboxed iframes + +- **Status:** FIXED +- **File:** `lib/middleware/cors.js` +- **Issue:** Null-origin rejection ran only for array/function configs. A string origin of `'null'` would reflect `Origin: null`. +- **Fix applied:** Missing/`null` origins rejected for every non-wildcard configuration. + +### ✅ R2-M4: MemoryStore had no `maxKeys` and scanned all keys on every request + +- **Status:** FIXED +- **File:** `lib/middleware/rate-limit.js` +- **Issue:** H-10 bounded the sliding-window limiter. The default fixed-window `MemoryStore` still grew without limit (worse after I-1 unique unknown keys) and ran O(n) cleanup on every increment. +- **Fix applied:** `maxKeys` (default 10,000) and amortized cleanup every 100 increments. + +### ✅ R2-M5: Client-supplied request IDs and default response logs leaked secrets + +- **Status:** FIXED +- **File:** `lib/middleware/logger.js` +- **Issue:** `requestIdHeader` values were copied verbatim (CRLF injection / log forging). Default response logging dumped all headers, including `Set-Cookie`. +- **Fix applied:** Sanitize request IDs (strip controls, max 128 chars). Redact `Set-Cookie`, `Authorization`, `Cookie`, `Proxy-Authorization`. + +### ✅ R2-L1: Optional JWT mode stored raw `error.message` on `req.ctx` + +- **Status:** FIXED +- **File:** `lib/middleware/jwt-auth.js` +- **Fix applied:** `req.ctx.authError` is now `'Invalid or expired token'`. + +### ✅ R2-L2: CORS origin validator ignored the documented `req` argument + +- **Status:** FIXED +- **File:** `lib/middleware/cors.js` +- **Fix applied:** Validators are called as `origin(requestOrigin, req)`. + +### ✅ R2-H2 (adversarial): FIFO `maxKeys` eviction reset victim counters + +- **Status:** FIXED +- **File:** `lib/middleware/rate-limit.js` +- **Issue:** First-pass `maxKeys` deleted the oldest Map entry, which an attacker could use to rotate keys and reset a victim's window. +- **Fix applied:** When the store is full, *new* keys fail closed (`totalHits` treated as over limit). Existing keys still increment. Same policy on the sliding-window limiter. + +### ✅ R2-M6 (adversarial): `getRequestPath()` trusted mutable `req.path` + +- **Status:** FIXED +- **File:** `lib/path.js` +- **Issue:** Security middleware preferred `req.path`, so application code (or a confused middleware) could set `req.path = '/health'` and skip JWT / rate-limit exclusions. +- **Fix applied:** Router stores the canonical path on a write-once `Symbol.for('0http.canonicalPath')`. `getRequestPath()` uses that symbol or re-parses `req.url` — never `req.path`. + +### ✅ R2-L3: CORS preflight method check was case-sensitive + +- **Status:** FIXED +- **File:** `lib/middleware/cors.js` +- **Fix applied:** Methods compared case-insensitively. --- diff --git a/common.d.ts b/common.d.ts index fe27523..5896d65 100644 --- a/common.d.ts +++ b/common.d.ts @@ -4,22 +4,30 @@ import {Logger} from 'pino' export interface IRouterConfig { cacheSize?: number defaultRoute?: RequestHandler - errorHandler?: (err: Error) => Response | Promise + errorHandler?: (err: Error, req?: ZeroRequest) => Response | Promise port?: number } export type StepFunction = (error?: unknown) => Response | Promise export interface ParsedFile { + filename?: string + originalName?: string name: string size: number type: string - data: File + mimetype?: string + data: Uint8Array } export type ZeroRequest = Request & { + // Canonical pathname set by the router (slash-collapsed, decoded, dot-resolved) + path?: string params: Record query: Record + // Parsed body / files (set by body-parser middleware) + body?: any + files?: Record // Connection-level IP address (set via Bun.serve's server.requestIP or upstream middleware) ip?: string remoteAddress?: string @@ -38,19 +46,22 @@ export type ZeroRequest = Request & { jwt?: { payload: any header: any - token: string } apiKey?: string + log?: Logger + requestId?: string // Context object for middleware data ctx?: { log?: Logger + requestId?: string user?: any jwt?: { payload: any header: any - token: string } apiKey?: string + authError?: string + authAttempted?: boolean rateLimit?: { limit: number used: number diff --git a/lib/middleware/README.md b/lib/middleware/README.md index a7ee14e..f9d02ee 100644 --- a/lib/middleware/README.md +++ b/lib/middleware/README.md @@ -575,6 +575,7 @@ router.use( }, }, logBody: false, + // exact or boundary match — '/health' does not skip '/healthcheck' excludePaths: ['/health', '/metrics'], }), ) @@ -660,7 +661,7 @@ const prometheus = createPrometheusIntegration({ // Control default Node.js metrics collection collectDefaultMetrics: true, - // Exclude paths from metrics collection (optimized for performance) + // Exclude paths from metrics collection (exact or boundary match) excludePaths: ['/health', '/ping', '/favicon.ico'], // Skip certain HTTP methods diff --git a/lib/middleware/body-parser.js b/lib/middleware/body-parser.js index b32310a..1fab4a3 100644 --- a/lib/middleware/body-parser.js +++ b/lib/middleware/body-parser.js @@ -45,8 +45,7 @@ async function readBodyWithLimit(req, maxBytes) { // If the request has no body stream, fall back to req.text() if (!req.body || typeof req.body.getReader !== 'function') { const text = await req.text() - const byteLength = new TextEncoder().encode(text).length - if (byteLength > maxBytes) { + if (Buffer.byteLength(text) > maxBytes) { const err = new Error('Request body size exceeded') err.status = 413 throw err @@ -81,9 +80,7 @@ async function readBodyWithLimit(req, maxBytes) { throw err } - // Concatenate chunks and decode to string - const totalLength = chunks.reduce((sum, chunk) => sum + chunk.byteLength, 0) - const combined = new Uint8Array(totalLength) + const combined = new Uint8Array(totalBytes) let offset = 0 for (const chunk of chunks) { combined.set(chunk, offset) @@ -93,6 +90,44 @@ async function readBodyWithLimit(req, maxBytes) { return new TextDecoder().decode(combined) } +const MAX_JSON_NESTING = 100 + +/** + * String-aware JSON nesting scan. Returns true if nesting exceeds the limit. + * Short-circuits as soon as the limit is exceeded. + * @param {string} text + * @param {number} maxNesting + * @returns {boolean} + */ +function isJsonNestingTooDeep(text, maxNesting = MAX_JSON_NESTING) { + let nestingLevel = 0 + let inString = false + let escape = false + for (let i = 0; i < text.length; i++) { + const ch = text[i] + if (escape) { + escape = false + continue + } + if (ch === '\\' && inString) { + escape = true + continue + } + if (ch === '"') { + inString = !inString + continue + } + if (inString) continue + if (ch === '{' || ch === '[') { + nestingLevel++ + if (nestingLevel > maxNesting) return true + } else if (ch === '}' || ch === ']') { + nestingLevel-- + } + } + return false +} + /** * Parses size limit strings with suffixes (e.g. '500b', '1kb', '2mb') * @param {number|string} limit - Size limit @@ -167,8 +202,10 @@ function isJsonType(contentType, jsonTypes) { * @param {Request} req - Request object * @returns {boolean} Whether the request method has a body */ +const BODY_METHODS = new Set(['POST', 'PUT', 'PATCH']) + function hasBody(req) { - return ['POST', 'PUT', 'PATCH'].includes(req.method.toUpperCase()) + return BODY_METHODS.has(req.method) } /** @@ -299,38 +336,9 @@ function createJSONParser(options = {}) { // Store raw body text for verification (L-3: use Symbol to prevent accidental serialization) req[RAW_BODY_SYMBOL] = text - // Additional protection against excessively deep nesting - if (text.length > 0) { - // Count nesting levels with string-awareness to prevent bypass via string content - let nestingLevel = 0 - let maxNesting = 0 - let inString = false - let escape = false - for (let i = 0; i < text.length; i++) { - const ch = text[i] - if (escape) { - escape = false - continue - } - if (ch === '\\' && inString) { - escape = true - continue - } - if (ch === '"') { - inString = !inString - continue - } - if (inString) continue - if (ch === '{' || ch === '[') { - nestingLevel++ - maxNesting = Math.max(maxNesting, nestingLevel) - } else if (ch === '}' || ch === ']') { - nestingLevel-- - } - } - if (maxNesting > 100) { - return new Response('JSON nesting too deep', {status: 400}) - } + // Additional protection against excessively deep nesting (short-circuits) + if (text.length > 0 && isJsonNestingTooDeep(text)) { + return new Response('JSON nesting too deep', {status: 400}) } // Handle empty string body — return undefined to distinguish from actual data (L-2 fix) @@ -756,6 +764,8 @@ function createBodyParser(options = {}) { multipartLimit || multipart.multipartLimit || multipart.limit || '10mb', } + const loweredJsonTypes = jsonTypes.map((t) => t.toLowerCase()) + // Create parsers with custom types consideration const jsonParserMiddleware = createJSONParser({ ...jsonOptions, @@ -775,6 +785,21 @@ function createBodyParser(options = {}) { deferNext: !!verify, }) + // Cache per-type JSON parsers so custom jsonTypes do not allocate every request + const customJsonParserCache = new Map() + function getCustomTypeJsonParser(type) { + let parser = customJsonParserCache.get(type) + if (!parser) { + parser = createJSONParser({ + ...jsonOptions, + type, + deferNext: !!verify, + }) + customJsonParserCache.set(type, parser) + } + return parser + } + return async function bodyParserMiddleware(req, next) { const contentType = req.headers.get('content-type') @@ -804,7 +829,7 @@ function createBodyParser(options = {}) { let result // Custom JSON parser handling for custom JSON types (case-insensitive) - if (jsonParser && isJsonType(contentType, jsonTypes)) { + if (jsonParser && isJsonType(contentType, loweredJsonTypes)) { const contentLength = req.headers.get('content-length') const jsonParsedLimit = parseLimit(jsonOptions.limit) if (contentLength) { @@ -836,16 +861,13 @@ function createBodyParser(options = {}) { // No result set, will be handled after verification } else { // Check if content type matches any JSON types first (including custom ones) - if (isJsonType(contentType, jsonTypes)) { - // Use a JSON parser that matches the detected custom type - const customTypeJsonParser = createJSONParser({ - ...jsonOptions, - type: jsonTypes.find((t) => + if (isJsonType(contentType, loweredJsonTypes)) { + // Use a cached JSON parser that matches the detected custom type + const matchedType = + jsonTypes.find((t) => contentType.toLowerCase().includes(t.toLowerCase()), - ), - deferNext: !!verify, - }) - result = await customTypeJsonParser(req, next) + ) || 'application/json' + result = await getCustomTypeJsonParser(matchedType)(req, next) } else { // Route to appropriate parser based on content type (case-insensitive) const lowerContentType = contentType.toLowerCase() diff --git a/lib/middleware/cors.js b/lib/middleware/cors.js index 14588ef..2d98dff 100644 --- a/lib/middleware/cors.js +++ b/lib/middleware/cors.js @@ -11,6 +11,17 @@ * @param {number} options.optionsSuccessStatus - Status code for successful OPTIONS requests * @returns {Function} Middleware function */ +function applyVaryOrigin(response) { + const existingVary = response.headers.get('Vary') + if (existingVary) { + if (!existingVary.includes('Origin')) { + response.headers.set('Vary', `${existingVary}, Origin`) + } + } else { + response.headers.set('Vary', 'Origin') + } +} + function createCORS(options = {}) { const { origin = '*', @@ -23,21 +34,22 @@ function createCORS(options = {}) { optionsSuccessStatus = 204, } = options + const methodsList = Array.isArray(methods) ? methods : [methods] + const methodsHeader = methodsList.join(', ') + const methodsUpper = new Set( + methodsList.map((m) => String(m).toUpperCase()), + ) + return function corsMiddleware(req, next) { const requestOrigin = req.headers.get('origin') const allowedOrigin = getAllowedOrigin(origin, requestOrigin, req) const addCorsHeaders = (response) => { + if (!response || !response.headers) return response + // Add Vary header for non-wildcard origins to prevent CDN cache poisoning if (origin !== '*') { - const existingVary = response.headers.get('Vary') - if (existingVary) { - if (!existingVary.includes('Origin')) { - response.headers.set('Vary', `${existingVary}, Origin`) - } - } else { - response.headers.set('Vary', 'Origin') - } + applyVaryOrigin(response) } if (allowedOrigin !== false) { @@ -62,10 +74,7 @@ function createCORS(options = {}) { } // Add method and header info - response.headers.set( - 'Access-Control-Allow-Methods', - (Array.isArray(methods) ? methods : [methods]).join(', '), - ) + response.headers.set('Access-Control-Allow-Methods', methodsHeader) const resolvedAllowedHeaders = typeof allowedHeaders === 'function' @@ -101,8 +110,11 @@ function createCORS(options = {}) { const requestMethod = req.headers.get('access-control-request-method') const requestHeaders = req.headers.get('access-control-request-headers') - // Check if requested method is allowed - if (requestMethod && !methods.includes(requestMethod)) { + // Check if requested method is allowed (case-insensitive) + if ( + requestMethod && + !methodsUpper.has(requestMethod.toUpperCase()) + ) { return new Response(null, {status: 404}) } @@ -124,23 +136,20 @@ function createCORS(options = {}) { const response = new Response(null, {status: optionsSuccessStatus}) + // Vary on all non-wildcard origins (including static strings) to prevent cache poisoning + if (origin !== '*') { + applyVaryOrigin(response) + } + if (allowedOrigin !== false) { response.headers.set('Access-Control-Allow-Origin', allowedOrigin) - // Add Vary header for dynamic origins - if (typeof origin === 'function' || Array.isArray(origin)) { - response.headers.set('Vary', 'Origin') - } - // Don't allow wildcard origin with credentials if (credentials && allowedOrigin !== '*') { response.headers.set('Access-Control-Allow-Credentials', 'true') } - response.headers.set( - 'Access-Control-Allow-Methods', - (Array.isArray(methods) ? methods : [methods]).join(', '), - ) + response.headers.set('Access-Control-Allow-Methods', methodsHeader) response.headers.set( 'Access-Control-Allow-Headers', @@ -185,19 +194,22 @@ function getAllowedOrigin(origin, requestOrigin, req) { return false } + // Reject null/missing origins for all non-wildcard configs (sandboxed iframe / file:// bypass) + if (!requestOrigin || requestOrigin === 'null') { + return false + } + if (typeof origin === 'string') { return origin === requestOrigin ? requestOrigin : false } if (Array.isArray(origin)) { - if (!requestOrigin || requestOrigin === 'null') return false return origin.includes(requestOrigin) ? requestOrigin : false } if (typeof origin === 'function') { - // Reject null/missing origins to prevent bypass via sandboxed iframes - if (!requestOrigin || requestOrigin === 'null') return false - const result = origin(requestOrigin) + // Pass req as documented so validators can inspect the request + const result = origin(requestOrigin, req) return result === true ? requestOrigin : result || false } diff --git a/lib/middleware/index.d.ts b/lib/middleware/index.d.ts index 567569b..9f7b856 100644 --- a/lib/middleware/index.d.ts +++ b/lib/middleware/index.d.ts @@ -6,6 +6,10 @@ export interface LoggerOptions { serializers?: Record any> logBody?: boolean excludePaths?: string[] + logger?: any + level?: string + requestIdHeader?: string + generateRequestId?: () => string } export function createLogger(options?: LoggerOptions): RequestHandler @@ -104,6 +108,7 @@ export function maskApiKey(key: string): string export interface RateLimitOptions { windowMs?: number max?: number + maxKeys?: number message?: string keyGenerator?: (req: ZeroRequest) => Promise | string handler?: ( @@ -127,7 +132,7 @@ export interface RateLimitStore { } export class MemoryStore implements RateLimitStore { - constructor() + constructor(options?: {maxKeys?: number; cleanupEvery?: number}) increment( key: string, windowMs: number, @@ -210,6 +215,7 @@ export interface BodyParserOptions { onError?: (error: Error, req: ZeroRequest, next: () => any) => any verify?: (req: ZeroRequest, rawBody: string) => void parseNestedObjects?: boolean + extended?: boolean jsonLimit?: number | string textLimit?: number | string urlencodedLimit?: number | string @@ -217,10 +223,13 @@ export interface BodyParserOptions { } export interface ParsedFile { + filename?: string + originalName?: string name: string size: number type: string - data: File + mimetype?: string + data: Uint8Array } export function createJSONParser(options?: JSONParserOptions): RequestHandler diff --git a/lib/middleware/jwt-auth.js b/lib/middleware/jwt-auth.js index a81f925..c9ac927 100644 --- a/lib/middleware/jwt-auth.js +++ b/lib/middleware/jwt-auth.js @@ -14,6 +14,7 @@ function loadJose() { } const crypto = require('crypto') +const {getRequestPath, isExcludedPath} = require('../path') /** * Symbol key for storing raw API key to prevent accidental serialization (M-7 fix) @@ -168,14 +169,8 @@ function createJWTAuth(options = {}) { } return async function jwtAuthMiddleware(req, next) { - const url = new URL(req.url) - - // Skip authentication for excluded paths - if ( - excludePaths.some( - (path) => url.pathname === path || url.pathname.startsWith(path + '/'), - ) - ) { + // Skip authentication for excluded paths (exact or boundary match on canonical path) + if (isExcludedPath(getRequestPath(req), excludePaths)) { return next() } @@ -279,7 +274,8 @@ function createJWTAuth(options = {}) { } catch (error) { if (optional && (!hasApiKeyMode || !req.headers.get(apiKeyHeader))) { req.ctx = req.ctx || {} - req.ctx.authError = error.message + // Do not leak jose/library internals via request context + req.ctx.authError = 'Invalid or expired token' req.ctx.authAttempted = true return next() } @@ -335,11 +331,17 @@ function extractToken(req, options = {}) { if (token) return token } - // Try query parameter + // Try query parameter (prefer already-parsed req.query) if (tokenQuery) { - const url = new URL(req.url) - const token = url.searchParams.get(tokenQuery) - if (token) return token + const fromQuery = req.query && req.query[tokenQuery] + if (fromQuery) return fromQuery + try { + const url = new URL(req.url) + const token = url.searchParams.get(tokenQuery) + if (token) return token + } catch (_) { + // Malformed URL — ignore query extraction + } } // Default: Authorization header diff --git a/lib/middleware/logger.js b/lib/middleware/logger.js index b72275c..f3605be 100644 --- a/lib/middleware/logger.js +++ b/lib/middleware/logger.js @@ -1,4 +1,40 @@ const crypto = require('crypto') +const {getRequestPath, isExcludedPath} = require('../path') + +const SENSITIVE_HEADERS = new Set([ + 'set-cookie', + 'authorization', + 'cookie', + 'proxy-authorization', + 'x-api-key', + 'x-auth-token', +]) + +/** + * Strip control characters and cap length so client-supplied IDs cannot inject logs. + * @param {unknown} id + * @returns {string|null} + */ +function sanitizeRequestId(id) { + if (typeof id !== 'string' || id.length === 0) return null + const cleaned = id.replace(/[\u0000-\u001f\u007f]/g, '').slice(0, 128) + return cleaned.length > 0 ? cleaned : null +} + +function redactHeaders(headers) { + const out = {} + if (!headers) return out + const entries = + typeof headers.entries === 'function' + ? headers.entries() + : Object.entries(headers) + for (const [key, value] of entries) { + out[key] = SENSITIVE_HEADERS.has(key.toLowerCase()) + ? '[Redacted]' + : value + } + return out +} // Lazy load pino to improve startup performance let pino = null @@ -52,7 +88,7 @@ function createLogger(options = {}) { req: (req) => ({ method: req.method, url: req.url, - headers: req.headers, + headers: redactHeaders(req.headers), ...(logBody && req.body ? {body: req.body} : {}), }), // Default res serializer removed to allow logResponse to handle it fully @@ -68,19 +104,21 @@ function createLogger(options = {}) { return function loggerMiddleware(req, next) { const startTime = process.hrtime.bigint() - const url = new URL(req.url) + const pathname = getRequestPath(req) - if (excludePaths.some((path) => url.pathname.startsWith(path))) { + if (isExcludedPath(pathname, excludePaths)) { return next() } - // Generate or extract request ID + // Generate or extract request ID (sanitize header-supplied values) let requestId - if (requestIdHeader && req.headers.get(requestIdHeader)) { - requestId = req.headers.get(requestIdHeader) - } else if (generateRequestId) { + if (requestIdHeader) { + requestId = sanitizeRequestId(req.headers.get(requestIdHeader)) + } + if (!requestId && generateRequestId) { requestId = generateRequestId() - } else { + } + if (!requestId) { requestId = crypto.randomUUID() } @@ -93,7 +131,7 @@ function createLogger(options = {}) { const childLogger = logger.child({ requestId: requestId, method: req.method, - path: url.pathname, + path: pathname, }) req.ctx.log = childLogger req.log = childLogger @@ -108,7 +146,7 @@ function createLogger(options = {}) { const logObj = { msg: 'Request started', method: req.method, - url: url.pathname, + url: pathname, } // Apply custom serializers if provided @@ -132,7 +170,7 @@ function createLogger(options = {}) { response, startTime, req, - url, + pathname, shouldLogInfo, serializers, logBody, @@ -149,7 +187,7 @@ function createLogger(options = {}) { result, startTime, req, - url, + pathname, shouldLogInfo, serializers, logBody, @@ -185,7 +223,7 @@ function logResponse( response, startTime, req, - url, + pathname, shouldLogInfo, customSerializers, // serializers from createLogger options logBodyOpt, // logBody from createLogger options @@ -224,7 +262,7 @@ function logResponse( const logEntry = { msg: 'Request completed', method: req.method, - url: url.pathname, + url: pathname, status: response && response.status, duration: duration, } @@ -239,15 +277,15 @@ function logResponse( const serializedRes = customSerializers.res(response) Object.assign(logEntry, serializedRes) } else { - // No custom res serializer: default handling for headers + // No custom res serializer: default handling for headers (redact secrets) if ( response && response.headers && typeof response.headers.entries === 'function' ) { - logEntry.headers = Object.fromEntries(response.headers.entries()) + logEntry.headers = redactHeaders(response.headers) } else if (response && response.headers) { - logEntry.headers = response.headers + logEntry.headers = redactHeaders(response.headers) } else { logEntry.headers = {} } @@ -280,8 +318,7 @@ function simpleLogger() { return function simpleLoggerMiddleware(req, next) { const startTime = Date.now() const method = req.method - const url = new URL(req.url) - const pathname = url.pathname + const pathname = getRequestPath(req) console.log(`→ ${method} ${pathname}`) diff --git a/lib/middleware/prometheus.js b/lib/middleware/prometheus.js index 35210a8..d757dca 100644 --- a/lib/middleware/prometheus.js +++ b/lib/middleware/prometheus.js @@ -1,3 +1,5 @@ +const {getRequestPath, isExcludedPath} = require('../path') + // Lazy load prom-client to improve startup performance let promClient = null function loadPromClient() { @@ -117,8 +119,7 @@ function extractRoutePattern(req) { // If params exist, try to reconstruct the pattern if (req.params && Object.keys(req.params).length > 0) { - const url = new URL(req.url, 'http://localhost') - let pattern = url.pathname + let pattern = getRequestPath(req) // Replace parameter values with parameter names Object.entries(req.params).forEach(([key, value]) => { @@ -135,8 +136,7 @@ function extractRoutePattern(req) { } // Try to normalize common patterns - const url = new URL(req.url, 'http://localhost') - let pathname = url.pathname + let pathname = getRequestPath(req) // Replace UUIDs, numbers, and other common ID patterns pathname = pathname @@ -268,21 +268,10 @@ function createPrometheusMiddleware(options = {}) { return async function prometheusMiddleware(req, next) { const startHrTime = process.hrtime() - // Skip metrics collection for excluded paths (performance optimization) - const url = req.url || '' - let pathname - try { - // Handle both full URLs and pathname-only URLs - if (url.startsWith('http')) { - pathname = new URL(url).pathname - } else { - pathname = url.split('?')[0] // Fast pathname extraction - } - } catch (error) { - pathname = url.split('?')[0] // Fallback to simple splitting - } + // Skip metrics collection for excluded paths (exact/boundary match) + const pathname = getRequestPath(req) - if (excludePaths.some((path) => pathname.startsWith(path))) { + if (isExcludedPath(pathname, excludePaths)) { return next() } @@ -429,9 +418,9 @@ function createMetricsHandler(options = {}) { const {endpoint = '/metrics', registry = client.register} = options return async function metricsHandler(req) { - const url = new URL(req.url, 'http://localhost') + const pathname = getRequestPath(req) - if (url.pathname === endpoint) { + if (pathname === endpoint) { try { const metrics = await registry.metrics() return new Response(metrics, { diff --git a/lib/middleware/rate-limit.js b/lib/middleware/rate-limit.js index dbd687c..7f16992 100644 --- a/lib/middleware/rate-limit.js +++ b/lib/middleware/rate-limit.js @@ -1,11 +1,16 @@ +const {getRequestPath, isExcludedPath} = require('../path') + /** * In-memory rate limiter implementation * For production use, consider using Redis-based storage */ class MemoryStore { - constructor() { + constructor(options = {}) { this.store = new Map() this.resetTimes = new Map() + this.maxKeys = options.maxKeys ?? 10000 + this.cleanupEvery = options.cleanupEvery ?? 100 + this._ops = 0 } increment(key, windowMs) { @@ -13,12 +18,30 @@ class MemoryStore { const windowStart = Math.floor(now / windowMs) * windowMs const storeKey = `${key}:${windowStart}` - // Clean up old entries - this.cleanup(now) + // Amortize cleanup: full scan every N increments instead of every request + this._ops++ + if (this._ops >= this.cleanupEvery) { + this._ops = 0 + this.cleanup(now) + } const current = this.store.get(storeKey) || 0 const newValue = current + 1 + // Fail closed for *new* keys when the store is full so eviction cannot + // reset a victim's counter. Existing keys still increment. + if (current === 0 && !this.store.has(storeKey)) { + if (this.store.size >= this.maxKeys) { + this.cleanup(now) + } + if (this.store.size >= this.maxKeys) { + return { + totalHits: Number.MAX_SAFE_INTEGER, + resetTime: new Date(windowStart + windowMs), + } + } + } + this.store.set(storeKey, newValue) this.resetTimes.set(storeKey, windowStart + windowMs) @@ -68,11 +91,13 @@ function createRateLimit(options = {}) { keyGenerator = defaultKeyGenerator, handler = defaultHandler, message, - store = new MemoryStore(), + maxKeys = 10000, + store, standardHeaders = true, excludePaths = [], skip, } = options + const resolvedStore = store || new MemoryStore({maxKeys}) /** * Helper function to add rate limit headers to a response @@ -108,14 +133,9 @@ function createRateLimit(options = {}) { } return async function rateLimitMiddleware(req, next) { - const activeStore = store + const activeStore = resolvedStore - const url = new URL(req.url) - if ( - excludePaths.some( - (path) => url.pathname === path || url.pathname.startsWith(path + '/'), - ) - ) { + if (isExcludedPath(getRequestPath(req), excludePaths)) { return next() } @@ -268,6 +288,8 @@ function createSlidingWindowRateLimit(options = {}) { const key = await keyGenerator(req) const now = Date.now() + const hadKey = requests.has(key) + // Get existing requests for this key let userRequests = requests.get(key) || [] @@ -287,17 +309,21 @@ function createSlidingWindowRateLimit(options = {}) { return response } + // Fail closed for new keys when the store is full (do not evict victims) + if (!hadKey && requests.size >= maxKeys) { + const response = await handler( + req, + max, + max, + new Date(now + windowMs), + ) + return response + } + // Add current request userRequests.push(now) requests.set(key, userRequests) - // Enforce max keys to prevent unbounded memory growth - if (requests.size > maxKeys) { - // Remove oldest entry - const firstKey = requests.keys().next().value - requests.delete(firstKey) - } - // Add rate limit info to context req.ctx = req.ctx || {} req.ctx.rateLimit = { diff --git a/lib/path.js b/lib/path.js new file mode 100644 index 0000000..d10b95f --- /dev/null +++ b/lib/path.js @@ -0,0 +1,187 @@ +/** + * Shared URL/path helpers used by the router and middleware. + * + * Goals: + * - One canonical pathname for routing AND excludePaths checks + * - Resolve `.` / `..` after decoding so `%2e%2e` cannot bypass filters + * - Preserve encoded slashes (`%2F`) so they cannot be used as segment separators + * - Avoid `new URL()` allocations on the hot path when `req.path` is already set + */ + +const DANGEROUS_QUERY_KEYS = new Set(['__proto__', 'constructor', 'prototype']) + +/** + * Write-once canonical pathname. Security middleware must not trust `req.path`, + * which application code can overwrite. + */ +const CANONICAL_PATH_SYMBOL = Symbol.for('0http.canonicalPath') + +/** + * Split a request URL into pathname and raw query string without using `new URL()`. + * @param {string} url + * @returns {{rawPath: string, queryString: string|null}} + */ +function splitUrl(url) { + if (typeof url !== 'string' || url.length === 0) { + return {rawPath: '/', queryString: null} + } + + let pathStart = 0 + let pathEnd = url.length + let queryString = null + + const protocolEnd = url.indexOf('://') + if (protocolEnd !== -1) { + pathStart = url.indexOf('/', protocolEnd + 3) + if (pathStart === -1) { + pathStart = url.length + } + } + + const queryStart = url.indexOf('?', pathStart) + if (queryStart !== -1) { + pathEnd = queryStart + queryString = url.substring(queryStart + 1) + } + + const rawPath = pathStart < pathEnd ? url.substring(pathStart, pathEnd) : '/' + return {rawPath, queryString} +} + +/** + * Resolve `.` and `..` segments. Never walks above the root. + * @param {string} path + * @returns {string} + */ +function resolveDotSegments(path) { + const parts = path.split('/') + const stack = [] + for (let i = 0; i < parts.length; i++) { + const part = parts[i] + if (part === '' || part === '.') continue + if (part === '..') { + stack.pop() + continue + } + stack.push(part) + } + return '/' + stack.join('/') +} + +/** + * Canonicalize a pathname: collapse slashes, decode (preserving %2F), resolve dots. + * @param {string} path + * @returns {string} + */ +function normalizePathname(path) { + if (!path || path === '/') return '/' + + let normalized = path.startsWith('/') ? path : '/' + path + if (normalized.includes('//')) { + normalized = normalized.replace(/\/\/+/g, '/') + } + + if (normalized.includes('%')) { + try { + normalized = decodeURIComponent(normalized.replace(/%2[fF]/g, '%252F')) + } catch (_) { + // Malformed URI — keep the collapsed path + } + } + + if (normalized.includes('.')) { + normalized = resolveDotSegments(normalized) + } + + return normalized || '/' +} + +/** + * Parse a request URL into a canonical path + raw query string. + * @param {string} url + * @returns {{path: string, queryString: string|null}} + */ +function parseRequestUrl(url) { + const {rawPath, queryString} = splitUrl(url) + return {path: normalizePathname(rawPath), queryString} +} + +/** + * Record the router-canonical path. `req.path` is set for app code; the + * non-enumerable symbol is what security middleware trusts. + * @param {object} req + * @param {string} path + */ +function setCanonicalPath(req, path) { + if (!req) return + try { + Object.defineProperty(req, CANONICAL_PATH_SYMBOL, { + value: path, + writable: false, + configurable: false, + enumerable: false, + }) + } catch (_) { + if (typeof req[CANONICAL_PATH_SYMBOL] !== 'string') { + req[CANONICAL_PATH_SYMBOL] = path + } + } + req.path = path +} + +/** + * Canonical pathname for security checks. + * Prefers the write-once symbol set by the router; never trusts mutable `req.path`. + * @param {{path?: string, url?: string}} req + * @returns {string} + */ +function getRequestPath(req) { + if (req && typeof req[CANONICAL_PATH_SYMBOL] === 'string') { + return req[CANONICAL_PATH_SYMBOL] + } + return parseRequestUrl(req && req.url ? req.url : '').path +} + +/** + * Exact or boundary match (NOT prefix). + * `/health` matches `/health` and `/health/live`, but not `/healthcheck`. + * @param {string} pathname + * @param {string[]} excludePaths + * @returns {boolean} + */ +function isExcludedPath(pathname, excludePaths) { + if (!excludePaths || excludePaths.length === 0) return false + for (let i = 0; i < excludePaths.length; i++) { + const p = excludePaths[i] + if (pathname === p || (p.length > 0 && pathname.startsWith(p + '/'))) { + return true + } + } + return false +} + +/** + * Remove prototype-pollution keys from a parsed query object. + * @param {Record} query + * @returns {Record} + */ +function sanitizeQuery(query) { + if (!query) return query + delete query['__proto__'] + delete query['constructor'] + delete query['prototype'] + return query +} + +module.exports = { + CANONICAL_PATH_SYMBOL, + DANGEROUS_QUERY_KEYS, + splitUrl, + resolveDotSegments, + normalizePathname, + parseRequestUrl, + setCanonicalPath, + getRequestPath, + isExcludedPath, + sanitizeQuery, +} diff --git a/lib/router/sequential.js b/lib/router/sequential.js index 1e00ab4..8e7cc04 100644 --- a/lib/router/sequential.js +++ b/lib/router/sequential.js @@ -1,6 +1,7 @@ const {Trouter} = require('trouter') const qs = require('fast-querystring') const next = require('./../next') +const {parseRequestUrl, sanitizeQuery, setCanonicalPath} = require('./../path') const STATUS_404 = { status: 404, @@ -9,9 +10,11 @@ const STATUS_500 = { status: 500, } +const DANGEROUS_PARAM_KEYS = new Set(['__proto__', 'constructor', 'prototype']) + module.exports = (config = {}) => { const cache = new Map() - const cacheSize = config.cacheSize || 1000 + const cacheSize = config.cacheSize ?? 1000 // Pre-create default responses to avoid object creation overhead const default404Response = new Response(null, STATUS_404) @@ -25,11 +28,12 @@ module.exports = (config = {}) => { return new Response('Internal Server Error', STATUS_500) }) - // Optimize empty params object reuse (frozen to prevent cross-request mutation) + // Optimize empty params/query object reuse (frozen to prevent cross-request mutation) const emptyParams = Object.freeze({}) + const emptyQuery = Object.freeze(Object.create(null)) const router = new Trouter() - router.port = config.port || 3000 + router.port = config.port ?? 3000 const _use = router.use @@ -44,50 +48,13 @@ module.exports = (config = {}) => { } router.fetch = (req) => { - const url = req.url - - // Highly optimized URL parsing - single pass through the string - let pathStart = 0 - let pathEnd = url.length - let queryString = null - - // Find protocol end - const protocolEnd = url.indexOf('://') - if (protocolEnd !== -1) { - // Find host end (start of path) - pathStart = url.indexOf('/', protocolEnd + 3) - if (pathStart === -1) { - pathStart = url.length - } - } + const {path: normalizedPath, queryString} = parseRequestUrl(req.url) - // Find query start - const queryStart = url.indexOf('?', pathStart) - if (queryStart !== -1) { - pathEnd = queryStart - queryString = url.substring(queryStart + 1) - } - - const path = pathStart < pathEnd ? url.substring(pathStart, pathEnd) : '/' - - // Normalize path: collapse double slashes and decode URI components - // Preserve encoded slashes (%2F/%2f) to maintain path structure - let normalizedPath = path.replace(/\/\/+/g, '/') - try { - normalizedPath = decodeURIComponent( - normalizedPath.replace(/%2[fF]/g, '%252F'), - ) - } catch (_) { - // Malformed URI — use the collapsed path as-is - } - - req.path = normalizedPath - req.query = queryString ? qs.parse(queryString) : {} - // L-1: Filter dangerous keys from query to prevent prototype pollution downstream + setCanonicalPath(req, normalizedPath) if (queryString) { - delete req.query['__proto__'] - delete req.query['constructor'] - delete req.query['prototype'] + req.query = sanitizeQuery(qs.parse(queryString)) + } else { + req.query = emptyQuery } // Optimized cache lookup with method-based Map structure @@ -117,31 +84,22 @@ module.exports = (config = {}) => { } if (match_result?.handlers?.length > 0) { - // Fast path for params assignment const params = match_result.params if (params) { - // Check if params object has properties without Object.keys() - let hasParams = false + let assigned = false for (const key in params) { - hasParams = true - break - } - - if (hasParams) { - req.params = req.params || {} - // Secure property copy with prototype pollution protection - for (const key in params) { - // Prevent prototype pollution by filtering dangerous properties - if ( - key !== '__proto__' && - key !== 'constructor' && - key !== 'prototype' && - Object.prototype.hasOwnProperty.call(params, key) - ) { - req.params[key] = params[key] + if ( + !DANGEROUS_PARAM_KEYS.has(key) && + Object.prototype.hasOwnProperty.call(params, key) + ) { + if (!assigned) { + req.params = req.params || {} + assigned = true } + req.params[key] = params[key] } - } else if (!req.params) { + } + if (!assigned && !req.params) { req.params = emptyParams } } else if (!req.params) { diff --git a/package.json b/package.json index 7c81c38..d030082 100644 --- a/package.json +++ b/package.json @@ -1,12 +1,12 @@ { "name": "0http-bun", - "version": "1.3.0", + "version": "1.3.1", "description": "0http for Bun", "main": "index.js", "scripts": { "lint": "prettier --check **/*.js", "test": "bun --coverage test", - "bench": "bun run bench.js", + "bench": "bun run bench.ts", "format": "prettier --write **/*.js" }, "dependencies": { diff --git a/test-types.ts b/test-types.ts index 0afef0c..f252450 100644 --- a/test-types.ts +++ b/test-types.ts @@ -156,7 +156,6 @@ const testRequestTypes = async (req: ZeroRequest): Promise => { if (jwt) { const payload = jwt.payload const header = jwt.header - const token = jwt.token } // Test rate limit structure @@ -176,14 +175,14 @@ const testRequestTypes = async (req: ZeroRequest): Promise => { const name: string = file.name const size: number = file.size const type: string = file.type - const data: File = file.data + const data: Uint8Array = file.data }) } else { const file: ParsedFile = value const name: string = file.name const size: number = file.size const type: string = file.type - const data: File = file.data + const data: Uint8Array = file.data } } } diff --git a/test/security/path-canonicalization.test.js b/test/security/path-canonicalization.test.js new file mode 100644 index 0000000..29f8b9e --- /dev/null +++ b/test/security/path-canonicalization.test.js @@ -0,0 +1,164 @@ +/* global describe, it, expect */ + +const routerFactory = require('../../lib/router/sequential') +const {createJWTAuth} = require('../../lib/middleware/jwt-auth') +const {createRateLimit} = require('../../lib/middleware/rate-limit') +const {createLogger} = require('../../lib/middleware/logger') +const {createTestRequest} = require('../helpers') + +describe('Path canonicalization security', () => { + describe('Router', () => { + it('routes /admin/../health to the /health handler', async () => { + const router = routerFactory() + router.get('/health', () => new Response('ok')) + router.get('/admin', () => new Response('admin')) + + const response = await router.fetch( + createTestRequest('GET', '/admin/../health'), + ) + expect(response.status).toBe(200) + expect(await response.text()).toBe('ok') + }) + + it('does not treat %2e%2e as a leftover traversal segment', async () => { + const router = routerFactory() + router.get('/health', () => new Response('ok')) + + const response = await router.fetch( + createTestRequest('GET', '/admin/%2e%2e/health'), + ) + expect(await response.text()).toBe('ok') + expect(response.status).toBe(200) + }) + + it('sets req.path to the canonical pathname', async () => { + const router = routerFactory() + let seen + router.get('/users', (req) => { + seen = req.path + return new Response('ok') + }) + + await router.fetch(createTestRequest('GET', '/api/../users')) + expect(seen).toBe('/users') + }) + + it('preserves encoded slashes in params', async () => { + const router = routerFactory() + router.get('/search/:term', (req) => + Response.json({term: req.params.term, path: req.path}), + ) + + const response = await router.fetch( + createTestRequest('GET', '/search/path%2Fwith%2Fslashes'), + ) + const data = await response.json() + expect(data.term).toBe('path%2Fwith%2Fslashes') + }) + }) + + describe('JWT excludePaths vs routing', () => { + it('does not skip auth for /healthcheck when /health is excluded', async () => { + const middleware = createJWTAuth({ + secret: 'test-secret-key-that-is-long-enough', + excludePaths: ['/health'], + }) + const next = () => new Response('ok') + + const skipped = await middleware( + createTestRequest('GET', '/health'), + next, + ) + expect(skipped.status).toBe(200) + + const protectedReq = createTestRequest('GET', '/healthcheck') + const blocked = await middleware(protectedReq, next) + expect(blocked.status).toBe(401) + }) + + it('uses the same canonical path as the router for excludePaths', async () => { + const middleware = createJWTAuth({ + secret: 'test-secret-key-that-is-long-enough', + excludePaths: ['/health'], + }) + const next = () => new Response('ok') + + // Traversal that canonicalizes to /health must skip auth + const traversal = createTestRequest('GET', '/admin/../health') + const skipped = await middleware(traversal, next) + expect(skipped.status).toBe(200) + + // Traversal that canonicalizes to /admin must require auth + const toAdmin = createTestRequest('GET', '/health/../admin') + const blocked = await middleware(toAdmin, next) + expect(blocked.status).toBe(401) + }) + + it('does not skip auth when req.path is forged to an excluded path', async () => { + const middleware = createJWTAuth({ + secret: 'test-secret-key-that-is-long-enough', + excludePaths: ['/health'], + }) + const next = () => new Response('ok') + const req = createTestRequest('GET', '/admin') + req.path = '/health' + + const blocked = await middleware(req, next) + expect(blocked.status).toBe(401) + }) + }) + + describe('Rate limit excludePaths', () => { + it('does not exclude prefix collisions like /healthcheck', async () => { + const middleware = createRateLimit({ + windowMs: 60_000, + max: 1, + excludePaths: ['/health'], + keyGenerator: () => 'same-client', + }) + const next = () => new Response('ok') + + const health1 = await middleware(createTestRequest('GET', '/health'), next) + const health2 = await middleware(createTestRequest('GET', '/health'), next) + expect(health1.status).toBe(200) + expect(health2.status).toBe(200) + + const first = await middleware( + createTestRequest('GET', '/healthcheck'), + next, + ) + const second = await middleware( + createTestRequest('GET', '/healthcheck'), + next, + ) + expect(first.status).toBe(200) + expect(second.status).toBe(429) + }) + }) + + describe('Logger excludePaths', () => { + it('does not skip logging for /healthcheck when /health is excluded', async () => { + const mockLog = { + child: () => mockLog, + info: () => {}, + error: () => {}, + } + let infoCalls = 0 + mockLog.info = () => { + infoCalls++ + } + + const middleware = createLogger({ + logger: mockLog, + excludePaths: ['/health'], + }) + const next = () => new Response('ok') + + await middleware(createTestRequest('GET', '/health'), next) + expect(infoCalls).toBe(0) + + await middleware(createTestRequest('GET', '/healthcheck'), next) + expect(infoCalls).toBeGreaterThan(0) + }) + }) +}) diff --git a/test/unit/cors.test.js b/test/unit/cors.test.js index 49fc8c9..9427971 100644 --- a/test/unit/cors.test.js +++ b/test/unit/cors.test.js @@ -89,6 +89,7 @@ describe('CORS Middleware', () => { expect(originValidator).toHaveBeenCalledWith( 'https://dynamic.example.com', + req, ) expect(response.headers.get('Access-Control-Allow-Origin')).toBe( 'https://dynamic.example.com', @@ -108,7 +109,7 @@ describe('CORS Middleware', () => { const response = await middleware(req, next) - expect(originValidator).toHaveBeenCalledWith('https://invalid.com') + expect(originValidator).toHaveBeenCalledWith('https://invalid.com', req) expect(response.headers.get('Access-Control-Allow-Origin')).toBeNull() }) }) @@ -523,6 +524,7 @@ describe('CORS Middleware', () => { expect(originValidator).toHaveBeenCalledWith( 'https://dynamic.example.com', + req, ) expect(response.headers.get('Access-Control-Allow-Origin')).toBe( 'https://allowed.example.com', @@ -785,4 +787,50 @@ describe('CORS Middleware', () => { ) }) }) + + describe('Hardening: Vary, method case, null origin', () => { + it('sets Vary: Origin on preflight for static string origins', async () => { + req = createTestRequest('OPTIONS', '/api/test') + req.headers = new Headers({ + Origin: 'https://example.com', + 'Access-Control-Request-Method': 'POST', + }) + + const middleware = cors({ + origin: 'https://example.com', + methods: ['GET', 'POST'], + }) + + const response = await middleware(req, next) + expect(response.status).toBe(204) + expect(response.headers.get('Vary')).toBe('Origin') + }) + + it('accepts preflight methods case-insensitively', async () => { + req = createTestRequest('OPTIONS', '/api/test') + req.headers = new Headers({ + Origin: 'https://example.com', + 'Access-Control-Request-Method': 'post', + }) + + const middleware = cors({ + origin: 'https://example.com', + methods: ['GET', 'POST'], + }) + + const response = await middleware(req, next) + expect(response.status).toBe(204) + }) + + it('rejects the literal null origin for string origin configs', async () => { + req.headers = new Headers({Origin: 'null'}) + + const middleware = cors({ + origin: 'null', + }) + + const response = await middleware(req, next) + expect(response.headers.get('Access-Control-Allow-Origin')).toBeNull() + }) + }) }) diff --git a/test/unit/logger.test.js b/test/unit/logger.test.js index 8a8483f..34e3461 100644 --- a/test/unit/logger.test.js +++ b/test/unit/logger.test.js @@ -733,4 +733,40 @@ describe('Logger Middleware', () => { ) }) }) + + describe('Hardening: request ID and header redaction', () => { + it('strips control characters from header-supplied request IDs', async () => { + req.headers = { + get: () => 'ok-id\nInjected-Header: evil', + } + + const middleware = logger({ + logger: mockLog, + requestIdHeader: 'x-request-id', + }) + + await middleware(req, next) + + expect(req.requestId).toBe('ok-idInjected-Header: evil') + expect(req.requestId).not.toMatch(/[\n\r]/) + }) + + it('redacts Set-Cookie on default response logs', async () => { + next = jest.fn( + () => + new Response('OK', { + headers: {'Set-Cookie': 'session=secret', 'X-Trace': '1'}, + }), + ) + + const middleware = logger({logger: mockLog}) + await middleware(req, next) + + const completionLog = mockLog.info.mock.calls.find( + (call) => call[0].msg === 'Request completed', + ) + expect(completionLog[0].headers['set-cookie']).toBe('[Redacted]') + expect(completionLog[0].headers['x-trace']).toBe('1') + }) + }) }) diff --git a/test/unit/path.test.js b/test/unit/path.test.js new file mode 100644 index 0000000..6a15e9e --- /dev/null +++ b/test/unit/path.test.js @@ -0,0 +1,122 @@ +/* global describe, it, expect */ + +const { + splitUrl, + resolveDotSegments, + normalizePathname, + parseRequestUrl, + getRequestPath, + setCanonicalPath, + isExcludedPath, + sanitizeQuery, +} = require('../../lib/path') + +describe('Path helpers', () => { + describe('splitUrl', () => { + it('extracts path and query from an absolute URL', () => { + expect(splitUrl('http://localhost:3000/api/users?q=1')).toEqual({ + rawPath: '/api/users', + queryString: 'q=1', + }) + }) + + it('handles missing path after host', () => { + expect(splitUrl('http://localhost')).toEqual({ + rawPath: '/', + queryString: null, + }) + }) + + it('handles empty or invalid input', () => { + expect(splitUrl('')).toEqual({rawPath: '/', queryString: null}) + expect(splitUrl(null)).toEqual({rawPath: '/', queryString: null}) + }) + }) + + describe('resolveDotSegments', () => { + it('resolves . and .. without escaping the root', () => { + expect(resolveDotSegments('/api/../admin')).toBe('/admin') + expect(resolveDotSegments('/foo/./bar')).toBe('/foo/bar') + expect(resolveDotSegments('/../secret')).toBe('/secret') + expect(resolveDotSegments('/foo/bar/..')).toBe('/foo') + }) + }) + + describe('normalizePathname', () => { + it('collapses duplicate slashes', () => { + expect(normalizePathname('/api//users')).toBe('/api/users') + }) + + it('decodes URI components but preserves encoded slashes', () => { + expect(normalizePathname('/search/hello%20world')).toBe( + '/search/hello world', + ) + expect(normalizePathname('/search/path%2Fwith%2Fslashes')).toBe( + '/search/path%2Fwith%2Fslashes', + ) + }) + + it('resolves encoded dot segments after decoding', () => { + expect(normalizePathname('/admin/%2e%2e/health')).toBe('/health') + expect(normalizePathname('/health/%2e%2e/admin')).toBe('/admin') + }) + + it('returns / for empty input', () => { + expect(normalizePathname('')).toBe('/') + expect(normalizePathname('/')).toBe('/') + }) + }) + + describe('parseRequestUrl / getRequestPath', () => { + it('parses a full request URL into a canonical path', () => { + expect(parseRequestUrl('https://example.com/a/../b?x=1')).toEqual({ + path: '/b', + queryString: 'x=1', + }) + }) + + it('does not trust a mutated req.path for security checks', () => { + expect( + getRequestPath({path: '/health', url: 'http://localhost/admin'}), + ).toBe('/admin') + }) + + it('prefers the write-once canonical path set by the router', () => { + const req = {url: 'http://localhost/other'} + setCanonicalPath(req, '/canonical') + req.path = '/health' + expect(getRequestPath(req)).toBe('/canonical') + }) + + it('falls back to parsing req.url', () => { + expect(getRequestPath({url: 'http://localhost/api/../users'})).toBe( + '/users', + ) + }) + }) + + describe('isExcludedPath', () => { + it('matches exact paths and descendants, not prefixes', () => { + expect(isExcludedPath('/health', ['/health'])).toBe(true) + expect(isExcludedPath('/health/live', ['/health'])).toBe(true) + expect(isExcludedPath('/healthcheck', ['/health'])).toBe(false) + expect(isExcludedPath('/api/users', ['/health'])).toBe(false) + }) + + it('returns false for empty exclude lists', () => { + expect(isExcludedPath('/health', [])).toBe(false) + expect(isExcludedPath('/health', null)).toBe(false) + }) + }) + + describe('sanitizeQuery', () => { + it('strips prototype pollution keys', () => { + const query = {safe: '1', __proto__: 'x', constructor: 'y', prototype: 'z'} + sanitizeQuery(query) + expect(query.safe).toBe('1') + expect(Object.hasOwn(query, '__proto__')).toBe(false) + expect(Object.hasOwn(query, 'constructor')).toBe(false) + expect(Object.hasOwn(query, 'prototype')).toBe(false) + }) + }) +}) diff --git a/test/unit/rate-limit.test.js b/test/unit/rate-limit.test.js index 9bf1eb2..6281504 100644 --- a/test/unit/rate-limit.test.js +++ b/test/unit/rate-limit.test.js @@ -610,6 +610,26 @@ describe('Rate Limit Middleware', () => { ) }) + it('fails closed for new sliding-window keys when maxKeys is exceeded', async () => { + const {createSlidingWindowRateLimit} = require('../../lib/middleware/rate-limit') + let n = 0 + const middleware = createSlidingWindowRateLimit({ + windowMs: 60_000, + max: 5, + maxKeys: 2, + keyGenerator: () => `client-${n++}`, + }) + const next = () => new Response('ok') + + const a = await middleware(createTestRequest('GET', '/a'), next) + const b = await middleware(createTestRequest('GET', '/b'), next) + const c = await middleware(createTestRequest('GET', '/c'), next) + + expect(a.status).toBe(200) + expect(b.status).toBe(200) + expect(c.status).toBe(429) + }) + it('should use custom handler in sliding window', async () => { const customHandler = jest .fn() @@ -725,4 +745,23 @@ describe('Rate Limit Middleware', () => { expect(response.headers.get('X-RateLimit-Used')).toBeNull() }) }) + + describe('MemoryStore bounds', () => { + it('fails closed for new keys when maxKeys is exceeded', async () => { + const {MemoryStore} = require('../../lib/middleware/rate-limit') + const store = new MemoryStore({maxKeys: 2, cleanupEvery: 1000}) + + const a = await store.increment('a', 60_000) + const b = await store.increment('b', 60_000) + const c = await store.increment('c', 60_000) + + expect(store.store.size).toBe(2) + expect(a.totalHits).toBe(1) + expect(b.totalHits).toBe(1) + expect(c.totalHits).toBeGreaterThan(1000) + + const a2 = await store.increment('a', 60_000) + expect(a2.totalHits).toBe(2) + }) + }) })