Cart Links - Missing code for "Empty cart" function

It appears that the "Empty cart" code was removed to fix the security issues outlined in SA-CONTRIB-2010-083 http://drupal.org/node/880396

More specifically:

3. The Ubercart Cart Links module is vulnerable to both an Access Bypass and Cross Site Request Forgery where a malicious user could both trick other users into adding or removing items from their cart and add items to a cart which are not published on the site. If you do not use Ubercart Cart Links module your site is not at risk to this vulnerability.

One of my shops require this. Thanks for the patch. :)

Nope. There's no way to prevent any other site from posting a link that will empty someone's cart if they click on it. I would argue that it's really not that big of a deal, but that's the way it is.

The instructions on the help page will be updated though.

Likewise, I guess hacking core henceforth is apropos at this point.

Thanks for the patch, works great! I dont see how this is a security threat though, its not like its exposing their information or charging their account. They can easily add the products back to their cart if they are tricked into emptying it. Seems like everyone knew this from the start anyway. Either way Im glad the option is still there, I hope they dont prevent it from being added back in the future!!

With regard to this being a security threat, I think the issue is that Cart Links doesn't really fire the API calls that it should. It seems to handle adding items to the cart in its own way without letting other modules alter the process. It should be a default configuration that you can't purchase files that are unpublished (unless a site admin lets it be so). It kind of surprises me that this would be concluded as a security issue and not a bug that requires patching.

Wow, crazy. I can't believe this is considered a security issue -- how is emptying a user's shopping cart a security issue?

In our use case, we send purchase links to customers via email, and always start it with an "e" to empty the cart. Removal of this code totally breaks this very useful functionality for us. Seems like a much better solution than simply removing existing functionality would be to simply default to not allowing the empty cart behavior -- force the admin to turn it on, and intentionally decide to use it. In ubercart 6.x-2.3 there was already a variable defined - uc_cart_links_empty -- and a section of the admin form to flip it. Only change necessary: set the default to FALSE instead of TRUE.

I see the OP's patch calls this variable. I also see in my diff between 2.4 and 2.3 that there's a form block in the system admin form for setting this variable that would also need to get put back. Also the instruction page needs to get modified one way or the other -- right now it lists the "e" option to empty the cart, even though that functionality no longer exists -- if we put it back, need to add a note that they need to turn on the admin setting to make it work.

I'll happily provide a patch, if there's a chance this issue will get re-opened and a patch considered...

P.S. To expand the use case, we email several different variations/recommended combinations of product options in our proposals to deliver functionality. If they follow more than one, suddenly their cart no longer makes sense, and we look stupid. That's easy enough for us to do all by ourselves... we don't need Ubercart making fun of us too!

Re-opening, to hopefully get more discussion of this/reversal of the decision.

If you really want reconsideration, this is not the place - it's a decision by the Drupal security team, so they're the ones who have to be consulted.

How do we go about asking the security team to reconsider?

I see the security problems with the other fix for this release -- about allowing unpublished items to be added to the cart. That's definitely information disclosure.

I don't see a security vulnerability here -- no information disclosure, no XSS, no CSRF, no SQL-injection, no session hijacking -- the only thing removing this functionality does is remove the ability to clear a shopping cart via a link -- which is exactly its intended purpose. The only category I can think of that might be somehow related to security in this is data loss -- of temporary data, when the admin can choose to make a cart expire through other means anyway (e.g. cart timeout). At the most malicious, an exploit of this is a minor nuisance, nothing more.

So how do we appeal this?

@freelock: I agree with you, but I don't know if there's an official appeal procedure. I guess you could start with an e-mail to the security team.

Great patch - works perfectly.

Agree with the comments here about this not really being a security exploit. I guess a small work around might be to check the referrer address to make sure its the site's links and not from an email or another site.

I guess a small work around might be to check the referrer address to make sure its the site's links and not from an email or another site.

Well, cart links are great for using in email blasts - I wouldn't want to sacrifice that functionality, but as long as the referrer check only happens if the "empty cart" option is used, then that would be a fine workaround.

Perhaps the security issue could be mitigated by displaying a confirmation page if the "empty cart" parameter is used and the user's cart isn't already empty?

Agree with TorgosPizza -- for us the whole point of using cart links is to be able to send people a payment link via e-mail. Referrer checks will fail -- people follow these links from all sorts of webmail programs, in addition to no referrer. Error messages would be majorly annoying, too.

I'll send a mail to the security team in the next day.

This is a patch, so should be marked as such.

However, the patch as listed seems untenable, since it merely restores the security vulnerability. (It's not clear to me why only 'e' was removed, but not also 'p'. A malicious user adding a $999.99 TV to your order right as you're checking out seems like a bigger problem than the nuisance of someone wiping out your list of 10 books you were about to purchase...)

In order for links to be safe from CSRF, they either need to be tokenized, or they need a confirmation form. For example, if you click on a "delete" link from a page like admin/build/block, a confirm form comes up and asks if you are sure you want to do that. This prevents a malicious user from posting an image with a src of 'admin/build/block/delete/41' that instantly removes blocks from the page if an admin views the post.

An example of a tokenized link is in Drupal 7's link to cron.php, which will end up being something like http://localhost/drupal-7.x-dev/cron.php?cron_key=3Nm66r2cp9-FeMVlHZy2R0.... This token is based off secret website information so that it can't be (easily) forged from a remote host.

developer-x pointed out to me on IRC the other day that there is also the Ubercart Add To Cart Tweaks module, which lets you configure this "empty cart first" behaviour on a per-product basis. I'm not sure, but it sounds like that could be a workaround for folks in the interim, if they can live with buttons rather than links.

Thanks for the comments, webchick!

Re: tokenized link - if the (apparent) malicious person knows the key, then they can still easily create the CSRF links, right? Or are we proposing some sort of hash of the query string - sounds a little over the top to me. Looking at the latest Drupal 7 snapshot, the code that generates the key is around modules/system/system.install (line 484):

