SameSite=Lax cookie attribute not correctly handled in Safari 14.0.2 …

…< version <= current https://bugs.webkit.org/show_bug.cgi?id=230568 <rdar://83360821> Reviewed by Darin Adler. The `isTopLevelNavigation` flag on the request needs to be properly set for top-level navigations in order for SameSite=Lax cookies to be sent. In the case of this bug, the navigation is initiated by a cross-origin iframe but it is still a top-level navigation since the navigation occurs in a new window, not in the iframe itself. However, the isTopLevelNavigation flag would be unset and SameSite=Lax cookies wouldn't get sent. The issue was that FrameLoader::updateRequestAndAddExtraFields() was checking if the FrameLoader's m_frame was the main frame to determine if the main frame is being navigated. This generally works because targeted (e.g. via `target="foo"`) are usually resolved before calling this function so we're supposed to be on the target frame's FrameLoader already. However, this is not the case when the navigation is to happen in a new window since there isn't a target frame yet (it will get created later on). To address the issue, call sites of updateRequestAndAddExtraFields() now pass in a new willOpenInNewWindow flag whenever the navigation will happen in a new window. As a result, updateRequestAndAddExtraFields() can properly set the `isTopLevelNavigation` flag in this case. Test: http/tests/cookies/same-site/popup-cross-site-from-cross-origin-iframe.html * LayoutTests/http/tests/cookies/same-site/popup-cross-site-from-cross-origin-iframe-expected.txt: Added. * LayoutTests/http/tests/cookies/same-site/popup-cross-site-from-cross-origin-iframe.html: Added. * LayoutTests/http/tests/cookies/same-site/resources/navigate-to-post-cookies-to-opener-iframe.html: Added. * Source/WebCore/loader/FrameLoader.cpp: (WebCore::FrameLoader::loadURL): (WebCore::FrameLoader::updateRequestAndAddExtraFields): (WebCore::FrameLoader::loadPostRequest): * Source/WebCore/loader/FrameLoader.h: Canonical link: https://commits.webkit.org/252341@main
WebKit · Jul 11, 2022 · ac5c740216a596455829533b6da376428ce40f98 · ac5c740
1 parent 5cdbff3
commit ac5c740216a596455829533b6da376428ce40f98
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 8 deletions.
diff --git a/...Tests/http/tests/cookies/same-site/popup-cross-site-from-cross-origin-iframe-expected.txt b/...Tests/http/tests/cookies/same-site/popup-cross-site-from-cross-origin-iframe-expected.txt
@@ -0,0 +1,4 @@
+
+
+PASS '127.0.0.1' is not same-site with 'localhost', so strict samesite cookies are not sent, but lax ones are.
+
diff --git a/LayoutTests/http/tests/cookies/same-site/popup-cross-site-from-cross-origin-iframe.html b/LayoutTests/http/tests/cookies/same-site/popup-cross-site-from-cross-origin-iframe.html
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="/js-test-resources/testharness.js"></script>
+<script src="/js-test-resources/testharnessreport.js"></script>
+<script src="../resources/testharness-helpers.js"></script>
+</head>
+<body>
+<script>
+if (window.location.hostname == "127.0.0.1") {
+    document.cookie = STRICT_DOM + "=1; SameSite=Strict; Max-Age=100; path=/";
+    document.cookie = IMPLICIT_STRICT_DOM + "=1; SameSite; Max-Age=100; path=/";
+    document.cookie = STRICT_BECAUSE_INVALID_SAMESITE_VALUE + "=1; SameSite=invalid; Max-Age=100; path=/";
+    document.cookie = LAX_DOM + "=1; SameSite=Lax; Max-Age=100; path=/";
+    document.cookie = NORMAL_DOM + "=1; Max-Age=100; path=/";
+    window.location.hostname = "localhost";
+} else {
+    async_test(t => {
+        let iframe = document.createElement("iframe");
+        iframe.src = "http://127.0.0.1:8000/cookies/same-site/resources/navigate-to-post-cookies-to-opener-iframe.html";
+        document.body.appendChild(iframe);
+        window.addEventListener("message", t.step_func_done(e => {
+            assert_equals(e.data.http[STRICT_DOM], undefined, "strict");
+            assert_equals(e.data.http[IMPLICIT_STRICT_DOM], "1", "implicit-strict");
+            assert_equals(e.data.http[STRICT_BECAUSE_INVALID_SAMESITE_VALUE], "1", "strict-because-invalid-SameSite-value");
+            assert_equals(e.data.http[LAX_DOM], "1", "lax");
+            assert_equals(e.data.http[NORMAL_DOM], "1", "normal");
+            assert_equals(normalizeCookie(e.data.document), normalizeCookie(IMPLICIT_STRICT_DOM + "=1; " + LAX_DOM + "=1; " + NORMAL_DOM + "=1; " + STRICT_BECAUSE_INVALID_SAMESITE_VALUE + "=1; " + STRICT_DOM + "=1"));
+        }));
+    }, "'127.0.0.1' is not same-site with 'localhost', so strict samesite cookies are not sent, but lax ones are.");
+}
+</script>
+</body>
+</html>
diff --git a/...sts/http/tests/cookies/same-site/resources/navigate-to-post-cookies-to-opener-iframe.html b/...sts/http/tests/cookies/same-site/resources/navigate-to-post-cookies-to-opener-iframe.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<body>
+<a id="testLink" href="../../resources/post-cookies-to-opener.py" target="foo">Click me</a>
+<script>
+
+window.addEventListener("message", (e) => {
+    top.postMessage(e.data, "*");
+});
+
+onload = () => {
+    setTimeout(() => {
+        document.getElementById("testLink").click();
+    }, 0);
+};
+</script>
+</body>
+</html>
diff --git a/.../should-open-external-urls/user-gesture-target-blank-with-flag-from-subframe-expected.txt b/.../should-open-external-urls/user-gesture-target-blank-with-flag-from-subframe-expected.txt
@@ -3,5 +3,5 @@
  - decidePolicyForNavigationAction
 <NSURLRequest URL user-gesture-target-blank-to-notify-done.html, main document URL user-gesture-target-blank-to-notify-done-from-subframe.html, http method GET> is main frame - no should open URLs externally - no
  - decidePolicyForNavigationAction
