fix: protect Stellar wallet lookups - #48
Conversation
WalkthroughStellar wallet lookups now require authentication and self-access validation. Payment and donation submissions validate signed Stellar transactions before submission, with transaction metadata supporting expected hashes and memos. New Jest setup and tests cover these flows. ChangesStellar security and payment integrity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant paymentController
participant validateSignedPaymentXdr
participant Transaction
participant StellarNetwork
Client->>paymentController: Submit signedXdr
paymentController->>Transaction: Load expected payment metadata
paymentController->>validateSignedPaymentXdr: Validate memo, source, destinations, and amounts
validateSignedPaymentXdr-->>paymentController: Validation result
paymentController->>StellarNetwork: Submit validated transaction
StellarNetwork-->>paymentController: Submission result
paymentController-->>Client: Payment response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/controllers/stellar/walletController.js (1)
180-201: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the required response envelope for both changed lookup endpoints.
The changed wallet-status responses put
hasWalletat the top level, while balance responses expose top-level balance fields; the tests lock in both incompatible shapes.
src/controllers/stellar/walletController.js#L180-L201: return{ success, message, data }for validation, success, missing-user, and failure cases; placehasWalletinsidedata.src/routes/stellar/walletRoutes.js#L20-L21: normalizegetWalletBalanceto the same envelope before exposing these changed lookup endpoints.test/stellarWalletRoutes.test.js#L88-L107: assertdataandmessagefor invalid-ID and wallet-status responses.test/stellarWalletRoutes.test.js#L149-L158: assert the balance payload underdatawith a success message.As per path instructions, new or changed endpoints require consistent response shapes
({ success, message, data }).🤖 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/walletController.js` around lines 180 - 201, Use the { success, message, data } envelope consistently: update the wallet-status handler in src/controllers/stellar/walletController.js:180-201 to place hasWallet in data and include message for validation, success, missing-user, and failure responses; normalize getWalletBalance in src/routes/stellar/walletRoutes.js:20-21 to the same shape; update src/ tests/stellarWalletRoutes.test.js:88-107 to assert data and message for invalid-ID and wallet-status responses, and src/ tests/stellarWalletRoutes.test.js:149-158 to assert the balance under data with a success message.Source: Path instructions
🤖 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/controllers/stellar/walletController.js`:
- Around line 187-188: Missing ownership authorization allows authenticated
users to query another user’s wallet status. In
src/controllers/stellar/walletController.js lines 187-188, update the controller
before the User.findById lookup to reject userId values that do not match
req.user._id, or apply the project’s privileged-role policy. In
test/stellarWalletRoutes.test.js lines 97-108, replace the permitted cross-user
lookup expectation with a 403 response test and retain coverage for successful
self-lookup.
In `@test/stellarWalletRoutes.test.js`:
- Around line 83-95: Add a test alongside the existing invalid-ID case in the
authenticated wallet route suite, using a different valid ObjectId that does not
exist. Assert the request returns status 404 and the expected missing-user
response, ensuring this path does not become a 500 server error.
- Around line 13-14: Remove the hardcoded fallback from the JWT_SECRET
initialization in stellarWalletRoutes tests and require the value to come from
the environment. Preserve the existing JWT_SECRET lookup while allowing missing
configuration to fail rather than supplying an inline signing secret, consistent
with the documented environment configuration.
---
Outside diff comments:
In `@src/controllers/stellar/walletController.js`:
- Around line 180-201: Use the { success, message, data } envelope consistently:
update the wallet-status handler in
src/controllers/stellar/walletController.js:180-201 to place hasWallet in data
and include message for validation, success, missing-user, and failure
responses; normalize getWalletBalance in
src/routes/stellar/walletRoutes.js:20-21 to the same shape; update src/
tests/stellarWalletRoutes.test.js:88-107 to assert data and message for
invalid-ID and wallet-status responses, and src/
tests/stellarWalletRoutes.test.js:149-158 to assert the balance under data with
a success message.
🪄 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: ec57be1f-b577-4d67-be6e-3fa2cd09f47d
📒 Files selected for processing (3)
src/controllers/stellar/walletController.jssrc/routes/stellar/walletRoutes.jstest/stellarWalletRoutes.test.js
…sh/memo; verify on-chain before granting access; add tests
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/controllers/stellar/walletController.js (1)
183-195: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep error payloads in the documented response shape.
The new
400and403responses omitdata, while successful responses include it. Adddata: nullhere (and align the endpoint’s remaining error branches) so clients can reliably consume{ success, message, data }.Suggested change
return res.status(400).json({ success: false, message: "Invalid user ID", + data: null, });As per path instructions, “New or changed endpoints need Jest coverage ... and consistent response shapes ({ success, message, data }).”
🤖 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/walletController.js` around lines 183 - 195, Update the invalid-user-ID and unauthorized-user checks in the wallet status endpoint to include data: null in their error payloads, and align all remaining error branches in the same endpoint with the documented { success, message, data } shape. Add or update Jest coverage for each error response to verify the consistent payload contract.Source: Path instructions
🧹 Nitpick comments (1)
src/services/stellar/stellarService.js (1)
283-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the public
Memoaccessors.
_typeand_valueare SDK internals. Usememo.typeandmemo.valueso this payment-integrity check remains compatible with the documented API. (stellar.github.io)Proposed fix
- if (memo && memo._type === "text") { - const val = memo._value; + if (memo?.type === "text") { + const val = memo.value;🤖 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 283 - 293, Update the memo validation logic around the transaction memo check to use the public memo.type and memo.value accessors instead of the internal memo._type and memo._value fields, while preserving the existing text-memo 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-40: Remove the hardcoded JWT_SECRET and MONGO_URI fallbacks
from the test-mode branch in the environment validation flow, including the
assignments in validateEnv. Keep configuration sourced exclusively from
environment variables documented in .env.example, while preserving only the
test-mode behavior and any non-sensitive default that is explicitly allowed.
In `@src/controllers/stellar/paymentController.js`:
- Around line 317-321: Update the validation-error responses in
paymentController.js at lines 317-321 and donationController.js at lines 181-185
to use the documented { success, message, data } envelope, placing validation
details under data.error or using data: null; add Jest coverage for both changed
endpoint branches and preserve their existing status and messages.
- Around line 280-297: Align the SEP-7 initialization flow with the fee-split
payments assembled in expectedPayments: when transaction.platformFee is present,
generate a multi-operation request covering both creator and platform transfers,
or omit the SEP-7 URI. Preserve the single-transfer URI for transactions without
a fee split, and add Jest regression coverage for both behaviors.
In `@src/models/Transaction.js`:
- Around line 43-49: add a migration that locates the existing stellarTxHash
index on the Transaction collection, drops it, and recreates it as a unique
sparse index matching the schema definition. Ensure the migration is safe to run
against deployments where the index is absent or already correctly configured,
and leave the stellarTxHash schema unchanged.
In `@test/jest.setup.js`:
- Line 3: Remove the JWT_SECRET fallback from test/jest.setup.js so tests
require the documented environment variable. In src/config/validateEnv.js,
remove the inline JWT and Mongo defaults and validate the documented test
environment values directly, keeping all configuration sourced from variables
defined in .env.example.
- Around line 6-8: Move the MongoMemoryServer startup currently in the setup
initialization to a Jest global setup/teardown lifecycle, ensuring one shared
instance is created for the test run and explicitly stopped afterward. Update
the MONGO_URI propagation and shared instance handling so tests continue using
the started server without leaving child processes running.
In `@test/stellarService.validation.test.js`:
- Around line 34-47: Update validateSignedPaymentXdr to verify that the signed
XDR contains a valid signature from the claimed source public key before
accepting it, while preserving the existing source, memo, and payment-operation
validation. Extend the validation tests to assert unsigned transactions and
transactions signed by a different key are rejected, while the correctly signed
transaction remains accepted.
---
Outside diff comments:
In `@src/controllers/stellar/walletController.js`:
- Around line 183-195: Update the invalid-user-ID and unauthorized-user checks
in the wallet status endpoint to include data: null in their error payloads, and
align all remaining error branches in the same endpoint with the documented {
success, message, data } shape. Add or update Jest coverage for each error
response to verify the consistent payload contract.
---
Nitpick comments:
In `@src/services/stellar/stellarService.js`:
- Around line 283-293: Update the memo validation logic around the transaction
memo check to use the public memo.type and memo.value accessors instead of the
internal memo._type and memo._value fields, while preserving the existing
text-memo 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: 18d7b7b7-1b0e-4fff-b305-02c64016d9e8
📒 Files selected for processing (10)
jest.config.jssrc/config/validateEnv.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/controllers/stellar/walletController.jssrc/models/Transaction.jssrc/services/stellar/stellarService.jstest/jest.setup.jstest/stellarService.validation.test.jstest/stellarWalletRoutes.test.js
| // During tests, relax strict env validation and provide sane defaults | ||
| if (process.env.NODE_ENV === "test") { | ||
| process.env.JWT_SECRET = process.env.JWT_SECRET || "test-secret"; | ||
| process.env.MONGO_URI = process.env.MONGO_URI || "mongodb://127.0.0.1:27017/deenbridge_test"; | ||
| process.env.PORT = process.env.PORT || "3000"; | ||
| logger.info("✅ Skipping strict env validation in test mode"); | ||
| return; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not embed test configuration defaults in source.
The fallback JWT secret and Mongo connection string mask missing test/CI configuration and violate the environment-only configuration rule. This shares the same root cause as test/jest.setup.js Line 3 and is consolidated below.
As per path instructions, “all configuration belongs in environment variables documented in .env.example.”
🤖 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 - 40, Remove the hardcoded
JWT_SECRET and MONGO_URI fallbacks from the test-mode branch in the environment
validation flow, including the assignments in validateEnv. Keep configuration
sourced exclusively from environment variables documented in .env.example, while
preserving only the test-mode behavior and any non-sensitive default that is
explicitly allowed.
Source: Path instructions
| // 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, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the advertised SEP-7 flow aligned with fee-split validation.
For a fee split, this requires creator and platform transfers, but initialization still returns a SEP-7 URI for one full-price transfer. Wallets following that URI will be rejected for a missing platform payment, and the pending transaction is marked failed. Generate an equivalent multi-operation request, or omit the SEP-7 link when a split applies; add a regression test.
As per path instructions, “New or changed endpoints need Jest coverage.”
🤖 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 280 - 297, Align
the SEP-7 initialization flow with the fee-split payments assembled in
expectedPayments: when transaction.platformFee is present, generate a
multi-operation request covering both creator and platform transfers, or omit
the SEP-7 URI. Preserve the single-transfer URI for transactions without a fee
split, and add Jest regression coverage for both behaviors.
Source: Path instructions
| return res.status(400).json({ | ||
| success: false, | ||
| message: "Signed transaction does not match expected payment details", | ||
| error: validationError.message, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use one validation-error envelope across Stellar submissions. Both new branches put error at the top level instead of the documented { success, message, data } shape.
src/controllers/stellar/paymentController.js#L317-L321: return validation detail asdata.error(ordata: null).src/controllers/stellar/donationController.js#L181-L185: apply the same envelope.
As per path instructions, “New or changed endpoints need Jest coverage” and require consistent response shapes ({ success, message, data }).
📍 Affects 2 files
src/controllers/stellar/paymentController.js#L317-L321(this comment)src/controllers/stellar/donationController.js#L181-L185
🤖 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 317 - 321, Update
the validation-error responses in paymentController.js at lines 317-321 and
donationController.js at lines 181-185 to use the documented { success, message,
data } envelope, placing validation details under data.error or using data:
null; add Jest coverage for both changed endpoint branches and preserve their
existing status and messages.
Source: Path instructions
| // Transaction hash from the Stellar network, only set after confirmation | ||
| stellarTxHash: { | ||
| type: String, | ||
| unique: true, | ||
| sparse: true, | ||
| index: true, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
: "${MONGO_URI:?Set MONGO_URI to inspect the deployed database}"
mongosh "$MONGO_URI" --quiet --eval '
db.getCollectionNames().forEach((name) => {
const indexes = db.getCollection(name).getIndexes()
.filter((index) => index.key && index.key.stellarTxHash === 1);
if (indexes.length) printjson({ collection: name, indexes });
});
'Repository: Deen-Bridge/dnb-backend
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Transaction model and any index-related migration/setup code.
git ls-files 'src/models/Transaction.js' 'src/**/*migration*' 'src/**/*index*' '.env.example' | sed -n '1,200p'
printf '\n--- Transaction.js outline ---\n'
ast-grep outline src/models/Transaction.js --view expanded
printf '\n--- relevant index/migration references ---\n'
rg -n "syncIndexes|createIndexes|ensureIndex|stellarTxHash|unique: true|sparse: true|autoIndex|index\(" src .env.exampleRepository: Deen-Bridge/dnb-backend
Length of output: 2880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- Transaction.js ---\n'
cat -n src/models/Transaction.js | sed -n '1,220p'
printf '\n--- startup / index syncing references ---\n'
rg -n "syncIndexes|ensureIndexes|ensureIndex|autoIndex|init\(\)|mongoose\.connect|set\('autoIndex'|dropIndexes|collection\.createIndex" srcRepository: Deen-Bridge/dnb-backend
Length of output: 5537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/config/db.js
printf '\n---\n'
rg -n "syncIndexes|ensureIndexes|dropIndex|dropIndexes|createIndex|autoIndex" srcRepository: Deen-Bridge/dnb-backend
Length of output: 3686
Rebuild the stellarTxHash index before shipping.
unique + sparse in the schema won’t change an existing collection, so any deployed non-sparse unique index will still reject multiple pending rows without a stellarTxHash. Add a migration to drop and recreate this index. src/models/Transaction.js:43-49
🤖 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/models/Transaction.js` around lines 43 - 49, add a migration that locates
the existing stellarTxHash index on the Transaction collection, drops it, and
recreates it as a unique sparse index matching the schema definition. Ensure the
migration is safe to run against deployments where the index is absent or
already correctly configured, and leave the stellarTxHash schema unchanged.
Source: Path instructions
| @@ -0,0 +1,8 @@ | |||
| import { MongoMemoryServer } from 'mongodb-memory-server'; | |||
|
|
|||
| process.env.JWT_SECRET = process.env.JWT_SECRET || 'test-secret'; | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Move test configuration out of JavaScript. Both sites silently inject configuration rather than requiring the documented test environment, allowing CI to run with an undeclared JWT secret or database target.
test/jest.setup.js#L3-L3: remove the inline JWT fallback and requireJWT_SECRETfrom the test environment.src/config/validateEnv.js#L34-L40: remove the inline JWT/Mongo defaults; validate the documented test environment instead of replacing it.
As per path instructions, “all configuration belongs in environment variables documented in .env.example.”
📍 Affects 2 files
test/jest.setup.js#L3-L3(this comment)src/config/validateEnv.js#L34-L40
🤖 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 `@test/jest.setup.js` at line 3, Remove the JWT_SECRET fallback from
test/jest.setup.js so tests require the documented environment variable. In
src/config/validateEnv.js, remove the inline JWT and Mongo defaults and validate
the documented test environment values directly, keeping all configuration
sourced from variables defined in .env.example.
Source: Path instructions
| const mongod = await MongoMemoryServer.create(); | ||
| process.env.MONGO_URI = mongod.getUri(); | ||
| global.__MONGOD__ = mongod; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'MongoMemoryServer|__MONGOD__|globalTeardown|setupFilesAfterEnv|\.stop\(' \
jest.config.js test srcRepository: Deen-Bridge/dnb-backend
Length of output: 614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files\n'
git ls-files | rg '(^|/)(jest\.config\..*|package\.json|test/|src/)' | sed -n '1,200p'
printf '\n## teardown-related search\n'
rg -n -C3 'MongoMemoryServer|__MONGOD__|globalTeardown|setupFilesAfterEnv|setupFiles|teardown|\.stop\(' .
printf '\n## jest setup file\n'
cat -n test/jest.setup.js
printf '\n## jest config candidates\n'
for f in $(git ls-files | rg '(^|/)jest\.config\..*$|(^|/)package\.json$'); do
echo "--- $f"
cat -n "$f" | sed -n '1,220p'
doneRepository: Deen-Bridge/dnb-backend
Length of output: 5716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## mongoose connection handling in tests\n'
rg -n -C2 'mongoose\.connect|mongoose\.disconnect|disconnect\(|close\(\)|__MONGOD__|MongoMemoryServer' test src
printf '\n## test bootstrap files\n'
for f in test/*.js; do
echo "--- $f"
cat -n "$f" | sed -n '1,240p'
doneRepository: Deen-Bridge/dnb-backend
Length of output: 44250
Move the MongoMemoryServer lifecycle to teardown.
test/jest.setup.js starts MongoMemoryServer.create() from setupFiles, so each test file spins up a new MongoDB instance and none of them are stopped. That leaves child processes alive until Jest force-exits and adds avoidable CI overhead. Move this to globalSetup/globalTeardown, or stop global.__MONGOD__ in teardown.
🤖 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 `@test/jest.setup.js` around lines 6 - 8, Move the MongoMemoryServer startup
currently in the setup initialization to a Jest global setup/teardown lifecycle,
ensuring one shared instance is created for the test run and explicitly stopped
afterward. Update the MONGO_URI propagation and shared instance handling so
tests continue using the started server without leaving child processes running.
| const tx = StellarSdk.TransactionBuilder.fromXDR(payment.xdr, networkPassphrase); | ||
| tx.sign(sourceKeypair); | ||
| const signedXdr = tx.toXDR(); | ||
|
|
||
| // Expect no throw | ||
| expect(() => | ||
| validateSignedPaymentXdr( | ||
| signedXdr, | ||
| [{ destination: destKeypair.publicKey(), amount: "5" }], | ||
| memo, | ||
| sourceKeypair.publicKey(), | ||
| true | ||
| ) | ||
| ).not.toThrow(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C5 'validateSignedPaymentXdr|verifyTxSignedBy|tx\.signatures|Keypair\.verify' \
src/services/stellar testRepository: Deen-Bridge/dnb-backend
Length of output: 3316
Add signature verification before accepting the signed XDR. validateSignedPaymentXdr checks the source, memo, and payment ops, but it doesn’t prove the claimed source actually authorized the transaction. Add a signature check and cover unsigned and wrong-signer cases in the test.
🤖 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 `@test/stellarService.validation.test.js` around lines 34 - 47, Update
validateSignedPaymentXdr to verify that the signed XDR contains a valid
signature from the claimed source public key before accepting it, while
preserving the existing source, memo, and payment-operation validation. Extend
the validation tests to assert unsigned transactions and transactions signed by
a different key are rejected, while the correctly signed transaction remains
accepted.
Source: Path instructions
1 participant
Closes #10
Summary by CodeRabbit
Security
Bug Fixes
hasWallet).Tests