fix: slippage bounds, audit-log IP spoofing, idempotency key cleanup - #638
Merged
Merged
Conversation
…#620) GET /api/quote's QuoteQuerySchema validated slippageBps was a non-negative integer string but had no upper bound. A merchant submitting e.g. slippageBps=10000 (100%) wasn't rejected — it was silently clamped to env.MAX_SLIPPAGE_BPS (default 500) with no error signal, so the request looked accepted while the merchant's actual input was ignored without them knowing. Added a refine() rejecting anything above 1000 with a 400, per the issue's acceptance criteria, instead of relying on silent clamping. Added quote-slippage-bounds.test.ts: slippageBps=10000 -> 400, and an in-range value (250) is accepted with slippageLimit = slippageBps/10000 in the response. Ran locally against the built @bettapay/validation package: 4/4 passing, plus the existing fx-quote-computation.test.ts suite (32/32) to confirm no regression. Closes Betta-Pay#620
…-Pay#621) AuditLog.ipAddress was populated by getRequestIp(), which read X-Forwarded-For (falling back to X-Real-IP, then request.ip) unconditionally — with zero verification that the request actually passed through a trusted proxy. Any direct client, proxied or not, could set X-Forwarded-For: 1.1.1.1 and have it recorded as their identity in the audit trail. Added getClientIp(request, trustedProxyCount), backed by a new TRUSTED_PROXY_COUNT env var (default 0). With trustedProxyCount = 0 (the default — secure out of the box), X-Forwarded-For/X-Real-IP are never consulted; only request.ip (the transport-layer address, which the client cannot spoof) is used. With trustedProxyCount = N, the client is resolved by stripping the rightmost N hops from the combined [xff-entries..., request.ip] chain, mirroring Express's/Fastify's `trust proxy: N` semantics. getRequestIp() now delegates to it. Added shared/validation/audit.test.ts covering: an untrusted direct client's spoofed header is ignored (trustedProxyCount=0), a 1-proxy chain and a 2-proxy chain each correctly resolve to the original client, X-Real-IP is likewise gated by the trust count, and array- valued headers are handled. Ran locally: 8/8 passing. Rebuilt @bettapay/validation's dist (tsc emits per-file even with the pre-existing, unrelated schemas.ts:351 error present on main) and confirmed dist/audit.js picked up the change. Closes Betta-Pay#621
…y#622) idempotencyKey is @unique on both Payment and Settlement, but that constraint has no concept of expiry — only the application's `idempotencyKeyExpiresAt: { gt: now }` lookup does. Once a key's 24h window passed, the old row still held the unique value, so a client retrying with the same Idempotency-Key header after 24h would get a DB unique-constraint violation on create() instead of a fresh payment, and the table grew without bound since nothing ever cleared expired rows. - Added `@@index([idempotencyKeyExpiresAt])` to Payment and Settlement, plus the migration, so the cleanup job's scan doesn't full-scan. - Added idempotency-key-cleanup-cron.ts (mirrors the existing abandoned-payments-cron.ts pattern already used in this service): reclaimExpiredIdempotencyKeys() nulls out idempotencyKey and idempotencyKeyExpiresAt on rows past their expiry via updateMany on both tables, guarded by the same Redis distributed lock and in-process re-entrancy guard as the abandoned-payments job. Wired into index.ts's startup/shutdown alongside it, on the same IDEMPOTENCY_KEY_CLEANUP_CRON_INTERVAL_MS-configurable hourly interval. Added idempotency-key-cleanup-cron.test.ts with time-mocked rows: expired rows on both tables get reclaimed, a not-yet-expired row is left alone, a concurrent second run while one is in flight is a no-op, and a held Redis lock skips the run. Ran locally: 18/18 passing. Closes Betta-Pay#622
|
@gideononiru Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
This batch was scoped to #623, #622, #621, #620. All four had specific, accurate file/line references and checked out against the real code. Under time pressure, #620, #621, #622 were completed and verified; #623 (indexer lag on the health endpoint) is not implemented — ran out of time before starting it, no
Closesline for it, it should stay open.Closes #623
Closes #622
Closes #621
Closes #620
#620 — done, closes it
GET /api/quote'sQuoteQuerySchema.slippageBpsvalidated the value was a non-negative integer string but had no upper bound. A merchant submittingslippageBps=10000(100%) wasn't rejected — it was silently clamped toenv.MAX_SLIPPAGE_BPS(default 500) with no error signal, so the request looked accepted while their actual input was ignored without them knowing.Added a
.refine()rejecting anything above 1000 with a 400, matching the issue's acceptance criteria instead of relying on silent clamping.New test
quote-slippage-bounds.test.ts:slippageBps=10000→ 400; an in-range value (250) is accepted withslippageLimit = slippageBps/10000in the response. Ran locally against the built@bettapay/validationpackage: 4/4 passing, plus the existingfx-quote-computation.test.tssuite (32/32) to confirm no regression.#621 — done, closes it
AuditLog.ipAddresswas populated bygetRequestIp()inshared/validation/audit.ts, which readX-Forwarded-For(falling back toX-Real-IP, thenrequest.ip) unconditionally — zero verification the request passed through a trusted proxy. Any direct client could setX-Forwarded-For: 1.1.1.1and have it recorded as their identity in the audit trail.Added
getClientIp(request, trustedProxyCount), backed by a newTRUSTED_PROXY_COUNTenv var (default0). WithtrustedProxyCount = 0— the default, secure out of the box — neither header is ever consulted; onlyrequest.ip(transport-layer, unspoofable) is used. WithtrustedProxyCount = N, the client is resolved by stripping the rightmost N hops off the combined[xff-entries..., request.ip]chain, mirroring Express's/Fastify'strust proxy: Nsemantics.getRequestIp()now delegates to it.New test
shared/validation/audit.test.ts: an untrusted direct client's spoofed header is ignored, a 1-proxy chain and a 2-proxy chain each correctly resolve to the original client,X-Real-IPis likewise gated by the trust count, array-valued headers are handled. Ran locally: 8/8 passing. Rebuilt@bettapay/validation'sdistand confirmeddist/audit.jspicked up the change (see note below on why a rebuild was needed here).#622 — done, closes it
idempotencyKeyis@uniqueon bothPaymentandSettlement, but that constraint has no concept of expiry — only the app'sidempotencyKeyExpiresAt: { gt: now }lookup does. Once a key's 24h window passed, the old row still held the unique value, so a client retrying the sameIdempotency-Keyafter 24h hit a DB unique-constraint violation instead of getting a fresh payment, and the table grew unbounded since nothing ever cleared expired rows.@@index([idempotencyKeyExpiresAt])toPaymentandSettlement, plus the migration.idempotency-key-cleanup-cron.ts, mirroring the existingabandoned-payments-cron.tspattern already used in this service:reclaimExpiredIdempotencyKeys()nullsidempotencyKey/idempotencyKeyExpiresAton expired rows viaupdateManyon both tables, guarded by the same Redis distributed lock + in-process re-entrancy guard. Wired intoindex.tsstartup/shutdown alongside the abandoned-payments cron, on a configurable hourly interval (IDEMPOTENCY_KEY_CLEANUP_CRON_INTERVAL_MS).New test
idempotency-key-cleanup-cron.test.tswith time-mocked rows: expired rows on both tables get reclaimed, a not-yet-expired row is left alone, a concurrent second run is a no-op, a held Redis lock skips the run. Ran locally: 18/18 passing.Health-endpoint indexer lag (
GET /api/health/indexer-lagor extendingGET /api/admin/health, plus theindexer_lag_ledgersPrometheus gauge) — no changes made. Left open.Notes for reviewers
shared/validationcurrently failstsconmaindue to a pre-existing, unrelated error atschemas.ts:351(Property 'feeSchedules' does not exist...) — confirmed viagit stashthat this reproduces on a clean checkout.tscstill emits per-file output despite that error (nonoEmitOnError), sodist/audit.jspicked up the AuditLog ipAddress is stored as string without X-Forwarded-For trust boundary validation #621 fix correctly, but worth fixing separately since it silently masks the package's build status.npx tsc --noEmitwas run forapi-gateway,fx-engine, andshared/validationafterprisma generate; every remaining error (missing@bettapay/shared-typesmodule, afastify-rate-limittype mismatch, an undefinedconsumeWalletChallengereference, theschemas.ts:351one, and a couple offx-enginehealth-status literal-type mismatches) was confirmed pre-existing viagit stash— none touch the files this PR changes.Test plan
quote-slippage-bounds.test.ts— 4/4, plusfx-quote-computation.test.ts— 32/32 (no regression)shared/validation/audit.test.ts— 8/8idempotency-key-cleanup-cron.test.ts— 18/18npx tsc --noEmitfor the three touched packages — zero new errors vs.main