Merge lp:~lifeless/bzr/bug-558343-wrong-host-with-proxy into lp:bzr

[Robert Collins]

Fix lp: urls behind an https proxy.

This should fix using lp: urls from behind an https proxy; no tests - it appears remarkably opaque :(.

Would love vincent's opinion on it, as I think he knows urllib2 guts much better than I.

[Robert Collins (lifeless)]

Oh, and as I'm travelling to UDS, please land if its ok (or fix and land) - this is fairly high importance IMO.

[Vincent Ladeuil (vila)]

At first glance, the following line:

26 + self.add_unredirected_header('Host', self.proxied_host)

looks obvious.

From memory, I'm pretty sure the Host header is added just before sending the request
if it's not there already. Yet, I also have some vague feeling that playing with *this* header
led to trouble in the past (this fix could have avoid that may be).

I'd blame the lack of test for this omission. Am I right in thinking that
the bug requires a redirection behind the proxy to trigger ?

If yes, that could explain why we didn't get more reports (and the NEWS entry may
be more precise then).

Since proxy and redirection tests are pretty hackish so far, there may be a way to test
this with pre-canned responses (I think there is some test http
server than can be used for that).

I won't have time to look at it until UDS so let's talk about it there.

Otherwise, thanks for the cleanup, we may also want to use 'http' in debug_flags instead of
debug_level, that will help diagnosing issues in the future.

[Robert Collins (lifeless)]

Yes, when I stepped through the code, urllib2 adds a host header if
there isn't one, and its grabbing the wrong host value because
'has_proxy' is returning False.

And yes, you have to have an https proxy defined to trigger this - see
my comments in the bug about reproducing it.

Sounds like you think this is ok? Could you land it(I'm hopping on a
plane in a minute for the next leg).

Thanks,
Rob

[Vincent Ladeuil (vila)]

I found time to look at the code. There is already a huge list of missing tests under a FIXME there (including the one that is missing here).

And yes, the fix is right and urllib2 didn't have the bug because it didn't implement CONNECT anyway.

review: Approve

[Robert Collins (lifeless)]

submitted to PQM by hand.

[Robert Collins (lifeless)]

submitted to PQM by hand.

[Robert Collins (lifeless)]

submitted to PQM by hand.

[Robert Collins (lifeless)]

submitted to PQM by hand.

[Robert Collins (lifeless)]

submitted to PQM by hand.

=== modified file 'NEWS'
--- NEWS	2010-05-06 06:51:11 +0000
+++ NEWS	2010-05-08 06:00:46 +0000
@@ -101,6 +101,9 @@
   messages.
   (Parth Malwankar, #563646)
 
+* Using bzr with `lp:` urls behind an http proxy should work.
+  (Robert Collins, #558343)
+
 Improvements
 ************
 
 
=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2010-02-17 17:11:16 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2010-05-08 06:00:46 +0000
@@ -378,6 +378,11 @@
             port = conn_class.default_port
         self.proxied_host = '%s:%s' % (host, port)
         urllib2.Request.set_proxy(self, proxy, type)
+        # When urllib2 makes a https request with our wrapper code and a proxy,
+        # it sets Host to the https proxy, not the host we want to talk to.
+        # I'm fairly sure this is our fault, but what is the cause is an open
+        # question. -- Robert Collins May 8 2010.
+        self.add_unredirected_header('Host', self.proxied_host)
 
 
 class _ConnectRequest(Request):
@@ -711,7 +716,7 @@
             connect = _ConnectRequest(request)
             response = self.parent.open(connect)
             if response.code != 200:
-                raise ConnectionError("Can't connect to %s via proxy %s" % (
+                raise errors.ConnectionError("Can't connect to %s via proxy %s" % (
                         connect.proxied_host, self.host))
             # Housekeeping
             connection.cleanup_pipe()
@@ -868,21 +873,19 @@
                 print 'Will unbind %s_open for %r' % (type, proxy)
             delattr(self, '%s_open' % type)
 
+        def bind_scheme_request(proxy, scheme):
+            if proxy is None:
+                return
+            scheme_request = scheme + '_request'
+            if self._debuglevel >= 3:
+                print 'Will bind %s for %r' % (scheme_request, proxy)
+            setattr(self, scheme_request,
+                lambda request: self.set_proxy(request, scheme))
         # We are interested only by the http[s] proxies
         http_proxy = self.get_proxy_env_var('http')
+        bind_scheme_request(http_proxy, 'http')
         https_proxy = self.get_proxy_env_var('https')
-
-        if http_proxy is not None:
-            if self._debuglevel >= 3:
-                print 'Will bind http_request for %r' % http_proxy
-            setattr(self, 'http_request',
-                    lambda request: self.set_proxy(request, 'http'))
-
-        if https_proxy is not None:
-            if self._debuglevel >= 3:
-                print 'Will bind http_request for %r' % https_proxy
-            setattr(self, 'https_request',
-                    lambda request: self.set_proxy(request, 'https'))
+        bind_scheme_request(https_proxy, 'https')
 
     def get_proxy_env_var(self, name, default_to='all'):
         """Get a proxy env var.