-<NSURLRequest URL notify-done.html, main document URL user-gesture-target-blank-to-notify-done-from-subframe.html, http method GET> is main frame - yes should open URLs externally - yes
+<NSURLRequest URL notify-done.html, main document URL notify-done.html, http method GET> is main frame - yes should open URLs externally - yes
 
diff --git a/...ould-open-external-urls/user-gesture-target-blank-without-flag-from-subframe-expected.txt b/...ould-open-external-urls/user-gesture-target-blank-without-flag-from-subframe-expected.txt
@@ -3,5 +3,5 @@
  - decidePolicyForNavigationAction
 <NSURLRequest URL user-gesture-target-blank-to-notify-done.html, main document URL user-gesture-target-blank-to-notify-done-from-subframe.html, http method GET> is main frame - no should open URLs externally - no
  - decidePolicyForNavigationAction
-<NSURLRequest URL notify-done.html, main document URL user-gesture-target-blank-to-notify-done-from-subframe.html, http method GET> is main frame - yes should open URLs externally - yes
+<NSURLRequest URL notify-done.html, main document URL notify-done.html, http method GET> is main frame - yes should open URLs externally - yes
 
diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
@@ -1361,7 +1361,8 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
     if (!referrer.isEmpty())
         request.setHTTPReferrer(referrer);
 
-    updateRequestAndAddExtraFields(request, IsMainResource::Yes, newLoadType, ShouldUpdateAppInitiatedValue::Yes, FrameLoader::IsServiceWorkerNavigationLoad::No, &frameLoadRequest.requester());
+    auto willOpenInNewWindow = !targetFrame && !effectiveFrameName.isEmpty() ? WillOpenInNewWindow::Yes : WillOpenInNewWindow::No;
+    updateRequestAndAddExtraFields(request, IsMainResource::Yes, newLoadType, ShouldUpdateAppInitiatedValue::Yes, FrameLoader::IsServiceWorkerNavigationLoad::No, willOpenInNewWindow, &frameLoadRequest.requester());
 
     ASSERT(newLoadType != FrameLoadType::Same);
 
@@ -2913,7 +2914,7 @@ ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const Resour
     return ResourceRequestCachePolicy::UseProtocolCachePolicy;
 }
 
