Attachment 348313 Details for Bug 187874 – PATCH

[patch] PATCH

187874.diff (text/plain), 13.67 KB, created by Basuke Suzuki on 2018-08-28 11:11:58 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Basuke Suzuki

Created: 2018-08-28 11:11:58 PDT

Size: 13.67 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 177983f494d..17b98d77398 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-08-27  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 NOBODY (OOPS!).
>+
>+        * http/tests/cookies/multiple-redirect-and-set-cookie-expected.txt: Added.
>+        * http/tests/cookies/multiple-redirect-and-set-cookie.php: Added.
>+
> 2018-08-24  Youenn Fablet  <youenn@apple.com>
> 
>         libwebrtc PeerConnection::AddTrack sometimes fail
>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
>index 00000000000..cc80b0be487
>--- /dev/null
>+++ b/LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie-expected.txt
>@@ -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
>index 00000000000..f9f34fc3376
>--- /dev/null
>+++ b/LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie.php
>@@ -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
>+}
>+?>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d10643d9539..779fbb4a21e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-08-27  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 NOBODY (OOPS!).
>+
>+        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-24  Youenn Fablet  <youenn@apple.com>
> 
>         libwebrtc PeerConnection::AddTrack sometimes fail
>diff --git a/Source/WebCore/platform/network/ResourceHandle.h b/Source/WebCore/platform/network/ResourceHandle.h
>index f3609d0ea62..90b0fda53b5 100644
>--- a/Source/WebCore/platform/network/ResourceHandle.h
>+++ b/Source/WebCore/platform/network/ResourceHandle.h
>@@ -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);
> 
>diff --git a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>index 6e0a6e45be5..55aa9b709ba 100644
>--- a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>+++ b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>@@ -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();
> }
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index f6b8722228a..f0d1789430a 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-08-27  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 NOBODY (OOPS!).
>+
>+        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-24  Basuke Suzuki  <Basuke.Suzuki@sony.com>
> 
>         [Curl] Match the interface used in NetworkDataTask and ResourceHandle.
>diff --git a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
>index b0623e3733d..15cbaea6906 100644
>--- a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
>+++ b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
>@@ -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)
>@@ -243,7 +241,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());
>@@ -253,7 +251,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)) {
>@@ -295,7 +293,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();
>@@ -362,12 +361,13 @@ void NetworkDataTaskCurl::tryHttpAuthentication(AuthenticationChallenge&& challe
> 
> 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) {
>diff --git a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h
>index d955f9a0836..2a521ef95f6 100644
>--- a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h
>+++ b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h
>@@ -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;
>@@ -80,7 +80,6 @@ private:
> 
>     State m_state { State::Suspended };
> 
>-    WebCore::ResourceRequest m_currentRequest;
>     RefPtr<WebCore::CurlRequest> m_curlRequest;
>     WebCore::ResourceResponse m_response;
>     unsigned m_redirectCount { 0 };

Actions: View | Formatted Diff | Diff

Attachments on bug 187874: 348185 | 348313