Repository navigation
feat(security): add abuse controls and correlation IDs to mutation routes - #138
Open
woahwhattheheck wants to merge 17 commits into
Open
woahwhattheheck wants to merge 17 commits into
woahwhattheheck wants to merge 17 commits into
Conversation
…utes Bound transfer, user, quote, and admin mutation traffic with per-actor fixed-window limits, bounded limiter state, and safe 429 responses. Sanitize and propagate X-Request-Id / X-Correlation-Id without echoing secrets. Closes RemitFlow#131.
Cache the earliest expiry and prune only when it becomes due. Keep the minimum exact across expired-key renewal, clock rollback and reset; preserve active budgets and 429 retry behavior. Regression: 128 rejected newcomers at 256 live keys previously traversed 65,536 entries; a deterministic one-table traversal bound now passes. Added expiry/retry, rollback and reset cases. Full suite: 271 passed, 0 failed, 0 skipped (original: 267 passed). Alternating real createApp HTTP runs (original/candidate/original/candidate), each with 10,000 admitted identities and three 2,000-request 429 batches at 32 connections. Across six batches per version: wall median 810.24 -> 336.59 ms; ranges 626.79-1050.47 -> 330.55-411.99 ms. Same-process client/server CPU median 849.97 -> 359.56 ms; ranges 653.17-1268.51 -> 351.43-578.41 ms. Local Node 24.19.0 / Express 4.22.2; explicit test-mode rate limiting, trusted proxy, error tracking disabled. Generated local load on a busy shared runtime; no absolute live throughput, deployed performance or hosted CI claim. Expiry-triggered cleanup remains O(maxKeys). Operation: REMIT138-SATURATED-LIMITER-20261003-8341
Assign correlation IDs and apply the global API limiter before JSON/form parsing. Malformed or oversized mutation bodies previously terminated before both middleware layers, allowing repeated parser work outside the documented request budget and returning uncorrelated errors. Preserve handler timeout, route-family limits and authentication behavior. An actual createApp HTTP regression sends malformed and oversized bodies with a two-request budget. The prior source returns 400,413,400,413; the repair returns 400,413,429,429, with request/correlation IDs on headers and errors plus global-policy and Retry-After metadata. Composed on e332e9b, retaining the saturated-limiter optimization and all four of its regressions. Exact composed source: npm test passes 272/272, 0 failed, 0 skipped on Node24.19.0. git diff --check passes. Operation: REMIT138-PARSER-BUDGET-20261003-443D
Add a runnable benchmark using only Node built-ins and the existing app dependencies. It starts the real createApp service on loopback, admits 10,000 identities, then records three 2,000-request rejection batches at 32 connections. Every rejection checks the global policy and correlation ID; the original exhausted budget must stay blocked. JSON retains raw samples, runtime versions and app/limiter source hashes. An optional checkout path supports alternating versions with the same script. Actual documented invocation completed on parent a41a01a, composing the existing saturated-table and parser-budget repairs: all 10,000 fills and 6,000 rejections matched, original budget remained blocked, exit 0. Client and server share one process; no live provider/deployed performance claim. Original before/after measurements remain scoped to e332e9b. Operation: REMIT138-BENCHMARK-REPRO-20261003-8341
Document trimming and first-valid header precedence, and distinguish the existing character/length check from secret or personal-data detection. Tell callers to use opaque IDs or accept a server-generated one because accepted values are echoed and logged. Only the correlation guide section changes. Existing limiter, parser, benchmark and source tests are retained. Source and full-text provider readback validate this continuation; no runtime, benchmark or maintained checks were replayed during the execution environment outage.
A printable token is not a safe request ID. Exclude inbound request/correlation IDs containing the Bearer or X-Admin-Token credential presented on the same request, before the value reaches response headers and request logging. Retain a separate safe fallback header or use the existing UUID generator. No authentication, actor-key, proxy-trust or rate-limit behavior changes; arbitrary unrelated PII is not claimed detectable. Focused native Node22.16.0 execution: test/requestIdCredentials.test.js before1pass/4fail, after5pass/0fail. Exercises both correlation headers, direct/embedded Bearer and admin tokens, independent fallback, ordinary precedence, syntax/length controls and fresh replacement IDs. Synthetic values only; no live credentials or account data. The complete source-pinned middleware and unchanged ids.js executed with request/response fixtures. The unavailable uuid package was supplied by an external preload mapping only uuid.v4 to node:crypto.randomUUID; that preload is not committed. No Express application, authentication flow, HTTP/provider, full-suite or deployed incident claim. With normal project dependencies: node --test test/requestIdCredentials.test.js. Composed on current dba2277, retaining the previous expiry implementation and its evidence. Existing PR138 and issue131 remain the submission path. No dependency, CI workflow or existing test removal.
Track the maximum fixed-window deadline and clear both bounded collections when no active budget remains. Preserve partial expiry, clock rollback, renewal and 429 behavior. The existing expiry/renewal selection passes four focused regressions, including one new whole-table boundary/reset case. Production-module comparisons retain identical response digests and eliminate 10,000 expired-entry Map deletions per full-table expiry. Exact paired timing data, admission-fill costs and replay script are retained in the abuse-controls guide; no deployed or total-throughput improvement is claimed. Preserves the preceding correlation guidance and all earlier source fixes.
Set X-RateLimit-Reset from the existing earliest-expiry index when the identity table is full. This prevents stacked global and mutation limiters from reporting different policies and deadlines. Three focused middleware-over-HTTP cases fail before the two-line repair and pass afterward, including standalone rejection and expiry reuse. Preserve all existing admission, expiry and retry behavior.
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
1 participant
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.
Summary
Closes #131.
Adds production-shaped abuse controls on transfer, user, quote, and admin mutation routes, plus sanitized correlation-id propagation so accepted commands can be traced without leaking account secrets.
Note: #133 already has an open solid PR (#137), so this targets #131 rather than duplicating lifecycle-concurrency work.
Current carrier
f02a6ae0edc7e5e5b99d890297f81381e1875a9fwoahwhattheheck:wire/rfb-131-abuse-controlsmain@b0a004ab8aa00b1e6f262aa84ea2b0ec0a622e1bWhat
maxKeys; indexed expiry avoids rescanning the full live-key map during cleanup.TRUST_PROXYgates whetherX-Forwarded-Forcan influence client identity (off by default).docs/ABUSE_CONTROLS.md.Validation boundary
Earlier carrier revisions recorded
npm testwith 267 passing tests, includingtest/abuseControls.test.js. That is historical evidence, not a fresh full-suite claim for the exact current head.For the current tip, the recorded focused HTTP regression for the capacity
X-RateLimit-Resetbehavior passed. No fresh exact-head full-suite or hosted-CI-green claim is made here.The committed component benchmark for the indexed-expiry repair used a 10,000-capacity limiter workload and recorded median cleanup latency improving from 224.325 ms to 2.818 ms (79.6×), while full-map visits dropped from 10,000,000 to 0. This is a pinned component workload, not a deployed-service or fleet-throughput benchmark.
Design tradeoffs
NODE_ENV=testunlessforceInTest/ENABLE_RATE_LIMIT_IN_TEST=1, keeping unrelated functional tests independent of the abuse budget while dedicated regressions exercise the limiter explicitly.Original contribution and compensation
/claim #131
I, @woahwhattheheck, claim this original contribution for #131 and request evaluation and the applicable GrantFox OSS / Third Campaign reward and payment. Please confirm acceptance, claimant eligibility, the reward allocation or amount, payout instructions and expected timing. The issue's Maybe Rewarded label establishes conditional consideration, not a fixed award or money received. Preserve the original contributor and compensation claim.
This update adds missing issue-scoped claim and compensation linkage only. Source head
f02a6ae0edc7e5e5b99d890297f81381e1875a9f, title, branch, original attribution and all validation limitations are unchanged.