Restore platform-specific cookie behavior

[jamesreggio]

This PR reverts commit 047961f, which was introduced by #10575.

The original patch was intended to make iOS behave more like a standards-compliant web browser in its handling of cookies. Specifically, it was built to seamlessly share the cookie jar between standard HTTP and WebSocket requests.

While the goals of the original patch are understandable, they contradict the outcome of a spirited debate within the community in #14063 (comment). The debate was whether React Native should default to platform-specific networking behavior, or whether it should try to normalize across platforms (with an eye towards behaving as close to a web browser as possible). The community resoundingly voted for preserving platform-specific behavior, 31 to 3.

This custom behavior arose as an issue because the patch in question breaks the semantics around withCredentials on fetch. Instead of mapping that value to setHTTPCookieAcceptPolicy, it pins setHTTPCookieAcceptPolicy to .Always and defines custom behavior. Naturally, the custom behavior fails to emulate the many edge-cases that exist in the system level cookie handling, which has led to a number of people complaining on #14869 and #14931.

I laid out an argument for both sides in #14931 (comment) and was met with mostly silence. However, stating my intention to revert the original patch garnered some support (as you'll see on that thread), and so here we are.

Hopefully this PR can provoke the conversation that needs to be had around cookie behavior, but in the absence of strong objection, I think the correct course of action is to merge this reversion and get RN a little further out of the business of abstracting the networking stack on iOS.

[panunu]

Thanks @jamesreggio for pushing this forward, appreciate it.

Would like to see this merged soon and have the unintentional BC break fixed.

[panunu]

@shergin sorry to trouble you.

As you approved #10575 would you mind checking this and sharing your thoughts? :-)

[facebook-github-bot]

@jamesreggio I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[pull-bot]

	Warnings
⚠️	📋 Test Plan - This PR appears to be missing a Test Plan.
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

[jamesreggio]

Anybody willing to take a look at this PR?

[shergin]

@clozr What do you think?

[Jacse]

@jamesreggio am I correct in understanding that this would fix #14869 and #15918? The commit breaks my app, as I need cookies to persist between redirects. Any way to get this going?

[Jacse]

After some testing, I can confirm that the PR does fix those issues.

[jamesreggio]

Hi @Jacse —

As you can see, I've put plenty of effort into collecting and explaining the history of this issue and the various philosophies that could govern its resolution. Maintainers have been repeatedly @mentioned and have dropped in for brief moments. But nothing has happened.

As with many React Native issues I've encountered, I've resigned myself to just monkey-patching the source code in my node_modules directory. It feels icky and makes RN upgrades a bit more onerous, but — in general — the approach has worked for us, and is something I can recommend.

For these patches, I generally make a commit against a local React Native repository, then use the follow command to export a patch:

git format-patch HEAD^ --stdout > my-changes.patch

From there, you can use a postinstall script in your package.json to apply the patch by doing the following:

cd node_module/react-native
patch -p1 -N < $PATCH_DIR/my-changes.patch

There's a tool called patch-package that supposedly automates this, but I haven't personally used it.

To save you some time, here's the patch for this PR:

https://gist.github.com/jamesreggio/5f4768906ee78a44c937e11e14b8d11e

And, if you're upset about non-standard, inconsistent handling of cookies, you might also be interested in this other patch I've been using, which fixes the behavior of the credentials option passed to fetch, which is also broken. (Without this patch, it's impossible to use credentials: 'omit' to disable cookies.)

https://gist.github.com/jamesreggio/fd3613c9af1dfa1938936e41eb16d6be

Both of these patches are against RN v0.50.3. You may have to rebase them and resolve conflicts if you're on a different version.

I hope this helps.

[aarondail]

@jamesreggio any update on this? This looks like it would solve the issue in #14869 (which sadly got closed as stale).

FWIW this is the patch I have been using to fix that problem:

diff --git a/node_modules/react-native/Libraries/Network/RCTNetworking.mm b/node_modules/react-native/Libraries/Network/RCTNetworking.mm
index 248dfce..ea3cd65 100644
--- a/node_modules/react-native/Libraries/Network/RCTNetworking.mm
+++ b/node_modules/react-native/Libraries/Network/RCTNetworking.mm
@@ -230,8 +230,14 @@ RCT_EXPORT_MODULE()
   request.HTTPMethod = [RCTConvert NSString:RCTNilIfNull(query[@"method"])].uppercaseString ?: @"GET";
 
   // Load and set the cookie header.
-  NSArray<NSHTTPCookie *> *cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookiesForURL:URL];
-  request.allHTTPHeaderFields = [NSHTTPCookie requestHeaderFieldsWithCookies:cookies];
+  // PATCH START
+  // Disable cookies from being automatically added unless withCredentials is set to true
+  // https://github.com/facebook/react-native/issues/14869
+  if ([RCTConvert BOOL:query[@"withCredentials"]]) {
+    NSArray<NSHTTPCookie *> *cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookiesForURL:URL];
+    request.allHTTPHeaderFields = [NSHTTPCookie requestHeaderFieldsWithCookies:cookies];
+  }
+  // PATCH END
 
   // Set supplied headers.
   NSDictionary *headers = [RCTConvert NSDictionary:query[@"headers"]];

