[Enhancement] Signed webhook event system: HMAC-signed payment lifecy… - #57
[Enhancement] Signed webhook event system: HMAC-signed payment lifecy…#57Johnsource-hub wants to merge 1 commit into
Conversation
…cle callbacks with retries and dead-letter redelivery
WalkthroughAdds a signed webhook system with endpoint management, HMAC payloads, asynchronous delivery, retries, dead-letter redelivery, SSRF validation, and payment, enrollment, and wallet event integrations. ChangesWebhook system
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant webhookService
participant WebhookDelivery
participant deliveryWorker
participant Endpoint
Controller->>webhookService: emitEvent(eventType, data)
webhookService->>WebhookDelivery: insert pending delivery
deliveryWorker->>WebhookDelivery: claim due delivery
deliveryWorker->>Endpoint: POST signed payload
Endpoint-->>deliveryWorker: HTTP response
deliveryWorker->>WebhookDelivery: update delivery status and attempts
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
src/services/webhooks/deliveryWorker.js (1)
109-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNice touch on the auto-disable safety valve — let's just not copy-paste it. 🙂
The exact same auto-disable block (set
isActive=false,disabledAt,disabledReason, log warn) is repeated verbatim in the non-2xx branch here and again in thecatchbranch at Lines 145-154. Duplicated logic like this tends to drift over time (someone fixes one path and forgets the other), so extracting a tiny helper keeps both failure paths honest.♻️ Suggested helper extraction
+async function maybeAutoDisableEndpoint(endpoint) { + if (endpoint.consecutiveFailures < MAX_CONSECUTIVE_FAILURES_TO_DISABLE) return; + endpoint.isActive = false; + endpoint.disabledAt = new Date(); + endpoint.disabledReason = `Auto-disabled after ${endpoint.consecutiveFailures} consecutive delivery failures`; + await endpoint.save(); + logger.warn( + { endpointId: endpoint._id, consecutiveFailures: endpoint.consecutiveFailures }, + "Webhook endpoint auto-disabled due to sustained failures" + ); +}Then call
await maybeAutoDisableEndpoint(endpoint);in both branches.🤖 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/webhooks/deliveryWorker.js` around lines 109 - 119, Extract the repeated auto-disable logic from the non-2xx and catch failure paths into a shared async helper, such as maybeAutoDisableEndpoint, preserving the existing threshold check, endpoint updates, save operation, and warning log. Replace both duplicated blocks with await maybeAutoDisableEndpoint(endpoint) so both branches use the same behavior.app.js (1)
184-193: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRouter mount + test-guarded worker start look correct. The
NODE_ENV !== "test"guard matches the.env.examplecontract (worker must not run background jobs under test).One optional follow-up:
webhookWorkerholds astop()handle but I don't see it invoked on shutdown (SIGTERM/SIGINT). Wiring it into graceful shutdown avoids leaked timers/half-sent deliveries during redeploys, if you have a shutdown hook elsewhere.🤖 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 `@app.js` around lines 184 - 193, Wire the `webhookWorker` stop handle into the existing graceful shutdown path for SIGTERM/SIGINT, invoking `stop()` before process termination. Preserve the current `NODE_ENV !== "test"` startup guard and ensure shutdown remains safe when `webhookWorker` is null.src/controllers/webhookController.js (2)
11-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused helper:
requireAdminis never called.Every handler enforces authorization inline via
req.user.role !== "admin" && endpoint.owner.toString() !== ..., so this function is dead code. Either wire it up where admin-only behavior is intended, or remove it to avoid confusing future maintainers about which authorization pattern is actually enforced.🤖 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/webhookController.js` around lines 11 - 20, Remove the unused requireAdmin helper from the controller, since authorization is currently enforced inline by the handlers. Do not alter the existing inline authorization checks or introduce a second authorization pattern.
270-281: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded/unvalidated pagination inputs.
page/limitcome straight fromreq.querywith no numeric validation or upper bound. A non-numeric value producesNaNforskip/limit(likely a driver error caught as a generic 500), and there's nothing preventinglimit=100000+ from doing a very large scan/transfer.♻️ Suggested guard
- const { status, page = 1, limit = 20 } = req.query; + const { status } = req.query; + const page = Math.max(1, parseInt(req.query.page, 10) || 1); + const limit = Math.min(100, Math.max(1, parseInt(req.query.limit, 10) || 20)); const query = { endpoint: endpoint._id }; if (status) query.status = status; - const skip = (parseInt(page) - 1) * parseInt(limit); + const skip = (page - 1) * limit; const [deliveries, total] = await Promise.all([ WebhookDelivery.find(query) .sort({ createdAt: -1 }) .skip(skip) - .limit(parseInt(limit)), + .limit(limit), WebhookDelivery.countDocuments(query), ]);🤖 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/webhookController.js` around lines 270 - 281, Validate and normalize the page and limit inputs in the delivery query flow before calculating skip, rejecting non-numeric or non-positive values and enforcing a reasonable maximum limit. Update the pagination logic around WebhookDelivery.find and countDocuments so invalid requests receive the controller’s established client-error response, while valid requests continue using the normalized values for skip and limit.src/services/webhooks/webhookService.js (1)
120-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead/buggy helper — not used by the delivery worker and contains a placeholder bug.
buildDeliveryHeaderssets"X-DeenBridge-Event": undefined(to be "set per-delivery") and never setsX-DeenBridge-Event-Id. Per the supplieddeliveryWorker.jscontext snippet, the actual delivery worker builds its headers manually inline rather than calling this function, so this exported helper appears to be dead/duplicated logic that has drifted out of sync with the real header set (missing the event-id header) and would need extra wiring to actually work if someone picked it up later.Recommend either removing this function or fixing it to match the real header contract and having
deliveryWorker.jscall it, so there's a single source of truth for delivery headers.🤖 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/webhooks/webhookService.js` around lines 120 - 133, The exported buildDeliveryHeaders helper is unused and contains an undefined event header while omitting the event-id header. Remove this dead helper, or make it the single source of truth by matching the delivery worker’s complete header contract and updating deliveryWorker.js to call it for every delivery.src/utils/ssrfGuard.js (1)
27-37: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMinor: IPv6 private-range detection is too coarse, and
isIPv6Loopbackis unused.
ip.startsWith("fc") || ip.startsWith("fe")(Line 29) will match any IPv6 address infe00::–feff::, not just the intendedfe80::/10link-local range, so some legitimate public-ish IPv6 literals could be rejected. This fails safe (over-blocks) so it's not exploitable, just imprecise. AlsoisIPv6Loopback(Lines 35-37) is defined but never called.Only checking
resolve4also means an endpoint hostname that only has an AAAA record has no IPv6-specific private-range check at all (it just fails DNS resolution and gets rejected as invalid, which is safe but not correct for legitimate IPv6-only hosts).🤖 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/utils/ssrfGuard.js` around lines 27 - 37, The IPv6 classification in isPrivateOrLoopback is overly broad and leaves isIPv6Loopback unused. Replace prefix checks with precise IPv6 range validation for fc00::/7 and fe80::/10, reuse isIPv6Loopback where appropriate, and update hostname resolution to inspect AAAA records alongside resolve4 so valid IPv6-only hosts receive the same private-range checks.src/models/WebhookDelivery.js (1)
61-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a compound index covering the
listDeliveriessort pattern.
webhookController.js'slistDeliveriesfilters by{ endpoint, status? }and sorts bycreatedAt: -1. The existing{ endpoint: 1, status: 1 }index doesn't cover the sort, so Mongo will need an in-memory sort once an endpoint accumulates many delivery records.♻️ Suggested additional index
webhookDeliverySchema.index({ endpoint: 1, status: 1 }); webhookDeliverySchema.index({ status: 1, nextAttemptAt: 1 }); +webhookDeliverySchema.index({ endpoint: 1, createdAt: -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/WebhookDelivery.js` around lines 61 - 62, Update webhookDeliverySchema indexes to add a compound index matching the listDeliveries query pattern: endpoint and optional status equality filters followed by createdAt descending sort. Preserve the existing indexes and ensure the new index supports efficient sorted retrieval without an in-memory sort.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/stellar/paymentController.js`:
- Line 436: Remove the newly added paymentsConfirmed.inc({ type: "purchase" })
call from the post-commit purchase flow in the payment controller. Keep the
existing increment after the confirmed transaction.save unchanged so each
confirmed purchase is counted once.
In `@src/controllers/webhookController.js`:
- Around line 349-376: Update pingEndpoint to create a WebhookDelivery targeted
directly at endpoint._id instead of calling the subscription-based emitEvent
broadcast. Ensure the delivery is created independently of payment.initialized
or wildcard subscriptions, while preserving the existing authorization, success
response, and error handling behavior.
- Around line 200-223: Align deleteEndpoint with the documented behavior by
limiting WebhookDelivery.deleteMany to pending deliveries only, using the
model’s existing pending-status field/value. Preserve the endpoint deletion and
authorization flow, or, if all delivery records are intentionally removed,
update the deleteEndpoint docstring to explicitly state that all deliveries are
deleted.
- Around line 26-43: Update all webhook controller handlers, including
listEndpoints, createEndpoint, listDeliveries, listEventTypes, getEndpoint,
updateEndpoint, deleteEndpoint, rotateSecret, and redeliver, to return the
mandated { success, message, data } envelope. Move endpoint payloads, counts,
secrets, deliveries, pagination, and events under data, while preserving
existing success status, messages, and error behavior; add or update Jest
coverage in test/webhook-api.test.js for the consistent response shapes.
In `@src/models/WebhookEndpoint.js`:
- Around line 19-23: Update the WebhookEndpoint secret persistence used by
createEndpoint, rotateSecret, and deliveryWorker so the platform can recover the
same raw secret consumers receive when generating outbound signatures. Replace
the one-way SHA-256-only storage with a server-recoverable representation, and
ensure signing reads that recoverable value while preserving select:false
protection.
In `@src/services/webhooks/deliveryWorker.js`:
- Around line 168-194: Update processDeliveries to implement a visibility lease
for claimed deliveries: stamp a claim timestamp when setting status to
"processing", and expand the selection criteria to include processing deliveries
whose lease has expired. Ensure the delivery schema/model persists the lease
field and use the existing timeout configuration or constant to determine
expiration, so crashed or failed workers can retry orphaned deliveries.
In `@src/services/webhooks/webhookService.js`:
- Around line 139-141: Update serializePayload to deep-sort the payload object
before calling JSON.stringify, rather than passing the top-level sorted keys as
the replacer array. Preserve all nested fields, including those under data,
while maintaining deterministic key ordering for the serialized body and
signature input.
In `@src/utils/ssrfGuard.js`:
- Around line 16-19: Update ipToNumber to validate input with net.isIP or an
equivalent strict IPv4 parser before conversion, returning null for hostnames,
IPv6 addresses, malformed dotted values, and non-IPv4 literals. Preserve numeric
conversion only for valid IPv4 literals so validateEndpointUrl continues to
perform DNS checks for hostnames and invalid IP-like inputs.
In `@test/webhook-api.test.js`:
- Line 8: Update the JWT_SECRET setup in the test to require
process.env.JWT_SECRET and fail fast when it is missing, removing the hardcoded
fallback. Ensure the test uses the same environment-provided secret as the app’s
protect middleware and does not introduce any inline secret.
- Around line 32-35: Update the test setup and teardown hooks around beforeEach
and afterAll to also clear the User collection with the existing collection
cleanup calls. Keep WebhookEndpoint and WebhookDelivery cleanup unchanged so
each test and the suite teardown reset all three collections.
In `@test/webhooks.test.js`:
- Around line 371-379: Ensure each affected SSRF test restores
process.env.NODE_ENV in a finally block around its validation and assertions.
Apply this to test/webhooks.test.js ranges 371-379, 392-402, 404-412, and
414-424; preserve each test’s existing setup, validation, and expectations while
moving restoration into finally so it runs on failures.
- Around line 366-369: Update the test case around validateEndpointUrl so its
requireHttps option matches the intended HTTPS-required scenario described by
the test title, while preserving the existing URL and validity assertion.
- Around line 251-283: Make the retry, dead, and auto-disable test cases
deterministic by stubbing axios.post in test/webhooks.test.js at lines 251-283,
285-323, and 325-356 to reject or return a non-2xx response; remove the
ECONNREFUSED-specific comment and ensure each site’s existing failure assertions
remain unchanged.
---
Nitpick comments:
In `@app.js`:
- Around line 184-193: Wire the `webhookWorker` stop handle into the existing
graceful shutdown path for SIGTERM/SIGINT, invoking `stop()` before process
termination. Preserve the current `NODE_ENV !== "test"` startup guard and ensure
shutdown remains safe when `webhookWorker` is null.
In `@src/controllers/webhookController.js`:
- Around line 11-20: Remove the unused requireAdmin helper from the controller,
since authorization is currently enforced inline by the handlers. Do not alter
the existing inline authorization checks or introduce a second authorization
pattern.
- Around line 270-281: Validate and normalize the page and limit inputs in the
delivery query flow before calculating skip, rejecting non-numeric or
non-positive values and enforcing a reasonable maximum limit. Update the
pagination logic around WebhookDelivery.find and countDocuments so invalid
requests receive the controller’s established client-error response, while valid
requests continue using the normalized values for skip and limit.
In `@src/models/WebhookDelivery.js`:
- Around line 61-62: Update webhookDeliverySchema indexes to add a compound
index matching the listDeliveries query pattern: endpoint and optional status
equality filters followed by createdAt descending sort. Preserve the existing
indexes and ensure the new index supports efficient sorted retrieval without an
in-memory sort.
In `@src/services/webhooks/deliveryWorker.js`:
- Around line 109-119: Extract the repeated auto-disable logic from the non-2xx
and catch failure paths into a shared async helper, such as
maybeAutoDisableEndpoint, preserving the existing threshold check, endpoint
updates, save operation, and warning log. Replace both duplicated blocks with
await maybeAutoDisableEndpoint(endpoint) so both branches use the same behavior.
In `@src/services/webhooks/webhookService.js`:
- Around line 120-133: The exported buildDeliveryHeaders helper is unused and
contains an undefined event header while omitting the event-id header. Remove
this dead helper, or make it the single source of truth by matching the delivery
worker’s complete header contract and updating deliveryWorker.js to call it for
every delivery.
In `@src/utils/ssrfGuard.js`:
- Around line 27-37: The IPv6 classification in isPrivateOrLoopback is overly
broad and leaves isIPv6Loopback unused. Replace prefix checks with precise IPv6
range validation for fc00::/7 and fe80::/10, reuse isIPv6Loopback where
appropriate, and update hostname resolution to inspect AAAA records alongside
resolve4 so valid IPv6-only hosts receive the same private-range checks.
🪄 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: 3a3ca31b-1c8b-4ad6-ba5d-b3ab2d7abfcf
📒 Files selected for processing (13)
app.jssrc/controllers/courses/courseController.jssrc/controllers/stellar/paymentController.jssrc/controllers/stellar/walletController.jssrc/controllers/webhookController.jssrc/models/WebhookDelivery.jssrc/models/WebhookEndpoint.jssrc/routes/webhookRoutes.jssrc/services/webhooks/deliveryWorker.jssrc/services/webhooks/webhookService.jssrc/utils/ssrfGuard.jstest/webhook-api.test.jstest/webhooks.test.js
|
|
||
| await buyer.save({ session }); | ||
| await session.commitTransaction(); | ||
| paymentsConfirmed.inc({ type: "purchase" }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Payment metric is being double-counted here. 📊
paymentsConfirmed.inc({ type: "purchase" }) already runs on Line 401 (right after the confirmed transaction.save), and this newly-added line increments the same counter a second time after commit. Every confirmed purchase will now count as two, which quietly corrupts dashboards and any alerting/SLOs built on paymentsConfirmed. Since the pre-commit increment on Line 401 is already in place, drop this one.
🐛 Suggested fix
await session.commitTransaction();
- paymentsConfirmed.inc({ type: "purchase" });
// Emit event after commit — fire-and-forget
emitEvent("payment.confirmed", {📝 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.
| paymentsConfirmed.inc({ type: "purchase" }); |
🤖 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 436, Remove the newly
added paymentsConfirmed.inc({ type: "purchase" }) call from the post-commit
purchase flow in the payment controller. Keep the existing increment after the
confirmed transaction.save unchanged so each confirmed purchase is counted once.
| export const listEndpoints = async (req, res) => { | ||
| try { | ||
| const query = req.user.role === "admin" | ||
| ? {} | ||
| : { owner: req.user._id }; | ||
|
|
||
| const endpoints = await WebhookEndpoint.find(query).sort({ createdAt: -1 }); | ||
|
|
||
| res.status(200).json({ | ||
| success: true, | ||
| count: endpoints.length, | ||
| endpoints, | ||
| }); | ||
| } catch (error) { | ||
| logger.error({ err: error }, "List webhook endpoints error"); | ||
| res.status(500).json({ success: false, message: "Failed to list endpoints" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Response shapes don't follow the mandated { success, message, data } contract.
Nearly every handler returns ad hoc top-level keys instead of nesting the payload under data, e.g. listEndpoints → { success, count, endpoints } (Line 34-38), createEndpoint → { success, message, endpoint, secret } (Line 95-107), listDeliveries → { success, deliveries, pagination } (Line 283-292), listEventTypes → { success, events } (Line 383-386), and so on through getEndpoint, updateEndpoint, deleteEndpoint, rotateSecret, and redeliver.
Consistent envelopes matter here because test/webhook-api.test.js (the layer that depends on this one) and any future consumers will need to know exactly where to look for the payload; deviating per-endpoint invites brittle client code.
As per path instructions, "New or changed endpoints need Jest coverage ... and consistent response shapes ({ success, message, data })."
Also applies to: 118-135, 141-198, 204-223, 229-253, 259-297, 299-343, 378-387
🤖 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/webhookController.js` around lines 26 - 43, Update all
webhook controller handlers, including listEndpoints, createEndpoint,
listDeliveries, listEventTypes, getEndpoint, updateEndpoint, deleteEndpoint,
rotateSecret, and redeliver, to return the mandated { success, message, data }
envelope. Move endpoint payloads, counts, secrets, deliveries, pagination, and
events under data, while preserving existing success status, messages, and error
behavior; add or update Jest coverage in test/webhook-api.test.js for the
consistent response shapes.
Source: Path instructions
| /** | ||
| * Delete an endpoint and its pending deliveries. | ||
| * DELETE /api/webhooks/:id | ||
| */ | ||
| export const deleteEndpoint = async (req, res) => { | ||
| try { | ||
| const endpoint = await WebhookEndpoint.findById(req.params.id); | ||
| if (!endpoint) { | ||
| return res.status(404).json({ success: false, message: "Endpoint not found" }); | ||
| } | ||
|
|
||
| if (req.user.role !== "admin" && endpoint.owner.toString() !== req.user._id.toString()) { | ||
| return res.status(403).json({ success: false, message: "Forbidden" }); | ||
| } | ||
|
|
||
| await WebhookDelivery.deleteMany({ endpoint: endpoint._id }); | ||
| await endpoint.deleteOne(); | ||
|
|
||
| res.status(200).json({ success: true, message: "Endpoint deleted" }); | ||
| } catch (error) { | ||
| logger.error({ err: error }, "Delete webhook endpoint error"); | ||
| res.status(500).json({ success: false, message: "Failed to delete endpoint" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Docstring/behavior mismatch: this deletes all deliveries, not just pending ones.
The comment says "Delete an endpoint and its pending deliveries" but WebhookDelivery.deleteMany({ endpoint: endpoint._id }) (Line 215) removes every delivery regardless of status — including successfully delivered history. If delivery records are meant to serve as an audit trail (dispute resolution, debugging), permanently wiping delivered/dead records on endpoint deletion may not be desired; if it is intended, please update the comment to match.
🤖 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/webhookController.js` around lines 200 - 223, Align
deleteEndpoint with the documented behavior by limiting
WebhookDelivery.deleteMany to pending deliveries only, using the model’s
existing pending-status field/value. Preserve the endpoint deletion and
authorization flow, or, if all delivery records are intentionally removed,
update the deleteEndpoint docstring to explicitly state that all deliveries are
deleted.
| export const pingEndpoint = async (req, res) => { | ||
| try { | ||
| const endpoint = await WebhookEndpoint.findById(req.params.id); | ||
| if (!endpoint) { | ||
| return res.status(404).json({ success: false, message: "Endpoint not found" }); | ||
| } | ||
|
|
||
| if (req.user.role !== "admin" && endpoint.owner.toString() !== req.user._id.toString()) { | ||
| return res.status(403).json({ success: false, message: "Forbidden" }); | ||
| } | ||
|
|
||
| // Emit a ping event — will be delivered like any other event | ||
| await emitEvent("payment.initialized", { | ||
| ping: true, | ||
| message: "DeenBridge webhook ping", | ||
| endpointId: endpoint._id.toString(), | ||
| sentAt: new Date().toISOString(), | ||
| }); | ||
|
|
||
| res.status(200).json({ | ||
| success: true, | ||
| message: "Ping event emitted", | ||
| }); | ||
| } catch (error) { | ||
| logger.error({ err: error }, "Ping webhook endpoint error"); | ||
| res.status(500).json({ success: false, message: "Failed to send ping" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Major: pingEndpoint doesn't actually target the endpoint being pinged — it broadcasts to every subscriber of payment.initialized.
emitEvent (imported from webhookService.js) fans an event out to all active endpoints subscribed to that event type or "*" — it has no concept of "deliver only to this one endpoint." So POST /:id/ping:
- Will not deliver anything to the target endpoint if it isn't subscribed to
payment.initialized(or wildcard) — yet the handler still responds{ success: true, message: "Ping event emitted" }, misleading the caller into thinking their integration was exercised. - Will deliver the ping (including
endpointId: endpoint._id.toString(), which identifies the endpoint someone else pinged) to every other active endpoint across all tenants that happens to subscribe topayment.initializedor"*". That's unsolicited webhook traffic and a small cross-tenant information leak (another user's ping request reveals your endpoint's ObjectId to their consumer).
The ping should create a WebhookDelivery targeted directly at endpoint._id, independent of that endpoint's event subscriptions, rather than going through the subscription-based broadcast path.
🤖 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/webhookController.js` around lines 349 - 376, Update
pingEndpoint to create a WebhookDelivery targeted directly at endpoint._id
instead of calling the subscription-based emitEvent broadcast. Ensure the
delivery is created independently of payment.initialized or wildcard
subscriptions, while preserving the existing authorization, success response,
and error handling behavior.
| secret: { | ||
| type: String, | ||
| required: true, | ||
| select: false, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File list\n'
git ls-files 'src/models/WebhookEndpoint.js' 'src/controllers/webhookController.js' 'src/workers/deliveryWorker.js' 'src/**/*.js' | sed -n '1,200p'
printf '\n## Outline: src/models/WebhookEndpoint.js\n'
ast-grep outline src/models/WebhookEndpoint.js --view expanded || true
printf '\n## Outline: src/controllers/webhookController.js\n'
ast-grep outline src/controllers/webhookController.js --view expanded || true
printf '\n## Outline: src/workers/deliveryWorker.js\n'
ast-grep outline src/workers/deliveryWorker.js --view expanded || true
printf '\n## Search secret-related symbols\n'
rg -n "generateSecret|verifySecret|computeSignature|secret" src/models/WebhookEndpoint.js src/controllers/webhookController.js src/workers/deliveryWorker.jsRepository: Deen-Bridge/dnb-backend
Length of output: 5453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the first file paths differ, locate the actual files and print compact context.
fd -a 'WebhookEndpoint\.js|webhookController\.js|deliveryWorker\.js' src 2>/dev/null || trueRepository: Deen-Bridge/dnb-backend
Length of output: 325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant regions once the files are located.
for f in src/models/WebhookEndpoint.js src/controllers/webhookController.js src/workers/deliveryWorker.js; do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
nl -ba "$f" | sed -n '1,220p'
fi
doneRepository: Deen-Bridge/dnb-backend
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target files and print the exact relevant lines only.
for f in $(fd -a '^WebhookEndpoint\.js$|^webhookController\.js$|^deliveryWorker\.js$' src 2>/dev/null); do
printf '\n### %s\n' "$f"
nl -ba "$f" | sed -n '1,220p'
doneRepository: Deen-Bridge/dnb-backend
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in /home/jailuser/git/src/models/WebhookEndpoint.js \
/home/jailuser/git/src/controllers/webhookController.js \
/home/jailuser/git/src/services/webhooks/deliveryWorker.js; do
echo "### $f"
sed -n '1,260p' "$f" | cat -n
echo
doneRepository: Deen-Bridge/dnb-backend
Length of output: 21737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' /home/jailuser/git/src/services/webhooks/webhookService.js | cat -nRepository: Deen-Bridge/dnb-backend
Length of output: 5251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "timestamped HMAC|HMAC|computeSignature|X-DeenBridge-Signature|webhook" /home/jailuser/git/src -g '*.js'Repository: Deen-Bridge/dnb-backend
Length of output: 6359
Webhook signatures need a recoverable secret
src/models/WebhookEndpoint.js stores only a SHA-256 hash, but src/services/webhooks/deliveryWorker.js signs with that persisted value. Since createEndpoint / rotateSecret return the raw secret only once, consumers get one secret while the platform signs with another. Store a server-recoverable secret for signing, or outbound signatures can never match what consumers compute.
🤖 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/WebhookEndpoint.js` around lines 19 - 23, Update the
WebhookEndpoint secret persistence used by createEndpoint, rotateSecret, and
deliveryWorker so the platform can recover the same raw secret consumers receive
when generating outbound signatures. Replace the one-way SHA-256-only storage
with a server-recoverable representation, and ensure signing reads that
recoverable value while preserving select:false protection.
| import WebhookEndpoint from "../src/models/WebhookEndpoint.js"; | ||
| import WebhookDelivery from "../src/models/WebhookDelivery.js"; | ||
|
|
||
| const JWT_SECRET = process.env.JWT_SECRET || "deenbridge-temp-secret-key-2024"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Move the JWT secret out of source and confirm it matches the app. A hardcoded fallback secret in a .js file is exactly what we want to keep out of the repo, and this one also has two practical hazards:
- The tokens signed here are only accepted by the
protectmiddleware if the app is verifying with the same secret. IfJWT_SECRETis unset in CI, this test falls back to"deenbridge-temp-secret-key-2024"whileapp.jsmay fall back to something else → every authenticated request returns401. - Per
.env.example,JWT_SECRETrequires a minimum of 32 chars; this fallback is 31, so it would fail that documented constraint.
Prefer requiring the env var (fail fast) rather than embedding a secret:
🔐 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 webhook API tests");
+}As per path instructions: "Flag hardcoded secrets, connection strings, JWT secrets, or wallet keys; 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 webhook API 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/webhook-api.test.js` at line 8, Update the JWT_SECRET setup in the test
to require process.env.JWT_SECRET and fail fast when it is missing, removing the
hardcoded fallback. Ensure the test uses the same environment-provided secret as
the app’s protect middleware and does not introduce any inline secret.
Source: Path instructions
| beforeEach(async () => { | ||
| await WebhookEndpoint.deleteMany({}); | ||
| await WebhookDelivery.deleteMany({}); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
CI blocker: the User collection is never reset between tests, so repeated fixture creation will throw duplicate-key errors. 👋 Welcome, and thanks for the thorough test coverage!
beforeEach (and afterAll) clean WebhookEndpoint and WebhookDelivery, but not User. Yet almost every test does User.create({ _id: adminUserId, email: "admin@test.com", ... }) (and the GET tests reuse regularUserId / user@test.com). Because _id and email are uniquely indexed, the second test to insert the same document fails with an E11000 duplicate key error, cascading into failures across the suite. The first test happens to pass, which can make this easy to miss locally if you only run one test.
Reset User alongside the other collections so each test starts from a clean slate:
🧹 Proposed fix
beforeEach(async () => {
+ const User = (await import("../src/models/User.js")).default;
+ await User.deleteMany({});
await WebhookEndpoint.deleteMany({});
await WebhookDelivery.deleteMany({});
});And mirror it in afterAll (Line 26-30) to avoid leaking fixtures into other suites:
afterAll(async () => {
+ const User = (await import("../src/models/User.js")).default;
+ await User.deleteMany({});
await WebhookEndpoint.deleteMany({});
await WebhookDelivery.deleteMany({});
await mongoose.disconnect();
});📝 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.
| beforeEach(async () => { | |
| await WebhookEndpoint.deleteMany({}); | |
| await WebhookDelivery.deleteMany({}); | |
| }); | |
| beforeEach(async () => { | |
| const User = (await import("../src/models/User.js")).default; | |
| await User.deleteMany({}); | |
| await WebhookEndpoint.deleteMany({}); | |
| await WebhookDelivery.deleteMany({}); | |
| }); |
🤖 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/webhook-api.test.js` around lines 32 - 35, Update the test setup and
teardown hooks around beforeEach and afterAll to also clear the User collection
with the existing collection cleanup calls. Keep WebhookEndpoint and
WebhookDelivery cleanup unchanged so each test and the suite teardown reset all
three collections.
| it("failed delivery transitions to retrying with future nextAttemptAt", async () => { | ||
| const endpoint = await WebhookEndpoint.create({ | ||
| url: "https://example.com/hook", | ||
| secret: "hashed", | ||
| events: ["payment.confirmed"], | ||
| owner: new mongoose.Types.ObjectId(), | ||
| }); | ||
|
|
||
| const payload = { | ||
| eventId: "test-event-retry", | ||
| eventType: "payment.confirmed", | ||
| apiVersion: WEBHOOK_API_VERSION, | ||
| createdAt: new Date().toISOString(), | ||
| data: { transactionId: "tx-retry" }, | ||
| }; | ||
| await WebhookDelivery.create({ | ||
| endpoint: endpoint._id, | ||
| eventId: "test-event-retry", | ||
| eventType: "payment.confirmed", | ||
| payload, | ||
| status: "pending", | ||
| nextAttemptAt: new Date(), | ||
| }); | ||
|
|
||
| // Process — axios is not mocked here so it will fail (ECONNREFUSED) | ||
| await processDeliveries(1); | ||
|
|
||
| const delivery = await WebhookDelivery.findOne({ eventId: "test-event-retry" }); | ||
| expect(delivery.status).toBe("retrying"); | ||
| expect(delivery.attempts).toHaveLength(1); | ||
| expect(delivery.attempts[0].at).toBeDefined(); | ||
| expect(delivery.nextAttemptAt.getTime()).toBeGreaterThan(Date.now()); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -HI 'jest.config*'
fd -HI axios -p '**/__mocks__/**'
rg -nP "jest\.mock\(\s*['\"].*axios" -g '!node_modules'
rg -nP "setupFiles(AfterEnv)?" -A2 package.json jest.config.* 2>/dev/nullRepository: Deen-Bridge/dnb-backend
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## jest.config.js\n'
cat -n jest.config.js
printf '\n## axios mock files\n'
fd -HI 'axios*' .
fd -HI '__mocks__' .
printf '\n## axios jest mocks / stubs\n'
rg -n "jest\.mock\\(.*axios|__mocks__/axios|from ['\"].*axios['\"]|require\\(['\"].*axios['\"]\\)" .
printf '\n## webhook delivery implementation\n'
rg -n "deliverWebhook|processDeliveries|axios\.post|nextAttemptAt|consecutiveFailures|disabledReason|isActive" -S .Repository: Deen-Bridge/dnb-backend
Length of output: 5603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## jest.config.js'
sed -n '1,220p' jest.config.js
echo
echo '## mock files'
find . -path '*/__mocks__/*' -type f | sort
echo
echo '## axios references'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!coverage' "jest\.mock\(['\"].*axios|from ['\"].*axios['\"]|require\(['\"].*axios['\"]\)|__mocks__/.+axios|axios\.post" .
echo
echo '## webhook-related implementation'
rg -n --hidden --glob '!node_modules' "processDeliveries|deliverWebhook|nextAttemptAt|consecutiveFailures|disabledReason|isActive" .Repository: Deen-Bridge/dnb-backend
Length of output: 5559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path('.')
patterns = [
re.compile(r"jest\.mock\(['\"].*axios"),
re.compile(r"__mocks__.*/axios"),
re.compile(r"from ['\"].*axios['\"]"),
re.compile(r"require\(['\"].*axios['\"]\)"),
re.compile(r"\baxios\.post\b"),
]
files = []
for p in root.rglob('*'):
if not p.is_file():
continue
if any(part in {'.git', 'node_modules', 'dist', 'build', 'coverage'} for part in p.parts):
continue
if p.suffix not in {'.js', '.ts', '.jsx', '.tsx', '.json'}:
continue
try:
text = p.read_text(encoding='utf-8')
except Exception:
continue
if any(pt.search(text) for pt in patterns):
files.append(p)
for p in files:
print(p)
PY
echo
echo '## jest config references'
rg -n --hidden --glob '!node_modules' "setupFiles|setupFilesAfterEnv|globalSetup|globalTeardown|jest\.mock|axios" jest.config.js package.json test .github . 2>/dev/null || trueRepository: Deen-Bridge/dnb-backend
Length of output: 2230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## src/services/webhooks/deliveryWorker.js'
sed -n '1,240p' src/services/webhooks/deliveryWorker.js
echo
echo '## test/webhooks.test.js (imports and relevant section)'
sed -n '1,120p' test/webhooks.test.js
echo
sed -n '240,370p' test/webhooks.test.jsRepository: Deen-Bridge/dnb-backend
Length of output: 15465
Mock axios.post in test/webhooks.test.js:251-356. These cases still make live requests to https://example.com, so the retry/dead/auto-disable paths depend on DNS and the remote response. Because deliverWebhook treats any 2xx as success, a real 200 would make these assertions fail. Stub the call to reject or return a non-2xx so the suite stays deterministic; the ECONNREFUSED note is too specific.
📍 Affects 1 file
test/webhooks.test.js#L251-L283(this comment)test/webhooks.test.js#L285-L323test/webhooks.test.js#L325-L356
🤖 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/webhooks.test.js` around lines 251 - 283, Make the retry, dead, and
auto-disable test cases deterministic by stubbing axios.post in
test/webhooks.test.js at lines 251-283, 285-323, and 325-356 to reject or return
a non-2xx response; remove the ECONNREFUSED-specific comment and ensure each
site’s existing failure assertions remain unchanged.
| it("accepts https URLs when requireHttps is true (skipping DNS)", async () => { | ||
| const result = await validateEndpointUrl("https://example.com/hook", { requireHttps: false }); | ||
| expect(result.valid).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test title contradicts the option under test. The name says "when requireHttps is true (skipping DNS)", but the call passes { requireHttps: false }, so this case never actually exercises the requireHttps: true HTTPS-accepted path — it just confirms a valid https URL passes when the check is off. Either flip the option to true or rename the test so the intent is clear; otherwise a regression in the requireHttps: true accept path would slip through.
✏️ Option A — test what the title claims
- it("accepts https URLs when requireHttps is true (skipping DNS)", async () => {
- const result = await validateEndpointUrl("https://example.com/hook", { requireHttps: false });
+ it("accepts https URLs when requireHttps is true (skipping DNS)", async () => {
+ const result = await validateEndpointUrl("https://example.com/hook", { requireHttps: true });
expect(result.valid).toBe(true);
});📝 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.
| it("accepts https URLs when requireHttps is true (skipping DNS)", async () => { | |
| const result = await validateEndpointUrl("https://example.com/hook", { requireHttps: false }); | |
| expect(result.valid).toBe(true); | |
| }); | |
| it("accepts https URLs when requireHttps is true (skipping DNS)", async () => { | |
| const result = await validateEndpointUrl("https://example.com/hook", { requireHttps: true }); | |
| expect(result.valid).toBe(true); | |
| }); |
🤖 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/webhooks.test.js` around lines 366 - 369, Update the test case around
validateEndpointUrl so its requireHttps option matches the intended
HTTPS-required scenario described by the test title, while preserving the
existing URL and validity assertion.
| it("accepts http URLs in development", async () => { | ||
| const original = process.env.NODE_ENV; | ||
| process.env.NODE_ENV = "development"; | ||
|
|
||
| const result = await validateEndpointUrl("http://localhost:3000/hook"); | ||
| expect(result.valid).toBe(true); | ||
|
|
||
| process.env.NODE_ENV = original; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore NODE_ENV in a finally block to prevent cross-test leakage. Each of these SSRF tests saves original, overwrites process.env.NODE_ENV, then restores it only on the last line. If any assertion in between fails (or throws), the restore never runs and the mutated NODE_ENV leaks into every subsequent test — turning one failure into a confusing cascade, since SSRF behavior itself depends on NODE_ENV. Wrap the body in try { ... } finally { process.env.NODE_ENV = original; } at each site:
test/webhooks.test.js#L371-L379: guard thedevelopment/http://localhostcase.test/webhooks.test.js#L392-L402: guard theproductionloopback-literal case.test/webhooks.test.js#L404-L412: guard theproduction10.x.x.xcase.test/webhooks.test.js#L414-L424: guard theproductionhttp-before-IP case.
📍 Affects 1 file
test/webhooks.test.js#L371-L379(this comment)test/webhooks.test.js#L392-L402test/webhooks.test.js#L404-L412test/webhooks.test.js#L414-L424
🤖 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/webhooks.test.js` around lines 371 - 379, Ensure each affected SSRF test
restores process.env.NODE_ENV in a finally block around its validation and
assertions. Apply this to test/webhooks.test.js ranges 371-379, 392-402,
404-412, and 414-424; preserve each test’s existing setup, validation, and
expectations while moving restoration into finally so it runs on failures.
1 participant
#close
#45
📝 Summary
Introduces a robust, decoupled outbound webhook/event delivery system. This allows external consumers (e.g., DeenBridge AI, analytics, educator tooling) to subscribe to key platform lifecycle events (e.g., payment.confirmed, course.enrolled) over secure, signed HTTP callbacks.
Deliveries are signed via HMAC-SHA256 (Stripe-style), include exponential backoff retries with jitter, transition to a dead-letter state upon repeated failures, support automated endpoint disabling, and expose a management API for endpoint and delivery lifecycle management.
🎯 Context & Motivation
Previously, the platform emitted no outbound notifications for payment state transitions or course enrollments—state updates were logged internally and required external consumers to poll REST endpoints. This PR establishes an asynchronous, machine-to-machine event layer without introducing tight coupling or blocking HTTP request paths.
🏗️ What Was Built
WebhookEndpoint (src/models/WebhookEndpoint.js):
Stores registration data (url, secret, events, isActive, description).
Enforces HTTPS outside development.
Secrets are stored using SHA-256 hashes (secretHash) and are shown only once upon registration or rotation.
Includes auto-disable metadata (consecutiveFailures, disabledAt, disabledReason).
WebhookDelivery (src/models/WebhookDelivery.js):
Tracks delivery attempts per event per endpoint (pending, delivered, retrying, dead).
Stores bounded attempt histories (at, statusCode, error, durationMs) and a snapshot of the payload.
Bounded payload and error string fields to protect database storage.
Summary by CodeRabbit
New Features
Bug Fixes
Tests