Created attachment 383348 [details] Example project with test page We found a bug in our app (using WKWebView) that images that are loaded from another origin don't load because the necessary cookie is not sent. I added a small sample project which opens a test page in WKWebview. This page has two img tags. One of the loads the image from the same origin, one from another one. Both images need a cookie, otherwhise an error is thrown. The cookies for thesse images are set in Swift code before opening the WebView. If I run this project with two devices and iOS version, I get different result: * iOS 13.2: Both images are displayed at app launch * iOS 13.3 Developer Beta 2: Only the same origin image is displayed Bug 200857 was a similiar issue in iOS 13, but is fixed since iOS 13.2
<rdar://problem/57120772>
Is the server up and running now? We are getting question marks for the images.
Yes that is the point. You need a cookie with a Value test. The Native App from the attachment Sets the cookie.
(In reply to Niklas Merz from comment #3) > Yes that is the point. You need a cookie with a Value test. The Native App > from the attachment Sets the cookie. If you open the page in a browser there will be no images. Setting a cookie like corstest=tes will make the image accessible. If you open the test app on iOS 13.3 there will be one image from the same origin, on iOS 13.2 two images from both origins. The test page is available on Github. I did set a check on cookies for the images via a serverless function for the hosting: https://github.com/NiklasMerz/corstest Can you reproduce this issue?
Created attachment 383705 [details] Proposed patch Patch for review. Note that it is branch-specific and won't apply to trunk.
Comment on attachment 383705 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=383705&action=review > LayoutTests/ChangeLog:9 > + * http/tests/resourceLoadStatistics/no-third-party-cookie-blocking-when-itp-is-off-expected.txt: Added. Test pieces will need to be landed on trunk too.
Comment on attachment 383705 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=383705&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:208 > + || (session.isResourceLoadStatisticsEnabled() && session.networkStorageSession() && session.networkStorageSession()->shouldBlockCookies(request, frameID, pageID)); There are other call sites that call shouldBlockCookies() without checking if ITP is enabled. Why are those OK? Why isn't the isITPEnabled check inside shouldBlockCookies()?
(In reply to Alexey Proskuryakov from comment #6) > Comment on attachment 383705 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383705&action=review > > > LayoutTests/ChangeLog:9 > > + * http/tests/resourceLoadStatistics/no-third-party-cookie-blocking-when-itp-is-off-expected.txt: Added. > > Test pieces will need to be landed on trunk too. Absolutely.
(In reply to Chris Dumez from comment #7) > Comment on attachment 383705 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383705&action=review > > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:208 > > + || (session.isResourceLoadStatisticsEnabled() && session.networkStorageSession() && session.networkStorageSession()->shouldBlockCookies(request, frameID, pageID)); > > There are other call sites that call shouldBlockCookies() without checking > if ITP is enabled. Why are those OK? I'll have a look. > Why isn't the isITPEnabled check inside shouldBlockCookies()? It's the NetworkSession that knows. But maybe I can forward that info to the NetworkStorageSession. But that increases the risk of the change, right?
(In reply to John Wilander from comment #9) > (In reply to Chris Dumez from comment #7) > > Comment on attachment 383705 [details] > > Proposed patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=383705&action=review > > > > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:208 > > > + || (session.isResourceLoadStatisticsEnabled() && session.networkStorageSession() && session.networkStorageSession()->shouldBlockCookies(request, frameID, pageID)); > > > > There are other call sites that call shouldBlockCookies() without checking > > if ITP is enabled. Why are those OK? > > I'll have a look. > > > Why isn't the isITPEnabled check inside shouldBlockCookies()? > > It's the NetworkSession that knows. But maybe I can forward that info to the > NetworkStorageSession. But that increases the risk of the change, right? Hmm, I see what you mean, NetworkStorageSession is actually in WebCore and does not have access to the NetworkSession and does not know about the NetworkSession. My alternate proposal then then would be to rename the method name to be less generic (e.g. shouldBlockCookieDueToITP()) and make sure all call sites check that ITP is enabled before calling. This is assuming this function is only used for ITP purposes (I don't actually know that for sure).
(In reply to Chris Dumez from comment #10) > (In reply to John Wilander from comment #9) > > (In reply to Chris Dumez from comment #7) > > > Comment on attachment 383705 [details] > > > Proposed patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=383705&action=review > > > > > > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:208 > > > > + || (session.isResourceLoadStatisticsEnabled() && session.networkStorageSession() && session.networkStorageSession()->shouldBlockCookies(request, frameID, pageID)); > > > > > > There are other call sites that call shouldBlockCookies() without checking > > > if ITP is enabled. Why are those OK? > > > > I'll have a look. > > > > > Why isn't the isITPEnabled check inside shouldBlockCookies()? > > > > It's the NetworkSession that knows. But maybe I can forward that info to the > > NetworkStorageSession. But that increases the risk of the change, right? > > Hmm, I see what you mean, NetworkStorageSession is actually in WebCore and > does not have access to the NetworkSession and does not know about the > NetworkSession. > > My alternate proposal then then would be to rename the method name to be > less generic (e.g. shouldBlockCookieDueToITP()) and make sure all call sites > check that ITP is enabled before calling. This is assuming this function is > only used for ITP purposes (I don't actually know that for sure). Your overall comment was valid - thank you for that! I have another approach. I will forward the on/off status of ITP to NetworkStorageSession and check it in the shouldBlockCookies() functions. This increases the patch's complexity slightly but not in a material way since it's just about forwarding a boolean flag and the infrastructure for the ITP setting is already there. New patch coming.
Created attachment 383708 [details] Proposed patch addressing Chris's comments
Created attachment 383709 [details] Proposed patch addressing Chris's comments
Comment on attachment 383709 [details] Proposed patch addressing Chris's comments View in context: https://bugs.webkit.org/attachment.cgi?id=383709&action=review > Source/WebKit/NetworkProcess/NetworkSession.cpp:134 > + if (auto* storageSession = networkStorageSession()) Could this be move to the top of the method to avoid the duplication? ASSERT(!m_isInvalidated); if (auto* storageSession = networkStorageSession()) storageSession->setResourceLoadStatisticsEnabled(enable);
(In reply to Chris Dumez from comment #14) > Comment on attachment 383709 [details] > Proposed patch addressing Chris's comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383709&action=review > > > Source/WebKit/NetworkProcess/NetworkSession.cpp:134 > > + if (auto* storageSession = networkStorageSession()) > > Could this be move to the top of the method to avoid the duplication? > > ASSERT(!m_isInvalidated); > if (auto* storageSession = networkStorageSession()) > storageSession->setResourceLoadStatisticsEnabled(enable); Yes. Will fix before sending to branch integrators. Thanks for the review!
(In reply to John Wilander from comment #15) > (In reply to Chris Dumez from comment #14) > > Comment on attachment 383709 [details] > > Proposed patch addressing Chris's comments > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=383709&action=review > > > > > Source/WebKit/NetworkProcess/NetworkSession.cpp:134 > > > + if (auto* storageSession = networkStorageSession()) > > > > Could this be move to the top of the method to avoid the duplication? > > > > ASSERT(!m_isInvalidated); > > if (auto* storageSession = networkStorageSession()) > > storageSession->setResourceLoadStatisticsEnabled(enable); > > Yes. Will fix before sending to branch integrators. Thanks for the review! Or wait, now I remember why I had it in two places - to guarantee that the ITP object has been created before I tell the storage session that ITP is on. Do you think that'll cause an issue with your proposed change?
(In reply to John Wilander from comment #16) > (In reply to John Wilander from comment #15) > > (In reply to Chris Dumez from comment #14) > > > Comment on attachment 383709 [details] > > > Proposed patch addressing Chris's comments > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=383709&action=review > > > > > > > Source/WebKit/NetworkProcess/NetworkSession.cpp:134 > > > > + if (auto* storageSession = networkStorageSession()) > > > > > > Could this be move to the top of the method to avoid the duplication? > > > > > > ASSERT(!m_isInvalidated); > > > if (auto* storageSession = networkStorageSession()) > > > storageSession->setResourceLoadStatisticsEnabled(enable); > > > > Yes. Will fix before sending to branch integrators. Thanks for the review! > > Or wait, now I remember why I had it in two places - to guarantee that the > ITP object has been created before I tell the storage session that ITP is > on. Do you think that'll cause an issue with your proposed change? It is just setting a Boolean, the setter does not rely on the ITP object in any way (and cannot really since they live at different layers), so I think it would be fine. Why did you have concerns?
(In reply to Chris Dumez from comment #17) > (In reply to John Wilander from comment #16) > > (In reply to John Wilander from comment #15) > > > (In reply to Chris Dumez from comment #14) > > > > Comment on attachment 383709 [details] > > > > Proposed patch addressing Chris's comments > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=383709&action=review > > > > > > > > > Source/WebKit/NetworkProcess/NetworkSession.cpp:134 > > > > > + if (auto* storageSession = networkStorageSession()) > > > > > > > > Could this be move to the top of the method to avoid the duplication? > > > > > > > > ASSERT(!m_isInvalidated); > > > > if (auto* storageSession = networkStorageSession()) > > > > storageSession->setResourceLoadStatisticsEnabled(enable); > > > > > > Yes. Will fix before sending to branch integrators. Thanks for the review! > > > > Or wait, now I remember why I had it in two places - to guarantee that the > > ITP object has been created before I tell the storage session that ITP is > > on. Do you think that'll cause an issue with your proposed change? > > It is just setting a Boolean, the setter does not rely on the ITP object in > any way (and cannot really since they live at different layers), so I think > it would be fine. Why did you have concerns? My concern was that the storage session would immediately start acting as if ITP is on.
I will keep this bug open until we have a revision for the branch patch landing.
It looks like the patch was landed on the branch here: https://trac.webkit.org/r252549. Are you planning on landing this on Trunk (or creating a trunk-compatible patch?)
Resolved with https://trac.webkit.org/changeset/252549/webkit, as pointed out by Brent. I will land a fix in trunk in another Bugzilla bug.
John, would you know if this will make it into 13.3 before GA? Thanks for the fix!
(In reply to David M from comment #22) > John, would you know if this will make it into 13.3 before GA? Thanks for > the fix! We cannot comment on when a change will get into a production release. However, we recognize the importance of the change and view this as a regression from 13.2, so are treating it with high priority.
Understood, thanks for the info and the help! Much appreciated!
I just did a quick test with iOS 13.3 Beta 4 and it look like the issue is resolved!
Agreed, thanks to those that fixed this!
FYI it appears as though this issue exists in the iOS 13.3 Simulator on the Mac with Xcode 11.3 (11C29)
*** Bug 206295 has been marked as a duplicate of this bug. ***
I just tested this with iOS 14 developer beta. This is broken again. If I run the attached test project with Xcode 12 beta on an iPhone with iOS 14 I dpn't see the cors image. The latest iOS 13 still works as expected.
(In reply to Niklas Merz from comment #29) > I just tested this with iOS 14 developer beta. This is broken again. > > If I run the attached test project with Xcode 12 beta on an iPhone with iOS > 14 I dpn't see the cors image. The latest iOS 13 still works as expected. Please file a new bug and relate this one!
(In reply to John Wilander from comment #21) > Resolved with https://trac.webkit.org/changeset/252549/webkit, as pointed > out by Brent. I will land a fix in trunk in another Bugzilla bug. Was this fix ever ported to trunk, or did we forget to do that? (In reply to David Kilzer (:ddkilzer) from comment #30) > (In reply to Niklas Merz from comment #29) > > I just tested this with iOS 14 developer beta. This is broken again. > > > > If I run the attached test project with Xcode 12 beta on an iPhone with iOS > > 14 I dpn't see the cors image. The latest iOS 13 still works as expected. > > Please file a new bug and relate this one! Bug 213510: REGRESSION (iOS 14): WKWebView does not include cookies in cross-origin images