feat(audit): tamper-evident audit log for security/financial actions with admin query API - #85
Conversation
- Add admin-aware ownership middleware to PUT /update/:id and DELETE /:id routes - Handle duplicate email errors with clean 409 response instead of raw Mongo 11000 - Trim getUser response to public fields only for non-self requests - Add tests for cross-account 403, self-edit 200, admin override, duplicate email 409, and public field filtering Fixes Deen-Bridge#13
- Add IngestionCursor and UnreconciledPayment models for cursor persistence and unreconciled payment tracking - Implement reconciliationService with hash lookup, memo/source fallback, and shared grantItemAccess - Build paymentIngestionWorker (polling loop, cursor management, page iteration, graceful shutdown) - Wire worker into server.js behind INGESTION_WORKER_ENABLED flag - Extract grantItemAccess from paymentController into reconciliationService for reuse - Add admin reconciliation status endpoint GET /api/stellar/payment/reconciliation/status - Add worker:ingest npm script and required env vars (INGESTION_WORKER_ENABLED, INGESTION_POLL_INTERVAL_MS) - Add 13 Jest tests covering hash match, memo fallback, donation creation, unreconciled, cursor status Closes Deen-Bridge#26
…-ingestion-worker feat: add Horizon payment ingestion worker with automated reconciliation
…le-authorization fix: prevent IDOR in user profile endpoints
…tadata() unit tests (strips password/token/signedXdr/otp)- Audit row written on register (success + failure)- Admin access: 401 unauthenticated, 403 non-admin, 200 admin- Filter by action, status (incl. 400 for invalid), actor ObjectId validation- Pagination: page/limit respected, limit capped at 100- Append-only: updateOne/updateMany/deleteOne/deleteMany all throw- No mutating verbs: POST/DELETE 405
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an append-only MongoDB audit log, non-blocking redacted audit recording, authentication and Stellar instrumentation, an admin-only read API, user access protections, and nested course sections and lessons. ChangesAudit logging
User access controls
Course curriculum
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/controllers/stellar/walletController.js (1)
116-135: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the read-then-update into a single round trip.
findByIdAndUpdatereturns the pre-update document by default (no{ new: true }needed), so the separatefindByIdjust to capturepreviousPublicKeyis an extra DB round trip that can be avoided.♻️ Proposed simplification
- // Capture the wallet key before unsetting it (for the audit row) - const currentUser = await User.findById(userId).select("stellarWallet"); - const previousPublicKey = currentUser?.stellarWallet?.publicKey ?? null; - - await User.findByIdAndUpdate(userId, { - $unset: { stellarWallet: 1 }, - }); + // findByIdAndUpdate returns the pre-update document by default, + // so we can capture the previous key in a single round trip. + const previousUser = await User.findByIdAndUpdate( + userId, + { $unset: { stellarWallet: 1 } } + ).select("stellarWallet"); + const previousPublicKey = previousUser?.stellarWallet?.publicKey ?? null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/stellar/walletController.js` around lines 116 - 135, In the wallet disconnect flow, replace the separate User.findById read and subsequent User.findByIdAndUpdate call with a single findByIdAndUpdate operation, retaining the pre-update document as the returned value. Derive previousPublicKey from that returned document before recording the existing audit entry, and preserve the $unset behavior.src/models/AuditLog.js (1)
133-166: 🩺 Stability & Availability | 🔵 TrivialSolid append-only enforcement — one defense-in-depth note.
The
save/query-middleware hooks correctly blockupdateOne,updateMany,findOneAndUpdate,findByIdAndUpdate,replaceOne,deleteOne,deleteMany,findOneAndDelete, andfindByIdAndDelete— and this is backed by passing tests. One thing to keep in mind:Model.bulkWrite()and any direct driver access (mongoose.connection.collection('auditlogs')) bypass Mongoose schema middleware entirely, so they're not covered by these guards. Since nothing in this PR currently callsbulkWriteonAuditLog, this isn't a live issue, but if the audit trail needs to be tamper-evident against future code changes (not just current call sites), consider a DB-level backstop too — e.g., a restricted Mongo role for the app user that deniesupdate/deleteon theauditlogscollection.🤖 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/AuditLog.js` around lines 133 - 166, Consider adding a database-level protection for the AuditLog collection because the middleware in auditLogSchema does not cover Model.bulkWrite() or direct MongoDB driver access. Configure the application’s MongoDB role or equivalent deployment-level permissions to deny update and delete operations on auditlogs while preserving insert access, without changing the existing middleware hooks.test/auditLog.test.js (1)
60-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock's
.then()branch returns unpaginated data — dead/inconsistent path, but harmless today.The
AuditLog.findmock'schain._docsis correctly sliced by.skip()/.limit(), butchain.thenresolves with the original unslicedfilteredarray instead ofthis._docs. Since the real route always calls.populate().lean()rather thanawait-ing the query directly, this branch is never exercised — but it's a latent trap if a future caller (or refactor) awaits the query object directly instead of chaining.lean().♻️ Optional fix for consistency
lean: async function () { return this._docs; }, - then: (resolve) => resolve(filtered), + then: (resolve) => resolve(this._docs),🤖 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/auditLog.test.js` around lines 60 - 83, Update the chain.then implementation in the AuditLog.find mock to resolve with the chain’s current _docs collection rather than the original filtered array, keeping direct awaits consistent with skip and limit pagination behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/stellar/paymentController.js`:
- Around line 654-667: Update the sibling catch (stellarError) branch in the
payment submission flow to call recordAudit with PAYMENT_SUBMIT_FAILED before
returning the 400 response. Populate the audit entry with the failed transaction
context, including buyerId, req, transactionId, status "failure", and a
failureReason derived from the Stellar submission error, matching the existing
verification-failure audit coverage.
In `@src/routes/admin/auditRoutes.js`:
- Around line 107-116: Update the audit response in the route handler around the
shown res.status(200).json call to use the project-wide { success, message, data
} envelope: add the appropriate message and place logs and pagination inside
data. Update test/auditLog.test.js assertions to read these values from
res.body.data while preserving the existing response contents.
In `@test/auditLog.test.js`:
- Around line 251-301: Prevent the register tests in “Audit row written on
register” from scheduling real background work that survives Jest teardown. In
the suite’s setup, mock or stub the module used by the registration flow to
dispatch registerJob/sendOtpEmail, matching the existing AuditLog/User/Session
mocks; alternatively, use fake timers and explicitly drain all scheduled jobs
before each test completes while preserving the audit-write assertions.
---
Nitpick comments:
In `@src/controllers/stellar/walletController.js`:
- Around line 116-135: In the wallet disconnect flow, replace the separate
User.findById read and subsequent User.findByIdAndUpdate call with a single
findByIdAndUpdate operation, retaining the pre-update document as the returned
value. Derive previousPublicKey from that returned document before recording the
existing audit entry, and preserve the $unset behavior.
In `@src/models/AuditLog.js`:
- Around line 133-166: Consider adding a database-level protection for the
AuditLog collection because the middleware in auditLogSchema does not cover
Model.bulkWrite() or direct MongoDB driver access. Configure the application’s
MongoDB role or equivalent deployment-level permissions to deny update and
delete operations on auditlogs while preserving insert access, without changing
the existing middleware hooks.
In `@test/auditLog.test.js`:
- Around line 60-83: Update the chain.then implementation in the AuditLog.find
mock to resolve with the chain’s current _docs collection rather than the
original filtered array, keeping direct awaits consistent with skip and limit
pagination behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f5c73230-63c5-4bcd-ba5d-6a06e27df218
📒 Files selected for processing (8)
app.jssrc/controllers/authController.jssrc/controllers/stellar/paymentController.jssrc/controllers/stellar/walletController.jssrc/models/AuditLog.jssrc/routes/admin/auditRoutes.jssrc/services/audit/auditService.jstest/auditLog.test.js
| recordAudit({ | ||
| action: AUDIT_ACTIONS.PAYMENT_SUBMIT_FAILED, | ||
| actor: buyerId, | ||
| req, | ||
| targetType: "Transaction", | ||
| targetId: transactionId, | ||
| status: "failure", | ||
| metadata: { | ||
| transactionId, | ||
| stellarTxHash: result.hash, | ||
| failureReason: `On-chain verification failed: ${verification.reason}`, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Missing audit coverage: Stellar submission errors never reach the audit log.
PAYMENT_SUBMIT_FAILED is correctly recorded here for on-chain verification failures, but the sibling catch (stellarError) branch a few lines above (around line 577-592) — which also marks the transaction "failed" with a failureReason and returns a 400 — has no recordAudit call at all. Since Stellar submission errors (bad signature, insufficient funds, network rejection, etc.) are a distinct and common payment failure mode, this leaves a real gap in the financial audit trail that issue #66 asks for.
🛡️ Proposed fix: audit the Stellar submission failure branch too
} catch (stellarError) {
// Handle Stellar submission errors
transaction.status = "failed";
transaction.failureReason = stellarError.message;
await transaction.save({ session });
await session.commitTransaction();
paymentsFailed.inc({ type: "purchase", reason: "stellar_error" });
logger.error(`Transaction ${transactionId} failed:`, stellarError);
+ recordAudit({
+ action: AUDIT_ACTIONS.PAYMENT_SUBMIT_FAILED,
+ actor: buyerId,
+ req,
+ targetType: "Transaction",
+ targetId: transactionId,
+ status: "failure",
+ metadata: {
+ transactionId,
+ failureReason: stellarError.message,
+ },
+ });
+
return res.status(400).json({
success: false,
message: "Transaction failed on Stellar network",
error: stellarError.message,
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/controllers/stellar/paymentController.js` around lines 654 - 667, Update
the sibling catch (stellarError) branch in the payment submission flow to call
recordAudit with PAYMENT_SUBMIT_FAILED before returning the 400 response.
Populate the audit entry with the failed transaction context, including buyerId,
req, transactionId, status "failure", and a failureReason derived from the
Stellar submission error, matching the existing verification-failure audit
coverage.
| res.status(200).json({ | ||
| success: true, | ||
| logs, | ||
| pagination: { | ||
| page: pageNum, | ||
| limit: limitNum, | ||
| total, | ||
| pages: Math.ceil(total / limitNum), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Response shape doesn't match the project's { success, message, data } convention.
This endpoint responds with { success, logs, pagination } — no message, and logs/pagination in place of a data wrapper. Path instructions call for a consistent envelope across src/**/*.js endpoints; deviating here makes client-side response handling inconsistent across the API surface.
♻️ Proposed fix
res.status(200).json({
success: true,
- logs,
- pagination: {
- page: pageNum,
- limit: limitNum,
- total,
- pages: Math.ceil(total / limitNum),
+ message: "Audit logs retrieved",
+ data: {
+ logs,
+ pagination: {
+ page: pageNum,
+ limit: limitNum,
+ total,
+ pages: Math.ceil(total / limitNum),
+ },
},
});Note: test/auditLog.test.js currently asserts against res.body.logs / res.body.pagination directly, so those assertions would need updating alongside this change.
📝 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.
| res.status(200).json({ | |
| success: true, | |
| logs, | |
| pagination: { | |
| page: pageNum, | |
| limit: limitNum, | |
| total, | |
| pages: Math.ceil(total / limitNum), | |
| }, | |
| }); | |
| res.status(200).json({ | |
| success: true, | |
| message: "Audit logs retrieved", | |
| data: { | |
| logs, | |
| pagination: { | |
| page: pageNum, | |
| limit: limitNum, | |
| total, | |
| pages: Math.ceil(total / limitNum), | |
| }, | |
| }, | |
| }); |
🤖 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/admin/auditRoutes.js` around lines 107 - 116, Update the audit
response in the route handler around the shown res.status(200).json call to use
the project-wide { success, message, data } envelope: add the appropriate
message and place logs and pagination inside data. Update test/auditLog.test.js
assertions to read these values from res.body.data while preserving the existing
response contents.
Source: Path instructions
| describe("Audit row written on register", () => { | ||
| it("writes a success row with correct action, status, and safe metadata", async () => { | ||
| const res = await request(app).post("/api/auth/register").send({ | ||
| name: "Alice", | ||
| email: "alice@example.com", | ||
| password: "password123", | ||
| role: "student", | ||
| }); | ||
|
|
||
| expect(res.statusCode).toBe(201); | ||
|
|
||
| // Give the fire-and-forget microtask a tick to complete | ||
| await new Promise((resolve) => setImmediate(resolve)); | ||
|
|
||
| const row = auditStore.find((r) => r.action === AUDIT_ACTIONS.AUTH_REGISTER_SUCCESS); | ||
| expect(row).toBeDefined(); | ||
| expect(row.status).toBe("success"); | ||
| expect(row.targetType).toBe("User"); | ||
|
|
||
| // Sensitive field must NOT appear in stored metadata | ||
| expect(row.metadata).not.toHaveProperty("password"); | ||
| expect(row.metadata?.email).toBe("alice@example.com"); | ||
| }); | ||
|
|
||
| it("writes a failure row when email already exists", async () => { | ||
| // First register | ||
| await request(app).post("/api/auth/register").send({ | ||
| name: "Bob", | ||
| email: "bob@example.com", | ||
| password: "password123", | ||
| }); | ||
| await new Promise((resolve) => setImmediate(resolve)); | ||
|
|
||
| // Second attempt with same email | ||
| const res = await request(app).post("/api/auth/register").send({ | ||
| name: "Bob Again", | ||
| email: "bob@example.com", | ||
| password: "password456", | ||
| }); | ||
|
|
||
| expect(res.statusCode).toBe(400); | ||
| await new Promise((resolve) => setImmediate(resolve)); | ||
|
|
||
| const row = auditStore.find((r) => r.action === AUDIT_ACTIONS.AUTH_REGISTER_FAILURE); | ||
| expect(row).toBeDefined(); | ||
| expect(row.status).toBe("failure"); | ||
| expect(row.actor).toBeNull(); | ||
| expect(row.metadata?.reason).toBe("email_already_exists"); | ||
| expect(row.metadata).not.toHaveProperty("password"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
CI is failing here: dangling async job leaks past Jest teardown.
The pipeline logs report ReferenceError: You are trying to import a file after the Jest environment has been torn down, originating from this file, with the stack pointing at registerJob('sendOtpEmail'...) → src/jobs/handlers.js:27 calling User.findById(userId).select('email'). These describe("Audit row written on register", ...) tests hit the real POST /api/auth/register endpoint, which appears to schedule a background job (OTP email) that isn't mocked or awaited anywhere in this suite. The setImmediate flush at lines 263/282/292 only lets the fire-and-forget audit write settle — it does nothing for a job queued on a separate timer/queue, so that job fires after the suite (and its module registry) has already been torn down, which is exactly why CI is red.
Please mock/stub whatever module dispatches registerJob/sendOtpEmail in beforeAll (the same way AuditLog/User/Session are mocked here), or use jest.useFakeTimers() + explicit jest.runAllTimers()/flush before each register test, so no job outlives the test run.
#!/bin/bash
# Locate the job scheduler invoked from the auth register flow to identify what needs mocking.
rg -n "registerJob|sendOtpEmail" src -g '!node_modules'
sed -n '1,40p' src/jobs/handlers.js 2>/dev/null🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 262-262: Avoid using the initial state variable in setState
Context: setImmediate(resolve)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 281-281: Avoid using the initial state variable in setState
Context: setImmediate(resolve)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 291-291: Avoid using the initial state variable in setState
Context: setImmediate(resolve)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/auditLog.test.js` around lines 251 - 301, Prevent the register tests in
“Audit row written on register” from scheduling real background work that
survives Jest teardown. In the suite’s setup, mock or stub the module used by
the registration flow to dispatch registerJob/sendOtpEmail, matching the
existing AuditLog/User/Session mocks; alternatively, use fake timers and
explicitly drain all scheduled jobs before each test completes while preserving
the audit-write assertions.
Source: Pipeline failures
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/controllers/stellar/paymentController.js (2)
736-799: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the duplicated entitlement and receipt flow.
grantItemAccessalready appends the purchase, updates stats, enrolls course users, and saves the buyer. Lines 753-779 repeat those writes, so confirmed purchases receive duplicate purchase records and double-counted stats. Also, the unguarded enqueue at Line 743 can abort the local transaction after the on-chain payment has succeeded. Keep onegrantItemAccesscall and one best-effort receipt enqueue (the guarded block).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/stellar/paymentController.js` around lines 736 - 799, Remove the duplicated buyer update block around transaction.itemType, including the manual purchase, stats, course enrollment, and buyer.save operations, since grantItemAccess already performs them. Remove the earlier unguarded generateReceipt enqueue, retaining only the guarded best-effort enqueue after grantItemAccess so receipt failures do not roll back the confirmed payment.
65-65: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRequire and document the platform collection wallet setting.
Line 65 falls back to
PLATFORM_WALLET_PUBLIC_KEY, while the provided.env.exampledoes not documentPLATFORM_WALLET_PUBLIC_KEY. A payment destination must not silently come from repository configuration; require the environment value and document it.As per path instructions, “all configuration belongs in environment variables documented in .env.example.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/stellar/paymentController.js` at line 65, Update the platform wallet configuration used by the payment controller to require process.env.PLATFORM_WALLET_PUBLIC_KEY without falling back to the repository constant PLATFORM_WALLET_PUBLIC_KEY, and add PLATFORM_WALLET_PUBLIC_KEY to the documented environment settings in .env.example.Source: Path instructions
src/controllers/userController.js (4)
119-126: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not return the full user document for self requests.
The self branch leaves the query unprojected, so it can return the password hash and other private fields. The supplied
protectmiddleware explicitly selects-password; apply an explicit private-safe projection here as well.As per path instructions,
src/**/*.jschanges must preserve response-field protections.🤖 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 119 - 126, Update the user lookup in the controller’s self-request branch to apply an explicit private-safe projection, including excluding the password field, instead of leaving User.findById unprojected. Preserve PUBLIC_FIELDS for non-self requests and ensure both branches return only permitted response fields.Source: Path instructions
101-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep all changed responses in the standard API envelope.
The new duplicate-email response and the learning-dashboard success/error responses omit
data(and the dashboard success response also omitsmessage). Returndata: nullfor errors and place dashboard results underdata, so clients do not need endpoint-specific parsing.As per path instructions,
src/**/*.jsendpoints must use{ success, message, data }response shapes.Suggested response shape
- message: "A user with this email already exists", + message: "A user with this email already exists", + data: null, - res.status(200).json({ success: true, courses }); + res.status(200).json({ + success: true, + message: "Learning dashboard fetched", + data: { courses }, + }); - res.status(500).json({ success: false, message: "Failed to fetch learning dashboard" }); + res.status(500).json({ + success: false, + message: "Failed to fetch learning dashboard", + data: null, + });Also applies to: 474-477
🤖 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 101 - 105, Update the duplicate-email response in the user controller and the learning-dashboard success/error responses to use the standard { success, message, data } envelope: set data to null for errors, include a message on dashboard success, and place dashboard results under data. Preserve the existing status codes and response content while applying this shape consistently to all affected branches.Source: Path instructions
119-126: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope the user profile cache by viewer identity.
src/routes/userRoutes.js:61-66cachesGET /:idusing only the target ID, butgetUserreturns different data for the owner vs other viewers. A self-view can be replayed to another user unless the cache key includes requester/visibility or this route is left uncached.🤖 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 119 - 126, The GET /:id cache in the user route must account for viewer-specific visibility because getUser returns PUBLIC_FIELDS for non-owners but full data for self-views. Update the cache configuration around the GET /:id handler to include req.user identity or visibility in the key, or remove caching for this route; preserve distinct self-view and public-view responses.
451-462: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsupported
statusvalues.Every value other than
"completed"currently silently selects incomplete courses, so typos return misleading data. Validatestatusagainst"completed"and"in-progress"and return HTTP 400 for anything else.As per path instructions,
src/**/*.jsendpoints must validate request input.Suggested validation
const userId = req.user._id; const { status = "in-progress" } = req.query; + if (!["completed", "in-progress"].includes(status)) { + return res.status(400).json({ + success: false, + message: "Invalid status", + data: null, + }); + }🤖 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 451 - 462, Validate the destructured status value in the user progress endpoint before querying CourseProgress, allowing only "completed" and "in-progress"; return HTTP 400 for any other value. Keep the existing filtering behavior unchanged for the two supported statuses, using the surrounding controller handler as the implementation point.Source: Path instructions
src/routes/userRoutes.js (1)
135-135: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd Jest coverage for the user update/delete auth branches. The
/update/:idandDELETE /:idhandlers need tests for admin, self, and forbidden other-user access so the allowed and 403 paths stay covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/userRoutes.js` at line 135, Add Jest tests for the user route update and delete handlers, covering admin access, self-access, and forbidden access to another user; assert successful behavior for allowed branches and HTTP 403 responses for forbidden branches, using the existing route test setup and authentication helpers.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.
Outside diff comments:
In `@src/controllers/stellar/paymentController.js`:
- Around line 736-799: Remove the duplicated buyer update block around
transaction.itemType, including the manual purchase, stats, course enrollment,
and buyer.save operations, since grantItemAccess already performs them. Remove
the earlier unguarded generateReceipt enqueue, retaining only the guarded
best-effort enqueue after grantItemAccess so receipt failures do not roll back
the confirmed payment.
- Line 65: Update the platform wallet configuration used by the payment
controller to require process.env.PLATFORM_WALLET_PUBLIC_KEY without falling
back to the repository constant PLATFORM_WALLET_PUBLIC_KEY, and add
PLATFORM_WALLET_PUBLIC_KEY to the documented environment settings in
.env.example.
In `@src/controllers/userController.js`:
- Around line 119-126: Update the user lookup in the controller’s self-request
branch to apply an explicit private-safe projection, including excluding the
password field, instead of leaving User.findById unprojected. Preserve
PUBLIC_FIELDS for non-self requests and ensure both branches return only
permitted response fields.
- Around line 101-105: Update the duplicate-email response in the user
controller and the learning-dashboard success/error responses to use the
standard { success, message, data } envelope: set data to null for errors,
include a message on dashboard success, and place dashboard results under data.
Preserve the existing status codes and response content while applying this
shape consistently to all affected branches.
- Around line 119-126: The GET /:id cache in the user route must account for
viewer-specific visibility because getUser returns PUBLIC_FIELDS for non-owners
but full data for self-views. Update the cache configuration around the GET /:id
handler to include req.user identity or visibility in the key, or remove caching
for this route; preserve distinct self-view and public-view responses.
- Around line 451-462: Validate the destructured status value in the user
progress endpoint before querying CourseProgress, allowing only "completed" and
"in-progress"; return HTTP 400 for any other value. Keep the existing filtering
behavior unchanged for the two supported statuses, using the surrounding
controller handler as the implementation point.
In `@src/routes/userRoutes.js`:
- Line 135: Add Jest tests for the user route update and delete handlers,
covering admin access, self-access, and forbidden access to another user; assert
successful behavior for allowed branches and HTTP 403 responses for forbidden
branches, using the existing route test setup and authentication helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57c98247-64b8-4880-a151-fba79e759026
📒 Files selected for processing (5)
src/controllers/authController.jssrc/controllers/stellar/paymentController.jssrc/controllers/stellar/walletController.jssrc/controllers/userController.jssrc/routes/userRoutes.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/controllers/stellar/walletController.js
- src/controllers/authController.js
…ix duplicate course grant in paymentController
3 participants
Summary
Adds a durable, structured, append-only audit log for sensitive security and financial events across auth, wallet, and payment domains, along with a read-only admin query API.
Key Changes
src/models/AuditLog.js): Schema with indexes{actor, createdAt},{action, createdAt}, and{targetType, targetId}. Pre-hooks block anyupdate*ordelete*calls at the Mongoose layer. Financial logs have no auto-expiry TTL.src/services/audit/auditService.js): Non-blockingrecordAudit()helper. Enforces an explicit metadata allowlist (redactMetadata()) to guarantee zero secrets (passwords, tokens, signed XDR, OTPs) reach storage.authController.js: register (success/failure), login (success/failure), logout, password reset.walletController.js: connect (success/failure/reassign attempt), disconnect.paymentController.js: initialize, submit confirmed, submit failed, cancel.src/routes/admin/auditRoutes.js):GET /api/admin/auditgated viaprotect → authorizeRoles("admin"). Supports pagination and filtering (actor,action,targetType,targetId,status, date ranges). All non-GET HTTP methods return405 Method Not Allowed.Verification
test/auditLog.test.js): 21/21 tests passing covering redaction, append-only model enforcement, event capture, 401/403/405 route security, filtering, and pagination.Summary by CodeRabbit
/api/admin/auditwith filtering and pagination.Courseto supportsectionswith ordered lessons.updateUsernow returns HTTP 409 with a clear “email already exists” message on duplicates.