Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#38 | 273574-remove-referer.patch | 6.66 KB | longwave |
#25 | 273574_referer_uri.patch | 5.82 KB | Island Usurper |
#23 | 273574_referer_uri.patch | 5.91 KB | Island Usurper |
#2 | referer_patch.diff | 6.63 KB | solarian |
referer_patch.diff | 6.53 KB | solarian | |
Comments
Comment #1
rszrama CreditAttribution: rszrama commentedI 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...
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?
Comment #2
solarian CreditAttribution: solarian commentedYou're right. It should be:
Comment #3
rszrama CreditAttribution: rszrama commentedChecked 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.?
Comment #4
solarian CreditAttribution: solarian commentedI 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:
PATCHED:
Comment #5
rszrama CreditAttribution: rszrama commentedApplied 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. : )
Comment #6
solarian CreditAttribution: solarian commentedThanks for this.
Have just seen the HTTP/HTTPS issue caused a problem elsewhere -- apologies for that, it was a really dumbass piece of coding.
Comment #7
solarian CreditAttribution: solarian commented--deleted--
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedThe $_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.
Comment #10
rszrama CreditAttribution: rszrama commentedAha! 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.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedCannot we make sure that the checkout can run without this? Sounds like it should.
Comment #12
rszrama CreditAttribution: rszrama commented#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.
Comment #13
Island Usurper CreditAttribution: Island Usurper commentedComment #14
mr.j CreditAttribution: mr.j commentedSubscribing. 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?
Comment #15
torgosPizzaWe 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.
Comment #16
longwaveOn 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.
Comment #17
longwaveSo, I'm trying to understand what this is actually used for and why it's needed. The following code acts on referers:
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?
Comment #18
mr.j CreditAttribution: mr.j commentedWell 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:
Comment #19
longwave#1093626: Ubercart causes drupal to save a session for every crawler request fixes #14. Leaving this open because I think #17 is still valid.
Comment #20
longwaveBumping 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.
Comment #21
longwaveComment #22
wodenx CreditAttribution: wodenx commentedIt 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).
Comment #23
Island Usurper CreditAttribution: Island Usurper commentedOK. 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.
Comment #24
longwaveWhy do we need a session variable for this? Can't we use $order->modified and drop the hook_exit() implementation entirely?
Comment #25
Island Usurper CreditAttribution: Island Usurper commentedWell...no reason, I guess. That whole section that decides whether to set $order = NULL can be simplified, too.
Comment #26
longwaveThis 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.
Comment #27
Island Usurper CreditAttribution: Island Usurper commentedI 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.
Comment #29
mr.j CreditAttribution: mr.j commentedWas it applied to 6.x too?
Comment #30
longwaveNo, but I guess it could be backported if someone wants to do so.
Comment #31
danisha CreditAttribution: danisha commentedI dont know why the issues is closed as all the patches are all failed.......
Comment #32
danisha CreditAttribution: danisha commented#25: 273574_referer_uri.patch queued for re-testing.
Comment #34
TR CreditAttribution: TR commentedResetting 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.
Comment #35
danisha CreditAttribution: danisha commentedBuddy 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.
Comment #36
TR CreditAttribution: TR commentedAgain, 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.
Comment #37
danisha CreditAttribution: danisha commentedThanks 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.
Comment #38
longwaveThis is an attempt to port #25 to 6.x, needs testing but should help close the following issues as well:
#1288114: uc_referer_check(), referrer checking code doesn't work on Apache with UseCanonicalName on, ubercart back button loses data
#1399558: uc_referer_check not handling language prefix
Comment #39
robbertnl CreditAttribution: robbertnl commented#38: 273574-remove-referer.patch queued for re-testing.
Comment #40
robbertnl CreditAttribution: robbertnl commented#38
I can confirm this helps with the language handling prefix bug.
What is the current state of the D6 bugfix version?
Comment #41
longwaveIf 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!
Comment #42
robbertnl CreditAttribution: robbertnl commentedWell i did not test all of the issues. I have to test more before i am going to use this patch live
Comment #43
TR CreditAttribution: TR commentedMost 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.