[webkit-changes] [WebKit/WebKit] 73b600: Update a request's SameSiteDisposition before usin...

[Alex Christensen]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 73b60062710e53db813763debaa06aa0defb1140
      https://github.com/WebKit/WebKit/commit/73b60062710e53db813763debaa06aa0defb1140
  Author: Alex Christensen <achristensen at apple.com>
  Date:   2024-07-26 (Fri, 26 Jul 2024)

  Changed paths:
    M LayoutTests/platform/ios/TestExpectations
    M LayoutTests/platform/mac-wk2/TestExpectations
    M LayoutTests/platform/wk2/TestExpectations
    M Source/WebCore/loader/cache/CachedResourceLoader.cpp

  Log Message:
  -----------
  Update a request's SameSiteDisposition before using it for memory cache matching
https://bugs.webkit.org/show_bug.cgi?id=277186
rdar://131857779

Reviewed by Matthew Finkel.

In macOS Sequoia and iOS 18, CFNetwork's default cookie samesite attribute changed from none to lax
along with other browsers.  This exposed an issue in our checking for Vary: Cookie header matching
when considering whether to use the memory cache or not.  This was found by a failure in two layout
tests, http/tests/cache/disk-cache/disk-cache-vary-cookie-private.html and
http/tests/cache/disk-cache/disk-cache-vary-cookie.html which showed a resource being loaded from the
disk cache instead of the memory cache.

After reduction and investigation and assistance from the CFNetwork team, I found we were using the
SameSiteDisposition from a ResourceRequest with this stack trace:

WebCore::cookieRequestHeaderFieldValue(WebCore::CookieJar const*, WebCore::ResourceRequest const&)
WebCore::verifyVaryingRequestHeaders(WebCore::CookieJar const*, WTF::Vector<std::__1::pair<WTF::String, WTF::String>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::ResourceRequest const&)::$_0::operator()(WTF::String const&) const::'lambda'()::operator()() const
WTF::Detail::CallableWrapper<WebCore::verifyVaryingRequestHeaders(WebCore::CookieJar const*, WTF::Vector<std::__1::pair<WTF::String, WTF::String>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::ResourceRequest const&)::$_0::operator()(WTF::String const&) const::'lambda'(), WTF::String>::call()
WTF::Function<WTF::String ()>::operator()() const
WebCore::headerValueForVary(WebCore::ResourceRequest const&, WTF::StringView, WTF::Function<WTF::String ()>&&)
WebCore::verifyVaryingRequestHeaders(WebCore::CookieJar const*, WTF::Vector<std::__1::pair<WTF::String, WTF::String>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::ResourceRequest const&)::$_0::operator()(WTF::String const&) const
WTF::Detail::CallableWrapper<WebCore::verifyVaryingRequestHeaders(WebCore::CookieJar const*, WTF::Vector<std::__1::pair<WTF::String, WTF::String>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::ResourceRequest const&)::$_0, WTF::String, WTF::String const&>::call(WTF::String const&)
WTF::Function<WTF::String (WTF::String const&)>::operator()(WTF::String const&) const
WebCore::verifyVaryingRequestHeadersInternal(WTF::Vector<std::__1::pair<WTF::String, WTF::String>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WTF::Function<WTF::String (WTF::String const&)>&&)
WebCore::verifyVaryingRequestHeaders(WebCore::CookieJar const*, WTF::Vector<std::__1::pair<WTF::String, WTF::String>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::ResourceRequest const&)
WebCore::CachedResource::varyHeaderValuesMatch(WebCore::ResourceRequest const&)
WebCore::CachedResourceLoader::determineRevalidationPolicy(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&, WebCore::CachedResource*, WebCore::CachedResourceLoader::ForPreload, WebCore::ImageLoading) const
WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&&, WebCore::CachedResourceLoader::ForPreload, WebCore::ImageLoading)
...
WebCore::XMLHttpRequest::send(WTF::String const&)

Before we were correctly setting the SameSiteDisposition with this stack trace:

WebCore::ResourceRequestBase::setIsSameSite(bool)
WebCore::FrameLoader::addSameSiteInfoToRequestIfNeeded(WebCore::ResourceRequest&, WebCore::Document const*, WebCore::Page const*)
WebCore::FrameLoader::updateRequestAndAddExtraFields(WebCore::Frame&, WebCore::ResourceRequest&, WebCore::IsMainResource, WebCore::FrameLoadType, WebCore::ShouldUpdateAppInitiatedValue, WebCore::FrameLoader::IsServiceWorkerNavigationLoad, WebCore::FrameLoader::WillOpenInNewWindow, WebCore::Document*)
WebCore::FrameLoader::updateRequestAndAddExtraFields(WebCore::ResourceRequest&, WebCore::IsMainResource, WebCore::FrameLoadType, WebCore::ShouldUpdateAppInitiatedValue, WebCore::FrameLoader::IsServiceWorkerNavigationLoad, WebCore::FrameLoader::WillOpenInNewWindow, WebCore::Document*)
WebCore::CachedResource::load(WebCore::CachedResourceLoader&)
WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&&, WebCore::CachedResourceLoader::ForPreload, WebCore::ImageLoading)
...
WebCore::XMLHttpRequest::send(WTF::String const&)

The result was that when we called SameSiteInfo::create, ResourceRequestBase::isSameSite() was returning
false when it should have been returning true, causing us to give CFNetwork a dictionary with a key of
_kCFHTTPCookiePolicyPropertySiteForCookies and a value of an empty URL instead of the non-empty URL, which,
when combined with the samesite attribute change caused them to give us fewer cookies than they should,
which caused us to incorrectly determine that we can't use the memory cache.

The fix is to call FrameLoader::addSameSiteInfoToRequestIfNeeded before calling determineRevalidationPolicy.
Calling FrameLoader::addSameSiteInfoToRequestIfNeeded multiple times in the loading flow is not an issue
because if it has already been called it returns early.

The test had been marked as failing and timing out on several Cocoa platforms. Since the tests seem
to be passing quite reliably on released OSes and this fixes it on Sequoia, I updated test expectations
to expect the tests to pass reliably on all Cocoa platforms.

* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac-wk2/TestExpectations:
* LayoutTests/platform/wk2/TestExpectations:
* Source/WebCore/loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::requestResource):

Canonical link: https://commits.webkit.org/281441@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications