Properly follow the spec in WebSocket::Constructor

components/net/fetch/methods.rs

@@ -617,7 +617,7 @@ fn should_block_nosniff(request: &Request, response: &Response) -> bool {
 }
 
 /// https://fetch.spec.whatwg.org/#block-bad-port
-fn should_be_blocked_due_to_bad_port(url: &ServoUrl) -> bool {
+pub fn should_be_blocked_due_to_bad_port(url: &ServoUrl) -> bool {
     // Step 1 is not applicable, this function just takes the URL directly.
 
     // Step 2.

components/net/websocket_loader.rs

@@ -2,11 +2,13 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+use cookie::Cookie;
 use cookie_storage::CookieStorage;
+use fetch::methods::should_be_blocked_due_to_bad_port;
 use http_loader;
-use hyper::header::Host;
+use hyper::header::{Host, SetCookie};
-use net_traits::{WebSocketCommunicate, WebSocketConnectData, WebSocketDomAction, WebSocketNetworkEvent};
+use net_traits::{CookieSource, MessageData, WebSocketCommunicate};
-use net_traits::MessageData;
+use net_traits::{WebSocketConnectData, WebSocketDomAction, WebSocketNetworkEvent};
 use net_traits::hosts::replace_hosts;
 use net_traits::unwrap_websocket_protocol;
 use servo_url::ServoUrl;
@@ -23,72 +25,90 @@ use websocket::sender::Sender;
 use websocket::stream::WebSocketStream;
 use websocket::ws::receiver::Receiver as WSReceiver;
 use websocket::ws::sender::Sender as Sender_Object;
-use websocket::ws::util::url::parse_url;
 
-/// *Establish a WebSocket Connection* as defined in RFC 6455.
+// https://fetch.spec.whatwg.org/#concept-websocket-establish
-fn establish_a_websocket_connection(resource_url: &ServoUrl, net_url: (Host, String, bool),
+fn establish_a_websocket_connection(resource_url: &ServoUrl,
-                                    origin: String, protocols: Vec<String>,
+                                    origin: String,
+                                    protocols: Vec<String>,
                                     cookie_jar: Arc<RwLock<CookieStorage>>)
-    -> WebSocketResult<(Headers, Sender<WebSocketStream>, Receiver<WebSocketStream>)> {
+                                    -> WebSocketResult<(Headers, Sender<WebSocketStream>, Receiver<WebSocketStream>)> {
-    let host = Host {
+    // Steps 1-2 are not really applicable here, given we don't exactly go
-        hostname: resource_url.host_str().unwrap().to_owned(),
+    // through the same infrastructure as the Fetch spec.
-        port: resource_url.port_or_known_default(),
-    };
 
-    let mut request = try!(Client::connect(net_url));
+    if should_be_blocked_due_to_bad_port(resource_url) {
-    request.headers.set(Origin(origin));
+        // Subset of steps 11-12, we inline the bad port check here from the
-    request.headers.set(host);
+        // main fetch algorithm for the same reason steps 1-2 are not
+        // applicable.
+        return Err(WebSocketError::RequestError("Request should be blocked due to bad port."));
+    }
+
+    // Steps 3-7.
+    let net_url = replace_hosts(resource_url);
+    let mut request = try!(Client::connect(net_url.as_url()));
+
+    // Client::connect sets the Host header to the host of the URL that is
+    // passed to it, so we need to reset it afterwards to the correct one.
+    request.headers.set(Host {
+        hostname: resource_url.host_str().unwrap().to_owned(),
+        port: resource_url.port(),
+    });
+
+    // Step 8.
     if !protocols.is_empty() {
         request.headers.set(WebSocketProtocol(protocols.clone()));
-    };
+    }
 
+    // Steps 9-10.
+    // TODO: support for permessage-deflate extension.
+
+    // Subset of step 11.
+    // See step 2 of https://fetch.spec.whatwg.org/#concept-fetch.
+    request.headers.set(Origin(origin));
+
+    // Transitive subset of step 11.
+    // See step 17.1 of https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch.
     http_loader::set_request_cookies(&resource_url, &mut request.headers, &cookie_jar);
 
+    // Step 11, somewhat.
     let response = try!(request.send());
+
+    // Step 12, 14.
     try!(response.validate());
 
-    {
+    // Step 13 and transitive subset of step 14.
-       let protocol_in_use = unwrap_websocket_protocol(response.protocol());
+    // See step 6 of http://tools.ietf.org/html/rfc6455#section-4.1.
-        if let Some(protocol_name) = protocol_in_use {
+    if let Some(protocol_name) = unwrap_websocket_protocol(response.protocol()) {
-                if !protocols.is_empty() && !protocols.iter().any(|p| (&**p).eq_ignore_ascii_case(protocol_name)) {
+            if !protocols.is_empty() && !protocols.iter().any(|p| (&**p).eq_ignore_ascii_case(protocol_name)) {
-                    return Err(WebSocketError::ProtocolError("Protocol in Use not in client-supplied protocol list"));
+                return Err(WebSocketError::ProtocolError("Protocol in Use not in client-supplied protocol list"));
-            };
         };
+    };
+
+    // Transitive subset of step 11.
+    // See step 15 of https://fetch.spec.whatwg.org/#http-network-fetch.
+    if let Some(cookies) = response.headers.get::<SetCookie>() {
+        let mut jar = cookie_jar.write().unwrap();
+        for cookie in &**cookies {
+            if let Some(cookie) = Cookie::new_wrapped(cookie.clone(), resource_url, CookieSource::HTTP) {
+                jar.push(cookie, resource_url, CookieSource::HTTP);
+            }
+        }
     }
 
     let headers = response.headers.clone();
     let (sender, receiver) = response.begin().split();
     Ok((headers, sender, receiver))
-
 }
 
 pub fn init(connect: WebSocketCommunicate, connect_data: WebSocketConnectData, cookie_jar: Arc<RwLock<CookieStorage>>) {
     thread::Builder::new().name(format!("WebSocket connection to {}", connect_data.resource_url)).spawn(move || {
-        // Step 8: Protocols.
-
-        // Step 9.
-
-        // URL that we actually fetch from the network, after applying the replacements
-        // specified in the hosts file.
-        let net_url_result = parse_url(replace_hosts(&connect_data.resource_url).as_url());
-        let net_url = match net_url_result {
-            Ok(net_url) => net_url,
-            Err(e) => {
-                debug!("Failed to establish a WebSocket connection: {:?}", e);
-                let _ = connect.event_sender.send(WebSocketNetworkEvent::Fail);
-                return;
-            }
-        };
         let channel = establish_a_websocket_connection(&connect_data.resource_url,
-                                                       net_url,
                                                        connect_data.origin,
-                                                       connect_data.protocols.clone(),
+                                                       connect_data.protocols,
                                                        cookie_jar);
-        let (_, ws_sender, mut receiver) = match channel {
+        let (ws_sender, mut receiver) = match channel {
-            Ok(channel) => {
+            Ok((headers, sender, receiver)) => {
-                let _ = connect.event_sender.send(WebSocketNetworkEvent::ConnectionEstablished(channel.0.clone(),
+                let _ = connect.event_sender.send(WebSocketNetworkEvent::ConnectionEstablished(headers));
-                                                                                               connect_data.protocols));
+                (sender, receiver)
-                channel
             },
             Err(e) => {
                 debug!("Failed to establish a WebSocket connection: {:?}", e);

components/net_traits/lib.rs

@@ -350,8 +350,7 @@ pub enum WebSocketDomAction {
 pub enum WebSocketNetworkEvent {
     ConnectionEstablished(#[serde(deserialize_with = "::hyper_serde::deserialize",
                                   serialize_with = "::hyper_serde::serialize")]
-                          header::Headers,
+                          header::Headers),
-                          Vec<String>),
     MessageReceived(MessageData),
     Close(Option<u16>, String),
     Fail,

components/script/dom/websocket.rs

@@ -23,17 +23,13 @@ use dom::globalscope::GlobalScope;
 use dom::messageevent::MessageEvent;
 use dom::urlhelper::UrlHelper;
 use dom_struct::dom_struct;
-use hyper;
-use hyper_serde::Serde;
 use ipc_channel::ipc::{self, IpcReceiver, IpcSender};
 use js::jsapi::JSAutoCompartment;
 use js::jsval::UndefinedValue;
 use js::typedarray::{ArrayBuffer, CreateWith};
 use net_traits::{WebSocketCommunicate, WebSocketConnectData, WebSocketDomAction, WebSocketNetworkEvent};
-use net_traits::CookieSource::HTTP;
+use net_traits::CoreResourceMsg::WebsocketConnect;
-use net_traits::CoreResourceMsg::{SetCookiesForUrl, WebsocketConnect};
 use net_traits::MessageData;
-use net_traits::hosts::replace_hosts;
 use net_traits::unwrap_websocket_protocol;
 use script_runtime::CommonScriptMsg;
 use script_runtime::ScriptThreadEventCategory::WebSocketEvent;
@@ -47,7 +43,6 @@ use std::thread;
 use task_source::TaskSource;
 use task_source::networking::NetworkingTaskSource;
 use websocket::header::{Headers, WebSocketProtocol};
-use websocket::ws::util::url::parse_url;
 
 #[derive(JSTraceable, PartialEq, Copy, Clone, Debug, HeapSizeOf)]
 enum WebSocketRequestState {
@@ -57,75 +52,6 @@ enum WebSocketRequestState {
     Closed = 3,
 }
 
-// list of bad ports according to
-// https://fetch.spec.whatwg.org/#port-blocking
-const BLOCKED_PORTS_LIST: &'static [u16] = &[
-    1,    // tcpmux
-    7,    // echo
-    9,    // discard
-    11,   // systat
-    13,   // daytime
-    15,   // netstat
-    17,   // qotd
-    19,   // chargen
-    20,   // ftp-data
-    21,   // ftp
-    22,   // ssh
-    23,   // telnet
-    25,   // smtp
-    37,   // time
-    42,   // name
-    43,   // nicname
-    53,   // domain
-    77,   // priv-rjs
-    79,   // finger
-    87,   // ttylink
-    95,   // supdup
-    101,  // hostriame
-    102,  // iso-tsap
-    103,  // gppitnp
-    104,  // acr-nema
-    109,  // pop2
-    110,  // pop3
-    111,  // sunrpc
-    113,  // auth
-    115,  // sftp
-    117,  // uucp-path
-    119,  // nntp
-    123,  // ntp
-    135,  // loc-srv / epmap
-    139,  // netbios
-    143,  // imap2
-    179,  // bgp
-    389,  // ldap
-    465,  // smtp+ssl
-    512,  // print / exec
-    513,  // login
-    514,  // shell
-    515,  // printer
-    526,  // tempo
-    530,  // courier
-    531,  // chat
-    532,  // netnews
-    540,  // uucp
-    556,  // remotefs
-    563,  // nntp+ssl
-    587,  // smtp
-    601,  // syslog-conn
-    636,  // ldap+ssl
-    993,  // imap+ssl
-    995,  // pop3+ssl
-    2049, // nfs
-    3659, // apple-sasl
-    4045, // lockd
-    6000, // x11
-    6665, // irc (alternate)
-    6666, // irc (alternate)
-    6667, // irc (default)
-    6668, // irc (alternate)
-    6669, // irc (alternate)
-];
-
 // Close codes defined in https://tools.ietf.org/html/rfc6455#section-7.4.1
 // Names are from https://github.com/mozilla/gecko-dev/blob/master/netwerk/protocol/websocket/nsIWebSocketChannel.idl
 #[allow(dead_code)]
@@ -202,34 +128,36 @@ impl WebSocket {
                            global, WebSocketBinding::Wrap)
     }
 
+    /// https://html.spec.whatwg.org/multipage/#dom-websocket
     pub fn Constructor(global: &GlobalScope,
                        url: DOMString,
                        protocols: Option<StringOrStringSequence>)
                        -> Fallible<Root<WebSocket>> {
-        // Step 1.
+        // Steps 1-2.
-        let resource_url = try!(ServoUrl::parse(&url).map_err(|_| Error::Syntax));
+        let url_record = ServoUrl::parse(&url).or(Err(Error::Syntax))?;
-        // Although we do this replace and parse operation again in the resource thread,
-        // we try here to be able to immediately throw a syntax error on failure.
-        let _ = try!(parse_url(&replace_hosts(&resource_url).as_url()).map_err(|_| Error::Syntax));
-        // Step 2: Disallow https -> ws connections.
 
-        // Step 3: Potentially block access to some ports.
+        // Step 3.
-        let port: u16 = resource_url.port_or_known_default().unwrap();
+        match url_record.scheme() {
-
+            "ws" | "wss" => {},
-        if BLOCKED_PORTS_LIST.iter().any(|&p| p == port) {
+            _ => return Err(Error::Syntax),
-            return Err(Error::Security);
         }
 
         // Step 4.
-        let protocols = match protocols {
+        if url_record.fragment().is_some() {
-            Some(StringOrStringSequence::String(string)) => vec![String::from(string)],
+            return Err(Error::Syntax);
-            Some(StringOrStringSequence::StringSequence(sequence)) => {
+        }
-                sequence.into_iter().map(String::from).collect()
-            },
-            _ => Vec::new(),
-        };
 
         // Step 5.
+        let protocols = protocols.map_or(vec![], |p| {
+            match p {
+                StringOrStringSequence::String(string) => vec![string.into()],
+                StringOrStringSequence::StringSequence(seq) => {
+                    seq.into_iter().map(String::from).collect()
+                },
+            }
+        });
+
+        // Step 6.
         for (i, protocol) in protocols.iter().enumerate() {
             // https://tools.ietf.org/html/rfc6455#section-4.1
             // Handshake requirements, step 10
@@ -244,16 +172,12 @@ impl WebSocket {
             }
         }
 
-        // Step 6: Origin.
+        let ws = WebSocket::new(global, url_record.clone());
-        let origin = UrlHelper::Origin(&global.get_url()).0;
-
-        // Step 7.
-        let ws = WebSocket::new(global, resource_url.clone());
         let address = Trusted::new(&*ws);
 
         let connect_data = WebSocketConnectData {
-            resource_url: resource_url.clone(),
+            resource_url: url_record,
-            origin: origin,
+            origin: UrlHelper::Origin(&global.get_url()).0,
             protocols: protocols,
         };
 
@@ -270,6 +194,7 @@ impl WebSocket {
             action_receiver: resource_action_receiver,
         };
 
+        // Step 8.
         let _ = global.core_resource_thread().send(WebsocketConnect(connect, connect_data));
 
         *ws.sender.borrow_mut() = Some(dom_action_sender);
@@ -279,11 +204,10 @@ impl WebSocket {
         thread::spawn(move || {
             while let Ok(event) = dom_event_receiver.recv() {
                 match event {
-                    WebSocketNetworkEvent::ConnectionEstablished(headers, protocols) => {
+                    WebSocketNetworkEvent::ConnectionEstablished(headers) => {
                         let open_thread = box ConnectionEstablishedTask {
                             address: address.clone(),
                             headers: headers,
-                            protocols: protocols,
                         };
                         task_source.queue_with_wrapper(open_thread, &wrapper).unwrap();
                     },
@@ -466,45 +390,31 @@ impl WebSocketMethods for WebSocket {
 
 
 /// Task queued when *the WebSocket connection is established*.
+/// https://html.spec.whatwg.org/multipage/#feedback-from-the-protocol:concept-websocket-established
 struct ConnectionEstablishedTask {
     address: Trusted<WebSocket>,
-    protocols: Vec<String>,
     headers: Headers,
 }
 
 impl Runnable for ConnectionEstablishedTask {
     fn name(&self) -> &'static str { "ConnectionEstablishedTask" }
 
+    /// https://html.spec.whatwg.org/multipage/#feedback-from-the-protocol:concept-websocket-established
     fn handler(self: Box<Self>) {
         let ws = self.address.root();
 
-        // Step 1: Protocols.
+        // Step 1.
-        if !self.protocols.is_empty() && self.headers.get::<WebSocketProtocol>().is_none() {
-            let task_source = ws.global().networking_task_source();
-            fail_the_websocket_connection(self.address, &task_source, &ws.global().get_runnable_wrapper());
-            return;
-        }
-
-        // Step 2.
         ws.ready_state.set(WebSocketRequestState::Open);
 
-        // Step 3: Extensions.
+        // Step 2: Extensions.
-        //TODO: Set extensions to extensions in use
+        // TODO: Set extensions to extensions in use.
 
-        // Step 4: Protocols.
+        // Step 3.
-        let protocol_in_use = unwrap_websocket_protocol(self.headers.get::<WebSocketProtocol>());
+        if let Some(protocol_name) = unwrap_websocket_protocol(self.headers.get::<WebSocketProtocol>()) {
-        if let Some(protocol_name) = protocol_in_use {
             *ws.protocol.borrow_mut() = protocol_name.to_owned();
         };
 
-        // Step 5: Cookies.
+        // Step 4.
-        if let Some(cookies) = self.headers.get::<hyper::header::SetCookie>() {
-            let cookies = cookies.iter().map(|c| Serde(c.clone())).collect();
-            let _ = ws.global().core_resource_thread().send(
-                SetCookiesForUrl(ws.url.clone(), cookies, HTTP));
-        }
-
-        // Step 6.
         ws.upcast().fire_event(atom!("open"));
     }
 }

tests/wpt/metadata/websockets/Create-Secure-blocked-port.htm.ini

@@ -0,0 +1,6 @@
+[Create-Secure-blocked-port.htm]
+  type: testharness
+  [W3C WebSocket API - Create Secure WebSocket - Pass a URL with a blocked port - SECURITY_ERR should be thrown]
+    expected: FAIL
+    bug: https://github.com/w3c/web-platform-tests/pull/5212
+

tests/wpt/metadata/websockets/cookies/007.html.ini

@@ -1,8 +1,13 @@
 [007.html]
   type: testharness
+  [WebSockets: when to process set-cookie fields in ws response]
+    expected: FAIL
+    bug: https://github.com/w3c/web-platform-tests/issues/5213
+
 
 [007.html?wss]
   type: testharness
   [WebSockets: when to process set-cookie fields in ws response]
     expected: FAIL
+    bug: https://github.com/w3c/web-platform-tests/issues/5213