[Curl] Fix issue that extra cookie is added when redirect happens.
authorBasuke.Suzuki@sony.com <Basuke.Suzuki@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Aug 2018 20:50:19 +0000 (20:50 +0000)
committerBasuke.Suzuki@sony.com <Basuke.Suzuki@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Aug 2018 20:50:19 +0000 (20:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187874

Reviewed by Alex Christensen.

Source/WebCore:

When initial request has cookie set and redirect happens, it add extra Cookie header to that
abd request was broken. Just stop modifying the original request by passing a value.

Test: http/tests/cookies/multiple-redirect-and-set-cookie.php

* platform/network/ResourceHandle.h:
* platform/network/curl/ResourceHandleCurl.cpp:
(WebCore::ResourceHandle::createCurlRequest):

Source/WebKit:

When initial request has cookie set and redirect happens, it add extra Cookie header to that
abd request was broken. Just stop modifying the original request by passing a value.

* NetworkProcess/curl/NetworkDataTaskCurl.cpp:
(WebKit::NetworkDataTaskCurl::createCurlRequest):
(WebKit::NetworkDataTaskCurl::willPerformHTTPRedirection):
(WebKit::NetworkDataTaskCurl::restartWithCredential):
* NetworkProcess/curl/NetworkDataTaskCurl.h:

LayoutTests:

* http/tests/cookies/multiple-redirect-and-set-cookie-expected.txt: Added.
* http/tests/cookies/multiple-redirect-and-set-cookie.php: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie.php [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/network/ResourceHandle.h
Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h

index 797fa4c..d764f44 100644 (file)
@@ -1,3 +1,13 @@
+2018-08-28  Basuke Suzuki  <Basuke.Suzuki@sony.com>
+
+        [Curl] Fix issue that extra cookie is added when redirect happens.
+        https://bugs.webkit.org/show_bug.cgi?id=187874
+
+        Reviewed by Alex Christensen.
+
+        * http/tests/cookies/multiple-redirect-and-set-cookie-expected.txt: Added.
+        * http/tests/cookies/multiple-redirect-and-set-cookie.php: Added.
+
 2018-08-28  Aditya Keerthi  <akeerthi@apple.com>
 
         [macOS] Color wells should appear rounded and textured
diff --git a/LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie-expected.txt b/LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie-expected.txt
new file mode 100644 (file)
index 0000000..cc80b0b
--- /dev/null
@@ -0,0 +1 @@
+Cookie: 42
diff --git a/LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie.php b/LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie.php
new file mode 100644 (file)
index 0000000..f9f34fc
--- /dev/null
@@ -0,0 +1,72 @@
+<?php
+
+$expire = time() + 30;
+$step = empty($_GET['step']) ? '' : $_GET['step'];
+$cookie_name = empty($_GET['cookie_name']) ? md5(__FILE__ . time()) : $_GET['cookie_name'];
+
+if (!$step) {
+    // Step 0: Set cookie for following request.
+    setcookie($cookie_name, 'not sure, but something', $expire);
+    step0();
+} elseif ($step == 1) {
+    // Step 1: Request caused by JS. It is sent with Cookie header with value of step 0.
+    setcookie($cookie_name, '42', $expire);
+    redirect_to_step(2);
+} elseif ($step == 2) {
+    // Step 2: Redirected request should have only Cookie header with update value/
+    step2($_COOKIE[$cookie_name]);
+} else {
+    die("Error: unknown step: {$step}");
+}
+
+exit(0);
+
+function redirect_to_step($step) {
+    header("HTTP/1.0 302 Found");
+    header('Location: ' . redirect_url($step));
+}
+
+function redirect_url($step) {
+    global $cookie_name;
+    return "http://127.0.0.1:8000/cookies/" . basename(__FILE__) . "?step={$step}&cookie_name={$cookie_name}";
+}
+
+function step0() {
+?>
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+function gotoStep1() {
+    window.location = "<?php echo redirect_url(1); ?>";
+}
+</script>
+
+<body onload="gotoStep1()">
+</body>
+</html>
+<?php
+}
+
+function step2($result) {
+?>
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<script>
+function finish() {
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+</script>
+
+<body onload="finish()">
+Cookie: <?php echo $result; ?>
+</body>
+</html>
+<?php
+}
+?>
index af21fff..cc2b637 100644 (file)
@@ -1,3 +1,19 @@
+2018-08-28  Basuke Suzuki  <Basuke.Suzuki@sony.com>
+
+        [Curl] Fix issue that extra cookie is added when redirect happens.
+        https://bugs.webkit.org/show_bug.cgi?id=187874
+
+        Reviewed by Alex Christensen.
+
+        When initial request has cookie set and redirect happens, it add extra Cookie header to that
+        abd request was broken. Just stop modifying the original request by passing a value.
+
+        Test: http/tests/cookies/multiple-redirect-and-set-cookie.php
+
+        * platform/network/ResourceHandle.h:
+        * platform/network/curl/ResourceHandleCurl.cpp:
+        (WebCore::ResourceHandle::createCurlRequest):
+
 2018-08-28  Aditya Keerthi  <akeerthi@apple.com>
 
         [macOS] Color wells should appear rounded and textured
index f3609d0..90b0fda 100644 (file)
@@ -259,7 +259,7 @@ private:
     };
 
     void addCacheValidationHeaders(ResourceRequest&);
-    Ref<CurlRequest> createCurlRequest(ResourceRequest&, RequestStatus = RequestStatus::NewRequest);
+    Ref<CurlRequest> createCurlRequest(ResourceRequest&&, RequestStatus = RequestStatus::NewRequest);
 
     bool shouldRedirectAsGET(const ResourceRequest&, bool crossOrigin);
 
index 6e0a6e4..55aa9b7 100644 (file)
@@ -75,13 +75,13 @@ bool ResourceHandle::start()
         return false;
 
     // Only allow the POST and GET methods for non-HTTP requests.
-    const ResourceRequest& request = firstRequest();
+    auto request = firstRequest();
     if (!request.url().protocolIsInHTTPFamily() && request.httpMethod() != "GET" && request.httpMethod() != "POST") {
         scheduleFailure(InvalidURLFailure); // Error must not be reported immediately
         return true;
     }
 
-    d->m_curlRequest = createCurlRequest(d->m_firstRequest);
+    d->m_curlRequest = createCurlRequest(WTFMove(request));
 
     if (auto credential = getCredential(d->m_firstRequest, false))
         d->m_curlRequest->setUserPass(credential->first, credential->second);
@@ -133,7 +133,7 @@ void ResourceHandle::addCacheValidationHeaders(ResourceRequest& request)
     }
 }
 
-Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest& request, RequestStatus status)
+Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest&& request, RequestStatus status)
 {
     ASSERT(isMainThread());
 
@@ -373,7 +373,7 @@ void ResourceHandle::restartRequestWithCredential(const String& user, const Stri
     auto previousRequest = d->m_curlRequest->resourceRequest();
     d->m_curlRequest->cancel();
 
-    d->m_curlRequest = createCurlRequest(previousRequest, RequestStatus::ReusedRequest);
+    d->m_curlRequest = createCurlRequest(WTFMove(previousRequest), RequestStatus::ReusedRequest);
     d->m_curlRequest->setUserPass(user, password);
     d->m_curlRequest->start();
 }
@@ -395,7 +395,7 @@ void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* contex
         return;
     }
 
-    handle->d->m_curlRequest = handle->createCurlRequest(localRequest);
+    handle->d->m_curlRequest = handle->createCurlRequest(WTFMove(localRequest));
     handle->d->m_curlRequest->start();
 
     do {
@@ -504,13 +504,14 @@ void ResourceHandle::continueAfterWillSendRequest(ResourceRequest&& request)
         return;
     }
 
+    auto shouldForwardCredential = protocolHostAndPortAreEqual(request.url(), delegate()->response().url());
+    auto credential = getCredential(request, true);
+
     d->m_curlRequest->cancel();
-    d->m_curlRequest = createCurlRequest(request);
+    d->m_curlRequest = createCurlRequest(WTFMove(request));
 
-    if (protocolHostAndPortAreEqual(request.url(), delegate()->response().url())) {
-        if (auto credential = getCredential(request, true))
-            d->m_curlRequest->setUserPass(credential->first, credential->second);
-    }
+    if (shouldForwardCredential && credential)
+        d->m_curlRequest->setUserPass(credential->first, credential->second);
 
     d->m_curlRequest->start();
 }
index 7b88417..be1e3cc 100644 (file)
@@ -1,3 +1,19 @@
+2018-08-28  Basuke Suzuki  <Basuke.Suzuki@sony.com>
+
+        [Curl] Fix issue that extra cookie is added when redirect happens.
+        https://bugs.webkit.org/show_bug.cgi?id=187874
+
+        Reviewed by Alex Christensen.
+
+        When initial request has cookie set and redirect happens, it add extra Cookie header to that
+        abd request was broken. Just stop modifying the original request by passing a value.
+
+        * NetworkProcess/curl/NetworkDataTaskCurl.cpp:
+        (WebKit::NetworkDataTaskCurl::createCurlRequest):
+        (WebKit::NetworkDataTaskCurl::willPerformHTTPRedirection):
+        (WebKit::NetworkDataTaskCurl::restartWithCredential):
+        * NetworkProcess/curl/NetworkDataTaskCurl.h:
+
 2018-08-28  Aditya Keerthi  <akeerthi@apple.com>
 
         [macOS] Color wells should appear rounded and textured
index 29b528e..a903569 100644 (file)
@@ -61,7 +61,7 @@ NetworkDataTaskCurl::NetworkDataTaskCurl(NetworkSession& session, NetworkDataTas
         }
     }
 
-    m_curlRequest = createCurlRequest(request);
+    m_curlRequest = createCurlRequest(WTFMove(request));
     if (!m_initialCredential.isEmpty())
         m_curlRequest->setUserPass(m_initialCredential.user(), m_initialCredential.password());
     m_curlRequest->start();
