stellar: validate signed XDR contents before submit; store expectedHa… - #51
Open
Banx17 wants to merge 1 commit into
Open
stellar: validate signed XDR contents before submit; store expectedHa…#51Banx17 wants to merge 1 commit into
Banx17 wants to merge 1 commit into
Conversation
…sh/memo; verify on-chain before granting access; add tests
WalkthroughJest now uses an in-memory MongoDB setup. Stellar payment and donation records persist expected hashes and memos, while signed XDR is validated against expected payment details before submission. ChangesStellar payment integrity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant paymentController
participant validateSignedPaymentXdr
participant Stellar
Client->>paymentController: Submit signed XDR
paymentController->>validateSignedPaymentXdr: Check expected payments, memo, and source
validateSignedPaymentXdr-->>paymentController: Return parsed transaction or error
paymentController->>Stellar: Submit validated XDR
Stellar-->>paymentController: Return submission result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/services/stellar/stellarService.js (1)
409-420: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the public
MemoAPI over_type/_value.
tx.memo._type/tx.memo._valuereach into the SDK's private storage fields instead of the documentedmemo.type/memo.valuegetters. It works today (the getters just return these fields), but it's not the guaranteed public contract and could silently break if the SDK internals change.♻️ Proposed refactor to use public getters
if (expectedMemo) { const memo = tx.memo; let memoText = null; - if (memo && memo._type === "text") { - const val = memo._value; + if (memo && memo.type === "text") { + const val = memo.value; memoText = Buffer.isBuffer(val) ? val.toString() : String(val); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/stellar/stellarService.js` around lines 409 - 420, Update the memo validation logic in the expectedMemo block to use the public tx.memo.type and tx.memo.value getters instead of the private _type and _value fields. Preserve the existing text-type check, Buffer/string conversion, and mismatch error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/validateEnv.js`:
- Around line 34-38: Centralize test environment setup before application
imports: in src/config/validateEnv.js, remove the test-mode assignments for
JWT_SECRET, MONGO_URI, and PORT so validateEnv() only consumes preconfigured
values; in test/jest.setup.js, preserve the generated MongoMemoryServer URI
while moving the static JWT secret and port defaults into the earliest
environment bootstrap path.
In `@src/controllers/stellar/paymentController.js`:
- Around line 433-477: Update validateSignedPaymentXdr in stellarService.js to
recognize and validate path_payment_strict_receive operations in addition to
regular payment operations. Match path payments against the expected destination
and amount using the operation’s path-payment fields, while preserving existing
validation behavior for standard payments so legitimate path-payment XDRs pass
pre-submission validation.
In `@src/services/stellar/stellarService.js`:
- Around line 389-453: Update validateSignedPaymentXdr to include
pathPaymentStrictReceive operations alongside payment operations, mirroring the
operation handling in verifyPaymentOperations. For path payments, compare the
destination asset, destination amount, and destination address against each
expected payment while preserving the existing native/USDC payment matching
behavior.
---
Nitpick comments:
In `@src/services/stellar/stellarService.js`:
- Around line 409-420: Update the memo validation logic in the expectedMemo
block to use the public tx.memo.type and tx.memo.value getters instead of the
private _type and _value fields. Preserve the existing text-type check,
Buffer/string conversion, and mismatch error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 708eb564-bb9f-4865-afe3-c1cdc522109a
📒 Files selected for processing (7)
jest.config.jssrc/config/validateEnv.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/models/Transaction.jssrc/services/stellar/stellarService.jstest/jest.setup.js
Comment on lines
+34
to
+38
| // Test mode: provide defaults for development of tests | ||
| if (process.env.NODE_ENV === "test") { | ||
| process.env.JWT_SECRET = process.env.JWT_SECRET || "test-secret-key-at-least-32-characters-long"; | ||
| process.env.MONGO_URI = process.env.MONGO_URI || "mongodb://test-db:27017/dnb-test"; | ||
| process.env.PORT = process.env.PORT || "5000"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Centralize test configuration before application imports.
The two files duplicate source-controlled test defaults, and validateEnv() can run before beforeAll publishes the in-memory URI. Remove hardcoded credentials/configuration and establish the test environment before importing application modules.
src/config/validateEnv.js#L34-L38: remove the JWT, Mongo URI, and port literals; require values from test bootstrap/environment.test/jest.setup.js#L11-L14: retain the generated MongoMemoryServer URI, but move static defaults to environment bootstrap that runs before app imports.
📍 Affects 2 files
src/config/validateEnv.js#L34-L38(this comment)test/jest.setup.js#L11-L14
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/validateEnv.js` around lines 34 - 38, Centralize test environment
setup before application imports: in src/config/validateEnv.js, remove the
test-mode assignments for JWT_SECRET, MONGO_URI, and PORT so validateEnv() only
consumes preconfigured values; in test/jest.setup.js, preserve the generated
MongoMemoryServer URI while moving the static JWT secret and port defaults into
the earliest environment bootstrap path.
Source: Path instructions
Comment on lines
+433
to
+477
| // Build expected payments to validate XDR BEFORE submit | ||
| let expectedPayments = transaction.platformFee?.platformAmount | ||
| ? [ | ||
| { | ||
| destination: transaction.creatorWallet, | ||
| amount: transaction.platformFee.creatorAmount, | ||
| }, | ||
| { | ||
| destination: transaction.platformFee.platformWallet, | ||
| amount: transaction.platformFee.platformAmount, | ||
| }, | ||
| ] | ||
| : [ | ||
| { | ||
| destination: transaction.creatorWallet, | ||
| amount: transaction.amount, | ||
| }, | ||
| ]; | ||
|
|
||
| // Validate signed XDR contents (memo, payments, optional source) | ||
| try { | ||
| validateSignedPaymentXdr( | ||
| signedXdr, | ||
| expectedPayments, | ||
| transaction.memo, | ||
| transaction.buyerWallet, | ||
| true | ||
| ); | ||
| } catch (validationError) { | ||
| transaction.status = "failed"; | ||
| transaction.failureReason = `validation_failed: ${validationError.message}`; | ||
| await transaction.save({ session }); | ||
| await session.commitTransaction(); | ||
| paymentsFailed.inc({ type: "purchase", reason: "validation_failed" }); | ||
|
|
||
| logger.error(`Transaction ${transactionId} validation failed:`, validationError.message); | ||
|
|
||
| return res.status(400).json({ | ||
| success: false, | ||
| message: "Signed transaction does not match expected payment details", | ||
| error: validationError.message, | ||
| }); | ||
| } | ||
|
|
||
| // Update status to submitted after validation |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Path payments will always fail this pre-submission check.
expectedPayments here doesn't account for path payments (see isPathPayment branch at Lines 278-304/295-304, which builds a path_payment_strict_receive operation via buildPathPaymentTransaction). validateSignedPaymentXdr in stellarService.js only matches on op.type === "payment", so a signed path-payment XDR will never find a match and every legitimate path payment will be rejected with 400 and marked failed. Root cause and fix belong in stellarService.js's validateSignedPaymentXdr — see that file's review for details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/controllers/stellar/paymentController.js` around lines 433 - 477, Update
validateSignedPaymentXdr in stellarService.js to recognize and validate
path_payment_strict_receive operations in addition to regular payment
operations. Match path payments against the expected destination and amount
using the operation’s path-payment fields, while preserving existing validation
behavior for standard payments so legitimate path-payment XDRs pass
pre-submission validation.
Comment on lines
+389
to
+453
| /** | ||
| * Validate a signed transaction XDR against expected payments and memo/source | ||
| * @param {string} signedXdr | ||
| * @param {Array<{destination:string, amount:string}>} expectedPayments | ||
| * @param {string} expectedMemo | ||
| * @param {string} expectedSource | ||
| * @param {boolean} requireSource | ||
| */ | ||
| export const validateSignedPaymentXdr = ( | ||
| signedXdr, | ||
| expectedPayments = [], | ||
| expectedMemo, | ||
| expectedSource, | ||
| requireSource = true | ||
| ) => { | ||
| const tx = StellarSdk.TransactionBuilder.fromXDR( | ||
| signedXdr, | ||
| networkPassphrase | ||
| ); | ||
|
|
||
| // Memo check | ||
| if (expectedMemo) { | ||
| const memo = tx.memo; | ||
| let memoText = null; | ||
| if (memo && memo._type === "text") { | ||
| const val = memo._value; | ||
| memoText = Buffer.isBuffer(val) ? val.toString() : String(val); | ||
| } | ||
| if (memoText !== expectedMemo) { | ||
| throw new Error("Memo mismatch"); | ||
| } | ||
| } | ||
|
|
||
| // Source check | ||
| if (requireSource && expectedSource) { | ||
| if (tx.source !== expectedSource) { | ||
| throw new Error("Source account mismatch"); | ||
| } | ||
| } | ||
|
|
||
| // Payment operations check | ||
| const paymentOps = tx.operations.filter((op) => op.type === "payment"); | ||
|
|
||
| for (const expected of expectedPayments) { | ||
| const match = paymentOps.find((op) => { | ||
| const assetMatches = | ||
| (op.asset && op.asset.code === "USDC" && op.asset.issuer === USDC_ISSUER) || | ||
| (op.asset_type === "credit_alphanum4" && op.asset?.code === "USDC" && op.asset?.issuer === USDC_ISSUER); | ||
|
|
||
| const amountMatches = toStroops(op.amount) === toStroops(expected.amount); | ||
| const destMatches = op.destination === expected.destination; | ||
|
|
||
| return assetMatches && amountMatches && destMatches; | ||
| }); | ||
|
|
||
| if (!match) { | ||
| throw new Error( | ||
| `Signed XDR missing expected USDC payment of ${expected.amount} to ${expected.destination}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return tx; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols first.
ast-grep outline src/services/stellar/stellarService.js --view expanded || true
# Show the target region with line numbers.
sed -n '340,560p' src/services/stellar/stellarService.js | cat -n
# Find the path-payment builder / initializer references.
rg -n "buildPathPaymentTransaction|isPathPayment|validateSignedPaymentXdr|verifyPaymentOperations|path_payment_strict_receive|pathPaymentStrictReceive|toStroops" src/services/stellar/stellarService.jsRepository: Deen-Bridge/dnb-backend
Length of output: 10474
validateSignedPaymentXdr should accept path payments.
paymentOps only keeps op.type === "payment", so valid pathPaymentStrictReceive XDRs fail this pre-submission check and get marked failed before the on-chain verifier runs. Mirror the verifyPaymentOperations branch here and compare the path-payment destination asset and amount.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/stellar/stellarService.js` around lines 389 - 453, Update
validateSignedPaymentXdr to include pathPaymentStrictReceive operations
alongside payment operations, mirroring the operation handling in
verifyPaymentOperations. For path payments, compare the destination asset,
destination amount, and destination address against each expected payment while
preserving the existing native/USDC payment matching behavior.
Source: Path instructions
This was referenced Jul 22, 2026
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.
Closes #18
Summary by CodeRabbit
New Features
Bug Fixes