WebSocketHandshake should not know about a Document
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Apr 2019 18:49:53 +0000 (18:49 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Apr 2019 18:49:53 +0000 (18:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196468

Reviewed by Tim Horton.

I'll need to move WebSocketHandshake to the NetworkProcess for rdar://problem/46287028
It currently uses the Document pointer for 3 things:
1. To get the user agent, which we can pass in as a creation parameter.
2. To get the origin, which we can also pass in as a creation parameter.
3. To get cookies for the web inspector.  We can pass in a functor instead and have the inspector provide cookies itself.

* Modules/websockets/WebSocketChannel.cpp:
(WebCore::WebSocketChannel::connect):
(WebCore::WebSocketChannel::disconnect):
(WebCore::WebSocketChannel::didOpenSocketStream):
(WebCore::WebSocketChannel::clientHandshakeRequest):
* Modules/websockets/WebSocketChannel.h:
(WebCore::WebSocketChannel::document):
* Modules/websockets/WebSocketHandshake.cpp:
(WebCore::WebSocketHandshake::WebSocketHandshake):
(WebCore::WebSocketHandshake::clientHandshakeMessage const):
(WebCore::WebSocketHandshake::clientHandshakeRequest const):
(WebCore::WebSocketHandshake::clientOrigin const): Deleted.
(WebCore::WebSocketHandshake::clientHandshakeCookieRequestHeaderFieldProxy const): Deleted.
(WebCore::WebSocketHandshake::clearDocument): Deleted.
* Modules/websockets/WebSocketHandshake.h:
* inspector/agents/InspectorNetworkAgent.cpp:
(WebCore::InspectorNetworkAgent::enable):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244391 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/Modules/websockets/WebSocketChannel.cpp
Source/WebCore/Modules/websockets/WebSocketChannel.h
Source/WebCore/Modules/websockets/WebSocketHandshake.cpp
Source/WebCore/Modules/websockets/WebSocketHandshake.h
Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp

index e7af829..a457e8c 100644 (file)
@@ -1,3 +1,34 @@
+2019-04-17  Alex Christensen  <achristensen@webkit.org>
+
+        WebSocketHandshake should not know about a Document
+        https://bugs.webkit.org/show_bug.cgi?id=196468
+
+        Reviewed by Tim Horton.
+
+        I'll need to move WebSocketHandshake to the NetworkProcess for rdar://problem/46287028
+        It currently uses the Document pointer for 3 things:
+        1. To get the user agent, which we can pass in as a creation parameter.
+        2. To get the origin, which we can also pass in as a creation parameter.
+        3. To get cookies for the web inspector.  We can pass in a functor instead and have the inspector provide cookies itself.
+
+        * Modules/websockets/WebSocketChannel.cpp:
+        (WebCore::WebSocketChannel::connect):
+        (WebCore::WebSocketChannel::disconnect):
+        (WebCore::WebSocketChannel::didOpenSocketStream):
+        (WebCore::WebSocketChannel::clientHandshakeRequest):
+        * Modules/websockets/WebSocketChannel.h:
+        (WebCore::WebSocketChannel::document):
+        * Modules/websockets/WebSocketHandshake.cpp:
+        (WebCore::WebSocketHandshake::WebSocketHandshake):
+        (WebCore::WebSocketHandshake::clientHandshakeMessage const):
+        (WebCore::WebSocketHandshake::clientHandshakeRequest const):
+        (WebCore::WebSocketHandshake::clientOrigin const): Deleted.
+        (WebCore::WebSocketHandshake::clientHandshakeCookieRequestHeaderFieldProxy const): Deleted.
+        (WebCore::WebSocketHandshake::clearDocument): Deleted.
+        * Modules/websockets/WebSocketHandshake.h:
+        * inspector/agents/InspectorNetworkAgent.cpp:
+        (WebCore::InspectorNetworkAgent::enable):
+
 2019-04-17  Timothy Hatcher  <timothy@apple.com>
 
         Unreviewed build fix for iOSMac after r244223.
index c432dd4..176e585 100644 (file)
@@ -86,7 +86,6 @@ void WebSocketChannel::connect(const URL& requestedURL, const String& protocol)
     LOG(Network, "WebSocketChannel %p connect()", this);
 
     URL url = requestedURL;
-    bool allowCookies = true;
 #if ENABLE(CONTENT_EXTENSIONS)
     if (auto* page = m_document->page()) {
         if (auto* documentLoader = m_document->loader()) {
@@ -106,14 +105,17 @@ void WebSocketChannel::connect(const URL& requestedURL, const String& protocol)
                     m_client->didUpgradeURL();
             }
             if (results.summary.blockedCookies)
-                allowCookies = false;
+                m_allowCookies = false;
         }
     }
 #endif
     
     ASSERT(!m_handle);
     ASSERT(!m_suspended);
-    m_handshake = std::make_unique<WebSocketHandshake>(url, protocol, m_document.get(), allowCookies);
+    
+    String userAgent = m_document->userAgent(m_document->url());
+    String clientOrigin = m_document->securityOrigin().toString();
+    m_handshake = std::make_unique<WebSocketHandshake>(url, protocol, userAgent, clientOrigin, m_allowCookies);
     m_handshake->reset();
     if (m_deflateFramer.canDeflate())
         m_handshake->addExtensionProcessor(m_deflateFramer.createExtensionProcessor());
@@ -247,8 +249,6 @@ void WebSocketChannel::disconnect()
     LOG(Network, "WebSocketChannel %p disconnect()", this);
     if (m_identifier && m_document)
         InspectorInstrumentation::didCloseWebSocket(m_document.get(), m_identifier);
-    if (m_handshake)
-        m_handshake->clearDocument();
     m_client = nullptr;
     m_document = nullptr;
     if (m_handle)
@@ -273,10 +273,18 @@ void WebSocketChannel::didOpenSocketStream(SocketStreamHandle& handle)
     ASSERT(&handle == m_handle);
     if (!m_document)
         return;
-    if (m_identifier && UNLIKELY(InspectorInstrumentation::hasFrontends()))
-        InspectorInstrumentation::willSendWebSocketHandshakeRequest(m_document.get(), m_identifier, m_handshake->clientHandshakeRequest());
+    if (m_identifier && UNLIKELY(InspectorInstrumentation::hasFrontends())) {
+        auto cookieRequestHeaderFieldValue = [document = m_document] (const URL& url) -> String {
+            if (!document || !document->page())
+                return { };
+            return document->page()->cookieJar().cookieRequestHeaderFieldValue(*document, url);
+        };
+        InspectorInstrumentation::willSendWebSocketHandshakeRequest(m_document.get(), m_identifier, m_handshake->clientHandshakeRequest(WTFMove(cookieRequestHeaderFieldValue)));
+    }
     auto handshakeMessage = m_handshake->clientHandshakeMessage();
-    auto cookieRequestHeaderFieldProxy = m_handshake->clientHandshakeCookieRequestHeaderFieldProxy();
+    Optional<CookieRequestHeaderFieldProxy> cookieRequestHeaderFieldProxy;
+    if (m_allowCookies)
+        cookieRequestHeaderFieldProxy = CookieJar::cookieRequestHeaderFieldProxy(*m_document, m_handshake->httpURLForAuthenticationAndCookies());
     handle.sendHandshake(WTFMove(handshakeMessage), WTFMove(cookieRequestHeaderFieldProxy), [this, protectedThis = makeRef(*this)] (bool success, bool didAccessSecureCookies) {
         if (!success)
             fail("Failed to send WebSocket handshake.");
@@ -835,9 +843,9 @@ void WebSocketChannel::sendFrame(WebSocketFrame::OpCode opCode, const char* data
     m_handle->sendData(frameData.data(), frameData.size(), WTFMove(completionHandler));
 }
 
-ResourceRequest WebSocketChannel::clientHandshakeRequest()
+ResourceRequest WebSocketChannel::clientHandshakeRequest(Function<String(const URL&)>&& cookieRequestHeaderFieldValue)
 {
-    return m_handshake->clientHandshakeRequest();
+    return m_handshake->clientHandshakeRequest(WTFMove(cookieRequestHeaderFieldValue));
 }
 
 const ResourceResponse& WebSocketChannel::serverHandshakeResponse() const
index fd44fcb..49bab54 100644 (file)
@@ -117,13 +117,15 @@ public:
 
     unsigned identifier() const { return m_identifier; }
     bool hasCreatedHandshake() { return !!m_handshake; }
-    ResourceRequest clientHandshakeRequest();
+    ResourceRequest clientHandshakeRequest(Function<String(const URL&)>&& cookieRequestHeaderFieldValue);
     const ResourceResponse& serverHandshakeResponse() const;
     WebSocketHandshake::Mode handshakeMode() const;
 
     using RefCounted<WebSocketChannel>::ref;
     using RefCounted<WebSocketChannel>::deref;
 
+    Document* document() { return m_document.get(); }
+    
 protected:
     void refThreadableWebSocketChannel() override { ref(); }
     void derefThreadableWebSocketChannel() override { deref(); }
@@ -203,6 +205,7 @@ private:
     bool m_suspended { false };
     bool m_closing { false };
     bool m_receivedClosingHandshake { false };
+    bool m_allowCookies { true };
     Timer m_closingTimer;
     bool m_closed { false };
     bool m_shouldDiscardReceivedData { false };
index 929f353..f37ced1 100644 (file)
@@ -35,7 +35,6 @@
 
 #include "Cookie.h"
 #include "CookieJar.h"
-#include "Document.h"
 #include "HTTPHeaderMap.h"
 #include "HTTPHeaderNames.h"
 #include "HTTPParsers.h"
@@ -119,12 +118,13 @@ String WebSocketHandshake::getExpectedWebSocketAccept(const String& secWebSocket
     return base64Encode(hash.data(), SHA1::hashSize);
 }
 
-WebSocketHandshake::WebSocketHandshake(const URL& url, const String& protocol, Document* document, bool allowCookies)
+WebSocketHandshake::WebSocketHandshake(const URL& url, const String& protocol, const String& userAgent, const String& clientOrigin, bool allowCookies)
     : m_url(url)
     , m_clientProtocol(protocol)
     , m_secure(m_url.protocolIs("wss"))
-    , m_document(makeWeakPtr(document))
     , m_mode(Incomplete)
+    , m_userAgent(userAgent)
+    , m_clientOrigin(clientOrigin)
     , m_allowCookies(allowCookies)
 {
     m_secWebSocketKey = generateSecWebSocketKey();
@@ -164,11 +164,6 @@ bool WebSocketHandshake::secure() const
     return m_secure;
 }
 
-String WebSocketHandshake::clientOrigin() const
-{
-    return m_document->securityOrigin().toString();
-}
-
 String WebSocketHandshake::clientLocation() const
 {
     StringBuilder builder;
@@ -192,7 +187,7 @@ CString WebSocketHandshake::clientHandshakeMessage() const
     fields.append("Upgrade: websocket");
     fields.append("Connection: Upgrade");
     fields.append("Host: " + hostName(m_url, m_secure));
-    fields.append("Origin: " + clientOrigin());
+    fields.append("Origin: " + m_clientOrigin);
     if (!m_clientProtocol.isEmpty())
         fields.append("Sec-WebSocket-Protocol: " + m_clientProtocol);
 
@@ -213,7 +208,7 @@ CString WebSocketHandshake::clientHandshakeMessage() const
         fields.append("Sec-WebSocket-Extensions: " + extensionValue);
 
     // Add a User-Agent header.
-    fields.append("User-Agent: " + m_document->userAgent(m_document->url()));
+    fields.append(makeString("User-Agent: ", m_userAgent));
 
     // Fields in the handshake are sent by the client in a random order; the
     // order is not meaningful.  Thus, it's ok to send the order we constructed
@@ -229,7 +224,7 @@ CString WebSocketHandshake::clientHandshakeMessage() const
     return builder.toString().utf8();
 }
 
-ResourceRequest WebSocketHandshake::clientHandshakeRequest() const
+ResourceRequest WebSocketHandshake::clientHandshakeRequest(Function<String(const URL&)>&& cookieRequestHeaderFieldValue) const
 {
     // Keep the following consistent with clientHandshakeMessage().
     ResourceRequest request(m_url);
@@ -237,14 +232,13 @@ ResourceRequest WebSocketHandshake::clientHandshakeRequest() const
 
     request.setHTTPHeaderField(HTTPHeaderName::Connection, "Upgrade");
     request.setHTTPHeaderField(HTTPHeaderName::Host, hostName(m_url, m_secure));
-    request.setHTTPHeaderField(HTTPHeaderName::Origin, clientOrigin());
+    request.setHTTPHeaderField(HTTPHeaderName::Origin, m_clientOrigin);
     if (!m_clientProtocol.isEmpty())
         request.setHTTPHeaderField(HTTPHeaderName::SecWebSocketProtocol, m_clientProtocol);
 
     URL url = httpURLForAuthenticationAndCookies();
-    if (m_allowCookies && m_document && m_document->page()) {
-        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(InspectorInstrumentation::hasFrontends());
-        String cookie = m_document->page()->cookieJar().cookieRequestHeaderFieldValue(*m_document, url);
+    if (m_allowCookies) {
+        String cookie = cookieRequestHeaderFieldValue(url);
         if (!cookie.isEmpty())
             request.setHTTPHeaderField(HTTPHeaderName::Cookie, cookie);
     }
@@ -259,29 +253,17 @@ ResourceRequest WebSocketHandshake::clientHandshakeRequest() const
         request.setHTTPHeaderField(HTTPHeaderName::SecWebSocketExtensions, extensionValue);
 
     // Add a User-Agent header.
-    request.setHTTPUserAgent(m_document->userAgent(m_document->url()));
+    request.setHTTPUserAgent(m_userAgent);
 
     return request;
 }
 
-Optional<CookieRequestHeaderFieldProxy> WebSocketHandshake::clientHandshakeCookieRequestHeaderFieldProxy() const
-{
-    if (!m_document || !m_allowCookies)
-        return WTF::nullopt;
-    return CookieJar::cookieRequestHeaderFieldProxy(*m_document, httpURLForAuthenticationAndCookies());
-}
-
 void WebSocketHandshake::reset()
 {
     m_mode = Incomplete;
     m_extensionDispatcher.reset();
 }
 
-void WebSocketHandshake::clearDocument()
-{
-    m_document = nullptr;
-}
-
 int WebSocketHandshake::readServerHandshake(const char* header, size_t len)
 {
     m_mode = Incomplete;
index 6a9fa5c..739fbbd 100644 (file)
@@ -40,7 +40,6 @@
 
 namespace WebCore {
 
-class Document;
 class ResourceRequest;
 
 class WebSocketHandshake {
@@ -49,7 +48,7 @@ public:
     enum Mode {
         Incomplete, Normal, Failed, Connected
     };
-    WebSocketHandshake(const URL&, const String& protocol, Document*, bool allowCookies);
+    WebSocketHandshake(const URL&, const String& protocol, const String& userAgent, const String& clientOrigin, bool allowCookies);
     ~WebSocketHandshake();
 
     const URL& url() const;
@@ -62,15 +61,12 @@ public:
 
     bool secure() const;
 
-    String clientOrigin() const;
     String clientLocation() const;
 
     CString clientHandshakeMessage() const;
-    ResourceRequest clientHandshakeRequest() const;
-    Optional<CookieRequestHeaderFieldProxy> clientHandshakeCookieRequestHeaderFieldProxy() const;
+    ResourceRequest clientHandshakeRequest(Function<String(const URL&)>&& cookieRequestHeaderFieldValue) const;
 
     void reset();
-    void clearDocument();
 
     int readServerHandshake(const char* header, size_t len);
     Mode mode() const;
@@ -101,9 +97,10 @@ private:
     URL m_url;
     String m_clientProtocol;
     bool m_secure;
-    WeakPtr<Document> m_document;
 
     Mode m_mode;
+    String m_userAgent;
+    String m_clientOrigin;
     bool m_allowCookies;
 
     ResourceResponse m_serverHandshakeResponse;
index 8dc3c8d..1ee3c13 100644 (file)
@@ -814,7 +814,12 @@ void InspectorNetworkAgent::enable()
 
             unsigned identifier = channel->identifier();
             didCreateWebSocket(identifier, webSocket->url());
-            willSendWebSocketHandshakeRequest(identifier, channel->clientHandshakeRequest());
+            auto cookieRequestHeaderFieldValue = [document = makeWeakPtr(channel->document())] (const URL& url) -> String {
+                if (!document || !document->page())
+                    return { };
+                return document->page()->cookieJar().cookieRequestHeaderFieldValue(*document, url);
+            };
+            willSendWebSocketHandshakeRequest(identifier, channel->clientHandshakeRequest(WTFMove(cookieRequestHeaderFieldValue)));
 
             if (channel->handshakeMode() == WebSocketHandshake::Connected)
                 didReceiveWebSocketHandshakeResponse(identifier, channel->serverHandshakeResponse());