$cron_key = drupal_hash_base64(drupal_random_bytes(55));
variable_set('cron_key', $cron_key);

Perhaps doing a confirmation step is safest. I know it would be a problem for my cart, but the 'Ubercart Add To Cart Tweaks' module would get around this.

http://crackingdrupal.com/blog/greggles/protecting-your-drupal-module-ag... is an article with info on CSRF protection. It recommends the use of drupal_get_token() rather than the D7 fanciness.

Hey Angie, good to hear from you.

Setting and getting a token is perfectly fine for links on the site, but this would mean destroying the functionality of having a static "add to cart" link in something like an email campaign, that would allow a user to click on it and immediately add the product to the cart. It's a small thing (minimizing the number of clicks to an end point is always a reasonable goal), and perhaps the workaround is simply to "not do that anymore," but I'm still waffling about that.

The issue is when the cart link is created, if we're using drupal_get_token() it will create a token based on my current session ID. If I send that cart link to another person (or 30,000 people, in the case of one of our email campaigns) none of those links will be viable, and each person who clicks one will get a validation error. (Unless I'm mistaken, which would be great.)

If the only way around this is to put links to the product page itself, and not to an "add to cart" link that triggers the action of adding a product to your cart, then so be it. I think the only way to fix that would be an additional callback that creates a token for the user clicking the link, which then fires the cart link, but that seems awfully redundant.

I'll write a small patch with this functionality and see if I can get it working in something like an email blast, which if it does work, would solve the issue at its core.

I'm still quite concerned about the usability of putting additional steps between the user and a sale. However, I think the key difference of opinion that we have here is that I don't consider the user's cart to be a security concern in any way, shape or form, until they check out. At that point, they're entering their credit card for specific products and services, and we need to be careful.

With the previous behavior, we don't bypass the CSRF protections of the checkout form -- where this matters. In our use case, we send a link that populates a user's cart with a specific set of products, and take them straight to the checkout form. Users cannot bypass the checkout form -- they clearly see what they're buying at the top of the form when they check out. If the token system is working correctly, if something else gets added to their cart on a different browser window, the CSRF protections should kick in when they go to submit the checkout form -- I'm all in favor of testing that behavior to be sure that's the case.

I understand that some store administrators may have a different opinion and prefer users to specifically add products from within their site, treat the cart as a security concern--which is why I'm suggesting making this behavior optional. Which it already is -- users don't have to enable the cart_links module.

What's next -- are we going to force all links to go through the form API? Are we going to block all inbound deep-links from outside our sites? Take a look at what you're trying to protect -- it's a shopping cart. Most of them get abandoned. All of them are temporary. As long as we make sure the checkout form is secure, and we protect cross-site scripting/javascript injection attacks (which this module does well) who cares about what's in the checking cart? I'd argue that an empty cart link is more secure than an add-product-to-cart link (as Angie does above) but if you're going to remove that, you're taking away the whole purpose of this module!

There's lots of reasons why administrators might want to populate a user's cart from another site, or email. There's not a whole lot of usefulness in cart links within the same site -- there are so many other ways to add products to your cart if you're already on the site.

Webchick's comment in #18 misses the mark:

However, the patch as listed seems untenable, since it merely restores the security vulnerability. (It's not clear to me why only 'e' was removed, but not also 'p'. A malicious user adding a $999.99 TV to your order right as you're checking out seems like a bigger problem than the nuisance of someone wiping out your list of 10 books you were about to purchase...)
...
This token is based off secret website information so that it can't be (easily) forged from a remote host.

The issue here is how does the malicious user benefit, and can the user be tricked into something bad? First off, the malicious user can't get money sent to them -- the money still goes to the shop owner. Secondly, the malicious user cannot get the TV -- it goes to the customer. There's no way for the malicious user to exploit this to gain anything -- they can't inject malicious Javascript (the cart links only empty the cart or load existing, known, and now published items in the store), they can't trick the user into going to a different site (again, at least not through this "vulnerability" -- though we might want to test the destination redirect to be sure), they can't bypass the checkout form, they can't gain access to the user's session, user account details or anything.

Meanwhile, the ability to send a user straight to a checkout page with a selection of products from a remote host is one of the biggest selling points we've been using for Ubercart -- that's a feature lots of people want. We're working on integrations with some accounting packages to send out invoices with cart links for payment. The checkout page shows exactly what's in the user's cart, and the functionality in this module cannot bypass that.

To use the REST metaphor, we see creating a cart as GET, basically assembling a collection of items to use in a POST -- the actual checkout. This module merely exposes that process to a URL pattern -- but there's no skipping the POST for any actual action on the site. And disabling this functionality is like not allowing deep links into your site -- it actively breaks functionality, and does not provide any security benefit.

And while I'm ranting, here's another equivalent:

http://my.site.com/user/login?destination=myevilpage

... is that a security issue? We're not using a token to validate that the user is getting redirected by our site. Oh, no! CSRF! That's the level of "vulnerability" we're discussing here.

Freelock,

I think you hit all the marks pretty well spot-on. At this point I feel like the patch should be reversed and put back into core, and instead some documentation put in its place. Or something - how do you suggest we reverse this decision? Were you able to contact the security team? Perhaps an in-depth explanation such as yours above would help to convince them that it was a silly decision in the first place.

The one thing I would keep about the issue would be to make sure we are adding products that are published, and any cart links attempting to add an unpublished node would be rejected.

The only other thing I can think of would be to check for a token only if the "e" parameter is set, but the scope of that Security issue seemed to be under the impression that "adding and removing products" from a person's shopping cart is also a threat, when the majority of us seem to think otherwise. Perhaps we could get greggles or someone else involved in the reporting of that issue in on this conversation?

Why don't we do it like the Drupal's user registration, whereby a random token is generated, and the link is only active for example 24 hours (configurable).
And the link will be removed by cron jobs automatically after expiring.

So the link is cross checked with db when the request is made.

