Hi guys,
While working on a module based on coupons, I found (and found fixes for) a moderately serious possible exploit in the coupon system.
This is not too big a deal for most shops, unless your shop offers generous limited-use coupons, in which case it could be very serious.
This is a public forum, so I won't go in to too much detail. Essentially, a malicious or opportunistic customer with a generous restricted-use coupon can discover a way to re-use it an unlimited number of times for one product type in any payment method and regardless of logged-in state. On Paypal shops, the problem is especially serious (any number of products!), and worryingly easy to stumble on by accident.
More worryingly, I believe (I'm no expert) that in some countries, the confirm page and confirmation email legally constitute a contract, meaning some shops might not legally be able to challenge this kind of coupon abuse. They might therefore be compelled to pack and ship 500 £15 + £3 shipping t-shirts bought for £0.00 each all on the same single-use £20 coupon...
The cause is, the fact that new order_ids are created each time the 'confirm' page is loaded, plus the fact there's no validation of coupons or update to their redemption status between the confirmation page and payment. This should be enough for the devs here to figure out how to replicate the problem (hint: ctrl-T).
I spent all afternoon on this, and found two possible fixes: one good, one pretty bad but maybe better in some rare cases.
------------
The 'good' fix is also the easiest: simply apply QPhoria's patch to order.php preventing new order IDs from being created - see http://forum.opencart.com/viewtopic.php?t=9192&f=23
Easy, job done! This should seal the problem completely on every payment method, and in my testing, did so on all those I tested, including PayPal.
As you can see from the linked discussion, many people have used this patch for other reasons without trouble. In my testing, I could only find one minor issue (this occurs in some unknown, unlabelled flavour of OpenCart 1.3).If someone checks out one tab using one payment method, then checks out another tab using PayPal, money is taken from their PayPal account, they are sent a 'success' confirmation email, but no change occurs to the Order table on the DB. The ID of their PayPal purchase matches that of their other purchase (therefore legally just one contract between buyer and seller? Don't quote me on that!). This should be a minor and unlikely scenario -if someone pays you twice by mistake or misadventure on one order ID, you can see by cross-referencing the order number in the PayPal email what has happened and can re-fund them if they complain.
--------------------------------------------------------
There's also a 'bad' fix, which I developed while I thought that generating new order_ids on each load of the 'confirm' page was a deliberate essential feature of OpenCart. This can be explored if for any reason you need to keep pumping out order_ids each time the confirmation page loads.
Basically, you need to find a way to sneak something like the following code in between the customer hitting 'Confirm Order' and the chosen payment controller loading:
Code: Select all
//THIS CODE IS UNTESTED ----------------------------
if ($this->session->data['coupon']){
$coupon_query = $this->db->query("SELECT uses_customer, coupon_id FROM " . DB_PREFIX . "coupon WHERE code = '" . $this->session->data['coupon'] . "'" ));
$coupon_customer_uses_query = $this->db->query("SELECT COUNT(*) AS total FROM `" . DB_PREFIX . "order` WHERE order_status_id > '0' AND coupon_id = '" . (int)$coupon_query->row['coupon_id'] . "' AND customer_id = '" . (int)$this->customer->getId() . "'"));
$coupon_total_uses_query = $this->db->query("SELECT COUNT(*) AS total FROM `" . DB_PREFIX . "order` WHERE order_status_id > '0' AND coupon_id = '" . (int)$coupon_query->row['coupon_id'] . "'"));
if ($coupon_customer_uses_query->row['total'] >= $coupon_query->row['uses_customer']||$coupon_total_uses_query->row['total'] >= $coupon_query->row['uses_total']) {
unset($this->session->data['coupon'];
$this->redirect($this->url->https('checkout/confirm'));
// Move this string to the appropriate language file...
$this->session->data['error'] = "Your discount coupon has been used its maximum number of times while you were completing checkout, and is no longer valid";
}
}
// Any other intermediary processing here...
// If we're okay to proceed to payment method:
$this->load->model('checkout/order');
$this->model_checkout_order->update($this->session->data['order_id'], 1);
This is not at all a trivial task, as the submit button goes straight into one of many payment methods, with no common code we can hook into - not even a common 'PaymentController' class. Before I found that preventing proliferation of order_ids was an option, the method I was exploring was to print an AJAX call to this PHP code into the HTML of the confirm/guest_step_3 pages ahead of the AJAX form submit, and then also call this code from the noscript alternative (controller/checkout/payment.php I believe?). Nothing else I tried came close to working.
Any approach like this also has two fairly major but seemingly unavoidable negative side-affects for the whole shop, stemming from that line about $this->model_checkout_order->update($this->session->data['order_id'], 1);
This is essential to fix the vulnerability, else a customer can rack up any number of PayPal login pages for the same coupon on different order ids, then work through them one at a time safe in the knowledge that PayPal will never question the accuracy of the total it has been sent.
There's two big problems it causes, for all customers to the shop:
1: Any customer trying to redeem a hard-earned one-use coupon through PayPal, who gets distracted or changes their mind mid way through the PayPal process and starts again, will lose their coupon forever. If a shop is trying to reward loyal customers, this is bad marketing!
2: Any customer who abandons an order mid-way through payment will have their status as 'pending'. This might cause confusion for shop administrators if 'pending' is used significantly in a payment process.
So, this type of fix is very much not recommended, unless generous single-use coupons are essential to your business model and for some reason you need new order ids to be created each time 'confirm' loads.
The first fix, however, seems to just work. Since it has a second benefit of reducing clutter in order reports, it seems a good idea to move it into core.
