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.
The function ip_address() is being used to prevent ill-intentioned users to have access to someone else's order data by editing their sessions. However, ip_address() not always returns the end-user ip address and shouldn't be trusted to perform this kind of validation.
Additionally, in the scenario where the ip_address() returns two different values intermittently (for instance node balancer), we can observe two orders being created for the same session and the shopping cart being alternated from one order to the other every time the IP changes.
return db_query('SELECT order_id FROM {commerce_order} WHERE order_id IN (:order_ids) AND uid = 0 AND hostname = :hostname AND status IN (:status_ids) ORDER BY order_id DESC', array(':order_ids' => commerce_cart_order_session_order_ids(), ':hostname' => ip_address(), ':status_ids' => $status_ids))->fetchField();
Comment | File | Size | Author |
---|---|---|---|
#9 | commerce_cart-duplicated_session-1797062-10.patch | 1.2 KB | Ivam dos Santos Luz |
#2 | commerce_cart-duplicated_session-1797062-2.patch | 1.33 KB | Ivam dos Santos Luz |
Comments
Comment #1
handrus CreditAttribution: handrus commentedThe detection of IP is unnecessary and is not very accurate. We should just trust the session on those cases and let security issues on this scenario be held by drupal core.
Beside the documentation of ip_address() state very clearly:
Behind a reverse proxy (load balancer etc), or using a dynamic ip the user ends with a duplicated order every time the ip changes, this is confusing. The shopping cart items simply disappear and only a new item will be show.
Comment #2
Ivam dos Santos Luz CreditAttribution: Ivam dos Santos Luz commentedRemoved ip_address verification from the query performed on commerce_cart_order_id() function.
Comment #3
Ivam dos Santos Luz CreditAttribution: Ivam dos Santos Luz commentedChanging status to "needs work".
Comment #4
Ivam dos Santos Luz CreditAttribution: Ivam dos Santos Luz commentedFixing the issue status. The patch works, but "needs review".
Comment #6
Ivam dos Santos Luz CreditAttribution: Ivam dos Santos Luz commentedRe-applying patch to the right module version.
Comment #7
Ivam dos Santos Luz CreditAttribution: Ivam dos Santos Luz commented#2: commerce_cart-duplicated_session-1797062-2.patch queued for re-testing.
Comment #9
Ivam dos Santos Luz CreditAttribution: Ivam dos Santos Luz commentedRe-generating patch from module repository (had generated it wrongly from a local repository).
Comment #10
rszrama CreditAttribution: rszrama commentedTouched up the comments and committed; no need to keep the previous comment about IP address verification. : )
Thanks for the patch, and congrats on your first commit!
Comment #11
Ivam dos Santos Luz CreditAttribution: Ivam dos Santos Luz commentedThanks for your comments, @rszrama.
Let's give the credits to the guys that actually were responsible for finding and fixing the issue: @handrus and @caiovlp. I just uploaded the patch, motivated by them :)
[]s
Ivam
Comment #12
rszrama CreditAttribution: rszrama commentedGood call! Thanks to everyone for the input - I was being overzealous in my use of IP detection. Very helpful context / comments from handrus and caiovlp. : )
Comment #14
GaborTorok CreditAttribution: GaborTorok commentedI just saw this modification, and it may be late, but I would share my solution to a similar problem. I'm developing a module that needs to track entity ID's in the session for anonymous users, and I had a look at the commerce implementation.
I had concerns with the IP check, because some internet providers disconnect users every 24-48-etc hours and they can get a different IP after the reconnection. Users could loose their work because of an IP change.
My solution was to not filter on IP, but to use key-value pairs in the session variable, with the key being the ID and the value is a token generated with drupal_get_token() from the ID concatenated with a string. When I load the list from the session, I check the tokens with drupal_valid_token() and only use the IDs with a valid token.I had an idea instead of the IP check, but unfortunately I had misunderstood session handling and my idea turned out to be wrong.