feat: add SEP-1 stellar.toml discovery - #72
Conversation
- Apply cacheMiddleware to GET endpoints for books, courses, users, spaces, and search routes with appropriate TTLs - Add cache invalidation middleware to POST, PUT, DELETE operations to ensure cache consistency when data changes - Add Redis environment variables to validateEnv.js optional vars - Create .env.example documenting all environment variables including Redis configuration options Closes Deen-Bridge#2
feat: configure Redis cache layer for production use
…ests test: add Stellar payment flow coverage
…nt-horizon-client feat: resilient horizon client layer
WalkthroughAdds SEP-1 ChangesSEP-1 discovery metadata
Horizon resilience and payment flow
API route caching
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant getStellarToml
participant stellarTomlService
Client->>ExpressApp: GET /.well-known/stellar.toml
ExpressApp->>getStellarToml: dispatch request
getStellarToml->>stellarTomlService: buildStellarToml()
stellarTomlService-->>getStellarToml: generated TOML
getStellarToml-->>Client: 200 text/toml with CORS
Possibly related issues
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
🧹 Nitpick comments (1)
test/stellarToml.test.js (1)
52-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the actual endpoint for missing-configuration behavior.
This test calls
buildStellarToml(createStellarTomlConfig({}))directly, so it does not verify thatGET /.well-known/stellar.tomlomits optional fields whenprocess.envis unset. Temporarily clear the optional variables, request the endpoint, parse the response, and restore the environment infinally.🤖 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/stellarToml.test.js` around lines 52 - 60, Update the test around the “omits unset optional metadata” case to exercise the actual GET /.well-known/stellar.toml endpoint instead of calling buildStellarToml directly. Temporarily clear the optional process.env variables, parse the endpoint response, retain the existing omission and currency assertions, and restore the environment in a finally block.
🤖 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/services/stellar/stellarTomlService.js`:
- Around line 51-57: Update the stellar TOML service configuration to read
ORG_NAME, ORG_DESCRIPTION, ORG_GITHUB, and the Telegram URL from environment
variables instead of hardcoded values, while retaining only invariant
protocol/network defaults in JavaScript. Add and document each new environment
variable in .env.example, and preserve the existing metadata field mappings.
- Around line 45-50: Update createStellarTomlConfig() to validate
STELLAR_PLATFORM_PUBLIC_KEY and SIGNING_KEY as Stellar public keys before
including them in the serialized TOML; omit or reject invalid, secret-seed, or
malformed values rather than publishing them. Add a regression test covering
invalid key input and confirming it is not serialized.
In `@test/stellarToml.test.js`:
- Around line 19-26: Preserve the original STELLAR_PLATFORM_PUBLIC_KEY value in
the suite setup before assigning PLATFORM_ACCOUNT, then restore that saved value
in afterAll instead of deleting the variable unconditionally. Keep the existing
app import and ensure restoration distinguishes an originally unset variable
from one with a defined value.
---
Nitpick comments:
In `@test/stellarToml.test.js`:
- Around line 52-60: Update the test around the “omits unset optional metadata”
case to exercise the actual GET /.well-known/stellar.toml endpoint instead of
calling buildStellarToml directly. Temporarily clear the optional process.env
variables, parse the endpoint response, retain the existing omission and
currency assertions, and restore the environment in a finally block.
🪄 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: 5cb5a319-273d-4c16-8ca8-929473762bca
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.env.exampleQUICK_START.mdapp.jspackage.jsonsrc/config/validateEnv.jssrc/routes/wellKnownRoutes.jssrc/services/stellar/stellarTomlService.jstest/stellarToml.test.js
|
Fixed all three: public and signing keys are now validated before TOML serialization, organization and Telegram metadata come from documented env vars, and the test restores its original env values. Added coverage for secret-seed and malformed-key rejection. |
|
@vreabernardo please fix conflicts |
|
@zeemscript done — merged upstream/main in and resolved the overlaps in |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middlewares/errorHandler.js (1)
82-104: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win503 payload breaks the standard response shape and is duplicated across both branches. Mapping
AllEndpointsOpenErrorto a 503 is the right call — but the body here is{ error, code }, whereas every other response in this handler (and the API convention) uses{ success: false, status, message, ... }. A consumer checkingres.body.successwill readundefinedand misclassify the outcome, anderrorhere is a human-readable string rather than the usualmessagefield. The identical block is also copy-pasted into both the dev and prod branches, and the dev path silently skips thelogger.errorthe other paths emit. Consolidating into one consistent response fixes all three.As per path instructions: "consistent response shapes ({ success, message, data })".
♻️ Suggested consolidation
export const errorHandler = (err, req, res, next) => { err.statusCode = err.statusCode || 500; err.status = err.status || "error"; + if (err.name === "AllEndpointsOpenError") { + logger.error({ message: err.message, reqId: req?.id }, "Stellar network unavailable"); + return res.status(503).json({ + success: false, + status: "error", + message: "Stellar network currently unreachable. Please try again later.", + code: "NETWORK_UNAVAILABLE", + reqId: req?.id, + }); + } + if (process.env.NODE_ENV === "development") { - if (err.name === "AllEndpointsOpenError") { - return res.status(503).json({ - error: "Stellar network currently unreachable. Please try again later.", - code: "NETWORK_UNAVAILABLE" - }); - } sendErrorDev(err, req, res); } else { let error = { ...err }; error.message = err.message; if (err.name === "CastError") error = handleCastErrorDB(err); if (err.code === 11000) error = handleDuplicateFieldsDB(err); if (err.name === "ValidationError") error = handleValidationErrorDB(err); if (err.name === "JsonWebTokenError") error = handleJWTError(); if (err.name === "TokenExpiredError") error = handleJWTExpiredError(); - - if (err.name === "AllEndpointsOpenError") { - return res.status(503).json({ - error: "Stellar network currently unreachable. Please try again later.", - code: "NETWORK_UNAVAILABLE" - }); - } sendErrorProd(error, req, res); } };🤖 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/middlewares/errorHandler.js` around lines 82 - 104, Consolidate the AllEndpointsOpenError handling in the error-handler flow so both development and production branches use one response path. Return the standard failure payload with success: false, status 503, and the network-unavailable text in message, while preserving the NETWORK_UNAVAILABLE code as appropriate. Ensure this centralized path performs the same logger.error behavior as other errors and remove both duplicated response blocks.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/config/validateEnv.js`:
- Around line 60-65: Update the STELLAR_NETWORK validation in validateEnv so
only “testnet” and “mainnet” are accepted; reject any other configured value
before deriving HORIZON_URLS. Preserve the existing endpoint selection for valid
values and ensure the required configuration error identifies the invalid
network.
- Around line 64-65: Update the Horizon endpoint selection in validateEnv so it
no longer hardcodes Stellar URLs in JavaScript. Read the endpoint values from
the documented HORIZON_URLS environment/configuration setting, or require that
setting during startup, while preserving the existing network-specific selection
behavior.
- Around line 67-70: Validate the Horizon tuning environment variables in the
configuration validation flow before assigning or consuming them:
HORIZON_TIMEOUT_MS, HORIZON_MAX_RETRIES, HORIZON_CB_THRESHOLD, and
HORIZON_CB_COOLDOWN_MS must be finite integers within appropriate non-negative
ranges. Reject malformed or out-of-range values with a clear startup
configuration error instead of allowing parseInt() to produce NaN or unsafe
settings.
- Around line 59-70: Ensure validateEnv() runs before app.js loads any
src/routes/stellar modules, preventing horizonClient’s exported singleton from
reading unset environment variables during module evaluation. Reorder
initialization so the defaults for HORIZON_URLS and Horizon settings are applied
before Stellar imports, or convert the client initialization to lazy/injected
creation while preserving existing configuration behavior.
In `@src/config/validateEnv.test.js`:
- Around line 11-12: Update the test setup around the MONGO_URI and JWT_SECRET
fixtures in validateEnv tests to read these values from documented environment
configuration or Jest setup instead of embedding literals in the JavaScript.
Ensure the validation tests continue using the configured credentials and
connection string, with suitable test-environment defaults handled outside the
test source.
In `@src/routes/books/bookRoutes.js`:
- Around line 35-44: Update the invalidateCacheMiddleware calls for book
creation at src/routes/books/bookRoutes.js:35-44, book deletion at
src/routes/books/bookRoutes.js:77-82, course creation at
src/routes/courses/courseRoutes.js:60-65, and course updates at
src/routes/courses/courseRoutes.js:78-83 to also clear the
`${CACHE_KEYS.USER}*:recommendations` namespace, while preserving each route’s
existing content-cache invalidation.
- Around line 78-82: Update the DELETE "/:id" route registration to include the
existing protect middleware before invalidateCacheMiddleware and deleteBook,
ensuring unauthenticated requests are rejected before cache invalidation or
deletion.
In `@src/routes/searchRoutes.js`:
- Around line 9-16: Update searchCacheKey to derive the query exclusively from
req.query.q, matching searchAll’s accepted parameter and preventing invalid
query requests from sharing valid-result cache keys. Also update cacheMiddleware
to cache and serve only successful responses, preserving status and error
behavior for unsuccessful requests.
In `@src/routes/spaceRoutes.js`:
- Around line 67-79: Update the updateSpace and deleteSpace controllers to load
the space identified by req.params.id and verify its host matches req.user._id
before performing any mutation. Reject unauthorized users consistently with the
existing authorization behavior, and preserve the current update/delete flows
for the owning host.
In `@src/routes/userRoutes.js`:
- Around line 46-67: Restrict the PUT "/update/:id" and DELETE "/:id" routes to
the authenticated account owner or an explicit admin role before invoking
updateUser or deleteUser. Add or reuse an authorization middleware that compares
req.user._id with the route id and permits the configured admin role, while
leaving the existing protection, upload, and cache middleware behavior intact.
In `@src/services/stellar/horizonClient.js`:
- Around line 157-184: Ensure execute always throws when its retry loop is
exhausted instead of implicitly returning undefined, particularly after the
submit-mode timeout verification path increments attempt and continues. Add a
defensive network-error throw immediately after the loop in execute, preserving
successful returns and existing error propagation.
- Around line 132-148: Update the request flow around fn(endpoint.server) and
the Promise.race timeout so the AbortController signal is propagated to the
underlying Horizon transport whenever cancellation is supported. Adapt the SDK
invocation or transport configuration used by fn to accept the signal, while
preserving the TimeoutError behavior; if the transport cannot consume signals,
avoid implying that abort cancels the request and retain only the documented
wait-timeout semantics.
In `@src/services/stellar/stellarService.js`:
- Around line 527-538: Update the verifyFn used by timedHorizonCall around
verifyTransaction so any on-chain transaction, including one with successful set
to false, is returned as the terminal result and is not resubmitted. Preserve
the existing null return only when ver.exists is false, while retaining the
transaction hash and ledger details.
---
Outside diff comments:
In `@src/middlewares/errorHandler.js`:
- Around line 82-104: Consolidate the AllEndpointsOpenError handling in the
error-handler flow so both development and production branches use one response
path. Return the standard failure payload with success: false, status 503, and
the network-unavailable text in message, while preserving the
NETWORK_UNAVAILABLE code as appropriate. Ensure this centralized path performs
the same logger.error behavior as other errors and remove both duplicated
response blocks.
🪄 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: d60d628f-194f-4420-aafc-8c13bd799e0f
📒 Files selected for processing (14)
.env.examplesrc/config/validateEnv.jssrc/config/validateEnv.test.jssrc/middlewares/errorHandler.jssrc/routes/books/bookRoutes.jssrc/routes/courses/courseRoutes.jssrc/routes/searchRoutes.jssrc/routes/spaceRoutes.jssrc/routes/userRoutes.jssrc/services/stellar/horizonClient.jssrc/services/stellar/horizonClient.test.jssrc/services/stellar/stellarService.jstest/stellarPaymentController.test.jstest/stellarService.test.js
| // Default values for Horizon resilient client if not provided | ||
| const network = process.env.STELLAR_NETWORK || "testnet"; | ||
| if (!process.env.HORIZON_URLS) { | ||
| process.env.HORIZON_URLS = | ||
| network === "mainnet" | ||
| ? "https://horizon.stellar.org" | ||
| : "https://horizon-testnet.stellar.org"; | ||
| } | ||
| process.env.HORIZON_TIMEOUT_MS = process.env.HORIZON_TIMEOUT_MS || "10000"; | ||
| process.env.HORIZON_MAX_RETRIES = process.env.HORIZON_MAX_RETRIES || "3"; | ||
| process.env.HORIZON_CB_THRESHOLD = process.env.HORIZON_CB_THRESHOLD || "5"; | ||
| process.env.HORIZON_CB_COOLDOWN_MS = process.env.HORIZON_CB_COOLDOWN_MS || "30000"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'src/config/validateEnv.js' 'src/**/horizonClient.js' 'src/**/stellar*.js' 'src/**/stellar*.mjs' 'src/**/stellar*.ts' 'src/**/stellar*.cjs' | sed 's#^`#-` #'
printf '\n== validateEnv.js outline ==\n'
ast-grep outline src/config/validateEnv.js --view expanded || true
printf '\n== horizonClient candidates ==\n'
fd -a 'horizonClient\.(js|mjs|cjs|ts)$' src || true
printf '\n== search for imports/usages ==\n'
rg -n --hidden --glob 'src/**/*.js' 'horizonClient|validateEnv\(' src || true
printf '\n== src/config/validateEnv.js (selected lines) ==\n'
nl -ba src/config/validateEnv.js | sed -n '1,180p'Repository: Deen-Bridge/dnb-backend
Length of output: 1509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/config/validateEnv.js ==\n'
sed -n '1,220p' src/config/validateEnv.js
printf '\n== src/services/stellar/horizonClient.js ==\n'
sed -n '1,240p' src/services/stellar/horizonClient.js
printf '\n== app/bootstrap imports ==\n'
rg -n --hidden --glob 'src/**/*.js' 'validateEnv\(\)|from "./config/validateEnv\.js"|from ".*/horizonClient\.js"|from "./services/stellar/stellarService\.js"|from "./services/stellar/horizonClient\.js"' srcRepository: Deen-Bridge/dnb-backend
Length of output: 10279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== root js files ==\n'
git ls-files '*.js' | sed -n '1,200p'
printf '\n== startup-related imports ==\n'
rg -n --hidden --glob 'src/**/*.js' --glob '*.js' \
'validateEnv\(\)|from "\./config/validateEnv\.js"|from "\./src/config/validateEnv\.js"|from ".*/stellarService\.js"|from ".*/horizonClient\.js"|from "\./services/stellar/stellarService\.js"|from "\./services/stellar/horizonClient\.js"' \
. || true
printf '\n== likely entrypoints ==\n'
fd -a '^(app|server|index|main)\.js$' . || true
printf '\n== package scripts ==\n'
sed -n '1,220p' package.json 2>/dev/null || trueRepository: Deen-Bridge/dnb-backend
Length of output: 6450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== app.js ==\n'
sed -n '1,220p' app.js
printf '\n== server.js ==\n'
sed -n '1,220p' server.js
printf '\n== direct stellar imports from app boot path ==\n'
rg -n --hidden --glob 'app.js' --glob 'server.js' --glob 'src/**/*.js' \
'stellarService\.js|horizonClient\.js|validateEnv\.js|routes/stellar|controllers/stellar' \
app.js server.js src || trueRepository: Deen-Bridge/dnb-backend
Length of output: 9633
Move env defaults before the Stellar client is instantiated
app.js pulls in src/routes/stellar/* before validateEnv() runs, and that import chain loads src/services/stellar/horizonClient.js, which creates the exported singleton from process.env during module evaluation. The Horizon client will keep the pre-default URL/timeout values, so mainnet routing and retry settings can be ignored. Call validateEnv() before any Stellar imports, or make the client lazy/injected.
🤖 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 59 - 70, Ensure validateEnv() runs
before app.js loads any src/routes/stellar modules, preventing horizonClient’s
exported singleton from reading unset environment variables during module
evaluation. Reorder initialization so the defaults for HORIZON_URLS and Horizon
settings are applied before Stellar imports, or convert the client
initialization to lazy/injected creation while preserving existing configuration
behavior.
| const network = process.env.STELLAR_NETWORK || "testnet"; | ||
| if (!process.env.HORIZON_URLS) { | ||
| process.env.HORIZON_URLS = | ||
| network === "mainnet" | ||
| ? "https://horizon.stellar.org" | ||
| : "https://horizon-testnet.stellar.org"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unsupported STELLAR_NETWORK values.
The current ternary treats every value other than exactly mainnet—including typos—as testnet. Since this setting controls the Horizon endpoint and Stellar discovery network, fail validation for values other than testnet or mainnet instead of silently selecting the wrong network.
As per path instructions, STELLAR_NETWORK must be configured as testnet or mainnet.
🤖 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 60 - 65, Update the STELLAR_NETWORK
validation in validateEnv so only “testnet” and “mainnet” are accepted; reject
any other configured value before deriving HORIZON_URLS. Preserve the existing
endpoint selection for valid values and ensure the required configuration error
identifies the invalid network.
Source: Path instructions
| ? "https://horizon.stellar.org" | ||
| : "https://horizon-testnet.stellar.org"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep Horizon connection strings in environment-backed configuration.
These URLs are hardcoded in JavaScript even though HORIZON_URLS is already documented in .env.example. Move endpoint defaults into the environment/configuration layer or require HORIZON_URLS during startup.
As per path instructions: “Flag hardcoded secrets, connection strings, JWT secrets, or wallet keys; 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 64 - 65, Update the Horizon endpoint
selection in validateEnv so it no longer hardcodes Stellar URLs in JavaScript.
Read the endpoint values from the documented HORIZON_URLS
environment/configuration setting, or require that setting during startup, while
preserving the existing network-specific selection behavior.
Source: Path instructions
| process.env.HORIZON_TIMEOUT_MS = process.env.HORIZON_TIMEOUT_MS || "10000"; | ||
| process.env.HORIZON_MAX_RETRIES = process.env.HORIZON_MAX_RETRIES || "3"; | ||
| process.env.HORIZON_CB_THRESHOLD = process.env.HORIZON_CB_THRESHOLD || "5"; | ||
| process.env.HORIZON_CB_COOLDOWN_MS = process.env.HORIZON_CB_COOLDOWN_MS || "30000"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate Horizon tuning values before passing them through.
Malformed values survive these defaults and are later parsed with parseInt(). NaN can cause the retry loop to skip entirely, disable circuit-breaker thresholds, or make timeouts fire immediately. Validate finite integer ranges and fail startup with a clear configuration error.
🤖 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 67 - 70, Validate the Horizon tuning
environment variables in the configuration validation flow before assigning or
consuming them: HORIZON_TIMEOUT_MS, HORIZON_MAX_RETRIES, HORIZON_CB_THRESHOLD,
and HORIZON_CB_COOLDOWN_MS must be finite integers within appropriate
non-negative ranges. Reject malformed or out-of-range values with a clear
startup configuration error instead of allowing parseInt() to produce NaN or
unsafe settings.
| MONGO_URI: "mongodb://localhost:27017/test", | ||
| JWT_SECRET: "test-secret-key-for-ci-minimum-32-chars", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Move test credentials and connection strings out of JavaScript.
These lines embed MONGO_URI and JWT_SECRET literals. Even though they are test values, this path requires connection strings and JWT secrets to come from documented environment configuration; load them through Jest setup or the test environment instead.
🤖 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.test.js` around lines 11 - 12, Update the test setup
around the MONGO_URI and JWT_SECRET fixtures in validateEnv tests to read these
values from documented environment configuration or Jest setup instead of
embedding literals in the JavaScript. Ensure the validation tests continue using
the configured credentials and connection string, with suitable test-environment
defaults handled outside the test source.
Source: Path instructions
| router.put( | ||
| "/update/:id", | ||
| protect, | ||
| invalidateCacheMiddleware([`${CACHE_KEYS.SPACES}*`, `${CACHE_KEYS.SPACE}*`]), | ||
| updateSpace | ||
| ); | ||
|
|
||
| // Delete a space - invalidates space caches | ||
| router.delete( | ||
| "/:id", | ||
| protect, | ||
| invalidateCacheMiddleware([`${CACHE_KEYS.SPACES}*`, `${CACHE_KEYS.SPACE}*`]), | ||
| deleteSpace |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce host ownership for space updates and deletes.
protect only authenticates the caller. Both controllers mutate by req.params.id without checking space.host === req.user._id, so any authenticated user can update or delete another host’s space. Enforce that comparison in updateSpace and deleteSpace before writing.
🤖 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/routes/spaceRoutes.js` around lines 67 - 79, Update the updateSpace and
deleteSpace controllers to load the space identified by req.params.id and verify
its host matches req.user._id before performing any mutation. Reject
unauthorized users consistently with the existing authorization behavior, and
preserve the current update/delete flows for the owning host.
Source: Path instructions
| router.put( | ||
| "/update/:id", | ||
| protect, | ||
| upload.single("avatar"), | ||
| invalidateCacheMiddleware([`${CACHE_KEYS.USER}*`]), | ||
| updateUser | ||
| ); | ||
|
|
||
| // Get personalized recommendations | ||
| router.get("/recommendations", protect, getRecommendations); | ||
| // Get user by ID - cached for 10 minutes | ||
| router.get( | ||
| "/:id", | ||
| protect, | ||
| cacheMiddleware(CACHE_TTL.USERS, userCacheKey), | ||
| getUser | ||
| ); | ||
|
|
||
| // Get user statistics | ||
| router.get("/:id/stats", protect, getUserStats); | ||
| // Delete user - invalidates user cache | ||
| router.delete( | ||
| "/:id", | ||
| protect, | ||
| invalidateCacheMiddleware([`${CACHE_KEYS.USER}*`]), | ||
| deleteUser |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict profile updates and deletion to the account owner (or an explicit admin role).
Both handlers only use protect; their controllers update/delete the supplied :id without comparing it to req.user._id. An authenticated user can therefore alter or delete another account. Enforce ownership/role authorization before invoking these controllers.
🤖 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/routes/userRoutes.js` around lines 46 - 67, Restrict the PUT
"/update/:id" and DELETE "/:id" routes to the authenticated account owner or an
explicit admin role before invoking updateUser or deleteUser. Add or reuse an
authorization middleware that compares req.user._id with the route id and
permits the configured admin role, while leaving the existing protection,
upload, and cache middleware behavior intact.
Source: Path instructions
| const abortController = new AbortController(); | ||
| const timeoutId = setTimeout(() => { | ||
| abortController.abort(); | ||
| }, this.timeoutMs); | ||
|
|
||
| try { | ||
| const callPromise = fn(endpoint.server); | ||
|
|
||
| const timeoutPromise = new Promise((_, reject) => { | ||
| abortController.signal.addEventListener('abort', () => { | ||
| const err = new Error("Horizon request timed out"); | ||
| err.name = "TimeoutError"; | ||
| reject(err); | ||
| }); | ||
| }); | ||
|
|
||
| const result = await Promise.race([callPromise, timeoutPromise]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @stellar/stellar-sdk 16 Horizon Server loadAccount/submitTransaction accept an AbortSignal to cancel requests?
💡 Result:
In @stellar/stellar-sdk version 16, the Horizon Server methods loadAccount and submitTransaction do not directly accept an AbortSignal as an argument in their method signatures [1][2][3]. While the SDK has undergone updates to its internal HTTP client—including improvements to fetch-based implementation, cancellation support, and a unified HTTP client contract—these changes are handled internally by the SDK's HttpClient [4][5]. The public-facing methods loadAccount(accountId: string) and submitTransaction(transaction: Transaction | FeeBumpTransaction, opts?: SubmitTransactionOptions) expose only specific, documented options (such as skipMemoRequiredCheck for transactions) [1][2][6]. If you need to cancel requests, you cannot pass an AbortSignal directly through these high-level Horizon Server methods. Cancellation logic is managed internally by the SDK's underlying transport layer [4][5]. Developers typically handle timeouts or cancellation by relying on the SDK's internal configuration (such as the default SUBMIT_TRANSACTION_TIMEOUT) or by utilizing the underlying httpClient if direct control over the request lifecycle is required, though this is often an advanced or non-standard usage pattern [1][6][3][4].
Citations:
- 1: https://stellar.github.io/js-stellar-sdk/reference/network-horizon/
- 2: https://github.com/stellar/js-stellar-sdk/blob/main/src/horizon/server.ts
- 3: https://github.com/stellar/js-stellar-sdk/blob/2f52d0e7/src/horizon/server.ts
- 4: Fix fetch implementation stellar/js-stellar-sdk#1390
- 5: stellar/js-stellar-sdk@7ad2b0a
- 6: https://stellar.github.io/js-stellar-sdk/libdocs_server.js.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== horizonClient.js (relevant lines) ==\n'
nl -ba src/services/stellar/horizonClient.js | sed -n '1,240p'
printf '\n== references to fn(endpoint.server) and timeout handling ==\n'
rg -n "fn\\(endpoint\\.server\\)|AbortController|timeoutPromise|submitTransaction|loadAccount|withTimeout|timeoutMs" src/services/stellar -S
printf '\n== sdk package references ==\n'
rg -n "`@stellar/stellar-sdk`|stellar-sdk" package.json package-lock.json yarn.lock pnpm-lock.yaml -S || trueRepository: Deen-Bridge/dnb-backend
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('src/services/stellar/horizonClient.js')
text = path.read_text().splitlines()
for i, line in enumerate(text[:260], 1):
if 120 <= i <= 170:
print(f"{i:4d}: {line}")
PY
printf '\n== references in stellar service code ==\n'
rg -n "fn\\(endpoint\\.server\\)|AbortController|timeoutPromise|signal|submitTransaction|loadAccount|verifyFn|timeoutMs" src/services/stellar -S
printf '\n== stellar sdk version references ==\n'
rg -n "\"`@stellar/stellar-sdk`\"|stellar-sdk" package.json package-lock.json yarn.lock pnpm-lock.yaml -S || trueRepository: Deen-Bridge/dnb-backend
Length of output: 6031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path_str in ['src/services/stellar/stellarService.js', 'src/services/stellar/horizonClient.test.js']:
path = Path(path_str)
print(f"\n== {path_str} ==")
text = path.read_text().splitlines()
# print only relevant ranges around mentioned call sites/tests
ranges = {
'src/services/stellar/stellarService.js': [(510, 545)],
'src/services/stellar/horizonClient.test.js': [(230, 310)],
}[path_str]
for start, end in ranges:
for i in range(start, min(end, len(text)) + 1):
print(f"{i:4d}: {text[i-1]}")
PYRepository: Deen-Bridge/dnb-backend
Length of output: 4158
Cancel the Horizon call, not just the wrapper timeout
AbortController only rejects Promise.race here; it never reaches fn(endpoint.server), and the SDK’s loadAccount / submitTransaction methods don’t accept an AbortSignal directly. That means the Horizon HTTP request keeps running in the background after timeout, which can leave unnecessary in-flight work and sockets under sustained slowness. If the transport supports cancellation, thread it through; otherwise this is only a wait timeout.
🤖 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/horizonClient.js` around lines 132 - 148, Update the
request flow around fn(endpoint.server) and the Promise.race timeout so the
AbortController signal is propagated to the underlying Horizon transport
whenever cancellation is supported. Adapt the SDK invocation or transport
configuration used by fn to accept the signal, while preserving the TimeoutError
behavior; if the transport cannot consume signals, avoid implying that abort
cancels the request and retain only the documented wait-timeout semantics.
| if (opts.mode === 'submit') { | ||
| if (error.name === 'TimeoutError' && opts.verifyFn && attempt === 0) { | ||
| const landedResult = await opts.verifyFn(); | ||
| if (landedResult && landedResult.successful) { | ||
| return landedResult; | ||
| } | ||
| attempt++; | ||
| continue; // resubmit at most once | ||
| } | ||
| throw error; // bypass generic blind retry entirely | ||
| } | ||
|
|
||
| const classification = this.classifyError(error, attempt); | ||
|
|
||
| if (classification.retriable) { | ||
| this.recordFailure(endpoint); | ||
| } | ||
|
|
||
| if (!classification.retriable || attempt === this.maxRetries) { | ||
| throw error; | ||
| } | ||
|
|
||
| // Wait for the computed delay before retrying | ||
| await new Promise(resolve => setTimeout(resolve, classification.delayMs)); | ||
| attempt++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
execute can resolve to undefined on the submit-mode resubmit path. Thanks for the thoughtful submission-safety design here — the verify-then-resubmit-once logic is exactly right for avoiding double charges. One gap though: when the timeout resubmit branch runs attempt++; continue, the loop re-checks attempt <= this.maxRetries. If HORIZON_MAX_RETRIES is configured to 0, the incremented attempt (1) fails that condition, the loop exits, and execute returns undefined. Downstream, submitTransaction immediately does result.hash, which throws a TypeError and surfaces as an opaque 500 instead of a clean network error. A small guard after the loop makes the function total and future-proofs it against config changes.
🛡️ Proposed defensive guard
await new Promise(resolve => setTimeout(resolve, classification.delayMs));
attempt++;
}
}
+
+ // Loop can only exit here if a resubmit was scheduled but no retry budget
+ // remained; fail loudly rather than resolving undefined.
+ const err = new Error("Horizon request exhausted without a result");
+ err.name = "TimeoutError";
+ throw err;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (opts.mode === 'submit') { | |
| if (error.name === 'TimeoutError' && opts.verifyFn && attempt === 0) { | |
| const landedResult = await opts.verifyFn(); | |
| if (landedResult && landedResult.successful) { | |
| return landedResult; | |
| } | |
| attempt++; | |
| continue; // resubmit at most once | |
| } | |
| throw error; // bypass generic blind retry entirely | |
| } | |
| const classification = this.classifyError(error, attempt); | |
| if (classification.retriable) { | |
| this.recordFailure(endpoint); | |
| } | |
| if (!classification.retriable || attempt === this.maxRetries) { | |
| throw error; | |
| } | |
| // Wait for the computed delay before retrying | |
| await new Promise(resolve => setTimeout(resolve, classification.delayMs)); | |
| attempt++; | |
| } | |
| } | |
| } | |
| if (opts.mode === 'submit') { | |
| if (error.name === 'TimeoutError' && opts.verifyFn && attempt === 0) { | |
| const landedResult = await opts.verifyFn(); | |
| if (landedResult && landedResult.successful) { | |
| return landedResult; | |
| } | |
| attempt++; | |
| continue; // resubmit at most once | |
| } | |
| throw error; // bypass generic blind retry entirely | |
| } | |
| const classification = this.classifyError(error, attempt); | |
| if (classification.retriable) { | |
| this.recordFailure(endpoint); | |
| } | |
| if (!classification.retriable || attempt === this.maxRetries) { | |
| throw error; | |
| } | |
| // Wait for the computed delay before retrying | |
| await new Promise(resolve => setTimeout(resolve, classification.delayMs)); | |
| attempt++; | |
| } | |
| } | |
| // Loop can only exit here if a resubmit was scheduled but no retry budget | |
| // remained; fail loudly rather than resolving undefined. | |
| const err = new Error("Horizon request exhausted without a result"); | |
| err.name = "TimeoutError"; | |
| throw err; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 179-179: Avoid using the initial state variable in setState
Context: setTimeout(resolve, classification.delayMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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/horizonClient.js` around lines 157 - 184, Ensure execute
always throws when its retry loop is exhausted instead of implicitly returning
undefined, particularly after the submit-mode timeout verification path
increments attempt and continues. Add a defensive network-error throw
immediately after the loop in execute, preserving successful returns and
existing error propagation.
| // Using mode: 'submit' and passing a verifyFn to safely handle timeouts | ||
| const verifyFn = async () => { | ||
| const ver = await verifyTransaction(transaction.hash().toString("hex")); | ||
| if (ver.exists) { | ||
| return { hash: transaction.hash().toString("hex"), ledger: ver.ledger, successful: ver.successful }; | ||
| } | ||
| return null; | ||
| }; | ||
|
|
||
| const result = await timedHorizonCall("submitTransaction", () => | ||
| server.submitTransaction(transaction) | ||
| client.execute(server => server.submitTransaction(transaction), { mode: 'submit', verifyFn }) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Treat landed-but-failed transactions as terminal
If verifyTransaction() finds the transaction on-chain but successful is false, stop there instead of resubmitting the same signed XDR. Otherwise the caller gets a misleading tx_bad_seq retry instead of the actual on-chain failure reason.
🤖 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 527 - 538, Update the
verifyFn used by timedHorizonCall around verifyTransaction so any on-chain
transaction, including one with successful set to false, is returned as the
terminal result and is not resubmitted. Preserve the existing null return only
when ver.exists is false, while retaining the transaction hash and ledger
details.
|
Withdrawing this contribution. Thanks for your time. |
5 participants
Closes #58
Adds the public
/.well-known/stellar.tomlendpoint before the restricted CORS and API rate-limit middleware. The document uses the existing Stellar network constants for its passphrase and USDC issuer, and leaves unset SEP-10/SEP-24 and organization values out instead of failing startup.Verification:
npm test -- --runInBand test/stellarToml.test.js: 2 tests passednpm test -- --runInBand: 52 passed, 33 failed because MongoDB is not available locally; cleanupstream/devhas the same 33 failures inapp.test.jsandrefund.test.js(50 passed)node --checkon every JavaScript file): passed/healthreturned 200The repository has no configured lint script. GitHub CI supplies MongoDB 7 for the full suite.
Summary by CodeRabbit
/.well-known/stellar.tomlSEP-1 discovery endpoint with generated TOML (network, platform account, organization details, auth/signing, transfer server, and USDC).503 NETWORK_UNAVAILABLEresponses.