[Jacse]

Getting really frustrated about the state of networking in React Native and the lack of decision making and action on this PR. Currently, cookies are completely lost when users are redirected.

[MishalValiyakath]

There is an issue with WebView in 0.54, which doesn't send cookies headers with any server requests. Anyone has any idea about this issue or fix for the issue, please help.

[SDrinkwater]

I am also facing a problem where the Authorization header is getting stripped when following a 302.
How can we get this moving again?

[morenoh149]

circleci hangs on android

#!/bin/bash -eo pipefail
source scripts/circle-ci-android-setup.sh && NO_BUCKD=1 retry3 buck install ReactAndroid/src/androidTest/buck-runner:instrumentation-tests --config build.threads=$BUILD_THREADS
Not using buckd because NO_BUCKD is set.
PARSING BUCK FILES... 1.9s (100%)
CREATING ACTION GRAPH: FINISHED IN 0.3s
BUILT 1/452 JOBS 0.0s //ReactAndroid/src/androidTest/buck-runner:instrumentation-tests#install
BUILT 55/421 JOBS 0.1s //ReactAndroid/src/main/jni/prebuilt:reactnative-libs
BUILT 125/394 JOBS 0.1s //ReactAndroid/src/main/third-party/java/buck-android-support:buck-android-support
BUILT 126/328 JOBS 0.0s //ReactAndroid/src/androidTest/assets:assets#assets-symlink-tree
warning: Ignoring InnerClasses attribute for an anonymous inner class
(org.hamcrest.generator.qdox.directorywalker.DirectoryScanner$1) that doesn't come with an
associated EnclosingMethod attribute. This class was probably produced by a
compiler that did not target the modern .class file format. The recommended
solution is to recompile the class from source, using an up-to-date compiler
and without specifying any "-target" type options. The consequence of ignoring
this warning is that reflective operations on this class will incorrectly
indicate that it is *not* an inner class.
warning: Ignoring InnerClasses attribute for an anonymous inner class
(org.hamcrest.generator.qdox.JavaDocBuilder$1) that doesn't come with an
associated EnclosingMethod attribute. This class was probably produced by a
compiler that did not target the modern .class file format. The recommended
solution is to recompile the class from source, using an up-to-date compiler
and without specifying any "-target" type options. The consequence of ignoring
this warning is that reflective operations on this class will incorrectly
indicate that it is *not* an inner class.
warning: Ignoring InnerClasses attribute for an anonymous inner class
(org.hamcrest.generator.qdox.JavaDocBuilder$2) that doesn't come with an
associated EnclosingMethod attribute. This class was probably produced by a
compiler that did not target the modern .class file format. The recommended
solution is to recompile the class from source, using an up-to-date compiler
and without specifying any "-target" type options. The consequence of ignoring
this warning is that reflective operations on this class will incorrectly
indicate that it is *not* an inner class.
warning: Ignoring InnerClasses attribute for an anonymous inner class
(org.hamcrest.generator.qdox.junit.APITestCase$1) that doesn't come with an
associated EnclosingMethod attribute. This class was probably produced by a
compiler that did not target the modern .class file format. The recommended
solution is to recompile the class from source, using an up-to-date compiler
and without specifying any "-target" type options. The consequence of ignoring
this warning is that reflective operations on this class will incorrectly
indicate that it is *not* an inner class.
warning: Ignoring InnerClasses attribute for an anonymous inner class
(org.hamcrest.generator.qdox.model.ModelBuilder$1) that doesn't come with an
associated EnclosingMethod attribute. This class was probably produced by a
compiler that did not target the modern .class file format. The recommended
solution is to recompile the class from source, using an up-to-date compiler
and without specifying any "-target" type options. The consequence of ignoring
this warning is that reflective operations on this class will incorrectly
indicate that it is *not* an inner class.
warning: Ignoring InnerClasses attribute for an anonymous inner class
(org.hamcrest.generator.qdox.tools.QDoxTester$1) that doesn't come with an
associated EnclosingMethod attribute. This class was probably produced by a
compiler that did not target the modern .class file format. The recommended
solution is to recompile the class from source, using an up-to-date compiler
and without specifying any "-target" type options. The consequence of ignoring
this warning is that reflective operations on this class will incorrectly
indicate that it is *not* an inner class.

