Issues

ZF-3189: Zend_Http_Client_Adapter_Proxy::connectHandshake ignores useragent set in headers

Description

If the user-agent is specified in the headers, as opposed to via setConfig(), the default UA of Zend_Http_Client is used for CONNECT requests by the proxy adapter.

I'm not too familiar with proxy servers, but there could be issues if the CONNECT request uses one UA and the actual request uses another. It seems like setting the UA via setHeaders should overwrite the useragent config item.

Comments

Please evaluate and fix/categorize as necessary.

Do you have any code to demonstrate this issue?

In tracing through the code, I noticed the following: Zend_Http_Client::request() calls Zend_Http_Client::_prepareHeaders(), which returns a numerically-indexed array of headers in 'name: value' format. That array is then passed to the adapter via an argument to it's connect() method. However, Zend_Http_Client_Adapter_Proxy::connect() treats $headers as an associative array of HeaderName=>HeaderValue pairs.

Did I miss something there, or is that actually the case? If it is, then Zend_Http_Client_Adapter_Proxy needs to be reworked to reflect that, as it uses $headers as an associative array in many places.

It looks to me that the culprit is here:


// Add the user-agent header
if (isset($this->config['useragent'])) {
    $request .= "User-agent: " . $this->config['useragent'] . "\r\n";
}

The config key 'useragent' always exists, as it has a default defined in Zend_Http_Client. In the connectHandshake() snippet above, the User-agent header will be set up with that default value (Zend_Http_Client) regardless of what might be set in $headers, as none of those headers are sent during the CONNECT handshake. As the original poster says, this may cause the CONNECT handshake and the subsequent request to have different UAs.

Test to reproduce the issue:


Index: tests/Zend/Http/Client/ProxyAdapterTest.php
===================================================================
--- tests/Zend/Http/Client/ProxyAdapterTest.php (revision 24490)
+++ tests/Zend/Http/Client/ProxyAdapterTest.php (working copy)
@@ -118,4 +118,73 @@
          * the TRACE response
          */
     }
+
+    /**
+     * @group ZF-3189
+     */
+    public function testConnectHandshakeSendsCustomUserAgentHeader()
+    {
+        // Change the adapter
+        $this->config['adapter'] = 'ZF3189_ProxyAdapter';
+        $this->config['useragent'] = 'ZendTest';
+        parent::setUp();
+
+        $base = preg_replace("/^http:/", "https:", $this->baseuri);
+        $this->client->setUri($base . 'testSimpleRequests.php');
+
+        // Ensure we're proxying a HTTPS request
+        $this->assertEquals('https', $this->client->getUri()->getScheme());
+
+        // Perform the request
+        $this->client->request();
+
+        $this->assertContains(
+            "User-agent: {$this->config['useragent']}",
+            $this->client->getAdapter()->getLastConnectHandshakeRequest()
+        );
+    }
+
+    /**
+     * @group ZF-3189
+     */
+    public function testConnectHandshakeSendsCustomUserAgentHeaderWhenSetInHeaders()
+    {
+        // Change the adapter
+        $this->config['adapter'] = 'ZF3189_ProxyAdapter';
+        parent::setUp();
+
+        $base = preg_replace("/^http:/", "https:", $this->baseuri);
+        $this->client->setUri($base . 'testSimpleRequests.php');
+        $this->client->setHeaders('User-Agent', 'ZendTest');
+
+        // Ensure we're proxying a HTTPS request
+        $this->assertEquals('https', $this->client->getUri()->getScheme());
+
+        // Perform the request
+        $this->client->request();
+        print_r($this->client->getAdapter()->getLastConnectHandshakeRequest());
+        $this->assertContains(
+            "User-agent: ZendTest",
+            $this->client->getAdapter()->getLastConnectHandshakeRequest()
+        );
+    }
+
 }
+
+/**
+ * Exposes internal variable connectHandshakeRequest for test purposes
+ * @see ZF-3189
+ */
+class ZF3189_ProxyAdapter extends Zend_Http_Client_Adapter_Proxy
+{
+
+    /**
+     * Retrieve the request data from last CONNECT handshake
+     * @return string
+     */
+    public function getLastConnectHandshakeRequest()
+    {
+        return $this->connectHandshakeRequest;
+    }
+
+}
\ No newline at end of file
Index: library/Zend/Http/Client/Adapter/Proxy.php
===================================================================
--- library/Zend/Http/Client/Adapter/Proxy.php  (revision 24490)
+++ library/Zend/Http/Client/Adapter/Proxy.php  (working copy)
@@ -75,6 +75,13 @@
      * @var boolean
      */
     protected $negotiated = false;
+
+    /**
+     * Stores the last CONNECT handshake request
+     *
+     * @var string
+     */
+    protected $connectHandshakeRequest;

     /**
      * Connect to the remote server
@@ -217,6 +224,9 @@
         }

         $request .= "\r\n";
+
+        // @see ZF-3189
+        $this->connectHandshakeRequest = $request;

         // Send the request
         if (! @fwrite($this->socket, $request)) {

The first test ({{testConnectHandshakeSendsCustomUserAgentHeader}}) will pass, as setting the user agent in the client configuration works as expected. However, the second test ({{testConnectHandshakeSendsCustomUserAgentHeaderWhenSetInHeaders}}) currently fails for the reason the OP suggested: {{connectHandshake}} does not check to see if {{$headers}}} defines a user agent header.

My reproduction test required a slight modification to {{Zend_Http_Client_Adapter_Proxy}} so that it would record the request headers sent during the {{connectHandshake}} method call. Rather than change the public API of {{Zend_Http_Client_Adapter_Proxy}}, I added a protected variable to store the request, then created a test-specific adapter class ({{ZF3189_ProxyAdapter}}) to expose it.

Suggested fix:


Index: library/Zend/Http/Client/Adapter/Proxy.php
===================================================================
--- library/Zend/Http/Client/Adapter/Proxy.php  (revision 24490)
+++ library/Zend/Http/Client/Adapter/Proxy.php  (working copy)
@@ -136,10 +136,20 @@
         }

         // Add Proxy-Authorization header
-        if ($this->config['proxy_user'] && ! isset($headers['proxy-authorization'])) {
-            $headers['proxy-authorization'] = Zend_Http_Client::encodeAuthHeader(
-                $this->config['proxy_user'], $this->config['proxy_pass'], $this->config['proxy_auth']
-            );
+        if ($this->config['proxy_user']) {
+            // Check to see if one already exists
+            $hasProxyAuthHeader = false;
+            foreach ($headers as $k => $v) {
+                if ($k == 'proxy-authorization' || preg_match("/^proxy-authorization:/i", $v) ) {
+                    $hasProxyAuthHeader = true;
+                    break;
+                }
+            }
+            if (!$hasProxyAuthHeader) {
+                $headers[] = 'Proxy-authorization: ' . Zend_Http_Client::encodeAuthHeader(
+                    $this->config['proxy_user'], $this->config['proxy_pass'], $this->config['proxy_auth']
+                );
+            }
         }

         // if we are proxying HTTPS, preform CONNECT handshake with the proxy
@@ -204,18 +214,18 @@
         $request = "CONNECT $host:$port HTTP/$http_ver\r\n" .
                    "Host: " . $this->config['proxy_host'] . "\r\n";

-        // Add the user-agent header
-        if (isset($this->config['useragent'])) {
-            $request .= "User-agent: " . $this->config['useragent'] . "\r\n";
+        // Process provided headers, including important ones to CONNECT request
+        foreach ( $headers as $k=>$v ) {
+            switch ( strtolower(substr($v,0,strpos($v,':'))) ) {
+                case 'proxy-authorization':
+                    // break intentionally omitted
+                case 'user-agent':
+                    $request .= $v . "\r\n";
+                    break;
+                default:
+                    break;
+            }
         }
-
-        // If the proxy-authorization header is set, send it to proxy but remove
-        // it from headers sent to target host
-        if (isset($headers['proxy-authorization'])) {
-            $request .= "Proxy-authorization: " . $headers['proxy-authorization'] . "\r\n";
-            unset($headers['proxy-authorization']);
-        }
-
         $request .= "\r\n";

         // Send the request

Two things here: * Rejig the authorization header processing {{Zend_Http_Client_Adapter_Proxy::write}} to properly detect an existing Proxy-Authorization header (old method assumed that $headers was a header=>value associative array, which it isn't. * Update {{Zend_Http_Client_Adapter_Proxy::connectHandshake}} to pass User-Agent and Proxy-Authorization headers through to CONNECT request.

I've also added a third test to enforce that {{Zend_Http_Client_Adapter_Proxy::write}} does indeed properly detect a preexisting Proxy-Authorization header:


    /**
     * @group ZF-3189
     */
    public function testProxyAdapterDoesNotOverwriteExistingProxyAuthorizationHeader()
    {      
        // Change the adapter
        $this->config['adapter'] = 'ZF3189_ProxyAdapter';
        parent::setUp();
        
        $base = preg_replace("/^http:/", "https:", $this->baseuri);
        $this->client->setUri($base . 'testSimpleRequests.php');
        $this->client->setHeaders('Proxy-Authorization', 'FooBarBaz');

        // Ensure we're proxying a HTTPS request
        $this->assertEquals('https', $this->client->getUri()->getScheme());
        
        // Perform the request
        $this->client->request();
        print_r($this->client->getAdapter()->getLastConnectHandshakeRequest());
        
        $resp = $this->client->getAdapter()->getLastConnectHandshakeRequest();
        $this->assertEquals(1, preg_match_all('/\r\nProxy-Authorization: ([^\r\n]+)\r\n/i', $resp, $matches));
        $this->assertEquals('FooBarBaz', $matches[1][0]);
    }

Thoughts?

I would like to have this fixed in ZF 1.12.0. Could someone with a proxy setup already in place test out this patch to ensure it works as expected?

Fixed in svn r24818.