@@ -126,16 +126,14 @@ NetworkDataTask::State NetworkDataTaskCurl::state() const
     return m_state;
 }
 
-Ref<CurlRequest> NetworkDataTaskCurl::createCurlRequest(const ResourceRequest& request, RequestStatus status)
+Ref<CurlRequest> NetworkDataTaskCurl::createCurlRequest(ResourceRequest&& request, RequestStatus status)
 {
-    m_currentRequest = request;
-
     if (status == RequestStatus::NewRequest)
-        appendCookieHeader(m_currentRequest);
+        appendCookieHeader(request);
 
     // Creates a CurlRequest in suspended state.
     // Then, NetworkDataTaskCurl::resume() will be called and communication resumes.
-    return CurlRequest::create(m_currentRequest, *this, CurlRequest::ShouldSuspend::Yes);
+    return CurlRequest::create(request, *this, CurlRequest::ShouldSuspend::Yes);
 }
 
 void NetworkDataTaskCurl::curlDidSendData(CurlRequest&, unsigned long long totalBytesSent, unsigned long long totalBytesExpectedToSend)
@@ -248,7 +246,7 @@ void NetworkDataTaskCurl::willPerformHTTPRedirection()
         return;
     }
 
-    ResourceRequest request = m_currentRequest;
+    ResourceRequest request = m_firstRequest;
     URL redirectedURL = URL(m_response.url(), m_response.httpHeaderField(HTTPHeaderName::Location));
     if (!redirectedURL.hasFragmentIdentifier() && request.url().hasFragmentIdentifier())
         redirectedURL.setFragmentIdentifier(request.url().fragmentIdentifier());
@@ -258,7 +256,7 @@ void NetworkDataTaskCurl::willPerformHTTPRedirection()
     if (m_shouldClearReferrerOnHTTPSToHTTPRedirect && !request.url().protocolIs("https") && protocolIs(request.httpReferrer(), "https"))
         request.clearHTTPReferrer();
 
-    bool isCrossOrigin = !protocolHostAndPortAreEqual(m_currentRequest.url(), request.url());
+    bool isCrossOrigin = !protocolHostAndPortAreEqual(m_firstRequest.url(), request.url());
     if (!equalLettersIgnoringASCIICase(request.httpMethod(), "get")) {
         // Change request method to GET if change was made during a previous redirection or if current redirection says so.
         if (!request.url().protocolIsInHTTPFamily() || shouldRedirectAsGET(request, isCrossOrigin)) {
@@ -300,7 +298,8 @@ void NetworkDataTaskCurl::willPerformHTTPRedirection()
         if (m_curlRequest)
             m_curlRequest->cancel();
 
-        m_curlRequest = createCurlRequest(newRequest);
+        auto requestCopy = newRequest;
+        m_curlRequest = createCurlRequest(WTFMove(requestCopy));
         if (didChangeCredential && !m_initialCredential.isEmpty())
             m_curlRequest->setUserPass(m_initialCredential.user(), m_initialCredential.password());
         m_curlRequest->start();
@@ -387,12 +386,13 @@ void NetworkDataTaskCurl::tryProxyAuthentication(WebCore::AuthenticationChalleng
 
 void NetworkDataTaskCurl::restartWithCredential(const Credential& credential)
 {
-    if (m_curlRequest)
-        m_curlRequest->cancel();
+    ASSERT(m_curlRequest);
+
+    auto previousRequest = m_curlRequest->resourceRequest();
+    m_curlRequest->cancel();
 
-    m_curlRequest = createCurlRequest(m_currentRequest, RequestStatus::ReusedRequest);
-    if (!credential.isEmpty())
-        m_curlRequest->setUserPass(credential.user(), credential.password());
+    m_curlRequest = createCurlRequest(WTFMove(previousRequest), RequestStatus::ReusedRequest);
+    m_curlRequest->setUserPass(credential.user(), credential.password());
     m_curlRequest->start();
 
     if (m_state != State::Suspended) {
index 1fd3d02..fcc4d34 100644 (file)
@@ -62,7 +62,7 @@ private:
     void invalidateAndCancel() override;
     NetworkDataTask::State state() const override;
 
-    Ref<WebCore::CurlRequest> createCurlRequest(const WebCore::ResourceRequest&, RequestStatus = RequestStatus::NewRequest);
+    Ref<WebCore::CurlRequest> createCurlRequest(WebCore::ResourceRequest&&, RequestStatus = RequestStatus::NewRequest);
     void curlDidSendData(WebCore::CurlRequest&, unsigned long long, unsigned long long) override;
     void curlDidReceiveResponse(WebCore::CurlRequest&, const WebCore::CurlResponse&) override;
     void curlDidReceiveBuffer(WebCore::CurlRequest&, Ref<WebCore::SharedBuffer>&&) override;
@@ -81,7 +81,6 @@ private:
 
     State m_state { State::Suspended };
 
-    WebCore::ResourceRequest m_currentRequest;
     RefPtr<WebCore::CurlRequest> m_curlRequest;
     WebCore::ResourceResponse m_response;
     unsigned m_redirectCount { 0 };