diff --git a/app/middleware/idempotency.js b/app/middleware/idempotency.js index 23d25ab..420d86c 100644 --- a/app/middleware/idempotency.js +++ b/app/middleware/idempotency.js @@ -66,16 +66,41 @@ function sha256(s) { * (e.g., `{a:1,b:2}` vs `{b:2,a:1}`) hash to the same value. Without * this, a client that reorders its JSON fields on retry would trip * the body-mismatch 409. + * + * Bounded by MAX_CANONICAL_DEPTH (default 64). Without the bound, a + * deeply-nested body (`{"a":{"a":...5000 levels...}}`) overflows + * Node's call stack with a RangeError at depth ~5000, which Express's + * async error path surfaces as a 500. The 100KB express.json limit + * allows up to ~20000 nesting levels (5 chars per level), so the + * pre-bound version was reliably DoS-able through `POST /v1/* + + * Idempotency-Key: x`. 64 levels is well above any legitimate + * planning-API body's nesting; we throw a tagged Error that the + * middleware catches and returns as a 400. */ -function canonicalJson(value) { +const MAX_CANONICAL_DEPTH = 64; + +class CanonicalJsonDepthError extends Error { + constructor() { + super(`Body exceeds maximum nesting depth (${MAX_CANONICAL_DEPTH}) for Idempotency-Key processing.`); + this.name = 'CanonicalJsonDepthError'; + this.code = 'body_too_deep'; + } +} + +function canonicalJson(value, depth = 0) { + if (depth > MAX_CANONICAL_DEPTH) { + throw new CanonicalJsonDepthError(); + } if (value === null || typeof value !== 'object') { return JSON.stringify(value); } if (Array.isArray(value)) { - return '[' + value.map(canonicalJson).join(',') + ']'; + return '[' + value.map((v) => canonicalJson(v, depth + 1)).join(',') + ']'; } const keys = Object.keys(value).sort(); - return '{' + keys.map(k => JSON.stringify(k) + ':' + canonicalJson(value[k])).join(',') + '}'; + return '{' + keys.map((k) => + JSON.stringify(k) + ':' + canonicalJson(value[k], depth + 1), + ).join(',') + '}'; } function hashBody(body) { @@ -135,7 +160,28 @@ async function idempotency(req, res, next) { } const scope = buildScope(req); - const bodyHash = hashBody(req.body); + let bodyHash; + try { + bodyHash = hashBody(req.body); + } catch (error) { + if (error instanceof CanonicalJsonDepthError) { + // Don't propagate as 500 via the global error path; emit a + // crisp 400 the client can act on. Skips both the cache + // lookup AND the handler — pre-bound, this branch would + // have stack-overflowed at depth ~5000, surfacing as 500. + // + // Message is hardcoded (rather than `error.message`) so we + // match the controller-error-shape policy — middleware + // bodies must never echo a caught error's .message field + // (regression pin in tests/unit/controller-error-shape.test.js). + // The depth limit IS the message text, so a constant works. + return res.status(400).json({ + message: `Body exceeds maximum nesting depth (${MAX_CANONICAL_DEPTH}) for Idempotency-Key processing.`, + code: 'body_too_deep', + }); + } + throw error; + } // Best-effort prune. Fire-and-forget so the request path never // waits on DELETE; errors are swallowed via the .catch(). Cheap @@ -227,6 +273,8 @@ module.exports = { buildScope, KEY_PATTERN, TTL_MS, + MAX_CANONICAL_DEPTH, + CanonicalJsonDepthError, // Test-only seam: pass a stub `{ sequelize: { query: ... }, Sequelize: // { QueryTypes: { SELECT } } }` to drive the cache lookup + write // paths from HTTP tests. Pass null (or no arg) to restore the diff --git a/tests/api/idempotency.test.js b/tests/api/idempotency.test.js index 44ae174..3bef97b 100644 --- a/tests/api/idempotency.test.js +++ b/tests/api/idempotency.test.js @@ -170,3 +170,57 @@ describe('Idempotency middleware: mounted on POST routes', () => { expect(res.status).not.toBe(400); }); }); + + +describe('Idempotency middleware: bounded canonicalJson (DoS defense)', () => { + test('deeply nested body returns 400 — NOT 500', async () => { + // Construct a 5000-deep payload AS A RAW STRING. JSON.stringify + // itself recurses and overflows on deeply nested objects, so + // .send(nestedObject) can't even build the payload — supertest + // would throw client-side. Build the JSON text iteratively + // instead; express.json() parses iteratively (V8 ≥9) so the + // body still reaches req.body intact. + const depth = 5000; + let payload = '0'; + for (let i = 0; i < depth; i++) payload = '{"a":' + payload + '}'; + + const res = await request(app) + .post('/v1/customer') + .set('authKey', 'any') + .set('Idempotency-Key', 'depth-test-001') + .set('Content-Type', 'application/json') + .send(payload); + + // Pre-fix: 500 (RangeError from canonicalJson surfacing via + // the global error handler). Post-fix: 400. The specific 400 + // body has code:'body_too_deep'; we don't insist on that exact + // shape (Express' own parser or a future express.json depth + // guard could also emit a different 400 body), only that + // we no longer 500. + expect(res.status).not.toBe(500); + expect(res.status).toBe(400); + if (res.body && res.body.code === 'body_too_deep') { + expect(res.body.message).toMatch(/nesting depth/i); + } + }); + + test('shallow body with Idempotency-Key still proceeds normally', async () => { + // Positive control: a normal body (any depth below the cap) + // does not 400. Whatever status the controller returns + // (likely 403 because of the empty ApiKey mock) is fine — + // we just want to prove the depth check didn't trip. + const res = await request(app) + .post('/v1/customer') + .set('authKey', 'any') + .set('Idempotency-Key', 'shallow-test-001') + .send({ + custCompanyName: 'Acme', + custFName: 'Test', + custLName: 'User', + }); + expect(res.status).not.toBe(400); + // Don't assert on body.code — the controller emits whatever + // it emits; we only care the depth check stayed silent. + expect(res.body && res.body.code).not.toBe('body_too_deep'); + }); +}); diff --git a/tests/unit/idempotency.test.js b/tests/unit/idempotency.test.js index f1fa0c4..8d6d711 100644 Binary files a/tests/unit/idempotency.test.js and b/tests/unit/idempotency.test.js differ