Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complies with rfc7231 about statuses 307 and 308 #5990

Merged
merged 3 commits into from
Apr 28, 2020
Merged

Complies with rfc7231 about statuses 307 and 308 #5990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a7f5bc
Complies with rfc7231 about statuses 307 and 308
vkravets Sep 14, 2018
31d666b
Add revert test
yschimke Apr 26, 2020
44a868d
Remove @NotNull annotations; we don’t use ’em.
swankjesse Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh this should probably be checking for not GET and not HEAD, otherwise PUT isn‘t consistent!

// 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