Repository navigation
security(backend): enforce token scopes consistently across transfer and admin routes - #143
Open
woahwhattheheck wants to merge 13 commits into
Open
woahwhattheheck wants to merge 13 commits into
woahwhattheheck wants to merge 13 commits into
Conversation
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 RemitFlow#125
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.
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.
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.
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 368f8b5 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
Issue RemitFlow#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 05a6279 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Closes #125.
Inconsistent route and service checks allowed token-scope rules to vary across transfer, user, audit, and admin operations. The bulk dispatcher also accepted inherited properties and coerced non-string actions. Explicitly malformed API_TOKENS configuration silently enabled public demo credentials, while array-shaped maps could create unintended numeric tokens and null maps caused request failures.
Changes
Compatibility and boundaries
Documentation continuation at b0c161f
Documentation head: b0c161f
Sole parent: c147a2e
Tree: 6c546ffc026b270c3cf23925089ce7f908d2a26c
Only README.md and docs/SCOPE_MATRIX.md change (+47/-6). The guide now explains that assertScopes skips null/undefined auth, so a request adapter must preserve its route middleware and pass authFromRequest(req) to the service. It also describes the existing 403 result when a recognized, under-scoped API bearer token is supplied alongside a valid legacy admin header.
All 175 other tracked leaf blobs and modes are unchanged, including every route, service, test, manifest and lockfile. The complete two-file contents, Git blobs, sole-parent commit/tree/message, original branch, PR head and one-commit diff were read back through the provider API. Documentation examples were reconciled directly with the current auth helper, transfer controller/service and admin middleware.
This continuation is API-only. No new native HTTP flow, maintained suite, renderer, syntax command, install or external-provider request was attempted while the execution environment was offline. The retained execution evidence below belongs to the unchanged implementation at the parent head; it is not presented as a new run.
At this documentation revision, CI run 37172150829 is completed/action_required, with zero jobs returned by the jobs endpoint. The PR remains open and unmerged. Maintainer CI approval and acceptance remain outstanding.
Retained validation at c147a2e
At that parent head, CI run 37116422606 reported action_required. Hosted CI success, sponsor acceptance, deployment, and live Stellar settlement are not claimed. All HTTP reproduction used local fixtures and closed its servers.
Later source continuations
Current original PR head:
38d34851ddfa7e66c4206db5d124aa720d32e9e9. These changes preserve the original scope catalog, bulk contract, token-map validation and contributor history.3f83f390makes bulk failures follow the central ApiError boundary. Unexpected thrown values cannot supply public status or message, and null/string failures remain per-item while independent rows continue. Intentional API conflicts and the canonical 404 remain. One completed native execution used the actual createApp/Express application over loopback: the new cases had 2 passes and 6 failures on the previous source; repaired maintained tokenScopes plus the new regressions had 33 passes, zero failures or skips. Source-bound report.368f8b5fvalidates the claim transition before adapter work and commits terminal state only after adapter success. Adapter failure leaves the pending record and timestamp unchanged for retry; successful independent bulk rows remain committed. Two focused actual-production-module recovery cases failed before and passed after on Node 24.19.0. That isolated execution used the maintained mock adapter failure and an external inert dotenv loader; it was not an HTTP, full-suite or live-chain run.05a6279benforces audit:read for an explicit auth context in getEntriesForResource before looking up a present, missing or empty resource. Omitted/null trusted contexts retain compatibility. Three focused cases passed through the actual audit, authz and configuration modules with a synthetic store on Node 24.19.0 and an external inert dotenv loader; no HTTP or full suite was run for this continuation.2834ab83enforces users:read in exported findUser before lookup when auth is supplied. Existing and missing IDs with insufficient scope produce the same 403 without unauthorized store lookup; authorized and trusted calls retain their results. The two maintained service cases changed from 1 pass / 1 failure to 2 passes / zero failures or skips on Node 24.19.0 with the actual service/store/authz/config/audit/UUID modules and the external inert dotenv loader. This was not a provider, HTTP or full-suite execution.38d34851adds optional auth to transferService.positionKeyAt(seq, auth) and auditService.positionKeyAt(seq, resourceId, auth), requiring transfers:read and audit:read respectively. Both history controllers pass their already-built auth context. Trusted internal calls without auth remain supported. The existing tokenScopes regression source includes one additional cursor-context case. No repository suite was executed for this latest continuation.Current validation boundary
The earlier full-suite and HTTP results above remain pinned to their recorded revisions. They are not combined into a new total and do not establish full-suite acceptance at
38d34851. At the current head, upstream CI run 37211981375 reports action_required with zero jobs, and no fork pull-request run was returned. The original PR remains open; hosted approval, deployment and live settlement remain separate.