From b9e6165433dd20948e19d949f5b8d896f7daf061 Mon Sep 17 00:00:00 2001 From: Erik Poort Date: Mon, 5 Mar 2018 15:47:59 -0300 Subject: [PATCH 1/4] This ensures no illegal cookies are send to okhttp 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. --- .../modules/network/ReactCookieJarContainer.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) 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(); } From f523fc414464e6166b4de4d4d2247860f1ab37d5 Mon Sep 17 00:00:00 2001 From: Erik Poort Date: Fri, 16 Mar 2018 14:49:14 -0300 Subject: [PATCH 2/4] Add tests for ReactCookieJarContainer This will test the filtering of invalid cookies that might be set through the Android WebView. --- .../network/ReactCookieJarContainerTest.java | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java 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..cd81e60a80e9d6 --- /dev/null +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java @@ -0,0 +1,90 @@ +/** + * 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.Rule; +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.powermock.modules.junit4.rule.PowerMockRule; +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 { + + @Rule + public PowerMockRule rule = new PowerMockRule(); + + @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("invalid") + .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") + .domain("domain") + .build() + ); + when(cookieJar.loadForRequest(any(HttpUrl.class))).thenReturn(cookies); + assertThat(jarContainer.loadForRequest(any(HttpUrl.class)).size()).isEqualTo(0); + } +} From ab9f91c26cea7c3507396467a6dab24df45b22cc Mon Sep 17 00:00:00 2001 From: Erik Poort Date: Fri, 16 Mar 2018 16:51:32 -0300 Subject: [PATCH 3/4] Fix confusing typo --- .../react/modules/network/ReactCookieJarContainerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index cd81e60a80e9d6..6b608f3068553e 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java @@ -64,7 +64,7 @@ public void testValidCookies() throws Exception { List cookies = new ArrayList<>(); cookies.add(new Cookie.Builder() .name("valid") - .value("invalid") + .value("valid value") .domain("domain") .build() ); @@ -80,7 +80,7 @@ public void testInvalidCookies() throws Exception { List cookies = new ArrayList<>(); cookies.add(new Cookie.Builder() .name("valid") - .value("înválíd") + .value("înválíd välūė") .domain("domain") .build() ); From 182fdc49979ef56bb23e860385063eedebe5c353 Mon Sep 17 00:00:00 2001 From: Erik Poort Date: Fri, 16 Mar 2018 16:52:57 -0300 Subject: [PATCH 4/4] Remove unused classes and annotations --- .../react/modules/network/ReactCookieJarContainerTest.java | 5 ----- 1 file changed, 5 deletions(-) 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 index 6b608f3068553e..b08e514e853d37 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/network/ReactCookieJarContainerTest.java @@ -14,12 +14,10 @@ import java.util.List; import java.util.ArrayList; -import org.junit.Rule; 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.powermock.modules.junit4.rule.PowerMockRule; import org.robolectric.RobolectricTestRunner; import static org.fest.assertions.api.Assertions.assertThat; @@ -39,9 +37,6 @@ public class ReactCookieJarContainerTest { - @Rule - public PowerMockRule rule = new PowerMockRule(); - @Test public void testMissingJar() throws Exception { ReactCookieJarContainer jarContainer = mock(ReactCookieJarContainer.class);