Skip to content

Commit

Permalink
OkHttp is more strict than other http libraries. (#21231)
Browse files Browse the repository at this point in the history
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: #21231

Reviewed By: axe-fb

Differential Revision: D10222129

Pulled By: hramos

fbshipit-source-id: b23340692d0fb059a90e338fa85ad4d9612275f2
  • Loading branch information
dryganets authored and facebook-github-bot committed Mar 21, 2019
1 parent fe3aebf commit aad4dbb
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 4 deletions.
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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));
}

}

0 comments on commit aad4dbb

Please sign in to comment.