I'd look elsewhere... and maybe suspect the JS Tools module before something like this. That session variable is supposed to be cleared when you complete an order.

I've now managed to reproduce this on a clean install using your distro without any modifications except:

• enabling the Payment and Credit Card modules
• inserting the block as stated above (must use Full HTML input format)
• turning on Clean URLs

The problem requires Clean URLs to manifest.

This is a long shot, but shouldn't the style end in a semicolon?
style="margin-top: 12px;"

The logs say it all:

Checkout page:

Review page (note extra GET request at the end which clears the session variable):

Order complete page:

So the client browser is loading the image with path relative to /cart/checkout and not relative to Drupal root, which Ubercart apparently redirects internally to /cart (hence the HTTP 200 response code) which calls the function that clears the session variable. Note that in the log for the order complete page, Drupal issues a 302 redirect.

So, should we not be getting a 404 for any requests below /cart that don't exist as functions or files, like Drupal does normally, such that a request for /admin/fakename will give a 404?

Yes, it's rather a liminal sort of issue, and as you point out it only arises when a user puts in a relative URL somewhere else on the page. I don't know Drupal very well at all, so I don't know where Ubercart binds to all these sub-URLs. If you could point me to where this happens then I might be able to think about methods of dealing with it.

Ah, I was aware of the menu callbacks but thought it didn't do a "greedy" match. So basically any call for "/cart/.*" will call uc_cart_view() and reset the cart unless there's already a match from the menu system (e.g., "cart/checkout").

I remember now (from the Pro Drupal Development book) that extra URL arguments are passed into the function, so in this case there's a simple method for checking whether we ought to destroy the checkout status: see if the number of function arguments exceeds the number of arguments in the function declaration. If so, there is an overloaded URL which might indicate a bad user request that should be discarded with a 404. So my suggestion to solve the current problem in uc_cart.module line 1267:

You might want to use this sort of code in other functions, if there's a chance of something similar happening (which there probably is, esp in other functions in uc_cart.module).

By the way, I noticed you use HTTP Referrers a lot in uc_cart.module. Are you aware that the referrer is sent by the client and thus -- like all user input -- it can be forged (e.g., with the RefControl FF extension) and that often referrers are blocked for privacy reasons (e.g., by antivirus software)? This is important because there's a security check (line 1356) that checks the referrer and clears the cart_order variable if it is absent, which makes checkout impossible for anyone who blocks the referrer -- potentially a much more widespread problem than the one I originally reported (actually I'm a bit bemused that this problem hasn't been mentioned before). I think you'll have to use $_SESSION to store the previous page -- if necessary -- rather than use the HTTP_REFERER value. HTTP_REFERER is really only useful for stats.

I would be happy to work on this one & offer my changes to the Cart module, but I don't see how any of this could be done in a contrib.

Yes, it's not a security risk at all, but rather a usability problem. I know ZoneAlarm has an option to block the referrer, and apparently so does Norton, as does Opera in the "Quick Preferences" menu; but perhaps more significantly corporate firewalls and caching proxies effectively block referrers, so we're probably talking about a significant number of users -- especially the larger corporates, institutions and government offices, some of whom wouldn't be able to get any orders past checkout. This is especially problematic because the order completed screen would be shown to the customer. It's a show-stopper for me.

My suggested solution is twofold:

1) Set a session variables that records the page just viewed in hook_exit() (which executes even on cached pages). Obviously it won't be useful if a user browses to a different website and comes back, but for that we can still check the referer as well, just allowing that it might be a null string. In fact, the Review Order page posts the address info, and a potential hijacker could use the Back button, so I really don't think this much security anyway. (Other modules -- Product Kit and Recurring Payments -- also currently use HTTP_REFERER, so this will benefit them too.)

2) Set a timed session variable at checkout that will clear the cart_order variable after 15 minutes. This is as good in terms of security as it can possibly get for this particular problem.

