diff --git a/apps/api/controllers/client/course_purchases.controller.js b/apps/api/controllers/client/course_purchases.controller.js index 5b1306b..1cf1a56 100644 --- a/apps/api/controllers/client/course_purchases.controller.js +++ b/apps/api/controllers/client/course_purchases.controller.js @@ -76,13 +76,17 @@ exports.captureCourseOrder = async (req, res) => { const { order_id } = req.body; if (!order_id) return R.error(res, 'order_id is required.', 400); - const purchase = await mdl_CoursePurchase.findOne({ + // Matched on provider_payload.order_id, not just "most recent pending" — + // see tiers.controller.js#captureOrder for why picking by recency alone + // can miss an older order that PayPal legitimately approved. + const pendingPurchases = await mdl_CoursePurchase.findAll({ where: { user_id: req.user.user_id, status: 'pending' }, include: [{ model: mdl_Product, as: 'product' }], order: [['createdAt', 'DESC']], }); + const purchase = pendingPurchases.find((p) => p.provider_payload?.order_id === order_id); - if (!purchase || purchase.provider_payload?.order_id !== order_id) + if (!purchase) return R.error(res, 'Pending purchase not found.', 404); let captureData; @@ -144,12 +148,14 @@ exports.cancelCourseOrder = async (req, res) => { const { order_id } = req.body; if (!order_id) return R.error(res, 'order_id is required.', 400); - const purchase = await mdl_CoursePurchase.findOne({ + // See captureCourseOrder above for why this matches on order_id instead of recency. + const pendingPurchases = await mdl_CoursePurchase.findAll({ where: { user_id: req.user.user_id, status: 'pending' }, order: [['createdAt', 'DESC']], }); + const purchase = pendingPurchases.find((p) => p.provider_payload?.order_id === order_id); - if (!purchase || purchase.provider_payload?.order_id !== order_id) + if (!purchase) return R.error(res, 'Pending purchase not found.', 404); await purchase.update({ diff --git a/apps/api/controllers/client/tiers.controller.js b/apps/api/controllers/client/tiers.controller.js index a92c7b5..88adf1a 100644 --- a/apps/api/controllers/client/tiers.controller.js +++ b/apps/api/controllers/client/tiers.controller.js @@ -310,13 +310,19 @@ exports.captureOrder = async (req, res) => { const { order_id } = req.body; if (!order_id) return R.error(res, 'order_id is required.', 400); - const payment = await mdl_Payments.findOne({ + // Matched on provider_payload.order_id, not just "most recent pending" — + // a user can have more than one pending payment at once (e.g. abandoned + // Plan A via browser-back instead of PayPal's cancel button, then started + // checkout on Plan B); picking by recency would miss an older order that + // PayPal legitimately approved. + const pendingPayments = await mdl_Payments.findAll({ where: { status: 'pending', user_id: req.user.user_id }, include: [{ model: mdl_TierPlans, as: 'plan' }], order: [['createdAt', 'DESC']], }); + const payment = pendingPayments.find((p) => p.provider_payload?.order_id === order_id); - if (!payment || payment.provider_payload?.order_id !== order_id) + if (!payment) return R.error(res, 'Pending payment not found.', 404); // Guard: plan was deactivated while user was on PayPal's approval page @@ -433,12 +439,14 @@ exports.cancelOrder = async (req, res) => { const { order_id } = req.body; if (!order_id) return R.error(res, 'order_id is required.', 400); - const payment = await mdl_Payments.findOne({ + // See captureOrder above for why this matches on order_id instead of recency. + const pendingPayments = await mdl_Payments.findAll({ where: { user_id: req.user.user_id, status: 'pending' }, order: [['createdAt', 'DESC']], }); + const payment = pendingPayments.find((p) => p.provider_payload?.order_id === order_id); - if (!payment || payment.provider_payload?.order_id !== order_id) + if (!payment) return R.error(res, 'Pending payment not found.', 404); await payment.update({ diff --git a/apps/api/routes/client/course_purchases.routes.js b/apps/api/routes/client/course_purchases.routes.js index cac3bcf..bc1561f 100644 --- a/apps/api/routes/client/course_purchases.routes.js +++ b/apps/api/routes/client/course_purchases.routes.js @@ -1,9 +1,10 @@ const express = require('express'); const router = express.Router(); const ctrl = require('../../controllers/client/course_purchases.controller'); +const { sensitiveOpsLimiter } = require('../../middleware/rateLimiter.middleware'); router.get ('/', ctrl.getMyPurchases); -router.post('/order', ctrl.createCourseOrder); +router.post('/order', sensitiveOpsLimiter, ctrl.createCourseOrder); router.post('/capture', ctrl.captureCourseOrder); router.post('/cancel', ctrl.cancelCourseOrder); diff --git a/apps/api/routes/client/tiers.routes.js b/apps/api/routes/client/tiers.routes.js index 2552a14..37780cc 100644 --- a/apps/api/routes/client/tiers.routes.js +++ b/apps/api/routes/client/tiers.routes.js @@ -1,6 +1,7 @@ const express = require('express'); const router = express.Router(); const ctrl = require('../../controllers/client/tiers.controller'); +const { sensitiveOpsLimiter } = require('../../middleware/rateLimiter.middleware'); // My tier router.get ('/me', ctrl.getMyTier); @@ -9,11 +10,13 @@ router.get ('/me/history', ctrl.getMyTierHistory); // Plans router.get ('/plans', ctrl.getPlans); -// Promo code validation -router.post('/promos/validate', ctrl.validatePromo); +// Promo code validation — rate-limited: unthrottled retries let an attacker +// brute-force/enumerate valid promo codes off the "Invalid promo code" reason. +router.post('/promos/validate', sensitiveOpsLimiter, ctrl.validatePromo); -// Checkout -router.post('/checkout/order', ctrl.createOrder); +// Checkout — order creation is rate-limited: unthrottled retries let a user +// burn through a limited-use promo's max_uses without ever paying. +router.post('/checkout/order', sensitiveOpsLimiter, ctrl.createOrder); router.post('/checkout/capture', ctrl.captureOrder); router.post('/checkout/cancel', ctrl.cancelOrder); router.post('/checkout/refund', ctrl.refundOrder); diff --git a/apps/api/services/payment.service.js b/apps/api/services/payment.service.js index 27538ed..12b274c 100644 --- a/apps/api/services/payment.service.js +++ b/apps/api/services/payment.service.js @@ -59,10 +59,13 @@ async function evaluatePromo(policy, plan, rawCode, effectivePrice = null) { if (rule.expires_at && new Date(rule.expires_at) < new Date()) return { valid: false, reason: 'Promo code has expired.' }; - // Count how many completed payments used this code for this plan + // Count how many completed payments used this code for this plan. Scoped to + // 'completed' only — pending/failed/cancelled attempts must NOT consume a + // limited-use code, or anyone can exhaust max_uses by creating orders they + // never pay for. if (rule.max_uses != null) { const uses = await mdl_Payments.count({ - where: { promo_code: code, plan_id: plan.plan_id }, + where: { promo_code: code, plan_id: plan.plan_id, status: 'completed' }, }); if (uses >= Number(rule.max_uses)) return { valid: false, reason: 'Promo code has reached its usage limit.' }; diff --git a/apps/api/tests/controllers/tiers.controller.capture.test.js b/apps/api/tests/controllers/tiers.controller.capture.test.js index c00452a..7eb6e0f 100644 --- a/apps/api/tests/controllers/tiers.controller.capture.test.js +++ b/apps/api/tests/controllers/tiers.controller.capture.test.js @@ -13,7 +13,7 @@ jest.mock('../../models/tiers/tier_categories.mdl', () => ({})); jest.mock('../../models/tiers/tier_plans.mdl', () => ({ findOne: jest.fn() })); jest.mock('../../models/tiers/user_tiers.mdl', () => ({ findOne: jest.fn(), create: jest.fn() })); -jest.mock('../../models/tiers/payments.mdl', () => ({ findOne: jest.fn() })); +jest.mock('../../models/tiers/payments.mdl', () => ({ findAll: jest.fn() })); jest.mock('../../models/system_badges/system_badges.mdl', () => ({})); jest.mock('../../models/assets/assets.mdl', () => ({})); jest.mock('../../models/notifications/user_notification.mdl', () => ({ create: jest.fn() })); @@ -57,7 +57,7 @@ describe('captureOrder() — declined/incomplete captures must not grant access' test('capture.status "DECLINED" marks the payment failed and creates no tier', async () => { const payment = makePayment(); - mdl_Payments.findOne.mockResolvedValue(payment); + mdl_Payments.findAll.mockResolvedValue([payment]); paymentSvc.captureOrder.mockResolvedValue({ status: 'COMPLETED', // outer order status can still say COMPLETED purchase_units: [{ payments: { captures: [{ id: 'CAP-1', status: 'DECLINED' }] } }], @@ -77,7 +77,7 @@ describe('captureOrder() — declined/incomplete captures must not grant access' test('capture.status "PENDING" (e.g. eCheck review) also withholds access', async () => { const payment = makePayment(); - mdl_Payments.findOne.mockResolvedValue(payment); + mdl_Payments.findAll.mockResolvedValue([payment]); paymentSvc.captureOrder.mockResolvedValue({ status: 'COMPLETED', purchase_units: [{ payments: { captures: [{ id: 'CAP-2', status: 'PENDING' }] } }], @@ -97,7 +97,7 @@ describe('captureOrder() — declined/incomplete captures must not grant access' test('capture.status "COMPLETED" still grants the tier (control case)', async () => { const payment = makePayment(); - mdl_Payments.findOne.mockResolvedValue(payment); + mdl_Payments.findAll.mockResolvedValue([payment]); mdl_UserTiers.findOne.mockResolvedValue(null); // no existing active tier mdl_UserTiers.create.mockResolvedValue({ tier_id: 99, tier: 'premium', expires_at: new Date() }); paymentSvc.captureOrder.mockResolvedValue({