Instead of
<a href="http://www.example.com/cart/add/e-p1_q5-imonday_special?destination=cart">Link text.</a>
It'll look like
<a href="http://www.example.com/cart/add/582da37be0b057162f09fe0cadaf526ea6ccbb00">Link text.</a>

That should make it "not-so-vulnerable" since the token is generated internally by Drupal?

@ktleow,

Not a bad idea, however, if we use the Drupal token system (drupal_get_token, drupal_valid_token) - we'd have to use token validation with $skip_anonymous = TRUE, though, but I'm not sure how much of this we want to put into the Cart Links system. Worth considering I guess.

This is for reasons I explained in #21 - the token system in Drupal uses the current user session, so that someone can't hijack a request. This doesn't do us any good if we want many users to use the same token.)

EDITED for clarity and after I looked at drupal_valid_token and noticed you could set $skip_anonymous. That might solve the issue with email campaigns, but again, we'd have to find an easy way for Admin Assistants to grab a "tokenized" link. I'm also not 100% sure the $skip_anonymous option would fix this issue for email campaigns and the like, but it'd be worth a shot.

@torgosPizza:

I have took the time today to create a module to enhance uc_cart_links. I'm not using drupal_get_token / drupal_valid_token since they rely on sessions (that would mean every user will see a different cart link?, we wouldn't want that to happen).

I'm using Mcrypt to do the encryption and decryption.

What do you think about this approach?

Before: http://ubercartsite/cart/add/e-p1_q1
After: http://ubercartsite/cart/buy/ZP01bIuNq_47_pxdkmaTNaKsvsSTEaoeHXmGMQon7xY...

cart/buy is the new menu callback to process this new request.

Are there any complications or issues with this approach? Ideas are welcomed~

EDIT: I'm using combination of Mcrypt with drupal_get_private_key salt.

If emptying the cart by a link is considered a CSFR vulnerability, then so is adding (or removing) single items. By this logic it seems that all Cart Links of any kind are a "secuity issue". [Sorry for the useless post, but it seems there's still no way to subscribe to an issue on D.o without posting a comment?]

Here's my very first attempt on creating a module to enhance the UC Cart Links

http://drupal.org/project/uc_better_cart_links

@SeanA
Yes I have the same thought as well. Why only 'empty' cart links was removed? This is really weird..

I agree that functionality should be restored. Here's a more complete patch to do that, which restores the admin setting and leaves an empty uc_cart_links_update_6001(). The fix for unpublished products remains.

As for the question of should this be restored, I read everyone saying things about the mechanism by which the cart links function, but not the intent. No matter what tokens may be placed on links, if they are sent out in an email blast, forwarded, etc, the end result is that an off site link takes an action. By adding tokens you're effectively "signing" a given cart link as "valid according to the site admin". I argue that this feature already exists in cart links via the "Cart links restrictions" admin setting. Why involve the complexity of tokens?

Why should we use tokens, or something like it? One thing that could be considered a weakness is that cart links are "guessable", if this feature were to be reinstated as is without tokens. I'm not so sure about the encrypt/decrypt approach of uc_better_cart_links. Perhaps more appropriate would be to "sign" the links (in the typical cryptographic sense). If this were done someone who didn't receive the email blast may be able to guess the base part of the cart link, but would be unable to guess the signature to make it function. Perhaps this is where a confirmation page could appear when a signature is missing or not valid. And the page should not be "DANGER You've clicked an unsigned link...", but simply "Are you sure you want to...?". But, IMHO this is waaay over engineered and my vote is to just restore the feature as it was.

[edit] Added some markup.

@cedarm: I mostly agree with you, but the fact remains that the Drupal Security Team has vetoed emptying a cart like this. Bottom line, it's not going to go back the way it was. The only way to move forward is to implement something like tokens. I'm not saying I think tokens is the best way of doing things, but it *is* an alternative that will probably pass the Security Team's review.

Ok. I've been sidetracked with (a lot of) other things... I just wrote a mail to the security team asking to appeal this decision:

Hi,

A few months ago the security team made a decision to ban the "empty
cart" action in the Cart Links module. I believe this decision was made
on faulty grounds -- there is no security issue with this feature, and
removing it is harmful to many e-commerce sites.

Is there a process for appealing your decisions?

The issue is that the security team removed the ability to empty a
user's shopping cart via a link. This is a necessary feature of the cart
links module -- which allows products to be added to the user's cart via
a link. Adding items to a cart without being able to make sure it's
empty first makes this entire module useless.

There are several reasons the decision is faulty:

1. It breaks an important use case: the ability to send out payment
links for e-commerce sites. For example, companies use this feature
every day to send a payment link for services to clients. If you don't
empty their cart first, you end up with a cart full of extra items, and
customer complaints.

2. You have removed one feature that changes the state of a user's cart,
without removing the others -- no consistent criteria here. If emptying
a user's cart is a security issue, surely adding a product to the user's
cart is exactly the same security issue -- you can modify the state of
the user's cart from a different web site.

3. There is no security threat here. No session hijacking. No
possibility of gaining administrative access to the site. No CSRF, no
XSS, no information disclosure, no SQL injection, no nothing. All this
does is allow a pre-defined action to take place on temporary data --
the user has to confirm their order on a review page with complete
details of the transaction, before anything permanent happens (like a
credit card charge). At the very, very worst, an attacker might use a
different vulnerability to trigger this feature as a nuisance to a
customer -- and that's the worst thing they can do with this
"vulnerability." Furthermore, the administrator of the site can turn
this functionality off.

Please read the discussion at http://drupal.org/node/881752 , and
re-assess this issue. It's a complete non-issue -- as dangerous as
having a predictable path for anything, like "node/add/page." And it
takes away a very useful feature. One other use case: a business sending
a newsletter containing a cart link for a special product they want to
highlight -- this feature allows their customers to get to the checkout
page with a single click.

--
John Locke

Webchick's comment lays out the problem and potential solutions.

The module could create a hash (based on something private to the user, private to the site, and specific to the action) and store it like comment_notify does.

Another idea is to just use a confirmation form for this operation then you don't have to mess with tokens in the mail.

If the ability to add items to the cart is still possible then that should be fixed as well.

Hi,

Can somebody on the security team please point out how an attacker can possibly do anything malicious to the site, or even attack a site visitor, via adding a product to the cart or emptying a cart from a remote site?

This is very useful functionality, and I fail to see any vulnerability whatsoever. Please explain. As I see it, the whole purpose of this module is to allow adding/removing items from a cart from another site. It's relatively easy to do this already on the existing site. As this module is not a requirement for any other module, and administrators don't have to enable it, why gut its functionality entirely?

Tokens are not a reasonable solution -- because this isn't really the problem the security team seems to think it is. There's not even a theoretical attack here that I can see -- because you can't skip the checkout!

Webchick wrote:

In order for links to be safe from CSRF, they either need to be tokenized, or they need a confirmation form. For example, if you click on a "delete" link from a page like admin/build/block, a confirm form comes up and asks if you are sure you want to do that. This prevents a malicious user from posting an image with a src of 'admin/build/block/delete/41' that instantly removes blocks from the page if an admin views the post.

The Checkout page IS the confirmation page. And there's one more confirmation page after that, the Review Order page. You cannot skip either one through a cart link. Cart data is temporary, transient -- it's not something that needs protection before the checkout page.

... and to follow up on my previous comment, any admin who DOES think cart data needs to be persistent and not changed from other sites or emails, doesn't have to use this module.

Obviously, I'm not going to apply patches that remove this functionality, and will fork this module if necessary. If it's not welcome on d.o., admins will be able to find it on my site.

@freelock, the reason emptying the cart is a problem is that it is a destructive, irreversible action. Also known as a "data loss" bug. The security team's philosophy is to think of the worst case scenario and assume it can happen. Imagine someone spent a month putting together a product combination with specific attributes and quantities. Then they visit a malicious site (perhaps looking for a coupon code?) and now their whole cart is gone.

As you and webchick and others have pointed out, adding items to a cart is not something that can be used in a clearly malicious manner though it could be awfully confusing to end users. I think it's not awesome to leave adding items to a cart as a CSRF it seems more or less acceptable. As I commented in #34, there are safer ways to achieve that functionality that should be used.

In my opinion, this issue should be marked as "won't fix." If you do provide a fork version of this somewhere else you should be sure to add a warning in the README.txt of the module that explains the potential for problems.

Edited typos

@greggles: Must the hash be based on all of "private to the user, private to the site, and specific to the action", or would a hash based on only "private to the site and specific to the action" be acceptable?

would a hash based on only "private to the site and specific to the action" be acceptable?

@cedarm: It would have to be. Assuming all cart links were tokenized (not just the ones that are emptying carts, although I would prefer that) making the cart links private to the user, without $skip_anonymous = TRUE, would render all cart links sent in an email campaign useless.

@torgosPizza, let's discuss in irc, maybe? As I mentioned in #34, the comment_notify module does the thing you say is impossible...

@cedarm, if it's going to be effective against csrf then yes it has to be. Otherwise someone could get the link for themself and use it on someone else.

@greggles, thanks for taking the time to reply:

@freelock, the reason emptying the cart is a problem is that it is a destructive, irreversible action. Also known as a "data loss" bug. The security team's philosophy is to think of the worst case scenario and assume it can happen. Imagine someone spent a month putting together a product combination with specific attributes and quantities. Then they visit a malicious site (perhaps looking for a coupon code?) and now their whole cart is gone.

On that reasoning, the feature built into Ubercart to expire carts after x hours is also a data loss bug -- it removes the cart data on a cron job.

How is this feature substantially different? See admin/store/settings/cart/edit/basic , and you'll find options for the site to destroy the cart on a schedule. By the reasoning you list above, this could be terribly confusing to users who take a month to add something to their shopping cart, if their cart times out after a week, or an hour.

That's not data loss, that's a feature available to administrators who want to use it. Why disallow this one?

@greggles: How can this be done when the person the tokenized link is sent to will hit the site anonymously? The only thing that identifies the user is the anonymous session that doesn't yet exist at time of token creation. Am I missing something?

freelock, there is an order of magnitude difference between an administrator intentionally configuring a setting to expire data on a given schedule and a module which opens the ability for a random malicious user being able to trigger this behaviour with neither admin nor end-user knowledge. You're arguing that this particular CSRF vulnerability is a feature, not a bug, but that doesn't change the fact that it's still a CSRF vulnerability.

This is really quite simple. We have well-established patterns on how to enable this sort of functionality safely and securely, and UC Cart Links isn't using these patterns. So let's figure out how we can resolve this situation, rather than arguing back and forth over the egregiousness level of this particular bug.

Incidentally, here's the approach that comment notify takes:

function comment_notify_menu() {
  ...
  $items['comment_notify/disable/%'] = array(
    'title' => 'Disable comment notification',
    'page callback' => 'comment_notify_disable_page',
    'page arguments' => array(2),
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK
  );
  ...
}

function comment_notify_disable_page($hash) {
  db_query("UPDATE {comment_notify} SET notify = 0 WHERE notify_hash = '%s'", $hash);

  drupal_set_message(t('Your comment follow-up notification for this post was disabled. Thanks.'));
  return ' ';
}

However this still seems to suffer from the same issue described above somewhere, and reiterated at #43. If sending bulk, non-personalized e-mails, everyone's going to get the same hash in the URL and hitting it for one anon user is the same as hitting it for all anon users.

Hm. Although. For emptying carts, does that matter, really?

@cedarm: the code for generating the token in comment_notify creates a token that is specific to the user receiving the e-mail. Then the fact that they get the e-mail is assumed to mean that they are the right user. This level of trust for the e-mail recipient as user is similar to the way core sends a lost password e-mail and trusts that the person following the link is a real user.

Right now most bulk mailing programs send user-specific tokens to track open/click-thru-rates, to personalize the salutation, etc. It's not always super easy to add a user-specific link that contains a token in it to clear the cart but it is possible.

The alternative is to use a confirmation form before clearing the cart. This could be displayed only if there were items in the cart to reduce the number of people who have to see it.

@greggles: Makes sense. So if an admin wanted to misuse the system they could send the same tokenized/hashed link to their entire list, effectively creating a csrf situation, but the code itself when used properly would be secure.

The difficulty now becomes creating these many tokens and relaying them to whatever/whoever sends mail. In our case it's a third party and will add considerable complexity to our processes. An alternate solution would be for the third party to make an (authenticated) web service call to Drupal to get a token on demand. Any other options?

One other item to note. The behavior should probably be different than lost password e-mail with regard to one time use. This would be much too strict, but a reasonable expiration date would probably be ok. Thoughts?

As I think about it more, the tokenized approach that comment_notify uses would only work for registered users who have a cart associated with their logged in account.

One other option is adding a form to the process if there is something in the cart. The form API (when used properly) adds a token to prevent against CSRF.

Regarding using a link more than once, I don't think that works since it is tied to their cart.

cedarm, what's the use case / actual link you're passing in your mass emails? I was trying to come up with a reason you'd want to empty a user's cart without their consent in a mass email campaign and came up short. Having some example here might help with coming up with an implementation idea.

In our case, we were building a recurring subscription-based website and didn't want someone to accidentally have both a $20/month *and* a $50/month product in their shopping cart. We also only sold subscriptions. So it made sense to remove all cart contents before adding another product. But a token-based approach would be fine for those purposes, since the user already has a session in order to click the link on our site. (We ended up going with UC ATC Tweaks module and just use buttons there instead of links, which has tokenization built-in through the Form API.)

Our use case is the same w/ subscriptions. If one were to market both subscriptions options in an email a user may click one link, read and close the window, go back to the email and click the other link. Another very important item is we don't want a user buying qty 2 of a subscription, but we wrote a uc_onlyone module that basically removes the qty field for the subscription product class and forces qty 1.

We've never had to deal yet with this use case together with normal products, where emptying the cart would be undesirable. I've been thinking about this off and on so as to be prepared for whenever it comes up. The thought is the idea of conflicting products. "If you want to add that item to your cart you must remove this other one... which one do you want to keep?"

Ok, so I understand the problem. But not the proposed solution. You want the same behaviour (emptying cart and only adding the new product) regardless if someone's adding a product through the website or by clicking links in email. So why wouldn't you build the link to simply add the product to the cart and use something like UC Add to Cart Tweaks or a custom hook_uc_cart_something_something to ensure that regardless of the method, this "empty the cart first" logic was first applied to the product itself?

This would also solve your problem of different product "types" as well; in a hook you can do whatever logic you want, and UC ATC Tweaks can be applied on a per-product basis.

Maybe what's actually needed here is to move at least part of UC Add To Cart Tweaks into Ubercart "core"; a checkbox on a product class for "allow only one of this type to be added to the cart."

That solves the problem regardless of how something's being added to the cart, and then we don't even need 'e' anymore. No?

Hi,

I'm sorry, I'm not arguing the severity of it being a CSRF vulnerability, I'm saying that the main value this module has is the ability to make cross-site requests. Specifically, the ability to have a short-cut to providing affiliate links, payment links from other systems, promotional URLs/landing pages, and much more.

The "vulnerability" we're talking about here is adding and removing things from a shopping cart.

If this were a physical store, it's certainly possible to slip something into somebody's shopping cart, or take stuff out. What happens? If the customer misses it at checkout, they can come back later to rectify the situation -- at the very worst, a minor nuisance. If your typical shopping cart has only a handful of items, it's not likely that users are going to miss a cart contents change -- the least sophisticated online shoppers carefully review the checkout page before typing in their credit card.

It's not a vulnerability for the store -- they get paid for everything they sell. It's not a benefit to an attacker -- the known information disclosure issues were fixed, they cannot hijack the checkout process, they cannot inject a bogus address or credit card. At worst, it's a minor, easily corrected nuisance for the customer -- and believe me, we've found far greater nuisances in Ubercart than anything like this. (Try checking out with Javascript disabled, for example -- or with Safari and lots of shipping/tax/gift certificate modules).

Lots of stuff in Drupal can be configured to expose far more data loss issues than are possible with this module. Grant delete rights to a content type to the anonymous user, for example. We don't prevent administrators from configuring that.

On the flip side, there are many great user scenarios for marketing that this module enables. Things like payment links for invoices, or email promotion blasts, or marketing promotions for partner businesses, or quick-and-dirty affiliate links. We use it to provide several different package price scenarios for bidding specific projects.

You are taking away a really great feature of Ubercart simply because an attacker can possibly use some other medium to trick a customer into adding or removing things from their shopping cart? You seriously think shopping cart contents is as critical to protect against all possible attacks as web site content?

In my experience, security must be done in the context of value of the target as well as ease of exploitation, cost of mitigation, and cost of having an exploit. In this case the value of the target is trivial, the cost of having an exploit is zero, it's quite difficult to exploit in practice (if you don't agree, figure out how you would use this to trick my customers into not being able to pay my invoices!) and very difficult to mitigate while preserving functionality.

