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] #21795

mdloselo · 2018-10-15T11:30:22Z

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.

The text was updated successfully, but these errors were encountered:

mdloselo · 2018-10-16T07:11:00Z

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 · 2018-10-16T07:15:29Z

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 · 2018-10-16T09:45:35Z

+1, very annoying issue

timdelange · 2018-10-16T10:05:35Z

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.

zinzan-vdm · 2018-10-16T10:42:14Z

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.

hey99xx · 2018-10-16T20:15:34Z

@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 · 2018-10-16T20:26:49Z

I think this issue is identical btw #18837 and the comments there also suggests the out of sync Java and C++ maps is the root cause.

mdloselo · 2018-10-17T07:53:32Z

@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 7891805 because before that mUseNativeAccessor was never there.

hey99xx · 2018-10-17T17:15:20Z

@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 4ac500a. 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 · 2018-11-01T17:25:19Z

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

astapinski · 2018-12-19T01:33:30Z

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 · 2019-01-11T18:37:09Z

This is still an issue on react-native@0.57.1 (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.

jbyrneie · 2019-02-08T16:18:12Z

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

astapinski · 2019-02-08T21:06:32Z

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 · 2019-02-28T10:27:33Z

Works still 0.55.4.

mxwiz · 2019-03-19T06:17:30Z

@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 · 2019-03-19T12:14:53Z

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

mxwiz · 2019-03-19T15:34:48Z

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

jerolimov · 2019-04-04T09:37:58Z

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.

react-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

Lines 647 to 661 in 9895d01

    
           private static WritableMap translateHeaders(Headers headers) { 
        
             WritableMap responseHeaders = Arguments.createMap(); 
        
             for (int i = 0; i < headers.size(); i++) { 
        
               String headerName = headers.name(i); 
        
               // multiple values for the same header 
        
               if (responseHeaders.hasKey(headerName)) { 
        
                 responseHeaders.putString( 
        
                     headerName, 
        
                     responseHeaders.getString(headerName) + ", " + headers.value(i)); 
        
               } else { 
        
                 responseHeaders.putString(headerName, headers.value(i)); 
        
               } 
        
             } 
        
             return responseHeaders; 
        
           }

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 #21795 (comment) and by @Return-1 #23005 (comment) 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 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. 🙌

(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 · 2019-04-05T09:21:33Z

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 · 2019-04-05T17:24:09Z

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 · 2019-04-05T18:56:29Z

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

sahrens · 2019-04-05T18:57:08Z

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

pinstripe-potatohead · 2019-06-10T06:06:19Z

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 · 2019-07-02T19:41:51Z

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

pinstripe-potatohead · 2019-08-22T20:59:47Z

@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 · 2019-10-01T00:19:03Z

@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 · 2019-10-14T10:55:09Z

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

vinceplusplus · 2019-10-31T04:29:55Z

@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

#27066) Summary: Multiple `set-cookie` headers should be aggregated as one `set-cookie` with values in a comma separated list. It is working as expected on iOS but not on Android. On Android, only the last one is preserved The problem arises because `NetworkingModule.translateHeaders()` uses `WritableNativeMap` as the translated headers but uses both native and non-native methods. The mixup causes out of sync data that both sets of methods do no agree. A simple fix is to use `Bundle` as the storage and only convert it to `WritableMap` at the end in one go Related issues: #26280, #21795, #23185 ## Changelog [Android] [Fixed] - Fix multiple headers of the same name (e.g. `set-cookie`) not aggregated correctly Pull Request resolved: #27066 Test Plan: A mock api, https://demo6524373.mockable.io/, will return 2 `set-cookie` as follows: ``` set-cookie: cookie1=value1 set-cookie: cookie2=value2 ``` Verify the following will print the `set-cookie` with a value `cookie1=value1, cookie2=value2` ```javascript fetch('https://demo6524373.mockable.io/') .then(response => { console.log(response.headers); }); ``` On iOS, `set-cookie` will have `cookie1=value1, cookie2=value2` while on Android it will have `cookie2=value2` (preserving only the last one) Differential Revision: D18298933 Pulled By: cpojer fbshipit-source-id: ce53cd41d7c6de0469700617900f30a7d0914c26

…s (#27066) Summary: Multiple `set-cookie` headers should be aggregated as one `set-cookie` with values in a comma separated list. It is working as expected on iOS but not on Android. On Android, only the last one is preserved The problem arises because `NetworkingModule.translateHeaders()` uses `WritableNativeMap` as the translated headers but uses both native and non-native methods. The mixup causes out of sync data that both sets of methods do no agree. A simple fix is to use `Bundle` as the storage and only convert it to `WritableMap` at the end in one go Related issues: facebook/react-native#26280, facebook/react-native#21795, facebook/react-native#23185 ## Changelog [Android] [Fixed] - Fix multiple headers of the same name (e.g. `set-cookie`) not aggregated correctly Pull Request resolved: facebook/react-native#27066 Test Plan: A mock api, https://demo6524373.mockable.io/, will return 2 `set-cookie` as follows: ``` set-cookie: cookie1=value1 set-cookie: cookie2=value2 ``` Verify the following will print the `set-cookie` with a value `cookie1=value1, cookie2=value2` ```javascript fetch('https://demo6524373.mockable.io/') .then(response => { console.log(response.headers); }); ``` On iOS, `set-cookie` will have `cookie1=value1, cookie2=value2` while on Android it will have `cookie2=value2` (preserving only the last one) Differential Revision: D18298933 Pulled By: cpojer fbshipit-source-id: ce53cd41d7c6de0469700617900f30a7d0914c26

stale · 2020-01-30T02:42:16Z

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 · 2020-02-06T03:06:34Z

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.

react-native-bot added the 🌐Networking Related to a networking API. label Oct 16, 2018

This comment has been minimized.

Sign in to view

hey99xx mentioned this issue Nov 1, 2018

"Set-Cookie" contains only last item #22064

Closed

react-native-bot added the Bug Report label Jan 26, 2019

hey99xx mentioned this issue Jan 29, 2019

Cookie based authentication issues aggregation #23185

Open

hramos removed the Bug Report label Feb 6, 2019

This comment has been minimized.

Sign in to view

cpojer mentioned this issue Mar 19, 2019

RN Issue Triage Group 1 #24028

Closed

jerolimov mentioned this issue Apr 4, 2019

Cookie-Based Authentication Is Not Possible #19958

Closed

jerolimov mentioned this issue Apr 4, 2019

Fetch only brings latest value of same-name headers on Android, works on iOS. #23005

Closed

react-native-bot added the Bug label May 4, 2019

hramos removed the Type: Bug Report label May 6, 2019

pinstripe-potatohead mentioned this issue Oct 10, 2019

Not getting all cookies in android react-native 0.60 #26781

Closed

pinstripe-potatohead mentioned this issue Oct 17, 2019

Multiple 'Set-Cookie' header values in HTTP response are not respected on Android RN 0.60.5 #26280

Closed

vinceplusplus mentioned this issue Oct 31, 2019

Fix multiple set-cookie not aggregated correctly in response headers #27066

Closed

stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 30, 2020

stale bot closed this as completed Feb 6, 2020

facebook locked as resolved and limited conversation to collaborators Feb 6, 2020

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] #21795

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] #21795

Comments

mdloselo commented Oct 15, 2018 • edited Loading

Environment

Description

Reproducible Demo

mdloselo commented Oct 16, 2018 • edited Loading

mdloselo commented Oct 16, 2018 • edited Loading

Cyberclown commented Oct 16, 2018 • edited Loading

This comment has been minimized.

This comment has been minimized.

timdelange commented Oct 16, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zinzan-vdm commented Oct 16, 2018

This comment has been minimized.

hey99xx commented Oct 16, 2018 • edited Loading

hey99xx commented Oct 16, 2018

mdloselo commented Oct 17, 2018 • edited Loading

hey99xx commented Oct 17, 2018 • edited Loading

dakiesse commented Nov 1, 2018

astapinski commented Dec 19, 2018 • edited Loading

Artorp commented Jan 11, 2019

This comment has been minimized.

jbyrneie commented Feb 8, 2019

astapinski commented Feb 8, 2019

bucketclan commented Feb 28, 2019

This comment has been minimized.

mxwiz commented Mar 19, 2019

hramos commented Mar 19, 2019

mxwiz commented Mar 19, 2019

jerolimov commented Apr 4, 2019

hey99xx commented Apr 5, 2019

sahrens commented Apr 5, 2019

sahrens commented Apr 5, 2019

sahrens commented Apr 5, 2019

pinstripe-potatohead commented Jun 10, 2019

VladyslavKochetkov commented Jul 2, 2019

pinstripe-potatohead commented Aug 22, 2019 • edited Loading

jeremywiebe commented Oct 1, 2019 • edited Loading

wibes commented Oct 14, 2019

vinceplusplus commented Oct 31, 2019

stale bot commented Jan 30, 2020

stale bot commented Feb 6, 2020

mdloselo commented Oct 15, 2018 •

edited

Loading

mdloselo commented Oct 16, 2018 •

edited

Loading

mdloselo commented Oct 16, 2018 •

edited

Loading

Cyberclown commented Oct 16, 2018 •

edited

Loading

hey99xx commented Oct 16, 2018 •

edited

Loading

mdloselo commented Oct 17, 2018 •

edited

Loading

hey99xx commented Oct 17, 2018 •

edited

Loading

astapinski commented Dec 19, 2018 •

edited

Loading

pinstripe-potatohead commented Aug 22, 2019 •

edited

Loading

jeremywiebe commented Oct 1, 2019 •

edited

Loading