Webhook controller dependency injection & modularity#127
Conversation
…ication, payment, webhook notifications, and webhook services
|
Warning Rate limit exceeded@DioChuks has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds manual dependency-injection documentation and interface contracts; refactors WebhookController and WebhookNotificationService to accept injected interfaces; wires concrete instances in routes; updates request config getter typing; and adapts webhook controller tests and signature/merchant-active validation. No behavioral control-flow changes beyond added validations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Route as "Express Route\n(/webhook/:merchantId)"
participant Controller as "WebhookController\n(uses interfaces)"
participant Auth as "MerchantAuthService\n(IMerchantAuthService)"
participant WS as "WebhookService\n(IWebhookService)"
participant Notif as "WebhookNotificationService\n(IWebhookNotificationService)"
participant Crypto as "CryptoGeneratorService\n(ICryptoGeneratorService)"
Client->>Route: POST /webhook/:merchantId (body + headers.signature)
Route->>Controller: handleWebhook(req, res)
Controller->>Auth: getMerchantById(merchantId)
alt merchant not found
Controller-->>Client: 404 MERCHANT_NOT_FOUND
else merchant inactive
Controller-->>Client: 404 MERCHANT_INACTIVE
else signature missing/invalid
Controller-->>Client: 400/401 MISSING_PARAMETERS / INVALID_SIGNATURE
else valid
Controller->>WS: getMerchantWebhook(merchantId)
alt webhook not found
Controller-->>Client: 404 WEBHOOK_NOT_FOUND
else found
Controller->>Notif: send/notify(payload)
Notif->>Crypto: generate/verify signature (as needed)
Notif-->>Controller: Promise<boolean>
Controller-->>Client: 200 OK
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…implify service instantiation
…hookController tests
…Service constructor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/middlewares/configurationMiddleware.ts (1)
18-20: Add explicit return type annotation for consistency.The
getfunction here should have an explicit return type annotation matching the one added inconfigCacheMiddleware(lines 178-181). Currently, this relies on type inference, which may not align with the explicit union type defined later.Apply this diff to add the explicit return type:
- get: async (key: string, defaultValue?: string) => { + get: async ( + key: string, + defaultValue?: string + ): Promise<string | number | boolean | Record<string, unknown> | null> => { return await configurationService.getConfig(key, defaultValue); },src/controllers/webhook.controller.ts (1)
24-71: Signature isn’t verified; external delivery is misused as “verification” (regression + security risk).
- You read the signature (Line 26) but never verify it.
- You call sendWebhookNotification and treat its boolean as “isValid” signature (Lines 66-71), returning INVALID_SIGNATURE on failure. That couples inbound auth to outbound delivery and can cause false 401s on network hiccups. It also triggers side effects during auth checks.
Action (essential):
- Introduce a dedicated signature-verification dependency and verify before any outbound calls.
- Separate “signature invalid” from “delivery failed.”
Proposed patch (adds a verifier dep and fixes flow):
-import { IWebhookNotificationService } from "../interfaces/IWebhookNotificationService"; +import { IWebhookNotificationService } from "../interfaces/IWebhookNotificationService"; +import { IWebhookSignatureVerifier } from "../interfaces/IWebhookSignatureVerifier"; @@ -export class WebhookController { +export class WebhookController { private webhookService: IWebhookService; private webhookNotificationService: IWebhookNotificationService; private merchantAuthService: IMerchantAuthService; + private signatureVerifier: IWebhookSignatureVerifier; @@ - constructor( - webhookService: IWebhookService, - merchantAuthService: IMerchantAuthService, - webhookNotificationService: IWebhookNotificationService, - ) { + constructor( + webhookService: IWebhookService, + merchantAuthService: IMerchantAuthService, + webhookNotificationService: IWebhookNotificationService, + signatureVerifier: IWebhookSignatureVerifier, + ) { this.webhookService = webhookService; this.merchantAuthService = merchantAuthService; this.webhookNotificationService = webhookNotificationService; + this.signatureVerifier = signatureVerifier; } @@ - // Verify signature - const payload = req.body as WebhookPayload; - const isValid = await this.webhookNotificationService.sendWebhookNotification( - webhook.url, - payload, - merchantId, - ); - - if (!isValid) { - return res.status(401).json({ - status: "error", - code: "INVALID_SIGNATURE", - message: "Invalid webhook signature", - }); - } + // Verify signature before any side-effects + const raw = (req as any).rawBody ?? JSON.stringify(req.body); + const verified = await this.signatureVerifier.verify( + String(sig), + raw, + merchant.secret + ); + if (!verified) { + return res.status(401).json({ + status: "error", + code: "INVALID_SIGNATURE", + message: "Invalid webhook signature", + }); + } + + // Normalize payload and deliver + const normalized: WebhookPayload = { + transactionId: req.body?.payload?.transaction?.id, + transactionType: req.body?.payload?.transaction?.type, + status: req.body?.payload?.transaction?.status, + amount: req.body?.payload?.transaction?.amount_in?.amount, + asset: req.body?.payload?.transaction?.amount_in?.asset, + merchantId: req.body?.payload?.customer?.id ?? merchantId, + timestamp: new Date().toISOString(), + eventType: `${req.body?.payload?.transaction?.type}.${req.body?.payload?.transaction?.status}`, + reqMethod: req.method, + }; + const delivered = await this.webhookNotificationService.sendWebhookNotification( + webhook.url, + normalized, + merchantId, + ); + if (!delivered) { + return res.status(502).json({ + status: "error", + code: "DELIVERY_FAILED", + message: "Failed to deliver webhook to merchant", + }); + }src/tests/controllers/webhook.controller.test.ts (1)
123-134: Assert that delivery is invoked with the normalized payload.You build _expectedWebhookPayload but never assert its use. Add a call-assertion to ensure the controller maps and forwards correctly.
expect(responseStatus).toHaveBeenCalledWith(200); expect(responseJson).toHaveBeenCalledWith({ message: "Webhook processed successfully", status: "success", }); + expect(webhookNotificationService.sendWebhookNotification).toHaveBeenCalledWith( + mockMerchantWebhook.url, + expect.objectContaining({ + transactionId: _expectedWebhookPayload.transactionId, + transactionType: _expectedWebhookPayload.transactionType, + status: _expectedWebhookPayload.status, + amount: _expectedWebhookPayload.amount, + asset: _expectedWebhookPayload.asset, + merchantId: _expectedWebhookPayload.merchantId, + eventType: _expectedWebhookPayload.eventType, + reqMethod: "POST", + }), + "merchant-123" + );Note: This assumes the controller performs normalization (see controller review).
🧹 Nitpick comments (10)
src/middlewares/configurationMiddleware.ts (1)
178-181: Consider broadening thedefaultValueparameter type.The
defaultValueparameter is typed asstring, but the return type now includesnumber | boolean | Record<string, unknown> | null. This creates a type mismatch where a string default might be provided when the actual config value is expected to be a different type.Consider updating the parameter type to match the return type:
req.config.get = async ( key: string, - defaultValue?: string + defaultValue?: string | number | boolean | Record<string, unknown> | null ): Promise<string | number | boolean | Record<string, unknown> | null> => {This same change should be applied to the base middleware at line 18 and to the
configurationService.getConfigmethod signature if not already done.src/interfaces/IMerchantAuthService.ts (1)
1-3: Tighten the return type.Using
Promise<any>forfeits type safety across the DI boundary. Please surface the actual merchant shape (e.g.,Promise<Merchant | null>) so the controller and tests benefit from compile‑time checks.src/controllers/webhook.controller.ts (4)
26-33: Header handling: use a configured header name and normalize access.Accessing req.headers.signature assumes a nonstandard header and exact casing. Prefer a constant (e.g., WEBHOOK_SIGNATURE_HEADER) and normalize:
-const sig = req.headers?.signature; +const sigHeader = process.env.WEBHOOK_SIGNATURE_HEADER ?? "x-stellar-signature"; +const sig = req.headers?.[sigHeader] as string | string[] | undefined;Also include sigHeader in the 400 error context for easier debugging (but don’t echo values).
85-91: Don’t leak raw error messages to clients.Returning (error as Error).message can disclose internals. Return a generic string and log details server-side.
- return res.status(500).json({ - status: "error", - code: "INTERNAL_ERROR", - message: (error as Error).message, - }); + return res.status(500).json({ + status: "error", + code: "INTERNAL_ERROR", + message: "An unexpected error occurred.", + });Same for testWebhook’s catch block.
Also applies to: 165-170
94-121: Optional: mirror inactive-merchant guard for testWebhook.If inactive merchants shouldn’t send test events, add the isActive guard here as well for consistency.
1-4: Interface return types are too loose (any).IMerchantAuthService.getMerchantById and IWebhookService.getMerchantWebhook return Promise. Tighten to Merchant and MerchantWebhook to regain type safety and prevent the unsafe cast above. Based on relevant snippets.
src/tests/controllers/webhook.controller.test.ts (4)
150-169: Reset mocks inside the status loop to avoid cross-iteration bleed.Without clearing, earlier calls can satisfy later expectations.
- for (const status of statuses) { + for (const status of statuses) { + responseStatus.mockClear(); + responseJson.mockClear(); + (webhookNotificationService.sendWebhookNotification as jest.Mock).mockClear(); mockRequest.body = {
171-187: Add negative test: missing parameters (signature or merchantId).Cover the 400 MISSING_PARAMETERS branch.
it("should return 404 when merchant is not found", async () => { (merchantAuthService.getMerchantById as jest.Mock).mockResolvedValue( null, ); @@ expect(responseJson).toHaveBeenCalledWith({ code: "MERCHANT_NOT_FOUND", message: "Merchant not found", status: "error", }); }); + + it("should return 400 when signature header is missing", async () => { + mockRequest.headers = { } as any; + responseStatus.mockClear(); responseJson.mockClear(); + await webhookController.handleWebhook(mockRequest as Request, mockResponse as Response); + expect(responseStatus).toHaveBeenCalledWith(400); + expect(responseJson).toHaveBeenCalledWith({ + status: "error", + code: "MISSING_PARAMETERS", + message: "Missing required parameters", + }); + });
205-217: Assert explicit error code for inactive merchants.Strengthen the check to ensure the API contract.
await webhookController.handleWebhook( mockRequest as Request, mockResponse as Response, ); expect(responseStatus).toHaveBeenCalledWith(404); + expect(responseJson).toHaveBeenCalledWith({ + status: "error", + code: "MERCHANT_INACTIVE", + message: "Merchant is inactive", + });
116-140: Add delivery-failure (not signature) test case.If delivery fails, expect 502 DELIVERY_FAILED (per suggested controller fix).
describe("handleWebhook", () => { it("should successfully process a valid webhook request", async () => { @@ }); + + it("should return 502 when delivery to merchant webhook fails", async () => { + (webhookNotificationService.sendWebhookNotification as jest.Mock).mockResolvedValue(false); + responseStatus.mockClear(); responseJson.mockClear(); + await webhookController.handleWebhook(mockRequest as Request, mockResponse as Response); + expect(responseStatus).toHaveBeenCalledWith(502); + expect(responseJson).toHaveBeenCalledWith({ + status: "error", + code: "DELIVERY_FAILED", + message: "Failed to deliver webhook to merchant", + }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)docs/MANUAL_DEPENDENCY_INJECTION.md(1 hunks)src/controllers/webhook.controller.ts(6 hunks)src/interfaces/IBookingService.ts(1 hunks)src/interfaces/ICryptoGeneratorService.ts(1 hunks)src/interfaces/IMerchantAuthService.ts(1 hunks)src/interfaces/IPaymentService.ts(1 hunks)src/interfaces/IWebhookNotificationService.ts(1 hunks)src/interfaces/IWebhookService.ts(1 hunks)src/middlewares/configurationMiddleware.ts(1 hunks)src/routes/webhook.routes.ts(1 hunks)src/services/webhookNotification.service.ts(1 hunks)src/tests/controllers/webhook.controller.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/routes/webhook.routes.ts (5)
src/services/webhook.service.ts (1)
WebhookService(7-85)src/services/merchant.service.ts (1)
MerchantAuthService(13-309)src/services/cryptoGenerator.service.ts (1)
CryptoGeneratorService(4-48)src/services/webhookNotification.service.ts (1)
WebhookNotificationService(10-84)src/controllers/webhook.controller.ts (1)
WebhookController(9-172)
src/services/webhookNotification.service.ts (2)
src/services/merchant.service.ts (1)
MerchantAuthService(13-309)src/services/cryptoGenerator.service.ts (1)
CryptoGeneratorService(4-48)
src/tests/controllers/webhook.controller.test.ts (4)
src/controllers/webhook.controller.ts (1)
WebhookController(9-172)src/interfaces/IMerchantAuthService.ts (1)
IMerchantAuthService(1-3)src/interfaces/IWebhookService.ts (1)
IWebhookService(1-3)src/interfaces/IWebhookNotificationService.ts (1)
IWebhookNotificationService(3-14)
src/interfaces/IWebhookNotificationService.ts (1)
src/interfaces/webhook.interfaces.ts (1)
MerchantWebhook(18-25)
src/controllers/webhook.controller.ts (3)
src/interfaces/IWebhookService.ts (1)
IWebhookService(1-3)src/interfaces/IWebhookNotificationService.ts (1)
IWebhookNotificationService(3-14)src/interfaces/IMerchantAuthService.ts (1)
IMerchantAuthService(1-3)
🪛 Biome (2.1.2)
src/interfaces/IBookingService.ts
[error] 1-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
src/interfaces/ICryptoGeneratorService.ts
[error] 1-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (8)
src/middlewares/configurationMiddleware.ts (1)
184-184: Type assertion safety verified.No other writes to
requestCachedetected; cached values originate solely fromoriginalGet.src/routes/webhook.routes.ts (1)
9-28: LGTM! Dependency wiring is correct.The manual dependency injection is properly implemented:
- Services are instantiated in correct order (no circular dependencies)
WebhookNotificationServicereceives required dependencies (merchantAuthService,cryptoGeneratorService)WebhookControllerreceives all three required dependencies- Aligns with the DI pattern documented in the PR
docs/MANUAL_DEPENDENCY_INJECTION.md (1)
1-79: Excellent documentation!The manual DI documentation is clear, comprehensive, and includes practical examples. The step-by-step guide will help developers understand and maintain the DI pattern consistently across the codebase.
src/interfaces/IWebhookNotificationService.ts (1)
1-14: LGTM! Interface definition is correct.The interface properly defines the contract for webhook notification services:
- Method signatures match the concrete implementation
- Appropriate use of
Omit<WebhookPayload, "timestamp">fornotifyPaymentUpdate- Imports necessary types correctly
src/services/webhookNotification.service.ts (1)
14-20: LGTM! Constructor properly enforces dependency injection.The constructor has been correctly refactored to require explicit dependencies:
- Removed optional parameters and default instantiation
- Dependencies must be injected at construction time
- Aligns with the manual DI pattern implemented across the PR
src/controllers/webhook.controller.ts (2)
14-22: DI refactor looks good.Constructor now cleanly injects dependencies via interfaces; this unblocks unit testing and route-level wiring.
47-53: Inactive merchant status code—confirm 404 vs 403 policy.Returning 404 hides existence (ok for anti-enumeration). If product policy favors explicit auth semantics, consider 403 Forbidden with code MERCHANT_INACTIVE. Please confirm desired behavior.
src/tests/controllers/webhook.controller.test.ts (1)
14-24: Nice shift to interface-typed mocks.Dropping jest.mock of classes and using jest.Mocked interfaces aligns with the new manual DI and keeps tests lightweight.
|
@respp pls review thanks |
2 participants
Pull Request Overview
📝 Summary
"Refactor webhook controller and related services to use manual dependency injection for improved modularity and testability."
🔗 Related Issues
🔄 Changes Made
🖼️ Current Output
🧪 Testing
webhook.controller.test.ts📚 Documentation
💬 Comments
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests