Authenticated cart's content deleted on cron [#1103342]

uc_cart_cron() empties anonymous and authenticated carts which are older than 4 hours.

However the query which retrieves the carts to be emptied distinguishes anon carts from auth carts with a check on the length of cart_id. Specifically anon carts are CHAR_LENGTH(cart_id) > 8 while auth carts are CHAR_LENGTH(cart_id) <= 8.

Authenticated users cart_id is their uid while anonymous users cart_id is their sid from the session table. As such authenticated users cart_id can be up to 10 characters long (the length of the uid field in the users table) while anonymous users are anywhere from 26+ characters (the minimum length of session_id returned from a call to session_id() when session.hash_function = 0 (i.e. md5) and session.hash_bits_per_character = 6).

As such an authenticated users cart with a uid of 10 characters will be treated as an anonymous cart and delete as per the uc_cart_anon_duration setting.

You could say this is academic as there are probably few (if any) sites who actually have 9 or 10 digit uid! However this bug manifested when reworking uc_save_for_later for Ubercart 6.x-2.x. uc_save_for_later uses the flexible length of the cart_id field (up to 255 chars) to enable multiple user carts, i.e. a normal cart and a saved cart. The saved cart is distinguished by prepending "saved|" to the cart_id. As a result the total character length of the cart_id field is increased by 6 characters. When uc_cart_cron() runs these authenticated carts with longer cart_id's (with uid's just 2 chars or more) are treated as anon carts and are deleted as per the anon cart settings (i.e. after 4 hours rather than 1 year!).

Some digging in Git blame reveals that the CHAR_LENGTH call looking for 8 char long cart id's was introduced in Ubercart 1.0. It seems it was never adjusted when cart_id was up from varchar(32) to varchar(255).

Short version: The check for auth/anon carts should look for a cart_id longer than 8 characters.

Patch forthcoming.

Comment	File	Size	Author
#7	1103342-7-cart_id-length-check-D7.patch	1.38 KB	fenstrat
#7
#4	1103342-4-cart_id-length-check.patch	1.54 KB	fenstrat
#4
#1	1103342-cart_id-length-check.patch	1.54 KB	fenstrat
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

fenstrat

he/him

English

Australia

CreditAttribution: fenstrat commented 23 March 2011 at 22:23

Status:

Active

» Needs review

File	Size
1103342-cart_id-length-check.patch	1.54 KB

The attached patch lengthens the check from 8 to 16 characters. This length could be bikesheded to death. I was tempted to make it 26 as it is the shorted length session_id() can return (see above).

Went with 16 as it splits of the difference between 26 and the current 8, and also allows "saved|1234567890" which is the longest cart_id which uc_save_for_later could generate.

Comment #2

TR CreditAttribution: TR commented 23 March 2011 at 23:03

Issue tags:

+Needs tests

I'm interested to see if the testbot will finally work ... waiting to see what it does with this patch. I'll commit after I hear back from the bot.

I think we should just go with 26 instead of 16 - no need to limit the length to anything smaller.

I'm also going to try out a new tag here - "Needs tests". This seems to be an issue ideally suited to a SimpleTest to ensure that 1) Authenticated cart_id is always <= 26, 2) Anonymous cart_id is always >26, 3) Authenticated carts get cleared after the amount of time set in the UI, and 4) Anonymous carts get cleared after the amount of time set in the UI. @fenstrat - if you can write those tests it would be fantastic! Otherwise, we'll just leave the tag on as a to-do reminder after the patch is committed.

Comment #3

mattcasey CreditAttribution: mattcasey commented 24 March 2011 at 22:48

subscribing

Comment #4

fenstrat

he/him

English

Australia

CreditAttribution: fenstrat commented 25 March 2011 at 04:00

File	Size
1103342-4-cart_id-length-check.patch	1.54 KB

Thanks for the review TR.

I've done some more digging on the session_id() function. The shortest value it can return is 22 characters (which happens with ini_set('session.hash_function', 0) and ini_set('session.hash_bits_per_character', 6)). I've updated the patch. Lets see what the testbot does with this (currently says postponed for the original patch?).

Flat strapped atm but will make some time next week to look at writing a test for this.

Comment #5

TR CreditAttribution: TR commented 25 March 2011 at 05:10

Test is "postponed" because the testbot is having problems loading up module dependencies. The testbot first runs the tests on Ubercart branch to make sure everything works, then if that passes the testbot will run all the outstanding patches. However, the Ubercart tests didn't pass because Ubercart needs the Token module in order to run and the testbot can't load Token. So all the tests for Ubercart are currently in "postponed" until that gets worked out.

One of these days it'll get straightened out, I know they're working really hard to get the testbot fully functional on contributed modules.

If you can write a test now or later that would be great - it will be experimental verification of the session id length, and will insulate us from any changes made in Drupal core or in PHP that affect the session id.