Code review comment for lp:~lifeless/bzr/bug-558343-wrong-host-with-proxy

Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

« Back to merge proposal