Skip to content

Conversation

rigdern
Copy link
Contributor

@rigdern rigdern commented Feb 8, 2017

Corresponding Android PR: #12276

Respect the withCredentials XMLHttpRequest flag for sending cookies with requests. This can reduce payload sizes where large cookies are set for domains.

This should fix #5347.

This is a breaking change because it alters the default behavior of XHR. Prior to this change, XHR would send cookies by default. After this change, by default, XHR does not send cookies which is consistent with the default behavior of XHR on web for cross-site requests. Developers can restore the previous behavior by passing true for XHR's withCredentials argument.

Test plan (required)

Verified in a test app that XHR works properly when specifying withCredentials as true, false, and undefined. Also, my team uses this change in our app.

Adam Comella
Microsoft Corp.

Respect the withCredentials XMLHttpRequest flag for sending cookies with requests. This can reduce payload sizes where large cookies are set for domains.

This should fix facebook#5347.

This is a breaking change because it alters the default behavior of XHR. Prior to this change, XHR would send cookies by default. After this change, by default, XHR does not send cookies which is consistent with the default behavior of XHR on web for cross-site requests. Developers can restore the previous behavior by passing `true` for XHR's `withCredentials` argument.
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Feb 8, 2017
rigdern pushed a commit to rigdern/react-native that referenced this pull request Feb 8, 2017
Corresponding iOS PR: facebook#12275

Respect the withCredentials XMLHttpRequest flag for sending cookies with requests. This can reduce payload sizes where large cookies are set for domains.

This should fix facebook#5347.

This is a breaking change because it alters the default behavior of XHR. Prior to this change, XHR would send cookies by default. After this change, by default, XHR does not send cookies which is consistent with the default behavior of XHR on web for cross-site requests. Developers can restore the previous behavior by passing `true` for XHR's `withCredentials` argument.

**Test plan (required)**

Verified in a test app that XHR works properly when specifying `withCredentials` as `true`, `false`, and `undefined`. Also, my team uses this change in our app.

Adam Comella
Microsoft Corp.
@rigdern rigdern changed the title Breaking Change: iOS: Support withCredentials flag in XHRs BREAKING: iOS: Support withCredentials flag in XHRs Feb 9, 2017
@mkonicek
Copy link
Contributor

mkonicek commented Mar 8, 2017

Docs on withCredentials here: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/withCredentials

Looks good. Thanks for adding the "BREAKING" prefix.

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Mar 8, 2017
@facebook-github-bot
Copy link
Contributor

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@rigdern rigdern deleted the rigdern/iosXhrWithCreds branch March 8, 2017 21:40
facebook-github-bot pushed a commit that referenced this pull request Apr 11, 2017
Summary:
Corresponding iOS PR: #12275

Respect the withCredentials XMLHttpRequest flag for sending cookies with requests. This can reduce payload sizes where large cookies are set for domains.

This should fix #5347.

This is a breaking change because it alters the default behavior of XHR. Prior to this change, XHR would send cookies by default. After this change, by default, XHR does not send cookies which is consistent with the default behavior of XHR on web for cross-site requests. Developers can restore the previous behavior by passing `true` for XHR's `withCredentials` argument.

**Test plan (required)**

Verified in a test app that XHR works properly when specifying `withCredentials` as `true`, `false`, and `undefined`. Also, my team uses this change in our app.

Adam Comella
Microsoft Corp.
Closes #12276

Differential Revision: D4673646

Pulled By: ericvicenti

fbshipit-source-id: 924c230c9df72071b3cf9151c3ac201905ac28a5
@matt-42
Copy link

matt-42 commented May 3, 2017

It seems like this conversion [1] does not work as expected. It always gives false, at least on the IOS simulator. This completely breaks cookies on IOS for 0.44.

[1] https://github.com/rigdern/react-native/blob/1b680213628ca9df3622acc9b67b49dcd6f6c30b/Libraries/Network/RCTNetworking.mm#L233

@Benjamin-Dobell
Copy link
Contributor

Benjamin-Dobell commented May 6, 2017

Very breaking indeed 😉 I didn't find mention to this change in the release notes even though it's a breaking change. Took me a while to find out what was going on.

However, I think things are little more broken than intended.

The fetch API is presently (in 0.44.0) not functioning correctly. The following:

fetch(url, {credentials: 'include'})

does not work. Instead the following is required:

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

Note the inclusion of withCredentials: true. withCredentials does not correspond to a fetch API property.

thotegowda pushed a commit to thotegowda/react-native that referenced this pull request May 7, 2017
Summary:
Corresponding iOS PR: facebook#12275

Respect the withCredentials XMLHttpRequest flag for sending cookies with requests. This can reduce payload sizes where large cookies are set for domains.

This should fix facebook#5347.

This is a breaking change because it alters the default behavior of XHR. Prior to this change, XHR would send cookies by default. After this change, by default, XHR does not send cookies which is consistent with the default behavior of XHR on web for cross-site requests. Developers can restore the previous behavior by passing `true` for XHR's `withCredentials` argument.

**Test plan (required)**

Verified in a test app that XHR works properly when specifying `withCredentials` as `true`, `false`, and `undefined`. Also, my team uses this change in our app.

Adam Comella
Microsoft Corp.
Closes facebook#12276

Differential Revision: D4673646

Pulled By: ericvicenti

fbshipit-source-id: 924c230c9df72071b3cf9151c3ac201905ac28a5
@rigdern
Copy link
Contributor Author

rigdern commented May 17, 2017

I just tested this in React Native 0.44 and everything seems to be working.

@matt-42 Are you setting withCredentials to true when constructing your XMLHttpRequest object?

@Benjamin-Dobell Things worked for me when using fetch(url, {credentials: 'include'}). Are you sure you are using the fetch polyfill that ships with React Native? I looked into how fetch and credentials work in React Native and it seems it just sets withCredentials on an XMLHttpRequest:

  • React Native publishes fetch as a global here by importing the fetch module.
  • The fetch module uses the npm packages whatwg-fetch as can be seen here.
  • Looking at the implementation of whatwg-fetch, you can see it's built on top of XMLHttpRequest and sets withCredentials based on the value of credentials.

If you can provide more details about what's broken for you, I can look into it. As far as I can tell, everything is working as expected.

I'll see if I can get this added to the breaking change list so it doesn't surprise other people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XMLHttpRequest polyfill doesn't provide the withCredentials property

5 participants