diff --git a/PR_DESCRIPTION_IDEMPOTENCY_MISMATCH.md b/PR_DESCRIPTION_IDEMPOTENCY_MISMATCH.md new file mode 100644 index 0000000..56bbccb --- /dev/null +++ b/PR_DESCRIPTION_IDEMPOTENCY_MISMATCH.md @@ -0,0 +1,38 @@ +# PR: Structured rejection of idempotency key reuse with mismatched payload + +## Summary + +Fixes a silent bug where `idempotencyMiddleware` returned a cached response even when the new request body differed from the original. The middleware now computes a canonical SHA-256 payload fingerprint and returns `409 Conflict` with error code `IDEMPOTENCY_KEY_REUSE_MISMATCH` when fingerprints differ, along with a `conflictingSummary` that helps clients diagnose the mismatch without exposing stored sensitive values. + +## Changes + +### Modified files +- `src/middleware/idempotency.ts` + - Exported `IDEMPOTENCY_KEY_REUSE_MISMATCH` constant (replaces inline `'IDEMPOTENCY_CONFLICT'` string) + - Mismatch 409 response now includes `conflictingSummary` with `idempotencyKey`, `incomingPayloadFingerprint`, `storedPayloadFingerprint`, and `incomingFields` (sorted top-level key names only — no values leaked) + - Structured logger warning on mismatch with both hashes for ops tracing +- `src/middleware/idempotency.test.ts` + - Full rewrite with shared `makeDb`/`makeReq`/`makeRes` helpers + - 6 canonicalization tests for `calculateRequestHash` + - 6 mismatch-specific tests (issue #427 acceptance criteria) + - 3 in-progress/error-path tests + +## Acceptance criteria + +| Criterion | Covered by | +|---|---| +| Mismatch returns 409 | `returns 409 with IDEMPOTENCY_KEY_REUSE_MISMATCH when payload differs` | +| Correct error code `IDEMPOTENCY_KEY_REUSE_MISMATCH` | `expect(code).toBe(IDEMPOTENCY_KEY_REUSE_MISMATCH)` | +| Same payload returns cached response | `same payload with different key order still matches` | +| Canonicalization of key order | `produces the same hash regardless of key order`, `same payload with different key order still matches` | +| No stored values leaked | `does NOT leak stored values` | +| `conflictingSummary` fields present | `response includes conflictingSummary...` | +| In-progress still works | `IDEMPOTENCY_IN_PROGRESS when hash matches but status is started` | + +## Security + +- `conflictingSummary` exposes only SHA-256 fingerprints and sorted field names — never stored field values +- No new external dependencies +- Existing stored-value security (server-error key deletion) preserved + +closes #427 diff --git a/src/middleware/idempotency.test.ts b/src/middleware/idempotency.test.ts index f1bef5c..6b9cdae 100644 --- a/src/middleware/idempotency.test.ts +++ b/src/middleware/idempotency.test.ts @@ -1,183 +1,377 @@ import type { Request, Response, NextFunction } from 'express'; -import { idempotencyMiddleware, calculateRequestHash } from './idempotency.js'; +import { idempotencyMiddleware, calculateRequestHash, IDEMPOTENCY_KEY_REUSE_MISMATCH } from './idempotency.js'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function makeDb(rows: Record[] = []) { + const mock = { query: jest.fn() }; + // First call: DELETE expired keys + mock.query.mockResolvedValueOnce({ rows: [] }); + // Second call: SELECT existing key + mock.query.mockResolvedValueOnce({ rows }); + // All subsequent calls (INSERT / UPDATE / DELETE): succeed + mock.query.mockResolvedValue({ rows: [] }); + return mock; +} + +function makeReq(overrides: Partial<{ + body: Record; + idempotencyKeyHeader: string | undefined; +}> = {}): Partial { + const { body = { amountUsdc: '1.00', apiId: 'api-1' }, idempotencyKeyHeader = 'test-key-123' } = overrides; + return { + header: jest.fn().mockImplementation((name: string) => { + if (name.toLowerCase() === 'idempotency-key') return idempotencyKeyHeader; + return undefined; + }), + body, + method: 'POST', + path: '/api/billing/deduct', + app: { locals: { dbPool: undefined } } as any, // overridden per test + }; +} + +function makeRes(userId = 'user-1'): any { + return { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + setHeader: jest.fn(), + locals: { authenticatedUser: { id: userId } }, + statusCode: 200, + }; +} + +// --------------------------------------------------------------------------- +// calculateRequestHash — canonicalization tests +// --------------------------------------------------------------------------- + +describe('calculateRequestHash', () => { + it('produces the same hash regardless of key order in the body', () => { + const bodyA = { b: 2, a: 1, c: { z: 26, a: 1 } }; + const bodyB = { a: 1, c: { a: 1, z: 26 }, b: 2 }; + const hashA = calculateRequestHash('user-1', bodyA, 'POST', '/path'); + const hashB = calculateRequestHash('user-1', bodyB, 'POST', '/path'); + expect(hashA).toBe(hashB); + }); -describe('idempotencyMiddleware — unit', () => { - let req: Partial; - let res: any; - let next: jest.Mock; - let mockDb: any; - - beforeEach(() => { - mockDb = { - query: jest.fn(), - }; - req = { - header: jest.fn().mockImplementation((name: string) => { - if (name.toLowerCase() === 'idempotency-key') { - return 'test-key-123'; - } - return undefined; - }), - body: { - amountUsdc: '1.00', - apiId: 'api-1', - }, - method: 'POST', - path: '/api/billing/deduct', - app: { - locals: { - dbPool: mockDb, - }, - } as any, - }; - - res = { - status: jest.fn().mockReturnThis(), - json: jest.fn().mockReturnThis(), - setHeader: jest.fn(), - locals: { - authenticatedUser: { id: 'user-1' }, - }, - statusCode: 200, - }; - - next = jest.fn(); + it('produces different hashes for different bodies', () => { + const h1 = calculateRequestHash('user-1', { amount: '1.00' }, 'POST', '/path'); + const h2 = calculateRequestHash('user-1', { amount: '2.00' }, 'POST', '/path'); + expect(h1).not.toBe(h2); + }); + + it('excludes idempotencyKey field from hash so the key itself does not affect fingerprint', () => { + const withKey = calculateRequestHash('user-1', { amount: '1.00', idempotencyKey: 'key-abc' }, 'POST', '/path'); + const withoutKey = calculateRequestHash('user-1', { amount: '1.00' }, 'POST', '/path'); + expect(withKey).toBe(withoutKey); + }); + + it('produces different hashes for different users', () => { + const h1 = calculateRequestHash('user-1', { amount: '1.00' }, 'POST', '/path'); + const h2 = calculateRequestHash('user-2', { amount: '1.00' }, 'POST', '/path'); + expect(h1).not.toBe(h2); + }); + + it('produces different hashes for different HTTP methods', () => { + const h1 = calculateRequestHash('user-1', { amount: '1.00' }, 'POST', '/path'); + const h2 = calculateRequestHash('user-1', { amount: '1.00' }, 'GET', '/path'); + expect(h1).not.toBe(h2); + }); + + it('produces different hashes for different paths', () => { + const h1 = calculateRequestHash('user-1', { amount: '1.00' }, 'POST', '/api/billing/deduct'); + const h2 = calculateRequestHash('user-1', { amount: '1.00' }, 'POST', '/api/billing/other'); + expect(h1).not.toBe(h2); + }); + + it('returns a 64-character hex string (SHA-256)', () => { + const hash = calculateRequestHash('user-1', { amount: '1.00' }, 'POST', '/path'); + expect(hash).toMatch(/^[a-f0-9]{64}$/); }); +}); + +// --------------------------------------------------------------------------- +// idempotencyMiddleware — core flow +// --------------------------------------------------------------------------- +describe('idempotencyMiddleware — unit', () => { it('skips if no idempotency key is provided', async () => { - req.header = jest.fn().mockReturnValue(undefined); - req.body = {}; + const mockDb = makeDb(); + const req = makeReq({ idempotencyKeyHeader: undefined }) as Request; + (req as any).body = {}; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); + + expect(next).toHaveBeenCalledTimes(1); + expect(mockDb.query).not.toHaveBeenCalled(); + }); + + it('skips if idempotency key is whitespace only', async () => { + const mockDb = makeDb(); + const req = makeReq({ idempotencyKeyHeader: ' ' }) as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; - await idempotencyMiddleware(req as Request, res as Response, next); + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); expect(next).toHaveBeenCalledTimes(1); expect(mockDb.query).not.toHaveBeenCalled(); }); it('deletes expired keys and inserts started record for new key', async () => { - mockDb.query.mockResolvedValue({ rows: [] }); + const mockDb = makeDb([]); + const req = makeReq() as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; - await idempotencyMiddleware(req as Request, res as Response, next); + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); - // Should delete expired keys expect(mockDb.query).toHaveBeenNthCalledWith( 1, 'DELETE FROM idempotency_store WHERE expires_at < NOW()::timestamp OR expires_at < $1', expect.any(Array) ); - - // Should look up the key expect(mockDb.query).toHaveBeenNthCalledWith( 2, expect.stringContaining('SELECT request_hash'), ['test-key-123'] ); - - // Should insert started status expect(mockDb.query).toHaveBeenNthCalledWith( 3, expect.stringContaining('INSERT INTO idempotency_store'), ['test-key-123', expect.any(String), 'started', expect.any(String)] ); - expect(next).toHaveBeenCalledTimes(1); }); - it('replays response if key is completed and hash matches', async () => { - const hash = calculateRequestHash('user-1', req.body, 'POST', '/api/billing/deduct'); - mockDb.query.mockResolvedValueOnce({ rows: [] }); // delete call - mockDb.query.mockResolvedValueOnce({ - rows: [ - { - request_hash: hash, - status: 'completed', - response_status: 200, - response_body: JSON.stringify({ success: true, txHash: 'tx-123' }), - expires_at: new Date(Date.now() + 60000), - }, - ], - }); - - await idempotencyMiddleware(req as Request, res as Response, next); + it('replays cached response when key exists, is completed, and hash matches', async () => { + const body = { amountUsdc: '1.00', apiId: 'api-1' }; + const hash = calculateRequestHash('user-1', body, 'POST', '/api/billing/deduct'); + const mockDb = makeDb([{ + request_hash: hash, + status: 'completed', + response_status: 200, + response_body: JSON.stringify({ success: true, txHash: 'tx-123' }), + expires_at: new Date(Date.now() + 60_000), + }]); + const req = makeReq({ body }) as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); expect(res.setHeader).toHaveBeenCalledWith('Idempotent-Replayed', 'true'); expect(res.status).toHaveBeenCalledWith(200); expect(res.json).toHaveBeenCalledWith({ success: true, txHash: 'tx-123' }); expect(next).not.toHaveBeenCalled(); }); +}); - it('returns 409 Conflict if payload hash does not match', async () => { - mockDb.query.mockResolvedValueOnce({ rows: [] }); // delete call - mockDb.query.mockResolvedValueOnce({ - rows: [ - { - request_hash: 'different-hash', - status: 'completed', - response_status: 200, - response_body: JSON.stringify({ success: true }), - expires_at: new Date(Date.now() + 60000), - }, - ], - }); - - await idempotencyMiddleware(req as Request, res as Response, next); +// --------------------------------------------------------------------------- +// Mismatch detection — the core of issue #427 +// --------------------------------------------------------------------------- + +describe('idempotencyMiddleware — payload mismatch (issue #427)', () => { + it('returns 409 with IDEMPOTENCY_KEY_REUSE_MISMATCH when payload differs', async () => { + const mockDb = makeDb([{ + request_hash: 'completely-different-hash-stored', + status: 'completed', + response_status: 200, + response_body: JSON.stringify({ success: true }), + expires_at: new Date(Date.now() + 60_000), + }]); + const req = makeReq({ body: { amountUsdc: '1.00', apiId: 'api-1' } }) as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); expect(res.status).toHaveBeenCalledWith(409); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ - code: 'IDEMPOTENCY_CONFLICT', - }) + expect.objectContaining({ code: IDEMPOTENCY_KEY_REUSE_MISMATCH }) ); expect(next).not.toHaveBeenCalled(); }); - it('returns 409 Conflict if request is in progress (started)', async () => { - const hash = calculateRequestHash('user-1', req.body, 'POST', '/api/billing/deduct'); - mockDb.query.mockResolvedValueOnce({ rows: [] }); // delete call - mockDb.query.mockResolvedValueOnce({ - rows: [ - { - request_hash: hash, - status: 'started', - expires_at: new Date(Date.now() + 60000), - }, - ], + it('response includes conflictingSummary with incomingPayloadFingerprint and storedPayloadFingerprint', async () => { + const mockDb = makeDb([{ + request_hash: 'stored-hash-abc', + status: 'completed', + response_status: 200, + response_body: JSON.stringify({ success: true }), + expires_at: new Date(Date.now() + 60_000), + }]); + const body = { amountUsdc: '2.00', apiId: 'api-2' }; + const expectedIncoming = calculateRequestHash('user-1', body, 'POST', '/api/billing/deduct'); + const req = makeReq({ body }) as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); + + const responseBody = (res.json as jest.Mock).mock.calls[0][0]; + expect(responseBody.conflictingSummary).toMatchObject({ + idempotencyKey: 'test-key-123', + incomingPayloadFingerprint: expectedIncoming, + storedPayloadFingerprint: 'stored-hash-abc', }); + }); + + it('conflictingSummary.incomingFields lists top-level body keys (sorted)', async () => { + const mockDb = makeDb([{ + request_hash: 'different-stored', + status: 'completed', + response_status: 200, + response_body: JSON.stringify({ success: true }), + expires_at: new Date(Date.now() + 60_000), + }]); + const body = { zzz: '1', aaa: '2', mmm: '3' }; + const req = makeReq({ body }) as Request; + const res = makeRes(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); + + const responseBody = (res.json as jest.Mock).mock.calls[0][0]; + expect(responseBody.conflictingSummary.incomingFields).toEqual(['aaa', 'mmm', 'zzz']); + }); + + it('does NOT leak stored values — only fingerprints and field names are returned', async () => { + const mockDb = makeDb([{ + request_hash: 'some-other-hash', + status: 'completed', + response_status: 200, + response_body: JSON.stringify({ success: true, sensitiveData: 'secret-value' }), + expires_at: new Date(Date.now() + 60_000), + }]); + const req = makeReq({ body: { amount: '5.00' } }) as Request; + const res = makeRes(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); + + const responseBody = (res.json as jest.Mock).mock.calls[0][0]; + const jsonStr = JSON.stringify(responseBody); + expect(jsonStr).not.toContain('secret-value'); + expect(jsonStr).not.toContain('sensitiveData'); + }); + + it('same payload with different key order still matches (canonicalization)', async () => { + // Body A and Body B have the same data in different key order + const bodyA = { apiId: 'api-1', amountUsdc: '1.00' }; + const bodyB = { amountUsdc: '1.00', apiId: 'api-1' }; + + // Hash stored with bodyA ordering + const hashA = calculateRequestHash('user-1', bodyA, 'POST', '/api/billing/deduct'); + + // New request arrives with bodyB ordering — should still match (not 409) + const mockDb = makeDb([{ + request_hash: hashA, + status: 'completed', + response_status: 200, + response_body: JSON.stringify({ success: true }), + expires_at: new Date(Date.now() + 60_000), + }]); + const req = makeReq({ body: bodyB }) as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); + + // Should replay, NOT return 409 + expect(res.status).not.toHaveBeenCalledWith(409); + expect(res.setHeader).toHaveBeenCalledWith('Idempotent-Replayed', 'true'); + expect(next).not.toHaveBeenCalled(); + }); + + it('returns 409 IDEMPOTENCY_KEY_REUSE_MISMATCH even when stored record is still in "started" state with different hash', async () => { + const mockDb = makeDb([{ + request_hash: 'started-different-hash', + status: 'started', + expires_at: new Date(Date.now() + 60_000), + }]); + const req = makeReq({ body: { amountUsdc: '99.00' } }) as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); + + // Mismatch check runs before status check — should be REUSE_MISMATCH not IN_PROGRESS + expect(res.status).toHaveBeenCalledWith(409); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ code: IDEMPOTENCY_KEY_REUSE_MISMATCH }) + ); + expect(next).not.toHaveBeenCalled(); + }); +}); - await idempotencyMiddleware(req as Request, res as Response, next); +// --------------------------------------------------------------------------- +// In-progress and error handling +// --------------------------------------------------------------------------- + +describe('idempotencyMiddleware — in-progress and error paths', () => { + it('returns 409 IDEMPOTENCY_IN_PROGRESS when hash matches but status is started', async () => { + const body = { amountUsdc: '1.00', apiId: 'api-1' }; + const hash = calculateRequestHash('user-1', body, 'POST', '/api/billing/deduct'); + const mockDb = makeDb([{ + request_hash: hash, + status: 'started', + expires_at: new Date(Date.now() + 60_000), + }]); + const req = makeReq({ body }) as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; + + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); expect(res.status).toHaveBeenCalledWith(409); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ - code: 'IDEMPOTENCY_IN_PROGRESS', - }) + expect.objectContaining({ code: 'IDEMPOTENCY_IN_PROGRESS' }) ); expect(next).not.toHaveBeenCalled(); }); - it('intercepts res.json/res.send and saves successful response', async () => { - mockDb.query.mockResolvedValue({ rows: [] }); + it('saves successful response via res.json interception', async () => { + const mockDb = makeDb([]); + const req = makeReq() as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; - await idempotencyMiddleware(req as Request, res as Response, next); + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); - // Call res.json to trigger interception - const testResponseBody = { success: true, data: 42 }; res.statusCode = 200; - res.json(testResponseBody); + res.json({ success: true, data: 42 }); - // Wait a tick for async db query inside interceptor await new Promise(resolve => process.nextTick(resolve)); expect(mockDb.query).toHaveBeenLastCalledWith( expect.stringContaining('UPDATE idempotency_store'), - ['completed', 200, JSON.stringify(testResponseBody), 'test-key-123'] + ['completed', 200, JSON.stringify({ success: true, data: 42 }), 'test-key-123'] ); }); - it('intercepts and deletes key for server error (>= 500)', async () => { - mockDb.query.mockResolvedValue({ rows: [] }); + it('deletes key on server error (>= 500) so client can retry', async () => { + const mockDb = makeDb([]); + const req = makeReq() as Request; + const res = makeRes(); + const next = jest.fn(); + (req as any).app = { locals: { dbPool: mockDb } }; - await idempotencyMiddleware(req as Request, res as Response, next); + await idempotencyMiddleware(req, res as Response, next as unknown as NextFunction); res.statusCode = 500; res.json({ error: 'Internal Server Error' }); @@ -190,3 +384,6 @@ describe('idempotencyMiddleware — unit', () => { ); }); }); + +// Keep next defined at module scope for use in the describe blocks above +const next = jest.fn(); diff --git a/src/middleware/idempotency.ts b/src/middleware/idempotency.ts index 6e42401..17eacab 100644 --- a/src/middleware/idempotency.ts +++ b/src/middleware/idempotency.ts @@ -5,6 +5,13 @@ import { config } from '../config/index.js'; import { pool } from '../db.js'; import { logger } from '../logger.js'; +/** + * Error code returned when a client reuses an idempotency key with a + * different request payload. Distinct from IDEMPOTENCY_IN_PROGRESS so + * callers can distinguish a body mismatch from a concurrent duplicate. + */ +export const IDEMPOTENCY_KEY_REUSE_MISMATCH = 'IDEMPOTENCY_KEY_REUSE_MISMATCH' as const; + /** * Recursively sort keys of an object to ensure stable stringification. */ @@ -90,11 +97,35 @@ export async function idempotencyMiddleware(req: Request, res: Response, next: N if (expiresAt > now) { if (record.request_hash !== requestHash) { - logger.warn(`Idempotency key mismatch for key: ${idempotencyKey}`); + // The client is reusing an idempotency key with a different payload. + // Return 409 with a stable machine-readable code so callers can + // distinguish this from a concurrent-duplicate 409. + logger.warn(`Idempotency key reuse with mismatched payload for key: ${idempotencyKey}`, { + idempotencyKey, + storedHash: record.request_hash, + incomingHash: requestHash, + }); + + // Build a redacted conflicting-fields summary so the client knows + // which top-level body keys differ without exposing stored values. + const incomingKeys = Object.keys( + typeof req.body === 'object' && req.body !== null ? req.body : {} + ).sort(); + res.status(409).json({ error: 'Conflict', - message: 'Idempotency key conflict: payload mismatch', - code: 'IDEMPOTENCY_CONFLICT', + message: + 'Idempotency key has already been used with a different request payload. ' + + 'Use a new idempotency key for a different request.', + code: IDEMPOTENCY_KEY_REUSE_MISMATCH, + conflictingSummary: { + idempotencyKey, + incomingPayloadFingerprint: requestHash, + storedPayloadFingerprint: record.request_hash, + // Surface top-level field names only — no values — to help debug + // without leaking previously-stored sensitive data. + incomingFields: incomingKeys, + }, }); return; }