From e7ca5d5d4ab8763ca981936b234daf1dfa6c46aa Mon Sep 17 00:00:00 2001 From: Thosine_01 Date: Thu, 22 Oct 2026 09:28:34 +0100 Subject: [PATCH] feat(backend): implement payload sanitization and strict validation for API Gateway Signature Verifier - Add api-gateway-payload-validation module: - validateSignatureRequestParts: allowlisted HTTP methods, path must start with "/", be <= 2048 chars and contain no control characters (blocks CR/LF injection into the newline-delimited canonical payload), and length-bounded timestamp/signature headers - validateSignatureBody: JSON-safe values only (rejects NaN/Infinity, undefined, functions, BigInt, Date, Buffer...), prototype-pollution keys, depth <= 20 and <= 10k nodes; iterative so hostile bodies cannot overflow the stack - resolveSignatureToleranceSeconds: invalid/non-positive values fall back to 300s and values above 3600s are capped - sanitizeForLog: strips control chars and truncates attacker-controlled values before logging - Verifier: run strict validation before hashing, require string secrets, sanitize all logged request values, enforce MAX_KEY_ROTATION_DEPTH and skip non-string rotation keys - Fix: a non-numeric API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS produced NaN, which silently disabled the timestamp window check - Fix: unserializable bodies were hashed as "" and so matched the signature of an empty request; signing/verification now fail closed - auth.js uses the shared tolerance resolver for replay reservation - Add tests and docs/API_GATEWAY_SIGNATURE_VALIDATION.md Co-Authored-By: Claude Opus 5.5 --- .../docs/API_GATEWAY_SIGNATURE_VALIDATION.md | 74 +++++ .../src/lib/api-gateway-payload-validation.js | 255 ++++++++++++++++++ .../api-gateway-payload-validation.test.js | 213 +++++++++++++++ backend/src/lib/api-gateway-signature.js | 102 +++++-- backend/src/lib/api-gateway-signature.test.js | 141 ++++++++++ backend/src/lib/auth.js | 5 +- 6 files changed, 762 insertions(+), 28 deletions(-) create mode 100644 backend/docs/API_GATEWAY_SIGNATURE_VALIDATION.md create mode 100644 backend/src/lib/api-gateway-payload-validation.js create mode 100644 backend/src/lib/api-gateway-payload-validation.test.js diff --git a/backend/docs/API_GATEWAY_SIGNATURE_VALIDATION.md b/backend/docs/API_GATEWAY_SIGNATURE_VALIDATION.md new file mode 100644 index 00000000..280e8b78 --- /dev/null +++ b/backend/docs/API_GATEWAY_SIGNATURE_VALIDATION.md @@ -0,0 +1,74 @@ +# API Gateway Signature: Payload Sanitization & Strict Validation + +Modules: `src/lib/api-gateway-payload-validation.js`, used by `src/lib/api-gateway-signature.js` and `src/lib/auth.js`. + +Signed requests send `x-api-key`, `x-api-timestamp` (Unix seconds) and `x-api-signature: sha256=<64 hex>`. The signature is an HMAC-SHA256 over: + +``` +METHOD \n PATH \n TIMESTAMP \n sha256(JSON.stringify(body)) +``` + +Because the HMAC covers the exact body, the verifier **never rewrites the body**. Input that cannot be signed unambiguously is rejected instead, and sanitization is applied to log output. + +## Validation rules + +Checks run after the circuit-breaker, rate-limit and secret checks, and before any hashing. + +| Input | Rule | Code on failure | +|---|---|---| +| Method | One of `GET HEAD POST PUT PATCH DELETE OPTIONS` (case-insensitive) | `API_GATEWAY_INVALID_REQUEST` | +| Path | String starting with `/`, ≤ 2048 chars, no control characters (blocks CR/LF injection into the newline-delimited canonical payload) | `API_GATEWAY_INVALID_REQUEST` | +| `x-api-timestamp` | String or number, ≤ 12 chars (then the existing digits-only, safe-integer and time-window checks) | `API_GATEWAY_INVALID_REQUEST` | +| `x-api-signature` | String, ≤ 128 chars (then the existing `sha256=` + 64 hex check) | `API_GATEWAY_INVALID_REQUEST` | +| Body | JSON-safe values only; no `__proto__`, `constructor` or `prototype` keys; depth ≤ 20; ≤ 10,000 nodes | `API_GATEWAY_INVALID_PAYLOAD` | +| Secret | Must be a string ≥ 16 chars | *(existing reason)* | + +The API responds `401` with `{ error, code, reason }`, the same shape as other signature failures. + +### Why non-JSON values are rejected + +`JSON.stringify` silently rewrites some values, which would let two different bodies share one signature: + +- `NaN` and `Infinity` become `null` +- `undefined` properties and functions are dropped +- `Date` becomes a string and `Buffer` becomes `{ type, data }` +- `BigInt` and circular structures throw + +Bodies parsed from JSON never contain these values. Programmatic callers that pass them now get a clear rejection instead of a signature that collides with another body. + +### Signing + +`signApiGatewayRequest` applies the same body validation and returns `null` for invalid bodies. Previously, a body that failed to serialize was hashed as an empty string, so its signature matched an empty request. + +## Configuration + +`API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS` sets the accepted clock skew in seconds, resolved by `resolveSignatureToleranceSeconds()`: + +| Value | Result | +|---|---| +| unset / empty | `300` (default) | +| valid positive number | used as-is | +| non-numeric, `NaN`, `0`, negative | `300`, with a warning logged | +| above `3600` | capped at `3600`, with a warning logged | + +Previously, a non-numeric value produced `NaN`. The comparison `delta > NaN` is always false, so the timestamp window was silently disabled. + +## Key rotation + +`verifyApiGatewayRequestSignatureWithRotation` honours at most `MAX_KEY_ROTATION_DEPTH` (2: current and previous) secrets and skips non-string entries. Extra keys are ignored, with a warning logged. + +## Logging + +`sanitizeForLog()` is applied to every attacker-controlled value the verifier logs: client IP, method, path, timestamp header and signature header. + +- Control characters are replaced with `?`, so CR/LF cannot forge log lines. +- Strings are truncated to 80 chars, with the number of dropped chars noted. +- Objects and arrays are logged as `[object]` / `[array]`, never dumped. + +## Tests + +```bash +cd backend +npx vitest run src/lib/api-gateway-payload-validation.test.js src/lib/api-gateway-signature.test.js src/lib/auth.test.js +npx vitest run tests/e2e/api-gateway-security.e2e.test.js +``` diff --git a/backend/src/lib/api-gateway-payload-validation.js b/backend/src/lib/api-gateway-payload-validation.js new file mode 100644 index 00000000..972eda09 --- /dev/null +++ b/backend/src/lib/api-gateway-payload-validation.js @@ -0,0 +1,255 @@ +/** + * api-gateway-payload-validation.js + * + * Payload sanitization and strict validation for the API Gateway Signature + * Verifier (api-gateway-signature.js). + * + * Unlike merchant-payload-validation.js, nothing here rewrites the request + * body: the signature is an HMAC over the exact body, so stripping or + * normalizing it before verification would break legitimate signatures. + * Instead, input that cannot be signed unambiguously is REJECTED, and + * sanitization is applied to what we log. + * + * 1. validateSignatureRequestParts + * - method must be a known HTTP method (no CR/LF smuggling into the + * newline-delimited canonical payload) + * - path must start with "/", be bounded in length and contain no + * control characters + * - timestamp / signature headers are length-bounded before any regex + * runs on them + * + * 2. validateSignatureBody + * - only JSON-safe values: values JSON.stringify would silently drop or + * rewrite (undefined, NaN, Infinity, functions, BigInt, Dates, Buffers) + * are rejected so two different bodies can never share a signature + * - prototype-pollution keys (__proto__, constructor, prototype) are + * rejected before the body reaches any route handler + * - depth and total node count are bounded so verification cannot be + * used to force deep recursion or excessive hashing work + * + * 3. resolveSignatureToleranceSeconds + * - a non-numeric, zero or negative tolerance would make the timestamp + * window check always pass (|delta| > NaN is false), so invalid values + * fall back to the default and large values are capped + * + * 4. sanitizeForLog + * - strips control characters and truncates attacker-controlled values + * before they are written to structured logs (log injection / log + * flooding) + */ + +import { logger } from "./logger.js"; + +export const DEFAULT_SIGNATURE_TOLERANCE_SECONDS = 300; +/** Upper bound on the accepted clock-skew window. Larger windows weaken replay protection. */ +export const MAX_SIGNATURE_TOLERANCE_SECONDS = 3600; + +export const GATEWAY_VALIDATION_LIMITS = Object.freeze({ + maxPathLength: 2048, + // Unix seconds fit in 10 digits until the year 2286; allow a little headroom. + maxTimestampLength: 12, + // "sha256=" + 64 hex chars, plus surrounding whitespace the verifier trims. + maxSignatureHeaderLength: 128, + maxBodyDepth: 20, + maxBodyNodes: 10000, + maxLogValueLength: 80, +}); + +export const ALLOWED_SIGNATURE_METHODS = Object.freeze( + new Set(["GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"]), +); + +const FORBIDDEN_KEYS = new Set(["__proto__", "constructor", "prototype"]); + +// eslint-disable-next-line no-control-regex +const CONTROL_CHARS = /[\u0000-\u001F\u007F]/; +// eslint-disable-next-line no-control-regex +const CONTROL_CHARS_GLOBAL = /[\u0000-\u001F\u007F]/g; + +export const GATEWAY_VALIDATION_CODES = Object.freeze({ + INVALID_REQUEST: "API_GATEWAY_INVALID_REQUEST", + INVALID_PAYLOAD: "API_GATEWAY_INVALID_PAYLOAD", +}); + +function invalid(code, reason, detail) { + return { valid: false, code, reason, ...(detail && { detail }) }; +} + +/** + * Make an untrusted value safe to include in a structured log entry. + * Strings have control characters replaced and are truncated; other values + * are reduced to their type so objects/arrays are never dumped wholesale. + * + * @param {unknown} value + * @param {number} [maxLength] + * @returns {string|number|boolean|null|undefined} + */ +export function sanitizeForLog(value, maxLength = GATEWAY_VALIDATION_LIMITS.maxLogValueLength) { + if (value === null || value === undefined) return value; + if (typeof value === "number" || typeof value === "boolean") return value; + if (typeof value !== "string") return `[${Array.isArray(value) ? "array" : typeof value}]`; + + const cleaned = value.replace(CONTROL_CHARS_GLOBAL, "?"); + return cleaned.length > maxLength + ? `${cleaned.slice(0, maxLength)}…(+${cleaned.length - maxLength} chars)` + : cleaned; +} + +/** + * Validate the request line and signature headers before they are used to + * build the canonical payload. + * + * `method` and `path` may be omitted by callers that sign internally + * generated requests; they then default to GET and "/" exactly as the + * canonical payload builder does. + * + * @returns {{ valid: true } | { valid: false, code: string, reason: string, detail?: string }} + */ +export function validateSignatureRequestParts({ method, path, timestampHeader, signatureHeader }) { + const { INVALID_REQUEST } = GATEWAY_VALIDATION_CODES; + + if (method !== undefined && method !== null) { + if (typeof method !== "string" || !ALLOWED_SIGNATURE_METHODS.has(method.toUpperCase())) { + return invalid(INVALID_REQUEST, "Unsupported HTTP method for signed request", "method"); + } + } + + if (path !== undefined && path !== null) { + if (typeof path !== "string" || !path.startsWith("/")) { + return invalid(INVALID_REQUEST, "Request path must be a string starting with '/'", "path"); + } + if (path.length > GATEWAY_VALIDATION_LIMITS.maxPathLength) { + return invalid(INVALID_REQUEST, "Request path exceeds maximum length", "path"); + } + if (CONTROL_CHARS.test(path)) { + return invalid(INVALID_REQUEST, "Request path contains control characters", "path"); + } + } + + if ( + timestampHeader !== undefined && + timestampHeader !== null && + (typeof timestampHeader !== "string" && typeof timestampHeader !== "number") + ) { + return invalid(INVALID_REQUEST, "Missing or invalid x-api-timestamp header", "timestamp"); + } + if (String(timestampHeader ?? "").trim().length > GATEWAY_VALIDATION_LIMITS.maxTimestampLength) { + return invalid(INVALID_REQUEST, "Missing or invalid x-api-timestamp header", "timestamp"); + } + + if (signatureHeader !== undefined && signatureHeader !== null) { + if (typeof signatureHeader !== "string") { + return invalid(INVALID_REQUEST, "Missing or invalid x-api-signature header", "signature"); + } + if (signatureHeader.length > GATEWAY_VALIDATION_LIMITS.maxSignatureHeaderLength) { + return invalid(INVALID_REQUEST, "Missing or invalid x-api-signature header", "signature"); + } + } + + return { valid: true }; +} + +function isPlainObject(value) { + if (value === null || typeof value !== "object") return false; + const proto = Object.getPrototypeOf(value); + return proto === Object.prototype || proto === null; +} + +/** + * Strictly validate a request body before it is hashed for signing. + * + * Accepts `undefined` (no body) and any JSON-safe value. Iterative rather + * than recursive so a hostile body cannot overflow the stack. + * + * @param {unknown} body + * @returns {{ valid: true } | { valid: false, code: string, reason: string, detail?: string }} + */ +export function validateSignatureBody(body) { + const { INVALID_PAYLOAD } = GATEWAY_VALIDATION_CODES; + const { maxBodyDepth, maxBodyNodes } = GATEWAY_VALIDATION_LIMITS; + + if (body === undefined) return { valid: true }; + + const stack = [{ value: body, depth: 0, at: "body" }]; + let nodes = 0; + + while (stack.length > 0) { + const { value, depth, at } = stack.pop(); + + nodes += 1; + if (nodes > maxBodyNodes) { + return invalid(INVALID_PAYLOAD, "Request body is too large to verify", "body"); + } + + if (value === null || typeof value === "string" || typeof value === "boolean") continue; + + if (typeof value === "number") { + if (!Number.isFinite(value)) { + return invalid(INVALID_PAYLOAD, "Request body contains a non-finite number", at); + } + continue; + } + + const isArray = Array.isArray(value); + if (!isArray && !isPlainObject(value)) { + // undefined, functions, symbols, BigInt, Date, Buffer, Map, class instances… + return invalid(INVALID_PAYLOAD, "Request body contains a value that is not valid JSON", at); + } + + if (depth >= maxBodyDepth) { + return invalid(INVALID_PAYLOAD, "Request body exceeds maximum nesting depth", at); + } + + if (isArray) { + for (let i = 0; i < value.length; i++) { + stack.push({ value: value[i], depth: depth + 1, at: `${at}[${i}]` }); + } + continue; + } + + for (const key of Object.keys(value)) { + if (FORBIDDEN_KEYS.has(key)) { + return invalid(INVALID_PAYLOAD, "Request body contains a forbidden key", sanitizeForLog(`${at}.${key}`)); + } + stack.push({ value: value[key], depth: depth + 1, at: `${at}.${key}` }); + } + } + + return { valid: true }; +} + +/** + * Resolve the accepted timestamp skew, in seconds. + * + * Invalid values (non-numeric, NaN, zero, negative) fall back to the default + * with a warning; values above MAX_SIGNATURE_TOLERANCE_SECONDS are capped. + * + * @param {unknown} [raw] - defaults to API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS + * @returns {number} + */ +export function resolveSignatureToleranceSeconds( + raw = process.env.API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS, +) { + if (raw === undefined || raw === null || raw === "") { + return DEFAULT_SIGNATURE_TOLERANCE_SECONDS; + } + + const value = typeof raw === "number" ? raw : Number(String(raw).trim()); + if (!Number.isFinite(value) || value <= 0) { + logger.warn( + { configured: sanitizeForLog(String(raw)), fallback: DEFAULT_SIGNATURE_TOLERANCE_SECONDS }, + "Invalid API gateway signature tolerance; using default", + ); + return DEFAULT_SIGNATURE_TOLERANCE_SECONDS; + } + + if (value > MAX_SIGNATURE_TOLERANCE_SECONDS) { + logger.warn( + { configured: value, max: MAX_SIGNATURE_TOLERANCE_SECONDS }, + "API gateway signature tolerance exceeds maximum; capping", + ); + return MAX_SIGNATURE_TOLERANCE_SECONDS; + } + + return value; +} diff --git a/backend/src/lib/api-gateway-payload-validation.test.js b/backend/src/lib/api-gateway-payload-validation.test.js new file mode 100644 index 00000000..6a8c56e9 --- /dev/null +++ b/backend/src/lib/api-gateway-payload-validation.test.js @@ -0,0 +1,213 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + DEFAULT_SIGNATURE_TOLERANCE_SECONDS, + GATEWAY_VALIDATION_CODES, + GATEWAY_VALIDATION_LIMITS, + MAX_SIGNATURE_TOLERANCE_SECONDS, + resolveSignatureToleranceSeconds, + sanitizeForLog, + validateSignatureBody, + validateSignatureRequestParts, +} from "./api-gateway-payload-validation.js"; + +vi.mock("./logger.js", () => ({ + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +const VALID_PARTS = { + method: "POST", + path: "/api/payments?limit=10", + timestampHeader: "1713916800", + signatureHeader: `sha256=${"a".repeat(64)}`, +}; + +function nest(depth) { + let value = { leaf: true }; + for (let i = 0; i < depth; i++) value = { child: value }; + return value; +} + +describe("sanitizeForLog", () => { + it("replaces control characters so log lines cannot be forged", () => { + expect(sanitizeForLog("abc\r\nINFO forged entry")).toBe("abc??INFO forged entry"); + expect(sanitizeForLog("a\u0000b\u001bc\u007f")).toBe("a?b?c?"); + }); + + it("truncates long values and reports how much was cut", () => { + const result = sanitizeForLog("x".repeat(100), 10); + expect(result).toBe(`${"x".repeat(10)}…(+90 chars)`); + }); + + it("reduces non-string values to their type instead of dumping them", () => { + expect(sanitizeForLog({ secret: "s" })).toBe("[object]"); + expect(sanitizeForLog(["a"])).toBe("[array]"); + expect(sanitizeForLog(() => {})).toBe("[function]"); + }); + + it("passes through primitives and nullish values unchanged", () => { + expect(sanitizeForLog(42)).toBe(42); + expect(sanitizeForLog(false)).toBe(false); + expect(sanitizeForLog(null)).toBeNull(); + expect(sanitizeForLog(undefined)).toBeUndefined(); + }); +}); + +describe("validateSignatureRequestParts", () => { + const { INVALID_REQUEST } = GATEWAY_VALIDATION_CODES; + + it("accepts a well-formed request", () => { + expect(validateSignatureRequestParts(VALID_PARTS)).toEqual({ valid: true }); + }); + + it("accepts lowercase methods and omitted method/path (canonical defaults)", () => { + expect(validateSignatureRequestParts({ ...VALID_PARTS, method: "post" }).valid).toBe(true); + expect(validateSignatureRequestParts({ ...VALID_PARTS, method: undefined, path: undefined }).valid).toBe(true); + }); + + it.each([ + ["unknown verb", "TRACE"], + ["CR/LF injection", "POST\n/api/admin"], + ["non-string", 123], + ["empty string", ""], + ])("rejects method: %s", (_label, method) => { + const result = validateSignatureRequestParts({ ...VALID_PARTS, method }); + expect(result).toMatchObject({ valid: false, code: INVALID_REQUEST, detail: "method" }); + }); + + it.each([ + ["relative path", "api/payments"], + ["absolute URL", "https://evil.example/api"], + ["newline injection", "/api/payments\n1713916800"], + ["NUL byte", "/api/pay\u0000ments"], + ["non-string", ["/api"]], + ["too long", `/${"a".repeat(GATEWAY_VALIDATION_LIMITS.maxPathLength)}`], + ])("rejects path: %s", (_label, path) => { + const result = validateSignatureRequestParts({ ...VALID_PARTS, path }); + expect(result).toMatchObject({ valid: false, code: INVALID_REQUEST, detail: "path" }); + }); + + it("rejects oversized or non-string timestamp headers before parsing", () => { + expect( + validateSignatureRequestParts({ ...VALID_PARTS, timestampHeader: "1".repeat(13) }), + ).toMatchObject({ valid: false, detail: "timestamp" }); + expect( + validateSignatureRequestParts({ ...VALID_PARTS, timestampHeader: { n: 1 } }), + ).toMatchObject({ valid: false, detail: "timestamp" }); + }); + + it("accepts numeric timestamps (internal callers)", () => { + expect(validateSignatureRequestParts({ ...VALID_PARTS, timestampHeader: 1713916800 }).valid).toBe(true); + }); + + it("rejects oversized or non-string signature headers", () => { + expect( + validateSignatureRequestParts({ ...VALID_PARTS, signatureHeader: `sha256=${"a".repeat(200)}` }), + ).toMatchObject({ valid: false, detail: "signature" }); + expect( + validateSignatureRequestParts({ ...VALID_PARTS, signatureHeader: ["sha256=a", "sha256=b"] }), + ).toMatchObject({ valid: false, detail: "signature" }); + }); + + it("leaves missing headers to the verifier's existing format checks", () => { + expect( + validateSignatureRequestParts({ ...VALID_PARTS, timestampHeader: undefined, signatureHeader: undefined }).valid, + ).toBe(true); + }); +}); + +describe("validateSignatureBody", () => { + const { INVALID_PAYLOAD } = GATEWAY_VALIDATION_CODES; + + it.each([ + ["no body", undefined], + ["null", null], + ["empty object", {}], + ["typical payment", { amount: 12.5, asset: "USDC", memo: "order #1", tags: ["a", "b"], meta: { ok: true } }], + ["array root", [1, "two", null]], + ["string root", "raw"], + ["null-prototype object", Object.assign(Object.create(null), { a: 1 })], + ])("accepts %s", (_label, body) => { + expect(validateSignatureBody(body)).toEqual({ valid: true }); + }); + + it.each([ + ["__proto__", JSON.parse('{"a":{"__proto__":{"isAdmin":true}}}'), "body.a.__proto__"], + ["constructor", { constructor: { prototype: {} } }, "body.constructor"], + ["prototype", { items: [{ prototype: 1 }] }, "body.items[0].prototype"], + ])("rejects prototype-pollution key %s and reports where", (_label, body, at) => { + expect(validateSignatureBody(body)).toMatchObject({ valid: false, code: INVALID_PAYLOAD, detail: at }); + }); + + it.each([ + ["NaN (serializes as null)", { amount: NaN }], + ["Infinity", { amount: Infinity }], + ["nested undefined (dropped by JSON)", { a: undefined }], + ["function", { fn: () => {} }], + ["BigInt", { n: BigInt(1) }], + ["Date", { at: new Date(0) }], + ["Buffer", { data: Buffer.from("x") }], + ["Map", { m: new Map() }], + ["symbol", { s: Symbol("x") }], + ])("rejects values JSON would silently rewrite: %s", (_label, body) => { + expect(validateSignatureBody(body)).toMatchObject({ valid: false, code: INVALID_PAYLOAD }); + }); + + it("enforces the maximum nesting depth", () => { + const { maxBodyDepth } = GATEWAY_VALIDATION_LIMITS; + expect(validateSignatureBody(nest(maxBodyDepth - 1)).valid).toBe(true); + expect(validateSignatureBody(nest(maxBodyDepth + 1))).toMatchObject({ + valid: false, + reason: expect.stringMatching(/depth/), + }); + }); + + it("handles extremely deep bodies without overflowing the stack", () => { + expect(() => validateSignatureBody(nest(100000))).not.toThrow(); + expect(validateSignatureBody(nest(100000)).valid).toBe(false); + }); + + it("enforces the maximum node count", () => { + const big = { items: new Array(GATEWAY_VALIDATION_LIMITS.maxBodyNodes).fill(1) }; + expect(validateSignatureBody(big)).toMatchObject({ valid: false, reason: expect.stringMatching(/too large/) }); + }); + + it("rejects circular structures (via the node limit) instead of looping forever", () => { + const circular = { a: 1 }; + circular.self = circular; + expect(validateSignatureBody(circular).valid).toBe(false); + }); +}); + +describe("resolveSignatureToleranceSeconds", () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("uses the default when unset or empty", () => { + expect(resolveSignatureToleranceSeconds(undefined)).toBe(DEFAULT_SIGNATURE_TOLERANCE_SECONDS); + expect(resolveSignatureToleranceSeconds("")).toBe(DEFAULT_SIGNATURE_TOLERANCE_SECONDS); + }); + + it("accepts valid numbers and numeric strings", () => { + expect(resolveSignatureToleranceSeconds(120)).toBe(120); + expect(resolveSignatureToleranceSeconds(" 60 ")).toBe(60); + }); + + it.each([["abc"], [NaN], [0], [-30], ["Infinity"]])( + "falls back to the default for invalid value %s", + (raw) => { + expect(resolveSignatureToleranceSeconds(raw)).toBe(DEFAULT_SIGNATURE_TOLERANCE_SECONDS); + }, + ); + + it("caps excessively large windows", () => { + expect(resolveSignatureToleranceSeconds(86400)).toBe(MAX_SIGNATURE_TOLERANCE_SECONDS); + }); + + it("reads API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS by default", () => { + vi.stubEnv("API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS", "45"); + expect(resolveSignatureToleranceSeconds()).toBe(45); + vi.stubEnv("API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS", "not-a-number"); + expect(resolveSignatureToleranceSeconds()).toBe(DEFAULT_SIGNATURE_TOLERANCE_SECONDS); + }); +}); diff --git a/backend/src/lib/api-gateway-signature.js b/backend/src/lib/api-gateway-signature.js index 6c934c4f..ae90615b 100644 --- a/backend/src/lib/api-gateway-signature.js +++ b/backend/src/lib/api-gateway-signature.js @@ -2,8 +2,13 @@ import crypto from "node:crypto"; import { logger } from "./logger.js"; import { apiGatewaySignatureCacheSize, apiGatewayReplayBlockedTotal } from "./metrics.js"; import { connectRedisClient } from "./redis.js"; +import { + resolveSignatureToleranceSeconds, + sanitizeForLog, + validateSignatureBody, + validateSignatureRequestParts, +} from "./api-gateway-payload-validation.js"; -const DEFAULT_SIGNATURE_WINDOW_SECONDS = 300; // Minimum HMAC secret length to prevent signing with trivially weak keys const MIN_SECRET_LENGTH = 16; @@ -243,15 +248,18 @@ function normalizeSignatureHeader(signatureHeader) { } } +// Fails closed: a body that cannot be serialized (circular, BigInt, …) must +// never be signed as if it were empty, or any such body would match the +// signature of an empty request. function safeJsonStringify(value) { + if (value === undefined) { + return ""; + } try { - if (value === undefined) { - return ""; - } return JSON.stringify(value); } catch (err) { logger.warn({ err }, "Failed to stringify value for signature"); - return ""; + throw new Error("Request body cannot be serialized for signature"); } } @@ -317,7 +325,7 @@ export async function reserveApiGatewaySignature({ const result = await client.set( distributedReplayKey(secret, signatureHeader), "1", - { NX: true, EX: Math.max(1, Math.ceil(toleranceSeconds)) }, + { NX: true, EX: Math.max(1, Math.ceil(resolveSignatureToleranceSeconds(toleranceSeconds))) }, ); if (result !== "OK") { @@ -361,16 +369,25 @@ export function verifyApiGatewayRequestSignatureWithRotation({ signatureHeader, body, now = Date.now(), - toleranceSeconds = Number( - process.env.API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS || DEFAULT_SIGNATURE_WINDOW_SECONDS, - ), + toleranceSeconds, }) { if (!secrets || !Array.isArray(secrets)) { return { valid: false, reason: "No secrets provided for signature verification" }; } - for (let keyIndex = 0; keyIndex < secrets.length; keyIndex++) { - const secret = secrets[keyIndex]; + // Only the current and previous key are honoured; anything beyond + // MAX_KEY_ROTATION_DEPTH would silently widen the set of accepted keys. + if (secrets.length > MAX_KEY_ROTATION_DEPTH) { + logger.warn( + { provided: secrets.length, max: MAX_KEY_ROTATION_DEPTH }, + "Too many secrets supplied for API gateway key rotation; extra keys ignored", + ); + } + const candidateSecrets = secrets.slice(0, MAX_KEY_ROTATION_DEPTH); + + for (let keyIndex = 0; keyIndex < candidateSecrets.length; keyIndex++) { + const secret = candidateSecrets[keyIndex]; + if (typeof secret !== "string") continue; const result = verifyApiGatewayRequestSignature({ secret, method, @@ -404,11 +421,17 @@ export function signApiGatewayRequest({ body, }) { try { - if (!secret || secret.length < MIN_SECRET_LENGTH || !timestamp) { + if (typeof secret !== "string" || secret.length < MIN_SECRET_LENGTH || !timestamp) { logger.warn({ secretLength: secret?.length }, "Invalid parameters for signing API gateway request"); return null; } + const bodyCheck = validateSignatureBody(body); + if (!bodyCheck.valid) { + logger.warn({ reason: bodyCheck.reason, at: bodyCheck.detail }, "Refusing to sign invalid API gateway payload"); + return null; + } + const payload = buildCanonicalPayload({ method, path, timestamp, body }); return crypto.createHmac("sha256", secret).update(payload, "utf8").digest("hex"); } catch (err) { @@ -426,10 +449,12 @@ export function verifyApiGatewayRequestSignature({ body, clientIp, now = Date.now(), - toleranceSeconds = Number( - process.env.API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS || DEFAULT_SIGNATURE_WINDOW_SECONDS, - ), + toleranceSeconds: rawToleranceSeconds, }) { + // A NaN / non-positive tolerance would make the window check always pass. + const toleranceSeconds = resolveSignatureToleranceSeconds(rawToleranceSeconds); + const logIp = sanitizeForLog(clientIp); + // Error recovery #900: Check circuit breaker first if (_isCircuitBreakerOpen(now)) { logger.warn("API gateway signature verification circuit breaker is open, rejecting request"); @@ -444,7 +469,7 @@ export function verifyApiGatewayRequestSignature({ // Rate limiting check (issue #897) if (clientIp && isApiGatewayRateLimited(clientIp, now)) { const rateLimitInfo = getApiGatewayRateLimitInfo(clientIp, now); - logger.warn({ clientIp, rateLimitInfo }, "API gateway signature verification rate limit exceeded"); + logger.warn({ clientIp: logIp, rateLimitInfo }, "API gateway signature verification rate limit exceeded"); return { valid: false, reason: "API gateway signature verification rate limit exceeded", @@ -453,25 +478,49 @@ export function verifyApiGatewayRequestSignature({ }; } - if (!secret || secret.length < MIN_SECRET_LENGTH) { + if (typeof secret !== "string" || secret.length < MIN_SECRET_LENGTH) { recordApiGatewaySignatureAttempt(clientIp, false, now); _recordCircuitBreakerFailure(now); - logger.warn({ secretLength: secret?.length, clientIp }, "Missing or insufficient signature secret"); + logger.warn({ secretLength: secret?.length, clientIp: logIp }, "Missing or insufficient signature secret"); return { valid: false, reason: "Missing or insufficient signature secret" }; } + // Strict validation of the request line, headers and body before any of + // them is hashed. Bounded checks run first so oversized input is rejected + // before regexes or JSON serialization touch it. + const inputCheck = validateSignatureRequestParts({ method, path, timestampHeader, signatureHeader }); + const validation = inputCheck.valid ? validateSignatureBody(body) : inputCheck; + if (!validation.valid) { + recordApiGatewaySignatureAttempt(clientIp, false, now); + _recordCircuitBreakerFailure(now); + logger.warn( + { + clientIp: logIp, + method: sanitizeForLog(method), + path: sanitizeForLog(path), + code: validation.code, + at: validation.detail, + }, + `Rejected API gateway request: ${validation.reason}`, + ); + return { valid: false, reason: validation.reason, code: validation.code }; + } + const timestampValue = String(timestampHeader || "").trim(); if (!/^[0-9]+$/.test(timestampValue)) { recordApiGatewaySignatureAttempt(clientIp, false, now); _recordCircuitBreakerFailure(now); - logger.warn({ timestampHeader, clientIp }, "Missing or invalid x-api-timestamp header"); + logger.warn( + { timestampHeader: sanitizeForLog(timestampHeader), clientIp: logIp }, + "Missing or invalid x-api-timestamp header", + ); return { valid: false, reason: "Missing or invalid x-api-timestamp header" }; } const timestamp = Number(timestampValue); if (!Number.isSafeInteger(timestamp)) { recordApiGatewaySignatureAttempt(clientIp, false, now); _recordCircuitBreakerFailure(now); - logger.warn({ clientIp }, "API gateway timestamp exceeds safe integer range"); + logger.warn({ clientIp: logIp }, "API gateway timestamp exceeds safe integer range"); return { valid: false, reason: "Missing or invalid x-api-timestamp header" }; } @@ -479,7 +528,7 @@ export function verifyApiGatewayRequestSignature({ if (deltaSeconds > toleranceSeconds) { recordApiGatewaySignatureAttempt(clientIp, false, now); _recordCircuitBreakerFailure(now); - logger.warn({ deltaSeconds, toleranceSeconds, clientIp }, "Request signature timestamp outside accepted window"); + logger.warn({ deltaSeconds, toleranceSeconds, clientIp: logIp }, "Request signature timestamp outside accepted window"); return { valid: false, reason: "Request signature timestamp is outside the accepted window" }; } @@ -487,7 +536,10 @@ export function verifyApiGatewayRequestSignature({ if (!receivedSignature) { recordApiGatewaySignatureAttempt(clientIp, false, now); _recordCircuitBreakerFailure(now); - logger.warn({ signatureHeader, clientIp }, "Missing or invalid x-api-signature header"); + logger.warn( + { signatureHeader: sanitizeForLog(signatureHeader), clientIp: logIp }, + "Missing or invalid x-api-signature header", + ); return { valid: false, reason: "Missing or invalid x-api-signature header" }; } @@ -502,7 +554,7 @@ export function verifyApiGatewayRequestSignature({ if (!expected || !signaturesEqual(receivedSignature, expected)) { recordApiGatewaySignatureAttempt(clientIp, false, now); _recordCircuitBreakerFailure(now); - logger.warn({ clientIp, method, path }, "Request signature verification failed"); + logger.warn({ clientIp: logIp, method: sanitizeForLog(method), path: sanitizeForLog(path) }, "Request signature verification failed"); return { valid: false, reason: "Request signature verification failed" }; } @@ -515,7 +567,7 @@ export function verifyApiGatewayRequestSignature({ recordApiGatewaySignatureAttempt(clientIp, false, now); _recordCircuitBreakerFailure(now); apiGatewayReplayBlockedTotal.inc(); - logger.warn({ clientIp, method, path }, "Rejected replayed API gateway signature"); + logger.warn({ clientIp: logIp, method: sanitizeForLog(method), path: sanitizeForLog(path) }, "Rejected replayed API gateway signature"); return { valid: false, reason: "Signature has already been used and cannot be replayed", @@ -532,7 +584,7 @@ export function verifyApiGatewayRequestSignature({ } catch (err) { // Error recovery #900: Catch unexpected errors and fail gracefully _recordCircuitBreakerFailure(now); - logger.error({ err, clientIp }, "Unexpected error during API gateway signature verification"); + logger.error({ err, clientIp: logIp }, "Unexpected error during API gateway signature verification"); return { valid: false, reason: "Signature verification encountered an unexpected error", diff --git a/backend/src/lib/api-gateway-signature.test.js b/backend/src/lib/api-gateway-signature.test.js index e12cc1a4..17a34639 100644 --- a/backend/src/lib/api-gateway-signature.test.js +++ b/backend/src/lib/api-gateway-signature.test.js @@ -577,3 +577,144 @@ describe("api-gateway-signature", () => { }); }); }); + +// ── Payload sanitization & strict validation ──────────────────────────────── + +describe("api-gateway-signature strict validation", () => { + const timestamp = 1713916800; + const now = timestamp * 1000; + + function signedParams(overrides = {}) { + const base = { method: "POST", path: "/api/payments", body: { amount: 1 }, ...overrides }; + const signature = signApiGatewayRequest({ secret: VALID_SECRET, timestamp, ...base }); + return { + secret: VALID_SECRET, + ...base, + timestampHeader: String(timestamp), + signatureHeader: `sha256=${signature ?? "a".repeat(64)}`, + now, + }; + } + + beforeEach(() => { + _resetApiGatewayRateLimitStateForTests(); + vi.unstubAllEnvs(); + }); + + it("still verifies a normal signed request", () => { + expect(verifyApiGatewayRequestSignature(signedParams()).valid).toBe(true); + }); + + it("rejects a path containing a newline (canonical payload injection)", () => { + const result = verifyApiGatewayRequestSignature( + signedParams({ path: "/api/payments\nPOST" }), + ); + expect(result).toMatchObject({ valid: false, code: "API_GATEWAY_INVALID_REQUEST" }); + }); + + it("rejects an unsupported HTTP method", () => { + const result = verifyApiGatewayRequestSignature({ ...signedParams(), method: "CONNECT" }); + expect(result).toMatchObject({ valid: false, code: "API_GATEWAY_INVALID_REQUEST" }); + }); + + it("rejects a body carrying a __proto__ key even when the signature is valid", () => { + const body = JSON.parse('{"amount":1,"__proto__":{"isAdmin":true}}'); + // Sign the canonical form directly so the only problem is the body shape. + const signature = signApiGatewayRequest({ + secret: VALID_SECRET, method: "POST", path: "/api/payments", timestamp, body: { amount: 1 }, + }); + const result = verifyApiGatewayRequestSignature({ + ...signedParams(), + body, + signatureHeader: `sha256=${signature}`, + }); + expect(result).toMatchObject({ valid: false, code: "API_GATEWAY_INVALID_PAYLOAD" }); + }); + + it("rejects NaN so it cannot share a signature with null", () => { + const signatureForNull = signApiGatewayRequest({ + secret: VALID_SECRET, method: "POST", path: "/api/payments", timestamp, body: { amount: null }, + }); + const result = verifyApiGatewayRequestSignature({ + ...signedParams(), + body: { amount: NaN }, + signatureHeader: `sha256=${signatureForNull}`, + }); + expect(result).toMatchObject({ valid: false, code: "API_GATEWAY_INVALID_PAYLOAD" }); + }); + + it("refuses to sign a body that cannot be serialized instead of signing it as empty", () => { + const circular = { a: 1 }; + circular.self = circular; + expect( + signApiGatewayRequest({ secret: VALID_SECRET, method: "POST", path: "/x", timestamp, body: circular }), + ).toBeNull(); + expect( + signApiGatewayRequest({ secret: VALID_SECRET, method: "POST", path: "/x", timestamp, body: { n: BigInt(1) } }), + ).toBeNull(); + }); + + it("rejects non-string secrets", () => { + const result = verifyApiGatewayRequestSignature({ ...signedParams(), secret: ["x".repeat(32)] }); + expect(result.valid).toBe(false); + expect(result.reason).toMatch(/secret/); + }); + + it("does not disable the timestamp window when the tolerance env var is invalid", () => { + vi.stubEnv("API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS", "not-a-number"); + const stale = signedParams(); + // One day after signing: must be outside the (default 300s) window. + const result = verifyApiGatewayRequestSignature({ ...stale, now: now + 86400 * 1000 }); + expect(result).toMatchObject({ valid: false, reason: expect.stringMatching(/outside the accepted window/) }); + }); + + it("caps an explicitly huge tolerance", () => { + const result = verifyApiGatewayRequestSignature({ + ...signedParams(), + now: now + 2 * 3600 * 1000, + toleranceSeconds: 10 * 24 * 3600, + }); + expect(result.valid).toBe(false); + }); + + it("sanitizes attacker-controlled header values before logging", async () => { + const { logger } = await import("./logger.js"); + logger.warn.mockClear(); + + verifyApiGatewayRequestSignature({ + ...signedParams(), + signatureHeader: "sha256=zz\r\nFAKE LOG LINE", + clientIp: "1.2.3.4\nforged", + }); + + const [fields] = logger.warn.mock.calls.at(-1); + expect(fields.signatureHeader).not.toMatch(/[\r\n]/); + expect(fields.clientIp).toBe("1.2.3.4?forged"); + }); + + it("only honours the current and previous key during rotation", () => { + const thirdKey = "third-key-should-be-ignored-32ch"; + const signature = signApiGatewayRequest({ + secret: thirdKey, method: "POST", path: "/api/payments", timestamp, body: {}, + }); + const result = verifyApiGatewayRequestSignatureWithRotation({ + secrets: ["current-secret-at-least-32-chars", "previous-secret-also-32-chars-ok", thirdKey], + method: "POST", + path: "/api/payments", + timestampHeader: String(timestamp), + signatureHeader: `sha256=${signature}`, + body: {}, + now, + }); + expect(result.valid).toBe(false); + }); + + it("skips non-string entries in the rotation key list", () => { + const params = signedParams({ body: {} }); + const result = verifyApiGatewayRequestSignatureWithRotation({ + ...params, + secrets: [null, VALID_SECRET], + }); + expect(result).toEqual({ valid: true, keyIndex: 1 }); + }); +}); diff --git a/backend/src/lib/auth.js b/backend/src/lib/auth.js index 333bc750..052ad92e 100644 --- a/backend/src/lib/auth.js +++ b/backend/src/lib/auth.js @@ -4,6 +4,7 @@ import { reserveApiGatewaySignature, verifyApiGatewayRequestSignature, } from "./api-gateway-signature.js"; +import { resolveSignatureToleranceSeconds } from "./api-gateway-payload-validation.js"; import { queryWithRetry } from "./db.js"; const SALT_ROUNDS = 12; @@ -143,9 +144,7 @@ export function createApiKeyAuth({ secret: apiKey, signatureHeader, method: req.method, - toleranceSeconds: Number( - process.env.API_GATEWAY_SIGNATURE_TOLERANCE_SECONDS || 300, - ), + toleranceSeconds: resolveSignatureToleranceSeconds(), }); if (!reservation.reserved) { return res.status(reservation.replay ? 401 : 503).json({