Bug 204109 – REGRESSION (iOS 13.3): WKWebView does not include cookies in cross-origin images

• Component: New Bugs
• Status: RESOLVED FIXED

[Niklas Merz]

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

[Radar WebKit Bug Importer]

<rdar://problem/57120772>

[Alexey Proskuryakov]

Is the server up and running now? We are getting question marks for the images.

[Niklas Merz]

Yes that is the point. You need a cookie with a Value test. The Native App from the attachment Sets the cookie.

[Niklas Merz]

(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?

[John Wilander]

Created attachment 383705 [details]
Proposed patch

Patch for review. Note that it is branch-specific and won't apply to trunk.

[Alexey Proskuryakov]

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.

[Chris Dumez]

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()?

[John Wilander]

(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.

[John Wilander]

(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?

[Chris Dumez]

(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).

[John Wilander]

(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.

[John Wilander]

Created attachment 383708 [details]
Proposed patch addressing Chris's comments

[John Wilander]

Created attachment 383709 [details]
Proposed patch addressing Chris's comments

[Chris Dumez]

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);

[John Wilander]

(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!

[John Wilander]

(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?

[Chris Dumez]

(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?

[John Wilander]

(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.

[John Wilander]

I will keep this bug open until we have a revision for the branch patch landing.

[Brent Fulgham]

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?)

[John Wilander]

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.

[David M]

John, would you know if this will make it into 13.3 before GA?  Thanks for the fix!

[Brent Fulgham]

(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.

[David M]

Understood, thanks for the info and the help!  Much appreciated!

[Niklas Merz]

I just did a quick test with iOS 13.3 Beta 4 and it look like the issue is resolved!

[David M]

Agreed, thanks to those that fixed this!

[David M]

FYI it appears as though this issue exists in the iOS 13.3 Simulator on the Mac with Xcode 11.3 (11C29)

[John Wilander]

*** Bug 206295 has been marked as a duplicate of this bug. ***

[Niklas Merz]

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.

[David Kilzer (:ddkilzer)]

(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!

[David Kilzer (:ddkilzer)]

(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