Currently, if the user blocks HTTP_REFERER in their privacy settings, checkout will silently fail. This patch prevents that from happening, and otherwise improves functionality, by correctly handling empty referrers and by providing a fallback referrer in a session variable.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

I really like the idea behind this patch - we're doing these checks all over the place, and doing it all through a single function will make it much easier to debug/improve.

As for this patch, I wanted to verify my understanding here...

   // Save the current request for tracking paths on subsequent page requests.
-  // $_SESSION['uc_referer_uri'] = $_GET['q'];
+  // When HTTP_REFERER is set, the session version is not; and vice versa.
+  if (referer_uri()=='') {
+    $_SESSION['uc_referer_uri'] = 'http://'.$_SERVER['SERVER_NAME'].'/'.$_GET['q'];
+  } else {
+    if (isset($_SESSION['uc_referer_uri'])) {
+    unset($_SESSION['uc_referer_uri']);
+    }
+  }

Will something like this using the SERVER_NAME allow for Drupal running in a subdirectory?

Also, this will seem to choke on HTTPS, won't it?

solarian’s picture

FileSize
6.63 KB

You're right. It should be:

$protocol = (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS']!='off') ? 'https': 'http';
$_SESSION['uc_referer_uri'] = $protocol.'://'.$_SERVER['SERVER_NAME'].base_path().$_GET['q'];
rszrama’s picture

Checked through it one more time, and my only remaining question is related to the issue that led me to remove this during my testing last time. How does this work when you refresh pages? Also, does it work flawlessly through the different checkout redirects? i.e. a complete, normal checkout; a checkout that goes back from review and then proceeds; a checkout that has validation errors; etc.?

solarian’s picture

I set about this new patch with your previous comments in mind, so now it uses the server HTTP_REFERER variable unless that is empty. In other words, it provides a fallback mechanism (via the session variable), but otherwise behaves as before. Therefore the page refresh issue would only apply to the folks who block HTTP_REFERER and who like to do a lot of page refreshes for some reason -- surely a negligeable issue, esp. in comparison to the fact that checkout currently fails completely for folks who block HTTP_REFERER?

Re: your second point about checkout working properly, so as to make it as easy as possible for this patch to be committed, I haven't changed the logic code at all except in uc_cart_checkout_review_form_submit() where I add a check that wasn't there before:

CURRENT:

<?php
function uc_cart_checkout_review_form_submit($form_id, $form_values) {
  switch ($form_values['op']) {
    case t('Back'):
      cache_clear_all();
      unset($_SESSION['do_review']);
      return 'cart/checkout';

    case variable_get('uc_checkout_submit_button', t('Submit order')):
      // Invoke hook_order($op = 'submit') to test to make sure the order can
      // be completed... used for auto payment in uc_credit.module.
      $order = uc_order_load($_SESSION['cart_order']);
      $pass = module_invoke_all('order', 'submit', $order, NULL);

// etc...
?>

PATCHED:

<?php
function uc_cart_checkout_review_form_submit($form_id, $form_values) {

  $order = uc_order_load($_SESSION['cart_order']);
  
  if ($order === FALSE || uc_order_status_data($order->order_status, 'state') != 'in_checkout' || !uc_referer_check('cart/checkout/review')) {
    unset($_SESSION['cart_order']);
    return 'cart/checkout/';
  }

  
  switch ($form_values['op']) {
    case t('Back'):
      cache_clear_all();
      unset($_SESSION['do_review']);
      return 'cart/checkout';

    case variable_get('uc_checkout_submit_button', t('Submit order')):
      // Invoke hook_order($op = 'submit') to test to make sure the order can
      // be completed... used for auto payment in uc_credit.module.
      $pass = module_invoke_all('order', 'submit', $order, NULL);

// etc...
?>
rszrama’s picture

Status: Needs review » Fixed

Applied and committed w/ minor clean-up modifications. We're doing testing now with the folks here for Ubercamp and will hopefully push out the 1.1 this afternoon. : )

solarian’s picture

Thanks for this.

Have just seen the HTTP/HTTPS issue caused a problem elsewhere -- apologies for that, it was a really dumbass piece of coding.

solarian’s picture

--deleted--

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

moshe weitzman’s picture

Status: Closed (fixed) » Active

The $_SESSION['uc_referer_uri'] property is set for all web crawlers since they don't provide referers. This is very bad, since there _SESSION is now non empty and they will initiate a new session for every single request. That is, the check in ses_write() that tries not to write a session for crawlers will fail. Check your ubercart sites and i bet you see a huge sessions table. Try this query to see if the same hostnames are causing loads of sessions.

SELECT COUNT(*) AS cnt, hostname FROM sessions GROUP BY hostname ORDER BY cnt DESC

I think this $_SESSION property does more harm than good.

rszrama’s picture

Version: 5.x-1.0 » 6.x-2.x-dev
Assigned: Unassigned » rszrama

Aha! Someone just posted about having extraneous sessions and we didn't know what to tell them. Thanks for turning this up, Moshe. Will work out a solution for the next minor release.

Damien Tournoud’s picture

Cannot we make sure that the checkout can run without this? Sounds like it should.

rszrama’s picture

#11 - Totally! That multi-step checkout form I demoed for you doesn't need this trash at all. ; )

I think the bigger problem is this variable is used for the "Continue shopping" link to send you back to the page you were on before getting redirected to the cart... whether you submit an add to cart form or follow a cart link. People who complete checkout will have sessions anyways to track their order ID (something else that was eliminated with the multi-step checkout form), so I'm not sure it's necessary to totally remove it from checkout. We could just make it part of a form submission.

Island Usurper’s picture

Priority: Critical » Normal
mr.j’s picture

Subscribing. This is a potential performance killer.

Googlebot is getting a separate entry in the session table for every page it crawls. The same things happens for anonymous users with no referrer set - eg click through from an email or manual URL entry.

On our site we don't even give anonymous users the option of ordering anything, and to aid in caching there should not be any session variables set for them. But despite that, this patch still adds to the session table because of the unconditional session variable set in uc_store_exit().

My question is can I safely put a conditional statement around the code so that it only runs for $user>uid != 0 or will it making something else break?

torgosPizza’s picture

We are in the same exact boat as mr. j - we get a huge sessions table even though only authenticated users are allowed to make purchases.

As far as putting a $user->uid condition, I think it's safe to do, though I haven't read through the code to see if this would impact anything. My hunch is to say it won't, but I'm not 100% on that. Won't be until I test it out, anyway.

longwave’s picture

On Debian/Ubuntu servers I use http://drupal.org/project/session_expire to help stop the sessions table getting too out of control, but ideally this should be fixed in Ubercart itself.

longwave’s picture

So, I'm trying to understand what this is actually used for and why it's needed. The following code acts on referers:

  1. uc_store_exit() stores the referer if HTTP_REFERER is not available, which is the source of this issue.
  2. uc_cart_form_alter() uses it to redirect back to the checkout after clicking a login link on the checkout page.
  3. uc_cart_add_item() uses it to store the previous URL for the "Continue shopping" button.
  4. uc_cart_checkout_form() uses it to prevent identity theft, clearing the checkout form if the user comes back to an in-progress order after visiting a non-checkout page.

So why do we need this at all?

Item 2 looks like it should be achieved in a better way by linking to /user/register?destination=cart/checkout from the checkout page.

Item 3 can be done using HTTP_REFERER instead where available; for the small number of users that don't send a referer for whatever reason, they will be sent back to the default "continue shopping" page instead.

Item 4 is apparently the reason why this code exists in the first place, but can't we just set a session variable in hook_exit for cart/checkout and cart/checkout/review only, clear it for all other pages, and test on that? Perhaps use a timestamp, so checkout is always cleared after a certain amount of time? (there is some commented out code in uc_store_exit() that looks like it was intended to do this, but never completed)

Is there something else I'm missing here?

mr.j’s picture

Well I have tried this patch and so far no problems.
I know its not a solution for everyone but if you don't allow anonymous purchases then you can use it as a quick fix in the meantime:

--- Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -555,8 +555,12 @@
   // Save the current request for tracking paths on subsequent page requests.
   // When HTTP_REFERER is set, the session version is not; and vice versa.
   if (referer_uri() == '') {
+    // patch to prevent anonymous users and bots from getting a session
+    global $user;
+    if ($user->uid > 0) {
     $protocol = (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off') ? 'https' : 'http';
     $_SESSION['uc_referer_uri'] = $protocol .'://'. $_SERVER['SERVER_NAME'] . $GLOBALS['base_path'] . $_GET['q'];
+    }
   }
   else {
     if (isset($_SESSION['uc_referer_uri'])) {
longwave’s picture

Title: HTTP_REFERER patch » Reduce HTTP_REFERER checks
Category: bug » task

#1093626: Ubercart causes drupal to save a session for every crawler request fixes #14. Leaving this open because I think #17 is still valid.

longwave’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev

Bumping for reconsideration in 7.x. I think the timestamp proposal in #17 item 4 is a way we can clean this up and fix #1120776: order number increments on reload checkout page at the same time.

longwave’s picture

Assigned: rszrama » Unassigned
wodenx’s picture

It would be easy enough to add a flag in uc_cart_checkout_form_validate() indicating whether or not the cart order has any data (i.e. has been submitted via ajax or otherwise) - no need to time it out if no data have been entered (e.g. if the customer simply refreshes the page or visits another page without ever submitting the form). This could further reduce the number of abandoned orders (assuming, as longwave suggests, the order is created at the beginning of the checkout process).

Island Usurper’s picture

Status: Active » Needs review
FileSize
5.91 KB

OK. I think this patch covers three different issues that are all kind of dealing with the same thing. This one, #1093626: Ubercart causes drupal to save a session for every crawler request, and #1120776: order number increments on reload checkout page.

uc_cart_exit() now sets a session variable when checkout pages are visited, recording the request time. Order data generated by the checkout pages is kept around as long as that variable is less than 20 minutes old. (Is that a reasonable amount of time? We can make it configurable, but I don't think it's needed.) That variable is deleted on other pages except 'system/ajax', since we don't want to lose the order data every time we change something on the checkout form.

As per #17, I took out uc_store_exit(). That's all it did, and we don't actually need it.
2 had already been done.
3 is fixed by the patch in #1093626-7, which I've copied into this patch.
And 4's fix is described above.

With these changes, the only other place uc_referer_uri() was used was in the cart links, which is a legitimate use of the referer URL. I put $_SERVER['HTTP_REFERER'] there in the hopes that it will be populated for anyone who uses that module. If anyone objects to the removal of uc_referer_uri(), it and uc_store_exit() can be put back since #1093626 is basically fixed anyway.

longwave’s picture

Why do we need a session variable for this? Can't we use $order->modified and drop the hook_exit() implementation entirely?

Island Usurper’s picture

FileSize
5.82 KB

Well...no reason, I guess. That whole section that decides whether to set $order = NULL can be simplified, too.

longwave’s picture

This looks good and seems to work for me.

Should this be split into separate patches (maybe one per file), as each change is made for different reasons and it will be easier to explain this in individual commit messages? This will also make things a bit easier if we backport this to D6.

Island Usurper’s picture

Status: Needs review » Fixed

I squashed the different commits together when I made the patch. Since it looks good to you, I'll just merge in the branch and keep all of those commits.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mr.j’s picture

Was it applied to 6.x too?

longwave’s picture

Version: 7.x-3.x-dev » 6.x-2.x-dev
Status: Closed (fixed) » Patch (to be ported)

No, but I guess it could be backported if someone wants to do so.

danisha’s picture

I dont know why the issues is closed as all the patches are all failed.......

danisha’s picture

Status: Patch (to be ported) » Needs review

#25: 273574_referer_uri.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 273574_referer_uri.patch, failed testing.

TR’s picture

Status: Needs work » Patch (to be ported)

Resetting status.

@danisha: Please read up on how the issue queue works and what the issue statuses mean. The links are below, in the section for posting a new comment.

danisha’s picture

Buddy TR, These all patches don't work. They all give some or the other error. Do you mean that there is no solution to this problem of multiple in checkout status problem. Actually I am using the updated version of ubercart drupal 6. In this too, there is a problem of multiple in checkout statuses, which uses a new order id everytime, means that a new order is created even if there is a validation error on the checkout page. This is really frustrating because if the page is reloaded for 10 then there are 10 in checkout status order created. This wastes a lot of database memory. After browsers hours and hours I think that this patch above was the solution. But it didnt work. Can you guide me for any solution.

TR’s picture

Again, READ the issue and the documentation. The patches are for Drupal 7, not 6, and have already been added to the D7 version of Ubercart. Of course they won't work under Drupal 6. The current status is "patch (to be ported)", which means it's waiting for someone to port the patch to the D6 version of Ubercart. If this is important to you, maybe you can work on it or hire someone to do it for you. If you're starting a new site, I strongly suggest you use D7 instead of D6, as D6 is approaching obsolescence.

danisha’s picture

Thanks for your advice TR... Anyways.

After hours of head banging, I had to hack the core ubercart drupal module. I dont know why but the ubercart uc_cart.module unsets the order session everytime the ubercart checkout page is loaded. Hence when the page is loaded or any validation error is there on the checkout page, a new order id is created for preventing the identity theft(which i have no clue what it is).

Coming back to my point. I just commented out the line 142 and 150 in uc_cart.pages.inc of ubercart.
eg:- unset($_SESSION['cart_order']);

Now since the cart order is not unset hence there are no multiple in checkout status order created. Seems to be working fine for now. I know that hacking core ubercart module is not good, but i couldn't fine any other solution for this. Please do reply if it will cause any future error.
Thank you.

longwave’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.66 KB
robbertnl’s picture

#38: 273574-remove-referer.patch queued for re-testing.

robbertnl’s picture

#38
I can confirm this helps with the language handling prefix bug.
What is the current state of the D6 bugfix version?

longwave’s picture

If this solves the language handling prefix issue, and does not cause any other problems on your site, this can be committed to 6.x and released with the next version - we were just waiting for someone to test it out. Set this issue to "reviewed & tested by the community" if this is the case!

robbertnl’s picture

Well i did not test all of the issues. I have to test more before i am going to use this patch live

TR’s picture

Issue summary: View changes
Status: Needs review » Closed (won't fix)

Most of this issue has already been fixed in 6.x-2.x, and ALL of it has been fixed in 7.x-3.x.

This issue is still open only to port the 7.x-3.x patch in #27 back to 6.x-2.x.

A 6.x-2.x patch was provided in #38 but has not been reviewed in 3 years. Meanwhile, Drupal 8.0.0 is now released so we are ending support for 6.x-2.x.