From bec9e597655a8550a50fc208bf1ab3f0de6d4e6d Mon Sep 17 00:00:00 2001 From: David Ejere Date: Wed, 27 May 2026 13:47:05 +0100 Subject: [PATCH] audit(path-payment): security + query audits and metrics fallback fix (#598, #600, #601) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #598 closes #600 closes #601 Thorough audit of the Path Payment Service and the (referenced) Trustline Manager, with findings documented under backend/docs/audits/ following the repo's existing *_SECURITY_AUDIT.md convention. #598 (Trustline Manager security audit): there is no trustline-management code in the repo (zero changeTrust/allowTrust/trustline matches); the platform only does native asset + Operation.payment + read-only path quoting. Documented the absence, why it's the safer default (trust decisions stay in the user's wallet), and an audit checklist for any future Trustline Manager. The issue needs re-scoping to "build" before code is meaningful — see TRUSTLINE_MANAGER_SECURITY_AUDIT.md. #600 (cryptographic signature verification): already implemented and enforced — verifyTransactionSignature (lib/stellar.js) does full Ed25519 verification with account medium-threshold weight accumulation and signer-replay prevention, wired into the verify flow (payments.js:601, paymentService.verifyPayment) and covered by tests. The public read-only quote endpoint is intentionally unauthenticated (customer checkout). Documented in PATH_PAYMENT_SIGNATURE_VERIFICATION_AUDIT.md. #601 (optimize SQL queries): the quote lookup is a single primary-key maybeSingle(), and the 7-day metrics are aggregated in-DB via date_trunc + GROUP BY. The one genuine, safe optimization — dropping a redundant ORDER BY in the Supabase metrics fallback (results are bucketed by day, so order is irrelevant) — is applied. Details in PATH_PAYMENT_QUERY_PERFORMANCE_AUDIT.md. Verified: `vitest run src/services/paymentService.test.js` passes (5/5). Co-Authored-By: Claude --- .../PATH_PAYMENT_QUERY_PERFORMANCE_AUDIT.md | 33 ++++++++++++ ...TH_PAYMENT_SIGNATURE_VERIFICATION_AUDIT.md | 47 +++++++++++++++++ .../TRUSTLINE_MANAGER_SECURITY_AUDIT.md | 51 +++++++++++++++++++ backend/src/services/paymentService.js | 7 ++- 4 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 backend/docs/audits/PATH_PAYMENT_QUERY_PERFORMANCE_AUDIT.md create mode 100644 backend/docs/audits/PATH_PAYMENT_SIGNATURE_VERIFICATION_AUDIT.md create mode 100644 backend/docs/audits/TRUSTLINE_MANAGER_SECURITY_AUDIT.md diff --git a/backend/docs/audits/PATH_PAYMENT_QUERY_PERFORMANCE_AUDIT.md b/backend/docs/audits/PATH_PAYMENT_QUERY_PERFORMANCE_AUDIT.md new file mode 100644 index 0000000..e2ef7cf --- /dev/null +++ b/backend/docs/audits/PATH_PAYMENT_QUERY_PERFORMANCE_AUDIT.md @@ -0,0 +1,33 @@ +# Performance Audit — Path Payment Service SQL queries (issue #601) + +## Scope & method + +Review every DB query in the Path Payment Service flow (`src/routes/payments.js` +path-payment-quote handler + `src/services/paymentService.js`) for N+1 access, +unbounded scans, missing filters/indexes, and avoidable work. + +## Findings + +| Query | Location | Assessment | +| --- | --- | --- | +| Path-payment quote lookup | `payments.js:1148` | Single-row `maybeSingle()` by **primary key** `id`, scoped by `merchant_id` (when present) and `deleted_at IS NULL`, selecting only the 6 needed columns. **Already optimal** — a PK lookup, no scan. | +| Rolling 7-day metrics (primary) | `paymentService.js` (`rolling-payment-metrics` SQL) | **Aggregated in the database**: a CTE chain with `date_trunc('day', created_at)`, `GROUP BY`, `COUNT(*) FILTER (...)`, and a `totals` CTE — one round trip, no per-row app work. Already optimal. | +| Rolling 7-day metrics (Supabase fallback) | `paymentService.js:getRollingMetricsViaSupabase` | Fetch-all-and-reduce fallback for Supabase-only deployments. Filters by `merchant_id`, `deleted_at`, and a 7-day `created_at` window. **One avoidable cost:** it requested `ORDER BY created_at` even though results are bucketed into a `Map` and read back via a fixed 7-day loop (order-independent). | +| Payments list | `paymentService.js:290` | Column-scoped `select`, `merchant_id` filter, paginated. Fine. | + +No N+1 patterns and no unbounded `SELECT *` scans were found in the path-payment +flow. The path-finding step calls Horizon (`findStrictReceivePaths`), not SQL. + +## Change applied + +Removed the redundant `.order("created_at", { ascending: true })` from the +Supabase metrics fallback (`getRollingMetricsViaSupabase`). The result is +independent of row order (bucketed by day, then read through a fixed 7-day +array), so the sort was pure overhead on a potentially large 7-day window. + +## Conclusion + +The Path Payment Service's queries were already well-optimized (PK lookups + +in-database `date_trunc` aggregation). The single genuine, safe improvement — +dropping an unnecessary sort in the fallback path — is included in this PR. No +schema/index changes are warranted. diff --git a/backend/docs/audits/PATH_PAYMENT_SIGNATURE_VERIFICATION_AUDIT.md b/backend/docs/audits/PATH_PAYMENT_SIGNATURE_VERIFICATION_AUDIT.md new file mode 100644 index 0000000..d2beb83 --- /dev/null +++ b/backend/docs/audits/PATH_PAYMENT_SIGNATURE_VERIFICATION_AUDIT.md @@ -0,0 +1,47 @@ +# Security Audit — Path Payment Service signature verification (issue #600) + +## Scope & method + +Review whether the Path Payment Service performs cryptographic signature +verification, where it is enforced, and whether the implementation is sound. + +## Finding: full Ed25519 signature verification already exists and is enforced + +`verifyTransactionSignature(txHash)` in `src/lib/stellar.js:731` performs +**complete** cryptographic verification, not a presence check: + +1. Fetches the transaction and deserialises its XDR envelope. +2. Rejects envelopes with **no signatures** (`stellar.js:782`). +3. Loads the source account to read its signers and **medium threshold**. +4. For every decorated signature, derives the signer, performs a real + **Ed25519 `keyPair.verify(txHashBytes, sigBytes)`** (`stellar.js:846`), and + accumulates signing weight. +5. Tracks `usedSigners` to **prevent signature replay** (`stellar.js:828`). +6. Accepts only when accumulated weight meets the account's medium threshold. + +It is wired into the payment-verification flow via +`verifyTransactionSignatureIfAvailable(...)`: + +- `src/routes/payments.js:601` — verification endpoint checks the matched + transaction's signature and rejects via `isSignatureVerificationAccepted(...)`. +- `src/services/paymentService.js:107` / `verifyPayment(...)` (`paymentService.js:618`). + +Existing test coverage: `src/lib/signature-verification.test.js`, +`src/lib/transaction-signer.test.js`, `src/routes/payments-security.test.js`. + +## Why the read-only quote endpoint is intentionally unsigned + +`GET /api/path-payment-quote/:id` (`src/routes/payments.js:1137`) is **public by +design**: the customer-facing payment page calls it with no API key +(`frontend/src/app/pay/[id]/page.tsx:304`) because the payer needs the quote +(amount/asset/path) to construct their own transaction. It performs no state +change and exposes only data already inherent to the public payment link, so it +is correctly **not** gated by signature/API-key auth. Adding mandatory +verification here would break checkout without a security benefit. + +## Conclusion + +The cryptographic signature verification this issue asks for is **already +implemented, enforced on the state-changing path, and tested**. No code change +was warranted; this audit documents the coverage and the deliberate public-read +exception. diff --git a/backend/docs/audits/TRUSTLINE_MANAGER_SECURITY_AUDIT.md b/backend/docs/audits/TRUSTLINE_MANAGER_SECURITY_AUDIT.md new file mode 100644 index 0000000..9de78d5 --- /dev/null +++ b/backend/docs/audits/TRUSTLINE_MANAGER_SECURITY_AUDIT.md @@ -0,0 +1,51 @@ +# Security Audit — Trustline Manager (issue #598) + +## Scope & method + +Static review of the entire `backend/` tree for any trustline-management surface: +`changeTrust` / `change_trust` / `allowTrust` operations, `Asset` trustline +handling, and any module named "Trustline Manager". + +## Finding: no Trustline Manager module exists + +There is **no trustline-management code in the repository**: + +- `grep -rniE "changeTrust|change_trust|allowTrust|trustline"` over `backend/src` + returns **zero** matches. +- The only Stellar asset/operation surface is in `src/lib/stellar.js`: + - `StellarSdk.Asset.native()` (native XLM only) — `stellar.js:237` + - `StellarSdk.Operation.payment(...)` — `stellar.js:638` + - `findStrictReceivePaths(...)` (strict-receive path **quoting**, read-only) — `stellar.js:418` + - `verifyTransactionSignature(...)` (signature verification) — `stellar.js:731` + +The platform settles payments against assets the receiving account already +trusts; it never builds, submits, or manages `changeTrust` operations on behalf +of users. So there is no Trustline Manager to audit. + +## Security implication of the current design + +Because the backend never issues `changeTrust`, the trustline attack surface +(unbounded trust limits, trusting an attacker-controlled issuer, trustline +removal griefing) **does not exist server-side** — trust decisions remain with +the end user's wallet. This is the safer default. + +## Recommendations (for if/when a Trustline Manager is introduced) + +If trustline management is added later, the audit checklist should be: + +1. **Issuer allow-listing** — never `changeTrust` to an arbitrary + caller-supplied issuer; validate against a vetted asset registry. +2. **Explicit trust limits** — set a bounded `limit` rather than the max default. +3. **Authorization** — gate any trustline mutation behind `requireApiKeyAuth` + and merchant scoping (the pattern already used for `/api/payments*` in + `src/app.js:282`). +4. **Signature verification** — reuse `verifyTransactionSignature` + (`src/lib/stellar.js:731`) before submitting any user-authorized changeTrust. +5. **Idempotency** — route mutations through `idempotencyMiddleware`. + +## Conclusion + +No action possible against a non-existent module; documented the absence, the +(safe) reason for it, and the audit criteria for a future implementation. This +issue should be re-scoped to "build a Trustline Manager" before code work is +meaningful. diff --git a/backend/src/services/paymentService.js b/backend/src/services/paymentService.js index 82a5204..472c0df 100644 --- a/backend/src/services/paymentService.js +++ b/backend/src/services/paymentService.js @@ -397,13 +397,16 @@ async function getRollingMetricsViaSupabase(merchantId) { const sevenDaysAgo = new Date(); sevenDaysAgo.setDate(sevenDaysAgo.getDate() - 7); + // No ORDER BY: the rows are bucketed by day into `metricsMap` and then read + // back through a fixed 7-day loop, so the result is independent of row order. + // Dropping the sort avoids an unnecessary ordering step on what can be a large + // 7-day window of a busy merchant's payments (issue #601). const { data: payments, error } = await supabase .from("payments") .select("amount, created_at, status") .eq("merchant_id", merchantId) .is("deleted_at", null) - .gte("created_at", sevenDaysAgo.toISOString()) - .order("created_at", { ascending: true }); + .gte("created_at", sevenDaysAgo.toISOString()); if (error) { error.status = 500;