From 8d4ad27386b082a7fbc54be934779bf1db276a57 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Thu, 24 Sep 2026 18:24:48 -0400 Subject: [PATCH 01/13] security(backend): enforce token scopes across transfer and admin routes Add a canonical scope catalog, re-check scopes at service boundaries, gate admin diagnostics on explicit admin:read (while mapping the legacy admin key), and introduce bulk transfer mutations with non-enumerating per-id errors so malformed and missing ids cannot be told apart. Closes #125 --- CHANGELOG.md | 5 + README.md | 11 +- docs/SCOPE_MATRIX.md | 49 ++++ src/config/index.js | 11 +- src/config/scopes.js | 73 +++++ src/controllers/adminController.js | 11 + src/controllers/auditController.js | 6 +- src/controllers/transferController.js | 32 ++- src/controllers/userController.js | 7 +- src/middleware/adminAuth.js | 48 +++- src/middleware/requireScope.js | 26 +- src/routes/transferRoutes.js | 3 + src/services/auditService.js | 9 +- src/services/transferService.js | 113 +++++++- src/services/userService.js | 12 +- src/utils/authz.js | 126 +++++++++ test/tokenScopes.test.js | 392 ++++++++++++++++++++++++++ 17 files changed, 878 insertions(+), 56 deletions(-) create mode 100644 docs/SCOPE_MATRIX.md create mode 100644 src/config/scopes.js create mode 100644 src/utils/authz.js create mode 100644 test/tokenScopes.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a8e4e0a..e8b543e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ When preparing a new release: 1. Review the `[Unreleased]` section and ensure all notable changes are documented. 2. Change the `[Unreleased]` heading to the new version number and current date (e.g., `## [1.0.0] - YYYY-MM-DD`). 3. Create a new empty `## [Unreleased]` section at the top of the file, with `### Added`, `### Changed`, `### Fixed`, etc., as needed. + +- Consistent token-scope enforcement across transfer, user, audit, and admin + surfaces (`admin:read` scope, service-boundary checks, `POST /api/transfers/bulk`). + See `docs/SCOPE_MATRIX.md`. Malformed and missing transfer ids now share one + non-enumerating `404 Transfer not found` response. 4. Update the `version` field in `package.json` to match the new version. 5. Commit these changes with a conventional commit message (e.g., `chore: release v1.0.0`). 6. Tag the commit with the new version (e.g., `git tag v1.0.0`) and push the changes and tags to the repository. diff --git a/README.md b/README.md index 4791414..a0435ae 100644 --- a/README.md +++ b/README.md @@ -63,13 +63,20 @@ Authorization: Bearer ### Scopes +Canonical catalog: `src/config/scopes.js`. Full route matrix: [`docs/SCOPE_MATRIX.md`](docs/SCOPE_MATRIX.md). + | Scope | Grants access to | |-------|-----------------| | `transfers:read` | `GET /api/transfers`, `GET /api/transfers/stats`, `GET /api/transfers/:id` | -| `transfers:write` | `POST /api/transfers`, `POST /api/transfers/:id/claim`, `POST /api/transfers/:id/cancel`, `POST /api/transfers/:id/archive`, `POST /api/transfers/:id/unarchive` | +| `transfers:write` | `POST /api/transfers`, `POST /api/transfers/bulk`, `POST /api/transfers/:id/claim`, `POST /api/transfers/:id/cancel`, `POST /api/transfers/:id/archive`, `POST /api/transfers/:id/unarchive` | | `users:read` | `GET /api/users`, `GET /api/users/:id` | | `users:write` | `POST /api/users` | | `audit:read` | `GET /api/audit` | +| `admin:read` | `GET /api/admin/diagnostics` (also granted by the legacy `ADMIN_API_KEY` / `X-Admin-Token`) | + +Scopes are enforced at the route **and** again at the service boundary. Missing, +malformed, and unknown transfer ids share one `404 Transfer not found` response +so callers cannot enumerate identifiers by status or message shape. ### Public endpoints (no token required) @@ -90,7 +97,7 @@ for local development only — rotate before deploying): | Demo token | Scopes | |------------|--------| -| `test-token-admin` | all scopes | +| `test-token-admin` | all scopes (including `admin:read`) | | `test-token-readonly` | `transfers:read`, `users:read`, `audit:read` | | `test-token-transfers` | `transfers:read`, `transfers:write` | diff --git a/docs/SCOPE_MATRIX.md b/docs/SCOPE_MATRIX.md new file mode 100644 index 0000000..2eea737 --- /dev/null +++ b/docs/SCOPE_MATRIX.md @@ -0,0 +1,49 @@ +# Scope matrix + +Canonical scope strings live in `src/config/scopes.js` (`SCOPES`, `SCOPE_MATRIX`). +Route middleware (`requireScope` / `adminAuth`) is the first gate; service helpers +in `src/utils/authz.js` re-check the same scopes so a forgotten middleware cannot +expose a privileged mutation. + +| Method | Path | Required scopes | Surface | +|--------|------|-----------------|---------| +| GET | `/api/transfers` | `transfers:read` | list | +| GET | `/api/transfers/stats` | `transfers:read` | direct | +| GET | `/api/transfers/:id` | `transfers:read` | direct | +| POST | `/api/transfers` | `transfers:write` | direct | +| POST | `/api/transfers/bulk` | `transfers:write` | bulk | +| POST | `/api/transfers/:id/claim` | `transfers:write` | direct | +| POST | `/api/transfers/:id/cancel` | `transfers:write` | direct | +| POST | `/api/transfers/:id/archive` | `transfers:write` | direct | +| POST | `/api/transfers/:id/unarchive` | `transfers:write` | direct | +| GET | `/api/users` | `users:read` | list | +| GET | `/api/users/:id` | `users:read` | direct | +| POST | `/api/users` | `users:write` | direct | +| GET | `/api/audit` | `audit:read` | list | +| GET | `/api/admin/diagnostics` | `admin:read` | admin | + +## Admin credentials + +`GET /api/admin/diagnostics` accepts either: + +1. `Authorization: Bearer ` whose catalog entry includes `admin:read`, or +2. Legacy `X-Admin-Token: ` / `Authorization: Bearer `, + which is mapped onto `[admin:read]` so the path stays scope-gated. + +## Non-enumeration + +Transfer lookups (direct and bulk) return the same `404 Transfer not found` +envelope for malformed ids and for unknown well-formed ids. Bulk per-id failures +use a stable `error.code` of `not_found` in both cases. + +## Bulk mutations + +```http +POST /api/transfers/bulk +Authorization: Bearer +Content-Type: application/json + +{ "action": "claim" | "cancel" | "archive" | "unarchive", "ids": ["txn_…", …] } +``` + +Up to 50 ids per call. The response is `{ results: [{ id, ok, transfer? | error? }] }`. diff --git a/src/config/index.js b/src/config/index.js index 41f9a71..e14c148 100644 --- a/src/config/index.js +++ b/src/config/index.js @@ -79,9 +79,16 @@ const config = { } // Default tokens for demo purposes return { - 'test-token-admin': ['transfers:read', 'transfers:write', 'users:read', 'users:write', 'audit:read'], + 'test-token-admin': [ + 'transfers:read', + 'transfers:write', + 'users:read', + 'users:write', + 'audit:read', + 'admin:read', + ], 'test-token-readonly': ['transfers:read', 'users:read', 'audit:read'], - 'test-token-transfers': ['transfers:read', 'transfers:write'] + 'test-token-transfers': ['transfers:read', 'transfers:write'], }; })(), }; diff --git a/src/config/scopes.js b/src/config/scopes.js new file mode 100644 index 0000000..5b526d7 --- /dev/null +++ b/src/config/scopes.js @@ -0,0 +1,73 @@ +'use strict'; + +/** + * Canonical action/resource scope catalog. + * + * Every secured route and service boundary must require one of these values. + * Unknown scope strings are rejected at configuration time so a typo cannot + * silently open an endpoint. + * + * Format: `:` + */ + +const SCOPES = Object.freeze({ + TRANSFERS_READ: 'transfers:read', + TRANSFERS_WRITE: 'transfers:write', + USERS_READ: 'users:read', + USERS_WRITE: 'users:write', + AUDIT_READ: 'audit:read', + ADMIN_READ: 'admin:read', +}); + +/** All known scope strings, for validation and docs. */ +const ALL_SCOPES = Object.freeze(Object.values(SCOPES)); + +/** + * Route → required scopes matrix. Used by docs and the scope-matrix test so + * the documented contract cannot drift from the mounted routers. + * + * `anyOf` is reserved for future OR-semantics; today every entry is AND. + * + * @type {ReadonlyArray<{ method: string, path: string, scopes: string[], surface: string }>} + */ +const SCOPE_MATRIX = Object.freeze([ + { method: 'GET', path: '/api/transfers', scopes: [SCOPES.TRANSFERS_READ], surface: 'list' }, + { method: 'GET', path: '/api/transfers/stats', scopes: [SCOPES.TRANSFERS_READ], surface: 'direct' }, + { method: 'GET', path: '/api/transfers/:id', scopes: [SCOPES.TRANSFERS_READ], surface: 'direct' }, + { method: 'POST', path: '/api/transfers', scopes: [SCOPES.TRANSFERS_WRITE], surface: 'direct' }, + { method: 'POST', path: '/api/transfers/:id/claim', scopes: [SCOPES.TRANSFERS_WRITE], surface: 'direct' }, + { method: 'POST', path: '/api/transfers/:id/cancel', scopes: [SCOPES.TRANSFERS_WRITE], surface: 'direct' }, + { method: 'POST', path: '/api/transfers/:id/archive', scopes: [SCOPES.TRANSFERS_WRITE], surface: 'direct' }, + { method: 'POST', path: '/api/transfers/:id/unarchive', scopes: [SCOPES.TRANSFERS_WRITE], surface: 'direct' }, + { method: 'POST', path: '/api/transfers/bulk', scopes: [SCOPES.TRANSFERS_WRITE], surface: 'bulk' }, + { method: 'GET', path: '/api/users', scopes: [SCOPES.USERS_READ], surface: 'list' }, + { method: 'GET', path: '/api/users/:id', scopes: [SCOPES.USERS_READ], surface: 'direct' }, + { method: 'POST', path: '/api/users', scopes: [SCOPES.USERS_WRITE], surface: 'direct' }, + { method: 'GET', path: '/api/audit', scopes: [SCOPES.AUDIT_READ], surface: 'list' }, + { method: 'GET', path: '/api/admin/diagnostics', scopes: [SCOPES.ADMIN_READ], surface: 'admin' }, +]); + +/** + * Assert every entry in `scopes` is a known catalog value. + * @param {string[]} scopes + * @param {string} [label] + * @returns {string[]} + */ +function assertKnownScopes(scopes, label = 'scope') { + if (!Array.isArray(scopes)) { + throw new Error(`${label} must be an array of scope strings`); + } + for (const scope of scopes) { + if (!ALL_SCOPES.includes(scope)) { + throw new Error(`Unknown ${label}: ${scope}`); + } + } + return scopes; +} + +module.exports = { + SCOPES, + ALL_SCOPES, + SCOPE_MATRIX, + assertKnownScopes, +}; diff --git a/src/controllers/adminController.js b/src/controllers/adminController.js index 870e10e..b666009 100644 --- a/src/controllers/adminController.js +++ b/src/controllers/adminController.js @@ -3,12 +3,23 @@ const config = require('../config'); const userService = require('../services/userService'); const transferService = require('../services/transferService'); +const { SCOPES, assertScopes, authFromRequest } = require('../utils/authz'); /** * GET /api/admin/diagnostics * Returns system diagnostics, active configurations, and usage statistics. + * + * Requires the explicit `admin:read` scope (enforced at the route by adminAuth + * and re-checked here so the privileged aggregate cannot be reached through a + * future helper that forgets the middleware). */ function getDiagnostics(req, res) { + const auth = authFromRequest(req); + assertScopes(auth, SCOPES.ADMIN_READ); + + // Admin diagnostics aggregates across tenants; call services without a + // caller auth context so the read-scope gates do not reject an admin-only + // token that intentionally has no transfers:read / users:read grants. const transferStats = transferService.getStats(); const userCount = userService.listUsers().length; diff --git a/src/controllers/auditController.js b/src/controllers/auditController.js index 799b428..987b178 100644 --- a/src/controllers/auditController.js +++ b/src/controllers/auditController.js @@ -2,6 +2,7 @@ const auditService = require('../services/auditService'); const { buildHistoryPage } = require('../utils/historyPage'); +const { authFromRequest } = require('../utils/authz'); /** * Audit log controllers. @@ -20,14 +21,15 @@ function listAuditEntries(req, res) { : String(req.query.resourceId); const filters = { resourceId }; + const auth = authFromRequest(req); const { items, envelope } = buildHistoryPage({ req, collection: 'audit', filters, defaultOrder: 'desc', - query: (args) => auditService.queryEntries({ resourceId, ...args }), - countTotal: () => auditService.countEntries(resourceId), + query: (args) => auditService.queryEntries({ resourceId, ...args, auth }), + countTotal: () => auditService.countEntries(resourceId, auth), resolvePosition: (seq) => auditService.positionKeyAt(seq, resourceId), }); diff --git a/src/controllers/transferController.js b/src/controllers/transferController.js index 4fb2e72..73a2158 100644 --- a/src/controllers/transferController.js +++ b/src/controllers/transferController.js @@ -4,6 +4,7 @@ const transferService = require('../services/transferService'); const { buildHistoryPage } = require('../utils/historyPage'); const idempotencyService = require('../services/idempotencyService'); const ApiError = require('../utils/ApiError'); +const { authFromRequest } = require('../utils/authz'); /** Upper bound on a client-supplied key, so the map cannot be grown without limit. */ const MAX_IDEMPOTENCY_KEY_LENGTH = 255; @@ -64,7 +65,7 @@ function createTransfer(req, res) { actor: req.token, key, fingerprint, - }); + }, authFromRequest(req)); // A replay answers 201 with the original transfer, exactly as the first call // did. Replaying the stored result means replaying all of it; downgrading the @@ -100,13 +101,14 @@ function listTransfers(req, res) { archived, }); + const auth = authFromRequest(req); const { items, envelope } = buildHistoryPage({ req, collection: 'transfers', filters, defaultOrder: 'asc', - query: (args) => transferService.queryTransfers({ ...filters, ...args }), - countTotal: () => transferService.listTransfers(filters).length, + query: (args) => transferService.queryTransfers({ ...filters, ...args, auth }), + countTotal: () => transferService.listTransfers(filters, auth).length, resolvePosition: (seq) => transferService.positionKeyAt(seq), }); @@ -118,7 +120,7 @@ function listTransfers(req, res) { * Return aggregate transfer statistics. */ function getStats(req, res) { - res.json(transferService.getStats()); + res.json(transferService.getStats(authFromRequest(req))); } /** @@ -126,7 +128,7 @@ function getStats(req, res) { * Fetch a single transfer by id. */ function getTransfer(req, res) { - const transfer = transferService.getTransferOrThrow(req.params.id); + const transfer = transferService.getTransferOrThrow(req.params.id, authFromRequest(req)); res.json(transfer); } @@ -135,7 +137,7 @@ function getTransfer(req, res) { * Mark a transfer as claimed by the recipient. */ function claimTransfer(req, res) { - const transfer = transferService.claimTransfer(req.params.id, req.id); + const transfer = transferService.claimTransfer(req.params.id, req.id, authFromRequest(req)); res.json(transfer); } @@ -144,7 +146,7 @@ function claimTransfer(req, res) { * Cancel a pending transfer. */ function cancelTransfer(req, res) { - const transfer = transferService.cancelTransfer(req.params.id, req.id); + const transfer = transferService.cancelTransfer(req.params.id, req.id, authFromRequest(req)); res.json(transfer); } @@ -153,7 +155,7 @@ function cancelTransfer(req, res) { * Archive a transfer, hiding it from default list results. */ function archiveTransfer(req, res) { - const transfer = transferService.archiveTransfer(req.params.id); + const transfer = transferService.archiveTransfer(req.params.id, authFromRequest(req)); res.json(transfer); } @@ -162,10 +164,21 @@ function archiveTransfer(req, res) { * Unarchive a transfer, restoring it to default list results. */ function unarchiveTransfer(req, res) { - const transfer = transferService.unarchiveTransfer(req.params.id); + const transfer = transferService.unarchiveTransfer(req.params.id, authFromRequest(req)); res.json(transfer); } +/** + * POST /api/transfers/bulk + * Apply one write action to many transfer ids. + */ +function bulkMutate(req, res) { + const action = req.body && req.body.action; + const ids = req.body && req.body.ids; + const result = transferService.bulkMutate(action, ids, req.id, authFromRequest(req)); + res.json(result); +} + module.exports = { createTransfer, listTransfers, @@ -175,4 +188,5 @@ module.exports = { cancelTransfer, archiveTransfer, unarchiveTransfer, + bulkMutate, }; diff --git a/src/controllers/userController.js b/src/controllers/userController.js index fa0153a..4bf9dc7 100644 --- a/src/controllers/userController.js +++ b/src/controllers/userController.js @@ -2,6 +2,7 @@ const userService = require('../services/userService'); const { parsePagination } = require('../utils/pagination'); +const { authFromRequest } = require('../utils/authz'); /** * User controllers. @@ -12,7 +13,7 @@ const { parsePagination } = require('../utils/pagination'); * List users with limit/offset pagination. */ function listUsers(req, res) { - const all = userService.listUsers(); + const all = userService.listUsers(authFromRequest(req)); const { limit, offset } = parsePagination(req.query); const users = all.slice(offset, offset + limit); res.json({ total: all.length, count: users.length, limit, offset, users }); @@ -23,7 +24,7 @@ function listUsers(req, res) { * Fetch a single user by id. */ function getUser(req, res) { - const user = userService.getUserOrThrow(req.params.id); + const user = userService.getUserOrThrow(req.params.id, authFromRequest(req)); res.json(user); } @@ -32,7 +33,7 @@ function getUser(req, res) { * Create a new user. */ function createUser(req, res) { - const user = userService.createUser(req.body, req.id); + const user = userService.createUser(req.body, req.id, authFromRequest(req)); res.status(201).json(user); } diff --git a/src/middleware/adminAuth.js b/src/middleware/adminAuth.js index 7828ad7..91d2bf1 100644 --- a/src/middleware/adminAuth.js +++ b/src/middleware/adminAuth.js @@ -2,28 +2,52 @@ const config = require('../config'); const ApiError = require('../utils/ApiError'); +const { SCOPES, hasAllScopes } = require('../utils/authz'); /** - * Middleware to restrict route access to authorized admins. - * Expects the admin key in either the 'X-Admin-Token' header or - * as a Bearer token in the 'Authorization' header. + * Restrict route access to callers that hold the explicit `admin:read` scope. + * + * Accepted credentials (checked in order): + * 1. `Authorization: Bearer ` whose catalog entry includes admin:read + * 2. Legacy admin key via `X-Admin-Token` or `Authorization: Bearer ` + * — the key is treated as an actor that holds `[admin:read]` so admin paths + * stay scope-gated even when the shared key is used. + * + * Failures are non-enumerating: missing, unknown, and under-scoped callers all + * receive the same 401/403 vocabulary used by requireScope. */ function adminAuth(req, res, next) { - let token = req.headers['x-admin-token']; + const authHeader = req.headers.authorization; + let bearer = null; + if (typeof authHeader === 'string' && authHeader.startsWith('Bearer ')) { + bearer = authHeader.slice('Bearer '.length).trim(); + } - // Check Authorization header for Bearer token - if (!token && req.headers.authorization) { - const parts = req.headers.authorization.split(' '); - if (parts.length === 2 && parts[0] === 'Bearer') { - token = parts[1]; + // Prefer a scoped API token when one is presented. + if (bearer && config.apiTokens[bearer]) { + const tokenScopes = config.apiTokens[bearer]; + if (!hasAllScopes(tokenScopes, [SCOPES.ADMIN_READ])) { + return next(ApiError.forbidden('Insufficient token scopes')); } + req.token = bearer; + req.tokenScopes = tokenScopes; + return next(); } - if (!token || token !== config.adminApiKey) { - return next(new ApiError(401, 'Unauthorized')); + const headerKey = req.headers['x-admin-token']; + const legacyKey = (typeof headerKey === 'string' && headerKey.trim() !== '') + ? headerKey.trim() + : bearer; + + if (!legacyKey || legacyKey !== config.adminApiKey) { + // Do not reveal whether the failure was "no credential" vs "wrong key". + return next(ApiError.unauthorized('Unauthorized')); } - next(); + // Legacy admin key maps onto the explicit admin:read scope. + req.token = legacyKey; + req.tokenScopes = [SCOPES.ADMIN_READ]; + return next(); } module.exports = adminAuth; diff --git a/src/middleware/requireScope.js b/src/middleware/requireScope.js index b7d511f..68477b9 100644 --- a/src/middleware/requireScope.js +++ b/src/middleware/requireScope.js @@ -2,15 +2,25 @@ const config = require('../config'); const ApiError = require('../utils/ApiError'); +const { assertKnownScopes } = require('../config/scopes'); +const { hasAllScopes } = require('../utils/authz'); /** * Middleware to require a specific set of scopes from the provided API token. * Validates the Bearer token in the Authorization header. - * + * + * Required scopes are checked against the canonical catalog at middleware + * construction time so a typo cannot ship as an open route. + * * @param {string[]} requiredScopes - Array of scopes required to access the endpoint. * @returns {import('express').RequestHandler} */ function requireScope(requiredScopes) { + const needed = assertKnownScopes( + Array.isArray(requiredScopes) ? requiredScopes : [requiredScopes], + 'required scope' + ); + return (req, res, next) => { const authHeader = req.headers.authorization; @@ -18,21 +28,23 @@ function requireScope(requiredScopes) { return next(ApiError.unauthorized('Missing or invalid Authorization header')); } - const token = authHeader.split(' ')[1]; + const token = authHeader.slice('Bearer '.length).trim(); + if (!token) { + return next(ApiError.unauthorized('Missing or invalid Authorization header')); + } + const tokenScopes = config.apiTokens[token]; if (!tokenScopes) { + // Same 401 as a missing header: do not confirm whether the secret looked + // "almost right". return next(ApiError.unauthorized('Invalid API token')); } - // Check if the token has all required scopes - const hasAllScopes = requiredScopes.every(scope => tokenScopes.includes(scope)); - - if (!hasAllScopes) { + if (!hasAllScopes(tokenScopes, needed)) { return next(ApiError.forbidden('Insufficient token scopes')); } - // Attach token and scopes to request for downstream usage if needed req.token = token; req.tokenScopes = tokenScopes; diff --git a/src/routes/transferRoutes.js b/src/routes/transferRoutes.js index 58eac20..93893b8 100644 --- a/src/routes/transferRoutes.js +++ b/src/routes/transferRoutes.js @@ -23,6 +23,9 @@ router.get('/', requireScope(['transfers:read']), asyncHandler(transferControlle // GET /api/transfers/stats (declared before /:id so it is not captured) router.get('/stats', requireScope(['transfers:read']), asyncHandler(transferController.getStats)); +// POST /api/transfers/bulk (declared before /:id so "bulk" is not captured) +router.post('/bulk', requireScope(['transfers:write']), asyncHandler(transferController.bulkMutate)); + // GET /api/transfers/:id router.get('/:id', requireScope(['transfers:read']), asyncHandler(transferController.getTransfer)); diff --git a/src/services/auditService.js b/src/services/auditService.js index 4e51403..6dee839 100644 --- a/src/services/auditService.js +++ b/src/services/auditService.js @@ -3,6 +3,7 @@ const { newId } = require('../utils/ids'); const { OrderedIndex } = require('../utils/orderedIndex'); const config = require('../config'); +const { SCOPES, assertScopes } = require('../utils/authz'); /** * Audit log service. @@ -69,7 +70,8 @@ function addEntry({ action, resourceId, payload = {}, requestId } = {}) { * Return all audit entries, newest first. * @returns {Array} */ -function getEntries() { +function getEntries(auth) { + assertScopes(auth, SCOPES.AUDIT_READ); return auditIndex.records.map((record) => record.item).reverse(); } @@ -109,7 +111,9 @@ function queryEntries({ afterSeq = null, skip = 0, maxScan = config.pagination.maxScan, + auth, } = {}) { + assertScopes(auth, SCOPES.AUDIT_READ); return auditIndex.scan({ group: resourceId == null || resourceId === '' ? null : String(resourceId), order, @@ -138,7 +142,8 @@ function positionKeyAt(seq, resourceId) { * @param {string} [resourceId] * @returns {number} */ -function countEntries(resourceId) { +function countEntries(resourceId, auth) { + assertScopes(auth, SCOPES.AUDIT_READ); if (resourceId == null || resourceId === '') return auditIndex.size; return auditIndex.recordsFor(String(resourceId)).length; } diff --git a/src/services/transferService.js b/src/services/transferService.js index 1517f62..a326854 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -9,6 +9,13 @@ const stellarService = require('./stellarService'); const idempotencyService = require('./idempotencyService'); const auditService = require('./auditService'); const config = require('../config'); +const { + SCOPES, + assertScopes, + transferNotFoundError, + isWellFormedTransferId, + normaliseBulkIds, +} = require('../utils/authz'); // Keep lifecycle timestamps strictly increasing even when multiple operations // happen within the same millisecond (common in tests and API batches). @@ -34,7 +41,8 @@ function nextTimestamp(previous) { * @param {boolean} [filters.archived] - if true, return only archived; if false, exclude archived (default) * @returns {Array} */ -function listTransfers(filters = {}) { +function listTransfers(filters = {}, auth) { + assertScopes(auth, SCOPES.TRANSFERS_READ); const match = buildTransferFilter(filters); return Array.from(store.transfers.values()).filter(match); } @@ -130,7 +138,9 @@ function queryTransfers({ afterSeq = null, skip = 0, maxScan = config.pagination.maxScan, + auth, } = {}) { + assertScopes(auth, SCOPES.TRANSFERS_READ); return store.transferIndex.scan({ match: buildTransferFilter({ status, search, archived }), order, @@ -158,7 +168,8 @@ function positionKeyAt(seq) { * Reports per-status counts and total send volume grouped by currency. * @returns {{ total: number, byStatus: object, volumeByCurrency: object }} */ -function getStats() { +function getStats(auth) { + assertScopes(auth, SCOPES.TRANSFERS_READ); const transfers = Array.from(store.transfers.values()); const byStatus = {}; @@ -181,10 +192,15 @@ function getStats() { * @param {string} id * @returns {object} */ -function getTransferOrThrow(id) { +function getTransferOrThrow(id, auth) { + assertScopes(auth, SCOPES.TRANSFERS_READ); + // Malformed and missing ids share one 404 so callers cannot enumerate. + if (!isWellFormedTransferId(id)) { + throw transferNotFoundError(); + } const transfer = store.transfers.get(id); if (!transfer) { - throw ApiError.notFound(`Transfer not found: ${id}`); + throw transferNotFoundError(); } return transfer; } @@ -206,7 +222,8 @@ function getTransferOrThrow(id) { * @param {{ actor: string, key: string, fingerprint: string }} [idempotency] * @returns {object} */ -function createTransfer(data, requestId, idempotency) { +function createTransfer(data, requestId, idempotency, auth) { + assertScopes(auth, SCOPES.TRANSFERS_WRITE); if (idempotency) { const outcome = idempotencyService.begin( store.idempotency, @@ -319,8 +336,11 @@ function transition(transfer, nextStatus) { * @param {string} [requestId] - optional correlation id for audit logging * @returns {object} */ -function claimTransfer(id, requestId) { - const transfer = getTransferOrThrow(id); +function claimTransfer(id, requestId, auth) { + assertScopes(auth, SCOPES.TRANSFERS_WRITE); + // Pass null auth into getTransferOrThrow: write scope already asserted, and + // re-asserting transfers:read would reject write-only tokens if introduced. + const transfer = getTransferOrThrow(id, null); transition(transfer, TRANSFER_STATUS.CLAIMED); transfer.claimableBalanceId = stellarService.createClaimableBalanceId(); @@ -340,8 +360,9 @@ function claimTransfer(id, requestId) { * @param {string} [requestId] - optional correlation id for audit logging * @returns {object} */ -function cancelTransfer(id, requestId) { - const transfer = getTransferOrThrow(id); +function cancelTransfer(id, requestId, auth) { + assertScopes(auth, SCOPES.TRANSFERS_WRITE); + const transfer = getTransferOrThrow(id, null); transition(transfer, TRANSFER_STATUS.CANCELLED); auditService.addEntry({ @@ -361,8 +382,9 @@ function cancelTransfer(id, requestId) { * @param {string} id * @returns {object} */ -function archiveTransfer(id) { - const transfer = getTransferOrThrow(id); +function archiveTransfer(id, auth) { + assertScopes(auth, SCOPES.TRANSFERS_WRITE); + const transfer = getTransferOrThrow(id, null); if (!transfer.archivedAt) { const timestamp = nextTimestamp(transfer.updatedAt); transfer.archivedAt = timestamp; @@ -376,8 +398,9 @@ function archiveTransfer(id) { * @param {string} id * @returns {object} */ -function unarchiveTransfer(id) { - const transfer = getTransferOrThrow(id); +function unarchiveTransfer(id, auth) { + assertScopes(auth, SCOPES.TRANSFERS_WRITE); + const transfer = getTransferOrThrow(id, null); if (!transfer.archivedAt) { throw ApiError.conflict(`Transfer is not archived: ${id}`); } @@ -386,6 +409,68 @@ function unarchiveTransfer(id) { return transfer; } + +/** + * Supported bulk mutation actions. Kept as a closed set so the route cannot be + * turned into an arbitrary RPC surface. + */ +const BULK_ACTIONS = Object.freeze({ + claim: claimTransfer, + cancel: cancelTransfer, + archive: (id, _requestId, auth) => archiveTransfer(id, auth), + unarchive: (id, _requestId, auth) => unarchiveTransfer(id, auth), +}); + +/** + * Apply one write action to many transfer ids. + * + * Per-id failures never leak whether the id was malformed, missing, or + * rejected by a lifecycle rule beyond a stable `code` string. A missing write + * scope fails the whole call before any mutation runs. + * + * @param {string} action + * @param {string[]} ids + * @param {string} [requestId] + * @param {{ actor?: string, scopes?: string[] }|null} [auth] + * @returns {{ results: Array }} + */ +function bulkMutate(action, ids, requestId, auth) { + assertScopes(auth, SCOPES.TRANSFERS_WRITE); + const handler = BULK_ACTIONS[action]; + if (!handler) { + throw ApiError.badRequest(`Unsupported bulk action: ${action}`, { + allowed: Object.keys(BULK_ACTIONS), + }); + } + const normalised = normaliseBulkIds(ids); + const results = normalised.map((id) => { + try { + const transfer = handler(id, requestId, auth); + return { id, ok: true, transfer }; + } catch (err) { + const status = err && err.statusCode ? err.statusCode : 500; + // Collapse not-found / malformed into one code. Lifecycle conflicts keep + // their own code so a client can retry sensibly without learning whether + // an unknown id existed. + let code = 'error'; + if (status === 404) code = 'not_found'; + else if (status === 409) code = 'conflict'; + else if (status === 403) code = 'forbidden'; + else if (status === 400) code = 'bad_request'; + return { + id, + ok: false, + error: { + code, + status, + message: status === 404 ? transferNotFoundError().message : (err.message || 'error'), + }, + }; + } + }); + return { results }; +} + module.exports = { listTransfers, normaliseTransferFilters, @@ -398,4 +483,6 @@ module.exports = { cancelTransfer, archiveTransfer, unarchiveTransfer, + bulkMutate, + BULK_ACTIONS: Object.keys(BULK_ACTIONS), }; diff --git a/src/services/userService.js b/src/services/userService.js index 1c2895d..d59ef83 100644 --- a/src/services/userService.js +++ b/src/services/userService.js @@ -4,6 +4,7 @@ const { store } = require('../store'); const { prefixedId } = require('../utils/ids'); const ApiError = require('../utils/ApiError'); const auditService = require('./auditService'); +const { SCOPES, assertScopes } = require('../utils/authz'); /** * User management backed by the in-memory store. @@ -13,7 +14,8 @@ const auditService = require('./auditService'); * Return all users. * @returns {Array} */ -function listUsers() { +function listUsers(auth) { + assertScopes(auth, SCOPES.USERS_READ); return Array.from(store.users.values()); } @@ -31,10 +33,11 @@ function findUser(id) { * @param {string} id * @returns {object} */ -function getUserOrThrow(id) { +function getUserOrThrow(id, auth) { + assertScopes(auth, SCOPES.USERS_READ); const user = findUser(id); if (!user) { - throw ApiError.notFound(`User not found: ${id}`); + throw ApiError.notFound('User not found'); } return user; } @@ -45,7 +48,8 @@ function getUserOrThrow(id) { * @param {string} [requestId] - optional correlation id for audit logging * @returns {object} */ -function createUser(data, requestId) { +function createUser(data, requestId, auth) { + assertScopes(auth, SCOPES.USERS_WRITE); const user = { id: prefixedId('usr'), name: data.name, diff --git a/src/utils/authz.js b/src/utils/authz.js new file mode 100644 index 0000000..9ea3167 --- /dev/null +++ b/src/utils/authz.js @@ -0,0 +1,126 @@ +'use strict'; + +const ApiError = require('./ApiError'); +const { SCOPES, assertKnownScopes } = require('../config/scopes'); + +/** + * Authorization helpers shared by middleware and service boundaries. + * + * Design notes: + * - Route middleware is the first gate; service helpers are the second so a + * forgotten middleware cannot expose a privileged mutation. + * - Missing, malformed, and unauthorized resource lookups share one 404 + * message so callers cannot enumerate identifiers by status/body shape. + * - Internal callers (seed, unit tests) omit `auth` and skip the checks. + */ + +/** Maximum ids accepted by a single bulk mutation. */ +const MAX_BULK_IDS = 50; + +/** + * Transfer ids are `txn_`. Anything else is treated as "not found" + * rather than 400, so probing the id format does not yield a distinct signal. + */ +const TRANSFER_ID_RE = /^txn_[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +/** Stable non-enumerating not-found message for transfers. */ +const TRANSFER_NOT_FOUND_MESSAGE = 'Transfer not found'; + +/** + * Build an auth context from an Express request that has already passed + * requireScope / adminAuth. + * @param {import('express').Request} req + * @returns {{ actor: string|null, scopes: string[] }} + */ +function authFromRequest(req) { + return { + actor: req.token || null, + scopes: Array.isArray(req.tokenScopes) ? req.tokenScopes.slice() : [], + }; +} + +/** + * @param {string[]|undefined|null} tokenScopes + * @param {string[]} required + * @returns {boolean} + */ +function hasAllScopes(tokenScopes, required) { + if (!Array.isArray(tokenScopes) || !Array.isArray(required)) return false; + return required.every((scope) => tokenScopes.includes(scope)); +} + +/** + * Require every scope in `required`. When `auth` is omitted the check is a + * no-op so trusted internal callers stay ergonomic. + * + * @param {{ scopes?: string[] }|null|undefined} auth + * @param {string|string[]} required + * @throws {ApiError} 403 when the caller is present but under-scoped + */ +function assertScopes(auth, required) { + if (auth == null) return; + const needed = assertKnownScopes( + Array.isArray(required) ? required : [required], + 'required scope' + ); + if (!hasAllScopes(auth.scopes, needed)) { + throw ApiError.forbidden('Insufficient token scopes'); + } +} + +/** + * Non-enumerating transfer 404. Same status and message whether the id is + * missing, malformed, or (in future) out of the caller's tenancy. + * @returns {ApiError} + */ +function transferNotFoundError() { + return ApiError.notFound(TRANSFER_NOT_FOUND_MESSAGE); +} + +/** + * @param {*} id + * @returns {boolean} + */ +function isWellFormedTransferId(id) { + return typeof id === 'string' && TRANSFER_ID_RE.test(id); +} + +/** + * Validate and normalise a bulk id list. + * @param {*} raw + * @returns {string[]} + * @throws {ApiError} 400 on shape problems (not on unknown ids) + */ +function normaliseBulkIds(raw) { + if (!Array.isArray(raw)) { + throw ApiError.badRequest('ids must be an array of transfer ids'); + } + if (raw.length === 0) { + throw ApiError.badRequest('ids must not be empty'); + } + if (raw.length > MAX_BULK_IDS) { + throw ApiError.badRequest(`ids may contain at most ${MAX_BULK_IDS} entries`, { + max: MAX_BULK_IDS, + }); + } + const ids = []; + for (const value of raw) { + if (typeof value !== 'string' || value.trim() === '') { + throw ApiError.badRequest('each id must be a non-empty string'); + } + ids.push(value.trim()); + } + return ids; +} + +module.exports = { + SCOPES, + MAX_BULK_IDS, + TRANSFER_NOT_FOUND_MESSAGE, + authFromRequest, + hasAllScopes, + assertScopes, + transferNotFoundError, + isWellFormedTransferId, + normaliseBulkIds, +}; diff --git a/test/tokenScopes.test.js b/test/tokenScopes.test.js new file mode 100644 index 0000000..f32250d --- /dev/null +++ b/test/tokenScopes.test.js @@ -0,0 +1,392 @@ +'use strict'; + +/** + * Scope enforcement regressions for issue #125. + * + * Covers: + * - Scope matrix (documented route → required scopes) + * - Cross-account / cross-surface isolation (transfers token cannot touch admin/audit/users writes) + * - Bulk-route authorization and non-enumerating per-id results + * - Malformed-ID handling (identical 404 to missing ids) + * - Audit authorization + * - Service-boundary enforcement (direct service calls with under-scoped auth) + */ + +const { test, before, after, beforeEach } = require('node:test'); +const assert = require('node:assert/strict'); + +process.env.NODE_ENV = 'test'; + +const createApp = require('../src/app'); +const { reset } = require('../src/store'); +const config = require('../src/config'); +const { SCOPES, ALL_SCOPES, SCOPE_MATRIX } = require('../src/config/scopes'); +const transferService = require('../src/services/transferService'); +const auditService = require('../src/services/auditService'); +const { TRANSFER_NOT_FOUND_MESSAGE } = require('../src/utils/authz'); + +let server; +let baseUrl; + +before(() => { + const app = createApp(); + return new Promise((resolve) => { + server = app.listen(0, () => { + baseUrl = `http://127.0.0.1:${server.address().port}`; + resolve(); + }); + }); +}); + +after(() => { + if (server) server.close(); +}); + +beforeEach(() => { + reset(); +}); + +function authHeader(token) { + return { Authorization: `Bearer ${token}` }; +} + +async function fetchJson(path, options = {}) { + const res = await fetch(`${baseUrl}${path}`, options); + let body; + try { + body = await res.json(); + } catch { + body = null; + } + return { status: res.status, body }; +} + +async function createTransfer(token, suffix) { + const { status, body } = await fetchJson('/api/transfers', { + method: 'POST', + headers: { + ...authHeader(token), + 'Content-Type': 'application/json', + 'Idempotency-Key': `idem-scopes-${suffix}`, + }, + body: JSON.stringify({ + senderName: 'Alice', + recipientName: 'Bob', + amount: 100, + from: 'USD', + to: 'EUR', + }), + }); + assert.equal(status, 201, JSON.stringify(body)); + return body; +} + +// ─── Scope matrix ───────────────────────────────────────────────────────────── + +test('scope catalog only contains documented resource:action values', () => { + for (const scope of ALL_SCOPES) { + assert.match(scope, /^[a-z]+:[a-z]+$/); + } + assert.ok(ALL_SCOPES.includes(SCOPES.ADMIN_READ)); + assert.ok(ALL_SCOPES.includes(SCOPES.TRANSFERS_WRITE)); +}); + +test('scope matrix lists every secured surface exactly once per route', () => { + const keys = SCOPE_MATRIX.map((row) => `${row.method} ${row.path}`); + assert.equal(keys.length, new Set(keys).size); + const surfaces = new Set(SCOPE_MATRIX.map((row) => row.surface)); + for (const needed of ['direct', 'list', 'bulk', 'admin']) { + assert.ok(surfaces.has(needed), `missing surface ${needed}`); + } + for (const row of SCOPE_MATRIX) { + for (const scope of row.scopes) { + assert.ok(ALL_SCOPES.includes(scope), `unknown scope ${scope} on ${row.path}`); + } + } +}); + +test('demo admin token holds every catalogued scope including admin:read', () => { + const scopes = config.apiTokens['test-token-admin']; + for (const scope of ALL_SCOPES) { + assert.ok(scopes.includes(scope), `admin demo token missing ${scope}`); + } +}); + +test('scope matrix: readonly token is rejected on every write and admin row', async () => { + const created = await createTransfer('test-token-admin', 'matrix-setup'); + + const cases = [ + { method: 'POST', path: '/api/transfers', body: { senderName: 'A', recipientName: 'B', amount: 1, from: 'USD', to: 'EUR' }, key: 'm1' }, + { method: 'POST', path: `/api/transfers/${created.id}/claim`, body: null, key: null }, + { method: 'POST', path: `/api/transfers/${created.id}/cancel`, body: null, key: null }, + { method: 'POST', path: '/api/transfers/bulk', body: { action: 'claim', ids: [created.id] }, key: null }, + { method: 'POST', path: '/api/users', body: { name: 'X', email: 'x@example.com' }, key: null }, + { method: 'GET', path: '/api/admin/diagnostics', body: null, key: null }, + ]; + + for (const item of cases) { + const headers = { ...authHeader('test-token-readonly') }; + if (item.body) headers['Content-Type'] = 'application/json'; + if (item.key) headers['Idempotency-Key'] = `idem-matrix-${item.key}`; + const { status, body } = await fetchJson(item.path, { + method: item.method, + headers, + body: item.body ? JSON.stringify(item.body) : undefined, + }); + assert.equal(status, 403, `${item.method} ${item.path} => ${status} ${JSON.stringify(body)}`); + assert.equal(body.error.message, 'Insufficient token scopes'); + } +}); + +// ─── Cross-account / cross-surface ──────────────────────────────────────────── + +test('cross-account: transfers-only token cannot read audit or users or admin', async () => { + const token = 'test-token-transfers'; + + for (const path of ['/api/audit', '/api/users', '/api/admin/diagnostics']) { + const { status, body } = await fetchJson(path, { headers: authHeader(token) }); + if (path === '/api/admin/diagnostics') { + // adminAuth returns 401 for an API token that lacks admin:read (token is + // recognised but under-scoped → 403). + assert.ok([401, 403].includes(status), `${path} => ${status}`); + if (status === 403) { + assert.equal(body.error.message, 'Insufficient token scopes'); + } + } else { + assert.equal(status, 403, `${path} => ${status}`); + assert.equal(body.error.message, 'Insufficient token scopes'); + } + } +}); + +test('cross-account: transfers-only token cannot create users', async () => { + const { status, body } = await fetchJson('/api/users', { + method: 'POST', + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + body: JSON.stringify({ name: 'Eve', email: 'eve@example.com' }), + }); + assert.equal(status, 403); + assert.equal(body.error.message, 'Insufficient token scopes'); +}); + +test('cross-account: admin diagnostics rejects a foreign bearer that is not the admin key and lacks admin:read', async () => { + const { status, body } = await fetchJson('/api/admin/diagnostics', { + headers: authHeader('test-token-readonly'), + }); + assert.equal(status, 403); + assert.equal(body.error.message, 'Insufficient token scopes'); +}); + +test('admin path accepts scoped API token with admin:read', async () => { + const { status, body } = await fetchJson('/api/admin/diagnostics', { + headers: authHeader('test-token-admin'), + }); + assert.equal(status, 200); + assert.ok(body.system); + assert.ok(body.stats); +}); + +test('admin path still accepts legacy X-Admin-Token and maps it to admin:read', async () => { + const { status, body } = await fetchJson('/api/admin/diagnostics', { + headers: { 'X-Admin-Token': config.adminApiKey }, + }); + assert.equal(status, 200); + assert.ok(body.system); +}); + +// ─── Bulk route ─────────────────────────────────────────────────────────────── + +test('bulk route requires transfers:write', async () => { + const { status } = await fetchJson('/api/transfers/bulk', { + method: 'POST', + headers: { ...authHeader('test-token-readonly'), 'Content-Type': 'application/json' }, + body: JSON.stringify({ action: 'claim', ids: ['txn_00000000-0000-4000-8000-000000000001'] }), + }); + assert.equal(status, 403); +}); + +test('bulk route claims and cancels with non-enumerating per-id errors', async () => { + const a = await createTransfer('test-token-transfers', 'bulk-a'); + const b = await createTransfer('test-token-transfers', 'bulk-b'); + + const { status, body } = await fetchJson('/api/transfers/bulk', { + method: 'POST', + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + body: JSON.stringify({ + action: 'claim', + ids: [a.id, 'txn_not-a-uuid', 'totally-wrong', b.id], + }), + }); + assert.equal(status, 200); + assert.equal(body.results.length, 4); + assert.equal(body.results[0].ok, true); + assert.equal(body.results[0].transfer.status, 'claimed'); + assert.equal(body.results[1].ok, false); + assert.equal(body.results[1].error.code, 'not_found'); + assert.equal(body.results[1].error.status, 404); + assert.equal(body.results[1].error.message, TRANSFER_NOT_FOUND_MESSAGE); + assert.equal(body.results[2].ok, false); + assert.equal(body.results[2].error.code, 'not_found'); + assert.equal(body.results[2].error.message, TRANSFER_NOT_FOUND_MESSAGE); + assert.equal(body.results[3].ok, true); + + // Identical error shape for malformed vs missing: no enumeration signal. + assert.deepEqual( + { ...body.results[1].error, message: body.results[1].error.message }, + { ...body.results[2].error, message: body.results[2].error.message } + ); +}); + +test('bulk cancel returns conflict code without leaking sibling outcomes', async () => { + const transfer = await createTransfer('test-token-transfers', 'bulk-cancel'); + await fetchJson(`/api/transfers/${transfer.id}/claim`, { + method: 'POST', + headers: authHeader('test-token-transfers'), + }); + + const { status, body } = await fetchJson('/api/transfers/bulk', { + method: 'POST', + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + body: JSON.stringify({ action: 'cancel', ids: [transfer.id] }), + }); + assert.equal(status, 200); + assert.equal(body.results[0].ok, false); + assert.equal(body.results[0].error.code, 'conflict'); + assert.equal(body.results[0].error.status, 409); +}); + +// ─── Malformed ID ───────────────────────────────────────────────────────────── + +test('malformed and missing transfer ids both return identical 404 bodies', async () => { + const probes = [ + 'not-an-id', + 'txn_short', + 'txn_00000000-0000-4000-8000-000000000099', // well-formed but absent + 'txn_../etc/passwd', + ]; + + const shapes = []; + for (const id of probes) { + const path = `/api/transfers/${encodeURIComponent(id)}`; + const { status, body } = await fetchJson(path, { + headers: authHeader('test-token-readonly'), + }); + assert.equal(status, 404, `id=${id}`); + assert.equal(body.error.status, 404); + assert.equal(body.error.message, TRANSFER_NOT_FOUND_MESSAGE); + shapes.push(body.error.message); + } + assert.ok(shapes.every((m) => m === shapes[0])); +}); + +test('malformed id on claim matches missing-id 404 (non-enumerating)', async () => { + const malformed = await fetchJson('/api/transfers/txn_nope/claim', { + method: 'POST', + headers: authHeader('test-token-transfers'), + }); + const missing = await fetchJson( + '/api/transfers/txn_00000000-0000-4000-8000-000000000042/claim', + { + method: 'POST', + headers: authHeader('test-token-transfers'), + } + ); + assert.equal(malformed.status, 404); + assert.equal(missing.status, 404); + assert.equal(malformed.body.error.message, missing.body.error.message); + assert.equal(malformed.body.error.message, TRANSFER_NOT_FOUND_MESSAGE); +}); + +// ─── Audit authorization ────────────────────────────────────────────────────── + +test('audit authorization: missing token → 401, wrong scope → 403, ok → 200', async () => { + const noToken = await fetchJson('/api/audit'); + assert.equal(noToken.status, 401); + + const wrong = await fetchJson('/api/audit', { + headers: authHeader('test-token-transfers'), + }); + assert.equal(wrong.status, 403); + assert.equal(wrong.body.error.message, 'Insufficient token scopes'); + + const ok = await fetchJson('/api/audit', { + headers: authHeader('test-token-readonly'), + }); + assert.equal(ok.status, 200); + assert.ok(Array.isArray(ok.body.entries)); +}); + +test('audit service boundary rejects under-scoped auth context', () => { + assert.throws( + () => auditService.getEntries({ actor: 'x', scopes: [SCOPES.TRANSFERS_READ] }), + (err) => err.statusCode === 403 + ); + assert.doesNotThrow(() => + auditService.getEntries({ actor: 'x', scopes: [SCOPES.AUDIT_READ] }) + ); + // Internal callers without auth still work. + assert.doesNotThrow(() => auditService.getEntries()); +}); + +// ─── Service-boundary enforcement ───────────────────────────────────────────── + +test('transfer service boundary rejects under-scoped auth on write and read', () => { + const transfer = transferService.createTransfer({ + senderName: 'A', + recipientName: 'B', + amount: 10, + from: 'USD', + to: 'EUR', + }); + + assert.throws( + () => + transferService.claimTransfer(transfer.id, 'req', { + actor: 'ro', + scopes: [SCOPES.TRANSFERS_READ], + }), + (err) => err.statusCode === 403 && /Insufficient token scopes/.test(err.message) + ); + + assert.throws( + () => + transferService.getTransferOrThrow(transfer.id, { + actor: 'w', + scopes: [SCOPES.TRANSFERS_WRITE], + }), + (err) => err.statusCode === 403 + ); + + assert.doesNotThrow(() => + transferService.getTransferOrThrow(transfer.id, { + actor: 'r', + scopes: [SCOPES.TRANSFERS_READ], + }) + ); +}); + +test('service boundary: malformed id throws the same not-found error as a miss', () => { + const malformed = () => transferService.getTransferOrThrow('txn_bad', { + actor: 'r', + scopes: [SCOPES.TRANSFERS_READ], + }); + const missing = () => + transferService.getTransferOrThrow('txn_00000000-0000-4000-8000-000000000077', { + actor: 'r', + scopes: [SCOPES.TRANSFERS_READ], + }); + + for (const fn of [malformed, missing]) { + assert.throws(fn, (err) => { + return err.statusCode === 404 && err.message === TRANSFER_NOT_FOUND_MESSAGE; + }); + } +}); + +test('regression: a transfers:write token still cannot hit admin diagnostics', async () => { + const { status, body } = await fetchJson('/api/admin/diagnostics', { + headers: authHeader('test-token-transfers'), + }); + assert.equal(status, 403); + assert.equal(body.error.message, 'Insufficient token scopes'); +}); From e7320944e2cc6012e099c8d883d01ab41a43d4ec Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sat, 3 Oct 2026 04:30:22 -0400 Subject: [PATCH 02/13] fix(transfers): restrict bulk dispatch to documented string actions Reject inherited action names and non-string values before handler lookup. Preserve scope checks, unsupported-string errors, and valid bulk behavior. Five HTTP regressions reproduce false success, a coerced cancellation, and a malformed-action 500 on the prior source. Full npm test: 281 pass. --- src/services/transferService.js | 7 ++++-- test/tokenScopes.test.js | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/services/transferService.js b/src/services/transferService.js index a326854..e82ec44 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -436,12 +436,15 @@ const BULK_ACTIONS = Object.freeze({ */ function bulkMutate(action, ids, requestId, auth) { assertScopes(auth, SCOPES.TRANSFERS_WRITE); - const handler = BULK_ACTIONS[action]; - if (!handler) { + if (typeof action !== 'string') { + throw ApiError.badRequest('action must be a string'); + } + if (!Object.hasOwn(BULK_ACTIONS, action)) { throw ApiError.badRequest(`Unsupported bulk action: ${action}`, { allowed: Object.keys(BULK_ACTIONS), }); } + const handler = BULK_ACTIONS[action]; const normalised = normaliseBulkIds(ids); const results = normalised.map((id) => { try { diff --git a/test/tokenScopes.test.js b/test/tokenScopes.test.js index f32250d..fdaf047 100644 --- a/test/tokenScopes.test.js +++ b/test/tokenScopes.test.js @@ -205,6 +205,47 @@ test('bulk route requires transfers:write', async () => { assert.equal(status, 403); }); +for (const [index, action] of [ + 'constructor', + 'toString', + '__proto__', + ['cancel'], + { toString: null }, +].entries()) { + test(`bulk route rejects action ${JSON.stringify(action)} without mutation`, async () => { + const transfer = await createTransfer('test-token-transfers', `bulk-invalid-${index}`); + const { status, body } = await fetchJson('/api/transfers/bulk', { + method: 'POST', + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + body: JSON.stringify({ action, ids: [transfer.id] }), + }); + assert.equal(status, 400, JSON.stringify(body)); + assert.equal(body.error.status, 400); + + const current = await fetchJson(`/api/transfers/${transfer.id}`, { + headers: authHeader('test-token-transfers'), + }); + assert.equal(current.status, 200); + assert.deepEqual(current.body, transfer); + }); +} + +test('bulk route preserves documented archive, unarchive and cancel actions', async () => { + const transfer = await createTransfer('test-token-transfers', 'bulk-valid-actions'); + for (const action of ['archive', 'unarchive', 'cancel']) { + const { status, body } = await fetchJson('/api/transfers/bulk', { + method: 'POST', + headers: { ...authHeader('test-token-transfers'), 'Content-Type': 'application/json' }, + body: JSON.stringify({ action, ids: [transfer.id] }), + }); + assert.equal(status, 200, JSON.stringify(body)); + assert.equal(body.results[0].ok, true); + assert.equal(body.results[0].transfer.id, transfer.id); + assert.equal(body.results[0].transfer.status, action === 'cancel' ? 'cancelled' : 'pending'); + assert.equal(Boolean(body.results[0].transfer.archivedAt), action === 'archive'); + } +}); + test('bulk route claims and cancels with non-enumerating per-id errors', async () => { const a = await createTransfer('test-token-transfers', 'bulk-a'); const b = await createTransfer('test-token-transfers', 'bulk-b'); From c147a2e7a1f828b68ec387ee77baa908bc8a66c5 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sat, 3 Oct 2026 06:25:57 -0400 Subject: [PATCH 03/13] fix(auth): reject invalid configured API token maps Fail startup for malformed or invalid supplied token maps instead of enabling public demo credentials. Validate configured scopes using the existing catalog and keep token lookup limited to explicit entries. Preserve absent-variable demo mode, valid maps, and the legacy admin credential behavior. Validation: 16 new failure-mode cases failed on original source; npm test now passes all 298 tests without skips. Actual local production-mode Express runs confirmed malformed/empty maps previously granted demo admin access and array-shaped maps granted token 0. All invalid configurations now stop startup; valid-map and absent-variable controls still pass. No dependency changes or external provider calls. --- README.md | 9 ++++++++ src/config/index.js | 46 ++++++++++++++++++++++++--------------- test/config.test.js | 45 +++++++++++++++++++++++++++++++++++++- test/requireScope.test.js | 10 +++++++++ 4 files changed, 92 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index a0435ae..366c8bd 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,15 @@ strings to their scope arrays: API_TOKENS='{"my-production-token":["transfers:read","transfers:write","users:read","users:write","audit:read"],"reporting-token":["transfers:read","audit:read"]}' ``` +An explicitly supplied value must be a JSON object with non-empty token keys, +no leading or trailing key whitespace, and arrays of scopes from the catalog. +Malformed JSON, blank values, invalid shapes, and unknown scopes stop startup +with a configuration error that does not print credentials. They never enable +the demo tokens. An empty object `{}` is valid and disables all API-token +credentials; the separately configured legacy admin key retains its behavior. +Only configured own entries are tokens, so inherited names such as `constructor` +are unknown unless explicitly configured. + If `API_TOKENS` is not set, the server starts with three **demo tokens** (safe for local development only — rotate before deploying): diff --git a/src/config/index.js b/src/config/index.js index e14c148..09cb099 100644 --- a/src/config/index.js +++ b/src/config/index.js @@ -1,6 +1,7 @@ 'use strict'; require('dotenv').config(); +const { assertKnownScopes } = require('./scopes'); /** * Centralized application configuration. @@ -71,25 +72,36 @@ const config = { apiTokens: (() => { try { - if (process.env.API_TOKENS) { - return JSON.parse(process.env.API_TOKENS); + // Demo credentials apply only when no token map was supplied. A broken + // deployment value must never silently enable the public demo tokens. + const configured = process.env.API_TOKENS === undefined ? { + 'test-token-admin': [ + 'transfers:read', + 'transfers:write', + 'users:read', + 'users:write', + 'audit:read', + 'admin:read', + ], + 'test-token-readonly': ['transfers:read', 'users:read', 'audit:read'], + 'test-token-transfers': ['transfers:read', 'transfers:write'], + } : JSON.parse(process.env.API_TOKENS); + + if (configured === null || typeof configured !== 'object' || Array.isArray(configured)) { + throw new Error('Invalid token map'); } - } catch (err) { - console.warn('Failed to parse API_TOKENS env var, falling back to defaults'); + + const tokens = Object.create(null); + for (const [token, scopes] of Object.entries(configured)) { + if (token === '' || token.trim() !== token) throw new Error('Invalid token key'); + tokens[token] = assertKnownScopes(scopes, 'configured scope').slice(); + } + return tokens; + } catch { + // Neither parser diagnostics nor scope-validation errors may expose + // configured token keys or values in startup logs. + throw new Error('API_TOKENS must be a JSON object mapping non-empty, unpadded token strings to arrays of known scopes'); } - // Default tokens for demo purposes - return { - 'test-token-admin': [ - 'transfers:read', - 'transfers:write', - 'users:read', - 'users:write', - 'audit:read', - 'admin:read', - ], - 'test-token-readonly': ['transfers:read', 'users:read', 'audit:read'], - 'test-token-transfers': ['transfers:read', 'transfers:write'], - }; })(), }; diff --git a/test/config.test.js b/test/config.test.js index 45c5d39..3e58833 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -2,6 +2,50 @@ const { test } = require('node:test'); const assert = require('node:assert/strict'); +const { spawnSync } = require('node:child_process'); +const path = require('node:path'); + +const invalidTokenMaps = [ + ['malformed JSON', '{"confidential-fixture-token":'], + ['empty text', ''], + ['whitespace', ' \n\t '], + ['null', 'null'], + ['number', '1'], + ['string', '"token"'], + ['array with privileged scopes', '[["admin:read","users:write"]]'], + ['non-array scopes', '{"confidential-fixture-token":"admin:read"}'], + ['null scopes', '{"confidential-fixture-token":null}'], + ['non-string scope', '{"confidential-fixture-token":[null]}'], + ['unknown scope', '{"confidential-fixture-token":["confidential-fixture-scope"]}'], + ['empty token', '{"":["admin:read"]}'], + ['padded token', '{" confidential-fixture-token ":["admin:read"]}'], +]; + +for (const [label, value] of invalidTokenMaps) { + test(`config rejects supplied API_TOKENS ${label} before application startup`, () => { + const child = spawnSync(process.execPath, ['-e', "require('./src/app')()"], { + cwd: path.join(__dirname, '..'), + env: { ...process.env, NODE_ENV: 'production', API_TOKENS: value }, + encoding: 'utf8', + }); + assert.notEqual(child.status, 0, 'invalid explicit credentials must not start a demo-credential app'); + assert.match(child.stderr, /API_TOKENS/); + assert.doesNotMatch(child.stderr, /confidential-fixture-token|confidential-fixture-scope/, + 'configuration errors must not print token keys or supplied scope values'); + }); +} + +test('config accepts known scopes and explicit empty maps without adding demo credentials', () => { + for (const value of ['{}', '{"fixture-token":["admin:read"],"read-token":["users:read"],"denied-token":[]}']) { + const child = spawnSync(process.execPath, ['-e', "console.log(JSON.stringify(require('./src/config').apiTokens))"], { + cwd: path.join(__dirname, '..'), + env: { ...process.env, API_TOKENS: value }, + encoding: 'utf8', + }); + assert.equal(child.status, 0, child.stderr); + assert.deepEqual(JSON.parse(child.stdout), JSON.parse(value)); + } +}); test('config loads default database connection pooling options', () => { // Clear require cache for the config module to load it fresh @@ -85,4 +129,3 @@ test('config respects cache environment variables', () => { delete require.cache[require.resolve('../src/config')]; } }); - diff --git a/test/requireScope.test.js b/test/requireScope.test.js index 33b723d..be8db88 100644 --- a/test/requireScope.test.js +++ b/test/requireScope.test.js @@ -113,6 +113,16 @@ test('GET /api/users returns 401 when token is unknown', async () => { assert.match(body.error.message, /Invalid API token/i); }); +for (const token of ['constructor', 'toString', '__proto__']) { + test(`unconfigured inherited token name ${token} stays unknown on user and admin routes`, async () => { + for (const route of ['/api/users', '/api/admin/diagnostics']) { + const { status, body } = await fetchJson(route, { headers: authHeader(token) }); + assert.equal(status, 401, `${route} must not recognize an inherited object property as a token`); + assert.equal(body.error.status, 401); + } + }); +} + test('POST /api/transfers returns 401 when token is unknown', async () => { const { status, body } = await fetchJson('/api/transfers', { method: 'POST', From b0c161f1c9c8200d762e5810ba82c15995a1be0a Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sat, 3 Oct 2026 22:47:01 -0400 Subject: [PATCH 04/13] docs(auth): clarify explicit service contexts and admin credential precedence --- README.md | 9 ++++++--- docs/SCOPE_MATRIX.md | 44 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 366c8bd..ef04b62 100644 --- a/README.md +++ b/README.md @@ -74,9 +74,12 @@ Canonical catalog: `src/config/scopes.js`. Full route matrix: [`docs/SCOPE_MATRI | `audit:read` | `GET /api/audit` | | `admin:read` | `GET /api/admin/diagnostics` (also granted by the legacy `ADMIN_API_KEY` / `X-Admin-Token`) | -Scopes are enforced at the route **and** again at the service boundary. Missing, -malformed, and unknown transfer ids share one `404 Transfer not found` response -so callers cannot enumerate identifiers by status or message shape. +HTTP controllers pass `authFromRequest(req)` so scopes are enforced at the route +**and** again at the service boundary. Trusted internal callers can omit that +context, so new request adapters must pass it explicitly; see the +[service integration contract](docs/SCOPE_MATRIX.md#service-integration). +Missing, malformed, and unknown transfer ids share one `404 Transfer not found` +response so callers cannot enumerate identifiers by status or message shape. ### Public endpoints (no token required) diff --git a/docs/SCOPE_MATRIX.md b/docs/SCOPE_MATRIX.md index 2eea737..d0e1ff3 100644 --- a/docs/SCOPE_MATRIX.md +++ b/docs/SCOPE_MATRIX.md @@ -1,9 +1,10 @@ # Scope matrix Canonical scope strings live in `src/config/scopes.js` (`SCOPES`, `SCOPE_MATRIX`). -Route middleware (`requireScope` / `adminAuth`) is the first gate; service helpers -in `src/utils/authz.js` re-check the same scopes so a forgotten middleware cannot -expose a privileged mutation. +Route middleware (`requireScope` / `adminAuth`) is the first gate. HTTP controllers +pass an explicit auth context to the service helpers in `src/utils/authz.js`, which +re-check the same scopes. New request handlers must preserve both steps; see +[Service integration](#service-integration). | Method | Path | Required scopes | Surface | |--------|------|-----------------|---------| @@ -22,6 +23,39 @@ expose a privileged mutation. | GET | `/api/audit` | `audit:read` | list | | GET | `/api/admin/diagnostics` | `admin:read` | admin | +## Service integration + +Service checks run when an `auth` context is supplied. `assertScopes(auth, required)` +deliberately returns without checking when `auth` is `null` or `undefined`, so +trusted internal callers such as seed code retain their existing behavior. +Omitting the argument is not an anonymous-request denial. + +For every HTTP adapter, keep the route's `requireScope` / `adminAuth` middleware +and derive the service context with `authFromRequest(req)`. This copies the +middleware-populated `req.tokenScopes`; a request with no scope array produces +an explicit empty array and is rejected by the service with +`403 Insufficient token scopes`. Do not substitute client-supplied body or query +fields for that context. + +Pass the context in the operation's existing argument position. For example, +the archive controller calls: + +```js +const { authFromRequest } = require('../utils/authz'); + +const transfer = transferService.archiveTransfer( + req.params.id, + authFromRequest(req) +); +``` + +The create controller instead passes it as the fourth argument, after the +payload, request ID and idempotency descriptor. Follow the corresponding +controller in `src/controllers/transferController.js` when adding an adapter. +The service guard therefore protects a missing route scope check only when +the adapter still supplies an explicit request context; it is not a replacement +for request authentication. + ## Admin credentials `GET /api/admin/diagnostics` accepts either: @@ -30,6 +64,10 @@ expose a privileged mutation. 2. Legacy `X-Admin-Token: ` / `Authorization: Bearer `, which is mapped onto `[admin:read]` so the path stays scope-gated. +A recognized API bearer token is checked first. If it lacks `admin:read`, the +request returns `403` even when a valid legacy `X-Admin-Token` is also present. +Choose the intended credential instead of combining both authentication modes. + ## Non-enumeration Transfer lookups (direct and bulk) return the same `404 Transfer not found` From 3f83f3907a4b750a128fe54022880d10b4d9ee50 Mon Sep 17 00:00:00 2001 From: woahwhattheheck <293286387+woahwhattheheck@users.noreply.github.com> Date: Sun, 4 Oct 2026 09:22:57 +0000 Subject: [PATCH 05/13] Keep unexpected bulk provider errors private [skip ci] --- .../bulk-error-boundary-20261004.md | 19 ++++ src/services/transferService.js | 6 +- test/bulkErrorBoundary.test.js | 100 ++++++++++++++++++ 3 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 docs/validation/bulk-error-boundary-20261004.md create mode 100644 test/bulkErrorBoundary.test.js diff --git a/docs/validation/bulk-error-boundary-20261004.md b/docs/validation/bulk-error-boundary-20261004.md new file mode 100644 index 0000000..cc36b44 --- /dev/null +++ b/docs/validation/bulk-error-boundary-20261004.md @@ -0,0 +1,19 @@ +# Bulk error boundary + +Bulk transfer actions now use the same error-disclosure boundary as the central HTTP handler: only an intentional ApiError controls the public status/message; other thrown values become 500 / Internal server error. Canonical missing-transfer errors, intended API conflicts and scope checks remain intact. Null and string throws remain per-item failures instead of crashing the batch. Independent items still execute. + +This change does not alter transfer state transitions or claim to roll back provider failures. + +## Executed evidence + +Base `b0c161f1c9c8200d762e5810ba82c15995a1be0a`; source before `e82ec442dec7a190eae24f0efddcaabfceff4a17`, after `41b91b665e183547619af829b2b5e82d739728e3`; new test `aeceda6e5cf69b31f20488af440442e941c5d16d`. Runtime `v22.23.3`. + +Original production with new HTTP regressions: 2 pass / 6 fail, exit 1. Failures cover ordinary and status-bearing provider errors, null/string throws, and single-item/bulk message parity. + +Repaired source with maintained token-scope suite and new HTTP regressions: 33 pass / 0 fail / 0 cancelled / 0 skipped, exit 0. Existing tests unchanged. + +```sh +NODE_ENV=test node --test --test-reporter=tap test/tokenScopes.test.js test/bulkErrorBoundary.test.js +``` + +Real createApp, Express authentication, service and loopback HTTP with synthetic provider failures and existing mock settlement. No live credentials, external provider, durable storage, full-suite or settlement rollback claim. diff --git a/src/services/transferService.js b/src/services/transferService.js index e82ec44..41b91b6 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -451,7 +451,8 @@ function bulkMutate(action, ids, requestId, auth) { const transfer = handler(id, requestId, auth); return { id, ok: true, transfer }; } catch (err) { - const status = err && err.statusCode ? err.statusCode : 500; + const isApiError = err instanceof ApiError; + const status = isApiError ? err.statusCode : 500; // Collapse not-found / malformed into one code. Lifecycle conflicts keep // their own code so a client can retry sensibly without learning whether // an unknown id existed. @@ -466,7 +467,8 @@ function bulkMutate(action, ids, requestId, auth) { error: { code, status, - message: status === 404 ? transferNotFoundError().message : (err.message || 'error'), + message: status === 404 ? transferNotFoundError().message + : (isApiError ? err.message : 'Internal server error'), }, }; } diff --git a/test/bulkErrorBoundary.test.js b/test/bulkErrorBoundary.test.js new file mode 100644 index 0000000..aeceda6 --- /dev/null +++ b/test/bulkErrorBoundary.test.js @@ -0,0 +1,100 @@ +'use strict'; + +const { test, before, after, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +process.env.NODE_ENV = 'test'; +const createApp = require('../src/app'); +const { reset } = require('../src/store'); +const transfers = require('../src/services/transferService'); +const stellar = require('../src/services/stellarService'); +const ApiError = require('../src/utils/ApiError'); +const payload = { senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }; +const original = stellar.createClaimableBalanceId; +const secret = 'synthetic-provider-secret@db.invalid'; +let server; +let origin; + +before(() => new Promise(resolve => { + server = createApp().listen(0, '127.0.0.1', () => { + origin = `http://127.0.0.1:${server.address().port}`; + resolve(); + }); +})); +after(() => new Promise((resolve, reject) => server.close(error => error ? reject(error) : resolve()))); +beforeEach(() => reset()); +afterEach(() => { stellar.createClaimableBalanceId = original; }); + +async function request(path, body, token = 'test-token-admin') { + const response = await fetch(origin + path, { + method: 'POST', + headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json' }, + body: body === undefined ? undefined : JSON.stringify(body), + }); + return { status: response.status, body: await response.json() }; +} + +const failures = [ + ['ordinary provider error', () => new Error(secret)], + ['provider object claiming forbidden status', () => Object.assign(new Error(secret), { statusCode: 403 })], + ['provider object claiming successful status', () => Object.assign(new Error(secret), { statusCode: 200 })], + ['null rejection', () => null], + ['string rejection', () => secret], +]; + +for (const [name, failure] of failures) { + test(`bulk hides ${name} and continues independent rows`, async () => { + const first = transfers.createTransfer(payload); + const second = transfers.createTransfer(payload); + let calls = 0; + stellar.createClaimableBalanceId = (...args) => { + calls += 1; + if (calls === 1) throw failure(); + return original(...args); + }; + const result = await request('/api/transfers/bulk', { action: 'claim', ids: [first.id, second.id] }); + assert.equal(result.status, 200, JSON.stringify(result.body)); + assert.equal(result.body.results.length, 2); + assert.deepEqual(result.body.results[0].error, { + code: 'error', status: 500, message: 'Internal server error', + }); + assert.equal(result.body.results[0].ok, false); + assert.equal(result.body.results[1].ok, true); + assert.equal(result.body.results[1].transfer.status, 'claimed'); + assert.equal(calls, 2); + assert.equal(JSON.stringify(result.body).includes(secret), false); + }); +} + +test('expected ApiError conflict keeps its intentional public message', async () => { + const transfer = transfers.createTransfer(payload); + stellar.createClaimableBalanceId = () => { throw ApiError.conflict('Settlement is already pending'); }; + const result = await request('/api/transfers/bulk', { action: 'claim', ids: [transfer.id] }); + assert.equal(result.status, 200); + assert.deepEqual(result.body.results[0].error, { + code: 'conflict', status: 409, message: 'Settlement is already pending', + }); +}); + +test('scope denial happens before any provider invocation', async () => { + const transfer = transfers.createTransfer(payload); + let calls = 0; + stellar.createClaimableBalanceId = () => { calls += 1; throw new Error(secret); }; + const result = await request('/api/transfers/bulk', { action: 'claim', ids: [transfer.id] }, 'test-token-readonly'); + assert.equal(result.status, 403); + assert.equal(result.body.error.message, 'Insufficient token scopes'); + assert.equal(calls, 0); + assert.equal(transfer.status, 'pending'); +}); + +test('bulk uses the same unexpected-error message as the single-item route', async () => { + const direct = transfers.createTransfer(payload); + const bulk = transfers.createTransfer(payload); + stellar.createClaimableBalanceId = () => { throw new Error(secret); }; + const one = await request(`/api/transfers/${direct.id}/claim`); + const many = await request('/api/transfers/bulk', { action: 'claim', ids: [bulk.id] }); + assert.equal(one.status, 500); + assert.equal(many.status, 200); + assert.equal(many.body.results[0].error.status, one.body.error.status); + assert.equal(many.body.results[0].error.message, one.body.error.message); + assert.equal(one.body.error.message, 'Internal server error'); +}); From 368f8b5fba12056beb580622a03742363e43733d Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 08:02:36 -0400 Subject: [PATCH 06/13] Keep failed transfer claims retryable Validate the lifecycle transition before adapter work and apply state only after it succeeds. Preserve write-scope checks, terminal conflicts, and independent bulk results. Focused production-module execution: two recovery cases failed before and pass after, including retry, redaction, terminal-state and under-scope controls. Node 24.19.0; existing simulated adapter with injected failure, external no-env dotenv loader. No HTTP, live-chain or full-suite result is claimed. --- src/services/transferService.js | 15 +++++-- test/claimRecovery.test.js | 71 +++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 test/claimRecovery.test.js diff --git a/src/services/transferService.js b/src/services/transferService.js index 41b91b6..16a77bd 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -313,18 +313,23 @@ function createTransferUnchecked(data, requestId, idempotency) { } /** - * Move a transfer to a new status if the transition is allowed. + * Validate a transition without changing the stored transfer. * @param {object} transfer * @param {string} nextStatus - * @returns {object} + * @returns {void} */ -function transition(transfer, nextStatus) { +function assertTransition(transfer, nextStatus) { const allowed = TRANSFER_TRANSITIONS[transfer.status] || []; if (!allowed.includes(nextStatus)) { throw ApiError.conflict( `Cannot change transfer from ${transfer.status} to ${nextStatus}` ); } +} + +/** Move a transfer to an allowed status. */ +function transition(transfer, nextStatus) { + assertTransition(transfer, nextStatus); transfer.status = nextStatus; transfer.updatedAt = nextTimestamp(transfer.updatedAt); return transfer; @@ -341,8 +346,10 @@ function claimTransfer(id, requestId, auth) { // Pass null auth into getTransferOrThrow: write scope already asserted, and // re-asserting transfers:read would reject write-only tokens if introduced. const transfer = getTransferOrThrow(id, null); + assertTransition(transfer, TRANSFER_STATUS.CLAIMED); + const claimableBalanceId = stellarService.createClaimableBalanceId(); transition(transfer, TRANSFER_STATUS.CLAIMED); - transfer.claimableBalanceId = stellarService.createClaimableBalanceId(); + transfer.claimableBalanceId = claimableBalanceId; auditService.addEntry({ action: 'transfer.claimed', diff --git a/test/claimRecovery.test.js b/test/claimRecovery.test.js new file mode 100644 index 0000000..2febe2e --- /dev/null +++ b/test/claimRecovery.test.js @@ -0,0 +1,71 @@ +'use strict'; + +const { test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +process.env.NODE_ENV = 'test'; +const { reset } = require('../src/store'); +const transfers = require('../src/services/transferService'); +const audit = require('../src/services/auditService'); +const stellar = require('../src/services/stellarService'); + +const payload = { senderName: 'Alice', recipientName: 'Bob', amount: 100, from: 'USD', to: 'EUR' }; +const writeAuth = { scopes: ['transfers:write'] }; +const readAuth = { scopes: ['transfers:read'] }; +const originalClaim = stellar.createClaimableBalanceId; + +beforeEach(() => reset()); +afterEach(() => { stellar.createClaimableBalanceId = originalClaim; }); + +test('direct failed claim preserves the pending record and can be retried with write scope', () => { + const transfer = transfers.createTransfer(payload, undefined, undefined, writeAuth); + const before = structuredClone(transfer); + const failure = new Error('controlled adapter failure'); + let calls = 0; + stellar.createClaimableBalanceId = () => { calls += 1; throw failure; }; + + assert.throws(() => transfers.claimTransfer(transfer.id, 'failed-claim', writeAuth), error => error === failure); + assert.deepEqual(transfer, before); + assert.equal(audit.getEntriesForResource(transfer.id).filter(entry => entry.action === 'transfer.claimed').length, 0); + + stellar.createClaimableBalanceId = (...args) => { calls += 1; return originalClaim(...args); }; + const recovered = transfers.claimTransfer(transfer.id, 'retry-claim', writeAuth); + assert.equal(recovered, transfer); + assert.equal(recovered.status, 'claimed'); + assert.equal(typeof recovered.claimableBalanceId, 'string'); + assert.ok(recovered.updatedAt > before.updatedAt); + assert.equal(audit.getEntriesForResource(transfer.id).filter(entry => entry.action === 'transfer.claimed').length, 1); + assert.equal(calls, 2); + + assert.throws(() => transfers.claimTransfer(transfer.id, 'already-claimed', writeAuth), error => error.statusCode === 409); + assert.throws(() => transfers.claimTransfer(transfer.id, 'under-scoped', readAuth), error => error.statusCode === 403); + assert.equal(calls, 2); +}); + +test('bulk failed claim stays retryable while successful independent rows remain committed', () => { + const first = transfers.createTransfer(payload, undefined, undefined, writeAuth); + const second = transfers.createTransfer(payload, undefined, undefined, writeAuth); + const before = structuredClone(first); + let calls = 0; + stellar.createClaimableBalanceId = (...args) => { + calls += 1; + if (calls === 1) throw new Error('private adapter detail'); + return originalClaim(...args); + }; + + const batch = transfers.bulkMutate('claim', [first.id, second.id], 'bulk-claim', writeAuth); + assert.deepEqual(batch.results[0].error, { code: 'error', status: 500, message: 'Internal server error' }); + assert.equal(batch.results[0].ok, false); + assert.equal(batch.results[1].ok, true); + assert.equal(second.status, 'claimed'); + assert.deepEqual(first, before); + assert.equal(calls, 2); + + const retried = transfers.bulkMutate('claim', [first.id], 'bulk-retry', writeAuth); + assert.equal(retried.results[0].ok, true); + assert.equal(first.status, 'claimed'); + assert.equal(calls, 3); + const terminal = transfers.bulkMutate('claim', [first.id, second.id], 'bulk-terminal', writeAuth); + assert.ok(terminal.results.every(row => !row.ok && row.error.status === 409)); + assert.throws(() => transfers.bulkMutate('claim', [first.id], 'bulk-under-scoped', readAuth), error => error.statusCode === 403); + assert.equal(calls, 3); +}); From 05a6279b894d3c110f9db539cb25b50e3a42e5c3 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 08:07:24 -0400 Subject: [PATCH 07/13] fix(audit): enforce explicit scopes on resource lookups getEntriesForResource ignored a supplied authorization context while the other audit readers enforced audit:read. Accept the optional context and check it before inspecting the resource, using the existing shared guard. Keep the documented null/undefined trusted internal behavior, resource filtering and newest-first ordering. On parent 368f8b5fba12056beb580622a03742363e43733d the actual service returned an audit row to an explicit transfers:read context while its getEntries sibling returned 403. One new maintained regression fails on the original and passes after the repair; its two existing resource-reader controls also pass (3/3, zero skipped). It covers uniform scope denial for present/missing/empty resources, audit:read access, trusted callers, and unchanged ordering. Executed with Node 24.19.0 on actual audit/index/authz/config modules and synthetic in-memory entries. Only unavailable dotenv initialization used an external inert no-.env shim; source/config/dependencies unchanged. The current HTTP controller already uses guarded query/count readers; this change makes the exported direct service reader consistent. No HTTP, full-suite or CI result is claimed. Existing transfer recovery preserved. Reproduce with installed project dependencies: node --test --test-name-pattern='^getEntriesForResource' test/audit.test.js --- src/services/auditService.js | 4 +++- test/audit.test.js | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/services/auditService.js b/src/services/auditService.js index 6dee839..2a41cd4 100644 --- a/src/services/auditService.js +++ b/src/services/auditService.js @@ -78,9 +78,11 @@ function getEntries(auth) { /** * Return only the entries for a specific resource id. * @param {string} resourceId + * @param {{ scopes?: string[] }|null} [auth] - optional service authorization context * @returns {Array} */ -function getEntriesForResource(resourceId) { +function getEntriesForResource(resourceId, auth) { + assertScopes(auth, SCOPES.AUDIT_READ); // A nullish id matches no resource. Guarded explicitly because the index // treats a null group key as "the whole index". if (resourceId == null || resourceId === '') return []; diff --git a/test/audit.test.js b/test/audit.test.js index 1c4c753..0562f04 100644 --- a/test/audit.test.js +++ b/test/audit.test.js @@ -93,6 +93,31 @@ test('getEntriesForResource returns an empty array for unknown resource', () => assert.deepEqual(entries, []); }); +test('getEntriesForResource enforces explicit scopes and preserves trusted access', () => { + const ApiError = require('../src/utils/ApiError'); + const first = auditService.addEntry({ action: 'transfer.created', resourceId: 'txn-A' }); + auditService.addEntry({ action: 'user.created', resourceId: 'usr-B' }); + const last = auditService.addEntry({ action: 'transfer.claimed', resourceId: 'txn-A' }); + + for (const auth of [{ scopes: ['transfers:read'] }, {}]) { + for (const resourceId of ['txn-A', 'missing', '', null]) { + assert.throws( + () => auditService.getEntriesForResource(resourceId, auth), + (err) => err instanceof ApiError && err.statusCode === 403 && + err.message === 'Insufficient token scopes' + ); + } + } + + const allowed = { scopes: ['audit:read'] }; + assert.deepEqual(auditService.getEntriesForResource('txn-A', allowed), [last, first]); + assert.deepEqual(auditService.getEntriesForResource('txn-A'), [last, first]); + assert.deepEqual(auditService.getEntriesForResource('txn-A', null), [last, first]); + assert.deepEqual(auditService.getEntriesForResource('missing', allowed), []); + assert.deepEqual(auditService.getEntriesForResource('', allowed), []); + assert.deepEqual(auditService.getEntriesForResource(null), []); +}); + // ─── reset ──────────────────────────────────────────────────────────────────── test('reset clears all entries', () => { From 2834ab839e59e40f23d0785126e20ff8be0f6845 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 08:25:29 -0400 Subject: [PATCH 08/13] fix: enforce user read scope on direct lookup Issue #125 requires scope enforcement at service boundaries. The exported findUser helper ignored an explicitly supplied context, unlike listUsers and getUserOrThrow. Require users:read before looking up any record while retaining the documented trusted behavior for omitted/null contexts. Add two maintained Node service regressions. On parent 05a6279b894d3c110f9db539cb25b50e3a42e5c3 they produced 1 pass / 1 fail (the direct lookup did not reject an under-scoped caller). With this repair they produce 2 pass / 0 fail / 0 skipped, including missing/existing-ID parity, zero unauthorized lookups, authorized readers and trusted-call compatibility. Execution: Node v24.19.0; node --require /dev/shm/harbor-6907-rfb143-audit/preload-dotenv.cjs --test test/userScope.test.js. That external execution-only shim made unavailable dotenv.config inert; actual service, store, authorization, scope catalog, audit and UUID modules ran unchanged. No .env file, HTTP/server, hosted CI, chain or full-suite result is claimed. Prior transfer recovery and audit-resource scope fixes are preserved. --- src/services/userService.js | 4 ++- test/userScope.test.js | 50 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 test/userScope.test.js diff --git a/src/services/userService.js b/src/services/userService.js index d59ef83..b78382e 100644 --- a/src/services/userService.js +++ b/src/services/userService.js @@ -22,9 +22,11 @@ function listUsers(auth) { /** * Find a user by id. * @param {string} id + * @param {{ scopes?: string[] }|null} [auth] - optional service authorization context * @returns {object|undefined} */ -function findUser(id) { +function findUser(id, auth) { + assertScopes(auth, SCOPES.USERS_READ); return store.users.get(id); } diff --git a/test/userScope.test.js b/test/userScope.test.js new file mode 100644 index 0000000..547c0d0 --- /dev/null +++ b/test/userScope.test.js @@ -0,0 +1,50 @@ +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { store, reset } = require('../src/store'); +const userService = require('../src/services/userService'); +const { SCOPES } = require('../src/config/scopes'); + +test('findUser rejects explicit under-scoped contexts before looking up users', () => { + reset(); + const user = userService.createUser({ + name: 'Scope fixture', + email: 'scope-fixture@example.invalid', + }); + const originalGet = store.users.get; + let lookups = 0; + store.users.get = function (...args) { + lookups += 1; + return originalGet.apply(this, args); + }; + + try { + for (const id of [user.id, 'usr_missing']) { + for (const auth of [{}, { scopes: [] }, { scopes: [SCOPES.USERS_WRITE] }]) { + assert.throws( + () => userService.findUser(id, auth), + (error) => error.statusCode === 403 + && error.message === 'Insufficient token scopes' + ); + } + } + assert.equal(lookups, 0); + } finally { + store.users.get = originalGet; + } +}); + +test('findUser preserves authorized reads and trusted internal lookup behavior', () => { + reset(); + const user = userService.createUser({ + name: 'Scope fixture', + email: 'scope-fixture@example.invalid', + }); + const reader = { scopes: [SCOPES.USERS_READ] }; + for (const auth of [reader, undefined, null]) { + assert.equal(userService.findUser(user.id, auth), user); + assert.equal(userService.findUser('usr_missing', auth), undefined); + } + assert.equal(userService.getUserOrThrow(user.id, reader), user); +}); From fd008399a588ad4cbdc0974d90feee8f1bcce8a9 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 11:09:47 -0400 Subject: [PATCH 09/13] fix: scope transfer cursor position reads --- src/services/transferService.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/services/transferService.js b/src/services/transferService.js index 16a77bd..9e63325 100644 --- a/src/services/transferService.js +++ b/src/services/transferService.js @@ -158,7 +158,8 @@ function queryTransfers({ * @param {number} seq * @returns {string|null} */ -function positionKeyAt(seq) { +function positionKeyAt(seq, auth) { + assertScopes(auth, SCOPES.TRANSFERS_READ); const record = store.transferIndex.recordAt(seq); return record ? record.key : null; } From e5908cd0c9ed8d40b1f71b792c570d0c9321639c Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 11:09:49 -0400 Subject: [PATCH 10/13] fix: scope audit cursor position reads --- src/services/auditService.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/services/auditService.js b/src/services/auditService.js index 2a41cd4..9b33993 100644 --- a/src/services/auditService.js +++ b/src/services/auditService.js @@ -133,7 +133,8 @@ function queryEntries({ * @param {string} [resourceId] * @returns {string|null} */ -function positionKeyAt(seq, resourceId) { +function positionKeyAt(seq, resourceId, auth) { + assertScopes(auth, SCOPES.AUDIT_READ); const group = resourceId == null || resourceId === '' ? null : String(resourceId); const record = auditIndex.recordAt(seq, group); return record ? record.key : null; From 8a0e307a3dee717ef2838d6e5469db58e3d15f8a Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 11:09:52 -0400 Subject: [PATCH 11/13] wire transfer cursor auth context --- src/controllers/transferController.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/transferController.js b/src/controllers/transferController.js index 73a2158..2384d77 100644 --- a/src/controllers/transferController.js +++ b/src/controllers/transferController.js @@ -109,7 +109,7 @@ function listTransfers(req, res) { defaultOrder: 'asc', query: (args) => transferService.queryTransfers({ ...filters, ...args, auth }), countTotal: () => transferService.listTransfers(filters, auth).length, - resolvePosition: (seq) => transferService.positionKeyAt(seq), + resolvePosition: (seq) => transferService.positionKeyAt(seq, auth), }); res.json({ ...envelope, transfers: items }); From 16bf61d2f52b8cb94c5872567a32e1e7e8f65a82 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 11:09:55 -0400 Subject: [PATCH 12/13] wire audit cursor auth context --- src/controllers/auditController.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/auditController.js b/src/controllers/auditController.js index 987b178..b6236c5 100644 --- a/src/controllers/auditController.js +++ b/src/controllers/auditController.js @@ -30,7 +30,7 @@ function listAuditEntries(req, res) { defaultOrder: 'desc', query: (args) => auditService.queryEntries({ resourceId, ...args, auth }), countTotal: () => auditService.countEntries(resourceId, auth), - resolvePosition: (seq) => auditService.positionKeyAt(seq, resourceId), + resolvePosition: (seq) => auditService.positionKeyAt(seq, resourceId, auth), }); res.json({ ...envelope, entries: items }); From 38d34851ddfa7e66c4206db5d124aa720d32e9e9 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Sun, 4 Oct 2026 11:09:58 -0400 Subject: [PATCH 13/13] test: cover cursor position service scopes --- test/tokenScopes.test.js | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/tokenScopes.test.js b/test/tokenScopes.test.js index fdaf047..69f9d65 100644 --- a/test/tokenScopes.test.js +++ b/test/tokenScopes.test.js @@ -406,6 +406,39 @@ test('transfer service boundary rejects under-scoped auth on write and read', () ); }); +test('cursor position helpers reject explicitly under-scoped service callers', () => { + assert.throws( + () => transferService.positionKeyAt(0, { + actor: 'w', + scopes: [SCOPES.TRANSFERS_WRITE], + }), + (err) => err.statusCode === 403 + ); + assert.throws( + () => auditService.positionKeyAt(0, null, { + actor: 't', + scopes: [SCOPES.TRANSFERS_READ], + }), + (err) => err.statusCode === 403 + ); + + assert.doesNotThrow(() => + transferService.positionKeyAt(0, { + actor: 'r', + scopes: [SCOPES.TRANSFERS_READ], + }) + ); + assert.doesNotThrow(() => + auditService.positionKeyAt(0, null, { + actor: 'a', + scopes: [SCOPES.AUDIT_READ], + }) + ); + // Trusted internal callers without auth remain compatible. + assert.doesNotThrow(() => transferService.positionKeyAt(0)); + assert.doesNotThrow(() => auditService.positionKeyAt(0)); +}); + test('service boundary: malformed id throws the same not-found error as a miss', () => { const malformed = () => transferService.getTransferOrThrow('txn_bad', { actor: 'r',