diff --git a/README.md b/README.md index ac8013d..925be9a 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,7 @@ rounded away. - `POST /api/transfers` — create a transfer. Body: `{ senderName, recipientName, amount, from, to }` + Requires an `Idempotency-Key` header (see below). - `GET /api/transfers` — list transfers. Supports `?status=`, `?q=` (name search), `?archived=` (true/false/all), and `?limit=`/`?offset=` pagination. Archived transfers are excluded from results by default. @@ -169,6 +170,34 @@ rounded away. - `POST /api/transfers/:id/archive` — archive a transfer, hiding it from default list results. - `POST /api/transfers/:id/unarchive` — unarchive a transfer, restoring it to default list results. +#### Idempotency + +`POST /api/transfers` requires an `Idempotency-Key` header. The endpoint moves +money, so a client that omits the header is not opting out of protection, it is +unaware it needs it; the request is rejected with 400 rather than risking a +duplicate. + +For a given (API token, key) pair the operation runs at most once: + +- **Retry with the same payload** replays the stored transfer, answering 201 + with the original body. The quote is not recomputed, so a moved rate cannot + change the answer, and no second payment is submitted. +- **Reuse with a different payload** answers 409. The fingerprint covers the + fields that determine the transfer, so property order, an amount sent as a + string, and unrelated extra fields all still count as the same request. +- **A retry arriving while the first is still in flight** answers 409 rather + than starting a second settlement. +- **A provider failure releases the key**, so retrying with the same key can + succeed once the provider recovers. + +Keys are scoped to the API token: two callers using the same key get two +independent transfers, and neither can reach the other's. + +Records live in the same store as the transfers, so with the in-memory store +that backs this demo a restart clears both together. That keeps them +consistent: a surviving reservation would replay a transfer that no longer +exists. + ### Users - `GET /api/users` — list users. diff --git a/src/controllers/transferController.js b/src/controllers/transferController.js index 5815bf7..e1ef2f4 100644 --- a/src/controllers/transferController.js +++ b/src/controllers/transferController.js @@ -1,8 +1,41 @@ 'use strict'; const transferService = require('../services/transferService'); +const idempotencyService = require('../services/idempotencyService'); +const ApiError = require('../utils/ApiError'); const { parsePagination } = require('../utils/pagination'); +/** Upper bound on a client-supplied key, so the map cannot be grown without limit. */ +const MAX_IDEMPOTENCY_KEY_LENGTH = 255; + +/** + * Read and validate the Idempotency-Key header. + * + * Required rather than optional: this endpoint moves money, and a client that + * omits the header is not opting out of protection, it is unaware it needs it. + * Failing the request is the only outcome that cannot silently duplicate a + * transfer. + * + * @param {import('express').Request} req + * @returns {string} + * @throws {ApiError} 400 when the header is missing or unusable. + */ +function requireIdempotencyKey(req) { + const raw = req.get('Idempotency-Key'); + if (typeof raw !== 'string' || raw.trim() === '') { + throw ApiError.badRequest( + 'Idempotency-Key header is required to create a transfer' + ); + } + const key = raw.trim(); + if (key.length > MAX_IDEMPOTENCY_KEY_LENGTH) { + throw ApiError.badRequest( + `Idempotency-Key must be at most ${MAX_IDEMPOTENCY_KEY_LENGTH} characters` + ); + } + return key; +} + /** * Transfer controllers. */ @@ -12,7 +45,31 @@ const { parsePagination } = require('../utils/pagination'); * Create a new transfer. */ function createTransfer(req, res) { - const transfer = transferService.createTransfer(req.body, req.id); + const key = requireIdempotencyKey(req); + + // The fingerprint covers the fields that determine the operation, not the raw + // body: an unrelated extra property must not read as a conflicting retry. + // `amount` is normalized because "100" and 100 both validate and produce the + // same transfer, so treating them as different requests would reject a + // legitimate retry from a client that re-serialized its payload. + const fingerprint = idempotencyService.fingerprint({ + senderName: req.body.senderName, + recipientName: req.body.recipientName, + amount: Number(req.body.amount), + from: req.body.from, + to: req.body.to, + }); + + const transfer = transferService.createTransfer(req.body, req.id, { + actor: req.token, + key, + fingerprint, + }); + + // A replay answers 201 with the original transfer, exactly as the first call + // did. Replaying the stored result means replaying all of it; downgrading the + // status would make a successful retry look different from the response it is + // standing in for. res.status(201).json(transfer); } diff --git a/src/services/idempotencyService.js b/src/services/idempotencyService.js new file mode 100755 index 0000000..40b50a6 Binary files /dev/null and b/src/services/idempotencyService.js differ diff --git a/src/services/transferService.js b/src/services/transferService.js index 11554ea..48f7fab 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -6,6 +6,7 @@ const ApiError = require('../utils/ApiError'); const { TRANSFER_STATUS, TRANSFER_TRANSITIONS } = require('../config/constants'); const quoteService = require('./quoteService'); const stellarService = require('./stellarService'); +const idempotencyService = require('./idempotencyService'); const auditService = require('./auditService'); // Keep lifecycle timestamps strictly increasing even when multiple operations @@ -106,11 +107,58 @@ function getTransferOrThrow(id) { /** * Create a new transfer using a freshly computed quote. + * + * When `idempotency` is supplied the whole operation is exactly-once for that + * (actor, key) pair: the key is reserved before the provider is called, so a + * retry arriving mid-flight cannot start a second settlement, and once the + * transfer exists the stored result is replayed instead of re-running. + * + * The context is optional because idempotency is actor-scoped and an actor only + * exists at the HTTP boundary; internal callers have no token to scope to. The + * route requires the header, so every request-driven creation is covered. + * * @param {object} data * @param {string} [requestId] - optional correlation id for audit logging + * @param {{ actor: string, key: string, fingerprint: string }} [idempotency] * @returns {object} */ -function createTransfer(data, requestId) { +function createTransfer(data, requestId, idempotency) { + if (idempotency) { + const outcome = idempotencyService.begin( + store.idempotency, + idempotency.actor, + idempotency.key, + idempotency.fingerprint + ); + // A completed record short-circuits before the quote is recomputed. Rates + // move, so recomputing would hand the client a different transfer under the + // same key, which is the duplicate this is meant to prevent. + if (outcome.status === 'replay') { + return outcome.result; + } + } + + try { + return createTransferUnchecked(data, requestId, idempotency); + } catch (err) { + // The operation never reached a terminal state, so the key must not stay + // burned: the client's correct response to a provider failure is to retry + // with the same key, and that has to be able to succeed. + if (idempotency) { + idempotencyService.release(store.idempotency, idempotency.actor, idempotency.key); + } + throw err; + } +} + +/** + * Perform the creation itself, with the reservation already held. + * @param {object} data + * @param {string} [requestId] + * @param {{ actor: string, key: string }} [idempotency] + * @returns {object} + */ +function createTransferUnchecked(data, requestId, idempotency) { const quote = quoteService.getQuote(data.amount, data.from, data.to); const settlement = stellarService.submitPayment({ amount: quote.sendAmount, @@ -150,6 +198,15 @@ function createTransfer(data, requestId) { requestId, }); + if (idempotency) { + idempotencyService.complete( + store.idempotency, + idempotency.actor, + idempotency.key, + transfer + ); + } + return transfer; } diff --git a/src/store/index.js b/src/store/index.js index b70fbcd..c018112 100644 --- a/src/store/index.js +++ b/src/store/index.js @@ -10,12 +10,17 @@ const auditService = require('../services/auditService'); const store = { users: new Map(), transfers: new Map(), + // Keyed by " ". Lives here rather than in a module + // local so it shares the transfers' lifetime: a replay can never outlive the + // transfer it would replay. + idempotency: new Map(), }; /** Remove all records from the store. Primarily used in tests/seeding. */ function reset() { store.users.clear(); store.transfers.clear(); + store.idempotency.clear(); auditService.reset(); } diff --git a/test/moneyPrecision.test.js b/test/moneyPrecision.test.js index 2814992..caccb6a 100644 --- a/test/moneyPrecision.test.js +++ b/test/moneyPrecision.test.js @@ -67,6 +67,7 @@ test('POST /api/transfers rejects an amount with sub-cent precision', async () = headers: { Authorization: 'Bearer test-token-admin', 'Content-Type': 'application/json', + 'Idempotency-Key': 'idem-precision-1', }, body: JSON.stringify({ senderName: 'Alice', @@ -88,6 +89,7 @@ test('POST /api/transfers accepts a well-formed two-decimal amount', async () => headers: { Authorization: 'Bearer test-token-admin', 'Content-Type': 'application/json', + 'Idempotency-Key': 'idem-precision-2', }, body: JSON.stringify({ senderName: 'Alice', diff --git a/test/requireScope.test.js b/test/requireScope.test.js index 41868fc..33b723d 100644 --- a/test/requireScope.test.js +++ b/test/requireScope.test.js @@ -116,7 +116,7 @@ test('GET /api/users returns 401 when token is unknown', async () => { test('POST /api/transfers returns 401 when token is unknown', async () => { const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('bad-token'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('bad-token'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-118' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 401); @@ -129,7 +129,7 @@ test('POST /api/transfers returns 403 when token only has transfers:read scope', // test-token-readonly has: transfers:read, users:read, audit:read — no :write scopes const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-readonly'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-readonly'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-131' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 403); @@ -153,7 +153,7 @@ test('POST /api/transfers/:id/claim returns 403 when token only has transfers:re // Create a transfer first using the admin token, then try to claim with read-only const createRes = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-155' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(createRes.status, 201); @@ -226,7 +226,7 @@ test('GET /api/transfers returns 200 with readonly token', async () => { test('POST /api/transfers returns 201 with admin token', async () => { const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-228' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 201); @@ -236,7 +236,7 @@ test('POST /api/transfers returns 201 with admin token', async () => { test('POST /api/transfers returns 201 with transfers-scoped token', async () => { const { status, body } = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-238' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }), }); assert.equal(status, 201); @@ -291,7 +291,7 @@ test('full transfer lifecycle: create → claim with correct scopes', async () = // Create const createRes = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-admin'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-293' }, body: JSON.stringify({ senderName: 'Alice', recipientName: 'Bob', amount: 200, from: 'USD', to: 'INR' }), }); assert.equal(createRes.status, 201); @@ -317,7 +317,7 @@ test('full transfer lifecycle: create → claim with correct scopes', async () = test('full transfer lifecycle: create → cancel with correct scopes', async () => { const createRes = await fetchJson('/api/transfers', { method: 'POST', - headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json', 'Idempotency-Key': 'idem-requireScope-319' }, body: JSON.stringify({ senderName: 'Carlos', recipientName: 'Diaz', amount: 500, from: 'EUR', to: 'MXN' }), }); assert.equal(createRes.status, 201); diff --git a/test/transferIdempotency.test.js b/test/transferIdempotency.test.js new file mode 100755 index 0000000..209051a --- /dev/null +++ b/test/transferIdempotency.test.js @@ -0,0 +1,345 @@ +'use strict'; + +const { test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); + +const { store, reset } = require('../src/store'); +const transferService = require('../src/services/transferService'); +const idempotencyService = require('../src/services/idempotencyService'); +const stellarService = require('../src/services/stellarService'); +const auditService = require('../src/services/auditService'); +const ApiError = require('../src/utils/ApiError'); + +const ACTOR = 'test-token-admin'; +const OTHER_ACTOR = 'test-token-write'; + +const PAYLOAD = { + senderName: 'Alice', + recipientName: 'Bob', + amount: 100, + from: 'USD', + to: 'EUR', +}; + +/** Build the idempotency context the controller would pass. */ +function ctx(key, payload = PAYLOAD, actor = ACTOR) { + return { actor, key, fingerprint: idempotencyService.fingerprint(payload) }; +} + +/** Count how many times the provider was asked to move money. */ +let providerCalls; +const realSubmitPayment = stellarService.submitPayment; + +beforeEach(() => { + reset(); + providerCalls = 0; + stellarService.submitPayment = (...args) => { + providerCalls += 1; + return realSubmitPayment(...args); + }; +}); + +afterEach(() => { + stellarService.submitPayment = realSubmitPayment; +}); + +/** Audit entries recorded for transfer creation. */ +function creationAudits() { + return auditService.getEntries().filter((e) => e.action === 'transfer.created'); +} + +// ============================================================================ +// The original failure mode +// ============================================================================ + +test('a retry returns the original transfer instead of creating a second one', () => { + const first = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-retry')); + const second = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-retry')); + + assert.equal(second.id, first.id); + assert.deepEqual(second, first); + assert.equal(store.transfers.size, 1); +}); + +test('a retry produces exactly one provider command and one audit record', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-once')); + transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-once')); + transferService.createTransfer(PAYLOAD, 'req-3', ctx('k-once')); + + // This is the assertion that would have failed before the fix: the provider + // was called on every attempt, so three retries moved money three times. + assert.equal(providerCalls, 1); + assert.equal(creationAudits().length, 1); +}); + +test('a replay does not recompute the quote, so a moved rate cannot change the answer', () => { + const first = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-rate')); + const rateAtCreation = first.rate; + + const replay = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-rate')); + + assert.equal(replay.rate, rateAtCreation); + assert.equal(replay.sendAmount, first.sendAmount); + assert.equal(replay.receiveAmount, first.receiveAmount); +}); + +// ============================================================================ +// Conflicting reuse +// ============================================================================ + +test('reusing a key with a different payload fails with 409 and moves no money', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-conflict')); + const callsAfterFirst = providerCalls; + + assert.throws( + () => + transferService.createTransfer( + { ...PAYLOAD, amount: 250 }, + 'req-2', + ctx('k-conflict', { ...PAYLOAD, amount: 250 }) + ), + (err) => err instanceof ApiError && err.statusCode === 409 + ); + + assert.equal(providerCalls, callsAfterFirst); + assert.equal(store.transfers.size, 1); +}); + +test('the conflict message names the header, so the client knows what to change', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-msg')); + + assert.throws( + () => + transferService.createTransfer( + { ...PAYLOAD, recipientName: 'Carol' }, + 'req-2', + ctx('k-msg', { ...PAYLOAD, recipientName: 'Carol' }) + ), + /Idempotency-Key was already used with a different request payload/ + ); +}); + +// ============================================================================ +// Canonical fingerprint +// ============================================================================ + +test('property order does not make a legitimate retry look like a conflict', () => { + const reordered = { + to: PAYLOAD.to, + amount: PAYLOAD.amount, + senderName: PAYLOAD.senderName, + from: PAYLOAD.from, + recipientName: PAYLOAD.recipientName, + }; + + const first = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-order')); + const retry = transferService.createTransfer(reordered, 'req-2', ctx('k-order', reordered)); + + assert.equal(retry.id, first.id); + assert.equal(providerCalls, 1); +}); + +test('two different keys with an identical payload are two deliberate transfers', () => { + const a = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-a')); + const b = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-b')); + + assert.notEqual(a.id, b.id); + assert.equal(store.transfers.size, 2); + assert.equal(providerCalls, 2); +}); + +// ============================================================================ +// Actor scoping +// ============================================================================ + +test('the same key from a different actor is a separate operation', () => { + const mine = transferService.createTransfer(PAYLOAD, 'req-1', ctx('shared-key')); + const theirs = transferService.createTransfer( + PAYLOAD, + 'req-2', + ctx('shared-key', PAYLOAD, OTHER_ACTOR) + ); + + // Keys are chosen by clients. Without actor scoping one caller could collide + // with another's key and be handed back a transfer that is not theirs. + assert.notEqual(mine.id, theirs.id); + assert.equal(store.transfers.size, 2); +}); + +test('one actor cannot read another actor transfer by guessing the key', () => { + const theirs = transferService.createTransfer( + PAYLOAD, + 'req-1', + ctx('guessable', PAYLOAD, OTHER_ACTOR) + ); + const mine = transferService.createTransfer(PAYLOAD, 'req-2', ctx('guessable')); + + assert.notEqual(mine.id, theirs.id); +}); + +// ============================================================================ +// Concurrency +// ============================================================================ + +test('a second request arriving while the first is in flight is rejected, not duplicated', () => { + // The service is synchronous, so two requests cannot interleave on their own. + // Re-entering from inside the provider call reproduces the exact window the + // original bug lived in: the first operation has started and has not yet + // recorded a result. Driving it this way tests the reservation rather than + // simulating one. + let reentrantError = null; + stellarService.submitPayment = (...args) => { + providerCalls += 1; + if (reentrantError === null) { + try { + transferService.createTransfer(PAYLOAD, 'req-concurrent', ctx('k-race')); + reentrantError = false; + } catch (err) { + reentrantError = err; + } + } + return realSubmitPayment(...args); + }; + + const transfer = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-race')); + + assert.ok(reentrantError instanceof ApiError, 'the concurrent attempt should have been refused'); + assert.equal(reentrantError.statusCode, 409); + assert.match(reentrantError.message, /still in progress/i); + assert.equal(store.transfers.size, 1); + assert.equal(transfer.id, [...store.transfers.keys()][0]); + assert.equal(providerCalls, 1); +}); + +test('a concurrent attempt with a conflicting payload reports the conflict, not the race', () => { + // Order matters here: reporting "still in progress" for what is really a + // client bug sends them into a retry loop that can never succeed. + let seen = null; + stellarService.submitPayment = (...args) => { + providerCalls += 1; + if (seen === null) { + const other = { ...PAYLOAD, amount: 999 }; + try { + transferService.createTransfer(other, 'req-x', ctx('k-race2', other)); + seen = false; + } catch (err) { + seen = err; + } + } + return realSubmitPayment(...args); + }; + + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-race2')); + + assert.ok(seen instanceof ApiError); + assert.match(seen.message, /different request payload/i); +}); + +// ============================================================================ +// Provider failure and retry +// ============================================================================ + +test('a provider failure releases the key so the client retry can still succeed', () => { + stellarService.submitPayment = () => { + providerCalls += 1; + throw new Error('stellar horizon timed out'); + }; + + assert.throws( + () => transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-fail')), + /stellar horizon timed out/ + ); + assert.equal(store.transfers.size, 0); + + // Burning the key on failure would be worse than the duplicate it prevents: + // the client retries correctly, with the same key, and can never win. + stellarService.submitPayment = (...args) => { + providerCalls += 1; + return realSubmitPayment(...args); + }; + + const recovered = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-fail')); + assert.ok(recovered.id); + assert.equal(store.transfers.size, 1); + assert.equal(creationAudits().length, 1); +}); + +test('a failed attempt leaves no reservation behind', () => { + stellarService.submitPayment = () => { + throw new Error('provider down'); + }; + + assert.throws(() => transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-clean'))); + assert.equal(store.idempotency.size, 0); +}); + +test('a completed key survives a later provider outage', () => { + const original = transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-durable')); + + stellarService.submitPayment = () => { + throw new Error('provider down'); + }; + + // The replay must not reach the provider at all, so an outage cannot turn a + // settled transfer into an error for a client that is merely retrying. + const replay = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-durable')); + assert.equal(replay.id, original.id); +}); + +// ============================================================================ +// Restart +// ============================================================================ + +test('reservations and transfers are cleared together on restart', () => { + transferService.createTransfer(PAYLOAD, 'req-1', ctx('k-restart')); + assert.equal(store.idempotency.size, 1); + assert.equal(store.transfers.size, 1); + + reset(); + + // Records live in the same store as the transfers, so their durability is the + // store's durability. Clearing together is the property that matters: a + // surviving reservation would replay a transfer that no longer exists, which + // is worse than losing both. + assert.equal(store.idempotency.size, 0); + assert.equal(store.transfers.size, 0); + + const afterRestart = transferService.createTransfer(PAYLOAD, 'req-2', ctx('k-restart')); + assert.ok(afterRestart.id); + assert.equal(store.transfers.size, 1); +}); + +// ============================================================================ +// Backwards compatibility of the service entry point +// ============================================================================ + +test('a call with no idempotency context still creates a transfer', () => { + // Idempotency is actor-scoped and internal callers have no actor. The HTTP + // route requires the header, so every request-driven creation is covered. + const transfer = transferService.createTransfer(PAYLOAD, 'req-1'); + assert.ok(transfer.id); + assert.equal(store.transfers.size, 1); + assert.equal(store.idempotency.size, 0); +}); + +// ============================================================================ +// Fingerprint helper +// ============================================================================ + +test('fingerprint is stable across key order and nesting', () => { + const a = idempotencyService.fingerprint({ x: 1, y: { b: 2, a: 3 }, z: [1, 2] }); + const b = idempotencyService.fingerprint({ z: [1, 2], y: { a: 3, b: 2 }, x: 1 }); + assert.equal(a, b); +}); + +test('fingerprint distinguishes array order, which is meaningful', () => { + const a = idempotencyService.fingerprint({ items: [1, 2] }); + const b = idempotencyService.fingerprint({ items: [2, 1] }); + assert.notEqual(a, b); +}); + +test('fingerprint treats undefined and null alike so an omitted field is stable', () => { + const a = idempotencyService.fingerprint({ a: 1, b: undefined }); + const b = idempotencyService.fingerprint({ a: 1, b: null }); + assert.equal(a, b); +}); diff --git a/test/transferIdempotencyHttp.test.js b/test/transferIdempotencyHttp.test.js new file mode 100755 index 0000000..eebaccd --- /dev/null +++ b/test/transferIdempotencyHttp.test.js @@ -0,0 +1,149 @@ +'use strict'; + +const { test, before, after, beforeEach } = require('node:test'); +const assert = require('node:assert/strict'); + +// Set NODE_ENV before requiring the app so config reads the right value at +// require-time, matching the convention in smoke.test.js. +process.env.NODE_ENV = 'test'; + +const createApp = require('../src/app'); +const { store, reset } = require('../src/store'); + +let server; +let baseUrl; + +before(() => { + const app = createApp(); + return new Promise((resolve) => { + server = app.listen(0, () => { + baseUrl = `http://127.0.0.1:${server.address().port}`; + resolve(); + }); + }); +}); + +after(() => { + if (server) { + server.close(); + } +}); + +beforeEach(() => { + reset(); +}); + +const BODY = { + senderName: 'Alice', + recipientName: 'Bob', + amount: 100, + from: 'USD', + to: 'EUR', +}; + +/** + * POST a transfer, optionally with an Idempotency-Key. + * @param {string|null} key + * @param {object} [body] + * @returns {Promise<{status: number, body: object}>} + */ +async function post(key, body = BODY) { + const headers = { + Authorization: 'Bearer test-token-admin', + 'Content-Type': 'application/json', + }; + if (key !== null) { + headers['Idempotency-Key'] = key; + } + const res = await fetch(`${baseUrl}/api/transfers`, { + method: 'POST', + headers, + body: JSON.stringify(body), + }); + return { status: res.status, body: await res.json() }; +} + +test('POST /api/transfers refuses a request with no Idempotency-Key', async () => { + const { status, body } = await post(null); + + // Failing is the only outcome that cannot silently duplicate a transfer: a + // client that omits the header is not opting out of protection, it is + // unaware it needs it. + assert.equal(status, 400); + assert.match(body.error.message, /Idempotency-Key header is required/i); + assert.equal(store.transfers.size, 0); +}); + +test('POST /api/transfers refuses a blank Idempotency-Key', async () => { + const { status } = await post(' '); + assert.equal(status, 400); + assert.equal(store.transfers.size, 0); +}); + +test('POST /api/transfers refuses an oversized Idempotency-Key', async () => { + // Keys are client-supplied and land in a map, so the length has to be bounded + // or the store can be grown without limit by a caller that never retries. + const { status, body } = await post('x'.repeat(256)); + assert.equal(status, 400); + assert.match(body.error.message, /at most 255 characters/i); +}); + +test('a retried POST returns the original transfer with the original status', async () => { + const first = await post('http-retry'); + assert.equal(first.status, 201); + + const second = await post('http-retry'); + + // 201 again, not 200: replaying the stored result means replaying all of it, + // so a successful retry is indistinguishable from the response it stands in + // for. + assert.equal(second.status, 201); + assert.equal(second.body.id, first.body.id); + assert.equal(store.transfers.size, 1); +}); + +test('a retried POST with a changed amount answers 409', async () => { + await post('http-conflict'); + const { status, body } = await post('http-conflict', { ...BODY, amount: 500 }); + + assert.equal(status, 409); + assert.match(body.error.message, /different request payload/i); + assert.equal(store.transfers.size, 1); +}); + +test('a key is trimmed, so surrounding whitespace does not fork the operation', async () => { + const first = await post('padded-key'); + const second = await post(' padded-key '); + + assert.equal(second.body.id, first.body.id); + assert.equal(store.transfers.size, 1); +}); + +test('an amount sent as a string still replays rather than conflicting', async () => { + // "100" and 100 both validate and produce the same transfer, so treating them + // as different requests would reject a client that re-serialized its payload. + const first = await post('http-coerce', BODY); + const second = await post('http-coerce', { ...BODY, amount: '100' }); + + assert.equal(second.status, 201); + assert.equal(second.body.id, first.body.id); + assert.equal(store.transfers.size, 1); +}); + +test('an unrelated extra field does not read as a conflicting retry', async () => { + const first = await post('http-extra', BODY); + const second = await post('http-extra', { ...BODY, clientNote: 'sent from mobile' }); + + assert.equal(second.status, 201); + assert.equal(second.body.id, first.body.id); +}); + +test('validation still runs before the key is reserved', async () => { + const { status } = await post('http-invalid', { ...BODY, amount: -5 }); + assert.equal(status, 400); + + // A rejected payload must not burn the key, otherwise a client that fixes its + // request and retries with the same key would be locked out of it. + const retry = await post('http-invalid', BODY); + assert.equal(retry.status, 201); +});