From aad4dbbbfe937d1924e5359556979ab067198a58 Mon Sep 17 00:00:00 2001 From: Sergei Dryganets Date: Thu, 21 Mar 2019 11:09:19 -0700 Subject: [PATCH] OkHttp is more strict than other http libraries. (#21231) Summary: It crashes with IllegalStateException in case you pass a wrong URL. It crashes if it meets unexpected symbols in the header name and value, while standard says it is not recommended to use those symbols not that they are prohibited. The headers handing is a special use case as a client might have an auth token in the header. In this case, we want to get 401 error response from the server to find out that token is wrong. In case of the onerror client will continue to retry with an existing token. [ANDROID][Fixed] - Networking: Passing invalid URL not crashes the app instead onerror callback of HttpClient is called. Invalid symbols are stripped from the headers to allow HTTP query to fail with 401 error code in case of the broken token. Pull Request resolved: https://github.com/facebook/react-native/pull/21231 Reviewed By: axe-fb Differential Revision: D10222129 Pulled By: hramos fbshipit-source-id: b23340692d0fb059a90e338fa85ad4d9612275f2 --- .../react/modules/network/HeaderUtil.java | 47 ++++++++++++ .../modules/network/NetworkingModule.java | 27 ++++++- .../react/modules/network/ResponseUtil.java | 2 +- .../react/modules/network/HeaderUtilTest.java | 76 +++++++++++++++++++ 4 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/modules/network/HeaderUtil.java create mode 100644 ReactAndroid/src/test/java/com/facebook/react/modules/network/HeaderUtilTest.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/HeaderUtil.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/HeaderUtil.java new file mode 100644 index 00000000000000..2f2d72bb90c3e3 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/HeaderUtil.java @@ -0,0 +1,47 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * 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; + +/** + * + * The class purpose is to weaken too strict OkHttp restriction on http headers. + * See: https://github.com/square/okhttp/issues/2016 + * Auth headers might have an Authentication information. It is better to get 401 from the + * server in this case, rather than non descriptive error as 401 could be handled to invalidate + * the wrong token in the client code. + */ +public class HeaderUtil { + + public static String stripHeaderName(String name) { + StringBuilder builder = new StringBuilder(name.length()); + boolean modified = false; + for (int i = 0, length = name.length(); i < length; i++) { + char c = name.charAt(i); + if (c > '\u0020' && c < '\u007f') { + builder.append(c); + } else { + modified = true; + } + } + return modified ? builder.toString() : name; + } + + public static String stripHeaderValue(String value) { + StringBuilder builder = new StringBuilder(value.length()); + boolean modified = false; + for (int i = 0, length = value.length(); i < length; i++) { + char c = value.charAt(i); + if ((c > '\u001f' && c < '\u007f') || c == '\t' ) { + builder.append(c); + } else { + modified = true; + } + } + return modified ? builder.toString() : value; + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java index 209ead5a396872..cbae1f0fbb0062 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java @@ -9,6 +9,7 @@ import android.net.Uri; import android.util.Base64; +import com.facebook.common.logging.FLog; import com.facebook.react.bridge.Arguments; import com.facebook.react.bridge.GuardedAsyncTask; import com.facebook.react.bridge.ReactApplicationContext; @@ -104,6 +105,7 @@ public interface ResponseHandler { protected static final String NAME = "Networking"; + private static final String TAG = "NetworkingModule"; private static final String CONTENT_ENCODING_HEADER_NAME = "content-encoding"; private static final String CONTENT_TYPE_HEADER_NAME = "content-type"; private static final String REQUEST_BODY_KEY_STRING = "string"; @@ -234,10 +236,29 @@ public void removeResponseHandler(ResponseHandler handler) { } @ReactMethod + public void sendRequest( + String method, + String url, + final int requestId, + ReadableArray headers, + ReadableMap data, + final String responseType, + final boolean useIncrementalUpdates, + int timeout, + boolean withCredentials) { + try { + sendRequestInternal(method, url, requestId, headers, data, responseType, + useIncrementalUpdates, timeout, withCredentials); + } catch (Throwable th) { + FLog.e(TAG, "Failed to send url request: " + url, th); + ResponseUtil.onRequestError(getEventEmitter(), requestId, th.getMessage(), th); + } + } + /** * @param timeout value of 0 results in no timeout */ - public void sendRequest( + public void sendRequestInternal( String method, String url, final int requestId, @@ -720,8 +741,8 @@ public void clearCookies(com.facebook.react.bridge.Callback callback) { if (header == null || header.size() != 2) { return null; } - String headerName = header.getString(0); - String headerValue = header.getString(1); + String headerName = HeaderUtil.stripHeaderName(header.getString(0)); + String headerValue = HeaderUtil.stripHeaderValue(header.getString(1)); if (headerName == null || headerValue == null) { return null; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/ResponseUtil.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/ResponseUtil.java index f163ae7746792c..9ad7bffdac0303 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/ResponseUtil.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/ResponseUtil.java @@ -85,7 +85,7 @@ public static void onRequestError( RCTDeviceEventEmitter eventEmitter, int requestId, String error, - IOException e) { + Throwable e) { WritableArray args = Arguments.createArray(); args.pushInt(requestId); args.pushString(error); diff --git a/ReactAndroid/src/test/java/com/facebook/react/modules/network/HeaderUtilTest.java b/ReactAndroid/src/test/java/com/facebook/react/modules/network/HeaderUtilTest.java new file mode 100644 index 00000000000000..10e52ddc5474e3 --- /dev/null +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/network/HeaderUtilTest.java @@ -0,0 +1,76 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * 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 org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class HeaderUtilTest { + public static final String TABULATION_TEST = "\teyJhbGciOiJS\t"; + public static final String TABULATION_STRIP_EXPECTED = "eyJhbGciOiJS"; + public static final String NUMBERS_TEST = "0123456789"; + public static final String SPECIALS_TEST = "!@#$%^&*()-=_+{}[]\\|;:'\",.<>/?"; + public static final String ALPHABET_TEST = "abcdefghijklmnopqrstuvwxyzABCDEFGHIGKLMNOPQRSTUVWHYZ"; + public static final String VALUE_BANNED_SYMBOLS_TEST = "���name�����������\u007f\u001f"; + public static final String NAME_BANNED_SYMBOLS_TEST = "���name�����������\u007f\u0020\u001f"; + public static final String BANNED_TEST_EXPECTED = "name"; + + @Test + public void nameStripKeepsLetters() { + assertEquals(ALPHABET_TEST, HeaderUtil.stripHeaderName(ALPHABET_TEST)); + + } + + @Test + public void valueStripKeepsLetters() { + assertEquals(ALPHABET_TEST, HeaderUtil.stripHeaderValue(ALPHABET_TEST)); + } + + @Test + public void nameStripKeepsNumbers() { + assertEquals(NUMBERS_TEST, HeaderUtil.stripHeaderName(NUMBERS_TEST)); + + } + + @Test + public void valueStripKeepsNumbers() { + assertEquals(NUMBERS_TEST, HeaderUtil.stripHeaderValue(NUMBERS_TEST)); + } + + @Test + public void valueStripKeepsSpecials() { + assertEquals(SPECIALS_TEST, HeaderUtil.stripHeaderValue(SPECIALS_TEST)); + } + + @Test + public void nameStripKeepsSpecials() { + assertEquals(SPECIALS_TEST, HeaderUtil.stripHeaderName(SPECIALS_TEST)); + } + + @Test + public void valueStripKeepsTabs() { + assertEquals(TABULATION_TEST, HeaderUtil.stripHeaderValue(TABULATION_TEST)); + } + + @Test + public void nameStripDeletesTabs() { + assertEquals(TABULATION_STRIP_EXPECTED, HeaderUtil.stripHeaderName(TABULATION_TEST)); + } + + @Test + public void valueStripRemovesExtraSymbols() { + assertEquals(BANNED_TEST_EXPECTED, HeaderUtil.stripHeaderValue(VALUE_BANNED_SYMBOLS_TEST)); + } + + @Test + public void nameStripRemovesExtraSymbols() { + assertEquals(BANNED_TEST_EXPECTED, HeaderUtil.stripHeaderName(NAME_BANNED_SYMBOLS_TEST)); + } + +}