React-native: HTTP response headers with the same key are not processed properly. [The expected behaviour is that the values of the identical keys are combined to a string separated by a comma]

[mdloselo]

Environment

OS: macOS High Sierra 10.13.6
Node: 8.12.0
Yarn: Not Found
npm: 6.4.1
Watchman: 4.9.0
Xcode: Xcode 10.0 Build version 10A255
Android Studio: 3.2 AI-181.5540.7.32.5014246

Packages: (wanted => installed)
react: 16.3.1 => 16.3.1
react-native: https://github.com/expo/react-native/archive/sdk-30.0.0.tar.gz => 0.55.4

Description

I have an issue supporting multiple cookies passed on the response header on Android. I tracked the issue to line 607 in translateHeaders function in NetworkingModule.java file from the link below.

https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

I debugged and realised that the code that checks if the headerMap of type WritableMap has the header key already in it is always failing and defaulting to the "Else" part which always replaces the previous key and value every time instead of combining previous key's value string separated by a comma as it is supposed to. Due to this, only one cookie is always passed to React side.

Furthermore, the code I mentioned above use ReadableNativeMap class which holds the actual key-value map as an instance variable. getLocalMap function in ReadableNativeMap class is used to retrieve the "mLocalMap" variable but always return an object that doesn't have values hence the code inside NetworkingModule is not doing what it is supposed to.

Lastly, I'm aware that my environment info state that I'm using react-native from Expo https://github.com/expo/react-native, however the code that I found an issue on, Expo also use it as is from https://github.com/facebook/react-native hence I created an issue here.

Reproducible Demo

Pass multiple headers with the same key from a network request.

In my case I'm getting response headers with multiple cookies with key "Set-Cookie" from a network request, but after it has been processed by the code I mentioned above, there is always one.

Sample response header with multiple cookies similar to what get from the server.

Server: Example Date: Tue, 16 Oct 2018 10:35:21 GMT Content-Type: application/json;charset=UTF-8 Connection: keep-alive Expires: 0 X-B3-TraceId: d6745404c1d2a7fb Set-Cookie: MyDBTokenForApps=6c785634-d62d-32a2-a4f7-8c1e0f42a93f; path=/; secure; Max-Age=43198; Expires=Tue, 16-Oct-2018 22:35:19 GMT Set-Cookie: anotherCookie=1$E4AAA5C9C37865158B02402AFFDB7856; path=/; domain=.example.co.za; secure X-XSS-Protection: 1; mode=block X-Frame-Options: DENY X-Content-Type-Options: nosniff Strict-Transport-Security: max-age=31536000 ; includeSubDomains Instance: /CSG/pool_vcoza_summer_80 10.112.205.215 80 Vary: Accept-Encoding Transfer-Encoding: chunked Cache-Control: public, max-age=0

After this has been processed by translateHeaders function, only one cookie header that was processed last remains.

[mdloselo]

I realised that if I introduce another variable to hold the headers key-value map in translateHeaders function line 602 in the NetworkingModule.java, it works. Something like this:

private static WritableMap translateHeaders(Headers headers) {
    WritableMap responseHeaders = Arguments.createMap();
    HashMap<String, String> responseHeadersMap = new HashMap<>();

   for (int i = 0; i < headers.size(); i++) {
      String headerName = headers.name(i);
      String headerValue = headers.value(i);

     if(responseHeadersMap.containsKey(headerName)) {
        String existingValueForHeaderName = responseHeadersMap.get(headerName);
        responseHeadersMap.put(headerName, existingValueForHeaderName + ", " + headerValue);
        responseHeaders.putString(headerName, existingValueForHeaderName + ", " + headerValue);
      } else {
       responseHeadersMap.put(headerName, headerValue);
       responseHeaders.putString(headerName, headerValue);
     }
   }

return responseHeaders;

}

[mdloselo]

There seem to be something wonky about using WritableMap because every time it is queried to check if there is an existing key "if (responseHeaders.hasKey(headerName)) ", the result is always false even though the key was added previously which causes the else part to be executed and the previous value of the same key is overwritten with the new value.

[Cyberclown]

+1, very annoying issue

[russell-matt]

+1, this gave me a lot of problems in my app. Hope this gets it fixed.

[brazerZa]

+1
Please fix asap.

[timdelange]

I have the same problem, when multiple set cookie headers are sent by the back end, we don't get to see them in the app- the system only shows the last one on Android. On ios we get them combined into one.

[WianNell]

+1
Hope this issue gets fixed soon.

[StatikVerse]

+1 Please fix this? :)

[tshepoR]

Please fix this!

[zinzan-vdm]

I have the same problem, when multiple set cookie headers are sent by the back end, we don't get to see them in the app- the system only shows the last one on Android. On ios we get them combined into one.

I've also experienced this on Android.

Multiple Set-Cookie headers seems to be the standard according to RFC-2109 section '_4.2.1 Origin Server Role - General_' as well as RFC-6265 section '_3 Overview_'.

RFC-6265 explicitely states that:

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into
a single header field. The usual mechanism for folding HTTP headers
fields (i.e., as defined in [RFC2616]) might change the semantics of
the Set-Cookie header field because the %x2C (",") character is used
by Set-Cookie in a way that conflicts with such folding.

I think we should conform to this standard as many frameworks already seem to.

[theBusinessAnalyst]

Please fix this.

[hey99xx]

@mdloselo I think the bug as you said is in WritableNativeMap class. Looking at the code it keeps maps both in the C++ side and Java side, and they're not synced properly for every operation. Specifically putString is implemented in C++ and hasKey in Java. I think Facebook is trying to make optimizations on native access but accidentally introduced this bug.

If you already have the project checked out, can you call ReadableNativeMap.setUseNativeAccessor(true) and ReadableNativeArray.setUseNativeAccessor(true)? This should make both array and map methods use the previous C++ implementation, and should prove the root cause is the bug I described above.

[hey99xx]

I think this issue is identical btw https://github.com/facebook/react-native/issues/18837 and the comments there also suggests the out of sync Java and C++ maps is the root cause.

[mdloselo]

@hey99xx thanks, it works when I set ReadableNativeMap.setUseNativeAccessor(true) and ReadableNativeArray.setUseNativeAccessor(true). I have checked if this can potentially have any unintended consequences but I couldn't find anything. I wonder if Facebook forgot to update the docs to let us know we have to set these flags when they introduced this here https://github.com/facebook/react-native/commit/7891805d22e3fdc821961ff0ccc5c450c3d625c8 because before that mUseNativeAccessor was never there.

[hey99xx]

@mdloselo I don't think Facebook lists all their changes in the public documentation website, they put minimal javadoc. You usually need to follow comments of core contributors to get an insight of what's coming next.

In the master branch they moved mUseNativeAccessor to a slightly more visible ReactFeatureFlags class https://github.com/facebook/react-native/commit/4ac500a337572996c4a822c3433da3e4297cf138. I'd guess they're still unsure whether this change results in any perf imrovement and no regresssions (obviously not) so they still have both code paths available hidden behind a flag, that they can switch from their apps. I don't really know their plan since they seem to be running this experiment since 0.54.

I think having bugs in their core map / array classes is a more serious issue and this HTTP headers bug is a very little symptom, this bug likely extends beyond networking. If anyone knows how to get traction from Facebook maintainers please do so.

@ayc1 and @mdvacca have their names on the commit above so maybe they can help, also @hramos

[dakiesse]

Is it possible to implement a simple HTTP client on Java to bypass fetch/xmlHttpRequest?

[astapinski]

I've tried upgrading our app from 0.53.3 to 0.55.4 and also hit this bug with multiple WWW-Authenticate headers which is allowed per RFC. iOS does not have the issue of only one header being sent on to the JS where as Android does. This is a serious issue.

[Artorp]

This is still an issue on [email protected] (Android).

Server code:

<?php
header("X-Duplicate-Header: First entry");
header("X-Duplicate-Header: Second entry", false);

RN client code:

fetch(url)
  .then(res => {
    for (const entry of res.headers) {
      console.log(`${entry[0]}: ${entry[1]}`);
    }
  });

Output:

[18:51:57] cache-control: public, max-age=0
[18:51:57] x-duplicate-header: Second entry
[18:51:57] connection: keep-alive
[18:51:57] transfer-encoding: chunked
[18:51:57] content-type: text/html; charset=UTF-8
[18:51:57] date: Fri, 11 Jan 2019 17:51:56 GMT
[18:51:57] server: nginx/1.14.0 (Ubuntu)

Expected both "X-Duplicate-Header" header values as a comma delimited string, but only the last entry is available.

As others, I encountered this issue when trying to authenticate to a web server that has multiple WWW-Authenticate headers.

[mxwiz]

+1, very annoying issue

[jbyrneie]

Anyone know if there are earlier versions of react-native that doesnt have this issue (multiple Cookies in response) for Android?

[astapinski]

Anyone know if there are earlier versions of react-native that doesnt have this issue (multiple Cookies in response) for Android?

0.53.3 is OK, I think this issue was introduced in 0.54.

[bucketclan]

Works still 0.55.4.

[mxwiz]

Not working 0.57.8 (bug exists in this version)

[rheng001]

Same as @mxwiz, not working 0.57.8 with Android

[rheng001]

@mxwiz

Where you able to resolve this bug or find a work around that works?

[mxwiz]

@rheng001 no, my comment marked as spam 😂😂 but suggested workarount does not work for me on product device 🤷‍♂️ Other solution does not exist yet, and spam marks does not help...

[hramos]

@mxwiz I believe your comment was marked as outdated due to it referencing an older release at the time it was posted.

[mxwiz]

@hramos maybe, but it is not fixed in newest versions.

[jerolimov]

Hello, I had the same problem here and analysed the internal issue:

