Regression(PSON) History navigations to twitter.com lead to a 403 HTTP error
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Jan 2019 02:23:55 +0000 (02:23 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Jan 2019 02:23:55 +0000 (02:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194023
<rdar://problem/47417981>

Reviewed by Geoffrey Garen.

Source/WebCore:

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):

LayoutTests:

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.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php [new file with mode: 0644]
LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h

index 9d457d6..6ff2311 100644 (file)
@@ -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 Geoffrey Garen.
+
+        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-30  Daniel Bates  <dabates@apple.com>
 
         [iOS] Keyups for non-modifier keys identified as "Dead" when not focused in a content-editable element
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 (file)
index 0000000..1a5792e
--- /dev/null
@@ -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 (file)
index 0000000..737895c
--- /dev/null
@@ -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 (file)
index 0000000..22dd477
--- /dev/null
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+onload = () => {
+    setTimeout(() => {
+        history.back();
+    }, 0);
+};
+</script>
+</body>
+</html>
index 14dd712..fd03b74 100644 (file)
@@ -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 Geoffrey Garen.
+
+        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-30  Justin Fan  <justin_fan@apple.com>
 
         [WebGPU] Support GPUDepthStencilStateDescriptor
index 960f559..6b3fde7 100644 (file)
@@ -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();
index 89dcb98..047cff4 100644 (file)
@@ -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 };