Cherry/be implement checkout flow#24
Open
CherryCIC wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR implements a comprehensive Stripe PaymentSheet-based checkout system featuring idempotent order creation keyed by paymentIntentId, server-side payment and shipping validation, flexible shipping provider abstraction supporting both live Sendcloud and mock implementations, and webhook-driven shipment status tracking with timestamp-based deduplication. ChangesStripe PaymentSheet Checkout with Idempotent Orders and Shipping Provider Abstraction
Possibly Related Issues
Poem
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3c07faf53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+20
to
+21
| requestUrl === '/api/payment/webhook' || | ||
| requestUrl === '/api/shipping/webhook' |
There was a problem hiding this comment.
The raw body is only captured when originalUrl is exactly /api/payment/webhook or /api/shipping/webhook, so valid webhook requests to equivalent route variants (for example a configured trailing slash like /api/payment/webhook/, or any query-string form) skip rawBody capture and then fail signature verification in the webhook controllers. This can silently break Stripe/Sendcloud event processing in production depending on endpoint configuration.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.env.example (1)
18-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
SENDCLOUD_WEBHOOK_SECRETandSENDCLOUD_MODEare missing from the Sendcloud section.Both variables are consumed by
sendcloudConfig.tsin this PR (webhookSecretsourced fromSENDCLOUD_WEBHOOK_SECRET, mode controlled bySENDCLOUD_MODE) but are not documented here. Developers following this template will not know to configure them, making production webhook verification silently broken.📝 Proposed addition
SENDCLOUD_PUBLIC_KEY=your_sendcloud_public_key SENDCLOUD_SECRET_KEY=your_sendcloud_secret_key SENDCLOUD_API_URL=https://panel.sendcloud.sc/api/v2 +# Separate signing secret configured on the Sendcloud webhook integration (Dashboard → Settings → Integrations → Webhook) +SENDCLOUD_WEBHOOK_SECRET=your_sendcloud_webhook_secret +# 'mock' (default for non-production) or 'live' – controls which ShippingProvider is used +SENDCLOUD_MODE=mock🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 18 - 27, The .env.example is missing SENDCLOUD_WEBHOOK_SECRET and SENDCLOUD_MODE which sendcloudConfig.ts expects (webhookSecret sourced from SENDCLOUD_WEBHOOK_SECRET and mode from SENDCLOUD_MODE); update the Sendcloud section of .env.example to add SENDCLOUD_WEBHOOK_SECRET and SENDCLOUD_MODE entries with brief guidance (e.g., that the webhook secret is server-only and mode should be "live" or "test") so developers know to configure them and webhook verification will work as intended.src/modules/shipping/controllers/shippingController.ts (1)
54-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse and validate
weightandshippingMethodIdbefore calling the provider.
req.bodyis untyped here, so"1000","12", or a negative number can flow straight into the Sendcloud payload. That turns client input errors into provider failures/500s, andcreateParcelToServicePoint()is typed to receive a numeric shipping-method ID. Reject non-positive integers locally before buildingparcelData.Suggested fix
- const { orderId, weight, shippingMethodId } = req.body; + const { orderId, weight, shippingMethodId } = req.body; + const parsedWeight = weight === undefined ? 1000 : Number(weight); + const parsedShippingMethodId = + shippingMethodId === undefined ? undefined : Number(shippingMethodId); if (!orderId) { ResponseHandler.badRequest(res, 'Invalid request', 'orderId is required'); return; } + + if (!Number.isInteger(parsedWeight) || parsedWeight <= 0) { + ResponseHandler.badRequest( + res, + 'Invalid weight', + 'weight must be a positive integer', + ); + return; + } + + if ( + parsedShippingMethodId !== undefined && + (!Number.isInteger(parsedShippingMethodId) || + parsedShippingMethodId <= 0) + ) { + ResponseHandler.badRequest( + res, + 'Invalid shipping method', + 'shippingMethodId must be a positive integer', + ); + return; + } @@ - weight: weight || 1000, // Default 1kg if not provided + weight: parsedWeight, // Default 1kg if not provided @@ - parcelData.shipment = { id: shippingMethodId }; + parcelData.shipment = { id: parsedShippingMethodId }; parcelData.to_service_point = order.pickupPointId; sendcloudParcel = await shippingProvider.createParcelToServicePoint( parcelData, order.pickupPointId, - shippingMethodId, + parsedShippingMethodId, ); } else { - if (shippingMethodId) { - parcelData.shipment = { id: shippingMethodId }; + if (parsedShippingMethodId !== undefined) { + parcelData.shipment = { id: parsedShippingMethodId }; }Also applies to: 107-108, 122-143
🤖 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/modules/shipping/controllers/shippingController.ts` around lines 54 - 55, The controller currently pulls orderId, weight, and shippingMethodId raw from req.body and passes them to createParcelToServicePoint/parcelData unvalidated; parse weight and shippingMethodId into numbers (e.g., Number()/parseInt), ensure shippingMethodId is a positive integer (Number.isInteger and >0) and weight is a positive number (>0), and if validation fails return a 400 with a clear error message rather than calling the provider; update every place that constructs parcelData or calls createParcelToServicePoint (including the other occurrences mentioned) to use the validated/converted numeric values.
🧹 Nitpick comments (1)
src/modules/payment/PaymentRepository.ts (1)
64-67: 💤 Low valueAdd error handling for deleted customer scenarios.
StripeService.getCustomerByIDexplicitly throws an error when the customer has been deleted (seestripeConfig.tslines 30-31). Since the call inorderController.ts:376is not wrapped in error handling, an unhandled exception would disrupt the checkout flow. Consider wrapping this method in a try-catch to gracefully handle the edge case.Suggested defensive handling
async getCustomerEmail(customerId: string): Promise<string | undefined> { - const customer = await StripeService.getCustomerByID(customerId); - return customer.email || undefined; + try { + const customer = await StripeService.getCustomerByID(customerId); + return customer?.email || undefined; + } catch { + return undefined; + } }🤖 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/modules/payment/PaymentRepository.ts` around lines 64 - 67, getCustomerEmail currently calls StripeService.getCustomerByID without handling the case where that service throws for a deleted customer; wrap the call to StripeService.getCustomerByID inside getCustomerEmail in a try-catch, catch the specific deleted-customer error (or inspect error.message/ty pe) and return undefined (and optionally log a debug/warn) so callers like orderController can continue the checkout flow instead of bubbling an unhandled exception.
🤖 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/app.ts`:
- Around line 19-24: The match on requestUrl (derived from req.originalUrl) is
too strict and misses webhook calls with query strings; update the middleware
that sets (req as any).rawBody to compare only the pathname or use startsWith on
requestUrl (e.g., check requestUrl.startsWith('/api/payment/webhook') or parse
the URL and use its pathname) so requests like /api/payment/webhook?foo=bar
still get rawBody set for signature verification.
In `@src/modules/order/controllers/orderController.ts`:
- Around line 205-213: The controller currently only reads pickupPoint from
req.body so valid requests that supply pickupPointId and courier (but omit
pickupPoint) lose those values; update the destructuring at the top of the
handler in orderController.ts to also extract pickupPointId and courier from
req.body, and when constructing the order payload and shipping data (the object
passed toward createShipment()) prefer the explicit pickupPointId and courier
values if present (fall back to pickupPoint or shipping option carrier only when
those explicit fields are missing) so createShipment() receives a persisted
pickupPointId and correct courier.
In `@src/modules/shipping/controllers/shippingController.ts`:
- Around line 435-458: The current guard only ignores updates when both incoming
and existing timestamps parse and incoming is older; add a check so if
incomingTimestampMs is null but existingTimestampMs is not null, treat the
webhook as stale and return 200 {received:true, ignored:true} without calling
shipmentRepo.updateShipment. Update the logic around parseWebhookTimestamp
(incomingTimestampMs / existingTimestampMs) before mapping status and before
calling shipmentRepo.updateShipment (and keep the lastWebhookTimestamp update
behavior unchanged when incomingTimestampMs is present) so that null incoming
timestamps cannot overwrite a stored newer event; reference
parseWebhookTimestamp, incomingTimestampMs, existingTimestampMs,
mapShipmentStatus, and shipmentRepo.updateShipment to locate the change.
In `@src/modules/shipping/tests/mockShippingProvider.test.ts`:
- Around line 37-48: The test for provider.verifyWebhookSignature relies on
process.env.SENDCLOUD_WEBHOOK_SECRET which may be undefined in CI; make the test
deterministic by setting a known secret before computing the expected signature
and restoring it after (or by mocking process.env.SENDCLOUD_WEBHOOK_SECRET).
Specifically, in the test setup for mockShippingProvider.test.ts set
process.env.SENDCLOUD_WEBHOOK_SECRET = 'test-secret' (or mock the env) prior to
calling crypto.createHmac and provider.verifyWebhookSignature, compute the
signature with that known secret, and ensure any global side-effects are cleaned
up after the test.
In `@src/shared/config/sendcloudConfig.ts`:
- Around line 20-34: The webhookSecret fallback is incorrectly using
SENDCLOUD_SECRET_KEY which will break webhook HMAC verification; update the
sendcloudConfig.webhookSecret to only use process.env.SENDCLOUD_WEBHOOK_SECRET
(no fallback to SENDCLOUD_SECRET_KEY) and make missing webhookSecret in live
mode emit a clear warning (or fail-fast) alongside the existing credential check
(use the sendcloudConfig.webhookSecret and sendcloudConfig.mode checks where
resolveSendcloudMode() is used) so verifyWebhookSignature receives the correct
key in production.
---
Outside diff comments:
In @.env.example:
- Around line 18-27: The .env.example is missing SENDCLOUD_WEBHOOK_SECRET and
SENDCLOUD_MODE which sendcloudConfig.ts expects (webhookSecret sourced from
SENDCLOUD_WEBHOOK_SECRET and mode from SENDCLOUD_MODE); update the Sendcloud
section of .env.example to add SENDCLOUD_WEBHOOK_SECRET and SENDCLOUD_MODE
entries with brief guidance (e.g., that the webhook secret is server-only and
mode should be "live" or "test") so developers know to configure them and
webhook verification will work as intended.
In `@src/modules/shipping/controllers/shippingController.ts`:
- Around line 54-55: The controller currently pulls orderId, weight, and
shippingMethodId raw from req.body and passes them to
createParcelToServicePoint/parcelData unvalidated; parse weight and
shippingMethodId into numbers (e.g., Number()/parseInt), ensure shippingMethodId
is a positive integer (Number.isInteger and >0) and weight is a positive number
(>0), and if validation fails return a 400 with a clear error message rather
than calling the provider; update every place that constructs parcelData or
calls createParcelToServicePoint (including the other occurrences mentioned) to
use the validated/converted numeric values.
---
Nitpick comments:
In `@src/modules/payment/PaymentRepository.ts`:
- Around line 64-67: getCustomerEmail currently calls
StripeService.getCustomerByID without handling the case where that service
throws for a deleted customer; wrap the call to StripeService.getCustomerByID
inside getCustomerEmail in a try-catch, catch the specific deleted-customer
error (or inspect error.message/ty pe) and return undefined (and optionally log
a debug/warn) so callers like orderController can continue the checkout flow
instead of bubbling an unhandled exception.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea44758a-aa04-42e2-a282-0a6d9cf68b96
📒 Files selected for processing (36)
.env.examplejest.config.jssrc/app.tssrc/modules/categories/controllers/categoryController.tssrc/modules/charities/controllers/charityController.tssrc/modules/order/controllers/orderController.tssrc/modules/order/model/Order.tssrc/modules/order/repositories/OrderRepository.tssrc/modules/order/routes/orderRoutes.tssrc/modules/order/tests/exportRoutes.test.tssrc/modules/order/tests/orderController.test.tssrc/modules/order/tests/orderValidator.test.tssrc/modules/order/validators/orderValidator.tssrc/modules/payment/PaymentRepository.tssrc/modules/payment/controllers/paymentController.tssrc/modules/payment/routes/paymentRoutes.tssrc/modules/payment/services/PaymentService.tssrc/modules/payment/tests/paymentController.test.tssrc/modules/products/controllers/productController.tssrc/modules/products/tests/productRoutes.test.tssrc/modules/products/validators/productIdValidator.tssrc/modules/shipping/controllers/shippingController.tssrc/modules/shipping/models/Shipment.tssrc/modules/shipping/repositories/ShipmentRepository.tssrc/modules/shipping/routes/shippingRoutes.tssrc/modules/shipping/services/MockShippingProvider.tssrc/modules/shipping/services/SendcloudService.tssrc/modules/shipping/services/ShippingProvider.tssrc/modules/shipping/services/ShippingProviderFactory.tssrc/modules/shipping/tests/mockShippingProvider.test.tssrc/modules/shipping/tests/sendcloudService.test.tssrc/modules/shipping/tests/shippingWebhook.test.tssrc/shared/config/firebaseConfig.tssrc/shared/config/sendcloudConfig.tssrc/shared/config/stripeConfig.tssrc/test/setupEnv.ts
Comment on lines
+19
to
+24
| if ( | ||
| requestUrl === '/api/payment/webhook' || | ||
| requestUrl === '/api/shipping/webhook' | ||
| ) { | ||
| (req as any).rawBody = Buffer.from(buffer); | ||
| } |
There was a problem hiding this comment.
Exact === match on originalUrl will miss webhook requests with a query string.
req.originalUrl includes the query string, so a URL like /api/payment/webhook?foo=bar won't match and req.rawBody won't be set, silently breaking signature verification. Use startsWith or parse only the pathname component.
🛡️ Proposed fix
- if (
- requestUrl === '/api/payment/webhook' ||
- requestUrl === '/api/shipping/webhook'
- ) {
+ const pathname = requestUrl.split('?')[0];
+ if (
+ pathname === '/api/payment/webhook' ||
+ pathname === '/api/shipping/webhook'
+ ) {📝 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.
Suggested change
| if ( | |
| requestUrl === '/api/payment/webhook' || | |
| requestUrl === '/api/shipping/webhook' | |
| ) { | |
| (req as any).rawBody = Buffer.from(buffer); | |
| } | |
| const pathname = requestUrl.split('?')[0]; | |
| if ( | |
| pathname === '/api/payment/webhook' || | |
| pathname === '/api/shipping/webhook' | |
| ) { | |
| (req as any).rawBody = Buffer.from(buffer); | |
| } |
🤖 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/app.ts` around lines 19 - 24, The match on requestUrl (derived from
req.originalUrl) is too strict and misses webhook calls with query strings;
update the middleware that sets (req as any).rawBody to compare only the
pathname or use startsWith on requestUrl (e.g., check
requestUrl.startsWith('/api/payment/webhook') or parse the URL and use its
pathname) so requests like /api/payment/webhook?foo=bar still get rawBody set
for signature verification.
Comment on lines
205
to
213
| const { | ||
| amount, | ||
| paymentIntentId, | ||
| productId, | ||
| productName, | ||
| shipping, | ||
| deliveryMethod, | ||
| courier, | ||
| pickupPointId, | ||
| shippingOptionId, | ||
| shippingWeight, | ||
| pickupPoint, | ||
| } = req.body; |
There was a problem hiding this comment.
Persist the fallback pickupPointId/courier fields for pickup-point orders.
The validator accepts pickup-point checkouts with pickupPointId + courier even when pickupPoint is omitted, but this controller only reads pickupPoint. In that valid request shape, the saved order loses pickupPointId, and courier falls back to the hard-coded shipping option carrier. The downstream impact is that createShipment() later rejects the order as missing pickupPointId.
Suggested fix
const {
amount,
paymentIntentId,
productId,
shipping,
shippingOptionId,
shippingWeight,
pickupPoint,
+ pickupPointId,
+ courier,
} = req.body;
@@
const orderPickupPoint = buildPickupPoint(pickupPoint);
+ const resolvedPickupPointId = orderPickupPoint?.id ?? pickupPointId;
+ const resolvedCourier =
+ orderPickupPoint?.carrier ?? courier ?? shippingOption.carrier;
const resolvedShippingWeight =
typeof shippingWeight === 'number' ? shippingWeight : 500;
@@
- courier: orderPickupPoint?.carrier || shippingOption.carrier,
- pickupPointId: orderPickupPoint?.id,
+ courier: resolvedCourier,
+ pickupPointId: resolvedPickupPointId,
pickupPoint: orderPickupPoint,Also applies to: 404-429
🤖 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/modules/order/controllers/orderController.ts` around lines 205 - 213, The
controller currently only reads pickupPoint from req.body so valid requests that
supply pickupPointId and courier (but omit pickupPoint) lose those values;
update the destructuring at the top of the handler in orderController.ts to also
extract pickupPointId and courier from req.body, and when constructing the order
payload and shipping data (the object passed toward createShipment()) prefer the
explicit pickupPointId and courier values if present (fall back to pickupPoint
or shipping option carrier only when those explicit fields are missing) so
createShipment() receives a persisted pickupPointId and correct courier.
Comment on lines
76
to
+91
| const { amount } = req.body; | ||
|
|
||
| if (!Number.isInteger(amount) || amount <= 0) { | ||
| ResponseHandler.badRequest( | ||
| res, | ||
| 'Invalid payment amount', | ||
| 'amount must be a positive integer in pence', | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // Use service to handle Stripe logic and reuse existing customers when possible | ||
| const paymentService = new PaymentService(); | ||
| const responseData = await paymentService.createPaymentIntentForUserByUid(firebaseUid, amount); | ||
| const responseData = await paymentService.createPaymentIntentForUserByUid( | ||
| firebaseUid, | ||
| amount, | ||
| ); |
There was a problem hiding this comment.
Price the PaymentIntent from server-side checkout inputs, not the raw amount.
Any authenticated client can currently create a successful PaymentIntent for an arbitrary positive integer. createOrder() then rejects mismatched totals only after the charge has already succeeded, which leaves paid PaymentIntents with no order and forces manual recovery/refunds. This endpoint needs the same product/shipping inputs and pricing validation as order creation, or it should create intents from a server-stored quote instead of trusting amount.
Comment on lines
+435
to
+458
| const incomingTimestampMs = parseWebhookTimestamp(timestamp); | ||
| const existingTimestampMs = parseWebhookTimestamp( | ||
| shipment.lastWebhookTimestamp, | ||
| ); | ||
|
|
||
| if ( | ||
| incomingTimestampMs !== null && | ||
| existingTimestampMs !== null && | ||
| incomingTimestampMs <= existingTimestampMs | ||
| ) { | ||
| res.status(200).json({ received: true, ignored: true }); | ||
| return; | ||
| } | ||
|
|
||
| const status = mapShipmentStatus(parcel.status?.message); | ||
|
|
||
| await shipmentRepo.updateShipment(shipment.id, { | ||
| status, | ||
| trackingNumber: parcel.tracking_number, | ||
| trackingUrl: parcel.tracking_url, | ||
| lastWebhookTimestamp: | ||
| incomingTimestampMs !== null | ||
| ? new Date(incomingTimestampMs) | ||
| : shipment.lastWebhookTimestamp, |
There was a problem hiding this comment.
Ignore webhook updates whose timestamp cannot be ordered against a newer stored event.
The stale-event guard only runs when timestamp parses successfully. If an older Sendcloud webhook arrives without a parsable timestamp after a newer one has already been stored, this path still overwrites the shipment status and keeps the old lastWebhookTimestamp. That reintroduces out-of-order status regression.
Suggested fix
const incomingTimestampMs = parseWebhookTimestamp(timestamp);
const existingTimestampMs = parseWebhookTimestamp(
shipment.lastWebhookTimestamp,
);
+
+ if (existingTimestampMs !== null && incomingTimestampMs === null) {
+ res.status(200).json({ received: true, ignored: true });
+ return;
+ }
if (
incomingTimestampMs !== null &&
existingTimestampMs !== null &&
incomingTimestampMs <= existingTimestampMs📝 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.
Suggested change
| const incomingTimestampMs = parseWebhookTimestamp(timestamp); | |
| const existingTimestampMs = parseWebhookTimestamp( | |
| shipment.lastWebhookTimestamp, | |
| ); | |
| if ( | |
| incomingTimestampMs !== null && | |
| existingTimestampMs !== null && | |
| incomingTimestampMs <= existingTimestampMs | |
| ) { | |
| res.status(200).json({ received: true, ignored: true }); | |
| return; | |
| } | |
| const status = mapShipmentStatus(parcel.status?.message); | |
| await shipmentRepo.updateShipment(shipment.id, { | |
| status, | |
| trackingNumber: parcel.tracking_number, | |
| trackingUrl: parcel.tracking_url, | |
| lastWebhookTimestamp: | |
| incomingTimestampMs !== null | |
| ? new Date(incomingTimestampMs) | |
| : shipment.lastWebhookTimestamp, | |
| const incomingTimestampMs = parseWebhookTimestamp(timestamp); | |
| const existingTimestampMs = parseWebhookTimestamp( | |
| shipment.lastWebhookTimestamp, | |
| ); | |
| if (existingTimestampMs !== null && incomingTimestampMs === null) { | |
| res.status(200).json({ received: true, ignored: true }); | |
| return; | |
| } | |
| if ( | |
| incomingTimestampMs !== null && | |
| existingTimestampMs !== null && | |
| incomingTimestampMs <= existingTimestampMs | |
| ) { | |
| res.status(200).json({ received: true, ignored: true }); | |
| return; | |
| } | |
| const status = mapShipmentStatus(parcel.status?.message); | |
| await shipmentRepo.updateShipment(shipment.id, { | |
| status, | |
| trackingNumber: parcel.tracking_number, | |
| trackingUrl: parcel.tracking_url, | |
| lastWebhookTimestamp: | |
| incomingTimestampMs !== null | |
| ? new Date(incomingTimestampMs) | |
| : shipment.lastWebhookTimestamp, |
🤖 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/modules/shipping/controllers/shippingController.ts` around lines 435 -
458, The current guard only ignores updates when both incoming and existing
timestamps parse and incoming is older; add a check so if incomingTimestampMs is
null but existingTimestampMs is not null, treat the webhook as stale and return
200 {received:true, ignored:true} without calling shipmentRepo.updateShipment.
Update the logic around parseWebhookTimestamp (incomingTimestampMs /
existingTimestampMs) before mapping status and before calling
shipmentRepo.updateShipment (and keep the lastWebhookTimestamp update behavior
unchanged when incomingTimestampMs is present) so that null incoming timestamps
cannot overwrite a stored newer event; reference parseWebhookTimestamp,
incomingTimestampMs, existingTimestampMs, mapShipmentStatus, and
shipmentRepo.updateShipment to locate the change.
Comment on lines
+37
to
+48
| it('verifies webhook signatures against the configured secret', () => { | ||
| const rawBody = JSON.stringify({ action: 'parcel_status_changed' }); | ||
| const signature = crypto | ||
| .createHmac('sha256', process.env.SENDCLOUD_WEBHOOK_SECRET!) | ||
| .update(rawBody) | ||
| .digest('hex'); | ||
|
|
||
| expect(provider.verifyWebhookSignature(rawBody, signature)).toBe(true); | ||
| expect(provider.verifyWebhookSignature(rawBody, 'bad-signature')).toBe( | ||
| false, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test relies on undefined environment variable.
The test computes the expected signature using process.env.SENDCLOUD_WEBHOOK_SECRET!, but if this variable isn't set in the test environment, createHmac receives undefined and the signature comparison becomes meaningless or fails unexpectedly. Consider setting a known test secret in the test setup or mocking process.env to ensure deterministic behavior.
Proposed fix
describe('MockShippingProvider', () => {
const provider = new MockShippingProvider();
+ const TEST_WEBHOOK_SECRET = 'test-webhook-secret';
+
+ beforeAll(() => {
+ process.env.SENDCLOUD_WEBHOOK_SECRET = TEST_WEBHOOK_SECRET;
+ });
// ... other tests ...
it('verifies webhook signatures against the configured secret', () => {
const rawBody = JSON.stringify({ action: 'parcel_status_changed' });
const signature = crypto
- .createHmac('sha256', process.env.SENDCLOUD_WEBHOOK_SECRET!)
+ .createHmac('sha256', TEST_WEBHOOK_SECRET)
.update(rawBody)
.digest('hex');🤖 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/modules/shipping/tests/mockShippingProvider.test.ts` around lines 37 -
48, The test for provider.verifyWebhookSignature relies on
process.env.SENDCLOUD_WEBHOOK_SECRET which may be undefined in CI; make the test
deterministic by setting a known secret before computing the expected signature
and restoring it after (or by mocking process.env.SENDCLOUD_WEBHOOK_SECRET).
Specifically, in the test setup for mockShippingProvider.test.ts set
process.env.SENDCLOUD_WEBHOOK_SECRET = 'test-secret' (or mock the env) prior to
calling crypto.createHmac and provider.verifyWebhookSignature, compute the
signature with that known secret, and ensure any global side-effects are cleaned
up after the test.
Comment on lines
+20
to
+34
| webhookSecret: | ||
| process.env.SENDCLOUD_WEBHOOK_SECRET || | ||
| process.env.SENDCLOUD_SECRET_KEY || | ||
| '', | ||
| apiUrl: process.env.SENDCLOUD_API_URL || 'https://panel.sendcloud.sc/api/v2', | ||
| mode: resolveSendcloudMode(), | ||
| }; | ||
|
|
||
| // Validate configuration | ||
| if (!sendcloudConfig.publicKey || !sendcloudConfig.secretKey) { | ||
| if ( | ||
| sendcloudConfig.mode === 'live' && | ||
| (!sendcloudConfig.publicKey || !sendcloudConfig.secretKey) | ||
| ) { | ||
| console.warn('⚠️ Sendcloud credentials are not configured. Please set SENDCLOUD_PUBLIC_KEY and SENDCLOUD_SECRET_KEY in your .env file.'); | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
Incorrect SENDCLOUD_SECRET_KEY fallback for webhookSecret will silently break webhook verification in production.
Sendcloud's webhook secret is "a password used to sign all Sendcloud webhook requests" — it is a separate, dashboard-configured credential that is distinct from the API auth pair (SENDCLOUD_PUBLIC_KEY / SENDCLOUD_SECRET_KEY). Using SENDCLOUD_SECRET_KEY as a fallback will cause verifyWebhookSignature to compute HMACs with the wrong key, which means every live Sendcloud webhook will fail signature verification. There is also no warning emitted in live mode when webhookSecret is unconfigured, so this misconfiguration will be invisible at startup.
🐛 Proposed fix
webhookSecret:
process.env.SENDCLOUD_WEBHOOK_SECRET ||
- process.env.SENDCLOUD_SECRET_KEY ||
'',
...
// Validate configuration
if (
sendcloudConfig.mode === 'live' &&
(!sendcloudConfig.publicKey || !sendcloudConfig.secretKey)
) {
console.warn('⚠️ Sendcloud credentials are not configured. Please set SENDCLOUD_PUBLIC_KEY and SENDCLOUD_SECRET_KEY in your .env file.');
}
+
+if (sendcloudConfig.mode === 'live' && !sendcloudConfig.webhookSecret) {
+ console.warn('⚠️ SENDCLOUD_WEBHOOK_SECRET is not set. Webhook signature verification will reject all incoming Sendcloud webhooks in live mode.');
+}🤖 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/shared/config/sendcloudConfig.ts` around lines 20 - 34, The webhookSecret
fallback is incorrectly using SENDCLOUD_SECRET_KEY which will break webhook HMAC
verification; update the sendcloudConfig.webhookSecret to only use
process.env.SENDCLOUD_WEBHOOK_SECRET (no fallback to SENDCLOUD_SECRET_KEY) and
make missing webhookSecret in live mode emit a clear warning (or fail-fast)
alongside the existing credential check (use the sendcloudConfig.webhookSecret
and sendcloudConfig.mode checks where resolveSendcloudMode() is used) so
verifyWebhookSignature receives the correct key in production.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the change in plain language.
Checklist
npm run buildnpm test -- --runInBand.env.exampleif config changedTesting
List the checks you ran and the result.
Risk
Call out anything reviewers should watch closely, especially around auth, Stripe, Sendcloud, and order creation.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements