Skip to content

Commit

Permalink
Use correct top origin when checking firstPartyForCookies with site i…
Browse files Browse the repository at this point in the history
…solation enabled

https://bugs.webkit.org/show_bug.cgi?id=272373
rdar://126114826

Reviewed by Charlie Wolfe and Sihui Liu.

There were two issues here:

1. Document::topOrigin was using topDocument() to get the SecurityOrigin, and with site isolation
enabled the top document isn't necessarily the main frame's Document.  Instead, use a strategy
like we do with mainFrameURL where the Page has the main frame's origin in all processes, whether
the main frame is remote or not.

2. In WebProcessPool::processForNavigation when navigating the main frame, use the RegistrableDomain
from the navigation instead of the PageLoadState, which may not have the activeURL yet.

With these 2 changes, we stop getting failures in NetworkProcess::allowsFirstPartyForCookies in 2 tests
when run with --site-isolation.  The tests change from crashes to regular time-outs.

* LayoutTests/platform/mac-site-isolation/TestExpectations:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::topOrigin const):
* Source/WebCore/dom/Document.h:
* Source/WebCore/page/Page.cpp:
(WebCore::Page::setMainFrameURL):
(WebCore::Page::mainFrameOrigin const):
* Source/WebCore/page/Page.h:
* Source/WebCore/page/SecurityOrigin.cpp:
(WebCore::SecurityOrigin::opaqueOrigin):
* Source/WebCore/page/SecurityOrigin.h:
* Source/WebKit/UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigation):

Canonical link: https://commits.webkit.org/277331@main
  • Loading branch information
achristensen07 committed Apr 10, 2024
1 parent 8d0f6f9 commit e8a014a
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 4 deletions.
4 changes: 2 additions & 2 deletions LayoutTests/platform/mac-site-isolation/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -484,8 +484,6 @@ http/tests/site-isolation/selection-focus.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/navigating-across-documents/top-level-data-url.window.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/read-media/cross-origin-video.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/nested-browsing-contexts/name-attribute.window.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/third-party-to-first-party-cross-partition-cross-origin.sub.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/third-party-to-first-party-cross-partition-same-origin.sub.html [ Skip ]

#######################
# Tests that time out #
Expand Down Expand Up @@ -1139,6 +1137,8 @@ imported/w3c/web-platform-tests/html/browsers/windows/post-message/first-party-t
imported/w3c/web-platform-tests/html/browsers/windows/post-message/first-party-to-first-party-same-partition.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/first-party-to-third-party-cross-partition-cross-origin.sub.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/first-party-to-third-party-cross-partition-same-origin.sub.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/third-party-to-first-party-cross-partition-cross-origin.sub.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/third-party-to-first-party-cross-partition-same-origin.sub.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/third-party-to-third-party-cross-partition-cross-origin.sub.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/third-party-to-third-party-cross-partition-same-origin.sub.html [ Skip ]
imported/w3c/web-platform-tests/html/browsers/windows/post-message/third-party-to-third-party-same-partition.sub.html [ Skip ]
Expand Down
16 changes: 16 additions & 0 deletions Source/WebCore/dom/Document.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,22 @@ Ref<SecurityOrigin> Document::protectedTopOrigin() const
return topOrigin();
}

SecurityOrigin& Document::topOrigin() const
{
// Keep exact pre-site-isolation behavior to avoid risking changing behavior when site isolation is not enabled.
if (!settings().siteIsolationEnabled())
return topDocument().securityOrigin();

RefPtr frame = this->frame();
if (RefPtr localMainFrame = frame ? dynamicDowncast<LocalFrame>(frame->mainFrame()) : nullptr) {
if (RefPtr mainFrameDocument = localMainFrame->document())
return mainFrameDocument->securityOrigin();
}
if (RefPtr page = this->page())
return page->mainFrameOrigin();
return SecurityOrigin::opaqueOrigin();
}

inline DocumentFontLoader& Document::fontLoader()
{
ASSERT(m_constructionDidFinish);
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/dom/Document.h
Original file line number Diff line number Diff line change
Expand Up @@ -1577,7 +1577,7 @@ class Document

SecurityOrigin& securityOrigin() const { return *SecurityContext::securityOrigin(); }
inline Ref<SecurityOrigin> protectedSecurityOrigin() const; // Defined in DocumentInlines.h.
SecurityOrigin& topOrigin() const final { return topDocument().securityOrigin(); }
WEBCORE_EXPORT SecurityOrigin& topOrigin() const final;
Ref<SecurityOrigin> protectedTopOrigin() const;
inline ClientOrigin clientOrigin() const;

Expand Down
8 changes: 8 additions & 0 deletions Source/WebCore/page/Page.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,14 @@ void Page::setMainFrame(Ref<Frame>&& frame)
void Page::setMainFrameURL(const URL& url)
{
m_mainFrameURL = url;
m_mainFrameOrigin = SecurityOrigin::create(url);
}

SecurityOrigin& Page::mainFrameOrigin() const
{
if (!m_mainFrameOrigin)
return SecurityOrigin::opaqueOrigin();
return *m_mainFrameOrigin;
}

bool Page::openedByDOM() const
Expand Down
2 changes: 2 additions & 0 deletions Source/WebCore/page/Page.h
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ class Page : public RefCounted<Page>, public Supplementable<Page>, public CanMak
WEBCORE_EXPORT Ref<Frame> protectedMainFrame() const;
WEBCORE_EXPORT void setMainFrame(Ref<Frame>&&);
const URL& mainFrameURL() const { return m_mainFrameURL; }
SecurityOrigin& mainFrameOrigin() const;
WEBCORE_EXPORT void setMainFrameURL(const URL&);

bool openedByDOM() const;
Expand Down Expand Up @@ -1202,6 +1203,7 @@ class Page : public RefCounted<Page>, public Supplementable<Page>, public CanMak
UniqueRef<EditorClient> m_editorClient;
Ref<Frame> m_mainFrame;
URL m_mainFrameURL;
RefPtr<SecurityOrigin> m_mainFrameOrigin;

RefPtr<PluginData> m_pluginData;

Expand Down
6 changes: 6 additions & 0 deletions Source/WebCore/page/SecurityOrigin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,12 @@ Ref<SecurityOrigin> SecurityOrigin::createOpaque()
return origin;
}

SecurityOrigin& SecurityOrigin::opaqueOrigin()
{
static NeverDestroyed<Ref<SecurityOrigin>> origin { createOpaque() };
return origin.get();
}

Ref<SecurityOrigin> SecurityOrigin::createNonLocalWithAllowedFilePath(const URL& url, const String& filePath)
{
ASSERT(!url.protocolIsFile());
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/page/SecurityOrigin.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class SecurityOrigin : public ThreadSafeRefCounted<SecurityOrigin> {
WEBCORE_EXPORT static Ref<SecurityOrigin> create(const URL&);
WEBCORE_EXPORT static Ref<SecurityOrigin> createForBlobURL(const URL&);
WEBCORE_EXPORT static Ref<SecurityOrigin> createOpaque();
static SecurityOrigin& opaqueOrigin();

WEBCORE_EXPORT static Ref<SecurityOrigin> createFromString(const String&);
WEBCORE_EXPORT static Ref<SecurityOrigin> create(const String& protocol, const String& host, std::optional<uint16_t> port);
Expand Down
2 changes: 1 addition & 1 deletion Source/WebKit/UIProcess/WebProcessPool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1974,7 +1974,7 @@ void WebProcessPool::processForNavigation(WebPageProxy& page, WebFrameProxy& fra
{
RegistrableDomain registrableDomain { navigation.currentRequest().url() };
if (page.preferences().siteIsolationEnabled() && !registrableDomain.isEmpty()) {
RegistrableDomain mainFrameDomain(URL(page.pageLoadState().activeURL()));
auto mainFrameDomain = frameInfo.isMainFrame ? registrableDomain : RegistrableDomain(URL(page.pageLoadState().activeURL()));
if (!frame.isMainFrame() && registrableDomain == mainFrameDomain) {
Ref mainFrameProcess = page.mainFrame()->protectedProcess();
if (!mainFrameProcess->isInProcessCache())
Expand Down

0 comments on commit e8a014a

Please sign in to comment.