I have implemented (1) in the attached files, based on the current Bazaar files. I had to put the referrer session variable in uc_store.module because it's used by modules other than Cart. I've patched those other modules -- Product Kit and Recurring Payments -- to use the new session variable rather than HTTP_REFERER. Thus, in addition to allowing privacy-sensitive users to use the checkout, we should now have 100% accurate redirects from add-to-cart links.

I've also added a simple check to prevent the order complete page being generated even on null orders in uc_cart_checkout_review_form_submit(). Previously it would show an order complete page with just blank variables (which might confuse users into thinking they had been charged twice), now it unsets cart_order and redirects to /cart/checkout with an error message. It also tests that the user has arrived from cart/checkout/review.

In doing all this, I haven't tampered with the $_SESSION['last_url'] variable, which has a slightly different purpose. It could be subsumed into the new variable, I suppose; but it's hardly essential. Also, in the process of making this patch, I noticed what appears to be an invalid reference to a $referer variable in the Catalog module which you might want to look at.

I haven't implemented (2) because I think to do it elegantly it would require a new "menu" entry (=page) for when a session times out, and also a configuration setting to go with it, so it's obviously more than just a simple patch.

In addition to implementing (1), I've added the func_num_args() patch mentioned in my previous post to solve the problem I originally reported re: images with relative URLs in side-blocks. I think this will be made irrelevant by Drupal 6.

Let me know what you think. It could do with further testing, but I really hope this can make it into 1.0.

bzr diff

I've attached the patches I mentioned and the timed session thing.

The tests are roughly as follows:
cart/checkout -- can arrive from anywhere, but any data is voided if the referer is not cart/checkout or cart/checkout/review
cart/checkout/review -- must arrive from cart/checkout or cart/checkout/review
cart/checkout/review submit -- must arrive from cart/checkout/review

If at any time the checkout session times out, the data is voided and an error message is set. There is an admin checkout setting to set the timeout value (default 15 minutes).

The helper functions I've created are:
uc_cart_checkout_timeout() -- in uc_cart.module
uc_referer_check() -- in uc_store.module, because that's where the session variable is yet

Note that the Review order page will allow a refresh, but won't refresh properly when using uc_credit.module because the credit card data disappears. I haven't looked into that module yet, but I note that when the "(Last 4)" thing comes up it can't reconstruct the full number again. It otherwise behaves a little unpredictably, so whilst you're reviewing the attached diff I'll look into the credit card module as there are a number of things I'd like to think about & perhaps prepare changes for, including the following:

• Add StartDate && IssueNumber
• Visa can issue cards for up to 20 years
• must have master database of accepted cards with ID numbers that can be accurately identified by payment gateway modules (to replace the
• user-defined select list which needs to be regexed in a payment gateway). Then can also displays additional credit card images at checkout.
• Credit card number should have basic validation -- is_numeric -- even if full validation is switched off
• Add some validation for other card types

I'll leave the view cart "continue shopping" thing for now, but it should a piece of cake now.

BTW I'm writing a payment gateway that will integrate 3D-Secure authentication somehow, and I was wondering if any of you guys had come across an implementation of this for Ubercart. It seems like it'll probably end up having to be quite AJAXy.

Haven't had a chance to test this yet, but it's bookmarked and I'll hopefully get to review it today.

Thanks Lyle, something of an oversight there -- too permissive in allowing cart/checkout referer without the do_review session variable. Have attached updated patch to combat this menace.

Did you notice anything else wrong, or is this it?

As an aside, I'm inclined to think there should be a message of some kind when users are redirected back to cart/checkout.

Patched my installation manually this time, and I think it works pretty well. I did clean it up a little (mostly spacing issues) and since drupal_goto() calls exit(), you don't need return after it.

I got the timeout message when I refreshed the checkout page after the patch, but I think that's just a fluke. Most people won't start from there after other sites are updated.

Even still, I'm not the best tester in the world, and I'd appreciate more people trying this out. It's in the Bazaar version now, so problems should show up pretty quickly.

Good catch. Enjoy your badge.

If necessary we can reduce the testing to just prevent offsite HTTP_REFERERs and not use the session variable in that function.

