Skip to content

Commit

Permalink
Complies with rfc7231 about statuses 307 and 308 (#5990)
Browse files Browse the repository at this point in the history 
* Complies with rfc7231 about statuses 307 and 308

Author:    Vladimir Kravets <vova.kravets@gmail.com>

* Add revert test

* Remove @NotNull annotations; we don’t use ’em.

Co-authored-by: Vladimir Kravets <vova.kravets@gmail.com>
Co-authored-by: Jesse Wilson <jesse@swank.ca>
  • Loading branch information
@yschimke @vkravets @swankjesse
3 people committed Apr 28, 2020
1 parent 269f556 commit 0cc3aef
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 38 deletions.
18 changes: 6 additions & 12 deletions okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt
View file
Expand Up @@ -222,16 +222,7 @@ class RetryAndFollowUpInterceptor(private val client: OkHttpClient) : Intercepto

HTTP_UNAUTHORIZED -> return client.authenticator.authenticate(route, userResponse)

HTTP_PERM_REDIRECT, HTTP_TEMP_REDIRECT -> {
// "If the 307 or 308 status code is received in response to a request other than GET
// or HEAD, the user agent MUST NOT automatically redirect the request"
if (method != "GET" && method != "HEAD") {
return null
}
return buildRedirectRequest(userResponse, method)
}

HTTP_MULT_CHOICE, HTTP_MOVED_PERM, HTTP_MOVED_TEMP, HTTP_SEE_OTHER -> {
HTTP_PERM_REDIRECT, HTTP_TEMP_REDIRECT, HTTP_MULT_CHOICE, HTTP_MOVED_PERM, HTTP_MOVED_TEMP, HTTP_SEE_OTHER -> {
return buildRedirectRequest(userResponse, method)
}

Expand Down Expand Up @@ -312,8 +303,11 @@ class RetryAndFollowUpInterceptor(private val client: OkHttpClient) : Intercepto
// Most redirects don't include a request body.
val requestBuilder = userResponse.request.newBuilder()
if (HttpMethod.permitsRequestBody(method)) {
val maintainBody = HttpMethod.redirectsWithBody(method)
if (HttpMethod.redirectsToGet(method)) {
val responseCode = userResponse.code
val maintainBody = HttpMethod.redirectsWithBody(method) ||
responseCode == HTTP_PERM_REDIRECT ||
responseCode == HTTP_TEMP_REDIRECT
if (HttpMethod.redirectsToGet(method) && responseCode != HTTP_PERM_REDIRECT && responseCode != HTTP_TEMP_REDIRECT) {
requestBuilder.method("GET", null)
} else {
val requestBody = if (maintainBody) userResponse.request.body else null
Expand Down
110 changes: 84 additions & 26 deletions okhttp/src/test/java/okhttp3/URLConnectionTest.java
View file
Expand Up @@ -86,6 +86,8 @@
import org.junit.Test;
import org.junit.rules.TemporaryFolder;

import static java.net.HttpURLConnection.HTTP_MOVED_PERM;
import static java.net.HttpURLConnection.HTTP_MOVED_TEMP;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Arrays.asList;
import static java.util.Locale.US;
Expand Down Expand Up @@ -2021,7 +2023,7 @@ private void testSecureStreamingPost(TransferKind streamingMode) throws Exceptio

private void testRedirected(TransferKind transferKind, boolean reuse) throws Exception {
MockResponse mockResponse = new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: /foo");
transferKind.setBody(mockResponse, "This page has moved!", 10);
server.enqueue(mockResponse);
Expand All @@ -2045,7 +2047,7 @@ private void testRedirected(TransferKind transferKind, boolean reuse) throws Exc
@Test public void redirectedOnHttps() throws Exception {
server.useHttps(handshakeCertificates.sslSocketFactory(), false);
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: /foo")
.setBody("This page has moved!"));
server.enqueue(new MockResponse()
Expand All @@ -2071,7 +2073,7 @@ private void testRedirected(TransferKind transferKind, boolean reuse) throws Exc
@Test public void notRedirectedFromHttpsToHttp() throws Exception {
server.useHttps(handshakeCertificates.sslSocketFactory(), false);
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: http://anyhost/foo")
.setBody("This page has moved!"));

Expand All @@ -2088,7 +2090,7 @@ private void testRedirected(TransferKind transferKind, boolean reuse) throws Exc

@Test public void notRedirectedFromHttpToHttps() throws Exception {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: https://anyhost/foo")
.setBody("This page has moved!"));

Expand All @@ -2106,7 +2108,7 @@ private void testRedirected(TransferKind transferKind, boolean reuse) throws Exc

server.useHttps(handshakeCertificates.sslSocketFactory(), false);
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: " + server2.url("/").url())
.setBody("This page has moved!"));

Expand All @@ -2127,7 +2129,7 @@ private void testRedirected(TransferKind transferKind, boolean reuse) throws Exc
.setBody("This is secure HTTPS!"));

server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: " + server2.url("/").url())
.setBody("This page has moved!"));

Expand Down Expand Up @@ -2167,7 +2169,7 @@ private void redirectToAnotherOriginServer(boolean https) throws Exception {
.setBody("This is the 2nd server, again!"));

server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: " + server2.url("/").url().toString())
.setBody("This page has moved!"));
server.enqueue(new MockResponse()
Expand Down Expand Up @@ -2213,7 +2215,7 @@ private void redirectToAnotherOriginServer(boolean https) throws Exception {
.setBody("This is the 2nd server!"));

server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: " + server2.url("/b").toString())
.setBody("This page has moved!"));

Expand Down Expand Up @@ -2253,19 +2255,19 @@ private void redirectToAnotherOriginServer(boolean https) throws Exception {
}

@Test public void response302MovedTemporarilyWithPost() throws Exception {
testResponseRedirectedWithPost(HttpURLConnection.HTTP_MOVED_TEMP, TransferKind.END_OF_STREAM);
testResponseRedirectedWithPost(HTTP_MOVED_TEMP, TransferKind.END_OF_STREAM);
}

@Test public void response303SeeOtherWithPost() throws Exception {
testResponseRedirectedWithPost(HttpURLConnection.HTTP_SEE_OTHER, TransferKind.END_OF_STREAM);
}

@Test public void postRedirectToGetWithChunkedRequest() throws Exception {
testResponseRedirectedWithPost(HttpURLConnection.HTTP_MOVED_TEMP, TransferKind.CHUNKED);
testResponseRedirectedWithPost(HTTP_MOVED_TEMP, TransferKind.CHUNKED);
}

@Test public void postRedirectToGetWithStreamedRequest() throws Exception {
testResponseRedirectedWithPost(HttpURLConnection.HTTP_MOVED_TEMP, TransferKind.FIXED_LENGTH);
testResponseRedirectedWithPost(HTTP_MOVED_TEMP, TransferKind.FIXED_LENGTH);
}

private void testResponseRedirectedWithPost(int redirectCode, TransferKind transferKind)
Expand Down Expand Up @@ -2294,7 +2296,7 @@ private void testResponseRedirectedWithPost(int redirectCode, TransferKind trans

@Test public void redirectedPostStripsRequestBodyHeaders() throws Exception {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: /page2"));
server.enqueue(new MockResponse()
.setBody("Page 2"));
Expand Down Expand Up @@ -2367,6 +2369,71 @@ private void testResponseRedirectedWithPost(int redirectCode, TransferKind trans
testRedirect(false, "POST");
}

class RevertInterceptor implements Interceptor {
@Override public Response intercept(Chain chain) throws IOException {
Response response = chain.proceed(chain.request());

return remapResponse(response);
}

private Response remapResponse(Response response) {
if (response.request().method().equals("POST") && (response.code() == HTTP_TEMP_REDIRECT || response.code() == HTTP_PERM_REDIRECT)) {
// special response code to indicate custom rules
return response.newBuilder().code(response.code() + 10).build();
}

return response;
}
}

@Test public void response307WithPostReverted() throws Exception {
client = client.newBuilder().addNetworkInterceptor(new RevertInterceptor()).build();

MockResponse response1 = new MockResponse()
.setResponseCode(HTTP_TEMP_REDIRECT)
.setBody("This page has moved!")
.addHeader("Location: /page2");
server.enqueue(response1);

Request.Builder requestBuilder = new Request.Builder()
.url(server.url("/page1"));
requestBuilder.post(RequestBody.create("ABCD", null));

Response response = getResponse(requestBuilder.build());
String responseString = readAscii(response.body().byteStream(), Integer.MAX_VALUE);

RecordedRequest page1 = server.takeRequest();
assertThat(page1.getRequestLine()).isEqualTo(("POST /page1 HTTP/1.1"));

assertThat(page1.getBody().readUtf8()).isEqualTo("ABCD");
assertThat(server.getRequestCount()).isEqualTo(1);
assertThat(responseString).isEqualTo("This page has moved!");
}

@Test public void response308WithPostReverted() throws Exception {
client = client.newBuilder().addNetworkInterceptor(new RevertInterceptor()).build();

MockResponse response1 = new MockResponse()
.setResponseCode(HTTP_PERM_REDIRECT)
.setBody("This page has moved!")
.addHeader("Location: /page2");
server.enqueue(response1);

Request.Builder requestBuilder = new Request.Builder()
.url(server.url("/page1"));
requestBuilder.post(RequestBody.create("ABCD", null));

Response response = getResponse(requestBuilder.build());
String responseString = readAscii(response.body().byteStream(), Integer.MAX_VALUE);

RecordedRequest page1 = server.takeRequest();
assertThat(page1.getRequestLine()).isEqualTo(("POST /page1 HTTP/1.1"));

assertThat(page1.getBody().readUtf8()).isEqualTo("ABCD");
assertThat(server.getRequestCount()).isEqualTo(1);
assertThat(responseString).isEqualTo("This page has moved!");
}

private void testRedirect(boolean temporary, String method) throws Exception {
MockResponse response1 = new MockResponse()
.setResponseCode(temporary ? HTTP_TEMP_REDIRECT : HTTP_PERM_REDIRECT)
Expand Down Expand Up @@ -2396,17 +2463,8 @@ private void testRedirect(boolean temporary, String method) throws Exception {
assertThat(responseString).isEqualTo("Page 2");
} else if (method.equals("HEAD")) {
assertThat(responseString).isEqualTo("");
} else {
// Methods other than GET/HEAD shouldn't follow the redirect.
if (method.equals("POST")) {
assertThat(page1.getBody().readUtf8()).isEqualTo("ABCD");
}
assertThat(server.getRequestCount()).isEqualTo(1);
assertThat(responseString).isEqualTo("This page has moved!");
return;
}

// GET/HEAD requests should have followed the redirect with the same method.
assertThat(server.getRequestCount()).isEqualTo(2);
RecordedRequest page2 = server.takeRequest();
assertThat(page2.getRequestLine()).isEqualTo((method + " /page2 HTTP/1.1"));
Expand All @@ -2415,7 +2473,7 @@ private void testRedirect(boolean temporary, String method) throws Exception {
@Test public void follow20Redirects() throws Exception {
for (int i = 0; i < 20; i++) {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: /" + (i + 1))
.setBody("Redirecting to /" + (i + 1)));
}
Expand All @@ -2430,7 +2488,7 @@ private void testRedirect(boolean temporary, String method) throws Exception {
@Test public void doesNotFollow21Redirects() throws Exception {
for (int i = 0; i < 21; i++) {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: /" + (i + 1))
.setBody("Redirecting to /" + (i + 1)));
}
Expand Down Expand Up @@ -2653,7 +2711,7 @@ protected ServerSocket configureServerSocket(ServerSocket serverSocket)

@Test public void connectionCloseWithRedirect() throws Exception {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: /foo")
.addHeader("Connection: close"));
server.enqueue(new MockResponse()
Expand All @@ -2675,7 +2733,7 @@ protected ServerSocket configureServerSocket(ServerSocket serverSocket)
*/
@Test public void sameConnectionRedirectAndReuse() throws Exception {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.setSocketPolicy(SHUTDOWN_INPUT_AT_END)
.addHeader("Location: /foo"));
server.enqueue(new MockResponse()
Expand Down Expand Up @@ -3506,7 +3564,7 @@ private void zeroLengthPayload(String method) throws Exception {
*/
@Test public void gzipWithRedirectAndConnectionReuse() throws Exception {
server.enqueue(new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.setResponseCode(HTTP_MOVED_TEMP)
.addHeader("Location: /foo")
.addHeader("Content-Encoding: gzip")
.setBody(gzip("Moved! Moved! Moved!")));
Expand Down

0 comments on commit 0cc3aef

Please sign in to comment.