Attachment 360588 Details for Bug 194023 – Patch

[patch] Patch

bug-194023-20190130093350.patch (text/plain), 10.17 KB, created by Chris Dumez on 2019-01-30 09:33:51 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-30 09:33:51 PST

Size: 10.17 KB

patch

obsolete

>Subversion Revision: 240668
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 90e64bca751f82074546c7ba251db8a144e02fb6..1939744efba0f9ac4ea9a79825bdb3d924d3334e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,38 @@
>+2019-01-30  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON) History navigations to twitter.com lead to a 403 HTTP error
>+        https://bugs.webkit.org/show_bug.cgi?id=194023
>+        <rdar://problem/47417981>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The issue was caused by the 'isTopSite' flag not getting properly set on the network request
>+        in case of a cross-site history navigation (with process-swap). As a result, twitter.com was
>+        not getting its same-site lax cookies.
>+
>+        The 'isTopSite' flag normally gets set by FrameLoader::addExtraFieldsToRequest(), but we were
>+        bypassing this method entirely when continuing a load in a new process after a swap. This was
>+        intentional as the network request is normally already fully populated by the previous process
>+        and we do not want the new process to modify the request in any way (e.g. we would not want to
>+        add a Origin header back after it was removed by the previous process). However, in case of a
>+        History navigation, we do not actually pass a request along from one process to another. Instead,
>+        we pass a HistoryItem and then build a fresh new request from the HistoryItem in the new process.
>+        In this case, we *want* addExtraFieldsToRequest() to be called on the new request, even though
>+        we are technically continuing a load in a new process.
>+
>+        We thus address the issue by bypassing FrameLoader::addExtraFieldsToRequest() only if we're
>+        continuing a load with a request and not when we're continuing a load with a HistoryItem.
>+
>+        Test: http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php
>+
>+        * loader/FrameLoader.cpp:
>+        (WebCore::FrameLoader::load):
>+        (WebCore::FrameLoader::loadWithDocumentLoader):
>+        (WebCore::FrameLoader::addExtraFieldsToRequest):
>+        (WebCore::FrameLoader::loadDifferentDocumentItem):
>+        * loader/FrameLoader.h:
>+        (WebCore::FrameLoader::shouldTreatCurrentLoadAsContinuingLoad const):
>+
> 2019-01-29  Keith Rollin  <krollin@apple.com>
> 
>         Add .xcfilelists to Run Script build phases
>diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
>index 960f5590c72c2d2ff5c961c95f9ae3464071c847..6b3fde74aae349d1ad9070e57e780e404622a2bf 100644
>--- a/Source/WebCore/loader/FrameLoader.cpp
>+++ b/Source/WebCore/loader/FrameLoader.cpp
>@@ -1484,7 +1484,7 @@ void FrameLoader::load(FrameLoadRequest&& request)
>         }
>     }
> 
>-    SetForScope<bool> currentLoadShouldBeTreatedAsContinuingLoadGuard(m_currentLoadShouldBeTreatedAsContinuingLoad, request.shouldTreatAsContinuingLoad());
>+    SetForScope<LoadContinuingState> continuingLoadGuard(m_currentLoadContinuingState, request.shouldTreatAsContinuingLoad() ? LoadContinuingState::ContinuingWithRequest : LoadContinuingState::NotContinuing);
>     load(loader.get());
> }
> 
>@@ -1518,7 +1518,7 @@ void FrameLoader::load(DocumentLoader& newDocumentLoader)
>         type = FrameLoadType::Same;
>     } else if (shouldTreatURLAsSameAsCurrent(newDocumentLoader.unreachableURL()) && isReload(m_loadType))
>         type = m_loadType;
>-    else if (m_loadType == FrameLoadType::RedirectWithLockedBackForwardList && ((!newDocumentLoader.unreachableURL().isEmpty() && newDocumentLoader.substituteData().isValid()) || m_currentLoadShouldBeTreatedAsContinuingLoad))
>+    else if (m_loadType == FrameLoadType::RedirectWithLockedBackForwardList && ((!newDocumentLoader.unreachableURL().isEmpty() && newDocumentLoader.substituteData().isValid()) || shouldTreatCurrentLoadAsContinuingLoad()))
>         type = FrameLoadType::RedirectWithLockedBackForwardList;
>     else
>         type = FrameLoadType::Standard;
>@@ -1625,7 +1625,7 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
> 
>     m_frame.navigationScheduler().cancel(NewLoadInProgress::Yes);
> 
>-    if (m_currentLoadShouldBeTreatedAsContinuingLoad) {
>+    if (shouldTreatCurrentLoadAsContinuingLoad()) {
>         continueLoadAfterNavigationPolicy(loader->request(), formState.get(), ShouldContinue::Yes, allowNavigationToInvalidURL);
>         return;
>     }
>@@ -2853,7 +2853,8 @@ ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const Resour
> 
> void FrameLoader::addExtraFieldsToRequest(ResourceRequest& request, FrameLoadType loadType, bool isMainResource)
> {
>-    if (m_currentLoadShouldBeTreatedAsContinuingLoad)
>+    // If the request came from a previous process due to process-swap-on-navigation then we should not modify the request.
>+    if (m_currentLoadContinuingState == LoadContinuingState::ContinuingWithRequest)
>         return;
> 
>     // Don't set the cookie policy URL if it's already been set.
>@@ -3683,7 +3684,7 @@ void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, FrameLoadType loa
> 
>     auto initiatedByMainFrame = InitiatedByMainFrame::Unknown;
> 
>-    SetForScope<bool> currentLoadShouldBeTreatedAsContinuingLoadGuard(m_currentLoadShouldBeTreatedAsContinuingLoad, shouldTreatAsContinuingLoad == ShouldTreatAsContinuingLoad::Yes);
>+    SetForScope<LoadContinuingState> continuingLoadGuard(m_currentLoadContinuingState, shouldTreatAsContinuingLoad == ShouldTreatAsContinuingLoad::Yes ? LoadContinuingState::ContinuingWithHistoryItem : LoadContinuingState::NotContinuing);
> 
>     if (CachedPage* cachedPage = PageCache::singleton().get(item, m_frame.page())) {
>         auto documentLoader = cachedPage->documentLoader();
>diff --git a/Source/WebCore/loader/FrameLoader.h b/Source/WebCore/loader/FrameLoader.h
>index 89dcb98a775805ba58e1f95e2e99190cc3119203..047cff413707fb064bf95b8840eccc31402f472b 100644
>--- a/Source/WebCore/loader/FrameLoader.h
>+++ b/Source/WebCore/loader/FrameLoader.h
>@@ -406,6 +406,9 @@ private:
>     bool isNavigationAllowed() const;
>     bool isStopLoadingAllowed() const;
> 
>+    enum class LoadContinuingState : uint8_t { NotContinuing, ContinuingWithRequest, ContinuingWithHistoryItem };
>+    bool shouldTreatCurrentLoadAsContinuingLoad() const { return m_currentLoadContinuingState != LoadContinuingState::NotContinuing; }
>+
>     Frame& m_frame;
>     FrameLoaderClient& m_client;
> 
>@@ -474,7 +477,8 @@ private:
>     Optional<ResourceRequestCachePolicy> m_overrideCachePolicyForTesting;
>     Optional<ResourceLoadPriority> m_overrideResourceLoadPriorityForTesting;
>     bool m_isStrictRawResourceValidationPolicyDisabledForTesting { false };
>-    bool m_currentLoadShouldBeTreatedAsContinuingLoad { false };
>+
>+    LoadContinuingState m_currentLoadContinuingState { LoadContinuingState::NotContinuing };
> 
>     bool m_checkingLoadCompleteForDetachment { false };
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index d6aff6da943466b7043833cb805e6e08cc96129a..1160b7b6d08f76492d65b65b0fe67dd5d59d7fef 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-30  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON) History navigations to twitter.com lead to a 403 HTTP error
>+        https://bugs.webkit.org/show_bug.cgi?id=194023
>+        <rdar://problem/47417981>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add layout test coverage.
>+
>+        * http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt: Added.
>+        * http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php: Added.
>+        * http/tests/cookies/same-site/resources/navigate-back.html: Added.
>+
> 2019-01-29  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: provide a way to edit page WebRTC settings on a remote target
>diff --git a/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt b/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..1a5792ec4b56e6608bf36161199cc7409c02c81a
>--- /dev/null
>+++ b/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt
>@@ -0,0 +1,10 @@
>+Tests that lax same-site cookies are sent on a cross-site history navigation.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS Was able to read the cookie after the back navigation
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php b/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php
>new file mode 100644
>index 0000000000000000000000000000000000000000..737895c4a456fe030a276cc0f0e71cae2cea69e4
>--- /dev/null
>+++ b/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php
>@@ -0,0 +1,26 @@
>+<?php
>+header("Cache-Control: no-store");
>+?>
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script src="/js-test-resources/js-test.js"></script>
>+<script>
>+description("Tests that lax same-site cookies are sent on a cross-site history navigation.");
>+jsTestIsAsync = true;
>+
>+onload = function() {
>+<?php
>+if (isset($_COOKIE["foo"])) {
>+    echo "    testPassed('Was able to read the cookie after the back navigation');";
>+    echo "    finishJSTest();";
>+    echo "    return;";
>+} else {
>+    echo "    document.cookie = 'foo=bar; SameSite=lax';";
>+    echo "    setTimeout(() => { window.location = 'http://localhost:8000/cookies/same-site/resources/navigate-back.html'; }, 0);";
>+}
>+?>
>+}
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html b/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..22dd477e4ad4b904558a6bcb6254c4acff6294d4
>--- /dev/null
>+++ b/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html
>@@ -0,0 +1,15 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script>
>+if (window.testRunner)
>+    testRunner.waitUntilDone();
>+
>+onload = () => {
>+    setTimeout(() => {
>+        history.back();
>+    }, 0);
>+};
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 194023: 360588