Merge pull request #69 from Ahbiz/feature/non-custodial-refunds feat(stellar): implement non-custodial refund and dispute flow (#62) - #70
Conversation
feat(stellar): add path payment support with quote endpoint and slipp…
feat(jobs): add durable background processing
Add preflightPayment() to stellarService.js, validating destination existence, USDC trustline, source balance/reserve, and SEP-29 memo-required status before an unsigned XDR is built. Wire it into initializePayment (400 with structured reasons, no Transaction row created on failure) and expose it standalone via POST /api/stellar/payment/preflight so the frontend can surface blocking issues before prompting the wallet to sign. Centralize the platform's DNB-<TYPE>-<id8> memo construction into a single helper shared by both call sites.
…ty-checks Add pre-flight payment safety checks with SEP-29 memo detection
…lone MongoDB CI compatibility
feat(stellar): implement non-custodial refund and dispute flow (#62)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds durable background jobs, queued email and Stellar verification, path-payment and preflight support, refund/dispute workflows, recommendation caching, media-upload hardening, SEP-1 metadata, supporting routes, models, configuration, documentation, and tests. ChangesBackground job processing
Stellar payment and refund flows
Recommendation caching
Media upload hardening
SEP-1 metadata
Authentication and project configuration
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 11
🧹 Nitpick comments (5)
src/controllers/stellar/refundController.js (1)
105-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider aligning response payloads with the
{ success, message, data }shape.These handlers return domain fields at the top level (
refund,unsignedXdr,txHash,disclaimer) rather than nesting them underdata. Standardizing helps clients parse responses uniformly across endpoints.As per path instructions (
src/**/*.js): responses should use "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/refundController.js` around lines 105 - 109, Update the refund response in the relevant refundController handler to preserve the success and message fields while nesting the refund domain payload under data. Apply the same { success, message, data } structure to nearby handlers returning unsignedXdr, txHash, or disclaimer, without changing their underlying values.Source: Path instructions
src/jobs/handlers.js (1)
44-59: 🩺 Stability & Availability | 🔵 TrivialAsync confirmations/failures never touch the payment metrics.
submitPaymentincrementspaymentsConfirmed/paymentsFailed, but this handler — which now owns the outcome for every transiently-deferred payment — doesn't. Any payment that goes through the retry path will be invisible in those counters, which will quietly skew your success/failure dashboards and alerting. Consider incrementing the same counters here when you setconfirmed(Line 56) andfailed(Line 50).🤖 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/jobs/handlers.js` around lines 44 - 59, Update the async outcome handling in the job handler to increment the existing payment metrics when setting transaction.status to "failed" or "confirmed". Reuse the same paymentsFailed and paymentsConfirmed counters and increment each exactly once for its corresponding terminal outcome, without counting transient retry attempts.src/models/Job.js (1)
3-24: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd indexes for the dashboard's query patterns.
jobsRoutes.jssorts all jobs bycreatedAt(Line 23) and dead jobs byfailedAt(Line 34), but neither field is indexed here. As thejobscollection grows, both become full collection scans with in-memory sorts.♻️ Suggested indexes
jobSchema.index({ status: 1, runAt: 1 }); +jobSchema.index({ createdAt: -1 }); +jobSchema.index({ status: 1, failedAt: -1 });As per path instructions, "Flag schema changes that break existing documents, missing indexes for new query patterns, and TTL indexes that could delete durable records."
🤖 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/Job.js` around lines 3 - 24, Add indexes to the jobSchema for the dashboard query fields: index createdAt for the all-jobs sort and failedAt for dead-job sorting, while preserving the existing schema fields and options.Source: Path instructions
test/jobs.test.js (2)
55-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the successful/JSON/HTML dashboard paths.
Only the auth-failure (401) branches of
/admin/jobsand/admin/jobs/deadare tested. Consider adding a case with a correct bearer token asserting a 200 response for both endpoints (and ideally the HTML-escaping behavior), since this is new endpoint behavior. 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 `@test/jobs.test.js` around lines 55 - 65, Add Jest coverage for the authenticated success paths of the /admin/jobs and /admin/jobs/dead endpoints alongside the existing bearer-token test. Use the configured JOBS_DASHBOARD_TOKEN, assert both requests return 200, and verify the expected JSON and HTML responses, including HTML escaping where applicable.Source: Path instructions
55-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
JOBS_DASHBOARD_TOKENafter the test to avoid leaking into other suites.The token is set on
process.envbut never cleared, so it can persist across other test files sharing the same Jest worker and make/admin/jobsunexpectedly gated (or ungated) for unrelated tests.🧹 Suggested cleanup
it("protects the jobs dashboard with a bearer token", async () => { process.env.JOBS_DASHBOARD_TOKEN = "dashboard-secret"; const missing = await request(app).get("/admin/jobs"); const wrong = await request(app) .get("/admin/jobs") .set("Authorization", "Bearer wrong"); expect(missing.statusCode).toBe(401); expect(wrong.statusCode).toBe(401); + + delete process.env.JOBS_DASHBOARD_TOKEN; });🤖 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/jobs.test.js` around lines 55 - 65, Update the “protects the jobs dashboard with a bearer token” test to restore the previous JOBS_DASHBOARD_TOKEN value after assertions complete, including when the test fails. Preserve any pre-existing value and remove the environment variable when none existed, so it cannot leak into other suites.
🤖 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 `@services/emails/sendMail.js`:
- Around line 33-43: Update sendOtpEmail and sendReceiptEmail to stop logging
raw recipient email addresses at info level. Remove the email values from both
logger.info messages, while preserving the OTP/receipt context and retaining
txHash in the receipt log.
In `@src/controllers/authController.js`:
- Around line 126-130: Guard the sendOtpEmail enqueue in the registration flow
with its own try/catch after User.create succeeds, logging the enqueue failure
and continuing without converting the committed registration into a 500
response. Preserve the existing OTP payload and enqueue options, and match the
prior direct sendMail error-handling behavior.
- Around line 126-130: Update registerUser to generate a fresh six-digit OTP per
request using the crypto-based generator, removing reliance on the module-level
generatedOtp from emailRoutes.js. Wrap User.create and enqueue in a database
session/transaction, committing only when both succeed and aborting/rolling back
on queue failure so no partial user remains; pass the request-scoped OTP to the
job payload.
In `@src/controllers/stellar/refundController.js`:
- Around line 267-292: The refund handling in the course and book branches must
compare nested purchase identifiers rather than whole subdocuments. In
src/controllers/stellar/refundController.js lines 267-292, update the
purchasedCourses filter to use courseId and purchasedBooks to use bookId; in
test/refund.test.js line 143, seed purchasedCourses with an object containing
courseId so the test matches the User schema and validates revocation.
- Around line 238-260: Update submitRefund’s Horizon verification flow to
validate the transaction’s payment operations using verifyPaymentOperations or
the established equivalent, not only verifyTransaction existence and success.
Require confirmation that the expected buyer payment occurred before allowing
the refund state to proceed to confirmed or revoking access, while preserving
the existing failed-state response for unsuccessful verification.
In `@src/jobs/handlers.js`:
- Around line 35-73: Update the confirmation flow around the handler’s status
mutation and purchase-grant logic to use a Mongo session with
session.withTransaction(...), keeping verifyPaymentOperations outside the
transaction. Include marking the transaction confirmed, clearing failureReason,
saving it with the session, and updating User/Course access within the same
transaction; ensure retries can re-enter and complete any missing grant before
the confirmed short-circuit, while queueReceipt remains after the transaction
commits.
In `@src/jobs/queue.js`:
- Around line 61-76: Update enqueue() around the Job.findOneAndUpdate call to
catch duplicate-key errors where error.code is 11000, then re-read the existing
job by idempotencyKey and return the same queued result with its duplicate
status. Preserve normal error propagation for other failures and retain the
current insert/update behavior.
- Around line 87-121: Separate handler execution from completion-status
persistence in the async job flow: only handler errors should enter the
retry/dead-letter logic, while a failure updating `"completed"` must not
reclassify or replay a successfully executed handler. Also guard the
failure-path `Job.updateOne` in the catch block so persistence errors are logged
or otherwise handled without leaving the promise from the IIFE unhandled; update
the surrounding promise cleanup to preserve the existing `inFlight` behavior.
In `@src/routes/jobsRoutes.js`:
- Around line 22-36: Restrict JSON output in the GET "/" and GET "/dead"
handlers to a safe field allowlist instead of returning full Job documents,
excluding payload and other sensitive fields such as OTPs and user identifiers.
Apply the same projection or explicit serialization to both jobs queries while
preserving the existing sorting, limits, and response structure.
In `@src/services/stellar/stellarService.js`:
- Around line 606-630: The payment matching logic in the expectedPayments loop
incorrectly reads destination_amount and destination asset fields for
path_payment_strict_receive operations. Update the paymentOps filter and amount
selection to use amount, asset_code, and asset_issuer for these records while
preserving the existing recipient check through op.to; add or update a fixture
covering a real path-payment record if the surrounding tests support fixtures.
In `@test/refund.test.js`:
- Line 26: Remove the hardcoded fallback from JWT_SECRET and require
process.env.JWT_SECRET to be present, failing fast when it is absent. Keep the
test’s existing JWT usage unchanged and ensure the required environment variable
is documented in .env.example.
---
Nitpick comments:
In `@src/controllers/stellar/refundController.js`:
- Around line 105-109: Update the refund response in the relevant
refundController handler to preserve the success and message fields while
nesting the refund domain payload under data. Apply the same { success, message,
data } structure to nearby handlers returning unsignedXdr, txHash, or
disclaimer, without changing their underlying values.
In `@src/jobs/handlers.js`:
- Around line 44-59: Update the async outcome handling in the job handler to
increment the existing payment metrics when setting transaction.status to
"failed" or "confirmed". Reuse the same paymentsFailed and paymentsConfirmed
counters and increment each exactly once for its corresponding terminal outcome,
without counting transient retry attempts.
In `@src/models/Job.js`:
- Around line 3-24: Add indexes to the jobSchema for the dashboard query fields:
index createdAt for the all-jobs sort and failedAt for dead-job sorting, while
preserving the existing schema fields and options.
In `@test/jobs.test.js`:
- Around line 55-65: Add Jest coverage for the authenticated success paths of
the /admin/jobs and /admin/jobs/dead endpoints alongside the existing
bearer-token test. Use the configured JOBS_DASHBOARD_TOKEN, assert both requests
return 200, and verify the expected JSON and HTML responses, including HTML
escaping where applicable.
- Around line 55-65: Update the “protects the jobs dashboard with a bearer
token” test to restore the previous JOBS_DASHBOARD_TOKEN value after assertions
complete, including when the test fails. Preserve any pre-existing value and
remove the environment variable when none existed, so it cannot leak into other
suites.
🪄 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: b4f1d85d-822e-495c-962a-83fdc7234e27
📒 Files selected for processing (26)
.env.example.gitignoreREADME.mdapp.jsserver.jsservices/emails/sendMail.jssrc/config/validateEnv.jssrc/controllers/authController.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/controllers/stellar/refundController.jssrc/jobs/handlers.jssrc/jobs/queue.jssrc/middlewares/authMiddleware.jssrc/models/Job.jssrc/models/Refund.jssrc/models/Transaction.jssrc/models/User.jssrc/routes/jobsRoutes.jssrc/routes/stellar/paymentRoutes.jssrc/services/stellar/stellarService.jstest/jobHandlers.test.jstest/jobs.test.jstest/pathPayments.test.jstest/preflightPayment.test.jstest/refund.test.js
| export const sendOtpEmail = async (otp, email) => { | ||
| if (!email || !otp) throw new Error("Email and OTP are required to send the email."); | ||
| logger.info(`Sending OTP email to: ${email}`); | ||
| return sendTemplate(EMAILJS_TEMPLATE_ID, email, { otp }); | ||
| }; | ||
|
|
||
| export const sendReceiptEmail = async (receipt) => { | ||
| if (!receipt.email || !receipt.txHash) throw new Error("Receipt email and transaction hash are required"); | ||
| logger.info(`Sending receipt for ${receipt.txHash} to: ${receipt.email}`); | ||
| return sendTemplate(EMAILJS_RECEIPT_TEMPLATE_ID, receipt.email, receipt); | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid writing full email addresses to logs (PII).
The generic sendTemplate refactor is clean and the input validation is a good touch. One privacy nit flagged by static analysis: both wrappers log the recipient's raw email at info level (Sending OTP email to: ${email} and Sending receipt for ${txHash} to: ${receipt.email}). Email addresses are user identifiers, so persisting them in logs is a GDPR/CCPA exposure (CWE-532) — logs are often shipped to third-party aggregators with broader access than the DB. The txHash is fine to keep; masking or dropping the email keeps the log useful for debugging without the PII.
🔒 Suggested masking
- logger.info(`Sending OTP email to: ${email}`);
+ logger.info("Sending OTP email");- logger.info(`Sending receipt for ${receipt.txHash} to: ${receipt.email}`);
+ logger.info(`Sending receipt for ${receipt.txHash}`);📝 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.
| export const sendOtpEmail = async (otp, email) => { | |
| if (!email || !otp) throw new Error("Email and OTP are required to send the email."); | |
| logger.info(`Sending OTP email to: ${email}`); | |
| return sendTemplate(EMAILJS_TEMPLATE_ID, email, { otp }); | |
| }; | |
| export const sendReceiptEmail = async (receipt) => { | |
| if (!receipt.email || !receipt.txHash) throw new Error("Receipt email and transaction hash are required"); | |
| logger.info(`Sending receipt for ${receipt.txHash} to: ${receipt.email}`); | |
| return sendTemplate(EMAILJS_RECEIPT_TEMPLATE_ID, receipt.email, receipt); | |
| }; | |
| export const sendOtpEmail = async (otp, email) => { | |
| if (!email || !otp) throw new Error("Email and OTP are required to send the email."); | |
| logger.info("Sending OTP email"); | |
| return sendTemplate(EMAILJS_TEMPLATE_ID, email, { otp }); | |
| }; | |
| export const sendReceiptEmail = async (receipt) => { | |
| if (!receipt.email || !receipt.txHash) throw new Error("Receipt email and transaction hash are required"); | |
| logger.info(`Sending receipt for ${receipt.txHash}`); | |
| return sendTemplate(EMAILJS_RECEIPT_TEMPLATE_ID, receipt.email, receipt); | |
| }; |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 34-34: Avoid logging sensitive data
Context: logger.info(Sending OTP email to: ${email})
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data)
[warning] 40-40: Avoid logging sensitive data
Context: logger.info(Sending receipt for ${receipt.txHash} to: ${receipt.email})
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-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 `@services/emails/sendMail.js` around lines 33 - 43, Update sendOtpEmail and
sendReceiptEmail to stop logging raw recipient email addresses at info level.
Remove the email values from both logger.info messages, while preserving the
OTP/receipt context and retaining txHash in the receipt log.
Source: Linters/SAST tools
| await enqueue( | ||
| "sendOtpEmail", | ||
| { userId: user._id.toString(), otp: generatedOtp }, | ||
| { attempts: 5, backoffMs: 1000, idempotencyKey: `otp:${user._id}:${generatedOtp}` } | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded enqueue() can fail an already-committed registration.
User.create() (Line 119) is not wrapped in a session/transaction, so if enqueue("sendOtpEmail", ...) throws (queue shutting down, transient DB error, or the E11000 upsert race flagged in queue.js), the surrounding catchAsync will turn this into a 500 response even though the user account was already persisted. The client is told registration failed while the account silently exists (and can't retry with the same email). Contrast with donationController.submitDonation, which wraps its enqueue() call inside the same Mongo session as the record write, so a queue failure cleanly aborts the whole transaction.
Wrap the enqueue call in its own try/catch (log-and-continue, matching the previous behavior for the removed direct sendMail call) or move user creation + OTP enqueue into a shared transaction.
🤖 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/authController.js` around lines 126 - 130, Guard the
sendOtpEmail enqueue in the registration flow with its own try/catch after
User.create succeeds, logging the enqueue failure and continuing without
converting the committed registration into a 500 response. Preserve the existing
OTP payload and enqueue options, and match the prior direct sendMail
error-handling behavior.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether generatedOtp is a static constant or invoked per call.
rg -n "generatedOtp" -C5 --type=jsRepository: Deen-Bridge/dnb-backend
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== generatedOtp references ==\n'
rg -n "generatedOtp" .
printf '\n== candidate auth/email files ==\n'
git ls-files 'src/**/*.js' | rg 'authController|emailRoutes|jobsRoutes|queue|donationController'
printf '\n== outline authController.js ==\n'
ast-grep outline src/controllers/authController.js --view expanded || true
printf '\n== outline emailRoutes.js if present ==\n'
ast-grep outline src/routes/emailRoutes.js --view expanded || trueRepository: Deen-Bridge/dnb-backend
Length of output: 2093
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show whether the referenced files and symbol actually exist.
for f in src/controllers/authController.js src/routes/emailRoutes.js src/routes/jobsRoutes.js src/controllers/donationController.js; do
if [ -f "$f" ]; then
printf '\n== %s ==\n' "$f"
nl -ba "$f" | sed -n '1,220p'
else
printf '\n== missing: %s ==\n' "$f"
fi
done
printf '\n== all matches for emailRoutes / generatedOtp ==\n'
rg -n "emailRoutes|generatedOtp|sendOtpEmail|admin/jobs" src . || trueRepository: Deen-Bridge/dnb-backend
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== authController registerUser excerpt ==\n'
sed -n '103,140p' src/controllers/authController.js
printf '\n== emailRoutes excerpt ==\n'
sed -n '1,40p' src/routes/emailRoutes.js
printf '\n== jobsRoutes excerpt ==\n'
sed -n '1,120p' src/routes/jobsRoutes.js
printf '\n== queue excerpt ==\n'
sed -n '1,220p' src/jobs/queue.jsRepository: Deen-Bridge/dnb-backend
Length of output: 8584
Generate the OTP per request and keep user creation/enqueue atomic
generatedOtp is a module-level constant in src/routes/emailRoutes.js, so every registration during the process lifetime reuses the same code. That makes one OTP valid for every account and stores the raw code in the job payload. Move OTP generation into registerUser (for example, crypto.randomInt(100000, 1000000).toString()) and wrap User.create() + enqueue() in a session/transaction so a queue failure doesn’t leave a half-created account behind.
🤖 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/authController.js` around lines 126 - 130, Update
registerUser to generate a fresh six-digit OTP per request using the
crypto-based generator, removing reliance on the module-level generatedOtp from
emailRoutes.js. Wrap User.create and enqueue in a database session/transaction,
committing only when both succeed and aborting/rolling back on queue failure so
no partial user remains; pass the request-scoped OTP to the job payload.
| // Submit transaction to Stellar network | ||
| let submissionResult; | ||
| try { | ||
| submissionResult = await submitTransaction(signedXdr); | ||
| } catch (submitErr) { | ||
| refund.status = "failed"; | ||
| await refund.save(); | ||
| return res.status(400).json({ | ||
| success: false, | ||
| message: `Stellar transaction failed: ${submitErr.message}`, | ||
| }); | ||
| } | ||
|
|
||
| // On-Chain Truth Verification via Horizon | ||
| const verification = await verifyTransaction(submissionResult.hash); | ||
| if (!verification.exists || !verification.successful) { | ||
| refund.status = "failed"; | ||
| await refund.save(); | ||
| return res.status(400).json({ | ||
| success: false, | ||
| message: "Reverse payment transaction could not be verified on Horizon", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect what verifyTransaction actually validates (op-level checks?)
ast-grep outline src/services/stellar/stellarService.js --items all
rg -nP 'verifyTransaction' src/services/stellar/stellarService.js -A 50Repository: Deen-Bridge/dnb-backend
Length of output: 4972
Use operation-level verification before confirming the refund. submitRefund still only checks that a Horizon transaction exists and succeeded, so an unrelated successful tx can mark the refund confirmed and revoke access without proving the buyer was paid. Use verifyPaymentOperations (or equivalent payment-op checks) here before updating state.
🤖 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/refundController.js` around lines 238 - 260, Update
submitRefund’s Horizon verification flow to validate the transaction’s payment
operations using verifyPaymentOperations or the established equivalent, not only
verifyTransaction existence and success. Require confirmation that the expected
buyer payment occurred before allowing the refund state to proceed to confirmed
or revoking access, while preserving the existing failed-state response for
unsuccessful verification.
Source: Path instructions
| if (refund.itemType === "course") { | ||
| // Remove course from buyer's purchased list | ||
| if (buyer) { | ||
| buyer.purchasedCourses = (buyer.purchasedCourses || []).filter( | ||
| (cId) => cId.toString() !== refund.itemId.toString() | ||
| ); | ||
| await buyer.save(); | ||
| } | ||
|
|
||
| // Remove buyer from Course.enrolledUsers | ||
| const course = await Course.findById(refund.itemId); | ||
| if (course) { | ||
| course.enrolledUsers = (course.enrolledUsers || []).filter( | ||
| (uId) => uId.toString() !== refund.buyer.toString() | ||
| ); | ||
| await course.save(); | ||
| } | ||
| } else if (refund.itemType === "book") { | ||
| // Remove book from buyer's purchased list | ||
| if (buyer) { | ||
| buyer.purchasedBooks = (buyer.purchasedBooks || []).filter( | ||
| (bId) => bId.toString() !== refund.itemId.toString() | ||
| ); | ||
| await buyer.save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Purchase arrays are subdocuments, not bare ObjectIds — controller revocation and the test seed both diverge from the User schema. In src/models/User.js (Lines 68-95) purchasedCourses/purchasedBooks are arrays of { courseId|bookId, purchaseDate }. The controller compares whole subdocuments so revocation is a silent no-op, and the test seeds bare ObjectIds so it never exercises the real shape.
src/controllers/stellar/refundController.js#L267-L292: filter on the nested id field (c.courseId/b.bookId) instead of the subdocument itself.test/refund.test.js#L143-L143: seedbuyer.purchasedCourses = [{ courseId: course._id }]to match the schema so the revocation path is actually validated.
📍 Affects 2 files
src/controllers/stellar/refundController.js#L267-L292(this comment)test/refund.test.js#L143-L143
🤖 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/refundController.js` around lines 267 - 292, The
refund handling in the course and book branches must compare nested purchase
identifiers rather than whole subdocuments. In
src/controllers/stellar/refundController.js lines 267-292, update the
purchasedCourses filter to use courseId and purchasedBooks to use bookId; in
test/refund.test.js line 143, seed purchasedCourses with an object containing
courseId so the test matches the User schema and validates revocation.
| if (transaction.status === "confirmed") { | ||
| await queueReceipt(transaction); | ||
| return; | ||
| } | ||
|
|
||
| const verification = await verifyPaymentOperations( | ||
| transaction.stellarTxHash, | ||
| expectedPaymentsFor(transaction) | ||
| ); | ||
| if (!verification.verified) { | ||
| transaction.retryCount = context.attempt; | ||
| if (verification.transient && context.attempt < context.maxAttempts) { | ||
| await transaction.save(); | ||
| throw new Error(verification.reason); | ||
| } | ||
| transaction.status = "failed"; | ||
| transaction.failureReason = `On-chain verification failed: ${verification.reason}`; | ||
| await transaction.save(); | ||
| return; | ||
| } | ||
|
|
||
| transaction.status = "confirmed"; | ||
| transaction.confirmedAt = new Date(); | ||
| transaction.failureReason = undefined; | ||
| await transaction.save(); | ||
|
|
||
| if (transaction.type === "purchase") { | ||
| await recordSaleEarnings(transaction); | ||
| const purchase = { purchaseDate: transaction.confirmedAt }; | ||
| if (transaction.itemType === "book") { | ||
| purchase.bookId = transaction.itemId; | ||
| await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedBooks: purchase } }); | ||
| } else { | ||
| purchase.courseId = transaction.itemId; | ||
| await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedCourses: purchase } }); | ||
| await Course.updateOne({ _id: transaction.itemId }, { $addToSet: { enrolledUsers: transaction.buyer } }); | ||
| } | ||
| } | ||
| await queueReceipt(transaction); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Confirm-and-grant isn't atomic, so a mid-way crash can leave a paid buyer without access. Great job building out the async verification path — this is the one spot I'd fix before merge.
The handler runs without a Mongo session, so these steps are independent writes: mark confirmed + save() (Lines 56-59), then grant the purchase to User/Course (Lines 61-71), then queue the receipt (Line 73). If the process dies (or the DB connection drops) after Line 59 commits but before the grant completes, the next retry re-enters, hits the status === "confirmed" short-circuit at Lines 35-38, re-queues the receipt, and returns — the purchase grant is skipped forever. The buyer's money moved on-chain, the transaction reads confirmed, yet they own nothing, and nothing will ever heal it.
The clean fix is to keep the Horizon call outside a transaction, then wrap the state mutation + access grant in a single session.withTransaction(...) so they commit atomically (mirroring what submitPayment already does).
🛡️ Sketch of an atomic confirm+grant
- transaction.status = "confirmed";
- transaction.confirmedAt = new Date();
- transaction.failureReason = undefined;
- await transaction.save();
-
- if (transaction.type === "purchase") {
- await recordSaleEarnings(transaction);
- const purchase = { purchaseDate: transaction.confirmedAt };
- if (transaction.itemType === "book") {
- purchase.bookId = transaction.itemId;
- await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedBooks: purchase } });
- } else {
- purchase.courseId = transaction.itemId;
- await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedCourses: purchase } });
- await Course.updateOne({ _id: transaction.itemId }, { $addToSet: { enrolledUsers: transaction.buyer } });
- }
- }
- await queueReceipt(transaction);
+ const session = await mongoose.startSession();
+ try {
+ await session.withTransaction(async () => {
+ transaction.status = "confirmed";
+ transaction.confirmedAt = new Date();
+ transaction.failureReason = undefined;
+ await transaction.save({ session });
+
+ if (transaction.type === "purchase") {
+ await recordSaleEarnings(transaction, { session });
+ const purchase = { purchaseDate: transaction.confirmedAt };
+ if (transaction.itemType === "book") {
+ purchase.bookId = transaction.itemId;
+ await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedBooks: purchase } }, { session });
+ } else {
+ purchase.courseId = transaction.itemId;
+ await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedCourses: purchase } }, { session });
+ await Course.updateOne({ _id: transaction.itemId }, { $addToSet: { enrolledUsers: transaction.buyer } }, { session });
+ }
+ }
+ });
+ } finally {
+ await session.endSession();
+ }
+ await queueReceipt(transaction);📝 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 (transaction.status === "confirmed") { | |
| await queueReceipt(transaction); | |
| return; | |
| } | |
| const verification = await verifyPaymentOperations( | |
| transaction.stellarTxHash, | |
| expectedPaymentsFor(transaction) | |
| ); | |
| if (!verification.verified) { | |
| transaction.retryCount = context.attempt; | |
| if (verification.transient && context.attempt < context.maxAttempts) { | |
| await transaction.save(); | |
| throw new Error(verification.reason); | |
| } | |
| transaction.status = "failed"; | |
| transaction.failureReason = `On-chain verification failed: ${verification.reason}`; | |
| await transaction.save(); | |
| return; | |
| } | |
| transaction.status = "confirmed"; | |
| transaction.confirmedAt = new Date(); | |
| transaction.failureReason = undefined; | |
| await transaction.save(); | |
| if (transaction.type === "purchase") { | |
| await recordSaleEarnings(transaction); | |
| const purchase = { purchaseDate: transaction.confirmedAt }; | |
| if (transaction.itemType === "book") { | |
| purchase.bookId = transaction.itemId; | |
| await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedBooks: purchase } }); | |
| } else { | |
| purchase.courseId = transaction.itemId; | |
| await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedCourses: purchase } }); | |
| await Course.updateOne({ _id: transaction.itemId }, { $addToSet: { enrolledUsers: transaction.buyer } }); | |
| } | |
| } | |
| await queueReceipt(transaction); | |
| if (transaction.status === "confirmed") { | |
| await queueReceipt(transaction); | |
| return; | |
| } | |
| const verification = await verifyPaymentOperations( | |
| transaction.stellarTxHash, | |
| expectedPaymentsFor(transaction) | |
| ); | |
| if (!verification.verified) { | |
| transaction.retryCount = context.attempt; | |
| if (verification.transient && context.attempt < context.maxAttempts) { | |
| await transaction.save(); | |
| throw new Error(verification.reason); | |
| } | |
| transaction.status = "failed"; | |
| transaction.failureReason = `On-chain verification failed: ${verification.reason}`; | |
| await transaction.save(); | |
| return; | |
| } | |
| const session = await mongoose.startSession(); | |
| try { | |
| await session.withTransaction(async () => { | |
| transaction.status = "confirmed"; | |
| transaction.confirmedAt = new Date(); | |
| transaction.failureReason = undefined; | |
| await transaction.save({ session }); | |
| if (transaction.type === "purchase") { | |
| await recordSaleEarnings(transaction, { session }); | |
| const purchase = { purchaseDate: transaction.confirmedAt }; | |
| if (transaction.itemType === "book") { | |
| purchase.bookId = transaction.itemId; | |
| await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedBooks: purchase } }, { session }); | |
| } else { | |
| purchase.courseId = transaction.itemId; | |
| await User.updateOne({ _id: transaction.buyer }, { $addToSet: { purchasedCourses: purchase } }, { session }); | |
| await Course.updateOne({ _id: transaction.itemId }, { $addToSet: { enrolledUsers: transaction.buyer } }, { session }); | |
| } | |
| } | |
| }); | |
| } finally { | |
| await session.endSession(); | |
| } | |
| await queueReceipt(transaction); |
🤖 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/jobs/handlers.js` around lines 35 - 73, Update the confirmation flow
around the handler’s status mutation and purchase-grant logic to use a Mongo
session with session.withTransaction(...), keeping verifyPaymentOperations
outside the transaction. Include marking the transaction confirmed, clearing
failureReason, saving it with the session, and updating User/Course access
within the same transaction; ensure retries can re-enter and complete any
missing grant before the confirmed short-circuit, while queueReceipt remains
after the transaction commits.
| const job = await Job.findOneAndUpdate( | ||
| { idempotencyKey: options.idempotencyKey }, | ||
| { | ||
| $setOnInsert: { | ||
| name, | ||
| payload, | ||
| idempotencyKey: options.idempotencyKey, | ||
| maxAttempts: options.attempts, | ||
| backoffMs: options.backoffMs, | ||
| status: "queued", | ||
| runAt: new Date(), | ||
| }, | ||
| }, | ||
| { upsert: true, new: true, session: options.session } | ||
| ); | ||
| return { queued: true, id: job._id, duplicate: job.attemptsMade > 0 }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
mongoose findOneAndUpdate upsert unique index duplicate key race condition E11000
💡 Result:
The E11000 duplicate key error during a Mongoose or MongoDB findOneAndUpdate operation with upsert: true is a known race condition [1][2]. Although findOneAndUpdate is atomic in many contexts, the upsert operation is effectively a multi-step process: the database first queries for a matching document and, if none is found, attempts an insert [3]. In a high-concurrency environment, two or more separate processes or threads can perform this query simultaneously, both observe that no matching document exists, and both proceed to attempt an insert [1][4]. Because you have a unique index on the field(s) used in your filter, the second process to arrive at the insert stage will trigger an E11000 duplicate key error [3][5]. To resolve this, you must handle the error at the application level [3][6]: 1. Implement a retry mechanism: When your code catches an E11000 error during an upsert, it implies that the document was successfully created by another concurrent process [3][2]. You can safely retry the operation (often simply by performing a standard update) because you now know the document exists [3][2]. 2. Use a unique index: Ensure the fields used in your query filter are covered by a unique index [7][8]. This prevents the database from ever having duplicate documents, but it makes handling the race condition with an application-level retry necessary [3][5]. 3. Minimize the window for conflict: Ensure your filter is as precise as possible [7]. If you are performing upserts, use operators like $setOnInsert if you only want to set certain fields during the creation phase without overwriting existing data if the document is found [9]. Example logic for handling the error: // Example using a simple retry pattern async function upsertWithRetry(model, filter, update, options = {}) { try { return await model.findOneAndUpdate(filter, update, {...options, upsert: true }); } catch (err) { if (err.code === 11000) { // Document likely created by another thread; retry as a standard update return await model.findOneAndUpdate(filter, update, {...options, upsert: false }); } throw err; } } [3][2] Top Results: [10][3][1][2]
Citations:
- 1: https://stackoverflow.com/questions/29305405/mongodb-impossible-e11000-duplicate-key-error-dup-key-when-upserting
- 2: https://joeywhelan.blogspot.com/2019/01/mongodb-e11000-duplicate-key-error.html
- 3: https://stackoverflow.com/questions/37295648/mongoose-duplicate-key-error-with-upsert
- 4: https://www.mongodb.com/community/forums/t/what-is-the-correct-way-to-do-an-upsert/294181
- 5: https://stackoverflow.com/questions/46378497/duplicate-key-error-in-mongoose-findoneandupdate-and-upsert
- 6: https://stackoverflow.com/questions/33346175/mongodb-upsert-operation-seems-not-atomic-which-throws-duplicatekeyexception
- 7: https://stackoverflow.com/questions/30404513/mongodb-update-with-upsert-and-unique-index-atomicity-when-document-is-not-in-th
- 8: https://www.mongodb.com/docs/manual/reference/method/db.collection.findoneandupdate/
- 9: https://stackoverflow.com/questions/53211567/mongoose-atomic-findone-or-insert-do-not-update-existing-instance-if-found
- 10: https://mongoosejs.com/docs/tutorials/findoneandupdate
Handle the duplicate-key race in enqueue()
Concurrent calls with the same idempotencyKey can still collide on the unique index and surface E11000 instead of returning the existing job. Catch error.code === 11000 and re-read by idempotencyKey so retries/double-submits stay idempotent rather than failing the caller.
🤖 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/jobs/queue.js` around lines 61 - 76, Update enqueue() around the
Job.findOneAndUpdate call to catch duplicate-key errors where error.code is
11000, then re-read the existing job by idempotencyKey and return the same
queued result with its duplicate status. Preserve normal error propagation for
other failures and retain the current insert/update behavior.
| const promise = (async () => { | ||
| try { | ||
| const handler = handlers.get(job.name); | ||
| if (!handler) throw new Error(`No handler registered for job ${job.name}`); | ||
| await handler(job.payload, { | ||
| attempt: job.attemptsMade, | ||
| maxAttempts: job.maxAttempts, | ||
| }); | ||
| await Job.updateOne( | ||
| { _id: job._id }, | ||
| { $set: { status: "completed", completedAt: new Date() }, $unset: { lockedAt: 1 } } | ||
| ); | ||
| } catch (error) { | ||
| const exhausted = job.attemptsMade >= job.maxAttempts; | ||
| await Job.updateOne( | ||
| { _id: job._id }, | ||
| { | ||
| $set: exhausted | ||
| ? { status: "dead", failedAt: new Date(), lastError: error.message } | ||
| : { | ||
| status: "retrying", | ||
| runAt: new Date(Date.now() + retryDelay(job.backoffMs, job.attemptsMade)), | ||
| lastError: error.message, | ||
| }, | ||
| $unset: { lockedAt: 1 }, | ||
| } | ||
| ); | ||
| logger[exhausted ? "error" : "warn"]( | ||
| { job: job.name, jobId: job._id, attempt: job.attemptsMade, error: error.message }, | ||
| exhausted ? "Job moved to dead letter" : "Job scheduled for retry" | ||
| ); | ||
| } | ||
| })().finally(() => inFlight.delete(promise)); | ||
| inFlight.add(promise); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Handler success can be misclassified as failure, causing duplicate side effects; failure-path write is unguarded.
Two related problems in this block:
- If the handler succeeds but the subsequent
Job.updateOne(...status:"completed"...)(Lines 95-98) throws (e.g. a transient DB error), execution lands in thecatchblock, which treats it as a handler failure and marks the job"retrying"/"dead". Since the handler itself already ran and its side effects (send email, generate receipt, mark payment confirmed) are typically not idempotent-safe against replay, the job will be re-executed and those effects can fire twice. - The failure-path
Job.updateOne(...)inside thecatchblock (Lines 101-113) has no error handling of its own. If it throws, the async IIFE's promise rejects, and since it's chained only with.finally()(not.catch()), that rejection remains unhandled until some laterstopJobs()/waitForIdle()call attaches a handler viaPromise.allSettled. In the interim this is an unhandled promise rejection, which can crash the process on modern Node.js.
🐛 Suggested restructuring
const promise = (async () => {
+ let handlerSucceeded = false;
try {
const handler = handlers.get(job.name);
if (!handler) throw new Error(`No handler registered for job ${job.name}`);
await handler(job.payload, {
attempt: job.attemptsMade,
maxAttempts: job.maxAttempts,
});
+ handlerSucceeded = true;
await Job.updateOne(
{ _id: job._id },
{ $set: { status: "completed", completedAt: new Date() }, $unset: { lockedAt: 1 } }
);
} catch (error) {
+ if (handlerSucceeded) {
+ logger.error({ job: job.name, jobId: job._id, error: error.message }, "Job completed but failed to persist completion");
+ return;
+ }
const exhausted = job.attemptsMade >= job.maxAttempts;
- await Job.updateOne(
- { _id: job._id },
- { ... }
- );
+ try {
+ await Job.updateOne(
+ { _id: job._id },
+ { ... }
+ );
+ } catch (writeError) {
+ logger.error({ job: job.name, jobId: job._id, error: writeError.message }, "Failed to persist job failure state");
+ }
logger[exhausted ? "error" : "warn"](...);
}
})().finally(() => inFlight.delete(promise));📝 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.
| const promise = (async () => { | |
| try { | |
| const handler = handlers.get(job.name); | |
| if (!handler) throw new Error(`No handler registered for job ${job.name}`); | |
| await handler(job.payload, { | |
| attempt: job.attemptsMade, | |
| maxAttempts: job.maxAttempts, | |
| }); | |
| await Job.updateOne( | |
| { _id: job._id }, | |
| { $set: { status: "completed", completedAt: new Date() }, $unset: { lockedAt: 1 } } | |
| ); | |
| } catch (error) { | |
| const exhausted = job.attemptsMade >= job.maxAttempts; | |
| await Job.updateOne( | |
| { _id: job._id }, | |
| { | |
| $set: exhausted | |
| ? { status: "dead", failedAt: new Date(), lastError: error.message } | |
| : { | |
| status: "retrying", | |
| runAt: new Date(Date.now() + retryDelay(job.backoffMs, job.attemptsMade)), | |
| lastError: error.message, | |
| }, | |
| $unset: { lockedAt: 1 }, | |
| } | |
| ); | |
| logger[exhausted ? "error" : "warn"]( | |
| { job: job.name, jobId: job._id, attempt: job.attemptsMade, error: error.message }, | |
| exhausted ? "Job moved to dead letter" : "Job scheduled for retry" | |
| ); | |
| } | |
| })().finally(() => inFlight.delete(promise)); | |
| inFlight.add(promise); | |
| }; | |
| const promise = (async () => { | |
| let handlerSucceeded = false; | |
| try { | |
| const handler = handlers.get(job.name); | |
| if (!handler) throw new Error(`No handler registered for job ${job.name}`); | |
| await handler(job.payload, { | |
| attempt: job.attemptsMade, | |
| maxAttempts: job.maxAttempts, | |
| }); | |
| handlerSucceeded = true; | |
| await Job.updateOne( | |
| { _id: job._id }, | |
| { $set: { status: "completed", completedAt: new Date() }, $unset: { lockedAt: 1 } } | |
| ); | |
| } catch (error) { | |
| if (handlerSucceeded) { | |
| logger.error( | |
| { job: job.name, jobId: job._id, error: error.message }, | |
| "Job completed but failed to persist completion" | |
| ); | |
| return; | |
| } | |
| const exhausted = job.attemptsMade >= job.maxAttempts; | |
| try { | |
| await Job.updateOne( | |
| { _id: job._id }, | |
| { | |
| $set: exhausted | |
| ? { status: "dead", failedAt: new Date(), lastError: error.message } | |
| : { | |
| status: "retrying", | |
| runAt: new Date(Date.now() + retryDelay(job.backoffMs, job.attemptsMade)), | |
| lastError: error.message, | |
| }, | |
| $unset: { lockedAt: 1 }, | |
| } | |
| ); | |
| } catch (writeError) { | |
| logger.error( | |
| { job: job.name, jobId: job._id, error: writeError.message }, | |
| "Failed to persist job failure state" | |
| ); | |
| } | |
| logger[exhausted ? "error" : "warn"]( | |
| { job: job.name, jobId: job._id, attempt: job.attemptsMade, error: error.message }, | |
| exhausted ? "Job moved to dead letter" : "Job scheduled for retry" | |
| ); | |
| } | |
| })().finally(() => inFlight.delete(promise)); | |
| inFlight.add(promise); | |
| }; |
🤖 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/jobs/queue.js` around lines 87 - 121, Separate handler execution from
completion-status persistence in the async job flow: only handler errors should
enter the retry/dead-letter logic, while a failure updating `"completed"` must
not reclassify or replay a successfully executed handler. Also guard the
failure-path `Job.updateOne` in the catch block so persistence errors are logged
or otherwise handled without leaving the promise from the IIFE unhandled; update
the surrounding promise cleanup to preserve the existing `inFlight` behavior.
| router.get("/", async (req, res) => { | ||
| const jobs = await Job.find().sort({ createdAt: -1 }).limit(100).lean(); | ||
| if (req.accepts(["html", "json"]) === "html") { | ||
| const rows = jobs | ||
| .map((job) => `<tr><td>${escapeHtml(job.name)}</td><td>${escapeHtml(job.status)}</td><td>${job.attemptsMade}/${job.maxAttempts}</td><td>${escapeHtml(job.lastError || "")}</td></tr>`) | ||
| .join(""); | ||
| return res.type("html").send(`<h1>DeenBridge Jobs</h1><table><tr><th>Name</th><th>Status</th><th>Attempts</th><th>Error</th></tr>${rows}</table>`); | ||
| } | ||
| res.json({ success: true, jobs }); | ||
| }); | ||
|
|
||
| router.get("/dead", async (req, res) => { | ||
| const jobs = await Job.find({ status: "dead" }).sort({ failedAt: -1 }).lean(); | ||
| res.json({ success: true, jobs }); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
JSON responses leak raw job payload, including OTPs and other sensitive data.
Both GET / (Line 30) and GET /dead (Line 35) return full lean() job documents as JSON, unfiltered. The HTML branch deliberately avoids exposing payload (only rendering name/status/attempts/error), but the JSON branch doesn't — and job payloads elsewhere in this PR contain sensitive data (e.g. sendOtpEmail's { userId, otp }). Anyone with the static dashboard bearer token could read live OTP codes and other user identifiers straight from this endpoint.
🔒 Suggested field allowlist
router.get("/", async (req, res) => {
- const jobs = await Job.find().sort({ createdAt: -1 }).limit(100).lean();
+ const jobs = await Job.find()
+ .select("name status attemptsMade maxAttempts lastError createdAt")
+ .sort({ createdAt: -1 })
+ .limit(100)
+ .lean();
...
});
router.get("/dead", async (req, res) => {
- const jobs = await Job.find({ status: "dead" }).sort({ failedAt: -1 }).lean();
+ const jobs = await Job.find({ status: "dead" })
+ .select("name status attemptsMade maxAttempts lastError failedAt")
+ .sort({ failedAt: -1 })
+ .lean();
res.json({ success: true, jobs });
});📝 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.
| router.get("/", async (req, res) => { | |
| const jobs = await Job.find().sort({ createdAt: -1 }).limit(100).lean(); | |
| if (req.accepts(["html", "json"]) === "html") { | |
| const rows = jobs | |
| .map((job) => `<tr><td>${escapeHtml(job.name)}</td><td>${escapeHtml(job.status)}</td><td>${job.attemptsMade}/${job.maxAttempts}</td><td>${escapeHtml(job.lastError || "")}</td></tr>`) | |
| .join(""); | |
| return res.type("html").send(`<h1>DeenBridge Jobs</h1><table><tr><th>Name</th><th>Status</th><th>Attempts</th><th>Error</th></tr>${rows}</table>`); | |
| } | |
| res.json({ success: true, jobs }); | |
| }); | |
| router.get("/dead", async (req, res) => { | |
| const jobs = await Job.find({ status: "dead" }).sort({ failedAt: -1 }).lean(); | |
| res.json({ success: true, jobs }); | |
| }); | |
| router.get("/", async (req, res) => { | |
| const jobs = await Job.find() | |
| .select("name status attemptsMade maxAttempts lastError createdAt") | |
| .sort({ createdAt: -1 }) | |
| .limit(100) | |
| .lean(); | |
| if (req.accepts(["html", "json"]) === "html") { | |
| const rows = jobs | |
| .map((job) => `<tr><td>${escapeHtml(job.name)}</td><td>${escapeHtml(job.status)}</td><td>${job.attemptsMade}/${job.maxAttempts}</td><td>${escapeHtml(job.lastError || "")}</td></tr>`) | |
| .join(""); | |
| return res.type("html").send(`<h1>DeenBridge Jobs</h1><table><tr><th>Name</th><th>Status</th><th>Attempts</th><th>Error</th></tr>${rows}</table>`); | |
| } | |
| res.json({ success: true, jobs }); | |
| }); | |
| router.get("/dead", async (req, res) => { | |
| const jobs = await Job.find({ status: "dead" }) | |
| .select("name status attemptsMade maxAttempts lastError failedAt") | |
| .sort({ failedAt: -1 }) | |
| .lean(); | |
| res.json({ success: true, jobs }); | |
| }); |
🤖 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/jobsRoutes.js` around lines 22 - 36, Restrict JSON output in the
GET "/" and GET "/dead" handlers to a safe field allowlist instead of returning
full Job documents, excluding payload and other sensitive fields such as OTPs
and user identifiers. Apply the same projection or explicit serialization to
both jobs queries while preserving the existing sorting, limits, and response
structure.
| const paymentOps = verification.operations.filter((op) => { | ||
| if (op.type === "payment") { | ||
| return ( | ||
| op.asset_code === "USDC" && op.asset_issuer === USDC_ISSUER | ||
| ); | ||
| } | ||
| if (op.type === "path_payment_strict_receive") { | ||
| return ( | ||
| op.destination_asset_code === "USDC" && | ||
| op.destination_asset_issuer === USDC_ISSUER | ||
| ); | ||
| } | ||
| return false; | ||
| }); | ||
|
|
||
| for (const expected of expectedPayments) { | ||
| const match = paymentOps.find( | ||
| (op) => | ||
| op.to === expected.destination && | ||
| toStroops(op.amount) === toStroops(expected.amount) | ||
| ); | ||
| const match = paymentOps.find((op) => { | ||
| if (op.to !== expected.destination) return false; | ||
|
|
||
| const opAmount = | ||
| op.type === "path_payment_strict_receive" | ||
| ? op.destination_amount | ||
| : op.amount; | ||
| return toStroops(opAmount) === toStroops(expected.amount); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby tests.
git ls-files 'src/services/stellar/stellarService.js' 'test/**' 'tests/**' | sed -n '1,120p'
echo
echo '--- stellarService.js outline ---'
ast-grep outline src/services/stellar/stellarService.js --view expanded | sed -n '1,220p'
echo
echo '--- nearby lines ---'
sed -n '590,650p' src/services/stellar/stellarService.js
echo
echo '--- search for verifyPaymentOperations tests/usages ---'
rg -n "verifyPaymentOperations|path_payment_strict_receive|destination_asset_code|destination_amount" src test testsRepository: Deen-Bridge/dnb-backend
Length of output: 6265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Pull the operation-record shape from installed docs if available in repo, otherwise inspect code references.
rg -n "Horizon.ServerApi|path_payment_strict_receive|destination_amount|source_amount|asset_code" . --glob '!node_modules' --glob '!dist' --glob '!build' | sed -n '1,220p'Repository: Deen-Bridge/dnb-backend
Length of output: 2537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, sys, urllib.request
# Public Stellar docs/spec references are enough here; fetch the Horizon operation record docs.
urls = [
"https://developers.stellar.org/docs/data/apis/horizon/api-reference/resources/operations/object",
"https://developers.stellar.org/docs/data/apis/horizon/api-reference/resources/paths/object",
]
for url in urls:
print(f"=== {url} ===")
try:
with urllib.request.urlopen(url, timeout=20) as r:
data = r.read().decode("utf-8", "replace")
for needle in ["path_payment_strict_receive", "destination_amount", "source_amount", "asset_code", "asset_issuer"]:
if needle in data:
idx = data.find(needle)
print(data[max(0, idx-200):idx+600].replace("\n", " "))
print()
except Exception as e:
print(f"fetch failed: {e}")
PYRepository: Deen-Bridge/dnb-backend
Length of output: 660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the repository has any fixture or mock that would contradict the review comment.
rg -n "path_payment_strict_receive|destination_asset_code|destination_amount|source_asset_code|source_amount" test tests src | sed -n '1,220p'Repository: Deen-Bridge/dnb-backend
Length of output: 1354
🌐 Web query:
Stellar Horizon operation record path_payment_strict_receive fields asset_code asset_issuer amount destination_amount operation record
💡 Result:
In the Stellar Horizon API, the path_payment_strict_receive operation resource represents a payment where the sender specifies the exact amount of the destination asset to be received [1]. The API response for a path_payment_strict_receive operation includes the following key fields: Destination and Amount Fields: - to: The account ID (or muxed account) receiving the payment [1][2]. - amount: The exact amount of the destination asset received [1][2]. - asset_type: The type of the destination asset (native, credit_alphanum4, or credit_alphanum12) [1][2]. - asset_code: The code of the destination asset (null for native XLM) [1][2]. - asset_issuer: The issuer account ID of the destination asset (null for native XLM) [1][2]. Source and Payment Constraints: - from: The account ID (or muxed account) that sent the payment [1][2]. - source_amount: The actual amount of the source asset deducted from the sender [1][2]. - source_max: The maximum amount of the source asset the sender was willing to pay [1][2]. - source_asset_type, source_asset_code, source_asset_issuer: Details describing the asset used by the sender to fund the payment [1][2]. Path Field: - path: An array of assets that defines the conversion path taken through the order books or liquidity pools to reach the destination asset [1][2]. Each entry in this array contains the asset_type, and if applicable, the asset_code and asset_issuer [1]. Standard Operation Metadata: - id, paging_token, transaction_successful, source_account, type, type_i, created_at, transaction_hash, and _links (containing references to related resources like the transaction and effects) [1].
Citations:
- 1: https://developers.stellar.org/docs/data/apis/horizon/api-reference/resources/operations/object/path-payment-strict-receive
- 2: https://pub.dev/documentation/stellar_flutter_sdk/latest/stellar_flutter_sdk/PathPaymentStrictReceiveOperationResponse-class.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '55,85p' src/services/stellar/stellarService.jsRepository: Deen-Bridge/dnb-backend
Length of output: 1101
Use amount/asset_code/asset_issuer for path-payment operation records
path_payment_strict_receive records still expose the received USDC on amount/asset_code/asset_issuer (with to for the recipient), so the destination_* reads here will drop valid path payments and make them look missing. A small fixture for a real path-payment record would lock this down.
🤖 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 606 - 630, The payment
matching logic in the expectedPayments loop incorrectly reads destination_amount
and destination asset fields for path_payment_strict_receive operations. Update
the paymentOps filter and amount selection to use amount, asset_code, and
asset_issuer for these records while preserving the existing recipient check
through op.to; add or update a fixture covering a real path-payment record if
the surrounding tests support fixtures.
| app.use(express.json()); | ||
| app.use("/api/stellar/payment", paymentRoutes); | ||
|
|
||
| const JWT_SECRET = process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Drop the hardcoded JWT fallback secret.
A hardcoded JWT signing secret in source (even a test fallback) is exactly the kind of value that should never live in code — if the same default leaks into a non-test path it becomes an auth bypass. Prefer failing fast when the env var is absent.
🔐 Suggested change
-const JWT_SECRET = process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024";
+const JWT_SECRET = process.env.JWT_SECRET;
+if (!JWT_SECRET) throw new Error("JWT_SECRET must be set for tests");As per path instructions (**/*.js): flag hardcoded JWT secrets — "all configuration belongs in environment variables documented in .env.example."
📝 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.
| const JWT_SECRET = process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"; | |
| const JWT_SECRET = process.env.JWT_SECRET; | |
| if (!JWT_SECRET) throw new Error("JWT_SECRET must be set for tests"); |
🤖 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/refund.test.js` at line 26, Remove the hardcoded fallback from
JWT_SECRET and require process.env.JWT_SECRET to be present, failing fast when
it is absent. Keep the test’s existing JWT usage unchanged and ensure the
required environment variable is documented in .env.example.
Source: Path instructions
The books/courses/spaces/search/users list and detail endpoints were already wired to the Redis cache layer in a prior change. The two POST /recommended endpoints (books and courses) were left uncached since they take interests via the request body, which the shared GET-only cacheMiddleware can't key off. Cache them explicitly in their controllers, keyed by the sorted interests list, and document every cached endpoint and TTL in docs/caching.md.
The stellarService.js mock in this suite only listed the handful of
exports the original paymentController.js used. Since then
paymentController.js grew (path payments, pre-flight checks,
platform-collect settlement) and other test files
(preflightPayment.test.js, pathPayments.test.js) started importing
the real, unmocked stellarService.js. When both run in the same Jest
worker, the incomplete mock trips a SyntaxError from Jest's ESM
module linker ("does not provide an export named 'USDC'") for
exports this file never even references.
Mirror every named export of stellarService.js in the mock, and add
mocks for preflightPayment/calculateFeeSplit (now called by
initializePayment) and the job queue's enqueue (now called by
submitPayment for receipt generation), which paymentController.js
picked up in the meantime.
…-layer Cache POST-based recommendation endpoints and document cache layer
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 (2)
test/stellarPaymentController.test.js (2)
135-139: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep Mongo session usage observable.
The save mock discards call arguments, so tests remain green if production code loses
{ session }. Record the arguments and assert the session on transaction creation and submit updates to protect payment/access atomicity.🤖 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/stellarPaymentController.test.js` around lines 135 - 139, Update the Transaction.prototype.save mock in the payment controller tests to record each invocation’s arguments alongside saved transactions. Add assertions for the expected session option on both transaction creation and submit-update saves, ensuring production code continues passing the active Mongo session.
183-188: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAssert the required response envelope.
This assertion accepts a top-level
paymentfield without checkingmessageordata, so an inconsistent response shape can remain green. Align the handler and test with the required{ success, message, data }contract.As per path instructions, response shapes must be consistent.
🤖 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/stellarPaymentController.test.js` around lines 183 - 188, Update the handler and the corresponding test around the payment response assertion to enforce the required { success, message, data } envelope. Move the expected payment fields under data, and assert the required message alongside success so responses using a top-level payment field no longer pass.Source: Path instructions
🧹 Nitpick comments (2)
test/stellarPaymentController.test.js (2)
321-337: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify receipt enqueue after confirmation.
The success test never checks that
generateReceiptis queued with the confirmed transaction, idempotency key, and session. A regression could grant access while silently dropping receipts and still pass.🤖 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/stellarPaymentController.test.js` around lines 321 - 337, Update the successful confirmation test to assert that generateReceipt is called with the confirmed transaction, the expected idempotency key, and the existing session. Add this assertion alongside the other post-confirmation expectations while preserving the current access, earnings, and transaction assertions.
146-279: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd controller tests for the preflight
400and retry202paths. The current suite still misses direct coverage for those branches, so a regression there could slip through even though the helper and job tests pass.🤖 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/stellarPaymentController.test.js` around lines 146 - 279, Add tests in the payment controller suite covering the preflight failure branch and retry response branch: assert the controller returns HTTP 400 with the expected error payload when preflight validation fails, and HTTP 202 with the expected retry response when submission requires retry. Exercise these through the existing payment request setup and verify relevant transaction/session side effects for each path.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 @.env.example:
- Around line 29-33: Remove the duplicate EmailJS variable block near the end of
.env.example, retain the existing block, and add EMAILJS_RECEIPT_TEMPLATE_ID
beside its existing EMAILJS_TEMPLATE_ID entry so each key appears only once.
In `@src/controllers/books/recommendedBooksController.js`:
- Around line 14-18: Validate request interests in recommendedBooksController.js
(cacheKey construction) and courseController.js (recommendation cache-key
construction) as bounded, non-empty strings, rejecting invalid input. Normalize
validated interests by sorting and encoding the full array losslessly with
JSON.stringify rather than comma-joining; add regression coverage showing
distinct comma-containing arrays do not share cached results.
In `@test/stellarPaymentController.test.js`:
- Around line 6-29: Define a named jest mock for buildPathPaymentTransaction
alongside the other service mocks and use it in the stellarService mock export.
Add Jest coverage for the sendAsset/sendMax initialization path, configuring the
named mock’s result and asserting its arguments, including persisted path
metadata.
---
Outside diff comments:
In `@test/stellarPaymentController.test.js`:
- Around line 135-139: Update the Transaction.prototype.save mock in the payment
controller tests to record each invocation’s arguments alongside saved
transactions. Add assertions for the expected session option on both transaction
creation and submit-update saves, ensuring production code continues passing the
active Mongo session.
- Around line 183-188: Update the handler and the corresponding test around the
payment response assertion to enforce the required { success, message, data }
envelope. Move the expected payment fields under data, and assert the required
message alongside success so responses using a top-level payment field no longer
pass.
---
Nitpick comments:
In `@test/stellarPaymentController.test.js`:
- Around line 321-337: Update the successful confirmation test to assert that
generateReceipt is called with the confirmed transaction, the expected
idempotency key, and the existing session. Add this assertion alongside the
other post-confirmation expectations while preserving the current access,
earnings, and transaction assertions.
- Around line 146-279: Add tests in the payment controller suite covering the
preflight failure branch and retry response branch: assert the controller
returns HTTP 400 with the expected error payload when preflight validation
fails, and HTTP 202 with the expected retry response when submission requires
retry. Exercise these through the existing payment request setup and verify
relevant transaction/session side effects for each path.
🪄 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: bb31e7b2-2142-4269-8bd2-2cf61089e996
📒 Files selected for processing (7)
.env.exampledocs/caching.mdsrc/config/validateEnv.jssrc/controllers/books/recommendedBooksController.jssrc/controllers/courses/courseController.jssrc/services/stellar/stellarService.jstest/stellarPaymentController.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config/validateEnv.js
- src/services/stellar/stellarService.js
| EMAILJS_PRIVATE_KEY=your_emailjs_private_key | ||
| EMAILJS_PUBLIC_KEY=your_emailjs_public_key | ||
| EMAILJS_SERVICE_ID=your_emailjs_service_id | ||
| EMAILJS_TEMPLATE_ID=your_emailjs_template_id | ||
| EMAILJS_RECEIPT_TEMPLATE_ID=your_emailjs_receipt_template_id |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the duplicate EmailJS entries.
These keys already exist at Lines 22-26. Because dotenv resolves duplicate keys by the later value, editing the first EmailJS block will be silently overridden by these placeholders, causing mail configuration failures. Keep only one EmailJS block and add EMAILJS_RECEIPT_TEMPLATE_ID beside the existing template ID.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 29-29: [DuplicatedKey] The EMAILJS_PRIVATE_KEY key is duplicated
(DuplicatedKey)
[warning] 30-30: [DuplicatedKey] The EMAILJS_PUBLIC_KEY key is duplicated
(DuplicatedKey)
[warning] 31-31: [DuplicatedKey] The EMAILJS_SERVICE_ID key is duplicated
(DuplicatedKey)
[warning] 32-32: [DuplicatedKey] The EMAILJS_TEMPLATE_ID key is duplicated
(DuplicatedKey)
[warning] 33-33: [UnorderedKey] The EMAILJS_RECEIPT_TEMPLATE_ID key should go before the EMAILJS_SERVICE_ID key
(UnorderedKey)
🤖 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 @.env.example around lines 29 - 33, Remove the duplicate EmailJS variable
block near the end of .env.example, retain the existing block, and add
EMAILJS_RECEIPT_TEMPLATE_ID beside its existing EMAILJS_TEMPLATE_ID entry so
each key appears only once.
Sources: Path instructions, Linters/SAST tools
| // This endpoint is POST (interests come in the body), so the shared | ||
| // cacheMiddleware (GET-only) can't key off req.query - cache explicitly here instead. | ||
| const cacheKey = hasInterests | ||
| ? `${CACHE_KEYS.BOOKS}recommended:${[...interests].sort().join(",")}` | ||
| : `${CACHE_KEYS.BOOKS}recommended:popular`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make recommendation cache keys lossless and validate interests. Both controllers use comma-joined request values, so distinct interest arrays containing commas can map to one cached payload.
src/controllers/books/recommendedBooksController.js#L14-L18: validate bounded non-empty string interests and use a lossless normalized encoding such asJSON.stringify([...interests].sort()).src/controllers/courses/courseController.js#L244-L250: apply the same validation and encoding so equivalent cache-key rules hold across recommendation endpoints.
Add regression coverage proving colliding comma-containing arrays do not share results. As per path instructions, flag unvalidated request input.
📍 Affects 2 files
src/controllers/books/recommendedBooksController.js#L14-L18(this comment)src/controllers/courses/courseController.js#L244-L250
🤖 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/books/recommendedBooksController.js` around lines 14 - 18,
Validate request interests in recommendedBooksController.js (cacheKey
construction) and courseController.js (recommendation cache-key construction) as
bounded, non-empty strings, rejecting invalid input. Normalize validated
interests by sorting and encoding the full array losslessly with JSON.stringify
rather than comma-joining; add regression coverage showing distinct
comma-containing arrays do not share cached results.
Source: Path instructions
| const buildPaymentTransaction = jest.fn(); | ||
| const buildSep7Uri = jest.fn(); | ||
| const calculateFeeSplit = jest.fn(); | ||
| const preflightPayment = jest.fn(); | ||
| const submitTransaction = jest.fn(); | ||
| const verifyPaymentOperations = jest.fn(); | ||
| const getExplorerUrl = jest.fn((hash) => `https://stellar.expert/tx/${hash}`); | ||
| const recordSaleEarnings = jest.fn(); | ||
| const enqueue = jest.fn(); | ||
|
|
||
| // Mirrors every named export of stellarService.js, not just the ones this | ||
| // suite exercises: other test files (preflightPayment.test.js, | ||
| // pathPayments.test.js) statically import the real module, and Jest's ESM | ||
| // module linker can resolve those against this mock's identity when both | ||
| // run in the same worker - an incomplete mock then fails with "does not | ||
| // provide an export named X" for exports this file never even references. | ||
| jest.unstable_mockModule("../src/services/stellar/stellarService.js", () => ({ | ||
| buildPaymentTransaction, | ||
| STROOPS_PER_UNIT: 10000000n, | ||
| toStroops: jest.fn(), | ||
| fromStroops: jest.fn(), | ||
| applySlippage: jest.fn(), | ||
| findPaymentPaths: jest.fn(), | ||
| buildPathPaymentTransaction: jest.fn(), | ||
| calculateFeeSplit, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the path-payment mock configurable.
buildPathPaymentTransaction is created inline, so the suite cannot set its result or assert its arguments. Define a named mock and cover the sendAsset/sendMax initialization path, including persisted path metadata.
As per path instructions, new or changed endpoints need Jest coverage.
Proposed mock adjustment
+const buildPathPaymentTransaction = jest.fn();
+
jest.unstable_mockModule("../src/services/stellar/stellarService.js", () => ({
- buildPathPaymentTransaction: jest.fn(),
+ buildPathPaymentTransaction,
}));📝 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.
| const buildPaymentTransaction = jest.fn(); | |
| const buildSep7Uri = jest.fn(); | |
| const calculateFeeSplit = jest.fn(); | |
| const preflightPayment = jest.fn(); | |
| const submitTransaction = jest.fn(); | |
| const verifyPaymentOperations = jest.fn(); | |
| const getExplorerUrl = jest.fn((hash) => `https://stellar.expert/tx/${hash}`); | |
| const recordSaleEarnings = jest.fn(); | |
| const enqueue = jest.fn(); | |
| // Mirrors every named export of stellarService.js, not just the ones this | |
| // suite exercises: other test files (preflightPayment.test.js, | |
| // pathPayments.test.js) statically import the real module, and Jest's ESM | |
| // module linker can resolve those against this mock's identity when both | |
| // run in the same worker - an incomplete mock then fails with "does not | |
| // provide an export named X" for exports this file never even references. | |
| jest.unstable_mockModule("../src/services/stellar/stellarService.js", () => ({ | |
| buildPaymentTransaction, | |
| STROOPS_PER_UNIT: 10000000n, | |
| toStroops: jest.fn(), | |
| fromStroops: jest.fn(), | |
| applySlippage: jest.fn(), | |
| findPaymentPaths: jest.fn(), | |
| buildPathPaymentTransaction: jest.fn(), | |
| calculateFeeSplit, | |
| const buildPaymentTransaction = jest.fn(); | |
| const buildSep7Uri = jest.fn(); | |
| const calculateFeeSplit = jest.fn(); | |
| const preflightPayment = jest.fn(); | |
| const submitTransaction = jest.fn(); | |
| const verifyPaymentOperations = jest.fn(); | |
| const getExplorerUrl = jest.fn((hash) => `https://stellar.expert/tx/${hash}`); | |
| const recordSaleEarnings = jest.fn(); | |
| const enqueue = jest.fn(); | |
| const buildPathPaymentTransaction = jest.fn(); | |
| // Mirrors every named export of stellarService.js, not just the ones this | |
| // suite exercises: other test files (preflightPayment.test.js, | |
| // pathPayments.test.js) statically import the real module, and Jest's ESM | |
| // module linker can resolve those against this mock's identity when both | |
| // run in the same worker - an incomplete mock then fails with "does not | |
| // provide an export named X" for exports this file never even references. | |
| jest.unstable_mockModule("../src/services/stellar/stellarService.js", () => ({ | |
| STROOPS_PER_UNIT: 10000000n, | |
| toStroops: jest.fn(), | |
| fromStroops: jest.fn(), | |
| applySlippage: jest.fn(), | |
| findPaymentPaths: jest.fn(), | |
| buildPathPaymentTransaction, | |
| calculateFeeSplit, |
🤖 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/stellarPaymentController.test.js` around lines 6 - 29, Define a named
jest mock for buildPathPaymentTransaction alongside the other service mocks and
use it in the stellarService mock export. Add Jest coverage for the
sendAsset/sendMax initialization path, configuring the named mock’s result and
asserting its arguments, including persisted path metadata.
Source: Path instructions
feat: media upload hardening with limits, magic bytes, and signed del…
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/controllers/userController.js (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood addition — but no test coverage for this new validation path.
This mirrors the book-upload magic-byte check nicely. However, none of the tests in this PR (only
test/bookUpload.test.jsis included) exercise the avatar-upload rejection path inupdateUser. Given this changes behavior on an existing, presumably tested endpoint, it'd be worth adding a case (mismatched magic bytes on avatar upload → 400).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/userController.js` around lines 16 - 20, Add Jest coverage for the new magic-byte rejection path in updateUser: submit an avatar upload whose content does not match the accepted image types, assert a 400 response, and verify the validation error response. Keep existing valid avatar-upload behavior covered unchanged.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/books/bookController.js`:
- Around line 210-215: Update the private_download_url call in the book download
handler to retain the "raw" format and add resource_type: "raw" and type:
"authenticated" to its options, while preserving the existing expiration
timestamp and redirect behavior.
In `@src/controllers/uploadController.js`:
- Around line 4-26: The generateSignature endpoint signs only the timestamp and
folder, allowing direct uploads to bypass the PR’s file restrictions. Update
generateSignature to include Cloudinary-enforced upload constraints such as
allowed formats, resource type, and the configured upload preset with a maximum
file size, ensuring the signed payload matches the limits used by
server-mediated uploads.
In `@src/middlewares/upload.js`:
- Around line 6-29: Update the rejection branches in imageFilter and bookFilter
to pass the actual upload field name as the second MulterError argument, then
assign the friendly validation text to the error’s message before invoking cb.
Preserve the existing acceptance rules and distinct messages for image and book
uploads.
In `@src/utils/fileValidation.js`:
- Around line 16-19: Remove the NODE_ENV === "test" branch from
validateMagicBytes so every environment uses the real fileTypeFromBuffer-based
validation. Move test-specific behavior into the tests by mocking or spying on
the file-type detector, preserving production validation without any
substring-based bypass.
---
Nitpick comments:
In `@src/controllers/userController.js`:
- Around line 16-20: Add Jest coverage for the new magic-byte rejection path in
updateUser: submit an avatar upload whose content does not match the accepted
image types, assert a 400 response, and verify the validation error response.
Keep existing valid avatar-upload behavior covered unchanged.
🪄 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: 43b8f50e-7b85-4d59-9f9a-ba58133ce60e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
app.jspackage.jsonsrc/controllers/books/bookController.jssrc/controllers/uploadController.jssrc/controllers/userController.jssrc/middlewares/errorHandler.jssrc/middlewares/upload.jssrc/models/Book.jssrc/routes/books/bookRoutes.jssrc/routes/uploadRoutes.jssrc/routes/userRoutes.jssrc/utils/fileValidation.jstest/bookUpload.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- app.js
| if (book.filePublicId) { | ||
| const signedUrl = cloudinary.utils.private_download_url(book.filePublicId, "raw", { | ||
| expires_at: Math.floor(Date.now() / 1000) + 3600, // 1 hour | ||
| }); | ||
| return res.redirect(302, signedUrl); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' src/controllers/books/bookController.js | nl -ba | sed -n '1,260p'Repository: Deen-Bridge/dnb-backend
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant controller with line numbers.
sed -n '1,260p' src/controllers/books/bookController.js | nl -ba | sed -n '1,260p'Repository: Deen-Bridge/dnb-backend
Length of output: 201
🌐 Web query:
Cloudinary Node.js SDK private_download_url signature resource_type type options authenticated raw download docs
💡 Result:
The Cloudinary Node.js SDK provides the private_download_url method to generate time-limited, signed URLs for accessing assets stored with private or authenticated delivery types [1][2]. These URLs are intended for secure, temporary access rather than permanent CDN-based delivery, as they are not cached and incur higher bandwidth costs [2]. Syntax The method is available under the utils namespace [1]: cloudinary.v2.utils.private_download_url(public_id, format, options); Parameters - public_id (String, required): The unique identifier of the asset [2]. - format (String, required): The file extension of the asset (e.g., 'jpg', 'png') [2]. - options (Object, optional): An object containing configuration or request parameters, including: - resource_type: The type of resource, such as 'image', 'video', or 'raw'. Defaults to 'image' [2]. - type: The delivery type of the asset, such as 'private' or 'authenticated'. Defaults to 'private' [2]. - expires_at: A UNIX timestamp (in seconds) defining when the URL should expire. Defaults to one hour from generation [2]. - attachment: If set to true, the URL triggers a download (Content-Disposition: attachment) rather than displaying the asset in the browser [2]. Important Considerations - Signature: The private_download_url method automatically calculates the required signature using your configured api_secret [1][3]. Ensure your Cloudinary account is configured with your cloud_name, api_key, and api_secret [4]. - Resource Compatibility: This method is specifically for assets explicitly uploaded or stored with the private or authenticated delivery types [1][2]. It will not function as intended for assets stored with the default upload type [5]. - Performance: Because these URLs bypass CDN caching to maintain access control, they should not be embedded for standard website delivery [2].
Citations:
- 1: https://cloudinary.com/documentation/image_upload_api_reference
- 2: https://cloudinary.com/documentation/control_access_to_media
- 3: https://cloudinary.com/documentation/delivery_url_signatures
- 4: https://cloudinary.com/documentation/node_integration
- 5: Photos cannot be downloaded with the function of private_download_url cloudinary/cloudinary_npm#37
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' src/controllers/books/bookController.js | nl -ba | sed -n '1,260p'Repository: Deen-Bridge/dnb-backend
Length of output: 201
🏁 Script executed:
sed -n '1,260p' src/controllers/books/bookController.js | cat -n | sed -n '1,260p'Repository: Deen-Bridge/dnb-backend
Length of output: 9125
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/controllers/books/bookController.js')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 180 <= i <= 230:
print(f"{i:4d}: {line}")
PYRepository: Deen-Bridge/dnb-backend
Length of output: 1947
🌐 Web query:
Cloudinary private_download_url raw authenticated example resource_type raw type authenticated
💡 Result:
To generate a signed, time-limited download URL for a raw asset with authenticated delivery in Cloudinary, use the private_download_url method provided by your backend SDK [1][2]. You must explicitly specify the resource_type as 'raw' in the options to ensure the URL is generated correctly for non-image/video files [3][2]. Example Usage (Ruby SDK): Cloudinary::Utils.private_download_url('public_id', 'format', { resource_type: 'raw', type: 'authenticated', expires_at: Time.now.to_i + 3600 # URL expires in 1 hour }) Key Considerations: 1. Resource Type: For raw files (e.g., CSVs, fonts, PDFs), always set resource_type to 'raw' [3][2]. If omitted, the SDK defaults to 'image', which will result in invalid URL generation for raw assets [2]. 2. Delivery Type: Ensure the asset was uploaded with type: 'authenticated' or 'private' [1][4]. The private_download_url method is specifically designed for these restricted delivery types [1]. 3. Security: The generated URL is signed and time-limited, preventing unauthorized permanent access [1][2]. Do not embed these URLs directly in public-facing web pages if you want to avoid unnecessary bandwidth costs, as they are intended for direct, secure access [2]. 4. SDKs: This method is available in all official Cloudinary backend SDKs (e.g., Node.js, Python, PHP, Java, Go, C#) [1][2]. Ensure your environment is configured with the correct API secret, as it is required to sign the URL [1][5][6].
Citations:
- 1: https://cloudinary.com/documentation/image_upload_api_reference
- 2: https://cloudinary.com/documentation/control_access_to_media
- 3: Downloading a private, raw file? cloudinary/cloudinary_gem#218
- 4: https://cloudinary.com/documentation/ts_how_to_view_resource_type_and_delivery_type
- 5: https://files-sdk.dev/adapters/cloudinary
- 6: https://cloudinary.com/documentation/delivery_url_signatures
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/controllers/books/bookController.js')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 120:
print(f"{i:4d}: {line}")
PYRepository: Deen-Bridge/dnb-backend
Length of output: 4931
private_download_url needs the raw/authenticated options
cloudinary.utils.private_download_url takes (public_id, format, options). Here "raw" is being passed as the format, but this download also needs resource_type: "raw" and type: "authenticated" in options to match the upload. Without that, Cloudinary signs the wrong delivery path and authenticated book downloads can fail.
🤖 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/books/bookController.js` around lines 210 - 215, Update the
private_download_url call in the book download handler to retain the "raw"
format and add resource_type: "raw" and type: "authenticated" to its options,
while preserving the existing expiration timestamp and redirect behavior.
| export const generateSignature = async (req, res) => { | ||
| try { | ||
| const timestamp = Math.round(new Date().getTime() / 1000); | ||
| const signature = cloudinary.utils.api_sign_request( | ||
| { | ||
| timestamp, | ||
| folder: "direct-uploads", | ||
| }, | ||
| process.env.CLOUDINARY_API_SECRET | ||
| ); | ||
|
|
||
| res.status(200).json({ | ||
| success: true, | ||
| timestamp, | ||
| signature, | ||
| cloudName: process.env.CLOUDINARY_CLOUD_NAME, | ||
| apiKey: process.env.CLOUDINARY_API_KEY, | ||
| }); | ||
| } catch (error) { | ||
| logger.error("Error generating Cloudinary signature:", error); | ||
| res.status(500).json({ success: false, message: "Failed to generate upload signature" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Signed direct-upload bypasses this PR's own upload hardening.
This endpoint lets any authenticated user upload directly to Cloudinary using a signature for { timestamp, folder: "direct-uploads" } only. None of this PR's hardening — Multer size/MIME limits, validateMagicBytes — applies to files pushed via this signed direct-upload path, since they never traverse the app's Multer middleware. A malicious or buggy client can push arbitrarily large or mistyped files straight into the Cloudinary account.
Consider constraining the signed payload itself (e.g., allowed_formats, resource_type, and a Cloudinary upload preset enforcing max file size) so Cloudinary rejects out-of-policy uploads at the edge, matching the guarantees the rest of this PR establishes for server-mediated uploads.
🤖 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/uploadController.js` around lines 4 - 26, The
generateSignature endpoint signs only the timestamp and folder, allowing direct
uploads to bypass the PR’s file restrictions. Update generateSignature to
include Cloudinary-enforced upload constraints such as allowed formats, resource
type, and the configured upload preset with a maximum file size, ensuring the
signed payload matches the limits used by server-mediated uploads.
| const imageFilter = (req, file, cb) => { | ||
| if (file.mimetype.startsWith("image/")) { | ||
| cb(null, true); | ||
| } else { | ||
| cb(new multer.MulterError("LIMIT_UNEXPECTED_FILE", "Not an image! Please upload only images.")); | ||
| } | ||
| }; | ||
|
|
||
| const bookFilter = (req, file, cb) => { | ||
| if ( | ||
| file.fieldname === "thumbnail" && | ||
| file.mimetype.startsWith("image/") | ||
| ) { | ||
| cb(null, true); | ||
| } else if ( | ||
| file.fieldname === "file" && | ||
| (file.mimetype === "application/pdf" || | ||
| file.mimetype === "application/epub+zip") | ||
| ) { | ||
| cb(null, true); | ||
| } else { | ||
| cb(new multer.MulterError("LIMIT_UNEXPECTED_FILE", "Invalid file format. Thumbnail must be an image, file must be PDF/EPUB.")); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== upload middleware ==\n'
sed -n '1,220p' src/middlewares/upload.js
printf '\n== error handler ==\n'
sed -n '1,220p' src/middlewares/errorHandler.js
printf '\n== upload tests search ==\n'
rg -n "LIMIT_UNEXPECTED_FILE|fileFilter|Unexpected field|Not an image|Invalid file format|upload" test src --glob '!**/node_modules/**'Repository: Deen-Bridge/dnb-backend
Length of output: 10062
Set the real field name in MulterError
The second MulterError argument becomes .field, not .message, so these friendly strings are discarded and users still get Multer’s generic “Unexpected field” response. Pass the actual field name there and set the custom message on the error before calling cb(...).
🤖 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/upload.js` around lines 6 - 29, Update the rejection branches
in imageFilter and bookFilter to pass the actual upload field name as the second
MulterError argument, then assign the friendly validation text to the error’s
message before invoking cb. Preserve the existing acceptance rules and distinct
messages for image and book uploads.
…lters, and pagination
…-known/stellar.toml
feat(stellar): publish SEP-1 stellar.toml metadata endpoint.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 6-17: Update the currency status in the stellar TOML generation
flow to emit "test" when the configured network is testnet and "live" otherwise,
using the existing network configuration symbols such as networkPassphrase and
USDC_ISSUER. Add or update coverage for both testnet and non-testnet modes,
ensuring wallets receive the appropriate status.
- Around line 33-46: Replace the regex-only validation in stellarTomlService.js
around signingKey with the Stellar SDK StrKey public-key validator, while
retaining trimming and excluding invalid or secret values before emitting
SIGNING_KEY. In test/stellarToml.test.js lines 123-138, add a checksum-invalid
G... fixture and assert it is not emitted.
🪄 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: e099f0b2-edd6-4d84-aa37-7b54aa4d50cc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.env.exampleQUICK_START.mdREADME.mdapp.jspackage.jsonsrc/config/validateEnv.jssrc/routes/wellKnownRoutes.jssrc/services/stellar/stellarTomlService.jstest/stellarToml.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- app.js
| const CURRENCIES = [ | ||
| { | ||
| code: "USDC", | ||
| issuer: USDC_ISSUER, | ||
| status: "live", | ||
| is_asset_anchored: true, | ||
| anchor_asset_type: "fiat", | ||
| anchor_asset: "USD", | ||
| desc: "USD Coin — a regulated stablecoin pegged 1:1 to the US dollar", | ||
| display_decimals: 7, | ||
| name: "USD Coin", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not advertise testnet USDC as live.
networkPassphrase and USDC_ISSUER are network-configured, but status is always "live". Emit "test" for testnet and cover both modes so wallets do not treat a test asset as production-ready. Stellar’s testnet example uses status = "test". (developers.stellar.org)
🤖 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/stellarTomlService.js` around lines 6 - 17, Update the
currency status in the stellar TOML generation flow to emit "test" when the
configured network is testnet and "live" otherwise, using the existing network
configuration symbols such as networkPassphrase and USDC_ISSUER. Add or update
coverage for both testnet and non-testnet modes, ensuring wallets receive the
appropriate status.
| const platformKey = process.env.STELLAR_PLATFORM_PUBLIC_KEY; | ||
| if (platformKey && platformKey.trim() !== "") { | ||
| lines.push(`ACCOUNTS = ["${platformKey.trim()}"]`); | ||
| lines.push(``); | ||
| } | ||
|
|
||
| // ── SEP endpoint hooks ── | ||
| // Emit SIGNING_KEY only if it is a valid public key starting with G; never emit secret seeds or invalid values | ||
| const signingKey = process.env.SIGNING_KEY; | ||
| const isValidPublicKey = | ||
| typeof signingKey === "string" && /^G[A-Z2-7]{55}$/.test(signingKey.trim()); | ||
|
|
||
| if (isValidPublicKey) { | ||
| lines.push(`SIGNING_KEY = "${signingKey.trim()}"`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use StrKey validation for Stellar public keys.
Regex shape checks will still accept checksum-invalid G... values, so stellar.toml can emit keys that are not real Stellar public keys. Use the SDK’s public-key validator here, and add a checksum-invalid G... fixture in the TOML tests so this stays covered.
src/services/stellar/stellarTomlService.js#L33-L46test/stellarToml.test.js#L123-L138
📍 Affects 2 files
src/services/stellar/stellarTomlService.js#L33-L46(this comment)test/stellarToml.test.js#L123-L138
🤖 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/stellarTomlService.js` around lines 33 - 46, Replace the
regex-only validation in stellarTomlService.js around signingKey with the
Stellar SDK StrKey public-key validator, while retaining trimming and excluding
invalid or secret values before emitting SIGNING_KEY. In
test/stellarToml.test.js lines 123-138, add a checksum-invalid G... fixture and
assert it is not emitted.
- Set secure and sameSite based on NODE_ENV - Fixes 401 loop on localhost (http) where secure:true blocks cookie
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/authController.js`:
- Around line 92-96: Update the environment validation used by authController
cookie operations so missing or unsupported NODE_ENV values fail closed instead
of selecting the non-production settings. Ensure the validated security
configuration is reused consistently across all three refresh-cookie operations,
including the paths around the visible isProd, secure, and sameSite options.
- Around line 92-96: Extend the Jest auth coverage for the cookie options used
by registration, refresh rotation, and logout clearing, including the `Secure`
and `SameSite` values. Add production-environment cases expecting secure cookies
with `SameSite: "None"` and test/development cases expecting non-secure cookies
with `SameSite: "Lax"`, while preserving existing `HttpOnly` and `Path`
assertions.
🪄 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: f0d3d192-22a8-42e2-ac58-1af60f911db0
📒 Files selected for processing (1)
src/controllers/authController.js
| const isProd = process.env.NODE_ENV === "production"; | ||
| res.cookie("refreshToken", rawRefreshToken, { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: "None", | ||
| secure: isProd, | ||
| sameSite: isProd ? "None" : "Lax", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when NODE_ENV is invalid or missing.
Because validateEnv.js only warns about unsupported values, NODE_ENV=prod or an unset value enters the non-production branch and emits a refresh cookie without Secure. A misconfigured production deployment could therefore expose refresh tokens over HTTP. Reject invalid environments during startup, or derive these options from a separately validated security configuration, and reuse it across all three cookie operations.
Also applies to: 350-354, 458-462
🤖 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/authController.js` around lines 92 - 96, Update the
environment validation used by authController cookie operations so missing or
unsupported NODE_ENV values fail closed instead of selecting the non-production
settings. Ensure the validated security configuration is reused consistently
across all three refresh-cookie operations, including the paths around the
visible isProd, secure, and sameSite options.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add coverage for the new cookie attributes.
The existing auth tests verify HttpOnly and Path, but not the changed Secure/SameSite behavior for registration, refresh rotation, and logout clearing. Add Jest cases for production and test/development environments, including the clearing response.
As per path instructions, changed endpoints in src/**/*.js need Jest coverage.
Also applies to: 350-354, 458-462
🤖 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/authController.js` around lines 92 - 96, Extend the Jest auth
coverage for the cookie options used by registration, refresh rotation, and
logout clearing, including the `Secure` and `SameSite` values. Add
production-environment cases expecting secure cookies with `SameSite: "None"`
and test/development cases expecting non-secure cookies with `SameSite: "Lax"`,
while preserving existing `HttpOnly` and `Path` assertions.
Source: Path instructions
…agination limit & schema index fix
…course, and user controllers
…alation, and content gating
Feature/roles and authorization
…resilience, and enhance security and validation middleware.
…tion tests for user updates
…eat/64-full-text-search
…variables and increase test timeout
Feat/64 full text search
7 participants
Summary by CodeRabbit
New Features
/admin/jobsdashboard and dead-job view./.well-known/stellar.toml.Security & Performance
Documentation