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;