Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Libraries/WebSocket/RCTSRWebSocket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@christopherdro christopherdro Feb 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

- (instancetype)initWithURLRequest:(NSURLRequest *)request;

// Some helper constructors.
- (instancetype)initWithURL:(NSURL *)url protocols:(NSArray<NSString *> *)protocols options:(NSDictionary<NSString *, NSString *> *)options;
- (instancetype)initWithURL:(NSURL *)url protocols:(NSArray<NSString *> *)protocols headers:(NSDictionary<NSString *, NSString *> *)headers;
- (instancetype)initWithURL:(NSURL *)url protocols:(NSArray<NSString *> *)protocols;
- (instancetype)initWithURL:(NSURL *)url;

Expand Down
33 changes: 24 additions & 9 deletions Libraries/WebSocket/RCTSRWebSocket.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ @implementation RCTSRWebSocket
__strong RCTSRWebSocket *_selfRetain;

NSArray<NSString *> *_requestedProtocols;
NSDictionary<NSString *, NSString *> *_requestedOptions;
NSDictionary<NSString *, NSString *> *_requestedHeaders;
RCTSRIOConsumerPool *_consumerPool;
}

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. 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]);

  2. 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.

{
RCTAssertParam(request);

Expand All @@ -254,7 +254,7 @@ - (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NS
_urlRequest = request;

_requestedProtocols = [protocols copy];
_requestedOptions = [options copy];
_requestedHeaders= [headers copy];

[self _RCTSR_commonInit];
}
Expand All @@ -265,20 +265,20 @@ - (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NS

- (instancetype)initWithURLRequest:(NSURLRequest *)request;
{
return [self initWithURLRequest:request protocols:nil options: nil];
return [self initWithURLRequest:request protocols:nil headers: nil];
}

- (instancetype)initWithURL:(NSURL *)URL;
{
return [self initWithURL:URL protocols:nil options:nil];
return [self initWithURL:URL protocols:nil headers:nil];
}

- (instancetype)initWithURL:(NSURL *)URL protocols:(NSArray<NSString *> *)protocols;
{
return [self initWithURL:URL protocols:protocols options:nil];
return [self initWithURL:URL protocols:protocols headers:nil];
}

- (instancetype)initWithURL:(NSURL *)URL protocols:(NSArray<NSString *> *)protocols options:(NSDictionary<NSString *, id> *)options
- (instancetype)initWithURL:(NSURL *)URL protocols:(NSArray<NSString *> *)protocols headers:(NSDictionary<NSString *, id> *)headers
{
NSMutableURLRequest *request;
if (URL) {
Expand All @@ -297,7 +297,7 @@ - (instancetype)initWithURL:(NSURL *)URL protocols:(NSArray<NSString *> *)protoc
NSArray<NSHTTPCookie *> *cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookiesForURL:components.URL];
[request setAllHTTPHeaderFields:[NSHTTPCookie requestHeaderFieldsWithCookies:cookies]];
}
return [self initWithURLRequest:request protocols:protocols options:options];
return [self initWithURLRequest:request protocols:protocols headers:headers];
}

- (void)_RCTSR_commonInit;
Expand Down Expand Up @@ -492,7 +492,22 @@ - (void)didConnect
CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Sec-WebSocket-Protocol"), (__bridge CFStringRef)[_requestedProtocols componentsJoinedByString:@", "]);
}

CFHTTPMessageSetHeaderFieldValue(request, CFSTR("Origin"), (__bridge CFStringRef)(_requestedOptions[@"origin"] ?: _url.RCTSR_origin));
if (_requestedHeaders) {

if (!_requestedHeaders[@"origin"]) {
CFHTTPMessageSetHeaderFieldValue(request, CFSTR("origin"), (__bridge CFStringRef)(_url.RCTSR_origin));
}

[_requestedHeaders enumerateKeysAndObjectsUsingBlock:^(id key, id obj, BOOL *stop) {
if ([obj description]) {
CFHTTPMessageSetHeaderFieldValue(request, (__bridge CFStringRef)key, (__bridge CFStringRef)obj);
} else {
RCTSRFastLog(@"Ignoring passed in header: %@, value not a string.", key);
}
}];
} else {
CFHTTPMessageSetHeaderFieldValue(request, CFSTR("origin"), (__bridge CFStringRef)(_url.RCTSR_origin));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @nicklockwood As my Obj-C fu is not great :)


[_urlRequest.allHTTPHeaderFields enumerateKeysAndObjectsUsingBlock:^(id key, id obj, BOOL *stop) {
CFHTTPMessageSetHeaderFieldValue(request, (__bridge CFStringRef)key, (__bridge CFStringRef)obj);
Expand Down
4 changes: 2 additions & 2 deletions Libraries/WebSocket/RCTWebSocketModule.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ - (void)dealloc
}
}

