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();
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

handrus’s picture

The 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:

If Drupal is behind a reverse proxy, we use the X-Forwarded-For header instead of $_SERVER['REMOTE_ADDR'], which would be the IP address of the proxy server, and not the client's. The actual header name can be configured by the reverse_proxy_header variable.

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.

Ivam dos Santos Luz’s picture

Removed ip_address verification from the query performed on commerce_cart_order_id() function.

Ivam dos Santos Luz’s picture

Status: Active » Needs work

Changing status to "needs work".

Ivam dos Santos Luz’s picture

Status: Needs work » Needs review

Fixing the issue status. The patch works, but "needs review".

Status: Needs review » Needs work

The last submitted patch, commerce_cart-duplicated_session-1797062-2.patch, failed testing.

Ivam dos Santos Luz’s picture

Version: 7.x-1.3 » 7.x-1.x-dev
Assigned: Unassigned » Ivam dos Santos Luz
Status: Needs work » Needs review

Re-applying patch to the right module version.

Ivam dos Santos Luz’s picture

Status: Needs review » Needs work

The last submitted patch, commerce_cart-duplicated_session-1797062-2.patch, failed testing.

Ivam dos Santos Luz’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

Re-generating patch from module repository (had generated it wrongly from a local repository).

rszrama’s picture

Title: Two shopping cart orders within the same session » IP address verification for anonymous cart order ID determination is unreliable
Assigned: Ivam dos Santos Luz » Unassigned
Status: Needs review » Fixed

Touched 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!

Ivam dos Santos Luz’s picture

Thanks 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

rszrama’s picture

Good 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. : )

Status: Fixed » Closed (fixed)

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

GaborTorok’s picture

I 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.