diff --git a/backend/src/lib/path-payment-recovery.test.js b/backend/src/lib/path-payment-recovery.test.js index 4f973e3..9aca7eb 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 0000000..acdbffc --- /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 f8a680e..91ea1b9 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 df726bc..80a9c18 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 2628c93..dc69368 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 a8a0c17..847120e 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 0751c68..9675840 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 ee52a3a..611631d 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 796cd64..c3c6915 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" }); + }); + }); });