BUILT 136/328 JOBS 1.6s //ReactAndroid/src/main/jni/prebuilt:android-jsc#aar_unzip
BUILT 141/328 JOBS 0.1s //ReactAndroid/src/main/third-party/java/testing-support-lib:runner#aar_unzip
BUILT 183/328 JOBS 0.0s //ReactAndroid/src/main/third-party/java/testing-support-lib:exposed-instrumentation-api#aar_unzip
BUILT 185/328 JOBS 0.5s //ReactAndroid/src/androidTest/java/com/facebook/react/tests:tests#dummy_r_dot_java
BUILT 186/328 JOBS 0.6s //ReactAndroid/src/main/android_res/com/facebook/catalyst/appcompat:appcompat
BUILT 187/328 JOBS 0.4s //ReactAndroid/src/main/java/com/facebook/react/modules/deviceinfo:deviceinfo#dex
BUILT 188/328 JOBS 0.6s //ReactAndroid/src/main/third-party/java/okio:okio#dex
BUILT 189/328 JOBS 0.0s //ReactAndroid/src/main/third-party/java/jsr-330:jsr-330#dex
BUILT 190/328 JOBS 0.1s //ReactAndroid/src/main/java/com/facebook/react/views/swiperefresh:swiperefresh#dex
BUILT 191/328 JOBS 0.0s //ReactAndroid/src/main/java/com/facebook/jni:jni#dex
BUILT 192/328 JOBS 0.0s //ReactAndroid/src/main/java/com/facebook/react/views/progressbar:progressbar#dex
...
BUILT 328/328 JOBS 4.0s //ReactAndroid/src/androidTest/buck-runner:instrumentation-tests
BUILDING: FINISHED IN 19.8s (100%) 328/328 JOBS, 151 UPDATED, 45.4% CACHE MISS
DOWNLOADED 0.00 BYTES/SEC AVG, 0 ARTIFACTS, 0.00 BYTES
Installing apk on emulator-5554 (testAVD).
Too long with no output (exceeded 10m0s)

@jamesreggio you need to add a test plan and release notes. See https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#pull-requests For inspiration see #21355 where test plan is as minimal as verifying the test suite passes ;-)

[jamesreggio]

To be honest, I don't think this should be merged any longer (and I don't think the maintainers do, either).

I opened this PR to quickly revert a controversial and (IMHO) ill-conceived change to diverge React Native's cookie behavior from the native platform specifications. I spent a fair bit of time summarizing the many related conversations in the PR description, and I eventually called for an up-or-down decision.

However, now that it's sat for over a year, I feel it's riskier to merge this, because developers have had a year to adapt to the behavior, and this could break the workarounds people have adopted.

I've held this PR open to help people find an answer for the bedeviling behavior I encountered, and I also wrote up a comment on how to revert the behavior locally using patches. For that reason, I'll keep it open, but I expect it to be closed eventually.

[ikhilko]

@jamesreggio am I right understand that for now there is no way to disable sending cookies using withCredentials: false in XMLHttpRequest? This is super weird, we have some cookies set in webview for api domain, so these cookies sent to api requests (and there is no way to disable it).

[hramos]

Closing per author's comment.