-void FrameLoader::updateRequestAndAddExtraFields(ResourceRequest& request, IsMainResource mainResource, FrameLoadType loadType, ShouldUpdateAppInitiatedValue shouldUpdate, IsServiceWorkerNavigationLoad isServiceWorkerNavigationLoad, Document* initiator)
+void FrameLoader::updateRequestAndAddExtraFields(ResourceRequest& request, IsMainResource mainResource, FrameLoadType loadType, ShouldUpdateAppInitiatedValue shouldUpdate, IsServiceWorkerNavigationLoad isServiceWorkerNavigationLoad, WillOpenInNewWindow willOpenInNewWindow, Document* initiator)
 {
     ASSERT(isServiceWorkerNavigationLoad == IsServiceWorkerNavigationLoad::No || mainResource != IsMainResource::Yes);
 
@@ -2924,7 +2925,7 @@ void FrameLoader::updateRequestAndAddExtraFields(ResourceRequest& request, IsMai
     // Don't set the cookie policy URL if it's already been set.
     // But make sure to set it on all requests regardless of protocol, as it has significance beyond the cookie policy (<rdar://problem/6616664>).
     bool isMainResource = mainResource == IsMainResource::Yes;
-    bool isMainFrameMainResource = isMainResource && m_frame.isMainFrame();
+    bool isMainFrameMainResource = isMainResource && (m_frame.isMainFrame() || willOpenInNewWindow == WillOpenInNewWindow::Yes);
     if (request.firstPartyForCookies().isEmpty()) {
         if (isMainFrameMainResource)
             request.setFirstPartyForCookies(request.url());
@@ -3080,7 +3081,11 @@ void FrameLoader::loadPostRequest(FrameLoadRequest&& request, const String& refe
     workingResourceRequest.setHTTPMethod("POST"_s);
     workingResourceRequest.setHTTPBody(inRequest.httpBody());
     workingResourceRequest.setHTTPContentType(contentType);
-    updateRequestAndAddExtraFields(workingResourceRequest, IsMainResource::Yes, loadType, ShouldUpdateAppInitiatedValue::Yes, FrameLoader::IsServiceWorkerNavigationLoad::No, &request.requester());
+
+    auto* targetFrame = formState || frameName.isEmpty() ? nullptr : findFrameForNavigation(frameName);
+
+    auto willOpenInNewWindow = !frameName.isEmpty() && !targetFrame ? WillOpenInNewWindow::Yes : WillOpenInNewWindow::No;
+    updateRequestAndAddExtraFields(workingResourceRequest, IsMainResource::Yes, loadType, ShouldUpdateAppInitiatedValue::Yes, FrameLoader::IsServiceWorkerNavigationLoad::No, willOpenInNewWindow, &request.requester());
 
     if (Document* document = m_frame.document())
         document->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(workingResourceRequest, ContentSecurityPolicy::InsecureRequestType::Load);
@@ -3091,7 +3096,7 @@ void FrameLoader::loadPostRequest(FrameLoadRequest&& request, const String& refe
 
     if (!frameName.isEmpty()) {
         // The search for a target frame is done earlier in the case of form submission.
-        if (auto* targetFrame = formState ? nullptr : findFrameForNavigation(frameName)) {
+        if (targetFrame) {
             targetFrame->loader().loadWithNavigationAction(workingResourceRequest, WTFMove(action), loadType, WTFMove(formState), allowNavigationToInvalidURL, request.shouldTreatAsContinuingLoad(), WTFMove(completionHandler));
             return;
         }

diff --git a/Source/WebCore/loader/FrameLoader.h b/Source/WebCore/loader/FrameLoader.h
@@ -318,8 +318,9 @@ class FrameLoader final {
     bool alwaysAllowLocalWebarchive() const { return m_alwaysAllowLocalWebarchive; }
 
     enum class IsServiceWorkerNavigationLoad : bool { No, Yes };
+    enum class WillOpenInNewWindow : bool { No, Yes };
     // For subresource requests the FrameLoadType parameter has no effect and can be skipped.
-    void updateRequestAndAddExtraFields(ResourceRequest&, IsMainResource, FrameLoadType = FrameLoadType::Standard, ShouldUpdateAppInitiatedValue = ShouldUpdateAppInitiatedValue::Yes, IsServiceWorkerNavigationLoad = IsServiceWorkerNavigationLoad::No, Document* = nullptr);
+    void updateRequestAndAddExtraFields(ResourceRequest&, IsMainResource, FrameLoadType = FrameLoadType::Standard, ShouldUpdateAppInitiatedValue = ShouldUpdateAppInitiatedValue::Yes, IsServiceWorkerNavigationLoad = IsServiceWorkerNavigationLoad::No, WillOpenInNewWindow = WillOpenInNewWindow::No, Document* = nullptr);
 
     void scheduleRefreshIfNeeded(Document&, const String& content, IsMetaRefresh);