The problem is a combination of NetworkingModule.java and the usage of WritableNativeMap. The function translateHeaders maps the OKHTTP header objekt to a React Native header map. Headers with the same key was overridden because the hasKey method always return false.

https://github.com/facebook/react-native/blob/9895d011374e655bcaeb390167abafb9c01fef18/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java#L647-L661

That hasKey always return false is an implementation error in ReadableNativeMap and WritableNativeMap (which extends the readable map). This native maps supports two different modes, which can be enabled and disabled for all maps with a static boolean useNativeAccessor.

This flag defines if the ReadableNativeMap reads data from a native map or a internal (java.util.) HashMap.

But the problem was that WritableNativeMap always writes into the native map and never uses the (Readable) internal HashMap.

That was the reason why hasKey always return null if useNativeAccessor was false and only one header was returned by translateHeaders.

Like said by @hey99xx in https://github.com/facebook/react-native/issues/21795#issuecomment-430384534 and by @Return-1 https://github.com/facebook/react-native/issues/23005#issuecomment-457107736 its possible to activate the this native accessor, if you add this code to your own/project MainActivity.java:

Add this imports:

import com.facebook.react.bridge.ReadableNativeArray;
import com.facebook.react.bridge.ReadableNativeMap;

And this add the beginning of the createReactActivityDelegate method, before your application was loaded.

        ReadableNativeArray.setUseNativeAccessor(true);
        ReadableNativeMap.setUseNativeAccessor(true);

For me this works fine. But notice that this global flag may cause some other troubles.

I also started to fix this behaviour in the latest React Native 0.59.3 release, but notice than that the master version of ReadableNativeMap/WritableNativeMap was already changed.

The commits https://github.com/facebook/react-native/commit/b257e06bc6c29236a1a19f645fb46b85b2ffc4d2 and https://github.com/facebook/react-native/commit/a062b34493f07b28378de2772914d838cb28e3d8 by FB eng @sahrens changed/removed the useNativeAccessor behaviour, so I hope this issue was also fixed with the next/upcoming minor release 0.60.0. 🙌

(I couldn't test the new version yet, because running from the npm package (also with sourcecode changes) works fine for me, but the sourcecode version of RN let fail some of my 3rd party libraries. I'm looking forward for a RC of 0.60.0 and maybe will update this text here then.)

[hey99xx]

The commits b257e06 and a062b34 by FB eng @sahrens changed/removed the useNativeAccessor behaviour, so I hope this issue was also fixed with the next/upcoming minor release 0.60.0. 🙌

The way I read linked commits, useNativeAccessor is being removed so the workaround is no longer possible, but the issue does not appear fixed either...

[sahrens]

That hasKey always return false is an implementation error in ReadableNativeMap and WritableNativeMap (which extends the readable map)

I believe the issue is that WritableNativeMap::putString is a pure JNI function that doesn't write to mLocalMap, which is what hasKey checks. Note that the docs say WritableNativeMap is intended to be write ONLY:

https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/bridge/WritableNativeMap.java#L20-L22

Should be fixable though - if we only write to mLocalMap until the map is consumed by native and we convert the whole thing at once at that point, there might also be a perf improvement to be had....

[sahrens]

A temporary workaround might be to maintain your own parallel java map and check hasKey there.

[sahrens]

Could be implemented as a wrapper type that holds a WritableNativeMap inside it.

[pinstripe-potatohead]

Any update on this? The changes to useNativeAccessor behaviour have made the workaround not possible anymore, and the issue doesn't seem to be fixed in RN 0.59.9

[VladyslavKochetkov]

This is literally unusable with express and express-session because of this issue

[pinstripe-potatohead]

@sahrens This issue still persists on 0.60.4, and the workaround doesn't work since you removed it. Please look into it, as it makes react native with cookie based auth unusable

[jeremywiebe]

@sahrens I was looking at this and is there any reason why we couldn't juse use a standard Map<> within translateHeaders and then return Arguments.makeNativeMap(responseHeaders)? The return value is a WriteableNativeMap but it doesn't appear that we need to use it for the implementation of the function itself.

Something like this:

  private static WritableMap translateHeaders(Headers headers) {
    Map<String, Object> responseHeaders = new HashMap<>();
    for (int i = 0; i < headers.size(); i++) {
      String headerName = headers.name(i);
      // multiple values for the same header
     if (responseHeaders.containsKey(headerName)) {
        responseHeaders.put(
            headerName,
           responseHeaders.get(headerName).toString() + ", " + headers.value(i));
      } else {
        responseHeaders.put(headerName, headers.value(i));
      }
    }
    return Arguments.makeNativeMap(responseHeaders);
  }

[wibes]

Can we modify NetworkingModule.java file and temp fix in our projects ?

[vinceplusplus]

@jeremywiebe @jerolimov fixing from the source seems to fix it. I made a release and you can try with yarn add github:vinceplusplus/react-native#release/v0.61.2-multiple-set-cookie

[stale[bot]]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[stale[bot]]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.