However, the referrer session variable will still be useful for a "Continue shopping" link in the cart page.

Well, if I'm brutally honest for a second, I don't see much value in this referer check anyway (except for the continue shopping link). The real problem is storing cc numbers in session variables, which often get stored on servers for long periods of time in world-writable /tmp directories, thus completely obviating your other work on PCI DSS. This does actually change the status of Ubercart for PCI DSS purposes... cc numbers should only ever be kept in POST.

I see the new function has now been removed from bazaar, thus restoring the silent requirement to have client referers. Ryan, do you have a diff for this against v1.0, so I can restore it in my own version? I would like to resolve these issues you raise so it can go back in again. I feel it's fairly essential for basic functionality.

I see my func_num_args hack got removed too...

OK, I'll post my various patches on drupal.org. I do want to get them "out of the way", though, then hopefully I can move on to stuff like VAT (as I'm based in the UK and currently work part-time as the book-keeper for a Limited Company).

Re: moving some stuff to the drupal.org issue tracker, I'm afraid I really don't have any ideas. It already seems quite complex, what with the forums & issue tracker here, as well as the support.ubercart.org.

On second thoughts I think it would be rather silly to move this current discussion over to d.o just yet. We need to be clear about how to approach this, and we need some consensus before I prepare a patch.

Firstly, there seems to be only a very weak case for wanting to check where the user has come from when they are at cart/checkout/review. The rationale seems to be that a shopper might leave their computer for another person to examine, and we therefore need to be careful about displaying checkout information. I don't think this is a danger, given that it all happens within a session; but whatever we do, we will probably always be defeated by the "Back" button. Are we not just wasting time with this?

Secondly, my view is that assuming HTTP_REFERER exists and is valid is bad, bad, bad. I don't know what the percentage of users without HTTP_REFERER is, but I don't think it's for Ubercart to arbitrarily exclude certain shoppers without providing a reason. At present, there is not even an error message. Therefore, whatever the answer to the first point, the current situation is untenable.

Thirdly, I think the referer check issue is being conflated with totally different issues relating to user redirects. Forms that need to redirect users should have the referring page passed to them directly, rather than seeking to guess it from referers or the like.

Thoughts?

solarian...does this patch work for the 'incheckout' problem? I just started having it when i switched themes.

If you've tried and tested the patches, that'd be great. I honestly just haven't had time to review them since they were posted. I hope to get to them this week so we can put up a 1.1 release, but I wouldn't hold your breath...

Solarian -

My problme was that all of my orders were going to incheckout (and not checkout).

For some reason my $base_path function didnt work so i had to use Global[Base_path], which i think fixed the problem.

However if i d/led you're patch, would this remove any future similiar errors like that from happening in the future?

I just upgraded to 1.2 and the uc_referer_check() broke checkouts for me (and others as discussed elsewhere). In my case the fix is not as simple as detecting https, because I was also doing some url rewriting to remove the 'cart' portion of the address.

Can you pull this function or make it optional?

I'm using custom_url_rewrite() to map /checkout/* to /cart/checkout/* on incoming urls and vice versa for outgoing urls. I'm not sure if that explains it for you or not?

I found a way to work around this without modifying any uc code. My solution is to use hook_init to clear the referrer environment variables whenever the request_uri matches 'checkout'. That way, the referer check always returns true. So long as this continues to work, I'm happy. I would hate to have to modify uc code.

Sorry to be repetitive but I am in NOOO WAY a pro programmer like you guys (infact I don't consider myself a programmer at all), but I am having a big headache from hitting the "Review Order" button. From what I can tell all my setting for ubercart and shipping quotes are coming out fine.

When I hit the "Review Order" Button, the next page never loads and eventually the browser times out. I am running GoDaddy hosting with a current upgrade to PHP Version 5.x from PHP version 4.x (after installing and configuring Ubercart)

Your time and help is much appreciated, I have been looking both on Ubercart and Drupal forum for at least 8 hrs straight, but nothing has fixed my problem.

I am using Ubercart 5.x-1.3 & I am running on Drupal 5.9.

Thank you again