Skip to content

Commit

Permalink
Limit 20 authorization attempts.
Browse files Browse the repository at this point in the history
We use one count for both redirects and authorization attempts. This
seems like good enough policy.

Closes square#960
  • Loading branch information
squarejesse committed Dec 30, 2014
1 parent b6a8da4 commit e49dd7a
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 13 deletions.
35 changes: 33 additions & 2 deletions okhttp-tests/src/test/java/com/squareup/okhttp/CallTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,37 @@ private void postBodyRetransmittedAfterAuthorizationFail(String body) throws Exc
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"));

Expand Down Expand Up @@ -1249,7 +1280,7 @@ private void postBodyRetransmittedAfterAuthorizationFail(String body) throws Exc
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());
}
}

Expand All @@ -1263,7 +1294,7 @@ private void postBodyRetransmittedAfterAuthorizationFail(String body) throws Exc

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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2111,7 +2111,7 @@ private void testRedirect(boolean temporary, String method) throws Exception {
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());
}
Expand Down Expand Up @@ -2786,6 +2786,37 @@ private void reusedConnectionFailsWithPost(TransferKind transferKind, int reques
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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(). */
Expand Down Expand Up @@ -385,8 +385,8 @@ private HttpEngine getResponse() throws IOException {
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...
Expand Down
8 changes: 4 additions & 4 deletions okhttp/src/main/java/com/squareup/okhttp/Call.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
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
Expand Down Expand Up @@ -251,7 +251,7 @@ Response getResponse(Request request, boolean forWebSocket) throws IOException {
// 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();
Expand Down Expand Up @@ -282,8 +282,8 @@ Response getResponse(Request request, boolean forWebSocket) throws IOException {
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())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,10 @@
*/
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() {
Expand Down

0 comments on commit e49dd7a

Please sign in to comment.