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.

Sign up for GitHub

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

Jump to bottom

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

Closed
pinstripe-potatohead opened this issue Aug 31, 2019 · 9 comments
Closed

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

pinstripe-potatohead opened this issue Aug 31, 2019 · 9 comments
Labels
Bug 🌐Networking Related to a networking API. Platform: Android Android applications. Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@pinstripe-potatohead
Copy link

pinstripe-potatohead commented Aug 31, 2019 •
edited
Loading

Multiple 'Set-Cookie' header values in HTTP response are not respected on Android RN 0.60.4 - only the last one shows up in JS. Multiple 'Set-Cookie' headers are concatenated into a string (correctly) on iOS.

React Native version: 0.60.5
@pinstripe-potatohead pinstripe-potatohead changed the title Multiple 'Set-Cookie' header values in HTTP response are not respected on Android RN 0.60.4 Multiple 'Set-Cookie' header values in HTTP response are not respected on Android RN 0.60.5 Aug 31, 2019
@react-native-bot react-native-bot added Platform: Android Android applications. 🌐Networking Related to a networking API. labels Aug 31, 2019
@chenyanfei-m
Copy link

Same problem in v0.59.8

@ronyv
Copy link

ronyv commented Oct 16, 2019

any updates on this ? is this a bug with react native or is it not a good practice to set multiple 'Set-Cookie' headers ?

@pinstripe-potatohead
Copy link
Author

pinstripe-potatohead commented Oct 17, 2019 •
edited
Loading

@ronyv Please look at #23005 and #21795 . AFAIK, multiple Set-Cookie headers are allowed by the spec. This is a bug with React Native Android. Downgrading to v0.59 will allow you to use the userNativeAccessor workaround

@ronyv
Copy link

ronyv commented Oct 17, 2019

@pinstripe-potatohead : I don't think downgrading the RN version is a good idea, considering the stability & other enhancements that the newer versions provides. I am wondering why this still remains as unresolved when these are something fundamental.

@hramos : any comments on this ?

@chenyanfei-m
Copy link

add

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

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

in MainApplication.java can resolve it.

@ronyv
Copy link

ronyv commented Oct 18, 2019

@chenyanfei-m : what you suggested is just a workaround for RN versions < 0.60. . I am using 0.60.5 and this is not working. I just don't want to downgrade my RN version for resolving this issue. Rather i will consider some other efficient alternative. Hope RN will fix this fundamental issue in the upcoming releases.

@vinceplusplus vinceplusplus mentioned this issue Oct 31, 2019
Closed
facebook-github-bot pushed a commit that referenced this issue Nov 5, 2019
@facebook-github-bot
Fix multiple set-cookie not aggregated correctly in response headers (
1df8bd4 
#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
@grabbou
Fix multiple set-cookie not aggregated correctly in response headers (
1a33607 
#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
@ozican
Copy link

ozican commented Dec 5, 2019

@ronyv Hello brother. Did you solve the issue? I still fighting with it, and any help would be great.

douglowder pushed a commit to react-native-tvos/react-native-tvos that referenced this issue Dec 11, 2019
@douglowder
Fix multiple set-cookie not aggregated correctly in response header…
3afcf92 
…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
esamelson pushed a commit to expo/react-native that referenced this issue Mar 2, 2020
@esamelson
Fix multiple set-cookie not aggregated correctly in response headers (
4af2ba2 
facebook#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#26280, facebook#21795, facebook#23185

## Changelog

[Android] [Fixed] - Fix multiple headers of the same name (e.g. `set-cookie`) not aggregated correctly
Pull Request resolved: facebook#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 Mar 4, 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 Mar 4, 2020
@stale
Copy link

stale bot commented Mar 12, 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 Mar 12, 2020
@facebook facebook locked as resolved and limited conversation to collaborators Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Bug 🌐Networking Related to a networking API. Platform: Android Android applications. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ronyv @ozican @pinstripe-potatohead @chenyanfei-m @react-native-bot