withCredentials flag in XHRs should default to "true"

[DanielZlotin]

Description

react-native 0.44 introduced withCredentials flag in XHRs, which, if not specified in every fetch request, defaults to false.
This change conflicts with the default behavior in native. In the iOS native SDK and the Android native SDK, when making a native HTTP request, cookies are sent by default.
We rarely have agreement between the platforms, but for the last 10 years they both agree on this security model for apps.
This greatly affects projects relying on cookies with their requests.

Additional Information

• React Native version: 0.44
• Platform: both
• Development Operating System: macOS

[talkol]

According to the commit description, the reason for this breaking change is to be

"consistent with the default behavior of XHR on web for cross-site requests".

This doesn't make much sense to me. These are native apps. Native apps don't have cross-site concerns. The JS bundle is not served from a domain like the web.

The security model for native mobile apps has been established a long time ago. The standard native API's for making HTTP requests in iOS and Android send cookies by default. Changing this behavior to conform to websites just because we're using JavaScript is strange.

In addition, there's a big problem with the override mechanism. This is a breaking change, and now we have apps in production that we cannot release due to this change.

The override mechanism according to the commit is:

"Developers can restore the previous behavior by passing true for XHR's withCredentials argument".

This makes the assumption that we can control the parameters for every request our app makes. Consider that we're using a 3rd party GraphQL client library that makes the fetch requests for us. This library is out of our control meaning we can't use the override mechanism. To support backwards compatibility for existing apps that are in production when introducing these types of changes, the minimum is to allow a global override when the app starts.

@rigdern

[rigdern]

I'm sorry that my commit is causing issues for you.

The standard native API's for making HTTP requests in iOS and Android send cookies by default.

Which Android API are you referring to?

I think there are several questions to think about here:

1. What should RN's withCredentials default to?

The answer is not obvious to me. Ignoring the web, different APIs I'm familiar with have made different choices regarding the default for sending and saving cookies:

• Libraries that disable cookies by default:
  • The request module for Node. You have to set jar to true to enable cookies.
  • OkHttp for Android. You have to provide a CookieJar to enable cookies.
• Libraries that enable cookies by default:
  • NSMutableURLRequest built into iOS. You have to set HTTPShouldHandleCookies to false to disable cookies.

I'm not familiar with the rationale behind the chosen defaults of any of these libraries. If anybody is deeply familiar with this, it would be useful if you could provide or link to an explanation. Hearing about the rationale behind withCredentials in browsers would be especially interesting because React Native copies XHR's API surface. Understanding all of this will be helpful in picking the right default for React Native.

2. Should libraries that make HTTP requests expose withCredentials?

If they don't expose withCredentials, it seems like you could run into similar problems in a web app when you're making requests to another domain. Is that correct? If so, how would you solve this problem in a web app?

3. Should RN make the default of withCredentials configurable?

Because changing the default of withCredentials was a breaking change, this might be useful to help apps adjust to the breaking change.

Disregarding the breaking change, would such an API be a good idea? I don't know. How do other HTTP APIs solve this problem? Do they give you a switch for globally enabling/disabling cookies?

Adam Comella
Microsoft Corp.

[isnifer]

It's time to follow semver :)

[itsmepetrov]

We also faced with this problem, but fortunately, we have direct access to all API calls in our app.
As a workaround, we use fetch with credentials: 'include'.

[vafada]

This broke our app too. The original fix looks like it conflicts with:

https://github.com/github/fetch/blob/08602ff819f4c41e9d9e9c2c31bfc853b1bb5bf2/fetch.js#L448-L450

It seems to me there a lot of places which sets withCredentials and each place does different things.

[rigdern]

It seems to me there a lot of places which sets withCredentials and each place does different things.

@vafada What places are you referring to? I believe the place you linked to in an implementation of fetch is fine. When you pass credentials: 'include' to fetch, it should have the same behavior as setting withCredentials to true in XMLHttpRequest. That's exactly the case the code you linked to is handling.

[vafada]

@rigdern

Sorry, I just didn't understand the code well enough:

I was doing:

fetch(url, {
   ...options
   withCredentials: true,
  credentials: 'include',
});

when I should be doing:

fetch(url, {
   ...options
  credentials: 'include', // sets    withCredentials to true
});

[itsmepetrov]

Also, what about credentials: 'same-origin'? Should it work as a fallback to 'include' or something else?

[shergin]

I personally agree with @rigdern, cookies should be disabled by default.
It is kinda standard nowadays (not only for browsers) that Cookies is opt-in feature.

We simply have to adopt new policy. Peace.

[ericvicenti]

Apologies for not taking this under more careful consideration when reviewing the pull request! I thought this would be a strict win because it brings the two platforms in alignment, but as @talkol points out, it now conflicts with the behavior of the native networking libraries.

At this point I think it may be worthwhile to keep the new behavior, because we've already switched it, it matches the behavior of JS fetch on the web, and it offers a slightly better security profile. Also, as I understand, the new behavior brings iOS in line with Android.

There are some tradeoffs here so I'd like to run a quick community poll for those paying attention to this issue. Please vote within the next 24 hours:

Vote Here!

Please "thumbs-up" this post if you want a mechanism to change the default. But we will keep the new behavior that was introduced in 0.44. (fetch default is credentials: 'omit').

To enable people to use newer versions of RN, we will add a mechanism to return the default to true. We will cherry-pick this new mechanism to 0.44 and 0.45. Pending naming, it would look like this:

fetch.setDefaultCredentials('include');

Give this comment a "thumbs-down" if you want to revert to the default credentials: 'include' behavior that we had on iOS before 0.44.

We could theoretically do this by reverting 454ab8, but it would probably be cleaner to override the default from fetch.js.

As a followup, we will need to decide what to do with the Android behavior.

[ericvicenti]

Ok, its only been an hour and we've got pretty clear signal: 13 votes to revert to the old credentials default, and 1 vote to keep the consistent behavior with override mechanism. I'll let the vote keep going for the next day, but it sounds like we should go back to the old default.