[Websocket] Update options parameter to headers. Update to spec. #6016

christopherdro · 2016-02-18T23:21:19Z

This is a follow up of 9b87e6c.

PR Overview:

Allows custom headers on connection request
Adds a default origin header to Android, just like iOS

Introduces no breaking changes.

PR Detail:

I was working on something similar and would like to propose a few changes that make the API more consistent across both iOS and Android platforms and brings this closer to spec.

I believe @aprock first implementation of adding custom headers was correct. It makes sense naming this argument headers since we have no other general options available, and the current options field is being used to pass in a header anyway.

My use case for custom headers was attaching a token to the Authorization header on the connection request. I have been testing this by passing a JWT inside the Authorization header and verifying it on the server before establishing a connection.

I tried to take this this abstraction further and read the response headers from the server to see if a new token was available if the connection was denied from a expired token. Unfortunately, this was only possible with Android since the third party library used by iOS (SquareRocket) does not return the server response on connection failures.

facebook-github-bot · 2016-02-18T23:22:20Z

By analyzing the blame information on this pull request, we identified @aprock, @hharnisc and @satya164 to be potential reviewers.

christopherdro · 2016-02-18T23:30:27Z

ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java

+      return defaultOrigin;
+
+    } catch(URISyntaxException e) {
+      // Handle error


@mkonicek @satya164 Not sure the best way to handle the error here. Ideas?

@christopherdro Since this is a user error, we should throw the error to show a redbox (IllegalArgumentException). This matches the behaviour on the web where it throws an error.

facebook-github-bot · 2016-02-19T18:05:20Z

@christopherdro updated the pull request.

christopherdro · 2016-02-19T18:28:40Z

@satya164 I updated the PR to handle the error message. However, it appears that passing a invalid url scheme will be caught beforehand through Request.Builder

mkonicek · 2016-02-21T23:19:41Z

Libraries/WebSocket/RCTSRWebSocket.h

@@ -61,11 +61,11 @@ extern NSString *const RCTSRHTTPResponseErrorKey;

 // Protocols should be an array of strings that turn into Sec-WebSocket-Protocol.
 // options can contain a custom "origin" NSString
- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NSString *> *)protocols options:(NSDictionary<NSString *, NSString *> *)options NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NSString *> *)protocols headers:(NSDictionary<NSString *, NSString *> *)headers NS_DESIGNATED_INITIALIZER;


Ah this used to be called options but it's really HTTP headers?

Yea, these were recently merged via 9b87e6c

I noticed that his original implementation did have an option for using headers but was asked to remove it because only the origin header was required by the contributor.

facebook-github-bot · 2016-02-23T10:11:54Z

@christopherdro updated the pull request.

facebook-github-bot · 2016-02-25T03:26:19Z

@christopherdro updated the pull request.

nicklockwood · 2016-02-25T09:17:15Z

Libraries/WebSocket/RCTSRWebSocket.m

@@ -245,7 +245,7 @@ + (void)initialize;
  CRLFCRLF = [[NSData alloc] initWithBytes:"\r\n\r\n" length:4];
 }

- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NSString *> *)protocols options:(NSDictionary<NSString *, NSString *> *)options
+- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NSString *> *)protocols headers:(NSDictionary<NSString *, NSString *> *)headers


Since NSURLRequest already includes headers, is there any reason for the seperate headers parameter? Can't we just include them in the request?

@nicklockwood That makes sense but it appears there are two other places that headers are being set.

react-native/Libraries/WebSocket/RCTSRWebSocket.m

Lines 478 to 489 in debcac5

// Set host first so it defaults

CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Host"), (__bridge CFStringRef)(_url.port ? [NSString stringWithFormat:@"%@:%@", _url.host, _url.port] : _url.host));

NSMutableData *keyBytes = [[NSMutableData alloc] initWithLength:16];

SecRandomCopyBytes(kSecRandomDefault, keyBytes.length, keyBytes.mutableBytes);

_secKey = [keyBytes base64EncodedStringWithOptions:0];

assert([_secKey length] == 24);

CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Upgrade"), CFSTR("websocket"));

CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Connection"), CFSTR("Upgrade"));

CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Sec-WebSocket-Key"), (__bridge CFStringRef)_secKey);

CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Sec-WebSocket-Version"), (__bridge CFStringRef)[NSString stringWithFormat:@"%ld", (long)_webSocketVersion]);

react-native/Libraries/WebSocket/RCTSRWebSocket.m

Line 298 in debcac5

[request setAllHTTPHeaderFields:[NSHTTPCookie requestHeaderFieldsWithCookies:cookies]];

So im not sure the best way to handle this.

mkonicek · 2016-02-26T00:06:45Z

We talked about this briefly at React.js conf but don't remember the conclusion. Regarding the setDefaultOriginMethod - why is the origin header needed in React Native? I thought it was designed for browsers?

christopherdro · 2016-02-26T01:30:56Z

@mkonicek Your are right that it is browser specfic. I was just trying to make things consistent across platform. If it's unnecessary then we should remove it from iOS as well.

@nicklockwood Thoughts on whether we need to have a default origin on iOS?

mkonicek · 2016-02-26T01:32:35Z

Cool, thanks for the explanation! I vote for getting rid of the origin headers if they're not needed on mobile.

facebook-github-bot · 2016-02-26T01:33:16Z

@christopherdro updated the pull request.

christopherdro · 2016-02-26T02:44:10Z

@nicklockwood Updated the PR based on some of your comments and made further notes on ones that I wasn't sure about.

mkonicek · 2016-03-01T19:16:18Z

Is the method setDefaultOrigin needed? Do we need to set an origin header if we're not in a browser (maybe yes, I just don't have the context)?

christopherdro · 2016-03-01T19:21:00Z

Technically no since RN is not a browser client.

The |Origin| header field [RFC6454] is used to protect against
unauthorized cross-origin use of a WebSocket server by scripts using
the WebSocket API in a web browser. The server is informed of the
script origin generating the WebSocket connection request. If the
server does not wish to accept connections from this origin, it can
choose to reject the connection by sending an appropriate HTTP error
code. This header field is sent by browser clients; for non-browser
clients, this header field may be sent if it makes sense in the
context of those clients.

I was waiting to see what @nicklockwood thought about removing the default origin header from iOS.

facebook-github-bot · 2016-03-06T05:33:13Z

@christopherdro updated the pull request.

christopherdro · 2016-03-08T14:44:00Z

@nicklockwood Any chance you can chime in here again so I can go ahead and wrap up this PR? Thanks!

mkonicek · 2016-03-10T20:18:57Z

This PR does two things which makes it a bit harder to review and land:

Allows custom headers on connection request

Adds a default origin header to Android, just like iOS

Do you think you could send the "Allows custom headers on connection request" part separately especially since we're not sure 2. is needed? Then it should be fairly straightforward to merge.

facebook-github-bot · 2016-03-11T04:10:20Z

@christopherdro updated the pull request.

christopherdro · 2016-03-11T04:11:05Z

@mkonicek Fair enough. I'll update the the PR to only update the headers. Should have it done by this weekend and will ping you again.

zxcpoiu · 2016-03-11T06:46:54Z

Hi @christopherdro,

What is your opinion to keep the argument name options, and use 'headers' as a key for setting up additional header ?

*ex: *

options = {
    headers: {}
    otherOptions: ...
}

for future workaround flags or custom actions.

christopherdro · 2016-03-11T07:36:27Z

@zxcpoiu I believe that was my original thought but avoided since it seems that the general attitude of PR's is to implement whats required atm and not over engineer. I will however revisit this shortly.

Can you provide any examples of what some future options might be?

zxcpoiu · 2016-03-11T09:16:33Z

Hi @christopherdro

you're right, not over engineer is a good point.

Currently what I can think of potential use cases are: maybe to tweak options of android OkHttpClient Consturrctor when create ws instance

for example:

read/write Timeout
self-signed certificate verify
force TLS versions

It's not that critical, and I don't want to hold this decent PR.
Just treat it as a note here for reference :)

nicklockwood · 2016-03-11T10:24:02Z

@facebook-github-bot shipit

facebook-github-bot · 2016-03-11T10:24:24Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

facebook-github-bot · 2016-03-14T16:14:07Z

@christopherdro updated the pull request.

Summary:This is a follow up of 9b87e6c. - Allows custom headers on connection request - Adds a default `origin` header to Android, just like iOS **Introduces no breaking changes.** I was working on something similar and would like to propose a few changes that make the API more consistent across both iOS and Android platforms and brings this closer to [spec](https://tools.ietf.org/html/rfc6455). I believe aprock first implementation of adding custom `headers` was correct. It makes sense naming this argument `headers` since we have no other general options available, and the current `options` field is being used to pass in a header anyway. My use case for custom headers was attaching a token to the `Authorization` header on the connection request. I have been testing this by passing a JWT inside the `Authorization` header and verifying it on the server before establishing a connection. Closes #6016 Differential Revision: D3040735 fb-gh-sync-id: 183744d2415b895f9d9fd8ecf6023a546e18a546 shipit-source-id: 183744d2415b895f9d9fd8ecf6023a546e18a546

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 18, 2016

christopherdro force-pushed the ws-update branch from fe9534c to dca5e85 Compare February 18, 2016 23:29

christopherdro reviewed Feb 18, 2016
View reviewed changes

christopherdro force-pushed the ws-update branch from dca5e85 to f22fa6a Compare February 19, 2016 18:04

mkonicek added the Size/L label Feb 21, 2016

mkonicek reviewed Feb 21, 2016
View reviewed changes

christopherdro force-pushed the ws-update branch from f22fa6a to 2be04cb Compare February 25, 2016 03:25

nicklockwood reviewed Feb 25, 2016
View reviewed changes

christopherdro force-pushed the ws-update branch 2 times, most recently from 5fb2301 to 66d7682 Compare February 26, 2016 01:33

christopherdro force-pushed the ws-update branch from 66d7682 to c7450a9 Compare March 6, 2016 05:32

satya164 mentioned this pull request Mar 10, 2016

[WebSocket][Android] make protocols argument work fixes #5810 #6223

Closed

[Websocket] Update options parameter to headers. Update to spec.

59e7194

christopherdro force-pushed the ws-update branch from c7450a9 to 59e7194 Compare March 11, 2016 04:10

facebook-github-bot added GH Review: accepted and removed GH Review: review-needed labels Mar 11, 2016

ghost closed this in 205b5d4 Mar 15, 2016

rozele mentioned this pull request Jan 23, 2024

WebSocketModule missing default Origin header behavior microsoft/react-native-windows#12626

Closed

This pull request was closed.

	// Set host first so it defaults
	CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Host"), (__bridge CFStringRef)(_url.port ? [NSString stringWithFormat:@"%@:%@", _url.host, _url.port] : _url.host));

	NSMutableData *keyBytes = [[NSMutableData alloc] initWithLength:16];
	SecRandomCopyBytes(kSecRandomDefault, keyBytes.length, keyBytes.mutableBytes);
	_secKey = [keyBytes base64EncodedStringWithOptions:0];
	assert([_secKey length] == 24);

	CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Upgrade"), CFSTR("websocket"));
	CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Connection"), CFSTR("Upgrade"));
	CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Sec-WebSocket-Key"), (__bridge CFStringRef)_secKey);
	CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Sec-WebSocket-Version"), (__bridge CFStringRef)[NSString stringWithFormat:@"%ld", (long)_webSocketVersion]);

[Websocket] Update options parameter to headers. Update to spec. #6016

[Websocket] Update options parameter to headers. Update to spec. #6016

Uh oh!

Conversation

christopherdro commented Feb 18, 2016

PR Overview:

PR Detail:

Uh oh!

facebook-github-bot commented Feb 18, 2016

Uh oh!

christopherdro Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

satya164 Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 19, 2016

Uh oh!

christopherdro commented Feb 19, 2016

Uh oh!

mkonicek Feb 21, 2016

Choose a reason for hiding this comment

Uh oh!

christopherdro Feb 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 23, 2016

Uh oh!

facebook-github-bot commented Feb 25, 2016

Uh oh!

nicklockwood Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

christopherdro Feb 26, 2016

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Feb 26, 2016

Uh oh!

christopherdro commented Feb 26, 2016

Uh oh!

mkonicek commented Feb 26, 2016

Uh oh!

facebook-github-bot commented Feb 26, 2016

Uh oh!

christopherdro commented Feb 26, 2016

Uh oh!

mkonicek commented Mar 1, 2016

Uh oh!

christopherdro commented Mar 1, 2016

Uh oh!

facebook-github-bot commented Mar 6, 2016

Uh oh!

christopherdro commented Mar 8, 2016

Uh oh!

mkonicek commented Mar 10, 2016

Uh oh!

facebook-github-bot commented Mar 11, 2016

Uh oh!

christopherdro commented Mar 11, 2016

Uh oh!

zxcpoiu commented Mar 11, 2016

Uh oh!

christopherdro commented Mar 11, 2016

Uh oh!

zxcpoiu commented Mar 11, 2016

Uh oh!

nicklockwood commented Mar 11, 2016

Uh oh!

facebook-github-bot commented Mar 11, 2016

Uh oh!

facebook-github-bot commented Mar 14, 2016

Uh oh!

Uh oh!

christopherdro Feb 21, 2016 •

edited

Loading