One of the great things about Drupal is that in general, it allows knowledgeable administrators to make reasonable decisions about how to configure their sites. Lots of modules can be easily configured to allow information disclosure (views), unauthenticated access (services), data loss (core node permissions) and much worse. This vulnerability, in contrast, at worst is a nuisance, does not have any meaningful risk, offers some very, very useful features to administrators who want to use it, and is completely optional. Why are you taking that option away?

I'm shutting up now. If this inane decision stands, I'll provide a forked version on my web site by next week to anybody who needs the old functionality.

So... Everyone else in this issue seems to be trying to constructively come up with a solution here. It'd be nice to see you participating instead of throwing rocks, freelock.

For example, outlining your use case and providing feedback about whether some of the counter-solutions would be an option?

Here's the use cases I'm already using this module to do:

1. Invoice payment links generated from another system.

   * Accounting system generates a simple product link with this pattern:
   empty cart - add invoice sku item - set invoice # attribute - set amount attribute - redirect to checkout page - set source to the invoice system name for reporting purposes.

   In this case, a single product is being added to the cart and a custom price module essentially copies the amount from the attribute into the product price.

   It might be possible to modify the invoice system to request a token from Drupal, and build it into the link, or designate the single product to empty the cart before proceeding. But that's now going to take a lot more programming in the external system, involving interacting with the external Ubercart site instead of just filling in a simple pattern locally.

