SameSite=Lax cookie attribute not correctly handled in Safari 14.0.2 < version <= current

[cdumez]

ac5c740

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

[darinadler]

I can’t follow the logic here. Is this check correct? Why do we have to check both frame name and target frame?

[cdumez]

Yes, I believe this check is correct. If you look at the code below, we will call policyChecker().checkNewWindowPolicy() in the case where we have a (target) frameName but no actual Frame object for it.

For example, if you do target='foo', we'll first try and find a target Frame within the process and use it if we find one. However, if we cannot find such a Frame, we'll do the navigation in a new window/Frame, whose name will be "foo".

Also see the check at this line in the other function:
https://github.com/WebKit/WebKit/pull/2236/files#diff-d607936f085c93e3e1113e2c988f664830ab21dbe8351cfc08744a8b5112d473R1381

Same condition for calling checkNewWindowPolicy().

[webkit-commit-queue]

Committed 252341@main (ac5c740): https://commits.webkit.org/252341@main

Reviewed commits have been landed. Closing PR #2236 and removing active labels.