From 580109be1df6378a7322c3cfc15cec41e4ddc116 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Wed, 20 May 2026 18:51:41 -0500 Subject: [PATCH] security(MEDIUM): bound canonicalJson recursion in idempotency middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `canonicalJson()` recurses once per nesting level. A POST that includes an Idempotency-Key header reaches `hashBody(req.body) -> canonicalJson(...)` synchronously inside the middleware. With Node's default ~10000-frame call stack, depth ~5000 blows the stack with RangeError — which Express's async error path surfaces as a 500. Reach: any POST endpoint mounted under /v1 (every CREATE handler and every /bulk handler). Body limit is 100KB (env-tunable), allowing up to ~20000 nesting levels at 5 chars per level (`{"a":`), so a single 25KB payload reliably reproduces the 500. The rate limiter caps abuse at ~100 requests / 15min, so this isn't a sustained-DoS vector — but it IS a reliable cheap 500 trigger, and a caught DoS is better than a swallowed one. Verified with a one-liner before the fix: $ node -e "const {canonicalJson}=require('./app/middleware/idempotency.js'); function n(d){let o=0;for(let i=0;i= 9) so the body reaches req.body intact. 804 tests pass (was 800); lint clean. --- app/middleware/idempotency.js | 56 ++++++++++++++++++++++++++++++--- tests/api/idempotency.test.js | 54 +++++++++++++++++++++++++++++++ tests/unit/idempotency.test.js | Bin 5066 -> 7401 bytes 3 files changed, 106 insertions(+), 4 deletions(-) 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 f1fa0c467ad5234c3801db068f48cb7b762c39ea..8d6d711e201c28bac2f973572d607ee1d7c490fe 100644 GIT binary patch delta 2207 zcmbtW&2HO95C(i}Zb5(^nxdHIA`+GjnLwE0tQ{d)KM;n{Pz^5HriI5xO5l9#(694NF-sxvxGf#KcbIF2D{W~95r z3}=-7G#7PsBV z$6p%dDsd%GoW~fba4}=1O@5uv9 zhC!Db1-g1NUnQ+`c8w|jcivz0Z<|y4oQ}(D*|Dd{lCw5X62#9^3xS>JbG-iU-u?56 z>o+OvyYny(!!T0XMNSWRFhRJ=N@T^v1)DBYnZ(xH_pg7t`}xA!l0Pf`E-U1Zl}|-V!qjycd5~ zwBe{ffD73)+(sEJZ!~7bY-fswD{u?FHp+UV1YR6IU1L^rI^~iv1laTxzD>c&J#dwv zas~gY5)qvUX+}NV$B|T+uF)Bqa&hKR(H2n&_D7q#)j&nN>x6VFM+}}^oIBC%1N{B| zgP-thtf`e#Eftv^o9B|V6v9_l(m8t5J9yE5ec