2. Purchase scenarios for providing alternative payment plans when doing a bid.

   You can see this in action on my site right now -- for example, we've got several payment plans for doing our base e-commerce plan. You can check them out at these convenient payment links:

   • Purchase our E-commerce package with extra 25 hours and mid-level plan
   • Purchase our E-commerce package only with minimum plan

   Having these cart links URLs available and published are not a security risk for my site -- the only thing an attacker can do is entice somebody to buy something from me! When the "empty" option disappeared, these payment links got a whole lot more confusing when products started multiplying in their carts...

   It's a whole lot easier to close sales when you can send people a payment link that works, takes them straight to the page where they fill in their credit card. For my business in particular, users never buy after manually adding items to their cart, browsing our site -- unless I'm directing them over the phone. The vast majority of our sales are the direct result of following a payment link like the two above. There's not even an opportunity for anybody to do a CSRF attack! So having to generate unique links for every customer is a cumbersome burden that makes the links subject to failure -- often we get people mulling over our proposals for a few weeks before deciding to buy, and I can tell them, just follow the link for the payment option you want, and check out.

3. Cross-promotions, partner promotions. We're working with a company now that carries a variety of products, and another that's starting to look into affiliate sales. Besides the two use cases above, the next logical step to me would be to be able to recommend a particular set of products from a partner site, and be able to send customers over to their partner's site with a recommended set of products added to their cart. As businesses struggle to find new ways to reach customers, these types of deals are becoming more necessary -- and this would add a whole lot of friction to the process of creating a cross-site link. (I know, there's the uc_affiliate module, but that brings a whole lot more complication to the process).

I can definitely see that if you're selling generic products on a site where users spend days or weeks tweaking or customizing something before purchasing, you may actually care about losing cart data. We have one site that might fall into this category. So we don't use cart links there, and it's not vulnerable.

For the rest of the use cases outlined above, what we're doing is equivalent to a deep link into the site -- a link straight to the checkout page or cart page with a pre-selected set of products. On these sites, storing cart data is completely irrelevant -- users rarely add items to their cart without assistance anyway. That's why, for the sites we choose to use this module, we don't consider CSRF to be a vulnerability -- these are all legitimate cross-site requests meant to populate the cart, and the checkout is the confirmation page. Sure it can be forged -- but with absolutely no bad consequences.

Cart links without tokens are a simple, elegant, useful way for an administrator to allow deep linking directly to a checkout page or cart page, with a cart filled with hand-picked products. Having to generate tokens for each user and/or purchase would introduce a whole lot of friction into the process, and make it harder for ecommerce sites to experiment with cross-site promotions -- and I hope you can see that for some (many) sites the ability for malicious sites to change cart contents is a total non-issue.

I'm open to suggestions for an alternative that will reliably populate a user's cart with a specific set of products, not multiply them, can be easily generated and sent via email from 3rd party systems, and pasted into 3rd party sites. Because that's what we rely on this module to do.

Thanks, those use cases make sense. But can you explain why #53 would be an inadequate solution?

Well, for partner promotions, there might be any number of products they would want to recommend to go along with their services.

For our "invoice" product from our accounting system, just today I sent a payment link to a customer with two different invoices added, each with a different amount/invoice #, to pay them both in a single transaction.

And our product bundles might include any combination of a monthly support plan, a bucket of hours, and a fixed-price package -- setting only one of these to empty the cart would not reliably do the right thing for any situation, and setting more than one would definitely do the wrong thing.

Certainly not against enhancements to uc_atctweaks, but it's definitely inadequate for our needs.

Ok, here are some alternative approaches that would address our needs.

For #1, creating invoice links. Create some interface for generating a token from a 3rd party site. I'm thinking something that uses Services module to provide authentication and the framework, and then will generate a token the 3rd party system can include in its link. 3rd party system needs to post item, attributes to service and get a payment link back that can be used by anonymous users to reach the checkout.

As an add-on interface for those 3rd party accounting systems that can't be so easily integrated, at least have a link generation page an admin can use to create a payment link, properly token-ified.

For #2, the real goal is to provide pre-constructed carts, or orders. The admin ordering interface currently sucks to use, but the real problem is, you can't create an order and send it to a customer who can then check out with it -- there's no way (at least not that I've seen) to send somebody a populated cart other than with cart links. We have several customers who currently use Masquerade to handle telephone orders because the admin order interface doesn't work well -- but the lack of a way for a customer to complete payment on an order created by the admin is a serious feature gap in Ubercart that cart links partially mitigates.

Ubercart really needs a way to store multiple carts, and transfer them to other users. And orders.

For #3, being able to store a cart and share it with a friend is what we're after. In fact, cart links might be the start of exactly that -- there's no user-friendly way to generate those links currently, but a "cart link builder" and some persistent cart system might be the solution we can all live with.

Ok. So here's what I'm picturing:

