From ea40eab3def1dc4f8b6e49ae65125752f4801c1b Mon Sep 17 00:00:00 2001 From: Precious Igwealor Date: Sat, 26 Sep 2026 23:12:37 +0100 Subject: [PATCH] Fix null pointer, unauthenticated quote endpoint, race condition, and cache-authorization bypass in Path Payment Service closes #1308 closes #1309 closes #1310 closes #1311 - #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 || [])`. - #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 #1311 below, since it's the same root cause. - #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). - #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 #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. --- backend/src/lib/path-payment-recovery.test.js | 31 ++ backend/src/lib/payment-cache-scoping.test.js | 109 +++++++ backend/src/lib/redis.js | 43 ++- backend/src/lib/stellar.js | 6 +- .../src/routes/payments-path-quote.test.js | 26 +- backend/src/routes/payments.js | 247 ++++++-------- .../paymentService-security-audit.test.js | 6 +- backend/src/services/paymentService.js | 106 +++++- backend/src/services/paymentService.test.js | 304 ++++++++++++++++++ 9 files changed, 711 insertions(+), 167 deletions(-) create mode 100644 backend/src/lib/payment-cache-scoping.test.js diff --git a/backend/src/lib/path-payment-recovery.test.js b/backend/src/lib/path-payment-recovery.test.js index 4f973e3b..9aca7ebd 100644 --- a/backend/src/lib/path-payment-recovery.test.js +++ b/backend/src/lib/path-payment-recovery.test.js @@ -30,6 +30,10 @@ vi.mock("stellar-sdk", () => { return { Asset: MockAsset, + Networks: { + PUBLIC: "Public Global Stellar Network ; September 2015", + TESTNET: "Test SDF Network ; September 2015", + }, StrKey: { isValidEd25519PublicKey: (value) => typeof value === "string" && value.startsWith("G") && value.length === 56, @@ -122,4 +126,31 @@ describe("findStrictReceivePaths", () => { message: "Horizon returned an invalid path payment quote", }); }); + + it("does not throw a null pointer error when Horizon omits `path` on the record (issue #1308)", async () => { + mockStrictReceivePaths.mockResolvedValueOnce({ + records: [ + { + source_amount: "60.1250000", + source_asset_type: "native", + source_asset_issuer: null, + destination_amount: "25.0000000", + // No `path` field at all — a shape Horizon can return for a + // direct, hop-free route, and one none of the other tests here + // exercise (they all set path: [] explicitly). + }, + ], + }); + + const result = await findStrictReceivePaths({ + sourceAccount, + destAssetCode: "USDC", + destAssetIssuer: issuer, + destAmount: "25", + sourceAssetCode: "XLM", + sourceAssetIssuer: null, + }); + + expect(result.path).toEqual([]); + }); }); diff --git a/backend/src/lib/payment-cache-scoping.test.js b/backend/src/lib/payment-cache-scoping.test.js new file mode 100644 index 00000000..acdbffc1 --- /dev/null +++ b/backend/src/lib/payment-cache-scoping.test.js @@ -0,0 +1,109 @@ +import { describe, it, expect, vi } from "vitest"; +import { + paymentCacheKey, + getCachedPayment, + setCachedPayment, + invalidatePaymentCache, +} from "./redis.js"; + +/** + * Payment status cache scoping (issue #1311). + * + * getPaymentStatus() is called both without a merchant scope (a customer + * following their public payment_link) and with one (an authenticated + * merchant lookup) for the same payment id. Before this fix, the cache key + * was keyed on `id` alone, so a cache hit returned whichever caller's + * result was cached first — completely bypassing the merchant_id filter + * the uncached query path enforces. These tests pin down that the cache + * key, and every function built on it, carries the merchant scope as part + * of its identity. + */ +describe("paymentCacheKey (issue #1311)", () => { + it("produces different keys for different merchant scopes on the same payment id", () => { + const publicKey = paymentCacheKey("pay_1", null); + const merchantAKey = paymentCacheKey("pay_1", "merchant-a"); + const merchantBKey = paymentCacheKey("pay_1", "merchant-b"); + + expect(new Set([publicKey, merchantAKey, merchantBKey]).size).toBe(3); + }); + + it("defaults to a stable public scope when no merchantId is given", () => { + expect(paymentCacheKey("pay_1")).toBe(paymentCacheKey("pay_1", null)); + }); + + it("produces the same key for the same (id, merchantId) pair", () => { + expect(paymentCacheKey("pay_1", "merchant-a")).toBe( + paymentCacheKey("pay_1", "merchant-a"), + ); + }); +}); + +function makeFakeRedisClient() { + const store = new Map(); + return { + store, + get: vi.fn(async (key) => store.get(key) ?? null), + set: vi.fn(async (key, value) => { + store.set(key, value); + }), + del: vi.fn(async (key) => { + store.delete(key); + }), + }; +} + +describe("getCachedPayment / setCachedPayment scoping (issue #1311)", () => { + it("a payment cached under one merchant's scope is not visible to a different merchant's lookup", async () => { + const client = makeFakeRedisClient(); + const merchantAsPayment = { id: "pay_1", merchant_id: "merchant-a", amount: "10" }; + + await setCachedPayment(client, "pay_1", merchantAsPayment, "merchant-a"); + + const seenByMerchantB = await getCachedPayment(client, "pay_1", "merchant-b"); + expect(seenByMerchantB).toBeNull(); + + const seenByMerchantA = await getCachedPayment(client, "pay_1", "merchant-a"); + expect(seenByMerchantA).toEqual(merchantAsPayment); + }); + + it("a payment cached under the public (unscoped) lookup is not returned for a merchant-scoped lookup", async () => { + const client = makeFakeRedisClient(); + const publicPayment = { id: "pay_1", merchant_id: "merchant-a", amount: "10" }; + + await setCachedPayment(client, "pay_1", publicPayment, null); + + const seenByMerchantA = await getCachedPayment(client, "pay_1", "merchant-a"); + expect(seenByMerchantA).toBeNull(); + + const seenPublicly = await getCachedPayment(client, "pay_1", null); + expect(seenPublicly).toEqual(publicPayment); + }); +}); + +describe("invalidatePaymentCache scoping (issue #1311)", () => { + it("invalidates both the public and the given merchant-scoped entry", async () => { + const client = makeFakeRedisClient(); + const payment = { id: "pay_1", amount: "10" }; + + await setCachedPayment(client, "pay_1", payment, null); + await setCachedPayment(client, "pay_1", payment, "merchant-a"); + + await invalidatePaymentCache(client, "pay_1", "merchant-a"); + + expect(await getCachedPayment(client, "pay_1", null)).toBeNull(); + expect(await getCachedPayment(client, "pay_1", "merchant-a")).toBeNull(); + }); + + it("does not touch a different merchant's cached entry it was not asked to invalidate", async () => { + const client = makeFakeRedisClient(); + const payment = { id: "pay_1", amount: "10" }; + + await setCachedPayment(client, "pay_1", payment, "merchant-a"); + await setCachedPayment(client, "pay_1", payment, "merchant-b"); + + await invalidatePaymentCache(client, "pay_1", "merchant-a"); + + expect(await getCachedPayment(client, "pay_1", "merchant-a")).toBeNull(); + expect(await getCachedPayment(client, "pay_1", "merchant-b")).toEqual(payment); + }); +}); diff --git a/backend/src/lib/redis.js b/backend/src/lib/redis.js index f8a680e5..91ea1b9a 100644 --- a/backend/src/lib/redis.js +++ b/backend/src/lib/redis.js @@ -99,19 +99,32 @@ export function resetRedisClientForTests() { /** TTL in seconds for payment-status cache entries. */ export const PAYMENT_STATUS_TTL = 2; -/** Consistent cache key for a payment-status entry. */ -export function paymentCacheKey(id) { - return `payment:status:${id}`; +/** + * Consistent cache key for a payment-status entry. + * + * Scoped by `merchantId` as well as `id` (issue #1311): getPaymentStatus() + * is called both without a merchant scope (the public payment_link tracking + * page) and with one (an authenticated merchant lookup) for the same + * payment id. A key that ignored merchantId meant whichever caller reached + * the cache first decided what every later caller saw for that id, + * regardless of whether their own `merchant_id` filter would have matched + * the row at all — a cache hit bypassed the authorization check the + * uncached path enforces. `merchantId` defaults to a fixed "public" bucket + * so the unscoped, link-based lookup path still gets its own cache entry. + */ +export function paymentCacheKey(id, merchantId = null) { + return `payment:status:${merchantId || "public"}:${id}`; } /** * Return the cached payment object, or null on miss / Redis unavailable. * @param {import("redis").RedisClientType} client * @param {string} id payment UUID + * @param {string|null} merchantId merchant scope this lookup was made under */ -export async function getCachedPayment(client, id) { +export async function getCachedPayment(client, id, merchantId = null) { try { - const raw = await client.get(paymentCacheKey(id)); + const raw = await client.get(paymentCacheKey(id, merchantId)); return raw ? JSON.parse(raw) : null; } catch (err) { // Never let a cache failure block the request @@ -125,10 +138,11 @@ export async function getCachedPayment(client, id) { * @param {import("redis").RedisClientType} client * @param {string} id payment UUID * @param {object} data the payment row to cache + * @param {string|null} merchantId merchant scope this lookup was made under */ -export async function setCachedPayment(client, id, data) { +export async function setCachedPayment(client, id, data, merchantId = null) { try { - await client.set(paymentCacheKey(id), JSON.stringify(data), { + await client.set(paymentCacheKey(id, merchantId), JSON.stringify(data), { EX: PAYMENT_STATUS_TTL, }); } catch (err) { @@ -137,13 +151,22 @@ export async function setCachedPayment(client, id, data) { } /** - * Invalidate the cache entry for a payment (call after any write). + * Invalidate the cache entries for a payment (call after any write). + * + * Invalidates both the public (unscoped) entry and, when a merchantId is + * supplied, that merchant's scoped entry — a write may be followed by reads + * from either lookup path. * @param {import("redis").RedisClientType} client * @param {string} id payment UUID + * @param {string|null} merchantId merchant scope to also invalidate, if known */ -export async function invalidatePaymentCache(client, id) { +export async function invalidatePaymentCache(client, id, merchantId = null) { try { - await client.del(paymentCacheKey(id)); + const keys = [paymentCacheKey(id, null)]; + if (merchantId) { + keys.push(paymentCacheKey(id, merchantId)); + } + await Promise.all(keys.map((key) => client.del(key))); } catch (err) { console.error("Redis DEL error:", err.message); } diff --git a/backend/src/lib/stellar.js b/backend/src/lib/stellar.js index df726bcf..80a9c187 100644 --- a/backend/src/lib/stellar.js +++ b/backend/src/lib/stellar.js @@ -206,7 +206,11 @@ export async function findStrictReceivePaths({ best.source_asset_type === "native" ? "XLM" : best.source_asset_code, source_asset_issuer: best.source_asset_issuer || null, destination_amount: best.destination_amount, - path: best.path.map((p) => ({ + // `path` is absent (not just empty) on some Horizon record shapes for + // a direct, hop-free route — `.map` on undefined threw a TypeError + // here uncaught by the try/catch's Horizon-error handling below, since + // it's a plain JS bug, not a rejected promise (issue #1308). + path: (best.path || []).map((p) => ({ asset_code: p.asset_type === "native" ? "XLM" : p.asset_code, asset_issuer: p.asset_issuer || null, })), diff --git a/backend/src/routes/payments-path-quote.test.js b/backend/src/routes/payments-path-quote.test.js index 2628c938..dc693686 100644 --- a/backend/src/routes/payments-path-quote.test.js +++ b/backend/src/routes/payments-path-quote.test.js @@ -42,13 +42,15 @@ function createSupabaseSelectMock(payment) { return chain; } -function getPathPaymentQuoteHandler() { +function getPathPaymentQuoteRoute() { const router = createPaymentsRouter(); - const layer = router.stack.find( + return router.stack.find( (entry) => entry.route?.path === "/path-payment-quote/:id", - ); + ).route; +} - return layer.route.stack.at(-1).handle; +function getPathPaymentQuoteHandler() { + return getPathPaymentQuoteRoute().stack.at(-1).handle; } function createMockResponse() { @@ -66,6 +68,22 @@ function createMockResponse() { }; } +describe("GET /api/path-payment-quote/:id — authentication (issue #1309)", () => { + it("is protected by requireApiKeyAuth, not left unauthenticated", () => { + // This route sits outside the `/api/payments` prefix that app.js gates + // with requireApiKeyAuth() at the mount level, so it needs its own + // per-route auth middleware or it is reachable by anyone. Assert the + // middleware stack actually contains an auth layer rather than relying + // only on behavioral tests, so a future refactor that silently drops it + // fails immediately here instead of shipping an unauthenticated route + // again. + const route = getPathPaymentQuoteRoute(); + const middlewareNames = route.stack.map((layer) => layer.name); + + expect(middlewareNames).toContain("requireApiKeyAuth"); + }); +}); + describe("GET /api/path-payment-quote/:id", () => { const paymentId = "9f927a2c-02d4-4f76-914c-62cf44d9525e"; const sourceAccount = diff --git a/backend/src/routes/payments.js b/backend/src/routes/payments.js index a8a0c17e..847120e5 100644 --- a/backend/src/routes/payments.js +++ b/backend/src/routes/payments.js @@ -15,6 +15,7 @@ import { validateRequest } from "../lib/validation.js"; import { createCreatePaymentRateLimit } from "../lib/create-payment-rate-limit.js"; import { createVerifyPaymentRateLimit } from "../lib/rate-limit.js"; import { createPathPaymentQuoteRateLimit } from "../lib/path-payment-quote-rate-limit.js"; +import { requireApiKeyAuth } from "../lib/auth.js"; import { recaptchaMiddleware } from "../lib/recaptcha.js"; import { sendWebhook, isEventSubscribed } from "../lib/webhooks.js"; import { sendReceiptEmail } from "../lib/email.js"; @@ -297,32 +298,6 @@ function createPaymentsRouter({ logger.error({ err, merchantId: req.merchant?.id }, "DEBUG: createSession error"); if (err.status === 400 && err.details) { return res.status(400).json({ error: err.message, ...err.details }); - const supabase = await getSupabaseClient(); - logger.info({ merchantId: req.merchant?.id, amount: req.body?.amount, asset: req.body?.asset }, "DEBUG: createSession started"); - - // Sanitization, strict payload checks and shared business rules - // (issues #1087, #1447) with validator metrics/health (#1448). - const validation = validatePaymentSession({ - body: req.body, - merchant: req.merchant, - source: "http", - }); - if (!validation.ok) { - const { rejection } = validation; - const assetLabel = req.body?.asset; - paymentFailedCounter.inc({ - asset: assetLabel, - reason: rejection.reason === "issuer_not_allowed" ? "invalid_issuer" : rejection.reason, - }); - paymentProcessorSessionsTotal.inc({ asset: assetLabel, outcome: "validation_failed" }); - paymentProcessorSessionDuration.observe( - { asset: assetLabel, outcome: "validation_failed" }, - (Date.now() - sessionStart) / 1000, - ); - return res.status(400).json({ - error: rejection.message, - ...(rejection.rule === "limits" ? rejection.details : {}), - }); } next(err); } @@ -331,63 +306,34 @@ function createPaymentsRouter({ async function createSessionUnlocked(req, res) { const sessionStart = Date.now(); const supabase = await getSupabaseClient(); - const body = req.body; - const asset = body.asset?.toUpperCase(); - logger.info({ merchantId: req.merchant?.id, amount: body.amount, asset: body.asset }, "DEBUG: createSession started"); - - // Shared business-rule validation (issue #1087) — issuer presence/format. - const { assetIssuer, rejection: issuerRejection } = resolveAndValidateIssuer( - asset, - body.asset_issuer, - ); - if (issuerRejection) { - paymentFailedCounter.inc({ asset: body.asset, reason: issuerRejection.reason }); - paymentProcessorSessionsTotal.inc({ asset: body.asset, outcome: "validation_failed" }); - paymentProcessorSessionDuration.observe( - { asset: body.asset, outcome: "validation_failed" }, - (Date.now() - sessionStart) / 1000, - ); - return res.status(400).json({ error: issuerRejection.message }); - } - const body = validation.payload; - const { asset, assetIssuer } = validation; - - // Shared business-rule validation (issue #1087) — per-asset limits (#153). - const limitRejection = validatePerAssetLimits({ - rawAsset: body.asset, - amount: body.amount, - paymentLimits: req.merchant.payment_limits, + logger.info({ merchantId: req.merchant?.id, amount: req.body?.amount, asset: req.body?.asset }, "DEBUG: createSession started"); + + // Sanitization, strict payload checks and shared business rules + // (issues #1087, #1447) with validator metrics/health (#1448). + const validation = validatePaymentSession({ + body: req.body, + merchant: req.merchant, + source: "http", }); - if (limitRejection) { - paymentFailedCounter.inc({ asset: body.asset, reason: limitRejection.reason }); - paymentProcessorSessionsTotal.inc({ asset: body.asset, outcome: "validation_failed" }); + if (!validation.ok) { + const { rejection } = validation; + const assetLabel = req.body?.asset; + paymentFailedCounter.inc({ + asset: assetLabel, + reason: rejection.reason === "issuer_not_allowed" ? "invalid_issuer" : rejection.reason, + }); + paymentProcessorSessionsTotal.inc({ asset: assetLabel, outcome: "validation_failed" }); paymentProcessorSessionDuration.observe( - { asset: body.asset, outcome: "validation_failed" }, + { asset: assetLabel, outcome: "validation_failed" }, (Date.now() - sessionStart) / 1000, ); return res.status(400).json({ - error: limitRejection.message, - ...limitRejection.details, + error: rejection.message, + ...(rejection.rule === "limits" ? rejection.details : {}), }); } - - // Shared business-rule validation (issue #1087) — allowed-issuers check: - // if the merchant has configured a non-empty allowlist, only those - // issuer addresses may be used. - const allowedIssuerRejection = validateAllowedIssuers({ - asset, - assetIssuer, - allowedIssuers: req.merchant.allowed_issuers, - }); - if (allowedIssuerRejection) { - paymentFailedCounter.inc({ asset: body.asset, reason: "invalid_issuer" }); - paymentProcessorSessionsTotal.inc({ asset: body.asset, outcome: "validation_failed" }); - paymentProcessorSessionDuration.observe( - { asset: body.asset, outcome: "validation_failed" }, - (Date.now() - sessionStart) / 1000, - ); - return res.status(400).json({ error: allowedIssuerRejection.message }); - } + const body = validation.payload; + const { asset, assetIssuer } = validation; const isSandbox = body.sandbox === true; const baseId = randomUUID(); @@ -497,8 +443,15 @@ function createPaymentsRouter({ try { const supabase = await getSupabaseClient(); // --- Redis read-through cache --- + // Scoped by req.merchant?.id (issue #1311): this route is reachable + // both anonymously (a customer following their payment_link) and by + // an authenticated merchant, and a cache hit must reflect the same + // merchant_id scoping the uncached query below applies — otherwise + // whichever caller populates the cache first decides what every + // later caller for that payment id sees, bypassing the scoping + // entirely on a cache hit. const redis = await connectRedisClient(); - const cached = await getCachedPayment(redis, req.params.id); + const cached = await getCachedPayment(redis, req.params.id, req.merchant?.id); if (cached) { paymentProcessorStatusCacheHits.inc(); return res.json({ payment: cached }); @@ -583,7 +536,7 @@ function createPaymentsRouter({ // Only cache confirmed/completed payments — never cache pending // so status changes are immediately visible to pollers if (data.status === "confirmed" || data.status === "completed") { - await setCachedPayment(redis, req.params.id, response); + await setCachedPayment(redis, req.params.id, response, req.merchant?.id); } // Prevent HTTP-level caching so 304 responses never mask status changes @@ -737,21 +690,38 @@ function createPaymentsRouter({ const diff = received - expected; if (diff < -0.0000001) { - // Underpayment — mark as failed with details - await supabase.from("payments").update({ - status: "failed", - tx_id: anyPayment.transaction_hash, - metadata: { - ...(data.metadata || {}), - failure_reason: "underpayment", - expected_amount: expected, - received_amount: received, - shortfall: Number((expected - received).toFixed(7)), - }, - }).eq("id", data.id); + // Underpayment — mark as failed with details. + // Conditional on status = "pending" (issue #1310, same class + // of race as the exact-match path below): two concurrent + // verify calls both reading "pending" before either writes + // must not both apply a terminal transition and both report + // success — only the call whose UPDATE actually matched a row + // proceeds past this point. + const { data: underpaymentUpdated } = await supabase + .from("payments") + .update({ + status: "failed", + tx_id: anyPayment.transaction_hash, + metadata: { + ...(data.metadata || {}), + failure_reason: "underpayment", + expected_amount: expected, + received_amount: received, + shortfall: Number((expected - received).toFixed(7)), + }, + }) + .eq("id", data.id) + .eq("status", "pending") + .select("id") + .maybeSingle(); + + if (!underpaymentUpdated) { + recordVerificationOutcome("tx_claim_conflict", data.asset); + return res.json({ status: "pending" }); // already processed concurrently + } const redis = await connectRedisClient(); - await invalidatePaymentCache(redis, data.id); + await invalidatePaymentCache(redis, data.id, data.merchant_id); recordVerificationOutcome("underpayment", data.asset); return res.status(402).json({ @@ -766,25 +736,37 @@ function createPaymentsRouter({ } if (diff > 0.0000001) { - // Overpayment — still confirm but flag it + // Overpayment — still confirm but flag it. Same conditional- + // update guard as underpayment above (issue #1310). const createdAt = new Date(data.created_at); const latencySeconds = (new Date() - createdAt) / 1000; - await supabase.from("payments").update({ - status: "confirmed", - tx_id: anyPayment.transaction_hash, - completion_duration_seconds: Math.floor(latencySeconds), - metadata: { - ...(data.metadata || {}), - overpayment: true, - expected_amount: expected, - received_amount: received, - excess: Number((received - expected).toFixed(7)), - }, - }).eq("id", data.id); + const { data: overpaymentUpdated } = await supabase + .from("payments") + .update({ + status: "confirmed", + tx_id: anyPayment.transaction_hash, + completion_duration_seconds: Math.floor(latencySeconds), + metadata: { + ...(data.metadata || {}), + overpayment: true, + expected_amount: expected, + received_amount: received, + excess: Number((received - expected).toFixed(7)), + }, + }) + .eq("id", data.id) + .eq("status", "pending") + .select("id") + .maybeSingle(); + + if (!overpaymentUpdated) { + recordVerificationOutcome("tx_claim_conflict", data.asset); + return res.json({ status: "pending" }); // already processed concurrently + } const redis = await connectRedisClient(); - await invalidatePaymentCache(redis, data.id); + await invalidatePaymentCache(redis, data.id, data.merchant_id); recordVerificationOutcome("overpayment", data.asset); return res.json({ @@ -852,7 +834,7 @@ function createPaymentsRouter({ // --- Invalidate cache so next poll sees confirmed status immediately --- const redis = await connectRedisClient(); - await invalidatePaymentCache(redis, data.id); + await invalidatePaymentCache(redis, data.id, data.merchant_id); // Record metrics for confirmation paymentConfirmedCounter.inc({ asset: data.asset }); paymentConfirmationLatency.observe({ asset: data.asset }, latencySeconds); @@ -1157,37 +1139,13 @@ function createPaymentsRouter({ async (req, res, next) => { try { const { tx_hash } = req.body; - const supabase = await getSupabaseClient(); - - const { data: payment, error } = await supabase - .from("payments") - .select("id, metadata") - .eq("id", req.params.id) - .eq("merchant_id", req.merchant.id) - .maybeSingle(); - if (error) { - error.status = 500; - throw error; - } - - if (!payment) { - return res.status(404).json({ error: "Payment not found" }); - } - - await supabase - .from("payments") - .update({ - metadata: { - ...payment.metadata, - refund_status: "refunded", - refund_tx_hash: tx_hash, - refund_confirmed_at: new Date().toISOString(), - }, - }) - .eq("id", payment.id); - - paymentProcessorRefundsTotal.inc({ stage: "confirm", outcome: "success" }); + // Delegates to paymentService.confirmRefundTx, which verifies + // tx_hash against the refund transaction generateRefundTx produced + // and confirms it landed on-chain before marking the payment + // refunded (issue #1309) — this route previously duplicated an + // older, unverified version of this logic inline. + await paymentService.confirmRefundTx(req.params.id, req.merchant.id, tx_hash); res.json({ status: "refunded", @@ -1262,6 +1220,12 @@ function createPaymentsRouter({ */ router.get( "/path-payment-quote/:id", + // This route sits outside the `/api/payments` prefix that app.js gates + // with requireApiKeyAuth(), so without its own auth here it was + // completely unauthenticated: any caller could read another merchant's + // payment amount/asset/recipient by id, and generate live Horizon quotes + // against it for free (issue #1309). + requireApiKeyAuth(), pathPaymentQuoteRateLimit, validateUuidParam(), validateRequest({ query: pathPaymentQuoteQuerySchema }), @@ -1275,16 +1239,11 @@ function createPaymentsRouter({ const supabase = await getSupabaseClient(); const sourceAccount = req.query.source_account; - let query = supabase + const { data, error } = await supabase .from("payments") - .select("id, amount, asset, asset_issuer, recipient, status"); - - if (req.merchant?.id) { - query = query.eq("merchant_id", req.merchant.id); - } - - const { data, error } = await query + .select("id, amount, asset, asset_issuer, recipient, status") .eq("id", req.params.id) + .eq("merchant_id", req.merchant.id) .is("deleted_at", null) .maybeSingle(); diff --git a/backend/src/services/paymentService-security-audit.test.js b/backend/src/services/paymentService-security-audit.test.js index 0751c689..96758402 100644 --- a/backend/src/services/paymentService-security-audit.test.js +++ b/backend/src/services/paymentService-security-audit.test.js @@ -248,16 +248,18 @@ describe("Payment Processor Security Audit", () => { }, error: null, }); - const update = vi.fn().mockResolvedValue({ error: null }); mockSupabaseFrom.mockReturnValue({ select: vi.fn().mockReturnValue({ eq: vi.fn().mockReturnThis(), is: vi.fn().mockReturnThis(), maybeSingle, }), + // update().eq(id).eq(status).select() — the conditional-update guard + // from issue #1310. Resolving with a non-empty array simulates this + // call being the one that won the race. update: vi.fn().mockReturnValue({ eq: vi.fn().mockReturnThis(), - is: vi.fn().mockReturnThis(), + select: vi.fn().mockResolvedValue({ data: [{ id: "pay_1" }], error: null }), }), }); diff --git a/backend/src/services/paymentService.js b/backend/src/services/paymentService.js index ee52a3a6..611631db 100644 --- a/backend/src/services/paymentService.js +++ b/backend/src/services/paymentService.js @@ -5,6 +5,7 @@ import { createRefundTransaction, findStrictReceivePaths, verifyTransactionSignature, + isValidTransactionHash, } from "../lib/stellar.js"; import { resolveBrandingConfig } from "../lib/branding.js"; import { sendWebhook } from "../lib/webhooks.js"; @@ -577,7 +578,7 @@ export const paymentService = { const supabase = await getSupabaseClient(); // --- Redis read-through cache --- const redis = await connectRedisClient(); - const cached = await getCachedPayment(redis, paymentId); + const cached = await getCachedPayment(redis, paymentId, merchantId); if (cached) { paymentProcessorStatusCacheHits.inc(); return { payment: cached }; @@ -621,7 +622,7 @@ export const paymentService = { delete response.merchants; // Cache the result to absorb polling bursts - await setCachedPayment(redis, paymentId, response); + await setCachedPayment(redis, paymentId, response, merchantId); return { payment: response }; }, @@ -712,23 +713,45 @@ export const paymentService = { const now = new Date(); const latencySeconds = (now - createdAt) / 1000; - const { error: updateError } = await supabase + // Conditional update (issue #1310): the initial `data.status === "confirmed"` + // check above and this write are not atomic — two concurrent calls to + // verifyPayment() for the same paymentId (e.g. a webhook-triggered check + // racing a client poll) can both read "pending" before either writes. + // Filtering the UPDATE on the status this call observed, and checking + // whether a row actually matched, makes only ONE of the racing calls the + // winner. The loser's `updatedRows` comes back empty and it must not + // proceed to fire webhooks/sockets/emails or bump confirmation metrics a + // second time for a payment another call already confirmed. + const { data: updatedRows, error: updateError } = await supabase .from("payments") .update({ status: "confirmed", tx_id: match.transaction_hash, completion_duration_seconds: Math.floor(latencySeconds) }) - .eq("id", data.id); + .eq("id", data.id) + .eq("status", data.status) + .select("id"); if (updateError) { updateError.status = 500; throw updateError; } - // Invalidate cache + if (!updatedRows || updatedRows.length === 0) { + // Another concurrent call already confirmed this payment between our + // read and this write. Report success without repeating side effects. + recordVerificationOutcome("already_confirmed_concurrent"); + return { + status: "confirmed", + tx_id: match.transaction_hash, + ledger_url: `https://stellar.expert/explorer/testnet/tx/${match.transaction_hash}`, + }; + } + + // Invalidate cache (both the public and merchant-scoped entries — #1311) const redis = await connectRedisClient(); - await invalidatePaymentCache(redis, data.id); + await invalidatePaymentCache(redis, data.id, data.merchant_id); // Record metrics paymentConfirmedCounter.inc({ asset: data.asset }); @@ -883,6 +906,13 @@ export const paymentService = { ...payment.metadata, refund_status: "pending", refund_xdr: refundTx.xdr, + // The Stellar transaction hash is computed over the unsigned + // envelope + network passphrase, not the signatures — it is + // stable once this merchant signs and submits refundTx.xdr + // unmodified. Recording it now lets confirmRefundTx verify the + // caller-supplied tx_hash actually corresponds to the refund we + // generated, rather than accepting any string (issue #1309). + refund_tx_hash_expected: refundTx.hash, refund_created_at: new Date().toISOString(), }, }) @@ -920,6 +950,70 @@ export const paymentService = { throw err; } + // Verify the caller-supplied tx_hash before ever marking a refund + // "refunded" (issue #1309). Previously this call took the merchant's + // word for it with no check at all — a merchant could call this + // endpoint with any string and the payment would be recorded as + // refunded without funds ever moving. + if (!isValidTransactionHash(txHash)) { + const err = new Error("Invalid transaction hash"); + err.status = 400; + throw err; + } + + const expectedHash = payment.metadata?.refund_tx_hash_expected; + if (!expectedHash) { + // generateRefundTx was never called for this payment (or predates + // this field) — there is nothing to verify the submitted hash + // against, so refuse rather than accept it on faith. + paymentProcessorRefundsTotal.inc({ stage: "confirm", outcome: "rejected" }); + const err = new Error( + "No pending refund found for this payment. Call the refund endpoint first.", + ); + err.status = 400; + throw err; + } + + if (txHash.toLowerCase() !== expectedHash.toLowerCase()) { + paymentProcessorRefundsTotal.inc({ stage: "confirm", outcome: "rejected" }); + const err = new Error( + "Transaction hash does not match the refund transaction generated for this payment", + ); + err.status = 400; + throw err; + } + + // Confirm the transaction actually landed on-chain and succeeded — a + // hash match alone only proves the merchant built the right envelope, + // not that they ever signed and submitted it. + const StellarSdk = await import("stellar-sdk"); + const HORIZON_URL = + process.env.STELLAR_HORIZON_URL || + (process.env.STELLAR_NETWORK === "public" + ? "https://horizon.stellar.org" + : "https://horizon-testnet.stellar.org"); + const server = new StellarSdk.Horizon.Server(HORIZON_URL); + + let onChainTx; + try { + onChainTx = await server.transactions().transaction(txHash).call(); + } catch (horizonErr) { + paymentProcessorRefundsTotal.inc({ stage: "confirm", outcome: "not_found" }); + const err = new Error( + "Transaction not found on Stellar network. Submit it before confirming.", + ); + err.status = 400; + err.cause = horizonErr; + throw err; + } + + if (!onChainTx.successful) { + paymentProcessorRefundsTotal.inc({ stage: "confirm", outcome: "failed_on_chain" }); + const err = new Error("Refund transaction failed on the Stellar network"); + err.status = 400; + throw err; + } + await supabase .from("payments") .update({ diff --git a/backend/src/services/paymentService.test.js b/backend/src/services/paymentService.test.js index 796cd642..c3c6915e 100644 --- a/backend/src/services/paymentService.test.js +++ b/backend/src/services/paymentService.test.js @@ -54,6 +54,8 @@ vi.mock("../lib/stellar.js", () => ({ withHorizonRetry: vi.fn().mockResolvedValue(undefined), isValidAssetCode: vi.fn().mockReturnValue(true), isValidStellarAccountId: vi.fn().mockReturnValue(true), + isValidTransactionHash: (value) => + typeof value === "string" && /^[0-9a-fA-F]{64}$/.test(value), })); vi.mock("../lib/branding.js", () => ({ @@ -90,6 +92,22 @@ vi.mock("../lib/metrics.js", () => ({ paymentFailedCounter: { inc: vi.fn() }, })); +const { mockHorizonTransaction } = vi.hoisted(() => ({ + mockHorizonTransaction: vi.fn(), +})); + +vi.mock("stellar-sdk", () => ({ + Horizon: { + Server: vi.fn(() => ({ + transactions: () => ({ + transaction: (hash) => ({ + call: () => mockHorizonTransaction(hash), + }), + }), + })), + }, +})); + import { paymentService } from "./paymentService.js"; const USDC_TESTNET_ISSUER = "GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5"; @@ -409,4 +427,290 @@ describe("paymentService", () => { expect(result).toEqual({ status: "pending" }); expect(mockVerifyTransactionSignature).toHaveBeenCalledWith("tx-invalid"); }); + + describe("getPaymentStatus cache scoping (issue #1311)", () => { + beforeEach(() => { + mockGetCachedPayment.mockResolvedValue(null); + mockSetCachedPayment.mockResolvedValue(undefined); + mockConnectRedisClient.mockResolvedValue({}); + }); + + it("reads and writes the cache scoped to the merchantId this call was made with", async () => { + const maybeSingle = vi.fn().mockResolvedValue({ + data: { + id: "payment-1", + amount: "10", + asset: "XLM", + asset_issuer: null, + recipient: "GDEST", + description: null, + memo: null, + memo_type: null, + status: "confirmed", + tx_id: "tx-1", + metadata: {}, + created_at: new Date().toISOString(), + merchants: { branding_config: null }, + }, + error: null, + }); + mockSupabaseFrom.mockReturnValue({ + select: vi.fn(() => ({ + eq: vi.fn().mockReturnThis(), + is: vi.fn().mockReturnThis(), + maybeSingle, + })), + }); + + await paymentService.getPaymentStatus("payment-1", "merchant-1"); + + expect(mockGetCachedPayment).toHaveBeenCalledWith( + expect.anything(), + "payment-1", + "merchant-1", + ); + expect(mockSetCachedPayment).toHaveBeenCalledWith( + expect.anything(), + "payment-1", + expect.objectContaining({ id: "payment-1" }), + "merchant-1", + ); + }); + + it("passes undefined merchant scope through as null when called without one (public payment_link lookup)", async () => { + const maybeSingle = vi.fn().mockResolvedValue({ + data: { + id: "payment-1", + amount: "10", + asset: "XLM", + asset_issuer: null, + recipient: "GDEST", + description: null, + memo: null, + memo_type: null, + status: "confirmed", + tx_id: "tx-1", + metadata: {}, + created_at: new Date().toISOString(), + merchants: { branding_config: null }, + }, + error: null, + }); + mockSupabaseFrom.mockReturnValue({ + select: vi.fn(() => ({ + eq: vi.fn().mockReturnThis(), + is: vi.fn().mockReturnThis(), + maybeSingle, + })), + }); + + await paymentService.getPaymentStatus("payment-1"); + + expect(mockGetCachedPayment).toHaveBeenCalledWith(expect.anything(), "payment-1", null); + }); + }); + + describe("verifyPayment concurrent confirmation (issue #1310)", () => { + const basePayment = { + id: "payment-1", + merchant_id: "merchant-1", + amount: "12.5", + asset: "USDC", + asset_issuer: "issuer-1", + recipient: "GDEST", + status: "pending", + tx_id: null, + memo: null, + memo_type: null, + webhook_url: "https://example.com/webhook", + created_at: "2026-04-24T10:00:00.000Z", + merchants: { + webhook_secret: "secret", + webhook_version: "v1", + notification_email: "merchant@example.com", + email: "merchant@example.com", + }, + }; + + function mockSupabaseForVerify({ updatedRows }) { + const maybeSingle = vi.fn().mockResolvedValue({ data: basePayment, error: null }); + const updateSelect = vi.fn().mockResolvedValue({ data: updatedRows, error: null }); + const updateEqStatus = vi.fn(() => ({ select: updateSelect })); + const updateEqId = vi.fn(() => ({ eq: updateEqStatus })); + const update = vi.fn(() => ({ eq: updateEqId })); + + mockSupabaseFrom.mockReturnValue({ + select: vi.fn(() => ({ + eq: vi.fn().mockReturnThis(), + is: vi.fn().mockReturnThis(), + maybeSingle, + })), + update, + }); + + return { update, updateEqId, updateEqStatus, updateSelect }; + } + + beforeEach(() => { + mockFindMatchingPayment.mockResolvedValue({ transaction_hash: "tx-1" }); + mockVerifyTransactionSignature.mockResolvedValue({ valid: true }); + mockConnectRedisClient.mockResolvedValue({}); + mockInvalidatePaymentCache.mockResolvedValue(undefined); + mockGetPayloadForVersion.mockReturnValue({ event: "payment.confirmed" }); + mockSendWebhook.mockResolvedValue({ delivered: true }); + }); + + it("filters the confirming UPDATE on the status it read, so only one racing call wins", async () => { + const { updateEqId, updateEqStatus } = mockSupabaseForVerify({ + updatedRows: [{ id: "payment-1" }], + }); + + await paymentService.verifyPayment("payment-1"); + + expect(updateEqId).toHaveBeenCalledWith("id", "payment-1"); + expect(updateEqStatus).toHaveBeenCalledWith("status", "pending"); + }); + + it("fires webhooks/metrics when this call's UPDATE actually matched a row", async () => { + mockSupabaseForVerify({ updatedRows: [{ id: "payment-1" }] }); + + const result = await paymentService.verifyPayment("payment-1"); + + expect(result.status).toBe("confirmed"); + expect(mockSendWebhook).toHaveBeenCalledTimes(1); + expect(mockInvalidatePaymentCache).toHaveBeenCalledTimes(1); + }); + + it("does not re-fire webhooks/emails when a concurrent call already confirmed the payment", async () => { + // The UPDATE ... WHERE status = 'pending' matched zero rows: another + // concurrent verifyPayment() call for the same payment won the race + // and already flipped the status. + mockSupabaseForVerify({ updatedRows: [] }); + + const result = await paymentService.verifyPayment("payment-1"); + + expect(result).toEqual({ + status: "confirmed", + tx_id: "tx-1", + ledger_url: "https://stellar.expert/explorer/testnet/tx/tx-1", + }); + expect(mockSendWebhook).not.toHaveBeenCalled(); + expect(mockInvalidatePaymentCache).not.toHaveBeenCalled(); + }); + }); + + describe("confirmRefundTx verification (issue #1309)", () => { + function mockSupabaseForConfirm(payment) { + const maybeSingle = vi.fn().mockResolvedValue({ data: payment, error: null }); + const update = vi.fn(() => ({ eq: vi.fn().mockResolvedValue({ data: null, error: null }) })); + + mockSupabaseFrom.mockReturnValue({ + select: vi.fn(() => ({ + eq: vi.fn().mockReturnThis(), + maybeSingle, + })), + update, + }); + + return { update }; + } + + const validHash = + "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2"; + + it("rejects a malformed transaction hash before touching the database write", async () => { + mockSupabaseForConfirm({ + id: "payment-1", + metadata: { refund_tx_hash_expected: validHash }, + }); + + await expect( + paymentService.confirmRefundTx("payment-1", "merchant-1", "not-a-hash"), + ).rejects.toMatchObject({ status: 400, message: "Invalid transaction hash" }); + }); + + it("rejects when no refund was ever generated for this payment", async () => { + mockSupabaseForConfirm({ id: "payment-1", metadata: {} }); + + await expect( + paymentService.confirmRefundTx("payment-1", "merchant-1", validHash), + ).rejects.toMatchObject({ status: 400 }); + expect(mockHorizonTransaction).not.toHaveBeenCalled(); + }); + + it("rejects a tx_hash that does not match the generated refund transaction", async () => { + mockSupabaseForConfirm({ + id: "payment-1", + metadata: { refund_tx_hash_expected: validHash }, + }); + const wrongHash = "f".repeat(64); + + await expect( + paymentService.confirmRefundTx("payment-1", "merchant-1", wrongHash), + ).rejects.toMatchObject({ + status: 400, + message: expect.stringContaining("does not match"), + }); + expect(mockHorizonTransaction).not.toHaveBeenCalled(); + }); + + it("rejects when the matching transaction cannot be found on Stellar", async () => { + mockSupabaseForConfirm({ + id: "payment-1", + metadata: { refund_tx_hash_expected: validHash }, + }); + mockHorizonTransaction.mockRejectedValue(new Error("404 Not Found")); + + await expect( + paymentService.confirmRefundTx("payment-1", "merchant-1", validHash), + ).rejects.toMatchObject({ status: 400 }); + }); + + it("rejects when the transaction exists but failed on-chain", async () => { + mockSupabaseForConfirm({ + id: "payment-1", + metadata: { refund_tx_hash_expected: validHash }, + }); + mockHorizonTransaction.mockResolvedValue({ successful: false }); + + await expect( + paymentService.confirmRefundTx("payment-1", "merchant-1", validHash), + ).rejects.toMatchObject({ + status: 400, + message: "Refund transaction failed on the Stellar network", + }); + }); + + it("confirms the refund once the hash matches and the transaction succeeded on-chain", async () => { + const { update } = mockSupabaseForConfirm({ + id: "payment-1", + metadata: { refund_tx_hash_expected: validHash, refund_status: "pending" }, + }); + mockHorizonTransaction.mockResolvedValue({ successful: true }); + + const result = await paymentService.confirmRefundTx("payment-1", "merchant-1", validHash); + + expect(result).toEqual({ message: "Refund confirmed successfully" }); + expect(update).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.objectContaining({ + refund_status: "refunded", + refund_tx_hash: validHash, + }), + }), + ); + }); + + it("accepts the hash comparison case-insensitively", async () => { + mockSupabaseForConfirm({ + id: "payment-1", + metadata: { refund_tx_hash_expected: validHash.toUpperCase() }, + }); + mockHorizonTransaction.mockResolvedValue({ successful: true }); + + await expect( + paymentService.confirmRefundTx("payment-1", "merchant-1", validHash), + ).resolves.toEqual({ message: "Refund confirmed successfully" }); + }); + }); });