From 5e9ee086f52125481392b94bf192296de06ccd20 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Thu, 24 Sep 2026 18:27:49 -0400 Subject: [PATCH 1/7] fix(backend): make archive and unarchive timestamps monotonic and auditable Archive/unarchive now append immutable history events with actor and reason, keep lastArchivedAt across unarchive, and reject stale expectedUpdatedAt commands so retries and races cannot overwrite lifecycle order. --- CHANGELOG.md | 7 + src/controllers/transferController.js | 45 ++- src/services/transferService.js | 178 +++++++++++- test/transferArchiveLifecycle.test.js | 386 ++++++++++++++++++++++++++ 4 files changed, 604 insertions(+), 12 deletions(-) create mode 100644 test/transferArchiveLifecycle.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a8e4e0a..4f778e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,13 @@ When preparing a new release: ### Added +- Monotonic, auditable archive/unarchive lifecycle for transfers: each + archive or unarchive appends an immutable `archiveHistory` event with + actor, reason, and a non-decreasing timestamp; `lastArchivedAt` retains + the prior archive instant after unarchive so reconciliation is not lost. + Optional `expectedUpdatedAt` (body or `If-Match`) rejects stale concurrent + commands with `409 STALE_ARCHIVE_COMMAND`. Audit log entries + `transfer.archived` / `transfer.unarchived` carry the same actor/reason. - Cursor pagination for `GET /api/transfers` and `GET /api/audit`. Pass `?cursor=` (with optional `?order=asc|desc`) to page by an indexed position instead of a row offset; responses carry a `pageInfo` block with diff --git a/src/controllers/transferController.js b/src/controllers/transferController.js index 4fb2e72..ae3a952 100644 --- a/src/controllers/transferController.js +++ b/src/controllers/transferController.js @@ -148,21 +148,62 @@ function cancelTransfer(req, res) { res.json(transfer); } +/** + * Read optional optimistic-concurrency token for archive mutations. + * Prefers body.expectedUpdatedAt; falls back to a bare If-Match header value. + * @param {import('express').Request} req + * @returns {string|undefined} + */ +function readExpectedUpdatedAt(req) { + const fromBody = req.body && req.body.expectedUpdatedAt; + if (typeof fromBody === 'string' && fromBody.trim() !== '') { + return fromBody.trim(); + } + const ifMatch = req.get('If-Match'); + if (typeof ifMatch === 'string' && ifMatch.trim() !== '') { + return ifMatch.trim().replace(/^W\//, '').replace(/^"|"$/g, ''); + } + return undefined; +} + +/** + * Shared options for archive / unarchive mutations. + * @param {import('express').Request} req + * @returns {object} + */ +function archiveMutationOptions(req) { + const body = req.body || {}; + return { + requestId: req.id, + actor: req.token || null, + reason: body.reason, + expectedUpdatedAt: readExpectedUpdatedAt(req), + }; +} + /** * POST /api/transfers/:id/archive * Archive a transfer, hiding it from default list results. + * Body may include `reason` and `expectedUpdatedAt` (optimistic concurrency). */ function archiveTransfer(req, res) { - const transfer = transferService.archiveTransfer(req.params.id); + const transfer = transferService.archiveTransfer( + req.params.id, + archiveMutationOptions(req) + ); res.json(transfer); } /** * POST /api/transfers/:id/unarchive * Unarchive a transfer, restoring it to default list results. + * Body may include `reason` and `expectedUpdatedAt` (optimistic concurrency). */ function unarchiveTransfer(req, res) { - const transfer = transferService.unarchiveTransfer(req.params.id); + const transfer = transferService.unarchiveTransfer( + req.params.id, + archiveMutationOptions(req) + ); res.json(transfer); } diff --git a/src/services/transferService.js b/src/services/transferService.js index 1517f62..22a529c 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -264,6 +264,10 @@ function createTransferUnchecked(data, requestId, idempotency) { createdAt: new Date().toISOString(), updatedAt: null, archivedAt: null, + lastArchivedAt: null, + // Append-only archive/unarchive events. Timestamps here are immutable once + // written so repeated archive cycles never erase earlier lifecycle order. + archiveHistory: [], }; transfer.updatedAt = nextTimestamp(transfer.createdAt); @@ -354,35 +358,189 @@ function cancelTransfer(id, requestId) { return transfer; } +/** + * Normalise archive/unarchive options. Accepts either an options object or a + * legacy bare requestId string so existing call sites keep working. + * @param {string|object} [optionsOrRequestId] + * @returns {{ requestId: string|undefined, actor: string|null, reason: string|null, expectedUpdatedAt: string|undefined }} + */ +function normaliseArchiveOptions(optionsOrRequestId) { + if (optionsOrRequestId == null) { + return { requestId: undefined, actor: null, reason: null, expectedUpdatedAt: undefined }; + } + if (typeof optionsOrRequestId === 'string') { + return { + requestId: optionsOrRequestId, + actor: null, + reason: null, + expectedUpdatedAt: undefined, + }; + } + const reason = optionsOrRequestId.reason; + return { + requestId: optionsOrRequestId.requestId, + actor: optionsOrRequestId.actor == null ? null : String(optionsOrRequestId.actor), + reason: reason == null || reason === '' ? null : String(reason), + expectedUpdatedAt: optionsOrRequestId.expectedUpdatedAt, + }; +} + +/** + * Reject a command whose caller observed a stale `updatedAt`. + * Omitting expectedUpdatedAt preserves the previous unversioned behaviour. + * @param {object} transfer + * @param {string|undefined} expectedUpdatedAt + */ +function assertFreshArchiveCommand(transfer, expectedUpdatedAt) { + if (expectedUpdatedAt == null) return; + if (transfer.updatedAt !== expectedUpdatedAt) { + throw ApiError.conflict( + `Stale archive command for transfer ${transfer.id}`, + { + code: 'STALE_ARCHIVE_COMMAND', + expectedUpdatedAt, + actualUpdatedAt: transfer.updatedAt, + archivedAt: transfer.archivedAt, + } + ); + } +} + +/** + * Latest immutable archive-history timestamp, used as the monotonic floor. + * @param {object} transfer + * @returns {string|null} + */ +function lastArchiveHistoryAt(transfer) { + const history = transfer.archiveHistory; + if (!Array.isArray(history) || history.length === 0) return null; + return history[history.length - 1].at; +} + +/** + * Append an immutable archive lifecycle event and return it. + * @param {object} transfer + * @param {object} event + * @returns {object} + */ +function appendArchiveHistory(transfer, event) { + if (!Array.isArray(transfer.archiveHistory)) { + transfer.archiveHistory = []; + } + const frozen = Object.freeze({ + action: event.action, + at: event.at, + actor: event.actor, + reason: event.reason, + requestId: event.requestId, + }); + transfer.archiveHistory.push(frozen); + return frozen; +} + /** * Archive a transfer. An archived transfer is hidden from default list results - * but remains queryable. Archiving is idempotent and orthogonal to the transfer - * lifecycle status. + * but remains queryable. Archiving is idempotent for retries that observe the + * current archived state, orthogonal to the transfer lifecycle status, and + * records an immutable, auditable event with actor and reason. + * + * Pass `expectedUpdatedAt` to opt into optimistic concurrency: a mismatched + * value is rejected as a stale command so concurrent archive/unarchive cycles + * cannot silently overwrite each other. + * * @param {string} id + * @param {string|object} [optionsOrRequestId] * @returns {object} */ -function archiveTransfer(id) { +function archiveTransfer(id, optionsOrRequestId) { + const options = normaliseArchiveOptions(optionsOrRequestId); const transfer = getTransferOrThrow(id); - if (!transfer.archivedAt) { - const timestamp = nextTimestamp(transfer.updatedAt); - transfer.archivedAt = timestamp; - transfer.updatedAt = timestamp; + assertFreshArchiveCommand(transfer, options.expectedUpdatedAt); + + // Already archived: idempotent retry. Do not move archivedAt / updatedAt + // backwards or rewrite history — that was the original failure mode. + if (transfer.archivedAt) { + return transfer; } + + const timestamp = nextTimestamp( + lastArchiveHistoryAt(transfer) || transfer.updatedAt + ); + transfer.archivedAt = timestamp; + transfer.lastArchivedAt = timestamp; + transfer.updatedAt = timestamp; + + appendArchiveHistory(transfer, { + action: 'archive', + at: timestamp, + actor: options.actor, + reason: options.reason, + requestId: options.requestId || null, + }); + + auditService.addEntry({ + action: 'transfer.archived', + resourceId: transfer.id, + payload: { + archivedAt: timestamp, + actor: options.actor, + reason: options.reason, + }, + requestId: options.requestId, + }); + return transfer; } /** * Unarchive a previously archived transfer, restoring it to default list results. + * Clears the current-state `archivedAt` flag but retains `lastArchivedAt` and + * an immutable history entry so the lifecycle order stays reconcilable. + * * @param {string} id + * @param {string|object} [optionsOrRequestId] * @returns {object} */ -function unarchiveTransfer(id) { +function unarchiveTransfer(id, optionsOrRequestId) { + const options = normaliseArchiveOptions(optionsOrRequestId); const transfer = getTransferOrThrow(id); + assertFreshArchiveCommand(transfer, options.expectedUpdatedAt); + if (!transfer.archivedAt) { - throw ApiError.conflict(`Transfer is not archived: ${id}`); + throw ApiError.conflict(`Transfer is not archived: ${id}`, { + code: 'NOT_ARCHIVED', + }); } + + const previousArchivedAt = transfer.archivedAt; + const timestamp = nextTimestamp( + lastArchiveHistoryAt(transfer) || transfer.updatedAt || previousArchivedAt + ); + transfer.archivedAt = null; - transfer.updatedAt = nextTimestamp(transfer.updatedAt); + transfer.lastArchivedAt = previousArchivedAt; + transfer.updatedAt = timestamp; + + appendArchiveHistory(transfer, { + action: 'unarchive', + at: timestamp, + actor: options.actor, + reason: options.reason, + requestId: options.requestId || null, + }); + + auditService.addEntry({ + action: 'transfer.unarchived', + resourceId: transfer.id, + payload: { + previousArchivedAt, + unarchivedAt: timestamp, + actor: options.actor, + reason: options.reason, + }, + requestId: options.requestId, + }); + return transfer; } diff --git a/test/transferArchiveLifecycle.test.js b/test/transferArchiveLifecycle.test.js new file mode 100644 index 0000000..f09f657 --- /dev/null +++ b/test/transferArchiveLifecycle.test.js @@ -0,0 +1,386 @@ +'use strict'; + +const { test, beforeEach } = require('node:test'); +const assert = require('node:assert/strict'); + +const { reset } = require('../src/store'); +const transferService = require('../src/services/transferService'); +const auditService = require('../src/services/auditService'); +const ApiError = require('../src/utils/ApiError'); + +beforeEach(() => { + reset(); +}); + +function createSample() { + return transferService.createTransfer({ + senderName: 'Alice', + recipientName: 'Bob', + amount: 100, + from: 'USD', + to: 'EUR', + }); +} + +function assertStrictlyIncreasing(timestamps) { + for (let i = 1; i < timestamps.length; i += 1) { + assert.ok( + timestamps[i] > timestamps[i - 1], + `expected ${timestamps[i]} > ${timestamps[i - 1]} at index ${i}` + ); + } +} + +// ============================================================================ +// State machine +// ============================================================================ + +test('state-machine: active → archived → active is the only archive cycle', () => { + const transfer = createSample(); + assert.equal(transfer.archivedAt, null); + assert.deepEqual(transfer.archiveHistory, []); + + const archived = transferService.archiveTransfer(transfer.id, { + actor: 'ops-token', + reason: 'retention-hold', + requestId: 'req-1', + }); + assert.ok(archived.archivedAt); + assert.equal(archived.archiveHistory.length, 1); + assert.equal(archived.archiveHistory[0].action, 'archive'); + + const active = transferService.unarchiveTransfer(transfer.id, { + actor: 'ops-token', + reason: 'hold-lifted', + requestId: 'req-2', + }); + assert.equal(active.archivedAt, null); + assert.equal(active.archiveHistory.length, 2); + assert.equal(active.archiveHistory[1].action, 'unarchive'); + + assert.throws( + () => transferService.unarchiveTransfer(transfer.id), + (err) => err instanceof ApiError && err.statusCode === 409 + ); +}); + +test('state-machine: archive is orthogonal to claim/cancel status', () => { + const claimed = createSample(); + transferService.claimTransfer(claimed.id); + transferService.archiveTransfer(claimed.id, { actor: 'a', reason: 'cleanup' }); + assert.equal(claimed.status, 'claimed'); + assert.ok(claimed.archivedAt); + + const cancelled = createSample(); + transferService.cancelTransfer(cancelled.id); + transferService.archiveTransfer(cancelled.id, { actor: 'a', reason: 'cleanup' }); + assert.equal(cancelled.status, 'cancelled'); + assert.ok(cancelled.archivedAt); +}); + +// ============================================================================ +// Stale-command rejection +// ============================================================================ + +test('stale-command: archive rejects mismatched expectedUpdatedAt', () => { + const transfer = createSample(); + const stale = transfer.updatedAt; + + // Advance updatedAt via an intervening claim so the original token is stale. + transferService.claimTransfer(transfer.id); + assert.notEqual(transfer.updatedAt, stale); + + assert.throws( + () => transferService.archiveTransfer(transfer.id, { + expectedUpdatedAt: stale, + actor: 'ops', + reason: 'stale-try', + }), + (err) => ( + err instanceof ApiError && + err.statusCode === 409 && + err.details && + err.details.code === 'STALE_ARCHIVE_COMMAND' && + err.details.expectedUpdatedAt === stale && + err.details.actualUpdatedAt === transfer.updatedAt + ) + ); + + assert.equal(transfer.archivedAt, null); + assert.equal(transfer.archiveHistory.length, 0); +}); + +test('stale-command: unarchive rejects mismatched expectedUpdatedAt', () => { + const transfer = createSample(); + transferService.archiveTransfer(transfer.id, { actor: 'ops', reason: 'park' }); + const fresh = transfer.updatedAt; + + assert.throws( + () => transferService.unarchiveTransfer(transfer.id, { + expectedUpdatedAt: '1999-01-01T00:00:00.000Z', + actor: 'ops', + reason: 'stale-unarchive', + }), + (err) => ( + err instanceof ApiError && + err.statusCode === 409 && + err.details.code === 'STALE_ARCHIVE_COMMAND' + ) + ); + + // Matching token still succeeds. + const result = transferService.unarchiveTransfer(transfer.id, { + expectedUpdatedAt: fresh, + actor: 'ops', + reason: 'release', + }); + assert.equal(result.archivedAt, null); +}); + +// ============================================================================ +// Race / optimistic concurrency +// ============================================================================ + +test('race: second concurrent archive with the same expectedUpdatedAt loses', () => { + const transfer = createSample(); + const observed = transfer.updatedAt; + + const winner = transferService.archiveTransfer(transfer.id, { + expectedUpdatedAt: observed, + actor: 'worker-a', + reason: 'race-win', + requestId: 'r-a', + }); + assert.ok(winner.archivedAt); + + assert.throws( + () => transferService.archiveTransfer(transfer.id, { + expectedUpdatedAt: observed, + actor: 'worker-b', + reason: 'race-lose', + requestId: 'r-b', + }), + (err) => ( + err instanceof ApiError && + err.statusCode === 409 && + err.details.code === 'STALE_ARCHIVE_COMMAND' + ) + ); + + // Winner's timestamp and single history event survive the loser. + assert.equal(transfer.archivedAt, winner.archivedAt); + assert.equal(transfer.archiveHistory.length, 1); + assert.equal(transfer.archiveHistory[0].actor, 'worker-a'); +}); + +test('race: archive then unarchive with stale token cannot reorder history', () => { + const transfer = createSample(); + const beforeArchive = transfer.updatedAt; + + transferService.archiveTransfer(transfer.id, { + expectedUpdatedAt: beforeArchive, + actor: 'a', + reason: 'first', + }); + const afterArchive = transfer.updatedAt; + + transferService.unarchiveTransfer(transfer.id, { + expectedUpdatedAt: afterArchive, + actor: 'a', + reason: 'second', + }); + + assert.throws( + () => transferService.archiveTransfer(transfer.id, { + expectedUpdatedAt: beforeArchive, + actor: 'b', + reason: 'stale-reorder', + }), + (err) => err instanceof ApiError && err.details.code === 'STALE_ARCHIVE_COMMAND' + ); + + assert.equal(transfer.archiveHistory.length, 2); + assert.equal(transfer.archivedAt, null); +}); + +// ============================================================================ +// Retry / idempotency +// ============================================================================ + +test('retry: re-archive without expectedUpdatedAt does not overwrite archivedAt', () => { + const transfer = createSample(); + const first = transferService.archiveTransfer(transfer.id, { + actor: 'ops', + reason: 'initial', + requestId: 'req-1', + }); + const archivedAt = first.archivedAt; + const updatedAt = first.updatedAt; + const historyLen = first.archiveHistory.length; + + const retry = transferService.archiveTransfer(transfer.id, { + actor: 'ops', + reason: 'retry', + requestId: 'req-1-retry', + }); + + assert.equal(retry.archivedAt, archivedAt); + assert.equal(retry.updatedAt, updatedAt); + assert.equal(retry.archiveHistory.length, historyLen); +}); + +test('retry: matching expectedUpdatedAt on already-archived transfer is a no-op', () => { + const transfer = createSample(); + transferService.archiveTransfer(transfer.id, { actor: 'ops', reason: 'park' }); + const token = transfer.updatedAt; + const archivedAt = transfer.archivedAt; + + const retry = transferService.archiveTransfer(transfer.id, { + expectedUpdatedAt: token, + actor: 'ops', + reason: 'retry-same', + }); + + assert.equal(retry.archivedAt, archivedAt); + assert.equal(retry.updatedAt, token); + assert.equal(retry.archiveHistory.length, 1); +}); + +// ============================================================================ +// Event order / monotonic timestamps +// ============================================================================ + +test('event-order: archive → unarchive → archive keeps strictly increasing timestamps', () => { + const transfer = createSample(); + + transferService.archiveTransfer(transfer.id, { + actor: 'ops', + reason: 'cycle-1', + requestId: 'e1', + }); + const firstArchivedAt = transfer.archivedAt; + + transferService.unarchiveTransfer(transfer.id, { + actor: 'ops', + reason: 'cycle-1-lift', + requestId: 'e2', + }); + assert.equal(transfer.lastArchivedAt, firstArchivedAt); + + transferService.archiveTransfer(transfer.id, { + actor: 'ops', + reason: 'cycle-2', + requestId: 'e3', + }); + + assert.equal(transfer.archiveHistory.length, 3); + const ats = transfer.archiveHistory.map((e) => e.at); + assertStrictlyIncreasing(ats); + assert.ok(transfer.archivedAt > firstArchivedAt); + assert.equal(transfer.archiveHistory[0].at, firstArchivedAt); + // Original archive timestamp is preserved in history (not overwritten). + assert.equal(transfer.archiveHistory[0].action, 'archive'); + assert.equal(transfer.archiveHistory[1].action, 'unarchive'); + assert.equal(transfer.archiveHistory[2].action, 'archive'); +}); + +test('event-order: history event timestamps are immutable after write', () => { + const transfer = createSample(); + transferService.archiveTransfer(transfer.id, { actor: 'ops', reason: 'lock' }); + const event = transfer.archiveHistory[0]; + const originalAt = event.at; + + assert.throws(() => { + event.at = '1999-01-01T00:00:00.000Z'; + }, TypeError); + + assert.equal(event.at, originalAt); +}); + +test('event-order: updatedAt never moves backward across archive lifecycle', () => { + const transfer = createSample(); + const stamps = [transfer.createdAt, transfer.updatedAt]; + + transferService.archiveTransfer(transfer.id, { actor: 'a', reason: 'r1' }); + stamps.push(transfer.updatedAt); + transferService.unarchiveTransfer(transfer.id, { actor: 'a', reason: 'r2' }); + stamps.push(transfer.updatedAt); + transferService.archiveTransfer(transfer.id, { actor: 'a', reason: 'r3' }); + stamps.push(transfer.updatedAt); + + assertStrictlyIncreasing(stamps); +}); + +// ============================================================================ +// Auditability (actor + reason) +// ============================================================================ + +test('audit: archive and unarchive record actor and reason', () => { + const transfer = createSample(); + + transferService.archiveTransfer(transfer.id, { + actor: 'auditor-token', + reason: 'compliance-review', + requestId: 'req-arch', + }); + transferService.unarchiveTransfer(transfer.id, { + actor: 'auditor-token', + reason: 'review-complete', + requestId: 'req-unarch', + }); + + const entries = auditService.getEntriesForResource(transfer.id); + const archived = entries.find((e) => e.action === 'transfer.archived'); + const unarchived = entries.find((e) => e.action === 'transfer.unarchived'); + + assert.ok(archived); + assert.equal(archived.payload.actor, 'auditor-token'); + assert.equal(archived.payload.reason, 'compliance-review'); + assert.equal(archived.requestId, 'req-arch'); + + assert.ok(unarchived); + assert.equal(unarchived.payload.actor, 'auditor-token'); + assert.equal(unarchived.payload.reason, 'review-complete'); + assert.ok(unarchived.payload.previousArchivedAt); + + assert.equal(transfer.archiveHistory[0].actor, 'auditor-token'); + assert.equal(transfer.archiveHistory[0].reason, 'compliance-review'); + assert.equal(transfer.archiveHistory[1].reason, 'review-complete'); +}); + +// ============================================================================ +// Regression: original failure mode +// ============================================================================ + +test('regression: repeated archive/unarchive does not hide earlier lifecycle timestamps', () => { + const transfer = createSample(); + + transferService.archiveTransfer(transfer.id, { + actor: 'ops', + reason: 'first-archive', + }); + const firstAt = transfer.archivedAt; + + transferService.unarchiveTransfer(transfer.id, { + actor: 'ops', + reason: 'temporary-restore', + }); + // Current flag is cleared, but lastArchivedAt and history retain the order. + assert.equal(transfer.archivedAt, null); + assert.equal(transfer.lastArchivedAt, firstAt); + assert.equal(transfer.archiveHistory[0].at, firstAt); + + transferService.archiveTransfer(transfer.id, { + actor: 'ops', + reason: 'second-archive', + }); + const secondAt = transfer.archivedAt; + + assert.ok(secondAt > firstAt); + // History still starts with the first archive — not overwritten. + assert.equal(transfer.archiveHistory[0].at, firstAt); + assert.equal(transfer.archiveHistory[0].reason, 'first-archive'); + assert.equal(transfer.archiveHistory[2].at, secondAt); + assert.equal(transfer.archiveHistory[2].reason, 'second-archive'); + assertStrictlyIncreasing(transfer.archiveHistory.map((e) => e.at)); +}); From 0a842a05772636708cd82fc4cf6dacb7e535ee55 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sat, 3 Oct 2026 03:20:15 -0400 Subject: [PATCH 2/7] fix(archive): advance version timestamps across status changes Archive and unarchive now advance beyond updatedAt, archive history, and the prior archive timestamp. Interleaved claim or cancel operations cannot leave a reused optimistic-concurrency token. Add focused regressions for mixed lifecycle commands and legacy timestamp floors. Validation: npm test (271 passed, 0 failed). --- src/services/transferService.js | 19 ++++++---- test/transferArchiveLifecycle.test.js | 54 +++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/services/transferService.js b/src/services/transferService.js index 22a529c..0d421ae 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -12,10 +12,12 @@ const config = require('../config'); // Keep lifecycle timestamps strictly increasing even when multiple operations // happen within the same millisecond (common in tests and API batches). -function nextTimestamp(previous) { - const now = Date.now(); - const previousMs = previous ? Date.parse(previous) : NaN; - return new Date(Math.max(now, Number.isFinite(previousMs) ? previousMs + 1 : now)).toISOString(); +function nextTimestamp(...previous) { + const next = previous.reduce((latest, value) => { + const previousMs = value ? Date.parse(value) : NaN; + return Number.isFinite(previousMs) ? Math.max(latest, previousMs + 1) : latest; + }, Date.now()); + return new Date(next).toISOString(); } /** @@ -407,7 +409,7 @@ function assertFreshArchiveCommand(transfer, expectedUpdatedAt) { } /** - * Latest immutable archive-history timestamp, used as the monotonic floor. + * Latest immutable archive-history timestamp, used as a monotonic floor. * @param {object} transfer * @returns {string|null} */ @@ -464,7 +466,8 @@ function archiveTransfer(id, optionsOrRequestId) { } const timestamp = nextTimestamp( - lastArchiveHistoryAt(transfer) || transfer.updatedAt + transfer.updatedAt, + lastArchiveHistoryAt(transfer) ); transfer.archivedAt = timestamp; transfer.lastArchivedAt = timestamp; @@ -514,7 +517,9 @@ function unarchiveTransfer(id, optionsOrRequestId) { const previousArchivedAt = transfer.archivedAt; const timestamp = nextTimestamp( - lastArchiveHistoryAt(transfer) || transfer.updatedAt || previousArchivedAt + transfer.updatedAt, + lastArchiveHistoryAt(transfer), + previousArchivedAt ); transfer.archivedAt = null; diff --git a/test/transferArchiveLifecycle.test.js b/test/transferArchiveLifecycle.test.js index f09f657..3458acc 100644 --- a/test/transferArchiveLifecycle.test.js +++ b/test/transferArchiveLifecycle.test.js @@ -352,6 +352,60 @@ test('audit: archive and unarchive record actor and reason', () => { // Regression: original failure mode // ============================================================================ +test('regression: interleaved status and archive commands cannot reuse a version token', (t) => { + const now = Date.now(); + t.mock.method(Date, 'now', () => now); + + for (const changeStatus of ['claimTransfer', 'cancelTransfer']) { + for (const initiallyArchived of [true, false]) { + const transfer = createSample(); + transferService.archiveTransfer(transfer.id); + if (!initiallyArchived) transferService.unarchiveTransfer(transfer.id); + + transferService[changeStatus](transfer.id); + const observed = transfer.updatedAt; + const action = initiallyArchived ? 'unarchiveTransfer' : 'archiveTransfer'; + const staleAction = initiallyArchived ? 'archiveTransfer' : 'unarchiveTransfer'; + + transferService[action](transfer.id, { expectedUpdatedAt: observed }); + assert.ok( + transfer.updatedAt > observed, + `${changeStatus} followed by ${action} must advance updatedAt` + ); + const historyLength = transfer.archiveHistory.length; + const updatedAt = transfer.updatedAt; + + assert.throws( + () => transferService[staleAction](transfer.id, { expectedUpdatedAt: observed }), + (err) => err instanceof ApiError && err.details.code === 'STALE_ARCHIVE_COMMAND' + ); + assert.equal(transfer.updatedAt, updatedAt); + assert.equal(transfer.archiveHistory.length, historyLength); + } + } +}); + +test('regression: archive transitions respect history and legacy archive timestamp floors', (t) => { + const now = Date.now(); + t.mock.method(Date, 'now', () => now - 60_000); + + const transfer = createSample(); + transferService.archiveTransfer(transfer.id); + transferService.unarchiveTransfer(transfer.id); + const historyAt = transfer.archiveHistory.at(-1).at; + transfer.updatedAt = transfer.createdAt; + transferService.archiveTransfer(transfer.id); + assert.ok(transfer.updatedAt > historyAt); + + const legacy = createSample(); + legacy.archivedAt = new Date(now + 60_000).toISOString(); + const previousArchivedAt = legacy.archivedAt; + transferService.unarchiveTransfer(legacy.id); + assert.ok(legacy.updatedAt > previousArchivedAt); + assert.equal(legacy.lastArchivedAt, previousArchivedAt); + assert.equal(legacy.archiveHistory[0].at, legacy.updatedAt); +}); + test('regression: repeated archive/unarchive does not hide earlier lifecycle timestamps', () => { const transfer = createSample(); From 5f8d0f94f7cce04cd798e0c8017fa2cbfa63dc85 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sat, 3 Oct 2026 11:23:00 -0400 Subject: [PATCH 3/7] fix(archive): fingerprint HTTP actors before recording history Archive and unarchive passed raw bearer credentials into readable transfer history and audit payloads. Reuse the existing keyed actor fingerprint at the controller boundary, retaining trusted service actor labels and all prior monotonic timestamp, stale-command and retry behavior. Add two real HTTP regressions to the existing lifecycle suite and document actor identity in CHANGELOG. No service, dependency or workflow change. Validation on Node 24.19.0 with all 76 retained package-lock versions matching: - 13 actual local app HTTP requests reproduce raw credential disclosure on parent 0a842a05772636708cd82fc4cf6dacb7e535ee55 and omit both writers' tokens after correction. Same/distinct actors, history/audit consistency, retries, and 409/401/403 controls pass. Invented local fixtures and mock settlement. - Focused archive suites: 36 passed. Parent controller plus new maintained tests: 2 expected HTTP regressions fail, 34 controls pass. - npm test -- --test-concurrency=1: 273 passed, 0 failed, 0 skipped. - git diff --check passes. No install or external settlement. Node 22 hosted CI and maintainer acceptance remain pending. Existing upstream metadata permission denial is retained for the authorized body publisher. --- CHANGELOG.md | 8 ++ src/controllers/transferController.js | 4 +- test/transferArchiveLifecycle.test.js | 128 ++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f778e9..8b8ded9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,14 @@ When preparing a new release: ### Fixed +- Archive and unarchive HTTP requests now store the existing keyed actor + fingerprint in lifecycle history and audit entries, keeping bearer credentials + out of transfer and audit responses while preserving attribution across cycles. + Fingerprints share the pagination signing secret and the process-local store's + lifetime; trusted service callers retain their explicit actor labels. Existing + event timestamps, reasons, request IDs, retries, and stale-command checks are + unchanged. + - Offset pagination over transfer and audit history repeated or skipped rows when records were written while a client was paging, because the window was defined by a row count rather than a position. Cursor pagination anchors to diff --git a/src/controllers/transferController.js b/src/controllers/transferController.js index ae3a952..bd8d90a 100644 --- a/src/controllers/transferController.js +++ b/src/controllers/transferController.js @@ -2,6 +2,7 @@ const transferService = require('../services/transferService'); const { buildHistoryPage } = require('../utils/historyPage'); +const { actorFingerprint } = require('../utils/cursor'); const idempotencyService = require('../services/idempotencyService'); const ApiError = require('../utils/ApiError'); @@ -175,7 +176,8 @@ function archiveMutationOptions(req) { const body = req.body || {}; return { requestId: req.id, - actor: req.token || null, + // History and audit responses must identify the caller without storing credentials. + actor: req.token ? actorFingerprint(req) : null, reason: body.reason, expectedUpdatedAt: readExpectedUpdatedAt(req), }; diff --git a/test/transferArchiveLifecycle.test.js b/test/transferArchiveLifecycle.test.js index 3458acc..e57ddaa 100644 --- a/test/transferArchiveLifecycle.test.js +++ b/test/transferArchiveLifecycle.test.js @@ -2,11 +2,16 @@ const { test, beforeEach } = require('node:test'); const assert = require('node:assert/strict'); +const { once } = require('node:events'); + +process.env.NODE_ENV = 'test'; const { reset } = require('../src/store'); const transferService = require('../src/services/transferService'); const auditService = require('../src/services/auditService'); const ApiError = require('../src/utils/ApiError'); +const config = require('../src/config'); +const createApp = require('../src/app'); beforeEach(() => { reset(); @@ -31,6 +36,46 @@ function assertStrictlyIncreasing(timestamps) { } } +const archiveWriterA = 'archive-lifecycle-invented-local-writer-a'; +const archiveWriterB = 'archive-lifecycle-invented-local-writer-b'; +const archiveReader = 'archive-lifecycle-invented-local-reader'; + +async function startArchiveHttp(t) { + const originalTokens = config.apiTokens; + config.apiTokens = { + [archiveWriterA]: ['transfers:write'], + [archiveWriterB]: ['transfers:write'], + [archiveReader]: ['transfers:read', 'audit:read'], + }; + const server = createApp().listen(0, '127.0.0.1'); + t.after(async () => { + config.apiTokens = originalTokens; + server.closeAllConnections(); + await new Promise((resolve) => server.close(resolve)); + }); + await once(server, 'listening'); + const baseUrl = `http://127.0.0.1:${server.address().port}/api`; + + return async (path, { token, method = 'GET', body, requestId } = {}) => { + const headers = { Authorization: `Bearer ${token}`, Connection: 'close' }; + if (body) headers['Content-Type'] = 'application/json'; + if (requestId) headers['X-Request-Id'] = requestId; + const response = await fetch(`${baseUrl}${path}`, { + method, + headers, + body: body ? JSON.stringify(body) : undefined, + }); + return { status: response.status, body: await response.json() }; + }; +} + +function assertNoArchiveCredentials(value) { + const serialized = JSON.stringify(value); + for (const token of [archiveWriterA, archiveWriterB, archiveReader]) { + assert.equal(serialized.includes(token), false, 'stored and readable records must omit bearer credentials'); + } +} + // ============================================================================ // State machine // ============================================================================ @@ -348,6 +393,89 @@ test('audit: archive and unarchive record actor and reason', () => { assert.equal(transfer.archiveHistory[1].reason, 'review-complete'); }); +test('HTTP archive cycles retain distinct, stable actors without exposing writer credentials', async (t) => { + const request = await startArchiveHttp(t); + const transfer = createSample(); + let version = transfer.updatedAt; + const commands = [ + { action: 'archive', token: archiveWriterA, reason: 'review' }, + { action: 'unarchive', token: archiveWriterA, reason: 'review-complete' }, + { action: 'archive', token: archiveWriterB, reason: 'second-review' }, + ]; + + for (const [index, command] of commands.entries()) { + const requestId = `archive-http-${index}`; + const response = await request(`/transfers/${transfer.id}/${command.action}`, { + token: command.token, + method: 'POST', + body: { reason: command.reason, expectedUpdatedAt: version }, + requestId, + }); + assert.equal(response.status, 200); + const event = response.body.archiveHistory[index]; + assert.match(event.actor, /^[a-f0-9]{16}$/); + assert.equal(event.reason, command.reason); + assert.equal(event.requestId, requestId); + assert.equal(event.at, response.body.updatedAt); + assert.ok(response.body.updatedAt > version); + assertNoArchiveCredentials(response.body); + version = response.body.updatedAt; + } + + const actors = transfer.archiveHistory.map((event) => event.actor); + assert.equal(actors[0], actors[1], 'the same writer retains one actor across both endpoints'); + assert.notEqual(actors[0], actors[2], 'different writers remain distinguishable'); + const entries = auditService.getEntriesForResource(transfer.id) + .filter((entry) => entry.action !== 'transfer.created').reverse(); + assert.deepEqual(entries.map((entry) => entry.payload.actor), actors); + assert.deepEqual(entries.map((entry) => entry.payload.reason), commands.map((command) => command.reason)); + assert.deepEqual(entries.map((entry) => entry.requestId), ['archive-http-0', 'archive-http-1', 'archive-http-2']); + assertNoArchiveCredentials(transfer); + assertNoArchiveCredentials(entries); + + for (const path of [ + `/transfers/${transfer.id}`, + '/transfers?archived=all', + `/audit?resourceId=${transfer.id}`, + ]) { + const response = await request(path, { token: archiveReader }); + assert.equal(response.status, 200); + assertNoArchiveCredentials(response.body); + } +}); + +test('HTTP unarchive fingerprints its writer and preserves legacy service actor labels', async (t) => { + const request = await startArchiveHttp(t); + const transfer = createSample(); + transferService.archiveTransfer(transfer.id, { + actor: 'backoffice-job', + reason: 'legacy-review', + requestId: 'legacy-request', + }); + const originalEvent = { ...transfer.archiveHistory[0] }; + + const response = await request(`/transfers/${transfer.id}/unarchive`, { + token: archiveWriterB, + method: 'POST', + body: { reason: 'restored-over-http', expectedUpdatedAt: transfer.updatedAt }, + requestId: 'http-restore', + }); + assert.equal(response.status, 200); + assert.equal(response.body.archivedAt, null); + assert.deepEqual(response.body.archiveHistory[0], originalEvent); + const event = response.body.archiveHistory[1]; + assert.match(event.actor, /^[a-f0-9]{16}$/); + assert.equal(event.reason, 'restored-over-http'); + assert.equal(event.requestId, 'http-restore'); + assertNoArchiveCredentials(response.body); + + const entries = auditService.getEntriesForResource(transfer.id); + assert.equal(entries.find((entry) => entry.action === 'transfer.archived').payload.actor, 'backoffice-job'); + assert.equal(entries.find((entry) => entry.action === 'transfer.unarchived').payload.actor, event.actor); + assertNoArchiveCredentials(transfer); + assertNoArchiveCredentials(entries); +}); + // ============================================================================ // Regression: original failure mode // ============================================================================ From 45d0bd4e8f1ad71a22ee3d759a84388ae2a9f968 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sat, 3 Oct 2026 22:26:33 -0400 Subject: [PATCH 4/7] fix(archive): retain supplied non-string version preconditions Preserve malformed body concurrency tokens for the existing service freshness check when no nonblank If-Match fallback applies. Keep optional values, body/header selection, actor fingerprints, timestamp ordering, and no-token retry behavior. Add real HTTP regressions to the existing lifecycle file and document the behavior. --- CHANGELOG.md | 7 +++ src/controllers/transferController.js | 3 +- test/transferArchiveLifecycle.test.js | 88 ++++++++++++++++++++++++++- 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b8ded9..072cdf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,13 @@ When preparing a new release: ### Fixed +- Archive and unarchive HTTP requests preserve supplied non-string + `expectedUpdatedAt` values when there is no nonblank `If-Match` fallback. + The existing service check rejects those requests with + `409 STALE_ARCHIVE_COMMAND` instead of treating them as unversioned writes. + Body/header selection, omitted/null/blank optional values, actor attribution, + timestamp ordering, and no-token retries keep their existing behavior. + - Archive and unarchive HTTP requests now store the existing keyed actor fingerprint in lifecycle history and audit entries, keeping bearer credentials out of transfer and audit responses while preserving attribution across cycles. diff --git a/src/controllers/transferController.js b/src/controllers/transferController.js index bd8d90a..bdb91b5 100644 --- a/src/controllers/transferController.js +++ b/src/controllers/transferController.js @@ -164,7 +164,8 @@ function readExpectedUpdatedAt(req) { if (typeof ifMatch === 'string' && ifMatch.trim() !== '') { return ifMatch.trim().replace(/^W\//, '').replace(/^"|"$/g, ''); } - return undefined; + // Preserve supplied non-string tokens for the service's strict freshness check. + return fromBody != null && typeof fromBody !== 'string' ? fromBody : undefined; } /** diff --git a/test/transferArchiveLifecycle.test.js b/test/transferArchiveLifecycle.test.js index e57ddaa..e377852 100644 --- a/test/transferArchiveLifecycle.test.js +++ b/test/transferArchiveLifecycle.test.js @@ -56,10 +56,11 @@ async function startArchiveHttp(t) { await once(server, 'listening'); const baseUrl = `http://127.0.0.1:${server.address().port}/api`; - return async (path, { token, method = 'GET', body, requestId } = {}) => { + return async (path, { token, method = 'GET', body, requestId, ifMatch } = {}) => { const headers = { Authorization: `Bearer ${token}`, Connection: 'close' }; if (body) headers['Content-Type'] = 'application/json'; if (requestId) headers['X-Request-Id'] = requestId; + if (ifMatch !== undefined) headers['If-Match'] = ifMatch; const response = await fetch(`${baseUrl}${path}`, { method, headers, @@ -566,3 +567,88 @@ test('regression: repeated archive/unarchive does not hide earlier lifecycle tim assert.equal(transfer.archiveHistory[2].reason, 'second-archive'); assertStrictlyIncreasing(transfer.archiveHistory.map((e) => e.at)); }); + +// ============================================================================ +// HTTP optimistic-concurrency token preservation +// ============================================================================ + +for (const action of ['archive', 'unarchive']) { + for (const [kind, value] of [ + ['number', 0], + ['boolean', false], + ['array', []], + ['object', {}], + ]) { + test(`HTTP concurrency token: ${action} rejects a supplied ${kind} without changing history`, async (t) => { + const request = await startArchiveHttp(t); + const transfer = createSample(); + if (action === 'unarchive') transferService.archiveTransfer(transfer.id); + const before = JSON.parse(JSON.stringify(transfer)); + const beforeAudit = JSON.parse(JSON.stringify(auditService.getEntriesForResource(transfer.id))); + + const response = await request(`/transfers/${transfer.id}/${action}`, { + token: archiveWriterA, + method: 'POST', + body: { expectedUpdatedAt: value, reason: 'stale-type-should-not-write' }, + }); + + assert.equal(response.status, 409); + assert.equal(response.body.error.details.code, 'STALE_ARCHIVE_COMMAND'); + assert.deepEqual(response.body.error.details.expectedUpdatedAt, value); + assert.deepEqual(JSON.parse(JSON.stringify(transferService.getTransferOrThrow(transfer.id))), before); + assert.deepEqual(JSON.parse(JSON.stringify(auditService.getEntriesForResource(transfer.id))), beforeAudit); + const readback = await request(`/transfers/${transfer.id}`, { token: archiveReader }); + assert.equal(readback.status, 200); + assert.deepEqual(readback.body, before); + }); + } + + test(`HTTP concurrency token: ${action} preserves optional values and body/header selection`, async (t) => { + const request = await startArchiveHttp(t); + const stale = '1999-01-01T00:00:00.000Z'; + const cases = [ + { name: 'omitted', options: () => ({ body: { reason: 'optional-token' } }) }, + { name: 'null', options: () => ({ body: { expectedUpdatedAt: null } }) }, + { name: 'empty', options: () => ({ body: { expectedUpdatedAt: '' } }) }, + { name: 'whitespace', options: () => ({ body: { expectedUpdatedAt: ' ' } }) }, + { name: 'trimmed body wins', options: version => ({ + body: { expectedUpdatedAt: ` ${version} ` }, ifMatch: stale, + }) }, + { name: 'bare header', options: version => ({ ifMatch: version }) }, + { name: 'weak quoted header', options: version => ({ ifMatch: `W/"${version}"` }) }, + { name: 'header fallback with non-string body', options: version => ({ + body: { expectedUpdatedAt: false }, ifMatch: version, + }) }, + ]; + + for (const scenario of cases) { + const transfer = createSample(); + if (action === 'unarchive') transferService.archiveTransfer(transfer.id); + const historyLength = transfer.archiveHistory.length; + const response = await request(`/transfers/${transfer.id}/${action}`, { + token: archiveWriterA, + method: 'POST', + ...scenario.options(transfer.updatedAt), + }); + assert.equal(response.status, 200, scenario.name); + assert.equal(response.body.archiveHistory.length, historyLength + 1, scenario.name); + assert.equal(response.body.archiveHistory.at(-1).action, action, scenario.name); + } + + const transfer = createSample(); + if (action === 'unarchive') transferService.archiveTransfer(transfer.id); + const before = JSON.parse(JSON.stringify(transfer)); + const beforeAudit = JSON.parse(JSON.stringify(auditService.getEntriesForResource(transfer.id))); + const response = await request(`/transfers/${transfer.id}/${action}`, { + token: archiveWriterA, + method: 'POST', + body: { expectedUpdatedAt: 0 }, + ifMatch: stale, + }); + assert.equal(response.status, 409); + assert.equal(response.body.error.details.code, 'STALE_ARCHIVE_COMMAND'); + assert.equal(response.body.error.details.expectedUpdatedAt, stale); + assert.deepEqual(JSON.parse(JSON.stringify(transferService.getTransferOrThrow(transfer.id))), before); + assert.deepEqual(JSON.parse(JSON.stringify(auditService.getEntriesForResource(transfer.id))), beforeAudit); + }); +} From a9d97a6d386119a8fe01d7cc867c70b09bd76355 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 05:24:09 -0400 Subject: [PATCH 5/7] docs: record PR144 native body-precondition acceptance [skip ci] --- .../rfb144-body-precondition-20261004.md | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 docs/validation/rfb144-body-precondition-20261004.md diff --git a/docs/validation/rfb144-body-precondition-20261004.md b/docs/validation/rfb144-body-precondition-20261004.md new file mode 100644 index 0000000..e0de050 --- /dev/null +++ b/docs/validation/rfb144-body-precondition-20261004.md @@ -0,0 +1,81 @@ +# PR 144 body-precondition acceptance — 4 October 2026 + +## Result + +The existing archive acceptance suites passed **46 tests, with 0 failures, skips, +or cancellations**, on source commit +`45d0bd4e8f1ad71a22ee3d759a84388ae2a9f968`. + +This completes the previously unexecuted native validation of the body-precondition +continuation on [RemitFlow-Backend PR 144](https://github.com/RemitFlow/RemitFlow-Backend/pull/144). +That continuation was published while its execution environment was unavailable. + +[Successful GitHub Actions run 37191775463](https://github.com/woahwhattheheck/RemitFlow-Backend/actions/runs/37191775463) +contains the complete command output. The acceptance job is `111405306802`. +It ran on the public contributor fork and checked out the exact source commit +above before installing or executing anything. + +## Execution + +Runtime: **Node.js v22.23.3**, **npm 10.9.9**, Linux x64, GitHub-hosted +`ubuntu-latest`. The existing repository CI also selects Node.js 22. + +The job used the unchanged manifest and lockfile with `npm ci`, followed by: + +```bash +node --test --test-concurrency=1 \ + test/transferArchive.test.js \ + test/transferArchiveLifecycle.test.js +``` + +The test runner reported a total duration of 589.94016 ms. This is the duration +of that suite invocation, excluding checkout and dependency installation; it is +not a backend throughput or latency benchmark. + +The existing HTTP cases execute the application through a local listener and +use its in-memory store and mock settlement adapter. The ten pending additions +all passed: + +- Eight malformed body-precondition cases cover number, boolean, array, and + object values for both archive and unarchive. They require + `409 STALE_ARCHIVE_COMMAND` and unchanged transfer history and audit records. +- Two compatibility groups cover optional values, trimmed body precedence, + bare and weak-quoted headers, valid header fallback for a non-string body, + and a stale header fallback. The existing successful and stale-command + behavior is preserved. + +All 36 earlier tests in these two suites also passed. No source correction was +needed after execution. + +## Source and artifact identities + +| Item | Identity | +| --- | --- | +| Tested repository | `woahwhattheheck/RemitFlow-Backend` | +| Tested source commit | `45d0bd4e8f1ad71a22ee3d759a84388ae2a9f968` | +| Validation workflow commit | `8ab10b252c90b28d161ad804b65fd0a22e1f95bb` | +| Package manifest Git blob | `a1bf7b6726b103f58de2e8cf6f2bab71fca6883b` | +| Package lock Git blob | `be3ad84b812121d2ec4f47541df3a55816bb210c` | +| Package lock SHA-256 | `74077a5a97f6548a4049aa9756e14dfbc04beab1c82b57923942ff0d99524894` | +| Locked dependencies | 76 | +| Actions artifact | `11299062132` / `rfb144-body-precondition-a993` | +| Artifact ZIP SHA-256 | `523eb61c2ef23e0f865b7387bef0dad14931688b2f7939995b9de88f5cfb8a8f` | +| Dependency tar SHA-256 | `0ae2e8dba5fb52549442edcae73b55a93d4e7f5d5141c842431373c565f09680` | + +The artifact contains `acceptance.tap`, `runtime-metadata.json`, and the +`node_modules.tar.gz` produced by that same lockfile installation. It expires +on **11 October 2026 at 09:19:31 UTC**. Its downloaded ZIP and dependency-tar +digests were verified. Reuse of these dependencies requires the matching +lockfile; the artifact does not establish execution of another source revision. + +## Limits + +Only the two named maintained suites ran in this continuation. There is no new +full-suite result, live-provider settlement result, deployment, or upstream +maintainer acceptance claim. The successful fork job does not replace upstream +CI approval. + +The original contribution remains on `latch/remitflow-126-archive-timestamps`. +This receipt is an additive documentation change; its evidence refers to the +tested source commit above. The separate validation workflow stays on +`validation/rfb144-body-precondition-a993`. From 59b58555de22d7eb71772e9423cd938ed3f608c4 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 08:25:28 -0400 Subject: [PATCH 6/7] fix: reject structured archive reasons as invalid input --- docs/validation/archive-reason-20261004.md | 43 +++++++++++++++ src/services/transferService.js | 3 ++ test/archiveReasonValidation.test.js | 61 ++++++++++++++++++++++ 3 files changed, 107 insertions(+) create mode 100644 docs/validation/archive-reason-20261004.md create mode 100644 test/archiveReasonValidation.test.js diff --git a/docs/validation/archive-reason-20261004.md b/docs/validation/archive-reason-20261004.md new file mode 100644 index 0000000..e9655a0 --- /dev/null +++ b/docs/validation/archive-reason-20261004.md @@ -0,0 +1,43 @@ +# Archive reason input validation — 2026-10-04 + +PR: https://github.com/RemitFlow/RemitFlow-Backend/pull/144 + +## Change + +Reject object and array `reason` values with a 400 validation error before archive or unarchive looks up a transfer. The previous `String(reason)` conversion could throw for valid JSON such as `{"toString":null}`; the application error handler then rendered an internal-error envelope (500). Plain objects and arrays could also become meaningless audit-reason strings. + +The three-line guard preserves the existing normalization of omitted, null, empty-string, string, number, and boolean reasons. This change does not alter archive history, timestamp ordering, or concurrency checks. + +## Source pins + +| Input | Git object | +| --- | --- | +| Baseline branch head | `a9d97a6d386119a8fe01d7cc867c70b09bd76355` | +| Original `src/services/transferService.js` | `0d421aeeac2cd3d7e9f131918c74d1cc086269bf` | +| Candidate `src/services/transferService.js` | `dc550d29e0fc0463de352ac834a835acac0c9b88` | +| `test/archiveReasonValidation.test.js` | `fd295a1489615673eaf11f7b7d2e44d280ddd94a` | +| Unchanged `src/utils/ApiError.js` | `6b586f60d991ab4e5174e0af4e0d94cf1e3ceb58` | +| Unchanged `src/middleware/errorHandler.js` | `e75a516bde4ec4052d39bea396298e9f2ac5c981` | +| Unchanged `src/services/errorTrackingService.js` | `205391bbd86d48478edc854bf4d5e4a4813596ac` | + +## Focused execution + +Node.js v24.19.0; the same three `node:test` cases executed once against each service version. + +| Case | Original | Candidate | +| --- | --- | --- | +| Reject structured reasons before transfer lookup | Failed: reached missing-transfer 404 | Passed: 400 for both actions | +| Preserve scalar normalization | Passed: retained missing-transfer 404 | Passed: retained missing-transfer 404 | +| Render malformed JSON reason through the application error handler | Failed: 500 instead of 400 | Passed: 400 validation envelope and request ID | + +Original: **1 passed, 2 failed**, exit 1. Candidate: **3 passed, 0 failed**, exit 0. Both executions had zero skipped or cancelled cases. + +The execution used `node --test-reporter=tap` with a CommonJS source loader in memory. It loaded the complete service, ApiError, error handler, and error-tracking source at the pins above, plus the unchanged regression-test source in both phases. Explicit collaborators were an empty transfer map, a logging sink, error tracking disabled in config, and empty unused service dependencies. The cases stop at reason normalization or missing-transfer lookup. + +This establishes the source and error-envelope behavior. It does not establish full Express routing, authentication, successful archive mutations, history integration, or full-suite acceptance. No dependency install or full-suite run was performed for this change. + +The maintained regression file can be run in a normal repository checkout with dependencies installed: + +```bash +node --test test/archiveReasonValidation.test.js +``` diff --git a/src/services/transferService.js b/src/services/transferService.js index 0d421ae..dc550d2 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -379,6 +379,9 @@ function normaliseArchiveOptions(optionsOrRequestId) { }; } const reason = optionsOrRequestId.reason; + if (reason !== null && typeof reason === 'object') { + throw ApiError.badRequest('reason must not be an object or array'); + } return { requestId: optionsOrRequestId.requestId, actor: optionsOrRequestId.actor == null ? null : String(optionsOrRequestId.actor), diff --git a/test/archiveReasonValidation.test.js b/test/archiveReasonValidation.test.js new file mode 100644 index 0000000..fd295a1 --- /dev/null +++ b/test/archiveReasonValidation.test.js @@ -0,0 +1,61 @@ +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const transferService = require('../src/services/transferService'); +const errorHandler = require('../src/middleware/errorHandler'); +const ApiError = require('../src/utils/ApiError'); + +const absentTransferId = 'archive-reason-validation-absent-transfer'; +const actions = ['archiveTransfer', 'unarchiveTransfer']; + +test('archive and unarchive reject structured reasons before transfer lookup', () => { + for (const action of actions) { + for (const reason of [{}, [], ['retention'], { toString: null }, { toString: [], valueOf: false }]) { + assert.throws( + () => transferService[action](absentTransferId, { reason }), + error => error instanceof ApiError && error.statusCode === 400 && /reason/.test(error.message), + action + ': structured reason must be an input error', + ); + } + } +}); + +test('existing scalar reasons continue through normalization to transfer lookup', () => { + for (const action of actions) { + for (const reason of [undefined, null, '', 'retention review', 0, 7, false, true]) { + assert.throws( + () => transferService[action](absentTransferId, { reason }), + error => error instanceof ApiError && error.statusCode === 404, + action + ': scalar reason must retain the missing-transfer result', + ); + } + } +}); + +test('malformed JSON reason renders a validation envelope instead of an internal error', () => { + for (const action of actions) { + let error; + try { + transferService[action](absentTransferId, { reason: JSON.parse('{"toString":null}') }); + } catch (caught) { + error = caught; + } + assert.ok(error); + let status; + let body; + const response = { + status(value) { status = value; return this; }, + json(value) { body = value; return this; }, + }; + errorHandler(error, { + id: 'archive-reason-regression', + method: 'POST', + originalUrl: '/api/transfers/' + absentTransferId + '/' + action, + }, response, () => assert.fail('error handler must finish the response')); + assert.equal(status, 400); + assert.equal(body.error.status, 400); + assert.equal(body.error.requestId, 'archive-reason-regression'); + assert.match(body.error.message, /reason/); + } +}); From ff31a5f6078af6fe103c9738232e8d870b24cb62 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 08:55:03 -0400 Subject: [PATCH 7/7] fix(archive): retain state and retry token when audit append fails Archive and unarchive changed their flags, updatedAt and history before the synchronous audit writer returned. A rejected audit append therefore produced an HTTP failure after changing the transfer and consuming its expectedUpdatedAt token. Perform the existing audit append before committing those state/history changes. Validation, precondition checks, timestamp calculation and existing archive no-op behavior remain in place. Preserve structured-reason validation, actor fingerprints, immutable history fields and the successful response. This protects rejection before an audit write; it is not a durable transaction or rollback guarantee for a writer that partially appends and then throws. Two maintained HTTP regressions inject a pre-write audit failure, read back the unchanged transfer/history/audit, and retry the same token. Each successful retry creates exactly one history event and audit entry with matching actor, reason, request ID and timestamp. Executed node --test test/archiveAuditRetry.test.js with locked npm ci dependencies on Ubuntu 24.04.5 / Node 22.23.3 / npm 10.9.9. Original source dc550d29e0fc0463de352ac834a835acac0c9b88: 0 pass, 2 assertion failures showing changed state. Repaired source 941667b50bcbc7a12301050bcd35884e03d0a0d1: 2 pass, 0 fail, 0 skipped. Test blob d03c6f0f7a2bafce000c4ff2ea1ccbb813e783e6. Actual application and in-memory audit service with only the controlled failure injected; no full-suite or live-provider claim. Execution: https://github.com/woahwhattheheck/RemitFlow-Backend/actions/runs/37203697476 Artifact archive-audit-retry-before-after, ID 11304325273, SHA256 5621549eb5b7cbd0cd452533dcb38c5ba77b1978210dfe7f441c565ca282ad52. The validation workflow remains on its separate branch; existing CI is unchanged. --- src/services/transferService.js | 48 +++++++++-------- test/archiveAuditRetry.test.js | 94 +++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 23 deletions(-) create mode 100644 test/archiveAuditRetry.test.js diff --git a/src/services/transferService.js b/src/services/transferService.js index dc550d2..941667b 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -472,6 +472,18 @@ function archiveTransfer(id, optionsOrRequestId) { transfer.updatedAt, lastArchiveHistoryAt(transfer) ); + // Keep the current state and concurrency token intact if audit append fails. + auditService.addEntry({ + action: 'transfer.archived', + resourceId: transfer.id, + payload: { + archivedAt: timestamp, + actor: options.actor, + reason: options.reason, + }, + requestId: options.requestId, + }); + transfer.archivedAt = timestamp; transfer.lastArchivedAt = timestamp; transfer.updatedAt = timestamp; @@ -484,17 +496,6 @@ function archiveTransfer(id, optionsOrRequestId) { requestId: options.requestId || null, }); - auditService.addEntry({ - action: 'transfer.archived', - resourceId: transfer.id, - payload: { - archivedAt: timestamp, - actor: options.actor, - reason: options.reason, - }, - requestId: options.requestId, - }); - return transfer; } @@ -525,18 +526,7 @@ function unarchiveTransfer(id, optionsOrRequestId) { previousArchivedAt ); - transfer.archivedAt = null; - transfer.lastArchivedAt = previousArchivedAt; - transfer.updatedAt = timestamp; - - appendArchiveHistory(transfer, { - action: 'unarchive', - at: timestamp, - actor: options.actor, - reason: options.reason, - requestId: options.requestId || null, - }); - + // Keep the current state and concurrency token intact if audit append fails. auditService.addEntry({ action: 'transfer.unarchived', resourceId: transfer.id, @@ -549,6 +539,18 @@ function unarchiveTransfer(id, optionsOrRequestId) { requestId: options.requestId, }); + transfer.archivedAt = null; + transfer.lastArchivedAt = previousArchivedAt; + transfer.updatedAt = timestamp; + + appendArchiveHistory(transfer, { + action: 'unarchive', + at: timestamp, + actor: options.actor, + reason: options.reason, + requestId: options.requestId || null, + }); + return transfer; } diff --git a/test/archiveAuditRetry.test.js b/test/archiveAuditRetry.test.js new file mode 100644 index 0000000..d03c6f0 --- /dev/null +++ b/test/archiveAuditRetry.test.js @@ -0,0 +1,94 @@ +'use strict'; + +process.env.NODE_ENV = 'test'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { once } = require('node:events'); +const createApp = require('../src/app'); +const config = require('../src/config'); +const { reset } = require('../src/store'); +const transferService = require('../src/services/transferService'); +const auditService = require('../src/services/auditService'); + +for (const action of ['archive', 'unarchive']) { + test(`HTTP ${action} preserves state and token after audit failure, then retries once`, async (t) => { + reset(); + const transfer = transferService.createTransfer({ + senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR', + }); + if (action === 'unarchive') transferService.archiveTransfer(transfer.id); + const before = JSON.parse(JSON.stringify(transfer)); + const beforeAudit = JSON.parse(JSON.stringify(auditService.getEntriesForResource(transfer.id))); + const originalTokens = config.apiTokens; + const token = 'archive-audit-invented-regression-token'; + config.apiTokens = { [token]: ['transfers:read', 'transfers:write'] }; + const server = createApp().listen(0, '127.0.0.1'); + await once(server, 'listening'); + const url = `http://127.0.0.1:${server.address().port}/api/transfers/${transfer.id}`; + const reason = `retry ${action} after audit recovery`; + const requestId = `audit-retry-${action}`; + const headers = { + Authorization: `Bearer ${token}`, + 'Content-Type': 'application/json', + 'X-Request-Id': requestId, + Connection: 'close', + }; + const command = () => fetch(`${url}/${action}`, { + method: 'POST', headers, + body: JSON.stringify({ expectedUpdatedAt: before.updatedAt, reason }), + }); + const auditAction = action === 'archive' ? 'transfer.archived' : 'transfer.unarchived'; + let rejectedWrites = 0; + let rejectAudit = t.mock.method(auditService, 'addEntry', () => { + rejectedWrites += 1; + throw new Error('synthetic audit append unavailable'); + }); + try { + const rejected = await command(); + await rejected.json(); + assert.equal(rejected.status, 500); + assert.equal(rejectedWrites, 1); + const readback = await fetch(url, { headers }); + assert.equal(readback.status, 200); + assert.deepEqual(await readback.json(), before, + 'failed audit append must leave transfer state and history unchanged'); + assert.deepEqual(auditService.getEntriesForResource(transfer.id), beforeAudit); + + rejectAudit.mock.restore(); + rejectAudit = null; + const retried = await command(); + const after = await retried.json(); + assert.equal(retried.status, 200, 'the unchanged concurrency token must remain retryable'); + assert.ok(after.updatedAt > before.updatedAt); + assert.deepEqual(after.archiveHistory.slice(0, -1), before.archiveHistory); + assert.equal(after.archiveHistory.length, before.archiveHistory.length + 1); + const event = after.archiveHistory.at(-1); + assert.equal(event.action, action); + assert.equal(event.at, after.updatedAt); + assert.equal(event.reason, reason); + assert.equal(event.requestId, requestId); + if (action === 'archive') { + assert.equal(after.archivedAt, after.updatedAt); + assert.equal(after.lastArchivedAt, after.updatedAt); + } else { + assert.equal(after.archivedAt, null); + assert.equal(after.lastArchivedAt, before.archivedAt); + } + const entries = auditService.getEntriesForResource(transfer.id); + assert.equal(entries.length, beforeAudit.length + 1); + const matching = entries.filter((entry) => entry.action === auditAction); + assert.equal(matching.length, 1, 'successful retry must append exactly one audit entry'); + assert.equal(matching[0].requestId, requestId); + assert.equal(matching[0].payload.reason, reason); + assert.equal(matching[0].payload.actor, event.actor); + assert.equal(matching[0].payload[action === 'archive' ? 'archivedAt' : 'unarchivedAt'], after.updatedAt); + } finally { + if (rejectAudit) rejectAudit.mock.restore(); + config.apiTokens = originalTokens; + server.closeAllConnections(); + await new Promise((resolve) => server.close(resolve)); + reset(); + } + }); +}