Fix null pointer, unauthenticated quote endpoint, race condition, and cache-authorization bypass in Path Payment Service - #1591
Merged
emdevelopa merged 1 commit intoSep 28, 2026
Conversation
… cache-authorization bypass in Path Payment Service closes emdevelopa#1308 closes emdevelopa#1309 closes emdevelopa#1310 closes emdevelopa#1311 - emdevelopa#1308 (null pointer): findStrictReceivePaths called best.path.map(...) unguarded. Horizon can return a record shape that omits `path` entirely (a direct, hop-free route) rather than an empty array, which threw a TypeError uncaught by the function's Horizon-error handling, since it's a plain JS bug, not a rejected promise. Guarded with `(best.path || [])`. - emdevelopa#1309 (security): three separate findings. 1. GET /api/path-payment-quote/:id had no auth middleware at all - it sits outside the /api/payments prefix app.js gates with requireApiKeyAuth(), so req.merchant was always undefined and the merchant_id scoping silently no-opped. Any anonymous caller could read another merchant's payment amount/asset/recipient by id and generate live Horizon quotes against it for free. Added requireApiKeyAuth() to the route and made the merchant_id filter mandatory. 2. POST /api/payments/:id/refund/confirm (and paymentService.confirmRefundTx, which the route never actually called - it duplicated an older, unverified version of the same logic inline) accepted any tx_hash string and marked the refund "refunded" unconditionally. A merchant could confirm a refund that never happened. Now verifies the hash against the refund transaction generateRefundTx produced (Stellar tx hashes are stable across signing, so this is an exact, cheap check) and confirms the transaction actually succeeded on Horizon before writing anything. 3. getPaymentStatus's Redis cache-hit path (both in paymentService.js and a second, duplicate implementation in payments.js's /payment-status/:id route) returned cached data without re-checking merchant_id ownership, since the cache key was id-only. Whichever caller populated the cache first decided what every later caller for that id saw, bypassing scoping entirely on a hit. Fixed as part of emdevelopa#1311 below, since it's the same root cause. - emdevelopa#1310 (race condition): verifyPayment's read-check-then-write (data.status === "confirmed" check, then an unconditional UPDATE) is not atomic - two concurrent calls for the same payment (a webhook-triggered check racing a client poll) can both read "pending" before either writes, and both fire webhooks/sockets/emails and double-count confirmation metrics. Made the UPDATE conditional on the status this call observed and check whether a row actually matched; the loser reports success without repeating side effects. Applied the identical fix to payments.js's separate /verify-payment/:id route, which had the same bug in its underpayment/overpayment branches (its exact-match branch was already correctly guarded). - emdevelopa#1311 (data inconsistency): paymentCacheKey was keyed on payment id alone, with no merchant dimension, even though getPaymentStatus is called both without a merchant scope (a customer's public payment_link) and with one (an authenticated merchant lookup) for the same id. A cache hit could therefore return a payment record whose access-control context did not match the current request's scope. Namespaced the cache key by (id, merchantId), with a stable "public" bucket for the unscoped path, and updated every call site (paymentService.js and payments.js's duplicate implementation) to pass the scope through consistently, including on invalidation. Also fixed, as a necessary prerequisite: backend/src/routes/payments.js had a severe pre-existing bug unrelated to any of the four issues above - a botched merge conflict (98119f7) left the ENTIRE FILE with a JavaScript syntax error (an unclosed brace and a dangling reference to an undefined `validation` variable), meaning the whole payments router could not even be parsed, let alone loaded, on main. It had merged two incompatible session-validation designs (an inline resolveAndValidateIssuer/ validatePerAssetLimits/validateAllowedIssuers approach, and a newer unified validatePaymentSession() abstraction) into createSession/ createSessionUnlocked. Reconciled in favor of the newer validatePaymentSession() abstraction (already has its own dedicated validator module, sanitization, metrics, and health monitoring) and removed the dangling old inline calls. Disclosure: fixing the parse error caused several previously-invisible (whole-suite-failed-to-load) test suites to actually run for the first time - tests/e2e/{payment-processor,exchange-rate,path-payment, audit-logger,transaction-signer}.e2e.test.js and two load-test files. Most of their individual test failures are pre-existing gaps between an earlier merge's new dependencies (payment-session-lock.js, payment-session-retry.js) and these E2E files' mocks, unrelated to Path Payment Service; fixing those is out of scope here. One direct consequence of this PR's own emdevelopa#1310 fix (a stale Supabase update-chain mock in paymentService-security-audit.test.js) was fixed. Verified via a full baseline-vs-after diff of every individual test name across the whole backend suite that zero net-new regressions were introduced beyond that one already-fixed case; several previously-whole-suite-failing files (path-payment-recovery, payments-path-quote, payments-pooler, payments-security) now pass in full. Testing: full backend vitest suite compared before/after via a saved list of every FAIL line; the 4 targeted issues' fixes each have dedicated new tests (path-payment-recovery.test.js, payments-path-quote.test.js, paymentService.test.js, payment-cache-scoping.test.js) plus a fixed mock in paymentService-security-audit.test.js, all passing.
|
@pre-cious-Igwealor 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! 🚀 |
|
@pre-cious-Igwealor is attempting to deploy a commit to the Emmanuel's projects Team on Vercel. A member of the Team first needs to authorize it. |
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
closes #1308
closes #1309
closes #1310
closes #1311
Also fixed
backend/src/routes/payments.js had a severe pre-existing bug unrelated to any of the four issues above: a botched merge conflict left the ENTIRE FILE with a JavaScript syntax error (an unclosed brace and a dangling reference to an undefined
validationvariable), meaning the whole payments router could not even be parsed, let alone loaded, on main. It had merged two incompatible session-validation designs (an inline resolveAndValidateIssuer/validatePerAssetLimits/validateAllowedIssuers approach, and a newer unified validatePaymentSession() abstraction) into createSession/createSessionUnlocked. Reconciled in favor of the newer validatePaymentSession() abstraction (already has its own dedicated validator module, sanitization, metrics, and health monitoring) and removed the dangling old inline calls.Disclosure: fixing the parse error caused several previously-invisible (whole-suite-failed-to-load) test suites to actually run for the first time, tests/e2e/{payment-processor,exchange-rate,path-payment,audit-logger,transaction-signer}.e2e.test.js and two load-test files. Most of their individual test failures are pre-existing gaps between an earlier merge's new dependencies (payment-session-lock.js, payment-session-retry.js) and these E2E files' mocks, unrelated to Path Payment Service; fixing those is out of scope here. One direct consequence of this PR's own #1310 fix (a stale Supabase update-chain mock in paymentService-security-audit.test.js) was fixed. Verified via a full baseline-vs-after diff of every individual test name across the whole backend suite that zero net-new regressions were introduced beyond that one already-fixed case; several previously-whole-suite-failing files (path-payment-recovery, payments-path-quote, payments-pooler, payments-security) now pass in full.
Test plan