VendorTxCode not guaranteed to be unique or unpredictable

[ev-mark]

The current implementation of \Academe\SagePay\Model\TransactionAbstract::makeVendorTxCode is not guaranteed to be unique or unpredictable according to the PHP documentation for uniqid.

I would suggest the following function as (a) the transaction will fail if the VendorTxCode is accidentally repeated; (b) this function will not create predictable strings that a malicious user could brute force, if the VendorTxCode is used in any user input.

/**
 * This function returns a UUID.
 * Source: http://stackoverflow.com/a/15875555/1971539
 *
 * @return string A UUID following GUIDv4 spec, without braces. 36 characters long.
 */
public static function guidv4() 
{
    $data = openssl_random_pseudo_bytes(16);

    $data[6] = chr(ord($data[6]) & 0x0f | 0x40); // set version to 0100
    $data[8] = chr(ord($data[8]) & 0x3f | 0x80); // set bits 6-7 to 10

    return vsprintf('%s%s-%s-%s-%s-%s%s%s', str_split(bin2hex($data), 4));
}

However, if context is important then you could use the following function. It returns something quite similar to the existing function, but uses the full length allowed by Sagepay:

/**
 * Make a new VendorTxCode.
 * To be give the code some context, we start it with a timestamp before
 * we add on a random hex string.
 * The VendorTxCode is limited to 40 characters, so we use 12 bytes for the hex.
 * Override this method if you want a different format.
 */

public function makeVendorTxCode()
{
    $data = openssl_random_pseudo_bytes(12);

    return vsprintf('%s-%s', Array(date('Ymd-His'), bin2hex($data)));
}

I have created a pull request with the latter function. Whether you merge it or not is down to your feelings on introducing OpenSSL as a dependency.

[judgej]

Thanks Mark.

It looks like that function has lost some code anyway - it should have the time mS appended to the end. Not how or when that disappeared.

$VendorTxCode = uniqid(date('Ymd-His-'), false);

If openssl_random_pseudo_bytes() is optional in a PHP installation (and it isn't PECL, so far as I can see, so should more-than-not likely be available) then it should have a fallback. If it is something that will always be available in PHP >5.3, then let's just go for it :-)

The makeVendorTxCode() in this package is just an example, and I would hope it would always be overridden. Context may be important to some merchant sites, and not to others. It's often easier when testing, but not so important in production.

I'll give the PR a go over the weekend, but it looks good to me (better than the blatant error that I put in there first, without any random element).

[judgej]

I must have been having a brainstorm at the time. Of course a random element was added to the transaction ID - that is what uniqid() does.

Anyway, the change looks good to me, and hopefully will be more secure. This takes to the ID up to its max length, which has always been defined as 40 in the metadata, so this should not break any sites that have been assuming that max length could be used. I'll release this on a point release just in case.

https://github.com/academe/SagePay/blob/master/src/Academe/SagePay/Metadata/Transaction.php#L127

[judgej]

Thank you for the fix.