From 5e9ee086f52125481392b94bf192296de06ccd20 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Thu, 24 Sep 2026 18:27:49 -0400 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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); + }); +}