1. On the cart page, cart links can generate a URL link that visitors can cut and paste to send to anybody they'd like to send their cart to, if the admin allows.
2. When any cart link is followed, if the user has any items currently in their cart, their current cart link is stored as a "previous" cart and a new empty cart is populated from the link.
3. The cart page shows a history of previous carts, back to a configurable time or count. Users may swap their current cart with any in their history, by simply following one of the links.
4. Any or all previous carts can be easily deleted.
5. On checkout with payment, the current cart is emptied along with any matching previous carts. Other carts remain in the history.
6. Provide users with a link to "save my cart for later"
7. Provide admins with a configurable security option -(a) "Private cart links" that include a user-based token that cannot be used by other users - no sharing, but cart history -(b) "Sharing cart links" that uses a hash to obscure links and associate with a particular user account, allowing them to be canceled and/or user account blocked, and the source tracked -(c) "Open cart links" that uses something similar to today's links, possible to construct from third party systems with knowledge of product nids and the pattern, skips tokens and hashing entirely
8. Perhaps even provide a permission for "Sharing cart links" to restrict that functionality to certain roles when the "Private cart links" is otherwise selected -- to allow members of a role to generate links that can be used by others, while only allowing regular users the cart history.

... this assumes that the security team will accept "open cart links" as not a data loss vulnerability, because the previous cart is stored instead of clobbered.

That's what I would like to see, would meet my current needs and make it much easier to use. It also opens up more viral marketing opportunities for e-commerce companies, as well as solving a couple of glaring missing features in Ubercart.

But it's also quite a bit bigger chunk of work than we're discussing... and we'd be happy to develop it if I can find a client to foot the bill...

I think the examples in #56 would all work with a confirmation form that only showed if the user had an item in their cart. It would reduce sales ever so slightly...but...

I suggested a confirmation form way back in #16, and I still think this is the best solution here. A patch is attached that restores the empty cart flag and adds a confirmation form if the flag is used, the feature is enabled by the administrator, and the customer already has items in their cart.

I don't think adding a product to the cart needs similar protection, as it doesn't cause data loss and it's the customer's responsibility to check their cart contents before making a payment.

While I still think this is inane, this is at least a solution I can live with.

Patch applies, works as expected -- shows confirmation form if anything is in the cart, skips it if the cart is empty. You can see it in action now on my previous cart links.

Anybody else interested in a cart history/link builder?

Thanks,
John

@freelock: Imagine one of your competitors includes <img src="http://www.freelock.com/cart/add/e" /> on every page of their site. A prospective customer is browsing both sites simultaneously, trying to make up their mind who to go with. Without this patch, their cart will be emptied without warning. You sell e-commerce services; wouldn't they be more likely to pick your competitor if your site doesn't seem capable of managing customer carts correctly?

Could we insert a check before displaying the confirm form that the cart has products in it, as greggles suggested in #60? That'd probably greatly reduce the number of people who see the message.

@webchick: that is already checked for in the patch, if the cart is already empty then the confirmation form is skipped.

@webchick, I think that's in there:

-  // Split apart the cart link on the -.
+  // Confirm with the user if the form contains a destructive action.
+  $items = uc_cart_get_contents();
+  if (variable_get('uc_cart_links_empty', TRUE) && !empty($items)) {
+    $actions = explode('-', urldecode($arg1));
+    foreach ($actions as $action) {
+      $action = drupal_substr($action, 0, 1);
+      if ($action == 'e' || $action == 'E') {
+        return confirm_form(array(), t('The current contents of your shopping cart will be lost. Are you sure you want to continue?'), variable_get('uc_cart_links_invalid_page', ''));
+      }
+    }
+  }
+
+  // No destructive actions, so process the link immediately.
+  uc_cart_links_process($arg1);

Just noticed the comment above that block should say "if the link contains a destructive action" rather than "if the form", not sure what my fingers were thinking when I typed that :)

@longwave it's a question of severity, and resources -- there's far more usability issues with Ubercart than some intentional buried attack on a competitor's site which will show up in referrer logs and be a minor nuisance -- again, on the sites we use these links on, most customers never use their cart except by following a cart link. We set up most of our cart links to take you straight to the checkout page.

If our customers find their cart contents changed, they just go back to the original payment link and follow it again. But there's a long list of far more confusing issues:

* page caching making anonymous cart blocks useless, anonymous users can't see their active cart
* totally broken uc_gift_certificate module
* checkout page where every address change triggers 4 - 8 Ajax requests, many of them identical, which Safari particularly seems to have trouble with (and some of those don't use tokens!)
* uc_cim module stores credit card numbers for anonymous users, until Authorize.net cuts it off at 10 (yikes! These are all attached to user 0! <-- Security team, take note...)
* uc_recurring api keeps changing, no way to fix a broken recurring transaction (e.g. provide an updated credit card), missing notifications when recurring charges fail
* uc_coupon multiplies the discounts when attached to a single product but user has multiple products in the cart
* users can't edit product attributes after adding to their cart

Those are the top Ubercart issues we're dealing with right now, along with some customer checkout complaints we haven't gotten enough info to be able to reproduce, and probably 3 or 4 others I can't remember right now.

That's why I get cranky when something that actually was working great suddenly breaks, for such a comparatively trivial issue -- it means a whole lot more work I'm not getting paid for, added to a task list that's already too long! But a confirmation screen works for me, all our outside integrations/current functionality works without modification.

Wow. I apparently did not eat my Wheaties this morning. :)

@freelock: About uc_gift_certificate... if there's anything I can help with let me know. I've been too busy to work on that module, but if there's anything really broken, send me a PM. The module itself needs a complete rewrite (I didn't write the original, just took it on as a maintainer.) Anyway, sorry for the digression. This patch looks good to me.

@torgosPizza it's on the back burner, client's out of budget, our most recent patch is working well enough for now, aside from admin orders... We're totally mercenary these days, we work on things clients pay us to work on! So we'll be back with patches as our clients prioritize.

I think freelock's comments in #59 really hit on what's missing, and why we've all used empty-add links to work around it. We need Ubercart to have a pre-constructed cart/order feature, which probably necessitates support for saving carts and/or multiple carts for a given user/session. And again for us the idea of conflicting products is a missing feature. Until someone writes these features there are a few different options:

