Skip to content
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

[iOS] Fix cookies not being sent after a redirected response #22178

Closed
wants to merge 1 commit into from

Conversation

corradio
Copy link
Contributor

@corradio corradio commented Nov 6, 2018

Fixes #19376

On iOS, the HTTPShouldHandleCookies boolean has no effect when custom cookies are set (see https://developer.apple.com/documentation/foundation/nsmutableurlrequest/1415485-httpshouldhandlecookies?preferredLanguage=occ). Setting custom cookies prevents the cookies of a redirect response from being re-used in the subsequent request.
This is why issue #19376 is opened.
The fix is to intercept the redirect request, and to manually set the cookies. This effectively makes the Android and iOS behaviours converge.

Changelog:

[iOS] [Fixed] - Fix cookies not being sent on iOS after redirect

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@pull-bot
Copy link

pull-bot commented Nov 6, 2018

Warnings
⚠️

📋 Test Plan - This PR appears to be missing a Test Plan.

⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 6, 2018
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Nov 6, 2018
@jamesreggio
Copy link
Contributor

I'm a little sad to see that this PR contributes additional proprietary cookie behavior that deviates from the platform.

I still think the correct course of action is what I proposed in #16127, which is to remove React Native's half-baked efforts to replicate browser cookie handling, and instead let the native networking on iOS and Android dictate the behavior.

Either way, I applaud your effort, and hope you hear from the maintainers.

@morenoh149
Copy link
Contributor

If the choice is between a uniform api that is inconsistent with iOS/android or an inconsistent api for each platform I'd choose the first. To do the second you'll have to expose the underlying apis and developers would have to read two sets of docs. I've seen this first hand with a native module I maintain, no easy answer.

@jamesreggio
Copy link
Contributor

@morenoh149 — I appreciate your viewpoint, but how do you ensure that the APIs are consistent between iOS and Android? As it stands, cookie handling on RN is already inconsistent between iOS and Android, and is inconsistent with the platform defaults for each. And because of the former inconsistencies, people just open PRs as needed to add/fix behavior they think is reasonable.

I still think the easiest way to avoid this thicket of complexity is to get RN out of the business of handling cookie-related edge cases.

@shergin
Copy link
Contributor

shergin commented Nov 23, 2018

@jamesreggio

I'm a little sad to see that this PR contributes additional proprietary cookie behavior that deviates from the platform.

Despite the famous "learn one, write anywhere" moto, React Native is a platform. There is a HUGE value in unifying APIs and behaviors across underlying platforms.
I am not aware of any value that relying on underlying-platform-specifics provide. I would love to learn about that.

Using web standards as a common determinator is usually a good thing, it provides consistency and a unified security model.

How do you ensure that the APIs are consistent between iOS and Android?

Even if we cannot ensure or proof that it's consistent, there is a value to have them unified even at some level.

Cookie handling on RN is already inconsistent between iOS and Android and is inconsistent with the platform defaults for each.

That's terrible. I don't care about being consistent with iOS or Android specifics, IMHO they should be consistent across RN on all platforms and with web best practices.

@shergin
Copy link
Contributor

shergin commented Nov 23, 2018

@corradio
Could you please provide a link to some standard that describe the implemented behaviour?

@corradio
Copy link
Contributor Author

corradio commented Nov 25, 2018

@shergin I haven't had time to investigate but the native iOS implementation actually works like that. The fact that RN uses a custom cookie store forces us to add this edge case in order to restore default behaviour.
Maybe there's something in https://www.ietf.org/rfc/rfc2109.txt ?

@shergin
Copy link
Contributor

shergin commented Nov 26, 2018

Yeah, I think it satisfies "3.3.6 Sending Cookies in Unverifiable Transactions" from https://www.ietf.org/rfc/rfc2965.txt .

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 26, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@corradio merged commit a686048 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Nov 26, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 26, 2018
shseike added a commit to shseike/react-native that referenced this pull request Dec 14, 2018
Fix cookies not being sent after a redirected response (facebook#22178)

Summary:
Fixes facebook#19376

On iOS, the `HTTPShouldHandleCookies` boolean has no effect when custom cookies are set (see https://developer.apple.com/documentation/foundation/nsmutableurlrequest/1415485-httpshouldhandlecookies?preferredLanguage=occ). Setting custom cookies prevents the cookies of a redirect response from being re-used in the subsequent request.
This is why issue facebook#19376 is opened.
The fix is to intercept the redirect request, and to manually set the cookies. This effectively makes the Android and iOS behaviours converge.

Changelog:
----------
[iOS] [Fixed] - Fix cookies not being sent on iOS after redirect
Pull Request resolved: facebook#22178

Differential Revision: D13191961

Pulled By: shergin

fbshipit-source-id: 4445a2499034a9846c6d7c37c1f1b72c9fd30129
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookies set in a redirect response are not persisted
8 participants