Skip to content

SDK / exoplayer

Go to a project
Commit 1e9b6d66 by olly Committed by Oliver Woodman
Options

[CronetDataSource] Support keeping the POST method and body for 302 

Currently when a HTTP POST request receives a 302, CronetDataSource will change the request method from POST to GET for the redirected request, and drop the post body. This aligns with the behaviours of many user agents, but our use case would like to keep the POST method and the post body.

org.chromium.net.UrlRequest.followRedirect also changes POST to GET for 302, so should be avoided here.

PiperOrigin-RevId: 381233011
parent fc26d4ee
Inline Side-by-side
Showing with 232 additions and 36 deletions
extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java
......@@ -90,6 +90,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
private int readTimeoutMs;
private boolean resetTimeoutOnRedirects;
private boolean handleSetCookieRequests;
private boolean keepPostFor302Redirects;
/**
* Creates an instance.
......@@ -246,6 +247,18 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
}
/**
* Sets whether we should keep the POST method and body when we have HTTP 302 redirects for a
* POST request.
*/
public Factory setKeepPostFor302Redirects(boolean keepPostFor302Redirects) {
this.keepPostFor302Redirects = keepPostFor302Redirects;
if (internalFallbackFactory != null) {
internalFallbackFactory.setKeepPostFor302Redirects(keepPostFor302Redirects);
}
return this;
}
/**
* Sets the {@link TransferListener} that will be used.
*
* <p>The default is {@code null}.
......@@ -297,7 +310,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
handleSetCookieRequests,
userAgent,
defaultRequestProperties,
contentTypePredicate);
contentTypePredicate,
keepPostFor302Redirects);
if (transferListener != null) {
dataSource.addTransferListener(transferListener);
}
......@@ -348,6 +362,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
private final Clock clock;
@Nullable private Predicate<String> contentTypePredicate;
private final boolean keepPostFor302Redirects;
// Accessed by the calling thread only.
private boolean opened;
......@@ -402,7 +417,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
/* handleSetCookieRequests= */ false,
/* userAgent= */ null,
defaultRequestProperties,
/* contentTypePredicate= */ null);
/* contentTypePredicate= */ null,
/* keepPostFor302Redirects */ false);
}
/** @deprecated Use {@link CronetDataSource.Factory} instead. */
......@@ -424,7 +440,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
handleSetCookieRequests,
/* userAgent= */ null,
defaultRequestProperties,
/* contentTypePredicate= */ null);
/* contentTypePredicate= */ null,
/* keepPostFor302Redirects */ false);
}
/** @deprecated Use {@link CronetDataSource.Factory} instead. */
......@@ -486,10 +503,11 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
handleSetCookieRequests,
/* userAgent= */ null,
defaultRequestProperties,
contentTypePredicate);
contentTypePredicate,
/* keepPostFor302Redirects */ false);
}
private CronetDataSource(
protected CronetDataSource(
CronetEngine cronetEngine,
Executor executor,
int connectTimeoutMs,
......@@ -498,7 +516,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
boolean handleSetCookieRequests,
@Nullable String userAgent,
@Nullable RequestProperties defaultRequestProperties,
@Nullable Predicate<String> contentTypePredicate) {
@Nullable Predicate<String> contentTypePredicate,
boolean keepPostFor302Redirects) {
super(/* isNetwork= */ true);
this.cronetEngine = Assertions.checkNotNull(cronetEngine);
this.executor = Assertions.checkNotNull(executor);
......@@ -509,6 +528,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
this.userAgent = userAgent;
this.defaultRequestProperties = defaultRequestProperties;
this.contentTypePredicate = contentTypePredicate;
this.keepPostFor302Redirects = keepPostFor302Redirects;
clock = Clock.DEFAULT;
urlRequestCallback = new UrlRequestCallback();
requestProperties = new RequestProperties();
......@@ -1009,11 +1029,15 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
return false;
}
private static String parseCookies(List<String> setCookieHeaders) {
@Nullable
private static String parseCookies(@Nullable List<String> setCookieHeaders) {
if (setCookieHeaders == null || setCookieHeaders.isEmpty()) {
return null;
}
return TextUtils.join(";", setCookieHeaders);
}
private static void attachCookies(UrlRequest.Builder requestBuilder, String cookies) {
private static void attachCookies(UrlRequest.Builder requestBuilder, @Nullable String cookies) {
if (TextUtils.isEmpty(cookies)) {
return;
}
......@@ -1062,8 +1086,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
}
UrlRequest urlRequest = Assertions.checkNotNull(currentUrlRequest);
DataSpec dataSpec = Assertions.checkNotNull(currentDataSpec);
if (dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) {
int responseCode = info.getHttpStatusCode();
if (dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) {
// The industry standard is to disregard POST redirects when the status code is 307 or 308.
if (responseCode == 307 || responseCode == 308) {
exception =
......@@ -1081,22 +1105,30 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
resetConnectTimeout();
}
if (!handleSetCookieRequests) {
boolean shouldKeepPost =
keepPostFor302Redirects
&& dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST
&& responseCode == 302;
// request.followRedirect() transforms a POST request into a GET request, so if we want to
// keep it as a POST we need to fall through to the manual redirect logic below.
if (!shouldKeepPost && !handleSetCookieRequests) {
request.followRedirect();
return;
}
@Nullable List<String> setCookieHeaders = info.getAllHeaders().get(HttpHeaders.SET_COOKIE);
if (setCookieHeaders == null || setCookieHeaders.isEmpty()) {
@Nullable
String cookieHeadersValue = parseCookies(info.getAllHeaders().get(HttpHeaders.SET_COOKIE));
if (!shouldKeepPost && TextUtils.isEmpty(cookieHeadersValue)) {
request.followRedirect();
return;
}
urlRequest.cancel();
DataSpec redirectUrlDataSpec;
if (dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) {
if (!shouldKeepPost && dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) {
// For POST redirects that aren't 307 or 308, the redirect is followed but request is
// transformed into a GET.
// transformed into a GET unless shouldKeepPost is true.
redirectUrlDataSpec =
dataSpec
.buildUpon()
......@@ -1114,7 +1146,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource {
exception = e;
return;
}
String cookieHeadersValue = parseCookies(setCookieHeaders);
attachCookies(requestBuilder, cookieHeadersValue);
currentUrlRequest = requestBuilder.build();
currentUrlRequest.start();
......
extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java
......@@ -72,6 +72,7 @@ import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.ArgumentMatchers;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
......@@ -1157,7 +1158,7 @@ public final class CronetDataSourceTest {
@Test
public void redirectParseAndAttachCookie_dataSourceDoesNotHandleSetCookie_followsRedirect()
throws HttpDataSourceException {
mockSingleRedirectSuccess();
mockSingleRedirectSuccess(/*responseCode=*/ 300);
mockFollowRedirectSuccess();
testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video");
......@@ -1182,7 +1183,7 @@ public final class CronetDataSourceTest {
dataSourceUnderTest.addTransferListener(mockTransferListener);
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
mockSingleRedirectSuccess();
mockSingleRedirectSuccess(/*responseCode=*/ 300);
testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video");
......@@ -1210,7 +1211,7 @@ public final class CronetDataSourceTest {
dataSourceUnderTest.addTransferListener(mockTransferListener);
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
mockSingleRedirectSuccess();
mockSingleRedirectSuccess(/*responseCode=*/ 300);
mockReadSuccess(0, 1000);
testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video");
......@@ -1225,7 +1226,7 @@ public final class CronetDataSourceTest {
@Test
public void redirectNoSetCookieFollowsRedirect() throws HttpDataSourceException {
mockSingleRedirectSuccess();
mockSingleRedirectSuccess(/*responseCode=*/ 300);
mockFollowRedirectSuccess();
dataSourceUnderTest.open(testDataSpec);
......@@ -1245,7 +1246,7 @@ public final class CronetDataSourceTest {
.setHandleSetCookieRequests(true)
.createDataSource();
dataSourceUnderTest.addTransferListener(mockTransferListener);
mockSingleRedirectSuccess();
mockSingleRedirectSuccess(/*responseCode=*/ 300);
mockFollowRedirectSuccess();
dataSourceUnderTest.open(testDataSpec);
......@@ -1254,6 +1255,66 @@ public final class CronetDataSourceTest {
}
@Test
public void redirectPostFollowRedirect() throws HttpDataSourceException {
mockSingleRedirectSuccess(/*responseCode=*/ 302);
mockFollowRedirectSuccess();
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
dataSourceUnderTest.open(testPostDataSpec);
verify(mockUrlRequest).followRedirect();
}
@Test
public void redirect302ChangesPostToGet() throws HttpDataSourceException {
dataSourceUnderTest =
(CronetDataSource)
new CronetDataSource.Factory(mockCronetEngine, executorService)
.setConnectionTimeoutMs(TEST_CONNECT_TIMEOUT_MS)
.setReadTimeoutMs(TEST_READ_TIMEOUT_MS)
.setResetTimeoutOnRedirects(true)
.setKeepPostFor302Redirects(false)
.setHandleSetCookieRequests(true)
.createDataSource();
mockSingleRedirectSuccess(/*responseCode=*/ 302);
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video");
dataSourceUnderTest.open(testPostDataSpec);
verify(mockUrlRequest, never()).followRedirect();
ArgumentCaptor<String> methodCaptor = ArgumentCaptor.forClass(String.class);
verify(mockUrlRequestBuilder, times(2)).setHttpMethod(methodCaptor.capture());
assertThat(methodCaptor.getAllValues()).containsExactly("POST", "GET").inOrder();
}
@Test
public void redirectKeeps302Post() throws HttpDataSourceException {
dataSourceUnderTest =
(CronetDataSource)
new CronetDataSource.Factory(mockCronetEngine, executorService)
.setConnectionTimeoutMs(TEST_CONNECT_TIMEOUT_MS)
.setReadTimeoutMs(TEST_READ_TIMEOUT_MS)
.setResetTimeoutOnRedirects(true)
.setKeepPostFor302Redirects(true)
.createDataSource();
mockSingleRedirectSuccess(/*responseCode=*/ 302);
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
dataSourceUnderTest.open(testPostDataSpec);
verify(mockUrlRequest, never()).followRedirect();
ArgumentCaptor<String> methodCaptor = ArgumentCaptor.forClass(String.class);
verify(mockUrlRequestBuilder, times(2)).setHttpMethod(methodCaptor.capture());
assertThat(methodCaptor.getAllValues()).containsExactly("POST", "POST").inOrder();
ArgumentCaptor<ByteArrayUploadDataProvider> postBodyCaptor =
ArgumentCaptor.forClass(ByteArrayUploadDataProvider.class);
verify(mockUrlRequestBuilder, times(2)).setUploadDataProvider(postBodyCaptor.capture(), any());
assertThat(postBodyCaptor.getAllValues().get(0).getLength()).isEqualTo(TEST_POST_BODY.length);
assertThat(postBodyCaptor.getAllValues().get(1).getLength()).isEqualTo(TEST_POST_BODY.length);
}
@Test
public void exceptionFromTransferListener() throws HttpDataSourceException {
mockResponseStartSuccess();
......@@ -1518,14 +1579,14 @@ public final class CronetDataSourceTest {
.start();
}
private void mockSingleRedirectSuccess() {
private void mockSingleRedirectSuccess(int responseCode) {
doAnswer(
invocation -> {
if (!redirectCalled) {
redirectCalled = true;
dataSourceUnderTest.urlRequestCallback.onRedirectReceived(
mockUrlRequest,
createUrlResponseInfoWithUrl("http://example.com/video", 300),
createUrlResponseInfoWithUrl("http://example.com/video", responseCode),
"http://example.com/video/redirect");
} else {
dataSourceUnderTest.urlRequestCallback.onResponseStarted(
......
library/common/src/main/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java
......@@ -69,6 +69,7 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
private int connectTimeoutMs;
private int readTimeoutMs;
private boolean allowCrossProtocolRedirects;
private boolean keepPostFor302Redirects;
/** Creates an instance. */
public Factory() {
......@@ -175,6 +176,15 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
return this;
}
/**
* Sets whether we should keep the POST method and body when we have HTTP 302 redirects for a
* POST request.
*/
public Factory setKeepPostFor302Redirects(boolean keepPostFor302Redirects) {
this.keepPostFor302Redirects = keepPostFor302Redirects;
return this;
}
@Override
public DefaultHttpDataSource createDataSource() {
DefaultHttpDataSource dataSource =
......@@ -184,7 +194,8 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
readTimeoutMs,
allowCrossProtocolRedirects,
defaultRequestProperties,
contentTypePredicate);
contentTypePredicate,
keepPostFor302Redirects);
if (transferListener != null) {
dataSource.addTransferListener(transferListener);
}
......@@ -209,6 +220,7 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
@Nullable private final String userAgent;
@Nullable private final RequestProperties defaultRequestProperties;
private final RequestProperties requestProperties;
private final boolean keepPostFor302Redirects;
@Nullable private Predicate<String> contentTypePredicate;
@Nullable private DataSpec dataSpec;
......@@ -260,7 +272,8 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
readTimeoutMillis,
allowCrossProtocolRedirects,
defaultRequestProperties,
/* contentTypePredicate= */ null);
/* contentTypePredicate= */ null,
/* keepPostFor302Redirects= */ false);
}
private DefaultHttpDataSource(
......@@ -269,7 +282,8 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
int readTimeoutMillis,
boolean allowCrossProtocolRedirects,
@Nullable RequestProperties defaultRequestProperties,
@Nullable Predicate<String> contentTypePredicate) {
@Nullable Predicate<String> contentTypePredicate,
boolean keepPostFor302Redirects) {
super(/* isNetwork= */ true);
this.userAgent = userAgent;
this.connectTimeoutMillis = connectTimeoutMillis;
......@@ -278,6 +292,7 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
this.defaultRequestProperties = defaultRequestProperties;
this.contentTypePredicate = contentTypePredicate;
this.requestProperties = new RequestProperties();
this.keepPostFor302Redirects = keepPostFor302Redirects;
}
/**
......@@ -486,7 +501,7 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
long length = dataSpec.length;
boolean allowGzip = dataSpec.isFlagSet(DataSpec.FLAG_ALLOW_GZIP);
if (!allowCrossProtocolRedirects) {
if (!allowCrossProtocolRedirects && !keepPostFor302Redirects) {
// HttpURLConnection disallows cross-protocol redirects, but otherwise performs redirection
// automatically. This is the behavior we want, so use it.
return makeConnection(
......@@ -500,7 +515,8 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
dataSpec.httpRequestHeaders);
}
// We need to handle redirects ourselves to allow cross-protocol redirects.
// We need to handle redirects ourselves to allow cross-protocol redirects or to keep the POST
// request method for 302.
int redirectCount = 0;
while (redirectCount++ <= MAX_REDIRECTS) {
HttpURLConnection connection =
......@@ -529,10 +545,14 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
|| responseCode == HttpURLConnection.HTTP_MOVED_PERM
|| responseCode == HttpURLConnection.HTTP_MOVED_TEMP
|| responseCode == HttpURLConnection.HTTP_SEE_OTHER)) {
// POST request follows the redirect and is transformed into a GET request.
connection.disconnect();
boolean shouldKeepPost =
keepPostFor302Redirects && responseCode == HttpURLConnection.HTTP_MOVED_TEMP;
if (!shouldKeepPost) {
// POST request follows the redirect and is transformed into a GET request.
httpMethod = DataSpec.HTTP_METHOD_GET;
httpBody = null;
}
url = handleRedirect(url, location);
} else {
return connection;
......@@ -618,7 +638,7 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
* @return The next URL.
* @throws IOException If redirection isn't possible.
*/
private static URL handleRedirect(URL originalUrl, @Nullable String location) throws IOException {
private URL handleRedirect(URL originalUrl, @Nullable String location) throws IOException {
if (location == null) {
throw new ProtocolException("Null location redirect");
}
......@@ -629,13 +649,14 @@ public class DefaultHttpDataSource extends BaseDataSource implements HttpDataSou
if (!"https".equals(protocol) && !"http".equals(protocol)) {
throw new ProtocolException("Unsupported protocol redirect: " + protocol);
}
// Currently this method is only called if allowCrossProtocolRedirects is true, and so the code
// below isn't required. If we ever decide to handle redirects ourselves when cross-protocol
// redirects are disabled, we'll need to uncomment this block of code.
// if (!allowCrossProtocolRedirects && !protocol.equals(originalUrl.getProtocol())) {
// throw new ProtocolException("Disallowed cross-protocol redirect ("
// + originalUrl.getProtocol() + " to " + protocol + ")");
// }
if (!allowCrossProtocolRedirects && !protocol.equals(originalUrl.getProtocol())) {
throw new ProtocolException(
"Disallowed cross-protocol redirect ("
+ originalUrl.getProtocol()
+ " to "
+ protocol
+ ")");
}
return url;
}
......
library/common/src/test/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSourceTest.java
......@@ -22,11 +22,14 @@ import static org.junit.Assert.assertThrows;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import com.google.android.exoplayer2.testutil.TestUtil;
import com.google.android.exoplayer2.upstream.HttpDataSource.HttpDataSourceException;
import java.net.HttpURLConnection;
import java.util.HashMap;
import java.util.Map;
import okhttp3.Headers;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;
import okhttp3.mockwebserver.RecordedRequest;
import okio.Buffer;
import org.junit.Test;
import org.junit.runner.RunWith;
......@@ -127,6 +130,86 @@ public class DefaultHttpDataSourceTest {
}
@Test
public void open_redirectChanges302PostToGet()
throws HttpDataSourceException, InterruptedException {
byte[] postBody = new byte[] {1, 2, 3};
DefaultHttpDataSource defaultHttpDataSource =
new DefaultHttpDataSource.Factory()
.setConnectTimeoutMs(1000)
.setReadTimeoutMs(1000)
.setKeepPostFor302Redirects(false)
.setAllowCrossProtocolRedirects(true)
.createDataSource();
MockWebServer mockWebServer = new MockWebServer();
String newLocationUrl = mockWebServer.url("/redirect-path").toString();
mockWebServer.enqueue(
new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.addHeader("Location", newLocationUrl));
mockWebServer.enqueue(new MockResponse().setResponseCode(HttpURLConnection.HTTP_OK));
DataSpec dataSpec =
new DataSpec.Builder()
.setUri(mockWebServer.url("/test-path").toString())
.setHttpMethod(DataSpec.HTTP_METHOD_POST)
.setHttpBody(postBody)
.build();
defaultHttpDataSource.open(dataSpec);
RecordedRequest request1 = mockWebServer.takeRequest(10, SECONDS);
assertThat(request1).isNotNull();
assertThat(request1.getPath()).isEqualTo("/test-path");
assertThat(request1.getMethod()).isEqualTo("POST");
assertThat(request1.getBodySize()).isEqualTo(postBody.length);
RecordedRequest request2 = mockWebServer.takeRequest(10, SECONDS);
assertThat(request2).isNotNull();
assertThat(request2.getPath()).isEqualTo("/redirect-path");
assertThat(request2.getMethod()).isEqualTo("GET");
assertThat(request2.getBodySize()).isEqualTo(0);
}
@Test
public void open_redirectKeeps302Post() throws HttpDataSourceException, InterruptedException {
byte[] postBody = new byte[] {1, 2, 3};
DefaultHttpDataSource defaultHttpDataSource =
new DefaultHttpDataSource.Factory()
.setConnectTimeoutMs(1000)
.setReadTimeoutMs(1000)
.setKeepPostFor302Redirects(true)
.createDataSource();
MockWebServer mockWebServer = new MockWebServer();
String newLocationUrl = mockWebServer.url("/redirect-path").toString();
mockWebServer.enqueue(
new MockResponse()
.setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP)
.addHeader("Location", newLocationUrl));
mockWebServer.enqueue(new MockResponse().setResponseCode(HttpURLConnection.HTTP_OK));
DataSpec dataSpec =
new DataSpec.Builder()
.setUri(mockWebServer.url("/test-path").toString())
.setHttpMethod(DataSpec.HTTP_METHOD_POST)
.setHttpBody(postBody)
.build();
defaultHttpDataSource.open(dataSpec);
RecordedRequest request1 = mockWebServer.takeRequest(10, SECONDS);
assertThat(request1).isNotNull();
assertThat(request1.getPath()).isEqualTo("/test-path");
assertThat(request1.getMethod()).isEqualTo("POST");
assertThat(request1.getBodySize()).isEqualTo(postBody.length);
RecordedRequest request2 = mockWebServer.takeRequest(10, SECONDS);
assertThat(request2).isNotNull();
assertThat(request2.getPath()).isEqualTo("/redirect-path");
assertThat(request2.getMethod()).isEqualTo("POST");
assertThat(request2.getBodySize()).isEqualTo(postBody.length);
}
@Test
public void factory_setRequestPropertyAfterCreation_setsCorrectHeaders() throws Exception {
MockWebServer mockWebServer = new MockWebServer();
mockWebServer.enqueue(new MockResponse());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment