Correct string length for X-VPS-Request-ID

Please do not use the ubercart site versions of this module. The latest will always be on this site, and contains important bugfixes.

If you are using the recurring system, use the latest devel d5 version (I will release once it has been tested by the community). Otherwise, use the beta release.

Let me know if this is still occurring with a more up to date version, and I'll investigate further if it is.

The problem for high traffic shopping sites is that the $request_id is a sha1() encryption of the $xml_request plus time(). sha1() produces a 40 char hash. The X-VPS-Request-ID only accepts 1 to 32 char strings (http://www.pdncommunity.com/pdn/board/message?board.id=payflow&thread.id...). Presuming they just chop the first 32 chars of whatever is sent, we've dramatically increased our chances of a collision in the hashes for two separate requests. The chances of colliding would still seem improbable I would think unless the $xml_request was non-unique.

We should probably change the $request_id to include more guaranteed differences in the seed data by using microtime() instead of time() and perhaps even concatenate two calls of microtime() for a higher level of confidence. Since we're not really dealing with a security type issue here so much as attaining a unique transaction identifier (PayPal holds them for 8 days), it may be worthwhile to switch from sha1() to md5(). md5 produces a 32 char hash string which fits perfectly into the X-VPS-Request-ID requirements. In all honesty, PayPal really should allow greater than 32 chars.

includes/PayflowProRecurring.class.php and uc_payflowpro.module contain a total of three $request_id creations.

There's a lot going on here:

Using sha1 doesn't make sense here for a variety of reasons, so I'll switch it to md5 (already committed). Now it is the right string length.

Now that the string length is right, it's incredibly unlikely for there to be a collision. The chances of two actually-valid XML strings concatenated with the time having the same md5 AND being close enough together that PayPal will consider the second a duplicate ("minimum of 7 to 8 days") is just not going to happen in practice. Sure, there are many possible strings that will collide, but they are going to just trigger an error from PayPal if you send them because they aren't valid XML.

Going from time() to microtime() will only make a difference if otherwise identical requests are submitted within one second. "Otherwise identical" meaning from the same person, like they double-submitted, so we want this case to be considered a duplicate. Doing double-microtime() is a tiny step further in the wrong direction.

The PayPal examples actually use the order ID as the unique ID, with no time or anything appended. That's not any good because a request using the right data after a failed attempt will be considered a duplicate.

However, we should probably still remove the time component from the request ID anyway. The md5 of the request should be sufficient to determine uniqueness (after all, if you submit the exact same thing twice 5 minutes apart it is just as duplicate as during the same second), but I'm not willing to make that change without significant community testing.

I don't think this most recent comment is related to the original request (which was probably unrelated and a result of a long-fixed bug), but I'm changing the issue title anyway. Heydemo, please open another issue if it pops back up using recent code.

The D6 version has already eliminated the excess calls via refactor.

I think I found it. Within uc_payflowpro.module for 5.x and 6.x, check out the formation of $xml_request prior to it's use in the sha1(). I think within _uc_payflowpro_submit_xml one meant to pass along $xml_request (or $xml) but it was switched up at some point. If we can get those changed to match up either all $xml or all $xml_request, then we'll have unique data, and I'll happily rescind my microtime() request :)

I really am interested in heydemo's request so I'm sorry this issue has covered too many areas.

I really need to have one issue nid per bug to keep track of it later. Moved the remaining changes to #549938: Transaction Unique ID uses variable that has been renamed