From 04028bf2169b01f79bd86ecd6b0d8aa5f99599f1 Mon Sep 17 00:00:00 2001 From: Erik Poort Date: Fri, 25 May 2018 11:04:46 -0700 Subject: [PATCH] This ensures no illegal cookies are send to okhttp Summary: When a website in a ReactNative WebView sets a cookie with an illegal character, this cookie will automatically be added to any request to the same domain. This happens through: BridgeInterceptor.java (l.84) ReactCookieJarContainer.java (l.44) JavaNetCookieJar.java (l.59) ForwardingCookieHandler.java (l.57) ForwardingCookieHandler.java (l.168) CookieManager.java (l.39) The BridgeInterceptor.java then tries to set a Cookie header, which validates both keys and values, and then crashes. okhttp3.6.0 Headers.java (l.320) This fix will strip illegal characters from any cookie that is being passed to the okhttp request. To demonstrate how to crash the app, you can find an example app here: https://github.com/erikpoort/react-native-test-illegal-cookie Or you can load the following url into a webview: https://invalidcookietest.us.dev.monkapps.com/ Press the 'Set cookie' button. Then try to fetch the same url. [ANDROID] [BREAKING] [ReactCookieJarContainer.java] - I'm filtering cookies containing illegal characters from any request. Closes https://github.com/facebook/react-native/pull/18203 Differential Revision: D8164302 Pulled By: hramos fbshipit-source-id: 6e58461df594eb2c7aad4c7ad70b76d12ac09b84 --- .../network/ReactCookieJarContainer.java | 14 ++- .../network/ReactCookieJarContainerTest.java | 85 +++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/ReactCookieJarContainer.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/ReactCookieJarContainer.java index a8436c21875ebc..370586fa50ca9c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/ReactCookieJarContainer.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/ReactCookieJarContainer.java @@ -1,5 +1,6 @@ package com.facebook.react.modules.network; +import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -7,6 +8,7 @@ import okhttp3.Cookie; import okhttp3.CookieJar; +import okhttp3.Headers; import okhttp3.HttpUrl; /** @@ -37,7 +39,17 @@ public void saveFromResponse(HttpUrl url, List cookies) { @Override public List loadForRequest(HttpUrl url) { if (cookieJar != null) { - return cookieJar.loadForRequest(url); + List cookies = cookieJar.loadForRequest(url); + ArrayList validatedCookies = new ArrayList<>(); + for (Cookie cookie : cookies) { + try { + Headers.Builder cookieChecker = new Headers.Builder(); + cookieChecker.add(cookie.name(), cookie.value()); + validatedCookies.add(cookie); + } catch (IllegalArgumentException ignored) { + } + } + return validatedCookies; } return Collections.emptyList(); } diff --git a/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java b/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java new file mode 100644 index 00000000000000..b08e514e853d37 --- /dev/null +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java @@ -0,0 +1,85 @@ +/** + * Copyright (c) 2015-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.modules.network; + +import com.facebook.react.modules.network.ReactCookieJarContainer; +import okhttp3.Cookie; +import okhttp3.CookieJar; +import okhttp3.HttpUrl; +import java.util.List; +import java.util.ArrayList; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.powermock.core.classloader.annotations.PowerMockIgnore; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.robolectric.RobolectricTestRunner; + +import static org.fest.assertions.api.Assertions.assertThat; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link NetworkingModule}. + */ +@PrepareForTest({ + ReactCookieJarContainer.class +}) +@RunWith(RobolectricTestRunner.class) +@PowerMockIgnore({"org.mockito.*", "org.robolectric.*", "android.*"}) + +public class ReactCookieJarContainerTest { + + @Test + public void testMissingJar() throws Exception { + ReactCookieJarContainer jarContainer = mock(ReactCookieJarContainer.class); + assertThat(jarContainer.loadForRequest(any(HttpUrl.class)).size()).isEqualTo(0); + } + + @Test + public void testEmptyCookies() throws Exception { + ReactCookieJarContainer jarContainer = mock(ReactCookieJarContainer.class); + List cookies = new ArrayList<>(); + when(jarContainer.loadForRequest(any(HttpUrl.class))).thenReturn(cookies); + assertThat(jarContainer.loadForRequest(any(HttpUrl.class)).size()).isEqualTo(0); + } + + @Test + public void testValidCookies() throws Exception { + ReactCookieJarContainer jarContainer = new ReactCookieJarContainer(); + CookieJar cookieJar = mock(CookieJar.class); + jarContainer.setCookieJar(cookieJar); + List cookies = new ArrayList<>(); + cookies.add(new Cookie.Builder() + .name("valid") + .value("valid value") + .domain("domain") + .build() + ); + when(cookieJar.loadForRequest(any(HttpUrl.class))).thenReturn(cookies); + assertThat(jarContainer.loadForRequest(any(HttpUrl.class)).size()).isEqualTo(1); + } + + @Test + public void testInvalidCookies() throws Exception { + ReactCookieJarContainer jarContainer = new ReactCookieJarContainer(); + CookieJar cookieJar = mock(CookieJar.class); + jarContainer.setCookieJar(cookieJar); + List cookies = new ArrayList<>(); + cookies.add(new Cookie.Builder() + .name("valid") + .value("înválíd välūė") + .domain("domain") + .build() + ); + when(cookieJar.loadForRequest(any(HttpUrl.class))).thenReturn(cookies); + assertThat(jarContainer.loadForRequest(any(HttpUrl.class)).size()).isEqualTo(0); + } +}