From 2bcffdec4c5a1e705858d977fc26177ea3554744 Mon Sep 17 00:00:00 2001 From: jwilson Date: Sat, 2 Feb 2013 09:24:16 -0500 Subject: [PATCH] Don't make the 'proxy' field do double-duty. Previously it was acting as both the user's requested proxy and also the proxy selector's selection. This breaks down, especially since redirects permit multiple proxy selections for a single request. --- .../okhttp/internal/http/HttpEngine.java | 15 ++--- .../internal/http/HttpURLConnectionImpl.java | 43 +++++------- .../internal/http/URLConnectionTest.java | 65 +++++++++++++++---- 3 files changed, 79 insertions(+), 44 deletions(-) diff --git a/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java b/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java index 8d4070528..947bb690d 100644 --- a/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java +++ b/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java @@ -272,9 +272,8 @@ public class HttpEngine { if (uriHost == null) { throw new UnknownHostException(uri.toString()); } - Address address = - new Address(uriHost, getEffectivePort(uri), getSslSocketFactory(), getHostnameVerifier(), - policy.getProxy()); + Address address = new Address(uriHost, getEffectivePort(uri), getSslSocketFactory(), + getHostnameVerifier(), policy.requestedProxy); routeSelector = new RouteSelector(address, uri, policy.proxySelector, policy.connectionPool, Dns.DEFAULT); } @@ -284,10 +283,8 @@ public class HttpEngine { policy.connectionPool.maybeShare(connection); } connected(connection); - Proxy proxy = connection.getProxy(); - if (proxy != null) { - policy.setProxy(proxy); - // Add the authority to the request line when we're using a proxy. + if (connection.getProxy() != policy.requestedProxy) { + // Update the request line if the proxy changed; it may need a host name. requestHeaders.getHeaders().setRequestLine(getRequestLine()); } } @@ -574,7 +571,9 @@ public class HttpEngine { * full URL, even if a proxy is in use. */ protected boolean includeAuthorityInRequestLine() { - return policy.usingProxy(); + return connection == null + ? policy.usingProxy() // A proxy was requested. + : connection.getProxy().type() == Proxy.Type.HTTP; // A proxy was selected. } /** diff --git a/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java b/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java index 40cd516f2..051869d28 100644 --- a/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java +++ b/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java @@ -65,7 +65,9 @@ public class HttpURLConnectionImpl extends HttpURLConnection { private final int defaultPort; - private Proxy proxy; + /** The proxy requested by the client, or null for a proxy to be selected automatically. */ + final Proxy requestedProxy; + final ProxySelector proxySelector; final CookieHandler cookieHandler; final ResponseCache responseCache; @@ -82,7 +84,7 @@ public class HttpURLConnectionImpl extends HttpURLConnection { CookieHandler cookieHandler, ResponseCache responseCache, ConnectionPool connectionPool) { super(url); this.defaultPort = defaultPort; - this.proxy = proxy; + this.requestedProxy = proxy; this.proxySelector = proxySelector; this.cookieHandler = cookieHandler; this.responseCache = responseCache; @@ -214,18 +216,14 @@ public class HttpURLConnectionImpl extends HttpURLConnection { } @Override public final Permission getPermission() throws IOException { - String connectToAddress = getConnectToHost() + ":" + getConnectToPort(); - return new SocketPermission(connectToAddress, "connect, resolve"); - } - - private String getConnectToHost() { - return usingProxy() ? ((InetSocketAddress) proxy.address()).getHostName() : getURL().getHost(); - } - - private int getConnectToPort() { - int hostPort = - usingProxy() ? ((InetSocketAddress) proxy.address()).getPort() : getURL().getPort(); - return hostPort < 0 ? getDefaultPort() : hostPort; + String hostName = getURL().getHost(); + int hostPort = Util.getEffectivePort(getURL()); + if (usingProxy()) { + InetSocketAddress proxyAddress = (InetSocketAddress) requestedProxy.address(); + hostName = proxyAddress.getHostName(); + hostPort = proxyAddress.getPort(); + } + return new SocketPermission(hostName + ":" + hostPort, "connect, resolve"); } @Override public final String getRequestProperty(String field) { @@ -385,15 +383,18 @@ public class HttpURLConnectionImpl extends HttpURLConnection { * prepare for a follow up request. */ private Retry processResponseHeaders() throws IOException { + Proxy selectedProxy = httpEngine.connection != null + ? httpEngine.connection.getProxy() + : requestedProxy; switch (getResponseCode()) { case HTTP_PROXY_AUTH: - if (!usingProxy()) { + if (selectedProxy.type() != Proxy.Type.HTTP) { throw new ProtocolException("Received HTTP_PROXY_AUTH (407) code while not using proxy"); } // fall-through case HTTP_UNAUTHORIZED: boolean credentialsFound = HttpAuthenticator.processAuthHeader(getResponseCode(), - httpEngine.getResponseHeaders().getHeaders(), rawRequestHeaders, proxy, url); + httpEngine.getResponseHeaders().getHeaders(), rawRequestHeaders, selectedProxy, url); return credentialsFound ? Retry.SAME_CONNECTION : Retry.NONE; case HTTP_MULT_CHOICE: @@ -441,16 +442,8 @@ public class HttpURLConnectionImpl extends HttpURLConnection { return chunkLength; } - final Proxy getProxy() { - return proxy; - } - - final void setProxy(Proxy proxy) { - this.proxy = proxy; - } - @Override public final boolean usingProxy() { - return (proxy != null && proxy.type() != Proxy.Type.DIRECT); + return (requestedProxy != null && requestedProxy.type() != Proxy.Type.DIRECT); } @Override public String getResponseMessage() throws IOException { diff --git a/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java b/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java index 2e7c598ec..6b348c241 100644 --- a/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java +++ b/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java @@ -1557,8 +1557,24 @@ public final class URLConnectionTest { } @Test public void redirectToAnotherOriginServer() throws Exception { - MockWebServer server2 = new MockWebServer(); + redirectToAnotherOriginServer(false); + } + + @Test public void redirectToAnotherOriginServerWithHttps() throws Exception { + redirectToAnotherOriginServer(true); + } + + private void redirectToAnotherOriginServer(boolean https) throws Exception { + server2 = new MockWebServer(); + if (https) { + server.useHttps(sslContext.getSocketFactory(), false); + server2.useHttps(sslContext.getSocketFactory(), false); + client.setSSLSocketFactory(sslContext.getSocketFactory()); + client.setHostnameVerifier(new RecordingHostnameVerifier()); + } + server2.enqueue(new MockResponse().setBody("This is the 2nd server!")); + server2.enqueue(new MockResponse().setBody("This is the 2nd server, again!")); server2.play(); server.enqueue(new MockResponse().setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP) @@ -1568,20 +1584,47 @@ public final class URLConnectionTest { server.play(); URLConnection connection = client.open(server.getUrl("/")); - assertEquals("This is the 2nd server!", - readAscii(connection.getInputStream(), Integer.MAX_VALUE)); + assertContent("This is the 2nd server!", connection); assertEquals(server2.getUrl("/"), connection.getURL()); // make sure the first server was careful to recycle the connection - assertEquals("This is the first server again!", - readAscii(client.open(server.getUrl("/")).getInputStream(), Integer.MAX_VALUE)); + assertContent("This is the first server again!", client.open(server.getUrl("/"))); + assertContent("This is the 2nd server, again!", client.open(server2.getUrl("/"))); - RecordedRequest first = server.takeRequest(); - assertContains(first.getHeaders(), "Host: " + hostName + ":" + server.getPort()); - RecordedRequest second = server2.takeRequest(); - assertContains(second.getHeaders(), "Host: " + hostName + ":" + server2.getPort()); - RecordedRequest third = server.takeRequest(); - assertEquals("Expected connection reuse", 1, third.getSequenceNumber()); + String server1Host = hostName + ":" + server.getPort(); + String server2Host = hostName + ":" + server2.getPort(); + assertContains(server.takeRequest().getHeaders(), "Host: " + server1Host); + assertContains(server2.takeRequest().getHeaders(), "Host: " + server2Host); + assertEquals("Expected connection reuse", 1, server.takeRequest().getSequenceNumber()); + assertEquals("Expected connection reuse", 1, server2.takeRequest().getSequenceNumber()); + } + + @Test public void redirectWithProxySelector() throws Exception { + final List proxySelectionRequests = new ArrayList(); + client.setProxySelector(new ProxySelector() { + @Override public List select(URI uri) { + proxySelectionRequests.add(uri); + MockWebServer proxyServer = (uri.getPort() == server.getPort()) ? server : server2; + return Arrays.asList(proxyServer.toProxyAddress()); + } + @Override public void connectFailed(URI uri, SocketAddress address, IOException failure) { + throw new AssertionError(); + } + }); + + server2 = new MockWebServer(); + server2.enqueue(new MockResponse().setBody("This is the 2nd server!")); + server2.play(); + + server.enqueue(new MockResponse().setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP) + .addHeader("Location: " + server2.getUrl("/b").toString()) + .setBody("This page has moved!")); + server.play(); + + assertContent("This is the 2nd server!", client.open(server.getUrl("/a"))); + + assertEquals(Arrays.asList(server.getUrl("/a").toURI(), server2.getUrl("/b").toURI()), + proxySelectionRequests); server2.shutdown(); }