Merge pull request #838 from davedumto/audit/path-payment-service-598-600-601

audit(path-payment): security + query audits + metrics fallback fix (#598, #600, #601)

Author: emdevelopa

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.

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.

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.

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;