Found and fixed bug in code that checks Braintree hashed value

Tested with and without clean urls, with the latest version of the API.
With your code, I get a fatal error during the transaction.

Notice: Undefined index: http_status in Braintree_TransparentRedirect::parseAndValidateQueryString() (line 257 of /media/nfsdrive/d7/www/sites/all/modules/contrib/commerce_braintree/braintree_php/lib/Braintree/TransparentRedirect.php).

Notice: Undefined index: http_status in Braintree_TransparentRedirect::parseAndValidateQueryString() (line 262 of /media/nfsdrive/d7/www/sites/all/modules/contrib/commerce_braintree/braintree_php/lib/Braintree/TransparentRedirect.php).

Braintree_Exception_Unexpected: Unexpected HTTP_RESPONSE # in Braintree_Util::throwStatusCodeException() (line 77 of /media/nfsdrive/d7/www/sites/all/modules/contrib/commerce_braintree/braintree_php/lib/Braintree/Util.php).

When you confirm the transaction, You call Braintree_TransparentRedirect::confirm() that use a validation function to parse the query string.

The function look like (I remove some parts) (in TransparentRedirect.php) :

   public static function parseAndValidateQueryString($queryString)
    {
        // parse the params into an array
        parse_str($queryString, $params);
        // remove the hash
        $queryStringWithoutHash = null;
        if(preg_match('/^(.*)&hash=[a-f0-9]+$/', $queryString, $match)) {
            $queryStringWithoutHash = $match[1];
        }
        (...)
        // recreate the hash and compare it
        if(self::_hash($queryStringWithoutHash) == $params['hash']) {
            return $params;
        } else {
            throw new Braintree_Exception_ForgedQueryString();
        }
    }

It first get the query without the hash, and then try to recompose the hash with the query it has and compare to the hash that is stored. Both need to be the same. This is used to avoid forged queries.

So that really look odd that you need to change the query. If the query string is altered, it should trigger an exception.

When you said "

This was causing a failure in communicating with Braintree API.

", can you be more explicit ?

What kind of error message do you have ?

Thanks

I also do have the q= in my query string (but not at the beginning).

On my env, it looks like that :

http_status=200&id=5mwtkkym65t8xrf4&kind=create_transaction&q=checkout/21/payment/return/Q92eohZ8u92bzv7tUKhVtgiEAYikz6qppJCtJ7n8nEI&hash=dc33763d0505be6d8b769ae536702641ff8ef617

And I did not get an exception from braintree with that.

I'm just using a regular expression to strip out the "q" parameter. It works.

/**
 * Return the query string that contains Braintree response, after a request.
 */
function commerce_braintree_get_feedback() {
  $feedback = FALSE;
  if (isset($_SERVER['QUERY_STRING'])) {
    $feedback = $_SERVER['QUERY_STRING'];
    // remove "q" parameter from query string
    $feedback = preg_replace("/&q(=[^&]*)?|^q(=[^&]*)?&?/", "", $feedback);
  }
  return $feedback;
}

Patch is attached.

I still do have the same issue as before ...

Clean url disabled, using the patch from #9, on the payment step :
Braintree_Exception_ForgedQueryString: in Braintree_TransparentRedirect::parseAndValidateQueryString() (line 269 of /core/profiles/modules/contrib/commerce_braintree/braintree_php/lib/Braintree/TransparentRedirect.php).

And this is how looks my $feedback variable from commerce_braintree_get_feedback(), just before and after the preg_replace();

before => 'http_status=200&id=pbqcdr5jfpw473q7&kind=create_transaction&q=checkout/621/payment/return/Beg9zcva_N-BUoqpYjmXX091vWZOM3sQklf-18v58Dc&hash=2f85dd49975afea05238ec1fb3abc5d7b2e154e5'
after => 'http_status=200&id=pbqcdr5jfpw473q7&kind=create_transaction&hash=2f85dd49975afea05238ec1fb3abc5d7b2e154e5'

Can we get this issue resolved? The before and after in #10 looks perfect to me.

As I said, I can't commit commit something that doesn't solve things for everyone. As said in #10, the patch from #9 leads me to a Braintree_Exception_ForgedQueryString

Just need to check for clean URLs before applying regular expression.

Patch attached.

I've confirmed this fix works with clean URLs enabled and disabled.

The patch from #13 fixed it for me.

Added sentence

I would recommend stripping the 'q=' variable regardless of the clean_urls setting, since some servers (like Pantheon) still have the 'q=' part in $_SERVER['QUERY_STRING'] with clean URLs enabled.

My solution was the following:

<?php
function commerce_braintree_get_feedback() {
  $feedback = FALSE;
  if (isset($_SERVER['QUERY_STRING'])) {
    $feedback = $_SERVER['QUERY_STRING'];
    if (preg_match('/^q=([^&]*)&?(.*)$/', $feedback, $match)) {
      $feedback = $match[2];
    }
  }
  return $feedback;
}
?>

However, Haza apparently had the q parameter in the middle of the query string, which this regex wouldn't fix. Any ideas what determines the order of the parameters in the query string?

Should we get this committed? Is this issue present in 7.x-2.x as well? At least the patch still applies.

It would be great to get this committed. I had to use the patch to get it to work for 7.x-2.x.

As I already said, some people needs to use this patch to get the module working, while some other people gets an error if they use the patch.

At this point, we can't commit that like it is right now if it will introduce crashes for some peple.

In that case it needs work.

the patch did not work for me

here is the error log

Suhosin-Patch configured -- resuming normal operations
[Sun Jun 28 00:20:57 2015] [notice] caught SIGTERM, shutting down
[Sun Jun 28 00:20:59 2015] [notice] ModSecurity for Apache/2.6.3 (http://www.modsecurity.org/) configured.
[Sun Jun 28 00:20:59 2015] [notice] ModSecurity: APR compiled version="1.4.6"; loaded version="1.4.6"
[Sun Jun 28 00:20:59 2015] [notice] ModSecurity: PCRE compiled version="8.12"; loaded version="8.12 2011-01-15"
[Sun Jun 28 00:20:59 2015] [notice] ModSecurity: LUA compiled version="Lua 5.1"
[Sun Jun 28 00:20:59 2015] [notice] ModSecurity: LIBXML compiled version="2.7.8"
[Sun Jun 28 00:21:00 2015] [notice] Apache/2.2.22 (Ubuntu) PHP/5.3.10-1ubuntu3.18 with Suhosin-Patch configured -- resuming normal operations

#13 worked for me. Thank you.

Hello,

Attached path solves the issue and is platform agnostic.

#13 worked for me. Thanks!

#22 works for me... thanks @blazey.

Unfortunately #22 requires that the q parameter is first, which might not always be the case. We can just use drupal_get_query_parameters(), which automatically strips out $_GET['q'].

None of the patches are working for me. I'm hitting same wall as OP. When you submit the credit card info using the drop-in UI, it immediately takes me to a page that says "Website encountered unexpected error."

Disregard my last comment. I figured out the problem. I removed the "billing information" pane from the checkout form. Apparently you can't do that or it throws an error. Added it back and it works fine.

#25 is the best approach and uses proper API methods to get query params, excluding Drupal's q. Reviewing

Thanks, everyone.