From e49dd7a2f08deac92fab3de2b8a63e90b588f746 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Tue, 30 Dec 2014 09:37:30 -0500 Subject: [PATCH] Limit 20 authorization attempts. We use one count for both redirects and authorization attempts. This seems like good enough policy. Closes https://github.com/square/okhttp/issues/960 --- .../java/com/squareup/okhttp/CallTest.java | 35 +++++++++++++++++-- .../internal/http/URLConnectionTest.java | 33 ++++++++++++++++- .../internal/huc/HttpURLConnectionImpl.java | 6 ++-- .../main/java/com/squareup/okhttp/Call.java | 8 ++--- .../okhttp/internal/http/HttpEngine.java | 6 ++-- 5 files changed, 75 insertions(+), 13 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/CallTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/CallTest.java index 795f34112..b4efd6759 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/CallTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/CallTest.java @@ -328,6 +328,37 @@ public final class CallTest { assertEquals(credential, recordedRequest2.getHeader("Authorization")); } + @Test public void attemptAuthorization20Times() throws Exception { + for (int i = 0; i < 20; i++) { + server.enqueue(new MockResponse().setResponseCode(401)); + } + server.enqueue(new MockResponse().setBody("Success!")); + + String credential = Credentials.basic("jesse", "secret"); + client.setAuthenticator(new RecordingOkAuthenticator(credential)); + + Request request = new Request.Builder().url(server.getUrl("/")).build(); + executeSynchronously(request) + .assertCode(200) + .assertBody("Success!"); + } + + @Test public void doesNotAttemptAuthorization21Times() throws Exception { + for (int i = 0; i < 21; i++) { + server.enqueue(new MockResponse().setResponseCode(401)); + } + + String credential = Credentials.basic("jesse", "secret"); + client.setAuthenticator(new RecordingOkAuthenticator(credential)); + + try { + client.newCall(new Request.Builder().url(server.getUrl("/0")).build()).execute(); + fail(); + } catch (IOException expected) { + assertEquals("Too many follow-up requests: 21", expected.getMessage()); + } + } + @Test public void delete() throws Exception { server.enqueue(new MockResponse().setBody("abc")); @@ -1249,7 +1280,7 @@ public final class CallTest { client.newCall(new Request.Builder().url(server.getUrl("/0")).build()).execute(); fail(); } catch (IOException expected) { - assertEquals("Too many redirects: 21", expected.getMessage()); + assertEquals("Too many follow-up requests: 21", expected.getMessage()); } } @@ -1263,7 +1294,7 @@ public final class CallTest { Request request = new Request.Builder().url(server.getUrl("/0")).build(); client.newCall(request).enqueue(callback); - callback.await(server.getUrl("/20")).assertFailure("Too many redirects: 21"); + callback.await(server.getUrl("/20")).assertFailure("Too many follow-up requests: 21"); } @Test public void canceledBeforeExecute() throws Exception { diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java index 9ac183e68..7f7362519 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/URLConnectionTest.java @@ -2111,7 +2111,7 @@ public final class URLConnectionTest { fail(); } catch (ProtocolException expected) { assertEquals(HttpURLConnection.HTTP_MOVED_TEMP, connection.getResponseCode()); - assertEquals("Too many redirects: 21", expected.getMessage()); + assertEquals("Too many follow-up requests: 21", expected.getMessage()); assertContent("Redirecting to /21", connection); assertEquals(server.getUrl("/20"), connection.getURL()); } @@ -2786,6 +2786,37 @@ public final class URLConnectionTest { assertEquals("/a", redirectedBy.request().url().getPath()); } + @Test public void attemptAuthorization20Times() throws Exception { + for (int i = 0; i < 20; i++) { + server.enqueue(new MockResponse().setResponseCode(401)); + } + server.enqueue(new MockResponse().setBody("Success!")); + + String credential = Credentials.basic("jesse", "peanutbutter"); + client.client().setAuthenticator(new RecordingOkAuthenticator(credential)); + + connection = client.open(server.getUrl("/0")); + assertContent("Success!", connection); + } + + @Test public void doesNotAttemptAuthorization21Times() throws Exception { + for (int i = 0; i < 21; i++) { + server.enqueue(new MockResponse().setResponseCode(401)); + } + + String credential = Credentials.basic("jesse", "peanutbutter"); + client.client().setAuthenticator(new RecordingOkAuthenticator(credential)); + + connection = client.open(server.getUrl("/")); + try { + connection.getInputStream(); + fail(); + } catch (ProtocolException expected) { + assertEquals(401, connection.getResponseCode()); + assertEquals("Too many follow-up requests: 21", expected.getMessage()); + } + } + @Test public void setsNegotiatedProtocolHeader_SPDY_3() throws Exception { setsNegotiatedProtocolHeader(Protocol.SPDY_3); } diff --git a/okhttp-urlconnection/src/main/java/com/squareup/okhttp/internal/huc/HttpURLConnectionImpl.java b/okhttp-urlconnection/src/main/java/com/squareup/okhttp/internal/huc/HttpURLConnectionImpl.java index 024966ca1..04ac55206 100644 --- a/okhttp-urlconnection/src/main/java/com/squareup/okhttp/internal/huc/HttpURLConnectionImpl.java +++ b/okhttp-urlconnection/src/main/java/com/squareup/okhttp/internal/huc/HttpURLConnectionImpl.java @@ -80,7 +80,7 @@ public class HttpURLConnectionImpl extends HttpURLConnection { /** Like the superclass field of the same name, but a long and available on all platforms. */ private long fixedContentLength = -1; - private int redirectionCount; + private int followUpCount; protected IOException httpEngineFailure; protected HttpEngine httpEngine; /** Lazily created (with synthetic headers) on first call to getHeaders(). */ @@ -385,8 +385,8 @@ public class HttpURLConnectionImpl extends HttpURLConnection { return httpEngine; } - if (response.isRedirect() && ++redirectionCount > HttpEngine.MAX_REDIRECTS) { - throw new ProtocolException("Too many redirects: " + redirectionCount); + if (++followUpCount > HttpEngine.MAX_FOLLOW_UPS) { + throw new ProtocolException("Too many follow-up requests: " + followUpCount); } // The first request was insufficient. Prepare for another... diff --git a/okhttp/src/main/java/com/squareup/okhttp/Call.java b/okhttp/src/main/java/com/squareup/okhttp/Call.java index 85fcedb83..b0118d313 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/Call.java +++ b/okhttp/src/main/java/com/squareup/okhttp/Call.java @@ -24,7 +24,7 @@ import java.net.URL; import java.util.logging.Level; import static com.squareup.okhttp.internal.Internal.logger; -import static com.squareup.okhttp.internal.http.HttpEngine.MAX_REDIRECTS; +import static com.squareup.okhttp.internal.http.HttpEngine.MAX_FOLLOW_UPS; /** * A call is a request that has been prepared for execution. A call can be @@ -251,7 +251,7 @@ public class Call { // Create the initial HTTP engine. Retries and redirects need new engine for each attempt. engine = new HttpEngine(client, request, false, false, forWebSocket, null, null, null, null); - int redirectionCount = 0; + int followUpCount = 0; while (true) { if (canceled) { engine.releaseConnection(); @@ -282,8 +282,8 @@ public class Call { return response; } - if (engine.getResponse().isRedirect() && ++redirectionCount > MAX_REDIRECTS) { - throw new ProtocolException("Too many redirects: " + redirectionCount); + if (++followUpCount > MAX_FOLLOW_UPS) { + throw new ProtocolException("Too many follow-up requests: " + followUpCount); } if (!engine.sameConnection(followUp.url())) { diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java index 8485e270a..007244b49 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java @@ -88,10 +88,10 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; */ public final class HttpEngine { /** - * How many redirects should we follow? Chrome follows 21; Firefox, curl, - * and wget follow 20; Safari follows 16; and HTTP/1.0 recommends 5. + * How many redirects and auth challenges should we attempt? Chrome follows 21 redirects; Firefox, + * curl, and wget follow 20; Safari follows 16; and HTTP/1.0 recommends 5. */ - public static final int MAX_REDIRECTS = 20; + public static final int MAX_FOLLOW_UPS = 20; private static final ResponseBody EMPTY_BODY = new ResponseBody() { @Override public MediaType contentType() {