- Revision
- 240750
- Author
- [email protected]
- Date
- 2019-01-30 18:23:55 -0800 (Wed, 30 Jan 2019)
Log Message
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (240749 => 240750)
--- trunk/LayoutTests/ChangeLog 2019-01-31 02:18:18 UTC (rev 240749)
+++ trunk/LayoutTests/ChangeLog 2019-01-31 02:23:55 UTC (rev 240750)
@@ -1,3 +1,17 @@
+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-01-30 Daniel Bates <[email protected]>
[iOS] Keyups for non-modifier keys identified as "Dead" when not focused in a content-editable element
Added: trunk/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt (0 => 240750)
--- trunk/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load-expected.txt 2019-01-31 02:23:55 UTC (rev 240750)
@@ -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: trunk/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php (0 => 240750)
--- trunk/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php (rev 0)
+++ trunk/LayoutTests/http/tests/cookies/same-site/lax-samesite-cookie-after-cross-site-history-load.php 2019-01-31 02:23:55 UTC (rev 240750)
@@ -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: trunk/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html (0 => 240750)
--- trunk/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html (rev 0)
+++ trunk/LayoutTests/http/tests/cookies/same-site/resources/navigate-back.html 2019-01-31 02:23:55 UTC (rev 240750)
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+if (window.testRunner)
+ testRunner.waitUntilDone();
+
+_onload_ = () => {
+ setTimeout(() => {
+ history.back();
+ }, 0);
+};
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (240749 => 240750)
--- trunk/Source/WebCore/ChangeLog 2019-01-31 02:18:18 UTC (rev 240749)
+++ trunk/Source/WebCore/ChangeLog 2019-01-31 02:23:55 UTC (rev 240750)
@@ -1,3 +1,38 @@
+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-01-30 Justin Fan <[email protected]>
[WebGPU] Support GPUDepthStencilStateDescriptor
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (240749 => 240750)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2019-01-31 02:18:18 UTC (rev 240749)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2019-01-31 02:23:55 UTC (rev 240750)
@@ -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;
}
@@ -2853,7 +2853,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.
@@ -3683,7 +3684,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: trunk/Source/WebCore/loader/FrameLoader.h (240749 => 240750)
--- trunk/Source/WebCore/loader/FrameLoader.h 2019-01-31 02:18:18 UTC (rev 240749)
+++ trunk/Source/WebCore/loader/FrameLoader.h 2019-01-31 02:23:55 UTC (rev 240750)
@@ -406,6 +406,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;
@@ -474,8 +477,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;
