Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mdloselo opened this issue Oct 15, 2018 · 41 comments
Labels
Bug 🌐Networking Related to a networking API. Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@mdloselo
Copy link

mdloselo commented Oct 15, 2018

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 mdloselo changed the title Response Headers with the same key is not processed properly. [The expected behaviour is that the values of the identical keys are combined to a string separated by a comma] 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] Oct 15, 2018
@mdloselo
Copy link
Author

mdloselo commented Oct 16, 2018

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
Copy link
Author

mdloselo commented Oct 16, 2018

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.

@mdloselo mdloselo changed the title 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] 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] Oct 16, 2018
@react-native-bot react-native-bot added the 🌐Networking Related to a networking API. label Oct 16, 2018
@Cyberclown
Copy link

Cyberclown commented Oct 16, 2018

+1, very annoying issue

@russell-matt

This comment has been minimized.

@brazerZa

This comment has been minimized.

@timdelange
Copy link

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

This comment has been minimized.

@StatikVerse

This comment has been minimized.

@tshepoR

This comment has been minimized.

@zinzan-vdm
Copy link

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.

@theTechnicalBA

This comment has been minimized.

@hey99xx
Copy link

hey99xx commented Oct 16, 2018

@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
Copy link

hey99xx commented Oct 16, 2018

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
Copy link
Author

mdloselo commented Oct 17, 2018

@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
Copy link

hey99xx commented Oct 17, 2018

@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
Copy link

dakiesse commented Nov 1, 2018

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

@astapinski
Copy link

astapinski commented Dec 19, 2018

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
Copy link

Artorp commented Jan 11, 2019

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.

@mxwiz

This comment has been minimized.

@jbyrneie
Copy link

jbyrneie commented Feb 8, 2019

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

@astapinski
Copy link

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
Copy link

Works still 0.55.4.

@mxwiz

This comment has been minimized.

@mxwiz
Copy link

mxwiz commented Mar 19, 2019

@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
Copy link
Contributor

hramos commented Mar 19, 2019

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

@mxwiz
Copy link

mxwiz commented Mar 19, 2019

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

@christoph-jerolimov
Copy link
Contributor

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.

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
Copy link

hey99xx commented Apr 5, 2019

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
Copy link
Contributor

sahrens commented Apr 5, 2019

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
Copy link
Contributor

sahrens commented Apr 5, 2019

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

@sahrens
Copy link
Contributor

sahrens commented Apr 5, 2019

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

@pinstripe-potatohead
Copy link

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
Copy link

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

@pinstripe-potatohead
Copy link

pinstripe-potatohead commented Aug 22, 2019

@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
Copy link

jeremywiebe commented Oct 1, 2019

@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
Copy link

wibes commented Oct 14, 2019

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

@vinceplusplus
Copy link

@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

facebook-github-bot pushed a commit that referenced this issue Nov 5, 2019
#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
grabbou pushed a commit that referenced this issue Nov 23, 2019
#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
douglowder pushed a commit to react-native-tvos/react-native-tvos that referenced this issue Dec 11, 2019
…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
Copy link

stale bot commented Jan 30, 2020

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 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
Copy link

stale bot commented Feb 6, 2020

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.

@stale stale bot closed this as completed Feb 6, 2020
@facebook facebook locked as resolved and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug 🌐Networking Related to a networking API. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

No branches or pull requests