RCT_EXPORT_METHOD(connect:(NSURL *)URL protocols:(NSArray *)protocols options:(NSDictionary *)options socketID:(nonnull NSNumber *)socketID)
RCT_EXPORT_METHOD(connect:(NSURL *)URL protocols:(NSArray *)protocols headers:(NSDictionary *)headers socketID:(nonnull NSNumber *)socketID)
{
RCTSRWebSocket *webSocket = [[RCTSRWebSocket alloc] initWithURL:URL protocols:protocols options:options];
RCTSRWebSocket *webSocket = [[RCTSRWebSocket alloc] initWithURL:URL protocols:protocols headers:headers];
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just merge the url and headers into an NSURLRequest at this point, and use the other constructor.

NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:URL];
request.allHTTPHeaderFields = headers;

webSocket.delegate = self;
webSocket.reactTag = socketID;
if (!_sockets) {
Expand Down
4 changes: 2 additions & 2 deletions Libraries/WebSocket/WebSocket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ class WebSocket extends WebSocketBase {
_socketId: number;
_subs: any;

connectToSocketImpl(url: string, protocols: ?Array<string>, options: ?{origin?: string}): void {
connectToSocketImpl(url: string, protocols: ?Array<string>, headers: ?Object): void {
this._socketId = WebSocketId++;

RCTWebSocketModule.connect(url, protocols, options, this._socketId);
RCTWebSocketModule.connect(url, protocols, headers, this._socketId);

this._registerEvents(this._socketId);
}
Expand Down
62 changes: 54 additions & 8 deletions ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
import com.squareup.okhttp.ws.WebSocketCall;
import com.squareup.okhttp.ws.WebSocketListener;

import java.net.URISyntaxException;
import java.net.URI;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -63,7 +65,7 @@ public String getName() {
}

@ReactMethod
public void connect(final String url, @Nullable final ReadableArray protocols, @Nullable final ReadableMap options, final int id) {
public void connect(final String url, @Nullable final ReadableArray protocols, @Nullable final ReadableMap headers, final int id) {
// ignoring protocols, since OKHttp overrides them.
OkHttpClient client = new OkHttpClient();

Expand All @@ -76,14 +78,25 @@ public void connect(final String url, @Nullable final ReadableArray protocols, @
.tag(id)
.url(url);

if (options != null && options.hasKey("origin")) {
if (ReadableType.String.equals(options.getType("origin"))) {
builder.addHeader("Origin", options.getString("origin"));
} else {
FLog.w(
ReactConstants.TAG,
"Ignoring: requested origin, value not a string");
if (headers != null) {
ReadableMapKeySetIterator iterator = headers.keySetIterator();

if (!headers.hasKey("origin")) {
builder.addHeader("origin", setDefaultOrigin(url));
Copy link
Contributor

Choose a reason for hiding this comment

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

n00b question: should this be "Origin" via the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, http headers are case-insensitive and get lowercased automatically when being sent down with the request.

}

while (iterator.hasNextKey()) {
String key = iterator.nextKey();
if (ReadableType.String.equals(headers.getType(key))) {
builder.addHeader(key, headers.getString(key));
} else {
FLog.w(
ReactConstants.TAG,
"Ignoring: requested " + key + ", value not a string");
}
}
} else {
builder.addHeader("origin", setDefaultOrigin(url));
}

WebSocketCall.create(client, builder.build()).enqueue(new WebSocketListener() {
Expand Down Expand Up @@ -188,4 +201,37 @@ private void notifyWebSocketFailed(int id, String message) {
params.putString("message", message);
sendEvent("websocketFailed", params);
}

/**
* Set a default origin
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain what setting a default origin means?

*
* @param Websocket connection endpoint
* @return A string of the endpoint converted to HTTP protocol
*/

private static String setDefaultOrigin(String uri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot tell what this does based on the name. Can you add short Javadoc please?

try {
String defaultOrigin;
String scheme = "";

URI requestURI = new URI(uri);
if (requestURI.getScheme().equals("wss")) {
scheme += "https";
} else if (requestURI.getScheme().equals("ws")) {
scheme += "http";
}

if (requestURI.getPort() != -1) {
defaultOrigin = String.format("%s://%s:%s", scheme, requestURI.getHost(), requestURI.getPort());
} else {
defaultOrigin = String.format("%s://%s/", scheme, requestURI.getHost());
}

return defaultOrigin;

} catch(URISyntaxException e) {
throw new IllegalArgumentException("Unable to set " + uri + " as default origin header.");
}
}

}