WebKit Bugzilla
Attachment 360588 Details for
Bug 194023
: Regression(PSON) History navigations to twitter.com lead to a 403 HTTP error
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch
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>
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194023
: 360588