added cookie headers to websocket and XMLHttpRequest #7167
Conversation
|
By analyzing the blame information on this pull request, we identified @philikon and @nicklockwood to be potential reviewers. |
|
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 - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
| @@ -48,6 +48,19 @@ - (void)dealloc | |||
| RCT_EXPORT_METHOD(connect:(NSURL *)URL protocols:(NSArray *)protocols headers:(NSDictionary *)headers socketID:(nonnull NSNumber *)socketID) | |||
| { | |||
| NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:URL]; | |||
|
|
|||
| // We load cookies from sharedHTTPCookieStorage (shared with XHR and | |||
| // fetch). To get HTTPS-only cookies for wss URLs, replace wss with https | |||
There was a problem hiding this comment.
Maybe say "secure" cookies instead of HTTPS-only because "http-only" means something different for cookies
There was a problem hiding this comment.
Good point. I will update the description
|
This looks mostly good to me -- just a couple small comments. We're working to get the CI testing back in a good state so it might be a few days before we feel more confident merging this but thank you for the PR! |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
@clozr updated the pull request. |
1 similar comment
|
@clozr updated the pull request. |
clozr
changed the title
|
Added the same code to XMLHttpRequest as well |
|
@clozr updated the pull request. |
1 similar comment
|
@clozr updated the pull request. |
|
@ide CI is green now. OK to merge this? Do we need a test plan? |
|
|
||
| // Load and set the cookie header. | ||
| NSArray<NSHTTPCookie *> *cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookiesForURL:URL]; | ||
| request.allHTTPHeaderFields = [NSHTTPCookie requestHeaderFieldsWithCookies:cookies]; |
There was a problem hiding this comment.
NSURLSession normally sends cookies automatically unless told not to. Are you sure this is necessary?
There was a problem hiding this comment.
RCTNetworking is using NSURLSession while Websocket is using NSURLConnection. So it may not be necessary for RCTNetworking. However currently NSURL session is initialized with default configuration, without specifying HTTPCookieAcceptPolicy, HTTPCookieStorage and HTTPShouldSetCookies. I can make the changes and test it out if you want to go that route.
I think that would be a better approach too.
On the other for Websocket, we may need to do this.
|
It would be great if you could include some test code for this - perhaps an example in UIExplorer? |
|
It's a good idea to include a test plan even if you don't have unit tests so that the next person who works on this code can figure out how to test that they didn't break something. |
|
Let me take a look on the existing test cases for RCTNetworking and Websocket and then I can add one for this change. |
|
@nicklockwood would you mind taking a look at this pull request? It's been a while since the last commit was reviewed. |
|
Thanks for reminding Nick. I have the NSURLSession based changes ready and tested in my local build. I will update the pull request today. |
|
The websocket code already sends cookies (see https://github.com/facebook/react-native/blob/master/Libraries/WebSocket/RCTSRWebSocket.m#L308). Does that not work? It seems the code in this PR's version of |
|
Ah. I see. The code I linked on master does not currently get called, so sending cookies on master is likely broken. Commit 205b5d4 changed the code in I would like to see this PR remove the unused |
Summary:
1. Using weak container to hold the currently opened alerts.
2. Using weak reference to alertController in action handler block.
3. BTW, remove the unused vars: _alertCallbacks, _alertButtonKeys.
Test plan (required)
```
- (void)invalidate
{
for (UIAlertController *alertController in _alertControllers) {
[alertController.presentingViewController dismissViewControllerAnimated:YES completion:nil];
}
}
```
Since we use weak container, _alertControllers should only contains the currently opened alerts.
I test this way: Put a breakpoint in invalidate, open the UIExplorer play with the 'Alert' & 'AlertIOS' examples, then fire a reload and see if _alertControllers contains the expected values.
Closes #10407
Differential Revision: D4078649
Pulled By: lacker
fbshipit-source-id: 8509e7e7142379a81d5b28c9067c085bad8bb5cb
Summary: Small contribution for MacOS users: For those users who using zsh with their Mac OS filename to place variables should be different Closes #10432 Differential Revision: D4078102 Pulled By: lacker fbshipit-source-id: 6cbfb81a472f37bfda85964e929c99b438348fd8
Summary: Update integrating with existing Swift App code for Swift 3. Closes #10547 Differential Revision: D4079631 Pulled By: lacker fbshipit-source-id: 7a493ca7131a10a2466816b104b9cb4f1ff369f5
Summary: Explain the **motivation** for making this change. What existing problem does the pull request solve? See #10506. A native `NSError` with `NSUnderlyingErrorKey` set causes a JSON stringify error from the websocket dispatcher if remote debugging is enabled. **Test plan (required)** I'm not familiar with the react native testing framework. Happy to add a test for this if someone can point me to where this part of the codebase is exercised :) I did some spot checks with nil user dictionaries and nil underlying errors here. The case that this solves is testable using https://github.com/superseriouscompany/react-native-error-repro, specifically: ```objective-c NSError *underlyingError = [NSError errorWithDomain:@"underlyingDomain" code:421 userInfo:nil]; NSError *err = [NSError errorWithDomain:@"domain" code:68 userInfo:@{@"NSUnderlyingError": underlyingError}]; reject(@"foo", @"bar", err); ``` Closes #10507 Differential Revision: D4080802 Pulled By: lacker fbshipit-source-id: 93a41d9e9a710e406a6ccac214a5617271b4bede
…position was set Differential Revision: D4080909 fbshipit-source-id: 7eb1885c615191055aa21e3435c6fbc652b883ae
Summary: * Motivation Locally I'm seeing consistent failures in IntegrationTests/TimersTest.js. Adding a little delay before starting the first test seems to make these issues go away. Closes #10548 Differential Revision: D4080920 Pulled By: bestander fbshipit-source-id: 2dec86073786658f24b809284123815e77fbbd99
Summary: **Motivation** I'm working on a project that uses React Native and needs to add direct synchronous bindings to native stuff through the JavaScriptCore C API. This is because it's performance-sensitive and would benefit from the quickest JS->C path. It does this using cross-platform C++ code that works on both iOS and Android. Most of the infrastructure for getting access to the JSC context is already in React Native actually, just had to add a few more things. (lexs you mentioned to tag you in this pull request) **Test plan** Modify the JavaScriptCore context through the `JSContextRef` returned (eg. add an object at global scope) and verify that it exists in JavaScript. Closes #10399 Differential Revision: D4080945 Pulled By: lexs fbshipit-source-id: 6659b7a01e09fd84475adde183c1d3aca2d4cf09
Differential Revision: D4081049 fbshipit-source-id: 0b9ad70339ad906ad5219ad2679329cfe2fd7abc
Reviewed By: javache Differential Revision: D4075622 fbshipit-source-id: 4a6b6c4068d762dce2b1535bfd9630e185f5489f
Summary: Simple documentation errata. Closes #10098 Differential Revision: D4082724 Pulled By: lacker fbshipit-source-id: 5b4df66472b37b643ced62fafefe461c0c2c86c7
Summary: Currently there is a typo in Accessibility.md which will result in an invalid prop type warning if directly adhered to. The instance of `no-hide-descendant` should instead be updated to `no-hide-descendants` (plural) in the documentation. Closes #10566 Differential Revision: D4082750 Pulled By: lacker fbshipit-source-id: 18e2d9db6004767903e9308a2c0a900d2d9055fc
Reviewed By: emilsjolander Differential Revision: D4081779 fbshipit-source-id: 4e2a1780738423396111c7cfde2fe12010ad0e93
Reviewed By: emilsjolander Differential Revision: D4081788 fbshipit-source-id: 91da6af0fff1317ff4661572e8e7c1cc5594f810
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
…d explicit cookie setting
Reviewed By: mmmulani Differential Revision: D3690133 fbshipit-source-id: a1769016a05b3163a3b1c93f448864aeaecc6e46
|
This pull request got messed up so I created another one. |
web socket connect doesn’t retrieve and set HTTP cookies automatically. Manual approach is to retrieve the cookie through a cookie manager plugin and pass it as headers to web socket. This may cause problems in integrating some library which abstract out web socket transport.