-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iOS 18 does not allow a client application to specify SameSite=None #33164
iOS 18 does not allow a client application to specify SameSite=None #33164
Conversation
EWS run on previous version of this PR (hash 65bcd8e) |
finished = true; | ||
}]; | ||
}]; | ||
TestWebKitAPI::Util::run(&finished); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you call clearCookies again after this test to leave no state behind?
Also, is it worth adding the same test with a non-persistent data store?
Also, could we test the actual functionality of a same-site:none cookie and see that it is sent correctly over the network?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you call clearCookies again after this test to leave no state behind?
Yes.
Also, is it worth adding the same test with a non-persistent data store?
Sure.
Also, could we test the actual functionality of a same-site:none cookie and see that it is sent correctly over the network?
Can you clarify what you mean by this? Do you want me to do that in this test? There are several existing layout tests that verify SameSite=None cookies are sent correctly over the network.
65bcd8e
to
d9ddccf
Compare
EWS run on previous version of this PR (hash d9ddccf) |
d9ddccf
to
596ac99
Compare
EWS run on previous version of this PR (hash 596ac99)
|
596ac99
to
49080f0
Compare
EWS run on previous version of this PR (hash 49080f0) |
1b5edda
to
8f84319
Compare
Thanks all for looking into this! To ensure our teams and client applications are ready for this change ahead of the release, could I get some clarification on:
let cookieProperties: [HTTPCookiePropertyKey: Any] = [
.domain: "auth.organization.com",
.path: "/",
.name: "sampleCookie",
.value: "sampleValue",
.secure: "TRUE",
.sameSitePolicy: "none",
]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is sufficient.
8f84319
to
232cadd
Compare
EWS run on current version of this PR (hash 232cadd) |
https://bugs.webkit.org/show_bug.cgi?id=279153 rdar://135312438 Reviewed by Pascoe and Alex Christensen. In the iOS 18 and macOS Sequoia betas, CFNetwork began treating cookies as SameSite=Lax by default. This caused an issue where, when WebKit converts from its internal enum `Cookie::SameSitePolicy` to `NSHTTPCookieStringPolicy`, we return nil for `SameSitePolicy::None`, which CFNetwork now interprets as lax instead of none. WebKit should explicitly return none when converting `WebCore::Cookie` to an `NSHTTPCookie`. We should use a string constant for none, but one is not currently available in the SDK. * Source/WTF/wtf/PlatformHave.h: * Source/WebCore/platform/network/cocoa/CookieCocoa.mm: (WebCore::coreSameSitePolicy): (WebCore::nsSameSitePolicy): * Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm: (TEST(WKHTTPCookieStore, SetSameSiteCookiePolicies)): (TEST(WKHTTPCookieStore, SetSameSiteCookiePoliciesNonPersistentStore)): Canonical link: https://commits.webkit.org/283230@main
232cadd
to
45ce92c
Compare
Committed 283230@main (45ce92c): https://commits.webkit.org/283230@main Reviewed commits have been landed. Closing PR #33164 and removing active labels. |
45ce92c
232cadd
🛠 tv🛠 watch