audit(path-payment): security + query audits + metrics fallback fix (#598, #600, #601)#838
Open
davedumto wants to merge 1 commit into
Open
Conversation
…emdevelopa#598, emdevelopa#600, emdevelopa#601) closes emdevelopa#598 closes emdevelopa#600 closes emdevelopa#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. emdevelopa#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. emdevelopa#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. emdevelopa#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 <noreply@anthropic.com>
|
@davedumto is attempting to deploy a commit to the Emmanuel's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@davedumto Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A 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.mdconvention), plus the one genuine, safe code optimization the review surfaced.closes #598
closes #600
closes #601
#598 — Trustline Manager security audit →
TRUSTLINE_MANAGER_SECURITY_AUDIT.mdThere is no trustline-management code in the repo — zero
changeTrust/allowTrust/trustlinematches acrossbackend/src. The Stellar surface is native asset +Operation.payment+ read-only path quoting only. The audit documents the absence, explains why it's the safer default (trust decisions remain in the user's wallet, so the server-side trustline attack surface doesn't exist), and provides an audit checklist for any future implementation. This issue should be re-scoped to "build a Trustline Manager" before code work is meaningful — there is nothing to audit today.#600 — Cryptographic signature verification →
PATH_PAYMENT_SIGNATURE_VERIFICATION_AUDIT.mdAlready implemented and enforced:
verifyTransactionSignature(src/lib/stellar.js:731) performs full Ed25519 verification — rejects unsigned envelopes, accumulates signer weight against the account's medium threshold, and prevents signer replay — and is wired into the verify flow (payments.js:601,paymentService.verifyPayment) with existing test coverage. The public read-only quote endpoint is intentionally unauthenticated (customer checkout calls it with no API key —frontend/.../pay/[id]/page.tsx:304). Audit confirms coverage; no code change warranted.#601 — Optimize SQL queries →
PATH_PAYMENT_QUERY_PERFORMANCE_AUDIT.mdThe quote lookup is a single primary-key
maybeSingle(), and the 7-day metrics are aggregated in-database (date_trunc('day')+GROUP BY+ totals CTE). No N+1 or unbounded scans. The one genuine, safe optimization — removing a redundantORDER BY created_atfrom the Supabase metrics fallback (results are bucketed by day, so order is irrelevant) — is applied insrc/services/paymentService.js.Test plan
vitest run src/services/paymentService.test.js→ 5/5 passing after the change.Honest caveats