-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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
Fix multiple set-cookie
not aggregated correctly in response headers
#27066
Fix multiple set-cookie
not aggregated correctly in response headers
#27066
Conversation
@mdvacca Can this be merged? This issue has been preventing cookie based auth since the changes made in v0.60, and a fix would be much appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by Vincent Cheung in 1df8bd4. When will my fix make it into a release? | Upcoming Releases |
Will this fix be available in the next release? |
Will this be available soon? |
#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
not working for me |
…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
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
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
Summary
Multiple
set-cookie
headers should be aggregated as oneset-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 preservedThe problem arises because
NetworkingModule.translateHeaders()
usesWritableNativeMap
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 useBundle
as the storage and only convert it toWritableMap
at the end in one goRelated issues: #26280, #21795, #23185
Changelog
[Android] [Fixed] - Fix multiple headers of the same name (e.g.
set-cookie
) not aggregated correctlyTest Plan
A mock api, https://demo6524373.mockable.io/, will return 2
set-cookie
as follows:Verify the following will print the
set-cookie
with a valuecookie1=value1, cookie2=value2
On iOS,
set-cookie
will havecookie1=value1, cookie2=value2
while on Android it will havecookie2=value2
(preserving only the last one)