- Revision
- 241070
- Author
- [email protected]
- Date
- 2019-02-06 14:16:58 -0800 (Wed, 06 Feb 2019)
Log Message
Cherry-pick r240750. rdar://problem/47774507
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.
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
Modified Paths
Added Paths
Diff
Modified: branches/safari-607-branch/LayoutTests/ChangeLog (241069 => 241070)
--- branches/safari-607-branch/LayoutTests/ChangeLog 2019-02-06 22:16:55 UTC (rev 241069)
+++ branches/safari-607-branch/LayoutTests/ChangeLog 2019-02-06 22:16:58 UTC (rev 241070)
@@ -1,5 +1,69 @@
2019-02-05 Alan Coon <[email protected]>
+ Cherry-pick r240750. rdar://problem/47774507
+
+ 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.
+
+ 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
+
+ 2019-01-30 Chris Dumez <[email protected]>
+
+ 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-02-05 Alan Coon <[email protected]>
+
Cherry-pick r240709. rdar://problem/47776349
AX: Role=switch not returning correct accessibilityValue
Added: branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt (0 => 241070)
--- branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt (rev 0)
+++ branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt 2019-02-06 22:16:58 UTC (rev 241070)
@@ -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
+
Added: branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php (0 => 241070)
--- branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php (rev 0)
+++ branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php 2019-02-06 22:16:58 UTC (rev 241070)
@@ -0,0 +1,26 @@
+<?php
+header("Cache-Control: no-store");
+?>
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<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>
Added: branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html (0 => 241070)
--- branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html (rev 0)
+++ branches/safari-607-branch/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html 2019-02-06 22:16:58 UTC (rev 241070)
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+if (window.testRunner)
+ testRunner.waitUntilDone();
+
+_onload_ = () => {
+ setTimeout(() => {
+ history.back();
+ }, 0);
+};
+</script>
+</body>
+</html>
Modified: branches/safari-607-branch/Source/WebCore/ChangeLog (241069 => 241070)
--- branches/safari-607-branch/Source/WebCore/ChangeLog 2019-02-06 22:16:55 UTC (rev 241069)
+++ branches/safari-607-branch/Source/WebCore/ChangeLog 2019-02-06 22:16:58 UTC (rev 241070)
@@ -1,5 +1,90 @@
2019-02-05 Alan Coon <[email protected]>
+ Cherry-pick r240750. rdar://problem/47774507
+
+ 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.
+
+ 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
+
+ 2019-01-30 Chris Dumez <[email protected]>
+
+ 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-02-05 Alan Coon <[email protected]>
+
Cherry-pick r240727. rdar://problem/47776358
LayoutTests/imported/w3c:
Modified: branches/safari-607-branch/Source/WebCore/loader/FrameLoader.cpp (241069 => 241070)
--- branches/safari-607-branch/Source/WebCore/loader/FrameLoader.cpp 2019-02-06 22:16:55 UTC (rev 241069)
+++ branches/safari-607-branch/Source/WebCore/loader/FrameLoader.cpp 2019-02-06 22:16:58 UTC (rev 241070)
@@ -1484,7 +1484,7 @@
}
}
- 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 @@
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 @@
m_frame.navigationScheduler().cancel(NewLoadInProgress::Yes);
- if (m_currentLoadShouldBeTreatedAsContinuingLoad) {
+ if (shouldTreatCurrentLoadAsContinuingLoad()) {
continueLoadAfterNavigationPolicy(loader->request(), formState.get(), ShouldContinue::Yes, allowNavigationToInvalidURL);
return;
}
@@ -2830,7 +2830,8 @@
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.
@@ -3660,7 +3661,7 @@
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();
Modified: branches/safari-607-branch/Source/WebCore/loader/FrameLoader.h (241069 => 241070)
--- branches/safari-607-branch/Source/WebCore/loader/FrameLoader.h 2019-02-06 22:16:55 UTC (rev 241069)
+++ branches/safari-607-branch/Source/WebCore/loader/FrameLoader.h 2019-02-06 22:16:58 UTC (rev 241070)
@@ -405,6 +405,9 @@
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;
@@ -473,8 +476,9 @@
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 };
URL m_previousURL;
