Skip to content

Commit

Permalink
Fix multiple set-cookie not aggregated correctly in response headers (
Browse files Browse the repository at this point in the history
#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
  • Loading branch information
Vincent Cheung authored and grabbou committed Nov 23, 2019
1 parent 51f74e7 commit 1a33607
Showing 1 changed file with 4 additions and 3 deletions.
Expand Up @@ -7,6 +7,7 @@
package com.facebook.react.modules.network;

import android.net.Uri;
import android.os.Bundle;
import android.util.Base64;
import androidx.annotation.Nullable;
import com.facebook.common.logging.FLog;
Expand Down Expand Up @@ -625,18 +626,18 @@ private synchronized void cancelAllRequests() {
}

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

@ReactMethod
Expand Down

2 comments on commit 1a33607

@ozican
Copy link

@ozican ozican commented on 1a33607 Dec 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not working for me

@JofBigHealth
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't thank you both enough for this! I just couldn't make sense of this bug and it's quite time-consuming to look at due to requiring use of Android Studio's network tools.

@ozican How are you measuring "not working"? It works for me. If you're looking at the traffic via RNDebugger that won't work. See issue here jhen0409/react-native-debugger#340 (comment)

Please sign in to comment.