• Use the patch from #61 - restores functionality but with confirm form if cart is not already empty.
• Use Ubercart Add To Cart Tweaks module - empty cart on a per product basis, uses buttons instead of links or URLs.
• Write your own page callback that does what you want to the cart.
• It's open source - add the "e" feature back to the code your own site runs and live with the security risk.

I would however like to propose an extension to this patch, if it can be accomplished without much complexity. I'm assuming my typical use case of "empty-add single product". If the cart currently contains node x and the cart link says to empty then add node x, can we bypass the confirm form? What about cart links that add more than one product?

Hi,

I created #966914: Cart History as a new issue to discuss my proposal in #59.

@cedarm, the current code iterates through each part of the cart link sequentially. Longwave's patch added a preprocess function to handle emptying the cart -- it shouldn't be too hard to put some code there to compare the items in the link with the current cart, though it's probably a bit of a repeat of what's in the process function.

I'd suggest we should create another function to compare a cart link to the current cart -- basically something that serializes the URL into a cart object of some kind. It can then be used as the base for some of the new features I described...

I'd also suggest we let this issue get resolved/fixed with longwave's patch in #61, and move further discussion over to the new feature request issue...

Cheers,
John

cedarm, your proposal makes sense to me but I don't know how much work it is. If the cart is the same before and after then there's no need to show a confirmation form (nor change the cart).

Another issue that's been two weeks at RTBC with no feedback from the maintainers; I wonder why I even bother contributing to this project any more.

Yeah, I'd love for some of these patches to get into core, at least before the next -dev release. I'd like to not have to remember all the patches next time I update my modules.

OK, it's finally committed.

Needs porting to 7.x.

Here's a patch for 3.x.

Since this is also a function i need, but what didnt work i landed in this issue topic.

This is a hard one... Putting a form in between just doesn't do it. The Add to Cart Tweaks_module... well actually also doesn't do it since when someone knows what products clear the cart they can do again exactly the same thing like e would. That being said, this is also an security issue in Add to Cart Tweaks_module.

I haven't made my mind up if there's a good way to fix this, i personally would suggest a history (i think that is also been suggested?). That would fix the 'data loss' bug and directly make this work.

Tokens are useless in this case.

The Ubercart Add to Cart Tweaks_module doesn't even seem to work when using links.

@lasac: What do you mean by "putting a form in between just doesn't do it"? This adds a single click to the process only when the user already has items in their cart (which should increase user confidence in the store, if anything!), is the only way to prevent malicious users emptying carts using code similar to my example in #63, and the security team will not allow the original code to be reinstated.

Committed the patch from #79 with two more minor fixes (wrong form function signature, uninitialised $id if no link ID specified).

@#82: sure it will do it, as in, be accepted by the security team. But as stated before, it's just another obstacle in the checkout process.

Haven't looked at the patch, but i gues it's something linke 'this will clear your cart'. User will have the option 'Ok' and 'No thanks'. What if they choose no thank? Does that not place the new product, or does it place the new product but just not clear the cart?

@longwave Would you mind posting the final version of your patch, with extra tweaks, mentioned in #83?
Thanks a lot for this work!

@checkmark: Those fixes are all in the beta2 release. You should upgrade now.

Thanks TR, that's good to hear. However, I need to patch 6.x-2.4. Is the patch in #61 up to date?

I assumed 7.x-3.x because that's the version on this issue and that's what longwave's patch in #83 was for ...

Yes, #61 appears to be up-to-date. You could always just grab the entire uc_cart_links module out of 6.x-2.x-dev and use that to make usre you have everything.

That's a great idea. Thanks TR.

It looks like I'm late to the discussion, but I have a use case where I don't need the confirmation page and it's not a security issue for me.

My homepage "signup" button is a Cart Link that takes you straight to checkout. There's only one product I sell on my site, and UC Attributes in Cart lets users select options during checkout. I would love to know if there's a better way to always have this product in checkout. Right now, I'm using Cart Link to 'reset' the cart, so everybody gets the same cart. It's awkward when a user exits checkout and returns later, then gets asked to empty a shopping cart, which up until now they'd never been told about.

I totally agree, and started another thread at http://drupal.org/node/1091166 to follow up on this issue, since this thread is so long.

First off, I'm using Ubercart-3.x-dev and the code from #79 seems to be in my code.

Problem is it's not working. I have to comment out the confirm_form() to make it work. I have a feeling this is because I'm doing it with a redirect (else the person who worked on this site before me, has done something to not make it work)

Cart link looks like this:
/cart/add/e-p129_q3?destination=cart

---

TL;DR Version:
Overzealous imho + fix will reduce conversions + it's more malicious to add things to someone's cart.

Long Version:
IMHo having my users purchase more items than they want (by re-clicking) is more of a "security issue" than someone maliciously clearing someones cart. What's worse, having someone purchase something they don't want, or having them not purchase something?

Say a shady affiliate sending out cart links with expensive products and large quantities. Trust me, as someone who wrote affiliate software for many years, this does happen.

Example: /affiliate/12/cart/add/p11196_q999 (?destination=cart/checkout)

Unfortunately destination=cart/checkout, doesn't appear to work, affiliate module probably nulls that out or there's a bug. From looking at uc_affiliate2 code, it should support this, so I think it's a bug.

Additionally, the "clear cart confirmation box", which is the desired outcome from patch #79, will confuse customers and reduce sales conversions. It's better to do it silently. Don't believe me? A/B test and let me know what does better. I for one will be disabling it anyways, because of this. If LulzSec wants to clear my customers carts, then so be it. If my site on the other hand relies heavily on cart_links (like the one I'm currently working on), then I'd prefer to take this risk to increase conversions.

On the current site I'm working on where it doesn't make sense for a user to purchase more than one item at a time. If a customer were to purchase multiple quantities of a product or more than one product, they'd complain and my client would have to refund the angry customer. If I put the confirmation in there, it's confusing UX.

But I suppose having /cart/add/e is pretty useless, unless you're clearing the cart to add fresh products. I'd probably be ok with disabling /cart/add/e